| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-11-12 | |||
| 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 | |
| 14:57:29 | sean-k-mooney | the reason i asked about packed form at is you can now set it here https://review.opendev.org/c/openstack/nova/+/966855/2/nova/virt/libvirt/vif.py#206 | |
| 14:58:19 | sean-k-mooney | by the way did you intenilaly remove this https://review.opendev.org/c/openstack/nova/+/966855/1/nova/virt/libvirt/config.py form your v2? | |
| 15:06:51 | vsaienko | I remove because driver_name is set not to viritio there, and that config.py change is not needed | |
| 15:07:20 | sean-k-mooney | ack | |
| 15:10:40 | sean-k-mooney | vsaienko: so it looks like the libviert supprot was added by https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/D5QUZT2ZCUKF2526EOLGCB5JWCHHTIRV/?sort=thread and qemu by https://patchew.org/QEMU/20210903091031.47303-1-jasowang@redhat.com/ | |
| 15:13:02 | sean-k-mooney | https://github.com/qemu/qemu/commit/402378407dbdce79ce745a13f5c84815f929cfdd | |
| 15:13:24 | sean-k-mooney | so that is qemu v6.2.0 and later | |
| 15:14:06 | vsaienko | yes, looks like its supported now | |
| 15:14:49 | vsaienko | and need to check mtu, it seems we need to set it on xml as well | |
| 15:15:03 | vsaienko | because right now I can't use more than 1500 inside guest | |
| 15:15:16 | sean-k-mooney | we do suport settign the mtu i think | |
| 15:15:29 | sean-k-mooney | alhtogh that is one fo the thing that will conflict with the vdpa tool | |
| 15:16:18 | sean-k-mooney | our min qemu is currently 6.2.0 https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L221 | |
| 15:16:28 | sean-k-mooney | so we dont have to check if that supprot vdpa multi queue | |
| 15:16:46 | sean-k-mooney | but we do have to confirm whne the libvirt supprot actully hiped and if its in 8.0.0 | |
| 15:17:53 | vsaienko | libvirt change is in 8x version https://bugzilla.redhat.com/show_bug.cgi?id=2024406 | |
| 15:18:12 | sean-k-mooney | ya i was looking at that but that 8.2.0 | |
| 15:19:38 | vsaienko | we do suport settign the mtu i think - does it mean that we can't use jumboframes with vdpa? | |
| 15:20:42 | vsaienko | on 8 libvirt indeed setting mtu for vdpa is not possible https://paste.openstack.org/show/bi1VqvW3ScWE5obNpaRu/ | |
| 15:20:45 | sean-k-mooney | so the mtu will only be set if the mtu on the neutorn netowrk is not 1500 | |
| 15:21:10 | sean-k-mooney | i tought https://issues.redhat.com/browse/RHEL-7298 refence both mac and mtu but apprently not | |
| 15:22:11 | vsaienko | yes they closed https://bugzilla.redhat.com/show_bug.cgi?id=2057748 as duplicate but forgot to implement? | |
| 15:22:36 | vsaienko | on nova side mtu is not configured for vdpa device at all | |
| 15:22:50 | vsaienko | even when neutron network has > 1500 | |
| 15:22:51 | sean-k-mooney | ack the its beacuse of that orgial limiation | |
| 15:23:12 | sean-k-mooney | right so i dont know if that was also fixed in libvirt/qemu or not | |
| 15:25:29 | sean-k-mooney | vsaienko: so ya https://github.com/libvirt/libvirt/commit/a5e659f071ae5f5fc9aadb46ad7c31736425f8cf | |
| 15:25:42 | sean-k-mooney | libvirt supprot for mutlqueue came in 8.2.0 | |
| 15:26:04 | sean-k-mooney | that means we need to either rais our min version to libvirt 10 which is declared as or next min version | |
| 15:26:24 | sean-k-mooney | or you woudl have to support libvirt 8.0.0 by checkign the local version before enabling it | |
| 15:26:54 | sean-k-mooney | bumping the min version is the better chocies btu mroe work | |
| 15:27:18 | sean-k-mooney | we now know the minium requiremts at lesat qemu:6.2.0 and libvirt:8.2.0 | |
| 15:27:37 | sean-k-mooney | that woudl need to be captured in the release notes | |