| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-02 | |||
| 16:43:13 | gibi | I mean FF mar11 | |
| 16:43:17 | sean-k-mooney | gibi: the only reservatio ni would have for makign it mandatory is that not all driver need it or will support cyborg | |
| 16:43:17 | gibi | for nova | |
| 16:43:25 | sean-k-mooney | but other then that ya it could be | |
| 16:43:30 | bauzas | gibi: yeah for sure | |
| 16:43:40 | bauzas | gibi: we actually pass the parameter everytime now | |
| 16:43:41 | gibi | it is the compute manager interface not the virt driver interface | |
| 16:43:50 | bauzas | if so, it's mandatory | |
| 16:44:00 | bauzas | but if we don't, then it should continue to be optional | |
| 16:44:03 | sean-k-mooney | gibi: oh i though rc1 was the 18 cool still tight | |
| 16:44:06 | bauzas | sean-k-mooney: am i right? | |
| 16:44:30 | sean-k-mooney | bauzas: when working with cyborg instace we need to pass it | |
| 16:44:41 | sean-k-mooney | but for none cycboge isntace its not needed | |
| 16:44:55 | bauzas | sean-k-mooney: the question is, do we pass them everytime even without cyborg ? | |
| 16:45:01 | gibi | bauzas: I also think that we pass accel_uuids now via the RPC. if this is not a cyborg intance then we pass [] I hope | |
| 16:45:21 | bauzas | sean-k-mooney: meaning we could pass an empty list | |
| 16:45:34 | sean-k-mooney | we can pass an empty list yes | |
| 16:45:36 | bauzas | but we could have made it optional | |
| 16:45:47 | bauzas | instead of an empty list | |
| 16:45:54 | sean-k-mooney | we cant make kwards defualt to empty list | |
| 16:46:00 | bauzas | like, you don't use cyborg, accel_uuids be None | |
| 16:46:04 | sean-k-mooney | but we could default to None | |
| 16:46:17 | sean-k-mooney | and then pass [] if its none | |
| 16:46:22 | bauzas | and if so, the RPC 6.0 version should continue to support None | |
| 16:46:37 | bauzas | but afaicu, we pass [] | |
| 16:46:43 | bauzas | so now, it's mandatory | |
| 16:46:50 | bauzas | I mean, the parameter is passed | |
| 16:46:56 | bauzas | every time | |
| 16:47:06 | bauzas | even if 99% of the time the value is an empty list | |
| 16:47:08 | sean-k-mooney | i honestly dont recall what i did orginally but i proably passed an empty list | |
| 16:47:08 | bauzas | that's sad | |
| 16:47:27 | bauzas | dansmith: thoughts on it ? | |
| 16:47:42 | bauzas | dansmith: I recall you reviewed the cyborg patches | |
| 16:47:45 | gibi | it is empty list in the cases I now checked | |
| 16:48:03 | bauzas | ok, then we need to make the parameter mandatory | |
| 16:48:14 | gibi | and you did that | |
| 16:48:18 | bauzas | yup | |
| 16:48:21 | bauzas | but that's sad | |
| 16:48:29 | bauzas | in particular for the boot case | |
| 16:48:51 | bauzas | I would have preferred to have this parameter be optional | |
| 16:49:02 | sean-k-mooney | actully i think we pass None though most of the calls | |
| 16:49:05 | sean-k-mooney | not [] | |
| 16:49:14 | bauzas | but the ship is sailed | |
| 16:49:22 | bauzas | has* sailed | |
| 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 | |