| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-06 | |||
| 09:46:19 | bauzas | just downloading the patch for verifying somethin | |
| 09:51:39 | johnthetubaguy | stephenfin: is it worth being more specific on what happens when you don't set an extra spec? or describing dependencies, like things that imply numa aware placed VMs, etc | |
| 09:52:10 | gibi | cores, here is a quick functional test stabilization patch needing a second core https://review.opendev.org/#/c/717070 | |
| 09:52:31 | stephenfin | You mean in the descriptions for the extra specs themselves? | |
| 09:52:32 | bauzas | gibi: on it, I'm mostly done | |
| 09:52:44 | johnthetubaguy | stephenfin: somewhere like that | |
| 09:52:50 | gibi | bauzas: thanks | |
| 09:53:01 | johnthetubaguy | stephenfin: comparing it to configuration, basically | |
| 09:53:21 | stephenfin | Yeah, sure. I probably need to better flesh out the descriptions for much of them, including things like what virt driver supports what extra spec | |
| 09:53:46 | stephenfin | In a follow-up though, preferably? I'd like to focus more on making sure every possible extra spec is listed there first | |
| 09:53:54 | johnthetubaguy | stephenfin: I thought you did fairly well on the virt support, I guess there are a bunch of libvirt only in there that are not spelled out | |
| 09:54:11 | johnthetubaguy | stephenfin: yeah, I am OK with that, just wondering about your plans really | |
| 09:54:41 | johnthetubaguy | stephenfin: there are things like PCI alias, when you check the format and not the content, which is typical of json schema checks, just wondering about your thoughts there | |
| 09:54:47 | stephenfin | Yeah, I plan to massively improve the documentation and do things like add cross-referencing | |
| 09:55:19 | johnthetubaguy | to be clear, I would be fine just checking the keys and not the value, as a step forward, so this is a step ahead of that | |
| 09:55:49 | stephenfin | Ah, so for more detailed things like that, I was planning to leave it to the virt driver for now | |
| 09:56:22 | stephenfin | ditto for things like "you need to specify 'hw:cpu_policy' to use 'hw:cpu_thread_policy'" | |
| 09:56:57 | stephenfin | I wanted to do that via the validator initially but it was way too much /o\ | |
| 09:57:12 | johnthetubaguy | I guess I see this as our global API abstraction, that has been implemented by limited drivers... but yeah, its totally a next step | |
| 10:05:05 | bauzas | stephenfin: I don't see anything in the spec about API interop consistency with updates on, say, https://review.opendev.org/#/c/704643/21/nova/api/validation/extra_specs/capabilities.py | |
| 10:05:28 | bauzas | like, I want 'baz' to be accepted in Victoria | |
| 10:05:45 | bauzas | but calling a Ussuri API will tell you 'sorry but no' | |
| 10:05:55 | bauzas | stephenfin: do you plan to address this later on ? | |
| 10:06:13 | johnthetubaguy | its not turned on yet right, haven't set the microversion where it starts | |
| 10:08:08 | bauzas | johnthetubaguy: indeed, see my comments https://review.opendev.org/#/c/704643/21 | |
| 10:08:36 | stephenfin | bauzas: There's no easy answer for that. I think that's the main reason we had to make the policy configurable. We didn't want to require a microversion to add a new extra spec so people needed a escape lever | |
| 10:09:22 | bauzas | stephenfin: okay, I just feel my biggest concern is that if we avoid to whitelist some key for some filter, then people will see a behavioural change | |
| 10:09:23 | bauzas | right? | |
| 10:09:26 | stephenfin | bauzas: tbh though, it's the exact same issue we see with image metadata today and configuring flavor extra specs, unlike configuring image metadata, is admin-only | |
| 10:10:13 | bauzas | alas. people can use 2.85 microversion if they want to disable it | |
| 10:10:14 | stephenfin | bauzas: Correct, but we have signalled that change with a microversion and we've provided a way to "escape" validation | |
| 10:10:29 | bauzas | stephenfin: yeah I understood it | |
| 10:10:32 | stephenfin | or pass '?validation=permissive', iirc | |
| 10:10:50 | bauzas | stephenfin: I'm just afraid that we could merge something that would opt-in | |
| 10:10:59 | bauzas | close to RC1 | |
| 10:11:09 | bauzas | stephenfin: but we can go like it is now | |
| 10:11:20 | bauzas | at least if we have ways to address issues easilty | |
| 10:11:42 | bauzas | because reviewing all the key and value regexes for every filter isn't an easy thing | |
| 10:11:47 | bauzas | but I don't want to hold this one | |
| 10:12:22 | bauzas | so, my take is : if we mess things up, we all assume that we're about to provide bugfixes that will touch the API behaviour and should be backportable | |
| 10:12:31 | bauzas | if we all agree on this, I'm thumbs up | |
| 10:12:36 | bauzas | gibi: stephenfin: johnthetubaguy: ^ | |
| 10:12:55 | bauzas | or we go permissive *by default* | |
| 10:13:05 | johnthetubaguy | hang on... I am missing something | |
| 10:13:06 | bauzas | (which is not the default case IIUC) | |
| 10:13:22 | johnthetubaguy | don't we currently do zero validation by default right now? | |
| 10:13:33 | gibi | bauzas: fixing bugs in the extra_spec validation will not need a new microversion in my view. | |
| 10:13:40 | johnthetubaguy | including after the proposed validation | |
| 10:13:58 | bauzas | gibi: me too, but I'm just saying that we will change the API behaviour silently | |
| 10:14:05 | johnthetubaguy | hang about | |
| 10:14:07 | bauzas | so we all need to accept it | |
| 10:14:10 | stephenfin | johnthetubaguy: no, after https://review.opendev.org/#/c/704643/ it's enabled by default | |
| 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 | |