Wednesday, 2025-08-27

opendevreviewTakashi Kajinami proposed openstack/nova master: Detect AMD SEV-ES support  https://review.opendev.org/c/openstack/nova/+/92568501:02
opendevreviewTakashi Kajinami proposed openstack/nova master: Add hw_mem_encryption_model image property  https://review.opendev.org/c/openstack/nova/+/92770601:02
opendevreviewTakashi Kajinami proposed openstack/nova master: libvirt: Launch instances with SEV-ES memory encryption  https://review.opendev.org/c/openstack/nova/+/92610601:02
opendevreviewTakashi Kajinami proposed openstack/nova master: Add functional test scenario for mixed SEV RPs  https://review.opendev.org/c/openstack/nova/+/95856201:02
opendevreviewTakashi Kajinami proposed openstack/nova master: Detect AMD SEV-ES support  https://review.opendev.org/c/openstack/nova/+/92568501:16
opendevreviewTakashi Kajinami proposed openstack/nova master: Add hw_mem_encryption_model image property  https://review.opendev.org/c/openstack/nova/+/92770601:16
opendevreviewTakashi Kajinami proposed openstack/nova master: libvirt: Launch instances with SEV-ES memory encryption  https://review.opendev.org/c/openstack/nova/+/92610601:16
opendevreviewTakashi Kajinami proposed openstack/nova master: Add functional test scenario for mixed SEV RPs  https://review.opendev.org/c/openstack/nova/+/95856201:16
*** mhen_ is now known as mhen01:17
opendevreviewTakashi Kajinami proposed openstack/nova master: Detect AMD SEV-ES support  https://review.opendev.org/c/openstack/nova/+/92568501:20
opendevreviewTakashi Kajinami proposed openstack/nova master: Add hw_mem_encryption_model image property  https://review.opendev.org/c/openstack/nova/+/92770601:20
opendevreviewTakashi Kajinami proposed openstack/nova master: libvirt: Launch instances with SEV-ES memory encryption  https://review.opendev.org/c/openstack/nova/+/92610601:20
opendevreviewTakashi Kajinami proposed openstack/nova master: Add functional test scenario for mixed SEV RPs  https://review.opendev.org/c/openstack/nova/+/95856201:20
opendevreviewMerged openstack/nova master: Remove logic for unsupported old libvirt/qemu  https://review.opendev.org/c/openstack/nova/+/95231804:43
opendevreviewMerged openstack/nova master: libvirt: Get info with abs path, rebase with rel path  https://review.opendev.org/c/openstack/nova/+/95503904:50
opendevreviewRajesh Tailor proposed openstack/nova master: Fix 'nova-manage image_property set' command  https://review.opendev.org/c/openstack/nova/+/94673304:54
opendevreviewRajesh Tailor proposed openstack/nova master: Fix concurrent migration of vms with multiattach volume failure  https://review.opendev.org/c/openstack/nova/+/87227804:57
opendevreviewRajesh Tailor proposed openstack/nova master: Fix instance vm_state during shelve  https://review.opendev.org/c/openstack/nova/+/93429405:02
opendevreviewRajesh Tailor proposed openstack/nova master: Update the api-ref for unshelve  https://review.opendev.org/c/openstack/nova/+/93805405:11
opendevreviewGhanshyam proposed openstack/nova master: Add service role in Nova policy  https://review.opendev.org/c/openstack/nova/+/95757805:26
opendevreviewMerged openstack/nova stable/2025.1: Don't reset port dns_name when shelving instances  https://review.opendev.org/c/openstack/nova/+/95621605:26
opendevreviewMerged openstack/nova stable/2025.1: Fix case-sensitivity for metadata keys  https://review.opendev.org/c/openstack/nova/+/94584205:27
opendevreviewMerged openstack/nova stable/2025.1: Fix disable memballoon device  https://review.opendev.org/c/openstack/nova/+/95295905:30
opendevreviewMerged openstack/nova stable/2025.1: Fix case sensitive comparison  https://review.opendev.org/c/openstack/nova/+/94584305:30
opendevreviewRajesh Tailor proposed openstack/nova master: Update api-ref for server create  https://review.opendev.org/c/openstack/nova/+/93753405:34
opendevreviewMerged openstack/nova stable/2025.1: [doc]Clarify where to set pci_in_placement  https://review.opendev.org/c/openstack/nova/+/95162405:35
opendevreviewMerged openstack/nova master: api: Separate volume, snapshot and volume attachments  https://review.opendev.org/c/openstack/nova/+/95234705:36
opendevreviewMerged openstack/nova master: tests: Use valid UUIDs for cinder resources  https://review.opendev.org/c/openstack/nova/+/95293505:36
opendevreviewRajesh Tailor proposed openstack/nova master: Fix KeyError on assisted snapshot call  https://review.opendev.org/c/openstack/nova/+/90078305:49
opendevreviewRajesh Tailor proposed openstack/nova master: Add upgrade status check for duplicate cell names  https://review.opendev.org/c/openstack/nova/+/90181005:59
opendevreviewMerged openstack/nova master: api: Only apply "soft" additionalProperties validation to requests  https://review.opendev.org/c/openstack/nova/+/95293606:04
opendevreviewMerged openstack/nova stable/2024.2: wrap wsgi_app.init_application with latch_error_on_raise  https://review.opendev.org/c/openstack/nova/+/94544006:04
opendevreviewMax proposed openstack/nova master: performance: reduce calls to libvirt / add caching  https://review.opendev.org/c/openstack/nova/+/92285506:43
opendevreviewFlorian proposed openstack/nova master: Add check for PCIe devices attach limit for volume and ports  https://review.opendev.org/c/openstack/nova/+/95558407:32
ralonsohsean-k-mooney, hello! qq regarding this: https://specs.openstack.org/openstack/nova-specs/specs/rocky/implemented/sriov-trusted-vfs.html07:58
ralonsohso if we have a VF bond inside the VM (with trusted VFs), it is possible to change the port MAC. But that will make the SR-IOV agent to fail. When the MAC-PCI address pair doesn't match, it returns a warning and forces a full resync07:59
ralonsohit will repeat this every cycle (that means, every 2 seconds, by default)07:59
ralonsohdid we considered somehow this scenario?? from the Neutron point of view08:00
opendevreviewBalazs Gibizer proposed openstack/nova stable/2024.2: [doc]Clarify where to set pci_in_placement  https://review.opendev.org/c/openstack/nova/+/95862308:00
gibisean-k-mooney: dansmith: bauzas: we have both green nova-next and unit test results for the nova-conductor eventlet removal change. See my comment in https://review.opendev.org/c/openstack/nova/+/957088/6#message-5d92177333ca556fc30c6934603d37ab55cd161c 08:27
gibisambork: ^^08:27
bauzasgibi: that's on my today's duties08:27
bauzasI'm finishing up the AMD SEV-ES series08:27
gibiI will do some local manual testing now and report back on the same patch08:27
bauzas++08:30
gibisambork: as far as I see this is the only remaining ask form yesterday https://review.opendev.org/c/openstack/nova/+/957088/6#message-cd81d9ce4afbc379e730f15d8df8334a5303e42f 08:32
samborkgibi, ack, thanks08:32
gibisambork: hm08:32
gibisambork: wait a sec08:32
gibisambork: I have a small thing in the parent of that https://review.opendev.org/c/openstack/nova/+/957424 so if we anyhow want to touch the top then I would like to fix the bottom as well in the same run08:33
gibisambork: I guess it is easier if I just go and do both changes in the same push08:34
samborkgibi, ok thanks a lot!08:34
gibisambork: also sean-k-mooney is happy that the doc/reno/zuul patch is separate so I will not squash that in https://review.opendev.org/c/openstack/nova/+/958575/108:34
gibibauzas: ^^ hold you reviews until I re-spin08:35
bauzasack, anyway, I was aiming to review only by the next 30 mins, no worries08:36
opendevreviewMerged openstack/nova stable/2024.1: Reproduce bug/2098496  https://review.opendev.org/c/openstack/nova/+/94510008:36
bauzastkajinam: I finished reviewing your series, nothing holding (I mostly +2d) except one breaking change you introduce by requiring a SEV trait08:42
bauzasI overlooked the spec and nothing was mentioned in the upgrade section, but I do feel that if we start requiring a trait, then we need to mention to the operator that he has to fully upgrade his computes then08:43
bauzasor he wouldn't get old computes supporting SEV too08:44
bauzasthat's what I called a behavioural change08:44
bauzassean-k-mooney[m]: when you're up, I'd like your thoughts on the rolling upgrade scenario with SEV nodes08:45
opendevreviewBalazs Gibizer proposed openstack/nova master: Ask for pre-prod testing for native threading  https://review.opendev.org/c/openstack/nova/+/95742408:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Switch nova-conductor to use ThreadPoolExecutor  https://review.opendev.org/c/openstack/nova/+/95708808:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Run nova-conductor in native threading mode  https://review.opendev.org/c/openstack/nova/+/95857508:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Allow to start unit test without eventlet  https://review.opendev.org/c/openstack/nova/+/95343608:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Run unit test with threading mode  https://review.opendev.org/c/openstack/nova/+/95347508:51
opendevreviewBalazs Gibizer proposed openstack/nova master: [test]RPC using threading or eventlet selectively  https://review.opendev.org/c/openstack/nova/+/95381508:51
opendevreviewBalazs Gibizer proposed openstack/nova master: [CI]Make nova-tox-py312-threading voting  https://review.opendev.org/c/openstack/nova/+/95579108:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Do not yield in threading mode  https://review.opendev.org/c/openstack/nova/+/95099408:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Make RBD Tpool usage conditional  https://review.opendev.org/c/openstack/nova/+/95608908:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Make libvirt Tpool proxying conditional  https://review.opendev.org/c/openstack/nova/+/95609008:51
opendevreviewBalazs Gibizer proposed openstack/nova master: Fix ProviderTree copying with threading Lock  https://review.opendev.org/c/openstack/nova/+/95609108:51
opendevreviewBalazs Gibizer proposed openstack/nova master: [test]Further categorization of disabled unit tests  https://review.opendev.org/c/openstack/nova/+/95609208:52
opendevreviewBalazs Gibizer proposed openstack/nova master: [vncproxy]Handle ssl.wrap_socket removal in py312  https://review.opendev.org/c/openstack/nova/+/95591508:52
opendevreviewBalazs Gibizer proposed openstack/nova master: Warn on long task wait time for executor  https://review.opendev.org/c/openstack/nova/+/95266608:52
gibibauzas: done. you can go wild now :)08:53
* gibi goes back to his local devstack running manual tests on the image cache with threading08:53
bauzasgibi: ack09:03
tkajinambauzas, the HW_CPU_X86_AMD_SEV trait is added even in compute nodes. https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L1322309:06
tkajinambauzas, the trait hasn't been unused really in scheduling but it has been added to compute nodes with sev enabled. so it does not break scheduling to old compute nodes with SEV09:06
bauzasoh, you're right, and in the reshape, you moved it to the nested RP09:06
tkajinamyes 09:06
bauzasmy brain forgot that while I was still reviewing it09:07
bauzassorry09:07
tkajinamno, np09:07
bauzaschanging my vote then09:07
bauzastkajinam: I also have a slight concern, about some deprecation release value you change09:07
bauzasnot related to SEV09:07
tkajinamI'll revert that change and push a new version09:08
tkajinamone sec09:08
tkajinam(the others are just rebased so +2 should be kept09:08
bauzascool09:09
opendevreviewTakashi Kajinami proposed openstack/nova master: Detect AMD SEV-ES support  https://review.opendev.org/c/openstack/nova/+/92568509:09
opendevreviewTakashi Kajinami proposed openstack/nova master: Add hw_mem_encryption_model image property  https://review.opendev.org/c/openstack/nova/+/92770609:09
opendevreviewTakashi Kajinami proposed openstack/nova master: libvirt: Launch instances with SEV-ES memory encryption  https://review.opendev.org/c/openstack/nova/+/92610609:09
opendevreviewTakashi Kajinami proposed openstack/nova master: Add functional test scenario for mixed SEV RPs  https://review.opendev.org/c/openstack/nova/+/95856209:09
bauzasand thanks for being present09:09
tkajinamnp and thank you for the review !09:10
gibisambork: bauzas: sean-k-mooney: dansmith: local testing of the image cache shows that we have a bug somewhere around executor shutdown as only one of the two hosts get the image if the executor size is 1, If I increase the executor size to 2 then both gets it. See logs in https://review.opendev.org/c/openstack/nova/+/957088/7#message-a1b56a536b88c1f4372a2d986364a1f61d5c75af09:17
bauzashmmm09:19
samborkgibi, ack, checking09:19
gibiIf I have to guess then it is a difference in behavior in executor shutdown when there are queued tasks between eventlet and threading executors09:20
gibibut I have to prove that...09:20
bauzastkajinam: saw my other concern about the nested RP deletion when the config changes ?09:24
bauzaswe should mention that in the docs09:25
opendevreviewTakashi Kajinami proposed openstack/nova master: Purge nested SEV RPs when SEV is disabled  https://review.opendev.org/c/openstack/nova/+/95862609:29
tkajinambauzas, yes I saw it... and think we can try ^^^09:29
samborkgibi, it maybe logic is a little bit different and in case of threadPoolExecutor we are not using work_queue in shutdown logic09:30
bauzasI had that approach first when fixing the GPU SRIOV RPs09:30
tkajinambauzas, I noticed one difference between vGPU RP and SEV RP. The former is named according to definition in nova.conf while the other is named statically so purging SEV rp should be possible09:30
bauzastkajinam: yes, you're correct09:30
bauzaswe statically define the list of RPs based on what the op provided09:31
bauzaswhile here, we automatically create the RPs based on what libvirt driver detects09:31
tkajinamyeah09:32
bauzasso yeah, we could do what you wrote (reporting total=0 so ensure_rps() will wipe it)09:32
bauzasnice catch09:32
tkajinamone challenge with vGPU is that it loose the name of RP when a record is completely removed from nova.conf09:32
tkajinamI mean nova loose *09:32
bauzasyes, this is config-driven09:32
tkajinams/loose/lose/09:33
tkajinamyeah09:33
tkajinambut for sev we know the exact names, as you said09:33
bauzasanyway, I'll quickly review your patch and then I'll see how much I can help gibi and sambork on their eventlet conductor patches09:33
tkajinam(I updated the functional tests to prove it, too09:33
bauzastkajinam: perfect, thanks09:33
gibiI confirmed it is the executor.sshutdow's behavior that is different https://paste.opendev.org/show/bmZZr707naiWYHMt7qim/ 09:35
gibiI feel this is a bug in futurist as python says all pending tasks should finish https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.Executor.shutdown09:36
gibiyepp python's own concurrent.future.ThreadPoolExecutor works as expected so this is a futuris bug. I will file it09:39
bauzasgibi: excuse my ignorance but I'm getting hard to find how you found that only one host got the image when comparing paste.opendev.org/show/b1TPrh7ESmcIqWuTZx3D/ and paste.opendev.org/show/bGwpu8My66GcyR8T12sW/09:40
bauzasI see that compute1 emitted a notification saying it cached the image but I have no clue where the other compute said nooope09:41
samborkok, so I assume that we are block here (creating some class witch will overwrite shutdown behaviour sounds like to much)09:41
gibiAug 27 09:07:07 aio nova-conductor[117076]: INFO nova.conductor.manager [None req-8fc723e3-ecc0-4ee3-a4aa-fa33aa2eaf94 admin admin] Preparing to request pre-caching of image(s) 342349b7-351a-470d-92ee-c99f581e55e7 on 2 hosts across 1 cells.09:41
bauzasI mean, I trust you but I want to be able to recognize that in logs09:41
gibiAug 27 09:07:11 aio nova-conductor[117076]: INFO nova.conductor.manager [None req-8fc723e3-ecc0-4ee3-a4aa-fa33aa2eaf94 admin admin] Image pre-cache operation for image(s) 342349b7-351a-470d-92ee-c99f581e55e7 completed in 4.54 seconds; 1 cached, 0 existing, 0 errors, 0 unsupported, 0 skipped (down) hosts09:41
bauzasoh the "1 cached, gotcha"09:42
bauzasthanks09:42
gibiif you  check the same log in the non threading case or in the executor=2 case the you will see the second log showing two cached hosts09:42
bauzasyup, saw it thanks09:42
gibialso the ls command from the two nodes shows that only aio got the image in _base compute1 does not09:42
gibisambork: yeah I feel the same, I don't think we should try to fix it in one and a half days and even if we have the fix it will be more surgery that I can expect folks will be able to review in short notice09:43
gibiI have to drop for a lunch now sorry...09:44
samborkgibi thanks for testing sorry that I didn't catch it09:44
gibino worries this is team work :)09:46
opendevreviewMerged openstack/nova stable/2024.1: Ignore metadata tags in pci/stats _find_pool logic  https://review.opendev.org/c/openstack/nova/+/94510110:04
opendevreviewMerged openstack/nova stable/2024.1: Update Nova bdm with updated swap info  https://review.opendev.org/c/openstack/nova/+/93761110:04
sean-k-mooneygibi: so i belive if you call shutdwon on an executor pool that has pending task it just discards them11:02
sean-k-mooneygibi: at least i belive that how the tread pools in the std lib concurrent.threadpool and process pool11:03
sean-k-mooneygibi: whil i could not express point to this in doc this is part of the reason i orginallys asked that we do not rely on shutdown for the pool to finish all work. i wanted to keep a list of all the future for the exectuted tasks then wait for them to  complete11:04
bauzassean-k-mooney: the SEV-ES is fully reviewed by me, I think we have a good opportunity to add that in Flamingo 11:06
sean-k-mooneybauzas: ok ill take a pass on it soon then11:07
sean-k-mooneyjust sarting a littel late today, i got new ubiquit netowrking gear and may have bene up  till 4 tweaking it....11:07
* sean-k-mooney tasty tasty coffee11:08
gibisean-k-mooney: the python stdlib and the greenpool in futurist behaves the same and waits for all pending tasks, the futurist threadpool only waits for the running tasks not the queued ones. I think this is a bug in futurist as it should mimick the stdlib behavior11:22
gibisean-k-mooney: and yes we agreed that we should refactor the logic to be nicer than shutdown 11:22
gibinow we will have plenty of time for it11:23
gibi:)11:23
gibistill I will file a bug to futurist as this is a problem even for others switching from GreenThreadPoolExecutor to ThreadPoolExecutor within futurist 11:27
sean-k-mooneygibi: are you goign ot unparent the untit test form the conductor change if we are not proceedign with it11:28
gibiyepp I will11:28
sean-k-mooneyack did you seem my comment about the db transaction issue11:28
gibiconductor will wait for Gazpatcho the unit test can go a bit if we want into F after FF11:28
sean-k-mooneyi think im just going to call that the GG release11:29
gibihaven't seen the db comment yet, I will check, I assume it is abotu an unit test issue11:29
gibisean-k-mooney: hehe GG sounds good :)11:29
sean-k-mooneyso there was only one failing test case in the non voting zuul job patch11:29
sean-k-mooneyit passed on the next 211:29
sean-k-mooneyso either that test need to be after the fixture change11:30
sean-k-mooneyor its flaky under threading and need more investigation11:30
sean-k-mooneyso i was going to suggest adding it to the exclude list11:30
sean-k-mooneyi was tired last night so i didnt dign into the error much11:30
sean-k-mooneygibi: i might do this as well but you may want to run it in a loop, im thinking of just letting the voting patch run in a loop executing unit test for an hour or so on my home server11:31
sean-k-mooneyactully i will kick that off now11:32
gibisean-k-mooney: I did run them in a loop locally. But I will investigate your comment 11:42
sean-k-mooneyit was exactly 1 failng test so it could be just a one off cause by some test to test interaction 12:00
opendevreviewTakashi Kajinami proposed openstack/nova master: Purge nested SEV RPs when SEV is disabled  https://review.opendev.org/c/openstack/nova/+/95862612:32
tkajinammy bad. I though I run the whole functional tests but I didn't (and didn't caught some tests also needing update )12:39
tkajinamI thought *12:39
ralonsohsean-k-mooney, hello! maybe you missed my message about SR-IOV trusted VFs and MAC changing (https://specs.openstack.org/openstack/nova-specs/specs/rocky/implemented/sriov-trusted-vfs.html)12:53
sean-k-mooneyralonsoh: ya i didnt see it but i think we talked about it in the past12:56
sean-k-mooneytrusted vf shoudl not allow the mac to be changed form the guest12:56
sean-k-mooneyi think we talked about there beign a bug in one of the intel drivers?12:56
sean-k-mooneythe tursted vf spec allow the VF to be in promisous mode12:57
sean-k-mooneyso it can recive traffic that is not destented for its mack12:57
sean-k-mooneybut its mac shoudl be still enforced by nova/neutron to be the neutron port mac12:57
sean-k-mooneythe kernel dirver is free to rest the mack to all 0000 when the vm is stopped but if it is started again it shoudl be programed by nova/libvirt to the requsted mac again12:59
sean-k-mooneychanges to things like link state in the guest or phsysical on the host shoudl not impact the mac at all12:59
sean-k-mooneychanign the mac in the guest shoudl not propagate to the vf in trusted mode and if it does i woudl consider that to be a security issue13:00
sean-k-mooneyspecificly in the vendors PF and VF driver13:00
ralonsohsean-k-mooney, but port bonding (and MAC changing) is also considered in the nova spec13:01
sean-k-mooneyyes an no13:01
sean-k-mooneywhat we were descibeing there13:01
sean-k-mooneywas the ablity to create a bond port in the guest with a diffent mac then what is on the VF13:02
sean-k-mooneyon the host13:02
sean-k-mooneyhow this used to work is if you change the mac on the guest interface the host mac woudl remain the same13:02
sean-k-mooneyin ip tools13:02
sean-k-mooneyor ifconfig13:02
sean-k-mooneybtu the mac on the packet that was sent on the wire would be updated 13:03
ralonsohsean-k-mooney, ahhhh I thought that if you add two interfaces to a bond (inside the VM) you also change the VF MAC13:03
sean-k-mooneyralonsoh: i think that is what has changed13:03
sean-k-mooneyyou can change the guest visable side13:03
ralonsohahhhh so you can update the bond MAC but it will eventually use the VF MAC outside the VM13:04
ralonsohI didn't know that13:04
sean-k-mooneybtu on niantic for exampel that never actully changed the mac on the host side form the perspecitive of libvirt or ifconfig13:04
sean-k-mooneyit would change the mac on the wire which wi why it worked btu the comtople plane view was nto updated13:04
sean-k-mooneyi woudl assume at some point they hooked up the mailbox protocol to propagate the mac change 13:04
ralonsohyeah, this is what is happening now13:05
sean-k-mooneyralonsoh: ideaaly the sriov nic agent would use the pci adddress not the the mac/name13:05
ralonsohand this is of course an issue (at least from Neutron point of view)13:05
ralonsohsriov agent uses the MAC/PCI tuple to identify a port13:05
sean-k-mooneyis this the cause of the customer bug where tehy were seeing the state flap13:06
ralonsohyes13:06
sean-k-mooneyah ok. im glad i updated that to the netwrokign team so :P13:06
sean-k-mooneyso ya i woudl consider just using the pci adress13:06
sean-k-mooneythe reason you check both is because of connectx-313:07
ralonsohso that would require a refactor of the sriov agent13:07
sean-k-mooneyin that generation of nic and only in that nic13:07
sean-k-mooneyyou could have 2 pfs with the same pci adress13:07
sean-k-mooneyit was dumb and broke a bunch of other things and no one does that anymore13:07
ralonsohthat's another story...13:08
ralonsohbut of course, that won't work with ML2/SRIOV13:08
sean-k-mooneyralonsoh: well just using the pci address shoudl work for ml2/sriov in general13:15
ralonsohsean-k-mooney, maybe, I need to check that. We need, at least, to write a warning if the expected MAC address is not set13:16
ralonsohhow do I know a VF is used if I the MAC is the only thing I can check (for a PCI address)13:16
sean-k-mooneyyou know its used by checkign the binding profile in the port13:16
sean-k-mooneywe pass the pci_slot13:17
sean-k-mooneyvar which has the full pci adress13:17
ralonsohyes but I mean, in the SR-IOV agent we are detecting the port binding checking that the VF in a specific PCI has the expected MAC13:18
ralonsohand the MAC is set by nova compute13:18
sean-k-mooneyya so it never actully needed to do that for what it worth13:18
ralonsohthat was the trigger to send the vif-plugged event13:18
sean-k-mooneyorginally the sriov nic againe was entirly optional and not needed for intel nics13:18
sean-k-mooneyit was created by melanonx ot all managing things like link state via the admin state of the port13:19
sean-k-mooneythe mac programming to this day is done by nova13:19
ralonsohor qos or changing the propagation flag13:19
ralonsohI'll raise this issue in the Neutron community, I'll create a LP bug13:20
sean-k-mooneyas with everythign else the l2 part of the networing (seting the mac) is done by nova based on the mac set in the neutron port13:20
sean-k-mooneyack13:20
sean-k-mooneyi think its fair to raise a warnign if it not the expectev version and its not using trusted VF13:20
sean-k-mooneyif it is then it shoud be info level IMO13:20
sean-k-mooneymaybe warning is valid but this is apprently vendor and drive rdependnet beahivor13:21
sean-k-mooneywarnings are normmly a call to arms for an operator to fix somethign and i dont think there is anything for them to fix in this case13:22
ralonsohwarning is better than forcing the resync13:22
sean-k-mooneyoh im not saying we shoudl resync13:22
ralonsohwell, if the mac is changed and doesn't match with the Neutron port one, this is indeed a warning13:23
sean-k-mooneyim just sayign the log message shoudl maybe be at a lower level13:23
sean-k-mooneyack13:23
ralonsohI'll raise the bug and propose a patch, we can decide this in the review13:23
sean-k-mooneyyou could also cosnider deprecating trusted_vf....13:23
ralonsohfor sure, that should only happen with trusted VF13:24
ralonsohif the port is not trusted, this is for sure something more serious13:24
sean-k-mooneyi dont like that feature becasue we stilll have not fixed the fact it required you to write data to a field only nova shoudl modify13:24
sean-k-mooneyi.e. there shoudl be a seprate trunsted vF extention that does not require you to mess with the binding profile13:25
sean-k-mooneysince humans shoudl never write to that13:25
ralonsohsorry, I don't get your point here13:25
ralonsohNeutron does not set the VF trusted flag13:26
sean-k-mooneythe trusted VF feature required you to requet it by writign data to the binding:profile field13:26
ralonsohahhh ok ok13:26
ralonsohnow I understand13:26
sean-k-mooneycorrect neutron does not a human has to inject data  into a filed that only nova should modify13:26
ralonsohwell, we have done this before (removing things from the binding profile)13:26
sean-k-mooneythere shoudl be a port extenion for it instead13:27
sean-k-mooneyyep13:27
ralonsohagree13:27
ralonsohok, I'll open another LP bug heheheheh13:27
sean-k-mooneyit woudl be nice to finally fix this so we dont need custom policy for the binding:profile filed13:27
sean-k-mooneythanks13:27
gibiI'm cancelling the eventlet sync today for the sake of the FeatureFreeze. I'm not tracking anything that is critical for the FF from the eventlet-removal series. The nova-conductor change is blocked by a bug in futurist so that is moved to GG.13:55
dansmithgibi: I'm just reading that now13:56
dansmithfuturist aside, we could just make a list of threads and wait for them manually right?13:56
gibidansmith: right13:56
dansmithprobably best to move to G anyway given it's not critical13:56
gibiwe have a list of futures we can wait on13:56
dansmithack13:56
gibiI will file a futurist bug as it might effect us or others elswhere when we shutting down executors13:57
gibibut that is also post FF :)13:57
* dansmith nods13:57
sambork_gibi, ack, I will work on that after ff then 13:57
*** sambork_ is now known as sambork13:58
opendevreviewTakashi Kajinami proposed openstack/nova master: Migrate MEM_ENCRYPTION_CONTEXT from root provider  https://review.opendev.org/c/openstack/nova/+/92181413:58
opendevreviewTakashi Kajinami proposed openstack/nova master: Detect AMD SEV-ES support  https://review.opendev.org/c/openstack/nova/+/92568513:58
opendevreviewTakashi Kajinami proposed openstack/nova master: Add hw_mem_encryption_model image property  https://review.opendev.org/c/openstack/nova/+/92770613:58
opendevreviewTakashi Kajinami proposed openstack/nova master: libvirt: Launch instances with SEV-ES memory encryption  https://review.opendev.org/c/openstack/nova/+/92610613:58
opendevreviewTakashi Kajinami proposed openstack/nova master: Add functional test scenario for mixed SEV RPs  https://review.opendev.org/c/openstack/nova/+/95856213:58
opendevreviewTakashi Kajinami proposed openstack/nova master: Purge nested SEV RPs when SEV is disabled  https://review.opendev.org/c/openstack/nova/+/95862613:58
ralonsohsean-k-mooney, just a heads-up: https://review.opendev.org/c/openstack/neutron/+/92606814:00
ralonsohwe are already sending the "trusted" field in the port dict14:00
ralonsohI think we don't set it always (if the flag is not present). But if the flag is True or False, we send this new field since 2024.214:01
dansmithgmaan: are you rebasing this for FF? https://review.opendev.org/c/openstack/nova/+/95757814:09
sean-k-mooneyralonsoh: oh nice14:24
sean-k-mooneyralonsoh: so if we have not picked that up yes on nova we shoudl supprot it14:24
sean-k-mooneyralonsoh: so ya looks like there are no nova patches to finish it14:25
dansmithyou know, I'm just realizing that we never merged this repro/fix for placement: https://review.opendev.org/c/openstack/placement/+/94546414:25
sean-k-mooneyralonsoh: so am i correct that htis can now be set on a port without modifying the binding:profile at all14:25
dansmithgibi: ^14:26
sean-k-mooneydansmith: ya i have that open in a tab form last week14:26
sean-k-mooneydansmith: i assume you would like to get that in this cycle14:26
gibidansmith: I can take a look quickly now14:26
sean-k-mooneyi can try and make time to review it too14:27
gibiunfortunately I missed it completly14:27
dansmithI wish it was already because the context is out of my head now.. but yes, would very much like to have it merged14:27
sean-k-mooneylooks like you adressed my main comments. i just discarded one i had pienidng because its not worth changign the patch for 14:28
sean-k-mooneydansmith: the answer to https://review.opendev.org/c/openstack/placement/+/945464/comment/0189c002_387845d8/ was just the test is doing to diffent things so i woudl have made it two tests14:29
sean-k-mooneyill just ack tthat comment and we can leave it as is14:29
sean-k-mooneythe repoducer looks fine ill need to spen a littel more time on the actual fix but ill do that later today14:32
ralonsohsean-k-mooney, yes, you can rely on this flag instead of the port binding14:32
ralonsohwe are setting now in both places (the field and port binding)14:32
ralonsohwe'll remove the last one (port binding) once accepted in Nova14:32
sean-k-mooneyack. ill add nova ot the bug and check with other if we are ok to fix and backport that as a security hardening bug14:33
sean-k-mooneyif not then ill propose doing it as a specless blueprint next cycle14:33
dansmithgibi: I was going to ping our PTL, but ... https://review.opendev.org/c/openstack/nova/+/95742414:42
gibidansmith: I'm +2 on both patch in placement about the overallocation. Thanks both was pretty clear.14:43
dansmithI wonder if we should point them to a specific outlet (or outlets) to report success14:43
dansmithgibi: sweet thanks14:43
gibidansmith: I can ask them to drop a mail to the ML or show up on IRC.14:44
gibiprobably ML is easier for them14:44
dansmithyeah the former is what I was thinking.. I know some may not even go that far, so I wish we had another venue but.. at least asking for success reporting seems like a good idea14:44
gibiI will respin and add it14:45
gibi(and in the same time I will remove the nova-conductor from the series)14:45
dansmithmaybe put bauzas' personal email and cell number (as our closest TC rep) in there if they don't want to join the ML :D14:45
gibilol :)14:45
bauzashmmm, WAT ?14:45
dansmithgibi: also why not just squash those renos into one?14:45
bauzas$context ?14:45
gibidansmith: about the renos there. We already have two from two independent patch but yeah I can try to merge them in that commit and see what our doc gen says about a deleted reno14:46
dansmithnot a big deal but when I read the second one I thought it was accidental duplication until I flipped back and forth a couple times to see14:46
dansmithgibi: oh right, duh, okay nevermind then14:46
dansmithusually renos in patches are new and I was reading it as if you were adding those.. no need to fix now I think14:47
gibiOK14:47
gibiUggla: ping :)15:01
Ugglagibi pong ! :)15:02
gibiprobably a netspilt happened between the IRC servers but those should be reported to us. At least in the univesity days that was somewhat frequent and that was a way to steal nicks :)15:02
bauzasgibi: dansmith: what was the problem with reno ? sorry, hardly following15:11
dansmithyeah I think after some netsplit my nick list just didn't recover.. I restarted my client and it's back now15:12
dansmithbauzas: no problem15:12
bauzasalso, I'd appreciate if some cores could give a look at tkajinam's series on SEV ES15:12
bauzasI reviewed that, Uggla found some typos that tkajinam will update but apart from that, we're in a very good spot to support next generation of AMD SEV :)15:12
dansmithbauzas: as I said, I'm not reviewing that before tomorrow.. I'm totally blank on it and don't think I'd be doing a good job in such a short time frame... just FYI15:12
bauzasdansmith: well, it was a general reminder to all of us, not particularly you who already said that indeed, cool15:13
dansmithyep, I know, just sayin' :)15:13
bauzasthere is also a quite simple feature patch worth reviewing, which is to copy our provider.yaml, I started reviewing it and I have an open question : https://review.opendev.org/c/openstack/nova/+/94830415:14
bauzaswe could fix that in a FUP if other cores like it15:14
gmaandansmith: RE 957578: ah it merge conflict, I will rebase it. it was all good until last night.  testing and everything done for this and it is good to review (once i rebase)15:15
dansmithack15:15
bauzasgmaan: I can give it a shot15:16
bauzasfwiw, the etherpad is updated with my findings https://etherpad.opendev.org/p/nova-2025.2-status15:16
bauzaspeople can use it for directing reviews15:16
lajoskatonabauzas, Uggla: Hi, perhaps you can help me out, we have a bug originally from a downstream customer: https://bugs.launchpad.net/nova/+bug/2051685  for migration15:29
lajoskatonabauzas, Uggla: it hit us few cycles back and than rubasov made an upstream reproduction which you can read in the bug description but he hit some walls in understanding the depths of Nova as you can read at the end of the report :-)15:30
lajoskatonabauzas, Uggla: if we should have a pointer if the issue is really an issue and can be fixed would be enough help to keep working on it but we lost how to dig deeper and if it is really something that can be / worth to be fixed15:31
Ugglalajoskatona, at least I can add this bug for review in our next bug scrubbing.15:33
lajoskatonaUggla: Thanks for your time, if any more info is needed we can check and add o course to the LP15:34
Ugglalajoskatona, I'll try to discuss it in next nova  meeting. But I can't promise we will have time, depending of the agenda.15:35
lajoskatonaUggla: no problem, thanks15:36
sean-k-mooneylajoskatona: so use reset state after a migration error si never corect16:15
sean-k-mooneyso if this only happens when you do that its not really a valid bug16:15
sean-k-mooneylajoskatona: also im surpesed we accepted --availability-zone :devstack016:16
sean-k-mooneyin new micorversion you ment to pass --host16:17
sean-k-mooneyif you jsut want to slect a host16:17
sean-k-mooneyi tought the az was intended to be requried and just passing :<host> was not  intended to be supprot but perhaps that worked by acidnet16:18
lajoskatonasean-k-mooney: reset state was only used to show that in devstack the issue can happen, as it happened in some customer site without proper logs and reproduction16:18
sean-k-mooneywe never really encurraged that usage 16:18
sean-k-mooneyright but reset state will break novas ablity ot properly clean up16:18
sean-k-mooneyas will restarting the nova-comapute durign the mighration16:19
lajoskatonathe issue is that when something happens with the destination host during migration the migration can stuck in error case 16:20
sean-k-mooneywell yo say stuck in error but i tough erro was a valid end state16:20
sean-k-mooneyso its not stuck its in the terminal state16:21
sean-k-mooneyor one of the terminal states16:21
lajoskatonaas rubasov added to the last lines as a thought experiment: "I don't see it yet, how this should be fixed. Maybe a migration should never be stuck indefinitely in status=error...."16:21
sean-k-mooneyfailed models a diffent endsate then error16:21
sean-k-mooneyerror is something went wrong that we cant recover form gracefully liek the dest host gets restarted16:22
lajoskatonayes of course16:22
sean-k-mooneyfailed is the end state when it somethig we can properly recover form16:22
sean-k-mooneylike we try to migrate and the cpu is incompatible16:22
sean-k-mooneyso the migration status being in error for ever is nto a bug to me16:23
sean-k-mooneythe orignal issue was caused by "overcommit on dedicated PCPUs and CPUPinningInvalid exception"16:23
sean-k-mooneyI.E. there was already invlaid sate in one or boht of the compute nodes16:24
lajoskatonaand the update_available_resource periodically fetches these also and tries to apply the migration that ended in error state but the repeated migration for the same server to the same host succeded so it should be find by the method that the migration is really done by a new migration (server uuid - dest host pair)16:24
sean-k-mooneyi.e. vms not runningn on the right core or the oepration modfied the nova config to change the core set16:24
lajoskatonaso the question is if we can say that ahhh, the same VM to the same host migration operation is finished so we can forget to stress on the previous migration (same vm uuid same host) that failed ?16:26
sean-k-mooneyin general no16:28
lajoskatonaso it is really a corner case as I see not error due to the things you mentioned (pcpu config or such) but like issue with the compute itself and after the fix the migration can be repeated 16:28
sean-k-mooneybut we liekly do have a bug in the logic16:28
sean-k-mooneyi dont thnk it woudl be correct for the erroed migration to be updated to failed16:29
sean-k-mooneyis shoudl stay in error16:29
sean-k-mooneybut if the vm  is not on this host we shoudl not process the old migration on new iterations16:29
sean-k-mooneyof the update_aviable_resouces perodic16:29
sean-k-mooneymovign it to fialed might achive that16:29
lajoskatonathat's fair to rethink it and make it clear and we can document it for customers in such case in worst case16:29
sean-k-mooneybut im not sure we can alwasy assume that is safe to do16:30
lajoskatonasounds great to think at least about it16:30
sean-k-mooneyyou were testing bfv was it using ceph by the way?16:31
sean-k-mooneyif so the already exist erro might eb related to https://docs.openstack.org/nova/latest/configuration/config.html#workarounds.ensure_libvirt_rbd_instance_dir_cleanup16:31
lajoskatonato tell the truth I am not sure but would say no (have to double check)16:31
sean-k-mooneyhttps://bugs.launchpad.net/nova/+bug/141489516:32
sean-k-mooneyis one of the rrefenece bugs16:32
sean-k-mooneywe dont generally delete the instnace storage on the dest on start up because we dont want ot loose data16:33
sean-k-mooneyi.e. incase the live migration actuly commpelted at teh libvirt level  while the agent was being restarted16:34
lajoskatonathanks I check this issue also if can be related16:34
sean-k-mooneywhen an migration ends in error it means an operator might need to fix the db16:34
sean-k-mooneyto update the host to bring nova back in sync with where the vm is. or carfully move the vm back ot the souce host16:35
sean-k-mooneyif a migration end in failed there shoudl be no action needed by an operator to fix anything16:35
opendevreviewGhanshyam proposed openstack/nova master: Add service role in Nova policy  https://review.opendev.org/c/openstack/nova/+/95757816:55
gmaanbauzas: thanks, rebased it ^^ dansmith sean-k-mooney ^^16:56
dansmithack16:56
bauzasgmaan: I'm a bit out of steam now, I'll look at it tomorrow morning16:57
gmaanno issue.16:57
sean-k-mooneygmaan: i did a test in cidner to see if i coudl make it work with the glance revert16:59
sean-k-mooneybut id dint work and i dont know why16:59
gmaansean-k-mooney: you mean nova change did not work with glance revert?17:00
gmaanor glance revert things only17:00
sean-k-mooneyno the cider oen i crfeated17:00
sean-k-mooneyi looke at how nova creates our glance client nad tried to do the same in cidner17:00
gmaanohk, i can check that. cinder to send right roles in nova, I can check for glance17:00
gmaani checked all those files recently so it will be quick, looking..17:01
sean-k-mooneyhttps://review.opendev.org/c/openstack/cinder/+/95858217:02
gmaansean-k-mooney: here is issue, cinder does not send the confiugured  service user to glance instead send the user token (context) + service token so user token does not have 'service' role - https://github.com/openstack/cinder/blob/89709ef589b74506885c72e42cad57eb8dedfb56/cinder/image/glance.py#L134C45-L134C5217:02
gmaanhere cinder should have first try to get the configured service user and that auth plugin (load from keystoneauth) they can send along with service token 17:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Ask for pre-prod testing for native threading  https://review.opendev.org/c/openstack/nova/+/95742417:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Allow to start unit test without eventlet  https://review.opendev.org/c/openstack/nova/+/95343617:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Run unit test with threading mode  https://review.opendev.org/c/openstack/nova/+/95347517:03
opendevreviewBalazs Gibizer proposed openstack/nova master: [test]RPC using threading or eventlet selectively  https://review.opendev.org/c/openstack/nova/+/95381517:03
opendevreviewBalazs Gibizer proposed openstack/nova master: [CI]Make nova-tox-py312-threading voting  https://review.opendev.org/c/openstack/nova/+/95579117:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Do not yield in threading mode  https://review.opendev.org/c/openstack/nova/+/95099417:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Make RBD Tpool usage conditional  https://review.opendev.org/c/openstack/nova/+/95608917:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Make libvirt Tpool proxying conditional  https://review.opendev.org/c/openstack/nova/+/95609017:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Fix ProviderTree copying with threading Lock  https://review.opendev.org/c/openstack/nova/+/95609117:03
opendevreviewBalazs Gibizer proposed openstack/nova master: [test]Further categorization of disabled unit tests  https://review.opendev.org/c/openstack/nova/+/95609217:03
opendevreviewBalazs Gibizer proposed openstack/nova master: [vncproxy]Handle ssl.wrap_socket removal in py312  https://review.opendev.org/c/openstack/nova/+/95591517:03
opendevreviewBalazs Gibizer proposed openstack/nova master: Warn on long task wait time for executor  https://review.opendev.org/c/openstack/nova/+/95266617:03
sean-k-mooney gmaan  that what i was tryign to do here https://review.opendev.org/c/openstack/cinder/+/958582/1/cinder/image/glance.py#14517:04
sean-k-mooneylet me update that quickly to get rid of the pep8 errors so its readable17:04
gmaansean-k-mooney: but you need to load service user token from keystoneauth and pass it to https://review.opendev.org/c/openstack/cinder/+/958582/1/cinder/image/glance.py#14817:05
gibidansmith: fixed the doc pre-prod testing doc patch to ask for a mail. 17:05
dansmithgibi: already approved17:06
dansmiththanks17:06
gibidansmith: thanks!17:06
gibisean-k-mooney: you threading unit test finding was valid, I missed that in our exclude list in the first patch and only added later. Now it is excluded from the begining17:06
gmaansean-k-mooney:  otherwise auth going to keystone sessions is still user token (with no service role) + service token - https://review.opendev.org/c/openstack/cinder/+/958582/1/cinder/image/glance.py#15417:06
sean-k-mooneygmaan: that what i was tryign to do by having the session object constucted form teh config 17:07
sean-k-mooneyim obvioulsy missing a step but im tring to use the user token form the config not the actual user17:08
gmaanI think keystonemiddleware do fetch the headers from auth plugin and populate the context with those data but let me check17:09
opendevreviewmelanie witt proposed openstack/nova stable/2025.1: libvirt: Get info with abs path, rebase with rel path  https://review.opendev.org/c/openstack/nova/+/95867417:10
gmaansean-k-mooney: but yes if you can cleanup those pep8, i can read it more clearly 17:10
sean-k-mooneyim doing that as we speak17:10
sean-k-mooneygibi: ack cool i was hoping it was just that17:10
sean-k-mooneygibi: so its still exclude and or passing in the later patches ya?17:11
opendevreviewBalazs Gibizer proposed openstack/nova master: Switch nova-conductor to use ThreadPoolExecutor  https://review.opendev.org/c/openstack/nova/+/95708817:12
opendevreviewBalazs Gibizer proposed openstack/nova master: Run nova-conductor in native threading mode  https://review.opendev.org/c/openstack/nova/+/95857517:12
gibisean-k-mooney: yepp it was excluded later, but now it is excluded from the first patch :) thanks for catching it17:12
gibithe conductor series is now move top of master out from the unit test series17:13
sean-k-mooneyack ill loop back over that shortly17:13
gibithanks. I'm filing the futurist bug now and the end my day17:13
sean-k-mooneyi need to actully finish at leat the first patch in the sev series17:13
sean-k-mooneyim going to be aroudn for amybe another hour doign review possible a little more17:14
sean-k-mooneygmaan: i didnt have a chance to revew the dnms but did you get clean runs on the service role the last time17:14
gmaansean-k-mooney: yes, all good. stable job on tempest change was failing in experimental pipeline so I fixed tests to continue using admin on stable and service on master17:15
sean-k-mooneyi was mostly happy with your nova change last time i looked so i think we can likely land that today or tomorrow17:15
sean-k-mooneydid you see my quetion on the tempest change17:16
gmaanI fixed your doc/release notes  comment too17:16
gmaanoh, not yet, checking17:16
sean-k-mooneybaout wither the primary role could be service?17:16
sean-k-mooneyinstead of admin17:16
gmaansean-k-mooney: oh, that one i replied yes17:16
sean-k-mooneyhttps://review.opendev.org/c/openstack/cinder/+/958582 now passes pep817:17
gmaanits not admin. actually 'credential' var in tests is little weird and unreadable 17:17
gmaancredentials = ['primary', 'admin', ['service_user', 'admin', 'service']] means: it create three users - 1. user with member role (primary), 2. user with admin (admin), 3. user with admin and saervice role (service_user)17:18
gibihere is the futurist bug https://bugs.launchpad.net/futurist/+bug/212154517:18
sean-k-mooneythis was a case of "i dont understand how to read that list" so i was just asking why primary was admin for what its worth17:18
sean-k-mooneyoh17:19
sean-k-mooneythats how you read it17:19
sean-k-mooneygmaan: ok that not what i was expectign at all ok17:19
gmaanyeah, it take, normal predefined users (primary, admin, reader, alt_admin etc), users with coustom name and roles so it is not so easy to read.17:19
gmaanand with historic alias in tempest, primary == member etc make it more unreadable17:20
gmaanI think we need to get rid of primary and use member consistently 17:20
sean-k-mooneyya a dict of username to list of roels would be simpler also17:20
sean-k-mooneyya the indirection though primary also was not somethign i was aware of17:21
gmaanit keep becoming complex with more personas and we need to keep old predefined personas for compatibility 17:21
sean-k-mooneyi assme that was to deall with "member" vs "_member_"17:21
gmaanself.os is another name of promary/member but we do not use that in many tests :)17:22
gmaanbut I have that cleanup in my list17:22
gmaanat least use latest personas name in tests and keep old one just for compatibility if tempest plugin are using17:23
sean-k-mooneyya that woudl help17:23
sean-k-mooneyso i need to finsh the sev revew but yuou thin the issue is https://review.opendev.org/c/openstack/cinder/+/958582/2/cinder/image/glance.py#14817:24
sean-k-mooneyevne though i am creating the session form the config now its still not useing tha tfor the user toekn an pulling that form the context17:24
sean-k-mooneynova does https://github.com/openstack/nova/blob/master/nova/service_auth.py#L33-L55  cidner does almost the same https://github.com/openstack/cinder/blob/master/cinder/service_auth.py#L80-L9117:27
gmaansean-k-mooney: you are creating the sessions from conf but passing the auth plugin to keystonauth https://review.opendev.org/c/openstack/cinder/+/958582/2/cinder/image/glance.py#14517:27
sean-k-mooneyif you have na idea of how to properly udpat that patch or can comment i can respien it later17:28
sean-k-mooneybut i see i can pass auth as a second parmater17:28
sean-k-mooneyso i just need to find an incanation to copy past for that17:28
gmaansean-k-mooney: that is all ok but in that method nova pass the loaded conf service user and cinder just context so user aith plugin loaded from user context17:28
gmaanyes17:28
gmaanfor nova case this is here cinder load the internal service user from conf https://github.com/openstack/cinder/blob/master/cinder/compute/nova.py#L98-L10017:29
gmaanand wrap that with service token in get_auth_plugin17:29
gmaanand then keystonmiddlware gets the right user_token and service_token headres and populate the requext context which is passed to services17:30
sean-k-mooneyoh so i need basiclly all of this right https://github.com/openstack/cinder/blob/master/cinder/compute/nova.py#L98-L11817:31
gmaanyes, you can do all. so like cinder call nova based on what they call they can with pass privileged_user to true or not17:32
sean-k-mooneyand ideally cinder would have a glance section like the nova one but i could steal the nova option or the keystone_auth ones just to who it works17:32
sean-k-mooneywhat im trying to show is that with a change to how cidner calls glance the revert is valid to show that its a cidner bug now a glance one17:33
gmaansean-k-mooney: that can be wrong but that is why I am checking in cinder code if they register the keystoneauth options for glance section also like they do for nova17:33
sean-k-mooneyif i can demonstrate tha then we coudl fix it properly17:33
sean-k-mooneygmaan: no not that i can see form there config ref17:33
gmaansean-k-mooney: I checked what all method cinder should pass user token (like they do currently) and I think these are the two call cinder need to pass config service user to glance - https://github.com/openstack/cinder/blob/master/cinder/image/glance.py#L345-L37017:36
gmaanso they can pass  privileged_user =True in common mehtod you mentioned to copy from nova file to glance17:37
gmaanthose are two glance call have service role for their policy chgeck https://github.com/search?q=repo%3Aopenstack%2Fglance+base.SERVICE&type=code17:38
gmaanrest all other call can continue using the user token. for example, this is nova case where cinder get servers volume from nova and just call it with user token (not configured service user) + service token https://github.com/openstack/cinder/blob/master/cinder/compute/nova.py#L243-L24617:39
sean-k-mooneyok i was hopign thiw woudl take me a few second to poc 17:39
sean-k-mooneybut it will take me more time then i have 17:40
sean-k-mooneygmaan: is this somethign you could try?17:40
sean-k-mooneygmaan: my concern is releaseing with the glance policy change 17:40
gmaansure, let me update it17:40
sean-k-mooneythanks17:41
gmaanis glance policy changed in this cycle or they are released last cycle?17:41
sean-k-mooneythe one in this cycle that was doen last week17:41
sean-k-mooneythe thing this reverts https://review.opendev.org/c/openstack/glance/+/95856717:42
gmaanbecause that can create issue (leak/security etc) where user with no privilege to get/add location can do it via cinder 17:42
gmaanohk, cook17:42
gmaancool17:42
gmaanlet me update your cinder change17:42
gmaansean-k-mooney: it is more work then that, cinder does not have service user configuration for glance so we need to 1. add those new conf in cinder 2. set those in devstack () and document it for operator to do the same) 3. load and pass that configured user to glance serviec APIs call17:57
gmaanI can push the changes but I am not sure all these can make it before FF17:57
gmaanwhoami-rajat_:  ^^ just CC you as you are involved in the service role stuff 17:58
gmaananyways we should do this discussion in glance or cinder channel and can decide what best can be done before FF17:59
whoami-rajat_i just want to mention that currently cinder passes service_roles:service and not role:service so please modify the policies accordingly to support both scenarios (role:service OR service_roles:service) until we reach a consensus on the right way to do it17:59
whoami-rajat_I'm trying to get in my patch to adopt GET location API which relies on the above ^18:00
gmaansean-k-mooney: and that is released in 2025.1 https://github.com/openstack/glance/blob/stable/2025.1/glance/policies/base.py#L11618:01
sean-k-mooneywhoami-rajat_: so service_roles: service is refering to roles on teh service token18:02
sean-k-mooneywhich is not somehtign that glance or nova shoudl be checkign in this context18:03
gmaanwhoami-rajat_: there is one issue in that. any user who is not allowed to get/add image location and can call cinder API will be able to get/add image location because  service_role: will always be successful rule in glance18:03
gmaanwhoami-rajat_: I try to add all use case here (5th reply from last )  https://review.opendev.org/c/openstack/nova/+/957578/comment/a097d558_fee01375/18:03
whoami-rajat_it shouldn't be if we haven't configured the [service_user] send_service_user_token = True18:04
sean-k-mooneythat shoudl more or less always be true18:04
gmaansean-k-mooney: yes18:04
gmaanwhoami-rajat_: operator need to do that which is for other purpose than RBAC. user token expiry use case18:05
sean-k-mooney^ that18:05
gmaanboth are for service to service communication but for separate purpose. that is what I was also confused with and was doing it in nova until sean-k-mooney corrected me18:06
whoami-rajat_I understand, just my main focus is not to break existing functionality, glance and cinder (and even nova) has been interacting in this way so we should keep the behavior when implementing the new one18:06
gmaansean-k-mooney: so 2024.2 policy was correct, I am wondering how it was working in cinder and your revert which is logically same as this is failing?  https://github.com/openstack/glance/blob/stable/2024.2/glance/policies/base.py#L11418:07
whoami-rajat_and TBH, i really don't like the redundancy as highlighted here https://review.opendev.org/c/openstack/glance/+/958567/comments/37c4390c_90f23c9d18:07
sean-k-mooneywhoami-rajat_: the current glance bevharie as fo 7 days ago or wheneve ris portiotaly a security problem18:07
sean-k-mooneyits using the service token for something ti was not intended to be used for18:07
sean-k-mooneywe did a one off exccption for the cinder cve 18:07
sean-k-mooneybtut that is not a policy check via the polciy framwork18:08
sean-k-mooneythat a special case that woudl eventually go away18:08
sean-k-mooneyby just checkign for the service role on the standard token18:08
gmaanwhoami-rajat_: just imagine one scenario. cinder calling to glance will ALWAYS pass the policy. so glance policy is basically noop 18:08
gmaansend_service_user_token has to be true always irrespective of RBAC, as you know - https://docs.openstack.org/cinder/latest/configuration/block-storage/service-token.html18:09
whoami-rajat_I understand and I'm not questioning that, but cinder doesn't support the new way to configure a glance section, read from it and pass role:[service] in request18:10
whoami-rajat_and i doubt it does for nova either, haven't tried that18:10
whoami-rajat_so it puts cinder in a condition where we can't really adopt the service role18:11
sean-k-mooneythere is code for nova18:11
gmaanwhoami-rajat_: oh, yeah that's true for glance. but It does not for nova -  https://github.com/openstack/cinder/blob/master/cinder/compute/nova.py#L54-L5918:11
sean-k-mooneywhoami-rajat_: you can use the service user section or keystone_auth section or nova section in the absence of a glance section18:11
gmaanand this is where it is set in conder conf by devstack https://zuul.opendev.org/t/openstack/build/c3c5a139509b47748977f00985b18d2e/log/controller/logs/etc/cinder/cinder_conf.txt#74-8518:11
whoami-rajat_ah yes, because we used to send the admin request to nova for operations, now that admin creds with be switched to service one18:12
sean-k-mooneywhoami-rajat_: exactly18:12
whoami-rajat_yeah there was no use case for glance so we never did it18:12
whoami-rajat_so what would be the next steps here?18:13
whoami-rajat_Cinder can't really adopt the new location APIs then, at least in 2025.218:13
sean-k-mooneythe best outcome woudl be to add a new galnce section adn fallback to the keystone_auth or service_user section when not defiend18:13
whoami-rajat_if we revert the glance changes ^ which i think now is right to do18:13
gmaanwhoami-rajat_: sean-k-mooney before going to next step, I would like to know how it was working in stable/2024.2 where glance policy is same as sean-k-mooney revert and cinder failing in revert - https://github.com/openstack/glance/blob/stable/2024.2/glance/policies/base.py#L11418:13
sean-k-mooneyboth of those will have the corect roels18:13
whoami-rajat_in 2024.2, we didn't adopt the location APIs18:14
gmaanohh18:14
whoami-rajat_so the feature in glance was merged in 2024.2 but we adopted it in 2025.2 (we=nova and cinder)18:14
gmaangot it18:14
sean-k-mooneythe nova change mergd before the glance plicy change however right18:15
whoami-rajat_yes18:15
whoami-rajat_the policy change is recent, last week18:15
gmaanbest way to not break user and provide option to adopt new way also can be:18:15
sean-k-mooneynova worked because we had an "admin" client18:15
sean-k-mooneywith the service role to use18:15
gmaan1. modify glance policy to service + admin right : this will allow cinder talk to glance with user token as it is currently 18:16
gmaan2. add keystoneauth service user conf option in glance section in cinder.conf18:16
gmaan3. cinder pass that service user to glance which has service role18:16
gmaan4. ask operator to configure the same in their deployment via releasenotes or doc18:17
whoami-rajat_3 and 4 could be the same cinder patch, which i can work on18:17
gmaan5, once we know operator are ok (or at least after one SLURP), make glance API to role:service only18:17
whoami-rajat_sounds good to me18:18
sean-k-mooneythat effectivly what we are doing in the nvoa changes for the external events api ectrs18:18
gmaanyes, even we have the service user config option available for so ling18:19
sean-k-mooneyso yes i think thiat is the correc approch as well18:19
whoami-rajat_and if possible, can we document all the details we've discussed here in some RBAC doc, it's really confusing to know the "right" way to adopt a new thing like SRBAC which created the confusion for me at least18:19
gmaanwhoami-rajat_: I can propose the changes if you are ok and help in review? bit if you want to do that also fine18:19
sean-k-mooneyone thing to note18:19
sean-k-mooneyyuo can achive the same effect without have per service config seciton with auth info18:20
sean-k-mooneynova's approch si way more flexible and i think cleaner18:20
whoami-rajat_gmaan, sure, that will be faster as i can be one of the cores approving things18:20
gmaanwhoami-rajat_: yeah, agree. that is my bad not to have proper doc for service role until I started working on those in nova, But I will do that as part of RBAC goal champion 18:20
gmaanwhoami-rajat_: cool18:20
sean-k-mooneybut watcher ueses the keyston_authoken user/password for all service eclients18:21
sean-k-mooneyim not sayint that ig the correct thing to do but you already need a cinder user in that section and it should have the service role18:21
gmaansean-k-mooney: yeah, we can do that but we need to formalize that in most of the services. and I am not sure if there are use case of per service user permission in any demployment or they are ok with same user for all serviecsd18:21
sean-k-mooneyi think we shoudl not  make that default18:22
gmaannova also has per service config users18:22
sean-k-mooneyhaving a per service config section i think is better18:22
gmaanyeah, do not know what use we can break18:22
sean-k-mooneybut you could have one fallback to the other if not configured18:22
gmaanok, let me propose the changes today and will add you  both as reviewer.18:23
gmaansean-k-mooney: you want me to do it in your glance revert or separate one as this is more than revert and add admin right also with serviec role?18:23
sean-k-mooneygmaan: that valid. i have alreayd modifed it once. its up to you if you want to review the review/changeid go for ti 18:24
whoami-rajat_modifications in the revert might not be clean and confuse people doing blame in the future, can we go for a new patch?18:24
sean-k-mooneyotherwise i can abandon it in favor of yours18:24
whoami-rajat_gmaan, just to be sure, you will be working on the cinder patch as well right? to use [glance] config section18:25
gmaanok let's do with new patch18:25
gmaanwhoami-rajat_: yes, whole series + devstack one and testing  it together like I did for nova18:25
whoami-rajat_okay, thanks!18:25
whoami-rajat_thanks again gmaan and sean-k-mooney for the details, I will go now, let me know when the glance and cinder patches are ready and i can help in merging them18:26
gmaanthanks, might be late for you18:27
whoami-rajat_generally I've stopped working late now but this was important so :D18:28
whoami-rajat_thanks again!18:28
gmaangood for you :)18:29
sean-k-mooneydansmith: so im more or less +w but before i hit it do you want to make this chagne https://review.opendev.org/c/openstack/placement/+/945465/comment/b1e034dd_f7b0b442/ or will i just approve as is19:10
sean-k-mooneydansmith: the wrodign change is defintly minor enouch that i dont think it needs a respon and gibi resolved there nit when they +2'd19:12
sean-k-mooneyso in the absence of a prefence one way or another ill add +w before i finish for today19:13
sean-k-mooneybut jsut said i woudl ask19:13
dansmithyeah, I dunno, I remember trying to figure out what to say there.. "does not increase existing overage" sounds a bit weird to me, even though I know what you mean19:13
dansmithmy preference is to +W so the code is right, and we can FUP the log text if we want19:13
sean-k-mooneyya same that waws the second time i wrote that19:13
sean-k-mooneyack ill do that so19:13
sean-k-mooneydone ok im going to try and finsin in the next 15 mins or so. i assume your going to backport those at some point19:15
sean-k-mooneyit could be nice to include that in 2024.1 before it goes unmaintained but that wont happen until 2025.2 is released so there si a little time still19:16
dansmithyeah, should be backported19:25
dansmithI guess I dunno how far it really needs, I was thinking just to 2025.119:26
opendevreviewGhanshyam proposed openstack/nova master: Add service role in Nova policy  https://review.opendev.org/c/openstack/nova/+/95757819:27
sean-k-mooneyi woudl say what is ever open when you get aroudn to it but 2025.1 is a good baseline19:27
sean-k-mooneyby open i mean still stable/*19:28
sean-k-mooneygmaan: same question to you https://review.opendev.org/c/openstack/nova/+/957578/comment/2b782393_1a20e07c/19:32
opendevreviewGhanshyam proposed openstack/nova master: Add service role in Nova policy  https://review.opendev.org/c/openstack/nova/+/95757819:32
sean-k-mooneyim +2 i can +w but if you feel like adressing the final nit then im ok with fast approving or having dan proxy19:33
opendevreviewGhanshyam proposed openstack/nova master: Add service role in Nova policy  https://review.opendev.org/c/openstack/nova/+/95757819:34
gmaandansmith: sean-k-mooney ^^ updated, I need to fix api-ref failure also. so this is updated one now19:34
sean-k-mooneyah yes stephsn sample split patch merged19:34
gmaanyeah :) 19:34
sean-k-mooneyi was wonderign what the prior conflict was19:35
gmaandansmith: sean-k-mooney thanks. can you please review this devstack change also which is needed but tempest service role tests https://review.opendev.org/c/openstack/devstack/+/95861219:47
sean-k-mooneyim actully going to finish reviewing the first change in the sev serice but yes i can aftger19:51
sean-k-mooneythen ill call it a day.19:52
sean-k-mooneymy priorty tomorrwo will be to try and finish hte sev serise adn maybe the provider.yaml one19:52
gmaanthanks19:53
sean-k-mooneyok its litrally jsut adding thr service role19:53
gmaanyeah, quick one :)19:53
sean-k-mooneyim +2 bnut shoud there be a comment above19:53
sean-k-mooneyor shoudl you also mention that the service role is added in 2025.219:54
sean-k-mooneybasicly im wonderign why you left the first comment19:54
gmaani might just missed that, i can fix later once touching that file for related things19:54
sean-k-mooneyack19:55
sean-k-mooneyi was just wonderign is there a convention that i shoudl be reviewing for19:55
sean-k-mooneylike in nova-next we sometime add comment when testing a future default so we know we can drop it form the job when it actully becomes the default19:55
sean-k-mooneyis noting the manager role so you can knwo when tempst can depend on it ?19:56
gmaanyeah, that is helpful specially to know when to drop those things once they are in all supported releases 19:56
sean-k-mooneyack19:57
sean-k-mooneyok first patch doen and it looks ok but i defintly dont have the brain power to go deeper in teh sev series today20:18
sean-k-mooneyill try an complete it tomorrow20:18
opendevreviewTakashi Kajinami proposed openstack/nova master: Detect AMD SEV-ES support  https://review.opendev.org/c/openstack/nova/+/92568523:50
opendevreviewTakashi Kajinami proposed openstack/nova master: Add hw_mem_encryption_model image property  https://review.opendev.org/c/openstack/nova/+/92770623:50
opendevreviewTakashi Kajinami proposed openstack/nova master: libvirt: Launch instances with SEV-ES memory encryption  https://review.opendev.org/c/openstack/nova/+/92610623:50
opendevreviewTakashi Kajinami proposed openstack/nova master: Add functional test scenario for mixed SEV RPs  https://review.opendev.org/c/openstack/nova/+/95856223:50
opendevreviewTakashi Kajinami proposed openstack/nova master: Purge nested SEV RPs when SEV is disabled  https://review.opendev.org/c/openstack/nova/+/95862623:51

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