Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-01-27
15:07:34 superdan I haven't looked at the two fixes
15:07:47 cdent as the way it works now is that it doesn't at all do what it says it is going to do
15:07:48 superdan I'll try to do that and make a judgment
15:08:22 cdent thanks, that would be awesome, just doing that analysis felt like I had a significant leap forward on understanding the resource tracker, but then right at the end I felt like I fell way backwards :(
15:08:47 cdent in the "why does this even work" sense
15:08:47 superdan well, I'm not a RT expert by any means
15:08:49 superdan probably jay would be better, but..
15:09:08 johnthetubaguy figleaf: cdent: this ironic resource provider change, I could be missing something again, but I feel like we are missing something around updating new instance allocations? well possibly anyways.
15:09:59 openstackgerrit Stephen Finucane proposed openstack/nova: Fix backwards compatibility for InstanceNUMACell https://review.openstack.org/396184
15:09:59 openstackgerrit Stephen Finucane proposed openstack/nova: Combine obj_make_compatible tests https://review.openstack.org/426261
15:10:08 cdent johnthetubaguy: figleaf has gone to the docs, I think, and knows more about that than me, but I'll listen...
15:10:22 mriedem johnthetubaguy: replied in https://review.openstack.org/#/c/404472/ -
15:10:33 mriedem johnthetubaguy: none of that fancy new code is hit unless node.resource_class is set
15:10:38 mriedem which it's not today in our CI
15:10:41 mriedem which kind of blows
15:10:55 mriedem if you look at the logs for the ironic job on that patch it's still setting ram/cpu/disk
15:11:12 mriedem would be nice if we could have a one-off DNM patch to set node.resource_class and test that stuff
15:11:14 mriedem jroll: ^
15:11:39 mriedem johnthetubaguy: and i think the ironic-specific stuff is mostly contained in the RT
15:11:55 mriedem which is why in earlier patches i had asked that long-term we subclass the ResourceTracker to do the ironic-specific bits
15:12:13 mriedem because i don't like the "if ironic" spaghetti code
15:12:28 mriedem i'll review the latest on that patch when i get back in an hour or so
15:13:56 cdent I may be misremembering, but I thought part of the rt+resosurce class stack was to change the resource tracker so that there is only one of them (for everything it tracks), not one for each node it cares about?
15:14:08 cdent that might be somewhere else...
15:14:37 melwitt cdent: fwiw the tracked_instances clear every update_available_resource period is known and intentional. it surprised me too when I first realized it. I don't recall why it's that way, I guess to prevent stale things from hanging around potentially
15:14:55 cdent melwitt: I took it out and no tests failed...
15:15:01 cdent which made me sad and confused
15:15:10 mriedem cdent: that's the same
15:15:14 mriedem but not what i'm talking about
15:15:17 mriedem bbiab
15:16:10 melwitt yeah, I don't think tests would fail from it. I have mentioned it to jaypipes in the past and he knew about it
15:17:24 cdent melwitt: hmm. well, thanks for providing some background. I wonder if it is worth revisiting since now it is spinning off redudant http requests?
15:17:32 melwitt bauzas: do you know anything about why tracked_instances is cleared each update_available_resource period?
15:19:45 johnthetubaguy cdent: so the bit I am stuck with, is where we make the new style ironic resource_class allocations
15:20:22 melwitt cdent: I think in general it's worth considering since it seemed excessive even in its original pre placement api form. just don't know what's involved in doing that or what gotchas there are
15:21:37 jroll mriedem: I'm working on devstack support right now, do you want a throwaway hack in the meantime?
15:21:51 superdan melwitt: cdent it's just to try to always stab the latest info into the database in case it's stale, right?
15:22:15 melwitt that's what I'm guessing
15:22:50 cdent if the node is authoritative for itself, and it keeps a cache of what it successfully wrote last time, how does staleness happen?
15:23:03 cdent (where last time could be 60s or 60 days ago)
15:23:24 cdent (mostly thinking about inventory here, not allocations)
15:23:48 cdent johnthetubaguy: i'm rereading the code, to bring it back into my brain
15:26:09 cfriesen cdent: refreshing tracked_instances would allow it to recover from errors where it missed an event that would cause the instance to be added/removed from the list. But it should be possible to handle that in a different way.
15:26:25 johnthetubaguy cdent: you probably want to ignore any changes to the "service" field in the compute_node as well, as service will keep getting updated every DB service update interval, unsure if thats related (re https://review.openstack.org/#/c/424305)
15:27:47 johnthetubaguy cdent: ignore that
15:29:23 melwitt cdent: looking back at the code to refresh my memory ... I think without the periodic clear, the only thing that removes instances from tracked_instances is the vm_state DELETED or SHELVED_OFFLOADED and if for some reason (errors) an instance doesn't get its state saved that way, it would never be removed
15:29:23 cdent cfriesen: is missing events of that nature a thing happens a) often, b) sometimes, c) rarely?
15:32:29 cfriesen cdent: I would expect rarely
15:33:54 cfriesen melwitt: abort_instance_claim() also removes it, by calling _update_usage_from_instance(is_removed=True)
15:34:10 melwitt yeah, sorry. I forgot to mention that too
15:34:12 cfriesen melwitt: cdent: but I think melwitt is correct that there would be cases we'd need to add explicit removal
15:34:57 cdent at least over the long run, making things more explicit would probably be better
15:35:29 cfriesen cdent: agreed, and it would make the resource tracking react faster to freeing up of resources.
15:40:54 johnthetubaguy cdent: at somepoint doesn't nova-conductor create the instance resource allocations, and retries if the generation id of the resource provider doesn't match, (as generation id increments on a related allocation, or something like that)? So the resource tracker is more just checking for rogue instances with missing allocations?
15:41:34 cfriesen so hardware.numa_usage_from_instances() expects a list, but we only ever call it with a single instance. Do we plan on ever using it with a list or should we maybe clean it up to expect a single instance?
15:42:29 cdent johnthetubaguy: when you say "allocations" do you mean allocations in the placement api, or something more general? because if the former, the conductor doesn't talk to the placement api at all.
15:43:18 johnthetubaguy cdent: I mean placement API, and when I say conductor, that happens in nova-scheduler today
15:45:06 cdent johnthetubaguy: as far as I know, right now, the only thing that writes allocations is the resource tracker
15:45:19 cdent and the only thing that writes inventories is the resource tracker
15:45:28 cdent when nova-scheduler talks to placement API it is read only
15:47:42 johnthetubaguy cdent: I am thinking about the (not too distant) future, as the sooner we add the allocations, the sooner other queries "notice" them
15:48:10 johnthetubaguy but thats probably just a distraction right now
15:48:12 cdent oh you mean "at some point won't it be the cast that...".
15:48:33 cdent Yes, at some point the scheduler will request and claim resources (thus make allocations) in one go
15:48:51 cdent probably with a POST to /allocations of something resembling a request spec
15:49:09 cdent instead of the curerrent GET /resoruce_provdiers (which then passing into existing filter scheduler)
15:49:30 cdent when that time comes, the resource tracker will change quite a bit, presumably
15:49:39 cdent but I suspect that that time is probably further off than we think
15:49:44 cdent s/think/hope/
16:03:30 johnthetubaguy cdent: fair point, just wondering if we can make baby steps in that direction, while we still have the middle ground
16:06:11 cdent johnthetubaguy: I'm not super familiar with the existing claim handling, so I'm too clear on what the options are for steps
16:08:59 openstackgerrit Chris Dent proposed openstack/nova: Add more debug logging on RP inventory update failures https://review.openstack.org/414230
16:09:00 openstackgerrit Chris Dent proposed openstack/nova: Add more debug logging on RP inventory delete failures https://review.openstack.org/426290
16:16:50 superdan sdague: are you back today?
16:16:57 sdague superdan: yeh
16:17:35 superdan sdague: you wanna look at the mess I've made here? https://review.openstack.org/#/c/424730/
16:17:53 superdan I think the wait for services is probably a good thing we want to avoid a race with the subnode checking back in,
16:18:03 superdan although I don't think it's related to the problem we were trying to solve
16:18:08 superdan and I figure that should be pulled out
16:18:12 superdan to a separate patch
16:18:30 superdan but, the configuring/starting of the placement stuff would be good to validate if you could
16:21:00 sdague yeh, it seems like that should be separate, and I do agree, based on how long all these phases take, I seriously doubt we'll have a practical issue there
16:21:05 cdent anyone: probably too late but: CORS for placement had a +2 on it a while back and there was some support for getting it in as part of ocata (probably not a huge deal if it doesn't happen): https://review.openstack.org/#/c/392891/
16:21:40 sdague superdan: I saw this patch briefly the other day, how about we bite the bullet now and make a 55_placement top level construct and do the placement service enablement and start there
16:21:43 superdan sdague: it would specifically catch the situation where we introduce a thing that broke the subnode entirely and call it out, instead of just trying to decipher it from the failed tempest run, at least
16:22:18 sdague superdan: sure, but has that shown up yet?
16:22:47 superdan sdague: meaning have we ever broken the subnode?
16:23:40 sdague superdan: in this flow right now
16:23:40 superdan sdague: at one point the service version code had a bug where the subnode couldn't check back in because its service record had timed out and it was told it couldn't check back in and regress the minimum version, yeah
16:23:55 superdan sdague: and it took a while to realize that the subnode was running, but never being sent instances
16:24:14 sdague I get the idea that it can happen, but lets not mix up problems being solved here
16:24:18 superdan sdague: unrelated to this flow, although this flow can definitely race with the subnode checking back in after the upgrade, I just haven't seen it happen yet
16:24:41 sdague we can always put a check like this back in later if it's happening at any frequency, and even backport it
16:24:57 superdan cdent: mriedem: scheduling patch just passed multinode grenade
16:25:00 superdan johnthetubaguy: ^
16:25:26 cdent amazeballs
16:25:35 superdan sdague: it just doesn't seem responsible to not check for and catch a thing we know can happen when it's trivial to do, but.. the fight is all gone from me this week
16:26:26 sdague superdan: I just want to fix one thing at a time here and get people unblocked. We can fix these other things later.
16:26:47 superdan sdague: I'm not saying that has to be in front of this necessarily
16:26:48 mriedem superdan: how is that possible

Earlier   Later