Wednesday, 2025-11-12

opendevreviewSteve Baker proposed openstack/nova master: Add VNC console support for the Ironic driver  https://review.opendev.org/c/openstack/nova/+/94252801:48
opendevreviewNicolai Ruckel proposed openstack/nova master: Preserve UEFI NVRAM variable store  https://review.opendev.org/c/openstack/nova/+/95968207:39
opendevreviewRajesh Tailor proposed openstack/nova-specs master: Show finish_time field in instance action show  https://review.opendev.org/c/openstack/nova-specs/+/92978007:41
nicolairuckelsean-k-mooney, sorry for the delay. I didn't realize that my responses were just drafts until today. But I think I'm finally getting the hang of Gerrit.08:44
sean-k-mooneynicolairuckel: :)08:44
sean-k-mooneyya the fact they are nto sent until you hit reply is slightly difefnt then gitlab and github which send them by defualt unless you explcitly tell it your doign a review08:45
nicolairuckelIt's funny because I used to complain about their default because it creates so many notifications. :D08:45
sean-k-mooneynicolairuckel: sound like at least localy you not whave a mostly working version.08:46
sean-k-mooneynicolairuckel: ya i dnot use notifcation on github basically ever08:46
sean-k-mooneyits an entirly broken feature given the signal to noise ratio08:46
sean-k-mooneynicolairuckel: so what im thinking currently is we fix nvram preservation for rebooo. we do the saem for vtpm08:48
nicolairuckelSame for GitLab where people just ignore every email and you have to notify them separately if there are relevant updates to a merge request.08:48
nicolairuckelsean-k-mooney, sounds good to me08:48
sean-k-mooneythen we proably will need a spec to fix cold migration and maybe shelve as i think that will need net new desigin work08:48
nicolairuckeland then we can look into the cold migration separately as that seems a lot more complicated08:49
nicolairuckelright08:49
sean-k-mooneythe cold migration part is actully not hard and might be a blueprint or bug08:49
sean-k-mooneyshelve is harder because we need to decised where we woudl store the tpm and nvram data while the vm is not on a host08:49
nicolairuckelah, I didn't look into that yet but that makes sense to me08:49
sean-k-mooneyfor cold migration we just need to copy the file to the correct location and deal with deleting it when you confirm ro revert08:50
nicolairuckelsimilar to the old version of my patch?08:51
sean-k-mooneykind of but diferent08:51
sean-k-mooneywe will need to use the removeet filestyem supprot to copy the file between hosts rahter then within the same host08:52
nicolairuckelI see08:52
sean-k-mooneythis https://github.com/openstack/nova/blob/master/nova/virt/libvirt/volume/remotefs.py#L172-L31608:54
sean-k-mooneywe just need to use create_dir and copy_file08:54
sean-k-mooneyand then that will use ssh or rsync to move it08:55
sean-k-mooneydepend9ign on which you ahve enabeld08:55
sean-k-mooneywe do that via https://github.com/openstack/nova/blob/master/nova/virt/libvirt/utils.py#L314-L35108:55
sean-k-mooneyfor cold migraiton you will jsut pass the host paramater08:56
nicolairuckelthanks, I will look into that08:57
nicolairuckelthen I'll update the commit message of my patch accordingly08:58
sean-k-mooneyill see if i can test this again once you have the new verison with the release note up although proably not today08:58
nicolairuckelah, how do I do the release note? I wasn't able to find documentation on that yet.09:00
sean-k-mooneygiven what you knwo know for this it woudl be good to sync with your coworker on updating https://review.opendev.org/c/openstack/nova/+/95565709:00
sean-k-mooneyah its pretty simple to create one do tox -e venv reno new <some-string-with-dashes>09:01
sean-k-mooneytox -e venv -- preserve-nvram09:01
sean-k-mooneythat will tempelate out a new file in the release notes folder09:01
sean-k-mooneyyou can delete 99% of the yaml content but keep the --- and the fixes section09:02
sean-k-mooneythen just wirte a short operator focused descrtipion of what was fixed09:02
sean-k-mooneyyou can then test it locally with tox -e releasenotes09:02
sean-k-mooneyhttps://docs.openstack.org/nova/latest/contributor/releasenotes.html09:03
nicolairuckelwe agreed on that this is a bugfix, right/09:10
opendevreviewNicolai Ruckel proposed openstack/nova master: Preserve UEFI NVRAM variable store  https://review.opendev.org/c/openstack/nova/+/95968209:15
nicolairuckelokay, then it should be ready for you to test now09:15
sean-k-mooneynicolairuckel:just realiased what version of libvirt you were checkign agaisnt sorry 09:41
sean-k-mooneywe only suppport libvirt 8.0.0 and above so we do not need to check if preserving the nvram is supproted09:41
sean-k-mooneynicolairuckel: for the vtpm change this was needed because it need 8.9.009:42
sean-k-mooneybut nvram only needs 2.3.009:42
nicolairuckelah, then I can remove that09:44
sean-k-mooneyyes im also wonderign about these changes https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/functional/libvirt/test_uefi.py09:45
sean-k-mooneyis this a side effect of when you remvoed os.append(nvram)09:45
sean-k-mooneywhen your testing the generation fo the xml https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/unit/virt/libvirt/test_config.py09:46
sean-k-mooneyyou are assert ign the nvram element is present which is correct09:46
sean-k-mooneyit still feels like you have mroe changes tehn are required but i have not gone line by line to verify09:47
nicolairuckelyou're right about the first one for sure.09:48
nicolairuckelLet me change that, the version check and run the tests locally again09:49
sean-k-mooneyack. 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 well09:49
nicolairuckelI guess I can remove everything to _may_keep_nvram then, right?09:51
sean-k-mooneyya09:52
sean-k-mooneywhat 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 changes09:52
sean-k-mooney- the _may_keep_nvram bits then run the test and see what breaks09:53
sean-k-mooneythat will tell you which parts of the rest of the chagne are actully needed 09:53
sean-k-mooneyhttps://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.py09:54
sean-k-mooneyi dont think you need https://review.opendev.org/c/openstack/nova/+/959682/14/nova/tests/unit/virt/test_virt_drivers.py09:55
sean-k-mooneybut maybe09:55
sean-k-mooneyhttps://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 is09:56
sean-k-mooneyso i fyou can confirm that it woudl be good09:56
nicolairuckelI'll do that.09:56
nicolairuckelI'm not sure if I'll be able to do that today though.09:57
sean-k-mooneythats ok just let us know when it ready09:58
opendevreviewsean mooney proposed openstack/nova master: add functional repoducer for bug 2048837  https://review.opendev.org/c/openstack/nova/+/91997910:32
opendevreviewsean mooney proposed openstack/nova master: ensure correct cleanup of multi-attach volumes  https://review.opendev.org/c/openstack/nova/+/91632210:32
sean-k-mooneygibi: bauzas  i realsised that ^ is still open. woudl ye mind looking at those again 10:33
bauzassean-k-mooney: ack I can try10:33
gibisean-k-mooney: added inline comments in the fix11:34
sean-k-mooneygibi: so many of those comemnt i dismissed as wont do in the previous revision11:37
sean-k-mooneypartly because if dont wnat to reopen the formating holy wars11:37
sean-k-mooneywell these comments https://review.opendev.org/c/openstack/nova/+/916322/8/nova/compute/manager.py im still readying the rest11:38
sean-k-mooneygibi: good question on the thread case.11:42
sean-k-mooneywhat i can do is aquire the named lock before i add it to the list11:42
sean-k-mooneythat way i think only one thread will be able to do that11:43
sean-k-mooneyso ill reorder https://review.opendev.org/c/openstack/nova/+/916322/8/nova/utils.py#1196 and 119711:43
sean-k-mooneythat shoudl fix that.11:43
gibisean-k-mooney: Im not sure that lock is enough12:21
sean-k-mooneywell the sharign between thread you are descibign is not inteded to be supprote dbut ill comment more on the review12:23
gibithere 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 list12:23
gibithe doc of FairLockGuard does not really explain what is supported and what is not12:24
sean-k-mooneyya i can improve that12:24
sean-k-mooneyto me its a logic bug to share a single instance of a context manager between treads12:28
gibiOK so T1 and T2 has its own context manager but both using the same list of names to aquire the same set of locks12:34
gibithat is the way we have shared locks but not shared context managers12:34
gibithat is not intuitive so needs to be documented 12:34
gibinormally you do `with self.lock` in both T1 and T2 so the context manager (=lock) is shared12:35
sahido/12:38
sahidI have noticed this function during reboot: https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L428612:38
sahidwe are in situation where we would like avoid to reboot the VMs12:39
sahidusually we use live-migration when we do have (expect) to fix issue like that12:39
sahidi '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 process12:40
sahidbasucally, somewhere around that point: https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L934212:41
sahidif that feel good for you I would provide a bug report and share a patch12:41
sean-k-mooneygibi: its pretty cheap for me to add that support12:46
sean-k-mooneygibi: so i can add it12:46
gibiOK12:47
*** haleyb|out is now known as haleyb12:55
sean-k-mooneygibi: 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 mins12:57
gibiack. I'm not advocating to make that work, I'm OK if we clearly document the usage pattern of the Guard13:28
gibiif the shared data is not the Guard just the list of names the Guard is intantiated with that is fine by me13:28
gibijust pointing out that 13:29
gibi`with threading.Lock()` is wrong while `with Guard(names)` is the correct way is non intuitive13:29
gibihence we need documentation13:29
sean-k-mooneyright13:29
sean-k-mooneythat the delta in usage13:30
sean-k-mooneyi need to push a patch to cyborg and venos to fix a requriement issue that i just fixed in watcher13:30
sean-k-mooneybut when im done ill update the patch again. thanks for reviewing13:30
gibicool thanks13:31
opendevreviewVasyl Saienko proposed openstack/nova master: Set vnic model and queue for VDPA  https://review.opendev.org/c/openstack/nova/+/96685514:21
opendevreviewVasyl Saienko proposed openstack/nova master: Set vnic model and queue for VDPA  https://review.opendev.org/c/openstack/nova/+/96685514:39
vsaienkosean-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 queue14:40
sean-k-mooneyvsaienko: yep i just -1 your patch14:41
sean-k-mooneyvsaienko: this si not a bug14:41
sean-k-mooneyit was not possibel to use multi queu whne we added vdpa supprot14:41
sean-k-mooneyand it was not implemented intentionally as a result14:41
vsaienkohm... but now its possible14:42
sean-k-mooneyso 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 well14:42
sean-k-mooneyvsaienko: rigth so as this is a feature requst it woudl normally need a bluefpirnt or a spec14:42
sean-k-mooneywe coudl do it under a bug in very limited cases but its not actully a bug from an upstream point of view14:42
vsaienkoack14:42
sean-k-mooneythe main gaps are when did this supprot land in the kernel and qemu14:43
sean-k-mooneywe need to document that both in a release note and in the vdpa docs14:43
sean-k-mooneydepending o nthe qemu veriosn we may also need to add a version check14:43
sean-k-mooneyvsaienko: have you tested packed ring format ?14:44
sean-k-mooneyvsaienko: one of the open quetion too14:45
sean-k-mooneyis what happen if you have more cpu then the VF that backs the vdpa device supprot in hardware queue14:45
sean-k-mooneyi.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 hardware14:46
sean-k-mooneyvsaienko: 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 spec14:48
sean-k-mooneyfor 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-R64214:49
sean-k-mooneynow we dont offically suprpot that in nova but we also dont not supprot it14:50
-opendevstatus- NOTICE: Zuul job log URLs for storage.*.cloud.ovh.net are temporarily returning an access denied/payment required error, but the provider has been engaged and is working to correct it14:50
sean-k-mooneyin the same way the number of quese for a sriov vf is determiend by how you allocate teh vf14:50
sean-k-mooneythat 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:51
sean-k-mooneyvsaienko: 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 matters14:52
vsaienkoI haven't check packed ring format14:53
vsaienkois 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
sean-k-mooneyack you change enabls opting in to that14:53
sean-k-mooneyvsaienko: in nova we set the queu count equal to the number of cpus when you enabel multi queue14:54
sean-k-mooneyso the question is fi the vdpa device only supprot 4 queue and you set it to 16 in qemu/libvirt what happens14:54
vsaienkoif the queue count in xml is higher than on VDPA device VM will see number that is configured on vdpa side14:55
vsaienkoI checked this14:55
sean-k-mooneyack so we woudl need to 1 agree thats ok, it shoudl be and 2 docuemnt that so OPs are not surpised by that14:55
sean-k-mooneywe 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 enebled14:56
sean-k-mooneythe 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#20614:57
sean-k-mooneyby the way did you intenilaly remove this https://review.opendev.org/c/openstack/nova/+/966855/1/nova/virt/libvirt/config.py form your v2?14:58
vsaienkoI remove because driver_name is set not to viritio there, and that config.py change is not needed15:06
sean-k-mooneyack15:07
sean-k-mooneyvsaienko: 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:10
sean-k-mooneyhttps://github.com/qemu/qemu/commit/402378407dbdce79ce745a13f5c84815f929cfdd15:13
sean-k-mooneyso that is qemu v6.2.0 and later15:13
vsaienkoyes, looks like its supported now15:14
vsaienkoand need to check mtu, it seems we need to set it on xml as well15:14
vsaienkobecause right now I can't use more than 1500 inside guest15:15
sean-k-mooneywe do suport settign the mtu i think15:15
sean-k-mooneyalhtogh that is one fo the thing that will conflict with the vdpa tool15:15
sean-k-mooneyour min qemu is currently 6.2.0 https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L22115:16
sean-k-mooneyso we dont have to check if that supprot vdpa multi queue15:16
sean-k-mooneybut we do have to confirm whne the libvirt supprot actully hiped and if its in 8.0.015:16
vsaienkolibvirt change is in 8x version https://bugzilla.redhat.com/show_bug.cgi?id=202440615:17
sean-k-mooneyya i was looking at that but that 8.2.015:18
vsaienkowe do suport settign the mtu i think - does it mean that we can't use jumboframes with vdpa?15:19
vsaienkoon 8 libvirt indeed setting mtu for vdpa is not possible https://paste.openstack.org/show/bi1VqvW3ScWE5obNpaRu/15:20
sean-k-mooneyso the mtu will only be set if the mtu on the neutorn netowrk is not 150015:20
sean-k-mooneyi tought https://issues.redhat.com/browse/RHEL-7298 refence both mac and mtu but apprently not15:21
vsaienkoyes they closed https://bugzilla.redhat.com/show_bug.cgi?id=2057748 as duplicate but forgot to implement?15:22
vsaienkoon nova side mtu is not configured for vdpa device at all15:22
vsaienkoeven when neutron network has > 150015:22
sean-k-mooneyack the its beacuse of that orgial limiation15:22
sean-k-mooneyright so i dont know if that was also fixed in libvirt/qemu or not15:23
sean-k-mooneyvsaienko: so ya https://github.com/libvirt/libvirt/commit/a5e659f071ae5f5fc9aadb46ad7c31736425f8cf15:25
sean-k-mooneylibvirt supprot for mutlqueue came in 8.2.015:25
sean-k-mooneythat means we need to either rais our min version to libvirt 10 which is declared as or next min version15:26
sean-k-mooneyor you woudl have to support libvirt 8.0.0 by checkign the local version before enabling it15:26
sean-k-mooneybumping the min version is the better chocies btu mroe work15:26
sean-k-mooneywe now know the minium requiremts at lesat qemu:6.2.0 and libvirt:8.2.015:27
sean-k-mooneythat woudl need to be captured in the release notes 15:27
vsaienkoack15:27
vsaienkodo we need additional check for libvirt version, because right now min libvirt is 8.0.0 and it is supported starting from 8.2.015:29
sean-k-mooneyyep i sasid that while you disconencted15:29
vsaienkoack15:29
sean-k-mooneywe either need to supprot 8.0.0 via a version check ro bump our min version15:29
sean-k-mooneywhcih we can also do this cycle15:30
sean-k-mooneyits a littel more work but we have 1 or 2 feature that it woudl simplfy15:30
bauzassean-k-mooney: gibi: I can join the eventlet meeting but honestly I don't have the context15:30
vsaienkoshould I add limitations section to and add mtu limitation here? https://docs.openstack.org/nova/latest/admin/vdpa.html15:34
sean-k-mooneyhttps://bugs.launchpad.net/nova/+bug/211911415:39
gibisean-k-mooney: dansmith: bauzas: Why does it make sense to use a named lock instance in the provide tree? https://github.com/openstack/nova/blob/b7d50570c7a79a38b0db6476ccb3c662b237f69b/nova/compute/provider_tree.py#L252 It was there from the beginning added by Jay but it does not make sense to me. This means that any instances of the ProviderTree class will share the same named lock instead of have 16:58
gibiits own lock. 16:58
dansmiththe providertree is always for a single compute provider root in placement (per process) right?16:59
gibiwe are deep.copying ProviderTree objects around within the same process17:00
dansmithright but they represent the same provider (tree) in placement right?17:00
gibibut a single ProviderTree represents a single compute17:00
gibihm17:01
gibimaybe ironic 17:01
gibiworking with multiple nodes per compute service17:01
gibihas multiple trees one for each node17:01
gibithe ProviderTree class allows multiplre roots for some reason :/17:02
bauzasyeah I remember sometimes before we said we could have multiple roots17:04
bauzasat least jay said it :p17:04
bauzasI can't unfortunately remember the usecase where we could have multiple roots per compute17:04
bauzasprobably shared storage or something like that17:05
gibiOK multiple roots is because the a single ProviderTree will have all the root providers that are connected to our compute root via placement aggregates17:06
gibiReturns a fresh ProviderTree representing all providers which are in the same tree or in the same aggregate as the specified provider, including their aggregates, traits, and inventories17:06
opendevreviewBalazs Gibizer proposed openstack/nova master: Fix ProviderTree copying with threading Lock  https://review.opendev.org/c/openstack/nova/+/95609117:23
opendevreviewBalazs Gibizer proposed openstack/nova master: [test]Further categorization of disabled unit tests  https://review.opendev.org/c/openstack/nova/+/95609217:23
opendevreviewBalazs Gibizer proposed openstack/nova master: Do not fork compute workers in native threading mode  https://review.opendev.org/c/openstack/nova/+/96546617:23
opendevreviewBalazs Gibizer proposed openstack/nova master: Compute manager to use thread pools selectively  https://review.opendev.org/c/openstack/nova/+/96601617:23
opendevreviewBalazs Gibizer proposed openstack/nova master: Libvirt event handling without eventlet  https://review.opendev.org/c/openstack/nova/+/96594917:23
opendevreviewBalazs Gibizer proposed openstack/nova master: Run nova-compute in native threading mode  https://review.opendev.org/c/openstack/nova/+/96546717:23
opendevreviewsean mooney proposed openstack/nova master: ensure correct cleanup of multi-attach volumes  https://review.opendev.org/c/openstack/nova/+/91632217:31
sean-k-mooneygibi: ^ i swap to a reader writer lock in the test too to show that it really fixes the issue17:32
sean-k-mooneyand hardened the context manager to technially work if shared across thread even if you shoudl not do that17:32

Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!