| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-02 | |||
| 16:49:43 | gibi | sean-k-mooney: hm, _create_and_bind_arqs and get_arq_uuids_for_instance returns [] | |
| 16:49:46 | dansmith | bauzas: just skimming the scrollback... the question is what the *client* should do, right? | |
| 16:49:51 | bauzas | sean-k-mooney: to be clear, do we pass the argument with a None value in it, or do we just call the API without this arg ? | |
| 16:50:10 | bauzas | dansmith: the question is, should we make accel_uuids mandatory (I did this, but this is terrible) | |
| 16:50:11 | dansmith | ideally, all RPC parameters for matching client/server would be passed, either None or [] depending, but never missing for a given version that supports it | |
| 16:50:23 | dansmith | bauzas: right but mandatory where, client or server? | |
| 16:50:29 | bauzas | server | |
| 16:50:42 | dansmith | it should be mandatory that it is passed over the wire, yes | |
| 16:50:49 | bauzas | that's my point | |
| 16:51:05 | bauzas | we could have make it optional | |
| 16:51:07 | dansmith | the client should always pass it, but the client's own python API can make it optional for the rest of the code if we want, just for convenience | |
| 16:51:13 | bauzas | but looks like we didn't | |
| 16:51:23 | dansmith | really? | |
| 16:51:23 | sean-k-mooney | the server side in teh compute manager seam to use None so i guess the rpcapi is what we need to check or the compute api | |
| 16:51:46 | sean-k-mooney | thats using [] | |
| 16:51:48 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4192 | |
| 16:51:54 | dansmith | either way, 6.0 not landed yet, so we can make it mandatory in 6.0 and correct that problem | |
| 16:52:03 | bauzas | we default to a empty list | |
| 16:52:08 | bauzas | and we pass the parameter anyway | |
| 16:52:24 | dansmith | since 5.x people can't talk to 6.0 anyway, and then the 5.x proxy can tolerate it being missing | |
| 16:52:24 | bauzas | which makes this argument mandatory from a manager perspective IIUC | |
| 16:52:46 | sean-k-mooney | ya so form the compute manager/driver point of view it hink it will happly use [] or None and treat it the same | |
| 16:53:23 | sean-k-mooney | if you really think [] is too much overhead vs None you could proably change it in 6.0 but im not sure it gets us much | |
| 16:54:36 | bauzas | mmm | |
| 16:54:39 | bauzas | I'm confused | |
| 16:54:45 | dansmith | whether it's [] or None doesn't really matter much, but I expect [] makes more sense | |
| 16:54:55 | gibi | ^^ agree | |
| 16:55:04 | bauzas | before this change, we were having accel_uuids be None | |
| 16:55:11 | bauzas | hence be optional | |
| 16:55:12 | sean-k-mooney | ya we just cant uses [] for the default in a kwarg | |
| 16:55:22 | bauzas | but this was just for compat reasonqs | |
| 16:55:24 | gibi | bauzas: only for the case when the client did not sent the param accel_uuids | |
| 16:55:26 | sean-k-mooney | because only one list would be created and shared between all calls | |
| 16:55:29 | bauzas | gibi: correct | |
| 16:55:45 | bauzas | gibi: so, a recent client was *always* passing an empty list | |
| 16:55:48 | sean-k-mooney | that why we use None in the compute manager but always pass [] or a populated list | |
| 16:56:09 | gibi | sean-k-mooney: we are not defaulting anyting to [], the client sends a list either empty or non empty | |
| 16:56:20 | bauzas | well, a recent client is *always* passing accel_uuids as a param, tbc | |
| 16:56:25 | sean-k-mooney | gibi: correct | |
| 16:56:36 | gibi | OK, we are in violent agreement then :) | |
| 16:56:39 | bauzas | ok, so let's leave it mandatory | |
| 16:56:42 | gibi | yepp | |
| 16:56:47 | bauzas | sad but ok | |
| 16:56:54 | dansmith | the reason I think [] makes more sense than explicitly passing None is that the latter is the only indication we have that it wasn't passed, which in the past was important for knowing "does the client even know about accel_uuids" in terms of compat behavior | |
| 16:57:09 | dansmith | but going forward, I think it should be mandatory for sure, and ideally [] if no accel_uuids | |
| 16:57:20 | bauzas | I agree, it was the usual signal for knowing whether the client was new enoguh | |
| 16:57:26 | dansmith | yeah | |
| 16:57:32 | bauzas | but we also have optional args | |
| 16:57:46 | dansmith | ideally we shouldn't | |
| 16:58:00 | bauzas | because the RPC signature can change ? | |
| 16:58:13 | bauzas | I think I get you | |
| 16:58:26 | sean-k-mooney | dansmith: true although i dont think we relay on that distiction anywhere today but its a valid reason. i take a different appoch and say well the data stucture should be a list so pass an empty one in preference to None | |
| 16:58:28 | bauzas | either way, let's stick with it then | |
| 16:58:41 | bauzas | gibi: I'll reply to your comments | |
| 16:58:50 | gibi | cool | |
| 16:58:53 | gibi | I will check back tomorro | |
| 16:58:54 | gibi | w | |
| 16:59:07 | dansmith | sean-k-mooney: yeah, it's explicit vs. implicit | |
| 17:08:28 | gibi | artom, stephenfin, kashyap: Do we have an agreement on the extra config option vs. +/- prefix in https://review.opendev.org/c/openstack/nova/+/774240/ ? | |
| 17:13:04 | stephenfin | I would still rather separate config opts since that seems clearer and less "unique" to me. I would appreciate other perspectives, though it's definitely into bikeshedding territory | |
| 17:13:57 | gibi | I can work with both but I do understand the uniqueness of the prefix based approach | |
| 17:14:21 | bauzas | stephenfin: dansmith: worth removing this argument now I'm bumping the RPC API ? https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L5311 | |
| 17:14:34 | bauzas | I mean, in https://review.opendev.org/c/openstack/nova/+/761452 | |
| 17:14:34 | gibi | I cannot recall if we discussed this during the meeting when we approved the bp | |
| 17:14:47 | artom | stephenfin, gibi, I don't really care, tbh - stephenfin's idea feels more rigorously correct, but the current +/- approach does have the "way less work" argument on its side | |
| 17:15:11 | artom | And it's not like +/- is super complex to understand. | |
| 17:15:32 | bauzas | stephenfin: dansmith: but I'm fine with punting this for a later minor bump, like 6.1 | |
| 17:15:45 | dansmith | bauzas: you can't drop a param in 6.1 :) | |
| 17:15:51 | artom | I'd be more inclined to go stephenfin's route if the alternative was edging into DSL territory, which it definitely isn't | |
| 17:15:58 | bauzas | oh shit | |
| 17:16:00 | bauzas | you're right | |
| 17:16:14 | bauzas | dansmith: worth the effort then ? | |
| 17:16:26 | stephenfin | No, it's not. It just feels a bit weird. I think I'd even be happier if we had a path to not supporting unprefixed opts | |
| 17:16:31 | dansmith | bauzas: yep, else you wait until 7.0 and those cleanups are the whole point of this | |
| 17:16:57 | bauzas | dansmith: my only concern is that we're passing this argument everytime now | |
| 17:17:02 | stephenfin | gibi: If you don't particularly care, I can drop my complaint. I don't want to have to rework the patch so something is better than nothing | |
| 17:17:05 | bauzas | it's just the manager which doesn't use it | |
| 17:17:06 | gibi | kashyap, stephenfin, artom: so the prefix based solution was raised during the meeting http://eavesdrop.openstack.org/meetings/nova/2021/nova.2021-02-11-16.00.log.html#l-148 | |
| 17:17:25 | gibi | and there was no objection then | |
| 17:17:26 | dansmith | bauzas: no we're not, because 6.0 hasn't landed yet and YOU are defining its behavior :) | |
| 17:17:42 | bauzas | but I'm lazy | |
| 17:17:48 | bauzas | :p | |
| 17:17:49 | dansmith | bauzas: well, that may be :) | |
| 17:18:11 | stephenfin | gibi: yeah, my lack of objections was to the general idea of disabling features rather than how we'd do it (I'd already zoned out by then, as kashyap thought we might have :)) | |
| 17:18:21 | artom | gibi, ah, last week when I was on PTO? :) | |
| 17:18:31 | bauzas | dansmith: I mean, I would have appreciated if someone (like stephenfin :p ) would have stopped to provide this arg by the client | |
| 17:18:36 | bauzas | client isez | |
| 17:18:38 | bauzas | side | |
| 17:18:49 | bauzas | now, we're passing it over the wire | |
| 17:18:54 | bauzas | so we could pretend we never did | |
| 17:18:55 | dansmith | bauzas: in 5.x | |
| 17:19:01 | bauzas | yup | |
| 17:19:06 | dansmith | but not in 6.x | |
| 17:19:11 | gibi | artom: you were present on that meeting :) | |
| 17:19:20 | bauzas | dansmith: right, making it optional | |
| 17:19:31 | bauzas | on the client | |
| 17:19:33 | artom | gibi, oh, hah :P | |
| 17:19:39 | gibi | but good try :P | |
| 17:19:41 | artom | Clearly shows how much I care | |
| 17:19:45 | bauzas | dansmith: but of course, still mandatory on the server until 6.0 | |