Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-04-19
16:27:55 bauzas looks enough trivial for me and uncontroversial as libvirt supports those enlightments with our bare minimum version
16:28:10 bauzas and we already add some of them
16:28:21 bauzas dansmith: only for windows guests, yay
16:28:43 bauzas dansmith: we have some conditional in the code that makes the windows guests half-smart
16:28:58 bauzas half-smart, because we only enable some and not all the enlightments
16:28:58 dansmith there was previously a strong desire from some ops to be able to avoid exposing more detail to guests for cases where software had arbitrary "not licensed for virtual environments" restrictions
16:29:08 dansmith okay
16:29:34 bauzas dansmith: the conditional is based on the image prop IIRC
16:29:55 dansmith some image prop indicating windows or that windows enlightenments should be added?
16:29:57 bauzas dansmith: so operators who don't want to expose such things don't have to mark the images accordingly
16:29:57 dansmith if so, then cool
16:30:01 dansmith ack
16:30:09 bauzas dansmith: no, windows IIRC
16:30:19 bauzas lemme try to see if I can dig the conditional
16:30:23 dansmith so if it's just enabling more of those on the same conditional, then that seems fine for specless
16:30:43 artom_ dansmith, so we currently already add some bits based on the image property
16:30:49 artom_ This is just updating/adding some newer bits
16:30:53 bauzas found
16:30:54 dansmith ack, cool
16:30:56 bauzas #link https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L6062
16:31:14 dansmith roger that
16:31:36 bauzas ok, any concern for the specless approval ?
16:31:42 dansmith nope
16:31:47 dansmith I mean.. nope from me ;)
16:31:53 artom_ dansmith, https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/driver.py#L6062-L6084
16:32:00 artom_ jinx :(
16:32:07 bauzas artom_: please confirm those enlightments are already provided with our minim libvirt version ?
16:32:15 gibi I'm OK to have this accepted as specless
16:32:22 artom_ Is "Sean said they are" good enough? :)
16:32:50 bauzas as 'yeah, bauzas, we don't need to bump our minimum supported libvirt version for the sake of such small change'
16:32:52 dansmith we should confirm, and just say so in the bp
16:33:08 bauzas sounds reasonable
16:33:39 bauzas #agreed https://blueprints.launchpad.net/nova/+spec/libvirt-update-windows-englightenments approved as a specless BP provided our minimum libvirt version already supports such enlightments
16:33:56 bauzas that's all we had for the meeting
16:34:06 bauzas any other item before we call it a wrap ?
16:34:35 bauzas looks not,
16:34:43 bauzas thanks all !
16:34:47 bauzas #endmeeting
16:34:47 opendevmeet Meeting ended Tue Apr 19 16:34:47 2022 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
16:34:47 opendevmeet Minutes: https://meetings.opendev.org/meetings/nova/2022/nova.2022-04-19-16.00.html
16:34:47 opendevmeet Minutes (text): https://meetings.opendev.org/meetings/nova/2022/nova.2022-04-19-16.00.txt
16:34:47 opendevmeet Log: https://meetings.opendev.org/meetings/nova/2022/nova.2022-04-19-16.00.log.html
16:35:33 gibi o/
16:37:51 opendevreview Balazs Gibizer proposed openstack/nova master: DNM:Allow claiming PCI PF if child VF is unavailable https://review.opendev.org/c/openstack/nova/+/838555
16:38:57 elodilles dansmith: gate on victoria and older branches are broken, no need for rechecking them until your devstack fix is not backported and merged o:)
16:39:37 dansmith elodilles: didn't it merge?
16:39:40 gibi sean-k-mooney: I've filed a bug about the PCI state inconsistency https://bugs.launchpad.net/nova/+bug/1969496 and pushed a patch that does the cleanup at agent startup https://review.opendev.org/q/topic:bug/1969496
16:40:18 gibi the top of that topic there is a patch that tries to fix the actual PCI claim procedure when the state inconsistency is present, but that seem more complicated that I first thought
16:40:27 gmann dansmith: it need to be fixed in ussuru as nova stale/vicrotia has nova-grenade job running as voting
16:40:53 dansmith gmann: ah
16:41:11 gmann I am quashing those backport in ussuri also
16:41:15 gmann squashing
16:42:05 elodilles gmann: thanks!
16:52:22 opendevreview Rico Lin proposed openstack/nova master: libvirt: Add vIOMMU device to guest https://review.opendev.org/c/openstack/nova/+/830646
17:50:06 melwitt gmann: you're gonna propose something to make nova-grenade-multinode n-v on stable/victoria? lmk when you post it and I will review
17:50:59 gmann melwitt: we can do that or I am backporting the fixes in stable/ussuri and it should unblock nova stable/victoria https://review.opendev.org/c/openstack/devstack/+/837749
17:52:19 melwitt gmann: oh gotcha, cool
18:18:24 mfo sean-k-mooney, hey! thanks for reviewing https://review.opendev.org/c/openstack/nova/+/828979 last week!
18:18:27 mfo sean-k-mooney, would you mind suggesting another reviewer for the pending +1? (I see several in git-log, and some folks in here could help review this, but I'd try not to rely on them as we're from the same company.)
18:19:19 sean-k-mooney mfo: melwitt is a stable core otherwise stephenfin has knowladge of that area
18:19:34 mfo sean-k-mooney, thanks!
18:28:41 dansmith I left some comments, but I'm not so sure it's reasonable a really old stable at this point
18:38:57 sean-k-mooney dansmith: just reading them i think this is really a ubuntu specific fix and that path is fixed in our code so other locations were not supported
18:39:36 dansmith sean-k-mooney: not sure I understand.. that path is in our code elsewhere?
18:39:50 sean-k-mooney dansmith: did you make progress on the gate jobs you were working on
18:40:03 sean-k-mooney dansmith: ya i think it was hardcoded in nova on older branches
18:40:11 mfo dansmith, hey, thx for reviewing; just replied. the path is hardcoded in the source in victoria/ussuri; not too specific to Ubuntu :)
18:40:54 mfo wallaby and later used a refactor to rely on qemu's firmware descriptor files, which specify the paths.
18:41:02 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/828979/4/nova/virt/libvirt/driver.py#143
18:41:06 dansmith mfo: it doesn't reference a variable where that path is stored, so it looks pretty one-off-ish
18:41:24 sean-k-mooney that is where its defiend
18:41:29 mfo dansmith, indeed, happy to change that, but the array is so small that i thought it wasnt a problem.
18:41:34 dansmith sean-k-mooney: yeah, better to use that than just the random string in the file
18:41:41 sean-k-mooney i dont think there is a way to override it form our config
18:42:21 dansmith anyway, they're super old branches, this is stable-only and seems like a stretch on policy to me, but whatever
18:49:57 mfo feedback appreciated; i'll send another version referencing the path from the path array.
18:51:55 mfo oh, dan's comment just arrived.
18:53:08 dansmith maybe define THE_SECBOOT_THING="/usr/share..." above the list, replace the string in the list, then use the aforementioned variable in your check
18:53:20 dansmith but that still won't change my opinion on suitability :)
18:55:45 mfo nice; that would help w/ the scenario you mentioned that another distro has the array patched, as it wouldn't apply cleanly, and they would notice it, rather than not realizing they have to change this too. (and your idea of referencing the array seemed the better/less-worse option, indeed..)
18:55:56 mfo yeah, understand.
18:58:50 mfo personally, i guess the real issue is having a .secboot.fd as an option in releases before secure boot support was added, but taking that out / larger behavior changes is certainly out of discussion on stable releases :)
19:01:12 mfo and then people just hit this when they try and enable UEFI, which is already supported.
19:02:57 mfo (bcz they dont know they'll get a secboot.fd file.. so even if they knew q35 is needed for secboot, it wouldnt help.)
19:04:11 dansmith so to be clear, what we really should have done was have the two uefi paths, and then iterate over those + [SECBOOT_ONE] if-and-only-if the guest is q35 right?
19:05:03 mfo yup, secboot on x86 requires q35.
19:05:29 mfo just maybe not have the secboot path there before wallaby/secboot support, as another option.
19:06:03 dansmith yeah sounds like that would have been better, but outright removing it now would be a lot of change
19:06:20 dansmith I guess it seems like it would be a lot clearer of a patch if you made the logic be:
19:06:28 dansmith paths = [non-secboot, ..]
19:06:42 dansmith if hw_platform == 'q35': paths += [secboot]
19:06:58 dansmith instead of the flag and skip logic
19:07:17 mfo i see.
19:07:29 dansmith I guess that's maybe more change given the wording of your log message, but that would be a lot more like a fix ... "this can't ever be right on pc"
19:07:38 mfo i tried to have smaller changes, but that complicated review.
19:08:29 mfo the reason was not to change much of what already existed, for the stable only / time past for these releases.
19:08:36 mfo but i guess it was too much :/
19:09:07 dansmith no, I see the reasoning, it just doesn't feel like much of a fix the way you have it
19:09:19 mfo got you.
19:09:30 dansmith anyway, let me add my comment about changing the logic there and we should get some other opinions.. I won't block it either way if others want to put it in

Earlier   Later