| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-02 | |||
| 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 | gibi | I cannot recall if we discussed this during the meeting when we approved the bp | |
| 17:14:34 | bauzas | I mean, in https://review.opendev.org/c/openstack/nova/+/761452 | |
| 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 | |
| 17:19:56 | artom | Genuinely, we could go either way and I'll sleep perfectly well that night | |
| 17:20:10 | gibi | OK, let's go with the proposed implementation. as I don't see it as a dealbreaker | |
| 17:20:26 | gibi | I'm upgrading my vote on the patch | |
| 17:21:12 | dansmith | gibi: it shouldn't even be optional on the client for 6.0 right? | |
| 17:21:21 | stephenfin | cool, I'll take a look tomorrow | |
| 17:21:26 | gibi | stephenfin: thanks | |
| 17:21:27 | stephenfin | still working through the VNC fun | |
| 17:21:38 | stephenfin | it's a pain in the a*** :) | |
| 17:22:47 | gibi | dansmith: logically accel_uuids are a part of the client code, we do gather them from the flavor and from cyborg. So I think it should be mandatory on the client side as well | |
| 17:22:48 | dansmith | oops s/gibi/bauzas/ above | |
| 17:23:04 | dansmith | gibi: that comment was about request_spec, not accel_uuids | |
| 17:23:08 | gibi | aah | |
| 17:23:11 | gibi | then ignore me | |
| 17:23:16 | dansmith | gibi: I confused you by replying to you instead of bauzas | |
| 17:23:17 | gibi | sorry | |
| 17:23:25 | bauzas | dansmith: right | |
| 17:23:44 | bauzas | OK, looks like I have work to do | |
| 17:28:34 | bauzas | honestly, since this request_spec argument isn't deprecated yet in some 5.x API, removing it now would make 6.0 semantically different from 5.13 | |
| 17:28:55 | bauzas | so I won't do it, unless someone steps up and write a 5.x version for making it deprecated | |
| 17:28:59 | bauzas | stephenfin: ^ | |
| 17:29:23 | bauzas | we lost some opportunity here | |
| 17:30:07 | stephenfin | I haven't been following, I'm afraid :( We're passing an unnecessary argument through to some RPC API? | |
| 17:34:46 | sean-k-mooney | bauzas: how would we get the request_spec if we dont pass it? via the instance? | |
| 17:35:02 | sean-k-mooney | i dont think we want to lazy load that if its currently used | |
| 17:35:19 | bauzas | sean-k-mooney: stephenfin's point is that we don't use this parameter even if we pass it thru the wire | |
| 17:35:37 | sean-k-mooney | for which api call | |
| 17:35:47 | bauzas | and we have a few other methods that have some parameters passed thru the wire that we don't use | |
| 17:36:05 | sean-k-mooney | right so those whould have to be removed in 6.0 | |
| 17:36:06 | bauzas | you know what ? I'm changing the docstring to be 7.0 | |
| 17:36:29 | bauzas | and we know we have to write some 6.x API version that would deprecate those parameters | |
| 17:37:02 | sean-k-mooney | do we have to do deprecations like that for internal rpc apis? | |
| 17:37:07 | bauzas | honestly, now I wrote a 6.0 bump, I feel brave enough for writing a 7.0 one | |
| 17:37:38 | bauzas | but this will have to be at least for X, ideally Y | |
| 17:37:41 | sean-k-mooney | bauzas: well we likely wont do a 7.0 bump for another couple of releases | |
| 17:37:49 | bauzas | yes | |
| 17:37:50 | bauzas | I know | |
| 17:38:01 | sean-k-mooney | proably not before Z | |
| 17:38:22 | sean-k-mooney | i mean there is not strict rule but we have mostly wated 4-6 release between major bumps | |