| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-03-26 | |||
| 17:47:36 | efried | edleafe: I got you, gimme sec | |
| 17:47:48 | jaypipes | dansmith: you will note I did not review that. | |
| 17:47:48 | cdent | edleafe: details! but yeah, I can dump some brain shortly if efried doesn't beat me to it | |
| 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 | dansmith | like we did for nova-compute | |
| 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: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 | 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:29 | mriedem | efried: nova-status has a min placement API version check | |
| 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 | |
| 17:58:12 | dansmith | mriedem: we do, but they are arguing that it should fall back to 1.14 | |
| 17:58:21 | efried | dansmith: I'm not talking about a Filter. Talking about code in the report client that manually does the trait filtering. | |
| 17:58:23 | dansmith | and I'm saying I don't know what we do in that case, because 1.14 isn't enough | |
| 17:58:25 | mriedem | i disagree, | |
| 17:58:37 | mriedem | if the flavor has required traits, the admin is saying, 'i require traits for this flavor so make it happen or fail' | |
| 17:58:56 | dansmith | mriedem: agreed, and same for my aggregate requirement | |
| 17:58:59 | dansmith | there's no point in falling back | |
| 17:59:26 | efried | dansmith: Except that putting a trait in a flavor is a new thing the admin would do. | |
| 17:59:30 | mriedem | https://docs.openstack.org/nova/latest/user/flavors.html has something about required traits but doesn't say we fail explicitly if 1.17 isn't available | |
| 17:59:36 | dansmith | efried: so? | |
| 17:59:46 | efried | dansmith: But AZ filtering is something we're trying to make compatible with existing setups... no? | |
| 18:00:18 | dansmith | efried: but we can't test for membership from the client side | |
| 18:00:32 | dansmith | efried: at least not providing the same semantics the user is expecting | |
| 18:00:45 | dansmith | we can approximate it by looking in nova's notion of the aggregate, but it's not the same | |
| 18:01:15 | efried | dansmith: Sure, why not? That method has received the list of aggregate UUIDs. We can glean RP UUIDs by looking at placement. We just do the filtering on the client side rather than at the placement server. | |
| 18:01:25 | efried | I mean, it's likely to be horribly inefficient. | |
| 18:01:27 | dansmith | mriedem: that text implies NoValidHost if we can't find RPs with the traits requested, which is even less obvious about why it's failing | |
| 18:01:51 | efried | mriedem: TBC, I think we agree it was a miss that this issue wasn't considered one way or another in that traits review. We're kind of post-morteming what we *should* have done there. | |
| 18:02:12 | dansmith | well, fwiw, I had been assuming what our docs say, which is placement goes first | |
| 18:02:17 | efried | s/glean RP UUIDs/glean aggregate UUIDs for those RPs/ | |
| 18:02:38 | cdent | Is there a reason we can't assume placement goes first? | |
| 18:03:05 | cdent | I mean can we decide today that's how it is going to be? | |
| 18:03:21 | mriedem | efried: eh? if placement 1.17 isn't available for required traits in a flavor, i think we should fail | |
| 18:03:21 | dansmith | or acknowledge that we already did? :) | |
| 18:03:52 | cdent | dansmith: well, sure, yeah | |
| 18:04:18 | efried | mriedem: That seems like a reasonable decision to me. I wasn't necessarily advocating for anything else, just pointing out that there are other options. | |
| 18:04:28 | dansmith | efried: BS you were too :) | |
| 18:04:28 | efried | ableit sucky ones. | |
| 18:04:48 | efried | dansmith: Well I certainly was advocating for that in the case of your patch. | |
| 18:04:49 | dansmith | <efried>Whoah. Yes, we do, every time. | |
| 18:04:52 | dansmith | ^ | |
| 18:04:56 | dansmith | heh, okay | |
| 18:05:12 | efried | dansmith: Yeah, that was before I realized we had screwed the pug on the traits one. | |
| 18:05:33 | mriedem | for the traits one, the pug needed to get laid imo | |
| 18:05:44 | mriedem | not every case is a fallback situation | |
| 18:05:53 | dansmith | right | |
| 18:05:59 | efried | I (now) agree with that. | |
| 18:06:53 | efried | But dansmith I'm still needing to be told that it won't represent a regression if we go the failure path in the agg filtering case. | |
| 18:07:22 | efried | Like, does an existing flavor in an existing cloud quit working because we made this path fail (if they don't have recent placement)? | |
| 18:07:32 | dansmith | az is per user request | |
| 18:07:41 | dansmith | nothing to do with flavor | |
| 18:07:43 | dansmith | in this case | |
| 18:07:57 | efried | same thing as far as I'm concerned. Clouds have scripts. Or orchestrators, or whatever. That expect it to work a certain way. | |
| 18:08:31 | efried | If we put a real stake in the ground about upgrading placement first, it's moot. We can rip out the existing 406 code, never have to implement it again going forward. | |
| 18:08:32 | dansmith | okay I'm not sure what you're looking for here | |
| 18:08:53 | dansmith | efried: as mriedem said, I'm not opposed to all failback code | |
| 18:09:05 | dansmith | by any means, but this one has no legit alternate path imho | |