| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-11-12 | |||
| 09:44:26 | nicolairuckel | ah, then I can remove that | |
| 09:45:35 | sean-k-mooney | yes im also wonderign about these changes https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/functional/libvirt/test_uefi.py | |
| 09:45:59 | sean-k-mooney | is this a side effect of when you remvoed os.append(nvram) | |
| 09:46:37 | sean-k-mooney | when your testing the generation fo the xml https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/unit/virt/libvirt/test_config.py | |
| 09:46:47 | sean-k-mooney | you are assert ign the nvram element is present which is correct | |
| 09:47:50 | sean-k-mooney | it still feels like you have mroe changes tehn are required but i have not gone line by line to verify | |
| 09:48:26 | nicolairuckel | you're right about the first one for sure. | |
| 09:49:04 | nicolairuckel | Let me change that, the version check and run the tests locally again | |
| 09:49:54 | sean-k-mooney | ack. this is pretty close to correct at thsi point so we jus tneed to get the last few bit fixed up and we can proceed with ti and start backproting once we get someone else to review as well | |
| 09:51:55 | nicolairuckel | I guess I can remove everything to _may_keep_nvram then, right? | |
| 09:52:22 | sean-k-mooney | ya | |
| 09:52:53 | sean-k-mooney | what i woudl do is create a clean clone and apply just the https://review.opendev.org/c/openstack/nova/+/959682/14/nova/virt/libvirt/guest.py and https://review.opendev.org/c/openstack/nova/+/959682/14/nova/virt/libvirt/driver.py changes | |
| 09:53:14 | sean-k-mooney | - the _may_keep_nvram bits then run the test and see what breaks | |
| 09:53:35 | sean-k-mooney | that will tell you which parts of the rest of the chagne are actully needed | |
| 09:54:39 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/unit/virt/libvirt/test_guest.py will be and https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/unit/virt/libvirt/test_driver.py | |
| 09:55:09 | sean-k-mooney | i dont think you need https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/unit/virt/test_virt_drivers.py | |
| 09:55:13 | sean-k-mooney | but maybe | |
| 09:56:09 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/unit/virt/libvirt/test_config.py is good for extra test coverage but im not sure https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/functional/libvirt/test_uefi.py is | |
| 09:56:19 | sean-k-mooney | so i fyou can confirm that it woudl be good | |
| 09:56:51 | nicolairuckel | I'll do that. | |
| 09:57:47 | nicolairuckel | I'm not sure if I'll be able to do that today though. | |
| 09:58:24 | sean-k-mooney | thats ok just let us know when it ready | |
| 10:32:38 | opendevreview | sean mooney proposed openstack/nova master: add functional repoducer for bug 2048837 https://review.opendev.org/c/openstack/nova/+/919979 | |
| 10:32:39 | opendevreview | sean mooney proposed openstack/nova master: ensure correct cleanup of multi-attach volumes https://review.opendev.org/c/openstack/nova/+/916322 | |
| 10:33:28 | sean-k-mooney | gibi: bauzas i realsised that ^ is still open. woudl ye mind looking at those again | |
| 10:33:40 | bauzas | sean-k-mooney: ack I can try | |
| 11:34:06 | gibi | sean-k-mooney: added inline comments in the fix | |
| 11:37:34 | sean-k-mooney | gibi: so many of those comemnt i dismissed as wont do in the previous revision | |
| 11:37:46 | sean-k-mooney | partly because if dont wnat to reopen the formating holy wars | |
| 11:38:58 | sean-k-mooney | well these comments https://review.opendev.org/c/openstack/nova/+/916322/8/nova/compute/manager.py im still readying the rest | |
| 11:42:16 | sean-k-mooney | gibi: good question on the thread case. | |
| 11:42:34 | sean-k-mooney | what i can do is aquire the named lock before i add it to the list | |
| 11:43:16 | sean-k-mooney | that way i think only one thread will be able to do that | |
| 11:43:41 | sean-k-mooney | so ill reorder https://review.opendev.org/c/openstack/nova/+/916322/8/nova/utils.py#1196 and 1197 | |
| 11:43:46 | sean-k-mooney | that shoudl fix that. | |
| 12:21:58 | gibi | sean-k-mooney: Im not sure that lock is enough | |
| 12:23:05 | sean-k-mooney | well the sharign between thread you are descibign is not inteded to be supprote dbut ill comment more on the review | |
| 12:23:08 | gibi | there can be guard([a,b,c]) at the staget of a released but b is not released yet by T1 in exit. Then T2 can enter and take lock a and manipulate the list | |
| 12:24:02 | gibi | the doc of FairLockGuard does not really explain what is supported and what is not | |
| 12:24:16 | sean-k-mooney | ya i can improve that | |
| 12:28:17 | sean-k-mooney | to me its a logic bug to share a single instance of a context manager between treads | |
| 12:34:06 | gibi | OK so T1 and T2 has its own context manager but both using the same list of names to aquire the same set of locks | |
| 12:34:28 | gibi | that is the way we have shared locks but not shared context managers | |
| 12:34:44 | gibi | that is not intuitive so needs to be documented | |
| 12:35:11 | gibi | normally you do `with self.lock` in both T1 and T2 so the context manager (=lock) is shared | |
| 12:38:47 | sahid | o/ | |
| 12:38:58 | sahid | I have noticed this function during reboot: https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L4286 | |
| 12:39:18 | sahid | we are in situation where we would like avoid to reboot the VMs | |
| 12:39:43 | sahid | usually we use live-migration when we do have (expect) to fix issue like that | |
| 12:40:19 | sahid | i 'm wondering if that make sen for you as-well that i share a patch to cleanup attachment that are not valid anymore during pre-live-migration process | |
| 12:41:02 | sahid | basucally, somewhere around that point: https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L9342 | |
| 12:41:25 | sahid | if that feel good for you I would provide a bug report and share a patch | |
| 12:46:10 | sean-k-mooney | gibi: its pretty cheap for me to add that support | |
| 12:46:17 | sean-k-mooney | gibi: so i can add it | |
| 12:47:55 | gibi | OK | |
| 12:57:53 | sean-k-mooney | gibi: i woudl still discurrage the patteern of creating a context manager and passign it as a parmater to 2 or more thread in general as i think that likely is not genericly thread safe but i can make it less unsafe in this case. sorry was on a 1:1 so was not really around for the last 30 mins | |
| 13:28:00 | gibi | ack. I'm not advocating to make that work, I'm OK if we clearly document the usage pattern of the Guard | |
| 13:28:42 | gibi | if the shared data is not the Guard just the list of names the Guard is intantiated with that is fine by me | |
| 13:29:01 | gibi | just pointing out that | |
| 13:29:38 | gibi | `with threading.Lock()` is wrong while `with Guard(names)` is the correct way is non intuitive | |
| 13:29:49 | gibi | hence we need documentation | |
| 13:29:59 | sean-k-mooney | right | |
| 13:30:03 | sean-k-mooney | that the delta in usage | |
| 13:30:26 | sean-k-mooney | i need to push a patch to cyborg and venos to fix a requriement issue that i just fixed in watcher | |
| 13:30:38 | sean-k-mooney | but when im done ill update the patch again. thanks for reviewing | |
| 13:31:27 | gibi | cool thanks | |
| 14:21:53 | opendevreview | Vasyl Saienko proposed openstack/nova master: Set vnic model and queue for VDPA https://review.opendev.org/c/openstack/nova/+/966855 | |
| 14:39:42 | opendevreview | Vasyl Saienko proposed openstack/nova master: Set vnic model and queue for VDPA https://review.opendev.org/c/openstack/nova/+/966855 | |
| 14:40:50 | vsaienko | sean-k-mooney: I found that queues parameter is not passed to domain xml interface section in case of VDPA, this was a reason of slow iperf test. I've reported a bug https://bugs.launchpad.net/nova/+bug/2131150 and created a fix https://gerrit.mcp.mirantis.com/c/packaging/sources/nova/+/247944 please add to your review queue | |
| 14:41:06 | sean-k-mooney | vsaienko: yep i just -1 your patch | |
| 14:41:17 | sean-k-mooney | vsaienko: this si not a bug | |
| 14:41:28 | sean-k-mooney | it was not possibel to use multi queu whne we added vdpa supprot | |
| 14:41:37 | sean-k-mooney | and it was not implemented intentionally as a result | |
| 14:42:03 | vsaienko | hm... but now its possible | |
| 14:42:09 | sean-k-mooney | so 1 this is a new feature, two if we are got supprot this we will need much more testing the what is in the patch and mroe docs as well | |
| 14:42:28 | sean-k-mooney | vsaienko: rigth so as this is a feature requst it woudl normally need a bluefpirnt or a spec | |
| 14:42:53 | sean-k-mooney | we coudl do it under a bug in very limited cases but its not actully a bug from an upstream point of view | |
| 14:42:54 | vsaienko | ack | |
| 14:43:12 | sean-k-mooney | the main gaps are when did this supprot land in the kernel and qemu | |
| 14:43:37 | sean-k-mooney | we need to document that both in a release note and in the vdpa docs | |
| 14:43:48 | sean-k-mooney | depending o nthe qemu veriosn we may also need to add a version check | |
| 14:44:51 | sean-k-mooney | vsaienko: have you tested packed ring format ? | |
| 14:45:20 | sean-k-mooney | vsaienko: one of the open quetion too | |
| 14:45:37 | sean-k-mooney | is what happen if you have more cpu then the VF that backs the vdpa device supprot in hardware queue | |
| 14:46:09 | sean-k-mooney | i.e. is it ok for the number of queue we pass to qemu to exceed or even not match exactly the number of queue supproted by the hardware | |
| 14:48:15 | sean-k-mooney | vsaienko: if we can answer those quetions and ensure there is no upgrade impact when live migrating between hsot with and without this supprot we may be fine to proceed with a specless bluepritn but those quetions are why this woudl normally require a spec | |
| 14:49:38 | sean-k-mooney | for example it is poosble to set the queue for each vdpa device when they are created https://github.com/os-net-config/os-net-config/commit/384f46f8e0dfb847ab543e82f8cd08f5065edceb#diff-f43ec4c1cd48d8d241917be63e57e70516ef2bbba6e9ba786ce2ad5f1a92ca7dR628-R642 | |
| 14:50:01 | sean-k-mooney | now we dont offically suprpot that in nova but we also dont not supprot it | |
| 14:50:36 | sean-k-mooney | in the same way the number of quese for a sriov vf is determiend by how you allocate teh vf | |
| 14:51:20 | sean-k-mooney | that is howe it work today for vdpa as well so we need to make sure we dont break that flow if you dont enabel multi queu vis teh flavor. | |
| 14:52:31 | sean-k-mooney | vsaienko: sorry that a bit of an info dump but you can see why this is non trivial to do corectly and why the documenation of this really matters | |
| 14:53:10 | vsaienko | I haven't check packed ring format | |
| 14:53:51 | vsaienko | is what happen if you have more cpu then the VF that backs the vdpa device supprot in hardware queue - probably this is always a case when we have no multiqueue, VM has always more CPU and queues. | |
| 14:53:58 | sean-k-mooney | ack you change enabls opting in to that | |
| 14:54:36 | sean-k-mooney | vsaienko: in nova we set the queu count equal to the number of cpus when you enabel multi queue | |
| 14:54:55 | sean-k-mooney | so the question is fi the vdpa device only supprot 4 queue and you set it to 16 in qemu/libvirt what happens | |
| 14:55:14 | vsaienko | if the queue count in xml is higher than on VDPA device VM will see number that is configured on vdpa side | |
| 14:55:21 | vsaienko | I checked this | |
| 14:55:59 | sean-k-mooney | ack so we woudl need to 1 agree thats ok, it shoudl be and 2 docuemnt that so OPs are not surpised by that | |
| 14:56:45 | sean-k-mooney | we have a config option to clamp the number of queue so it is already posibel today for the vcpu count and queu count to be diffent even with muliqueueu enebled | |