| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2025-11-12 | |||
| 08:58:10 | nicolairuckel | then I'll update the commit message of my patch accordingly | |
| 08:58:55 | sean-k-mooney | ill see if i can test this again once you have the new verison with the release note up although proably not today | |
| 09:00:01 | nicolairuckel | ah, how do I do the release note? I wasn't able to find documentation on that yet. | |
| 09:00:26 | sean-k-mooney | given what you knwo know for this it woudl be good to sync with your coworker on updating https://review.opendev.org/c/openstack/nova/+/955657 | |
| 09:01:08 | sean-k-mooney | ah its pretty simple to create one do tox -e venv reno new <some-string-with-dashes> | |
| 09:01:23 | sean-k-mooney | tox -e venv -- preserve-nvram | |
| 09:01:47 | sean-k-mooney | that will tempelate out a new file in the release notes folder | |
| 09:02:06 | sean-k-mooney | you can delete 99% of the yaml content but keep the --- and the fixes section | |
| 09:02:26 | sean-k-mooney | then just wirte a short operator focused descrtipion of what was fixed | |
| 09:02:46 | sean-k-mooney | you can then test it locally with tox -e releasenotes | |
| 09:03:44 | sean-k-mooney | https://docs.openstack.org/nova/latest/contributor/releasenotes.html | |
| 09:10:46 | nicolairuckel | we agreed on that this is a bugfix, right/ | |
| 09:15:39 | opendevreview | Nicolai Ruckel proposed openstack/nova master: Preserve UEFI NVRAM variable store https://review.opendev.org/c/openstack/nova/+/959682 | |
| 09:15:54 | nicolairuckel | okay, then it should be ready for you to test now | |
| 09:41:23 | sean-k-mooney | nicolairuckel:just realiased what version of libvirt you were checkign agaisnt sorry | |
| 09:41:50 | sean-k-mooney | we only suppport libvirt 8.0.0 and above so we do not need to check if preserving the nvram is supproted | |
| 09:42:31 | sean-k-mooney | nicolairuckel: for the vtpm change this was needed because it need 8.9.0 | |
| 09:42:42 | sean-k-mooney | but nvram only needs 2.3.0 | |
| 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 ? | |