| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-26 | |||
| 17:06:22 | bauzas | anyway, I'm running out of fuel for my brain | |
| 17:06:49 | sean-k-mooney | bauzas: i think we would need to pass in the allocation candiates to the filter yes and then pass that down into the fit_instance_to_host fucntion or whatever it is called so that it could make a descission based on the allcoation candiate | |
| 17:07:10 | bauzas | right, that's what I meant | |
| 17:07:41 | sean-k-mooney | ya that fuction is a pain to modify or debug but its going to need to be scoped to the allocation candiate to work correct when numa is in placement | |
| 17:09:54 | jaypipes | bauzas, stephenfin, sean-k-mooney, efried: sorry, done with call now. | |
| 17:10:13 | bauzas | jaypipes: I'm just rat-holing | |
| 17:11:44 | bauzas | my concern is, how to make sure we can still have all the NUMA features be workable in a world with nested RPs albeit all things solved in the future++ with placement resources | |
| 17:12:00 | sean-k-mooney | jaypipes: the issue is basically how to correlate placement RPs with the compute node resouce tracker so that when the the numa topology filter or pining code runs we only look at the resouce selected by placement in the allocation candidate and not all numa nodes for example. | |
| 17:20:22 | jaypipes | efried: iota? | |
| 17:33:24 | jaypipes | sean-k-mooney: I don't think it's really a big issue. Basically, let the NUMA topology filter just run as-is. It will "pick" a NUMA node to pin the instance to (and then promptly forget about its pick). The scheduler will claim resources against one of the NUMA nodes on the host (via the normal allocation request claim_resources() process). The build request gets to the compute host. During the instance_claim() process, the numa_fit_instance_to_ | |
| 17:33:24 | jaypipes | host() is run again. If that picks a different NUMA node than what is in the allocation_request that is sent along with the build request, then we raise an exception and just retry the scheduling. | |
| 17:35:11 | dansmith | efried: so, questions about L371 here: https://review.openstack.org/#/c/547990/10/nova/scheduler/client/report.py | |
| 17:35:16 | dansmith | efried: what 406 are you talking about? | |
| 17:35:38 | dansmith | the only one I know of is if the version isn't supported that we need for member_of | |
| 17:36:10 | dansmith | efried: and, I'm only running the intersection and setting of the member_of if aggregates is non-empty, which is what you're saying I'll need to do | |
| 17:36:15 | sean-k-mooney | jaypipes: thats one option be se should really not have to retry here | |
| 17:36:32 | sean-k-mooney | jaypipes: we should be able to just look at the cell that was selected by placement | |
| 17:38:00 | sean-k-mooney | also when numa_fit_instance_to_host runs how to you tell if it picked a different node to the one in the allocation_request if you cant correlate between them | |
| 17:39:44 | efried | dansmith: The functionality you want is to be able to do the set logic on aggregates. We plan to allow placement to do that via some new syntax (your pending spec delta). Once that happens, there'll be a microversion for that. And at that time, if you try to use that new microversion, you'll also have to handle the case where placement is downlevel, just like we do everywhere else where we might be straddling versions. | |
| 17:40:25 | edleafe | efried: is there a spec/bp/bug for adding the consumer generation? | |
| 17:40:40 | efried | dansmith: And what I'm saying is that the 406 branch will have to do the placement query without taking member_of into account (or using the intersection thing as a prefilter) and then do the set logic on the candidates that come back for that too-broad query. | |
| 17:40:53 | dansmith | efried: why? the last couple times we've bumped that version we have't supported an older placement | |
| 17:41:04 | efried | Whoah. Yes, we do, every time. | |
| 17:41:11 | dansmith | efried: we've been saying placement goes first in the upgrade stack | |
| 17:41:18 | efried | edleafe: Not as of the last time I checked, which I think was yesterday. | |
| 17:41:35 | dansmith | efried: where? that code has changed several times while I was working on this and we don't have any fallback code there | |
| 17:42:05 | efried | dansmith: I think we say that as a best practice or something, but we've got somewhere else that defines the minimum placement microversion for a given release and it's always lower than the maximum for same release. mriedem help me out here. | |
| 17:42:11 | jaypipes | dansmith: example? the code has changed in a backwards *incompatible* way and we don't have fallback code? | |
| 17:42:16 | efried | dansmith: Just look for 406 in report.py | |
| 17:42:37 | efried | cdent culled the stale ones at the end of Queens, so there aren't many left, but still some. | |
| 17:42:51 | dansmith | jaypipes: the last couple of times this exact method has changed | |
| 17:43:07 | sean-k-mooney | efried: the grenagde job in the gate is proably upgradeing placement first so even if it could work its proably not tested today | |
| 17:43:13 | dansmith | I can go actually dig up reviews if you care | |
| 17:43:30 | edleafe | efried: yeah, I didn't see anything either. So would you call this a bug fix or a new feature? | |
| 17:43:34 | dansmith | efried: why are we using different initial versions for any of those calls then? | |
| 17:43:53 | efried | edleafe: needs a bp fo sho | |
| 17:44:38 | edleafe | efried: ok. Is there any discussion I can refer to? I'm not clear on the reasons for doing this | |
| 17:44:55 | efried | dansmith: Because the newer way is usually more efficient or similar; and it gives us an easier delta when we do bump the min | |
| 17:44:57 | cdent | edleafe: needs a spec, is a new feature and an api change | |
| 17:45:09 | dansmith | efried: jaypipes this one is the one I have in my head right now: https://review.openstack.org/#/c/536085/9/nova/scheduler/client/report.py | |
| 17:45:26 | cdent | edleafe: if you want to chat about a bit later this evening I can do a bit of a brain dump on you if you want | |
| 17:45:36 | cdent | but in the midst of something at the moment | |
| 17:45:45 | edleafe | cdent: you mean you haven't dumped it in an email yet? :) | |
| 17:46:14 | dansmith | efried: the problem is that if placement doesn't go first, scheduler depends on it for az calculation, then we have to fail any request that cares about az if placement doesn't support 1.21 _anyway_ | |
| 17:46:23 | cdent | no, because I was trying not to own this one. I did have a to do list item which said "write email about consumer uuid" but deleted it when you "got ownershipw of this" :) | |
| 17:47:10 | edleafe | cdent: ah, I was just volunteering my cycles to code it. If I gotta write a spec, I should at least understand the motivation behind it. | |
| 17:47:22 | edleafe | (that usually helps) :) | |
| 17:47:36 | efried | edleafe: I got you, gimme sec | |
| 17:47:48 | cdent | edleafe: details! but yeah, I can dump some brain shortly if efried doesn't beat me to it | |
| 17:47:48 | jaypipes | dansmith: you will note I did not review that. | |
| 17:49:00 | dansmith | jaypipes: so cdent was culling all the microversions when, at the end of each release? | |
| 17:49:07 | efried | dansmith, jaypipes: Agree that one should have had the 406-fallback conditions. I shoulda reviewed it too, but didn't. IIRC that got jammed in really late in Q because we weren't going to make granular happen in time. | |
| 17:49:16 | dansmith | jaypipes: so you want a bunch of microversion handling code for within-release changes? | |
| 17:49:17 | cdent | jaypipes, dansmith: we've been inconsistent about our expectation with nova-side microversion fallback. I've not been able to discern a pattern, mostly because _some_ of the time we say "we expect placement to upgrade first" at which point why bother falling back? | |
| 17:49:34 | cdent | I didn't cull them all, just anything less than 1.14 (I think it was 1.14, would need to check to be sure) | |
| 17:49:48 | dansmith | cdent: yeah I see some places where we do fallback, and some where we don't | |
| 17:49:49 | dansmith | cdent: it was 1.14 | |
| 17:50:06 | jaypipes | dansmith: I'm just noting I did not review that patch (didn't know about it at all). I believe I was in Dallas that week. | |
| 17:50:46 | jaypipes | dansmith: I do support the reportclient supporting fallback code (406 handling) for at least a release, yes. | |
| 17:50:47 | dansmith | so what do you people propose? scheduler fails if you request an az and placement is old? fall back but make sure the old filter is enabled otherwise fail? | |
| 17:51:08 | jaypipes | dansmith: yes, that sounds reasonable to me. | |
| 17:51:11 | dansmith | jaypipes: we're not testing nova against an older placement, so it basically doesn't work | |
| 17:51:17 | dansmith | jaypipes: seriously? that was a joke :) | |
| 17:51:46 | dansmith | well, the second part was a joke | |
| 17:51:52 | dansmith | maybe you mean the first part? | |
| 17:51:56 | dansmith | but that doesn't seem very friendly to me | |
| 17:51:58 | jaypipes | dansmith: well, the "make sure the old filter is enabled" was a joke, right? | |
| 17:52:05 | dansmith | yes | |
| 17:52:07 | sean-k-mooney | jaypipes: as other service start using placement it will likely make sense to have placement upgrade early in the sequence. is there a reason you do not want to commit to placement before nova in general? | |
| 17:52:12 | jaypipes | dansmith: the "fail if placement doesn't support the az filter" wasn't. | |
| 17:52:58 | dansmith | jaypipes: I think if we're adding code for all those branches we need a grenade test where we leave placement behind on queens and run master against it | |
| 17:53:00 | efried | dansmith: I think what I'm saying is that nova can support the AZ filter even without support for GET /a_c?member_of= | |
| 17:53:00 | dansmith | like we did for nova-compute | |
| 17:53:03 | dansmith | otherwise it's broken | |
| 17:53:18 | efried | dansmith: And if we need to support N-1 release of placement in nova, we've gotta write that code "the hard way". | |
| 17:53:20 | dansmith | efried: it can if you leave the post-processing filter enabled | |
| 17:53:29 | mriedem | efried: nova-status has a min placement API version check | |
| 17:53:29 | jaypipes | sean-k-mooney: if the change in the placement API is backwards compatible (which a *new* query param is), I don't see a reason to fixate an upgrade to always do placement first. | |
| 17:53:32 | dansmith | efried: yeah, I'm asserting we do not | |
| 17:53:59 | efried | mriedem: So the discussion here is: why isn't that minimum version always the same as the max-in-release? | |
| 17:54:23 | mriedem | because the nova side code might not be using the max yet | |
| 17:54:38 | dansmith | mriedem: in most cases it's not | |
| 17:54:41 | efried | mriedem: Under what circumstances? Haven't we always said you have to upgrade placement first? | |
| 17:54:42 | mriedem | generally it likely is because we added the new placement api versions for things nova needs | |
| 17:54:48 | sean-k-mooney | jaypipes: that is true but it appears we are not quite to that point yet. as dan said we likely need a gate job that leaves placement on N-1 to validate it works if that is the guareentee we want to give | |
| 17:55:09 | dansmith | mriedem: that's why I'm saying we might as well use the same default microversion in the whole file if we're going to require this, which i think defeats a lot of the point of microversioning this at all | |
| 17:55:27 | mriedem | see first bullet in #2 here https://docs.openstack.org/nova/latest/user/upgrade.html#rolling-upgrade-process | |
| 17:55:46 | mriedem | i haven't been paying attention to this conversation so don't have context | |
| 17:56:49 | dansmith | so by the same argument, | |
| 17:56:54 | dansmith | if a flavor has any required trait in it, | |
| 17:56:57 | sean-k-mooney | mriedem: that likely should be expanded to "before and openstack service that uses placement" rather then nova but ya. | |
| 17:57:03 | dansmith | we must fail if placement doesn't support 1.17 for that rev I linked above | |
| 17:57:13 | mriedem | sean-k-mooney: these are nova upgrade docs... | |
| 17:57:17 | dansmith | sean-k-mooney: those are nova upgrade notes | |
| 17:57:18 | dansmith | heh | |
| 17:57:21 | efried | dansmith: Or write a post-filter. Yeah. | |
| 17:57:31 | sean-k-mooney | hehe ok fair point :) | |
| 17:57:45 | dansmith | efried: we can't do that, AFAIK | |
| 17:57:53 | mriedem | dansmith: i thought we did fail if a flavor has required traits in it but 1.17 isn't available? | |
| 17:57:55 | dansmith | efried: filters don't have access to the actual a-c I don't think | |