| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-06 | |||
| 10:14:11 | johnthetubaguy | this totally requires a microversion | |
| 10:14:16 | gibi | bauzas: we fix bugs in the API behavior. Which was allowed before as well | |
| 10:14:24 | stephenfin | whoops | |
| 10:14:24 | bauzas | johnthetubaguy: this is enabled later in another change | |
| 10:14:27 | stephenfin | https://review.opendev.org/#/c/708436/ | |
| 10:14:33 | bauzas | 2.86 | |
| 10:14:33 | stephenfin | johnthetubaguy: got that ^ | |
| 10:14:59 | stephenfin | johnthetubaguy: It's a noop with the first patch. The microversion change to turn it on is a separate change | |
| 10:15:05 | bauzas | tbc, I'm okay with approving https://review.opendev.org/#/c/704643/ provided stephenfin (or me) just changes the commit msg | |
| 10:15:28 | johnthetubaguy | stephenfin: yeah, i.e. its added in a microversion | |
| 10:15:32 | bauzas | but I'm not okay with https://review.opendev.org/#/c/708436/ until we basically agree on the stuff I said | |
| 10:15:51 | johnthetubaguy | so... if you do an old microversion, there is no change | |
| 10:15:51 | bauzas | but as gibi said, bugfixes are accepted, so maybe I'm overthinking | |
| 10:16:00 | bauzas | correct | |
| 10:16:06 | gibi | correct | |
| 10:16:12 | johnthetubaguy | nope, bug fixes than change the API would be really bad right? | |
| 10:16:22 | johnthetubaguy | I am really confused here | |
| 10:16:32 | johnthetubaguy | code looks OK to me, but not agreed with the discussion here | |
| 10:16:36 | gibi | johnthetubaguy: if we broke the API we have to fix them with a bugfix. I don't see how that is a bad thing. | |
| 10:17:04 | johnthetubaguy | gibi: nope, we just keep a broken microversion, we did exactly that in the past, see the limits APIs | |
| 10:17:23 | johnthetubaguy | well, it depends on the bug... | |
| 10:17:26 | stephenfin | we're saying that additions to and bug fixes for the _registry_ of extra specs are okay and wouldn't need a microversion | |
| 10:17:32 | gibi | I'm sure that in other cases we said "client should not have to opt in to get a fix" | |
| 10:17:33 | stephenfin | not the API itself | |
| 10:17:41 | bauzas | okay, see, that's why I wanted some discussion about this | |
| 10:17:48 | johnthetubaguy | hmm... | |
| 10:17:50 | bauzas | ... or we set it to permissive by default | |
| 10:18:12 | johnthetubaguy | permissive by default seems pointless to me, needs to get on by default in a new microversion | |
| 10:18:14 | bauzas | and then we have a later change for turning it by default that would require approvals separately | |
| 10:18:15 | stephenfin | that has to be okay, because operators are currently allowed to specify their own extra specs for use in e.g. custom scheduler filters | |
| 10:18:37 | gibi | I think the differentiator was always like "Was the bug in the API was clearly a bad behavior?" | |
| 10:18:51 | bauzas | johnthetubaguy: I agree, the microversion bump would have to be on the 'turn on by default' | |
| 10:18:52 | stephenfin | if we were to say new extra spec == new microversion, we'd have to take that away from operators | |
| 10:18:57 | johnthetubaguy | it was, "does it impact security" I think | |
| 10:19:17 | johnthetubaguy | stephenfin: we would have to give them a namespace like CUSTOM: | |
| 10:19:34 | johnthetubaguy | so to use the new API microversion, they need to update their flavors | |
| 10:19:45 | stephenfin | if we were doing this from scratch, yes | |
| 10:19:57 | stephenfin | but there are custom extra specs in the wild now, so that ship has sailed | |
| 10:20:05 | johnthetubaguy | well, not quite | |
| 10:20:17 | johnthetubaguy | they would still work with older microversions right? | |
| 10:20:27 | bauzas | I don't want to rediscuss the spec https://specs.openstack.org/openstack/nova-specs/specs/ussuri/approved/flavor-extra-spec-validators.html | |
| 10:20:27 | johnthetubaguy | to move forward, they need a migration path | |
| 10:21:02 | johnthetubaguy | well... I guess we should be comparing this to image properties | |
| 10:21:06 | bauzas | I'm just okay with the plan, but from an implementation point of view, I feel that has necessarly to be discussed | |
| 10:21:47 | stephenfin | yeah, they could use the older microversions | |
| 10:22:50 | stephenfin | compared to image metadata properties, I think we're mostly in the same place now | |
| 10:23:22 | stephenfin | you can add a new image metadata property or modify an existing one (add a new value to an enum) | |
| 10:23:51 | johnthetubaguy | so I am stuggling to understand everyone's position in text form | |
| 10:25:08 | stephenfin | for example, setting the 'hw_pci_numa_affinity_policy' property would be rejected by a Train cloud but not an Ussuri cloud, regardless of microversion | |
| 10:25:20 | stephenfin | (since it was only added this cycle) | |
| 10:26:55 | johnthetubaguy | but to be rejected, they would need to be calling the new microversion right? | |
| 10:26:58 | bauzas | johnthetubaguy: stephenfin: the consensus I think is that https://review.opendev.org/#/c/704643/ is a nobrainer | |
| 10:27:01 | gibi | my position: I assume that the defined key-values are good (reviewed a good chunk of it), if we break something then we will fix it in a bugfix, I don't see other ways to prevent a break. custom extra_specs can be used with old microversion, and custom validator can be added to use the new microversion | |
| 10:27:36 | bauzas | but https://review.opendev.org/#/c/708436/ is debatable until we reach an agreement | |
| 10:27:57 | johnthetubaguy | I think we agree with the patch, but are unsure on future changes? | |
| 10:28:16 | stephenfin | johnthetubaguy: my position is that tying extra spec registry modifications to microversions is a lot of work and gives us almost nothing in return w.r.t. API interop | |
| 10:28:27 | bauzas | johnthetubaguy: I'm okay with https://review.opendev.org/#/c/704643/ because it's just a non-enabled framework | |
| 10:29:09 | bauzas | but we need to draw a line | |
| 10:29:32 | bauzas | we said in the spec that a microvesion would signal the fact that we enforce now | |
| 10:29:38 | bauzas | and I'm still OK with this | |
| 10:29:56 | stephenfin | because A) many extra specs are virt driver specific and therefore do different things on different clouds, B) flavor extra specs are admin-only by default, C) we've provided an escape lever for people that don't want this, and D) this is how image metadata props also work | |
| 10:30:10 | johnthetubaguy | bauzas: because that is what the current patches do right? | |
| 10:30:49 | johnthetubaguy | stephenfin: no, they mean the same thing on every cloud, just not all clouds have them available, at least that is the design | |
| 10:31:04 | bauzas | johnthetubaguy: correct, my only concern is the potential issues we could raise or any potential new keys we would want to provide | |
| 10:31:22 | johnthetubaguy | plus the wild west of custom scheduler stuff, which we need to keep | |
| 10:31:23 | bauzas | and that wasn't addressed in the spec AFAIK | |
| 10:31:39 | johnthetubaguy | bauzas: yeah, agreed with that worry | |
| 10:32:42 | bauzas | there are actually 2 different things in my mind | |
| 10:32:52 | bauzas | A/ a forgotten key | |
| 10:33:00 | bauzas | B/ a new key in a future release | |
| 10:33:36 | bauzas | A/ sounds a bug, and I personnally agree with gibi on the fact we should just accept to backport a bugfix aiming to fix such things | |
| 10:33:56 | bauzas | B/ is still undesigned in my mind | |
| 10:34:34 | bauzas | because 'Cloud OVH' could unsupport 'myfancynewkey' while 'Cloud Vexxhost' would | |
| 10:34:46 | gibi | bauzas: we might now better what to do with B as when we have the situation to add an new in tree key | |
| 10:35:05 | stephenfin | kick that decision to Victoria? :) | |
| 10:35:07 | bauzas | compared to now where both support it (well, actually, just accepting it silently) | |
| 10:35:18 | gibi | bauzas: those OVH and Vexxhost keys won't be in tree keys | |
| 10:35:36 | gibi | so they need to implement custom validators | |
| 10:35:45 | gibi | I guess | |
| 10:36:02 | bauzas | gibi: if this key is written to be used in a Victoria change, and if OVH lags with a Ussuri cloud compared to Vexxhost, then you'll see a change | |
| 10:36:33 | stephenfin | only the cloud operators themselves will see it though | |
| 10:36:37 | gibi | bauzas: do you mean a situation, when I have a script that creates flavors both in OVH and Veexhost? | |
| 10:36:46 | stephenfin | and they'd see the same thing for image metadata | |
| 10:36:48 | bauzas | that's exactly why 3 years ago, we left the custom filters to be wildcards | |
| 10:37:28 | bauzas | gibi: for a flavor, this requires an admin by default policy | |
| 10:37:35 | bauzas | so i wouldn't worry too much | |
| 10:37:51 | gibi | bauzas: then in what situation you worry? | |
| 10:38:06 | bauzas | image properties that can be user-defined | |
| 10:38:28 | bauzas | do we expose the flavor extra specs to the users ? I think so too | |
| 10:38:58 | gibi | stephenfin: does the same validator code runs for the image properties? | |
| 10:39:26 | sean-k-mooney | bauzas: image propertiese cant be userdeined | |
| 10:39:34 | stephenfin | gibi: no, image properties are already validated because they're mapped to o.vos | |
| 10:39:36 | sean-k-mooney | bauzas: we made them ovo years ago | |
| 10:39:57 | bauzas | hem you're right | |
| 10:40:00 | sean-k-mooney | bauzas: yes its configurable by policy but extra specs are shown | |
| 10:40:02 | gibi | bauzas: so the image properties are a different story | |
| 10:40:30 | bauzas | okay, I just to reconsider whether we have a problem or not | |
| 10:40:33 | stephenfin | different but also the same, in as far as they're not tied to microversions | |
| 10:40:40 | sean-k-mooney | gibi: they use to be a blank sting until like extra specs but peopel were exploiting them to pass virt driver specific stuff | |
| 10:40:42 | gibi | adding an extra spec is also admin only by default, like creating flavor | |