| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-02 | |||
| 15:41:28 | sean-k-mooney | bauzas: it look like they might have been wrong. libvirt is provide qemu the capablity to lock memory but it does not look like its using it | |
| 15:42:15 | kashyap | gibi: artom: When you get a min, I think I've addressed all the pressing concerns: https://review.opendev.org/c/openstack/nova/+/774240 | |
| 15:43:24 | gibi | kashyap: ack, I will check back | |
| 15:44:21 | kashyap | Thanks; see the small change log for PS10 ["Mar 01 12:54 PM"] -- the only diff in PS11 is to put back the line that I accidentally removed during rebase. | |
| 16:34:46 | gibi | bauzas: I left some questions in the RPC bump series https://review.opendev.org/c/openstack/nova/+/761452 but overall the patch looks good to me | |
| 16:35:02 | bauzas | thanks for the review, /me looks | |
| 16:35:18 | bauzas | gibi: tbc, we need to hold this change until FF | |
| 16:35:36 | gibi | bauzas: is there any RPC impacting change open I should be aware of? | |
| 16:35:39 | bauzas | but the more we review, the quickier we could merge it just after m-3 | |
| 16:35:57 | bauzas | gibi: some compute service bumps AFAIK | |
| 16:36:14 | gibi | true, service version will be in conflict if we merge those | |
| 16:36:29 | bauzas | https://review.opendev.org/c/openstack/nova/+/761452 has a lot of merge conflicts | |
| 16:36:44 | bauzas | so I'm prepared to rebase this change | |
| 16:37:09 | gibi | bauzas: ack. I will prioritize to land this after FF | |
| 16:37:25 | gibi | but we might get FFE requests | |
| 16:37:36 | gibi | with service bumps | |
| 16:37:45 | gibi | so it will be a tricky balance | |
| 16:38:08 | gibi | anyhow we have to be strict with FFEs due to sortness of time til RC1 | |
| 16:38:37 | gibi | brinzhang0: could you please check what would be the good accel_uuid paramter here https://review.opendev.org/c/openstack/nova/+/761452/9/nova/compute/manager.py#9481 ? | |
| 16:40:25 | bauzas | gibi: what we could discuss is whether this parameter should be mandatory | |
| 16:40:56 | sean-k-mooney | gibi: ya with one week i would say we likely dont have time in most case | |
| 16:40:58 | bauzas | same for all the other methods like rebuild | |
| 16:41:57 | gibi | bauzas: I think it should be mandatory as the impl uses accel_uuids to support cyborg devices, so if accel_uuids param is not passed then we loose cyborg supprot | |
| 16:42:00 | gibi | suport | |
| 16:42:01 | gibi | support even | |
| 16:42:12 | gibi | sean-k-mooney: yeah | |
| 16:42:55 | gibi | sean-k-mooney: we have two week between FF (Mar12) and the lat possible date of RC1 Mar26 | |
| 16:43:13 | gibi | I mean FF mar11 | |
| 16:43:17 | gibi | for nova | |
| 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: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 | bauzas | that's sad | |
| 16:47:08 | sean-k-mooney | i honestly dont recall what i did orginally but i proably passed an empty list | |
| 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 | 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:23 | dansmith | really? | |
| 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 | bauzas | which makes this argument mandatory from a manager perspective IIUC | |
| 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: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 | |