| *** dasm is now known as dasm|off | 00:11 | |
| gibi | good morning | 06:23 |
|---|---|---|
| opendevreview | Wenping Song proposed openstack/os-traits master: Remove unnecessary unicode prefixes https://review.opendev.org/c/openstack/os-traits/+/838436 | 06:32 |
| *** ministry is now known as __ministry | 06:48 | |
| bauzas | good morning | 07:38 |
| * bauzas does a bit of triaging and discovers a few issues with oslo.policy | 07:38 | |
| bauzas | this one seems weird to me : https://bugs.launchpad.net/nova/+bug/1968605 | 07:38 |
| bauzas | probably a dependency issue, I'm about to ask the oslo.policy release version they use | 07:39 |
| gibi | bauzas: o/ | 07:44 |
| gibi | yeah the second half of that stack trace really look like an infinite recursion | 07:47 |
| gibi | ahh the first half even states it that 2022-04-11 19:49:41.126 3391739 ERROR nova.api.openstack.wsgi RecursionError: maximum recursion depth exceeded | 07:48 |
| gibi | yeah we should figure out a reproducer for this... | 07:53 |
| bauzas | gibi: marked as Incomplete and asked for more details about the policies they use | 07:57 |
| gibi | ack | 07:58 |
| bauzas | another bug which seems legit but I can't test it : https://bugs.launchpad.net/nova/+bug/1968645 | 07:58 |
| gibi | if the policy language allows the definition of recursive rules then that would be interesting | 07:59 |
| bauzas | gibi: yeah, they could have fucked their DSL | 07:59 |
| gibi | bauzas: the devstack lvm cinder backend allows multiattach so in theory we could test this | 07:59 |
| bauzas | gibi: I know, that's just me who lacks having an env ready for this :) | 08:00 |
| * bauzas needs to setup some devstack in the weeds | 08:00 | |
| gibi | yeah, I also left my multinode env at my previous employer, but I will set up something new in the coming days... | 08:01 |
| opendevreview | Wenping Song proposed openstack/os-resource-classes master: Remove unnecessary unicode prefixes https://review.opendev.org/c/openstack/os-resource-classes/+/838448 | 08:03 |
| bauzas | gibi: I'll pass you the triage baton tonight :p | 08:03 |
| gibi | bauzas: that will be nice, anther reason to build some devstack env :D | 08:03 |
| bauzas | gibi: I have two VMs running somewhere :) | 08:04 |
| gibi | bauzas: btw I think I see similar multiattach state handling issues in cinder. See my comment https://bugs.launchpad.net/cinder/+bug/1968645/comments/3 | 08:04 |
| bauzas | gibi: this sounds legit as we race | 08:04 |
| bauzas | we race for the status change | 08:04 |
| gibi | yepp | 08:04 |
| bauzas | and given we look at it before... | 08:04 |
| gibi | It seems cinder suggest to retry | 08:04 |
| bauzas | simple and classic concurrency issue | 08:05 |
| opendevreview | Wenping Song proposed openstack/os-resource-classes master: Update python testing classifier https://review.opendev.org/c/openstack/os-resource-classes/+/822473 | 08:09 |
| songwenping | bauzas, gibi: please review the spec of 'Usage of new trait of OWNER_NOVA' https://review.opendev.org/c/openstack/nova-specs/+/819510 | 08:18 |
| bauzas | songwenping: OK, I can try later in the day | 08:18 |
| songwenping | thanks | 08:21 |
| opendevreview | ZhouYanbing proposed openstack/nova master: correct the wrong content in the notes https://review.opendev.org/c/openstack/nova/+/838205 | 08:21 |
| opendevreview | Rico Lin proposed openstack/nova master: libvirt: Ignore LibvirtConfigObject kwargs https://review.opendev.org/c/openstack/nova/+/830644 | 08:55 |
| opendevreview | Rico Lin proposed openstack/nova master: libvirt: Remove unnecessary TODO https://review.opendev.org/c/openstack/nova/+/830645 | 08:55 |
| opendevreview | Rico Lin proposed openstack/nova master: libvirt: Add vIOMMU device to guest https://review.opendev.org/c/openstack/nova/+/830646 | 08:55 |
| opendevreview | Kashyap Chamarthy proposed openstack/nova master: libvirt: Replace compareCPU() with compareHypervisorCPU() https://review.opendev.org/c/openstack/nova/+/762330 | 08:56 |
| opendevreview | Kashyap Chamarthy proposed openstack/nova master: test_report_cpu_traits: Update stale comment https://review.opendev.org/c/openstack/nova/+/838195 | 08:56 |
| kashyap | gibi: When you get time this week, can you please have a look at these? I've spent a lot of time splitting up the patches: https://review.opendev.org/q/topic:bp%252Fcpu-selection-with-hypervisor-consideration | 08:57 |
| gibi | kashyap: added to my review list | 08:57 |
| kashyap | gibi: Thank you. (I'd really like to be done with this. So all help appreciated :)) | 08:58 |
| gibi | I also would like to get this done :D | 08:58 |
| kashyap | Yeah, sorry for pausing and dragging this | 09:01 |
| gibi | no worries | 09:01 |
| *** icey_ is now known as icey | 09:53 | |
| *** amoralej is now known as amoralej|lunch | 12:08 | |
| *** amoralej|lunch is now known as amoralej | 13:02 | |
| *** dasm|off is now known as dasm | 13:03 | |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/xena: Adds regression test for bug LP#1944619 https://review.opendev.org/c/openstack/nova/+/838323 | 13:09 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/xena: Fix pre_live_migration rollback https://review.opendev.org/c/openstack/nova/+/836015 | 13:09 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/xena: stable/xena: Fix lower-constraint job https://review.opendev.org/c/openstack/nova/+/838509 | 13:09 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/wallaby: Adds regression test for bug LP#1944619 https://review.opendev.org/c/openstack/nova/+/838332 | 13:21 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/wallaby: Fix pre_live_migration rollback https://review.opendev.org/c/openstack/nova/+/836016 | 13:21 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/victoria: Adds regression test for bug LP#1944619 https://review.opendev.org/c/openstack/nova/+/838334 | 13:26 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/victoria: Fix pre_live_migration rollback https://review.opendev.org/c/openstack/nova/+/836017 | 13:26 |
| kashyap | stephenfin: Also, when you get an hour or so, please give this a once-over (I've split it last week): https://review.opendev.org/q/topic:bp%252Fcpu-selection-with-hypervisor-consideration | 13:52 |
| *** amoralej is now known as amoralej|off | 14:54 | |
| bauzas | reminder: nova meeting in 1 hour here at #openstack-nova | 15:00 |
| elodilles | bauzas: i've just updated the meeting page :X | 15:04 |
| elodilles | for the stable stuff | 15:04 |
| * elodilles hopes that did not interfere with bauzas updates | 15:05 | |
| bauzas | elodilles: nope, I haven't updated it ;) | 15:05 |
| bauzas | thanks ! | 15:05 |
| elodilles | :] | 15:06 |
| bauzas | artom: gentle reminder you have one topic for the nova meeting about a specless bp approval | 15:31 |
| bauzas | artom: if you can't join b/c you're playing the snowman, lemme know | 15:31 |
| bauzas | last reminder: nova meeting in 15 mins hzez | 15:45 |
| bauzas | here* even | 15:45 |
| artom | bauzas, ah, thanks! | 15:51 |
| artom | No, I can do IRC meeings | 15:51 |
| bauzas | artom: ok, cool | 15:51 |
| opendevreview | kiran pawar proposed openstack/nova master: VMware: Early fail spawn if memory is not multiple of 4. https://review.opendev.org/c/openstack/nova/+/835739 | 15:51 |
| artom | Trying to avoid video calls to conserve batteries | 15:52 |
| artom | Heh, Hydro Quebec crew is out front as we speak, actually | 15:55 |
| bauzas | #startmeeting nova | 16:00 |
| opendevmeet | Meeting started Tue Apr 19 16:00:34 2022 UTC and is due to finish in 60 minutes. The chair is bauzas. Information about MeetBot at http://wiki.debian.org/MeetBot. | 16:00 |
| opendevmeet | Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. | 16:00 |
| opendevmeet | The meeting name has been set to 'nova' | 16:00 |
| bauzas | heya folks | 16:00 |
| bauzas | #link https://wiki.openstack.org/wiki/Meetings/Nova#Agenda_for_next_meeting | 16:00 |
| bauzas | who's around ? | 16:01 |
| dansmith | o/ | 16:01 |
| chateaulav | \o | 16:01 |
| elodilles | o/ | 16:01 |
| gibi | \o | 16:01 |
| bauzas | okay I guess we can start | 16:02 |
| bauzas | let's try to have a quick meeting | 16:02 |
| bauzas | #topic Bugs (stuck/critical) | 16:02 |
| bauzas | #info No Critical bug | 16:02 |
| bauzas | #link https://bugs.launchpad.net/nova/+bugs?search=Search&field.status=New 35 new untriaged bugs (-3 since the last meeting) | 16:02 |
| bauzas | #link https://storyboard.openstack.org/#!/project/openstack/placement 26 open stories (0 since the last meeting) in Storyboard for Placement | 16:02 |
| bauzas | #info Add yourself in the team bug roster if you want to help https://etherpad.opendev.org/p/nova-bug-triage-roster | 16:02 |
| bauzas | I did some triage this week | 16:02 |
| bauzas | we had like 41 new bugs | 16:02 |
| bauzas | so eventually I triaged 6 of them | 16:02 |
| bauzas | I still need to create some environment for verifying some bugs | 16:03 |
| bauzas | but now, I'll pass the bug triage baton to someone else as we agreed on the PTG :) | 16:03 |
| gibi | I'm happy to take it :) | 16:03 |
| bauzas | do we want to have a name for it ? :) | 16:03 |
| bauzas | looks like bug czar is not accepted :p | 16:04 |
| gibi | the-one-who-triage-the-bugs :D | 16:04 |
| bauzas | lol | 16:04 |
| bauzas | anyway, | 16:04 |
| bauzas | #info Next bug triage baton is passed to gibi | 16:05 |
| bauzas | voila | 16:05 |
| bauzas | any bug to discuss ? | 16:05 |
| bauzas | gibi: I'll try to help you btw. as I'm creating a new environment | 16:05 |
| gibi | bauzas: thanks | 16:05 |
| bauzas | next topic then | 16:06 |
| bauzas | #topic Gate status | 16:06 |
| bauzas | #link https://bugs.launchpad.net/nova/+bugs?field.tag=gate-failure Nova gate bugs | 16:06 |
| bauzas | #link https://zuul.openstack.org/builds?project=openstack%2Fplacement&pipeline=periodic-weekly Placement periodic job status | 16:06 |
| bauzas | #link https://zuul.opendev.org/t/openstack/builds?job_name=nova-emulation&pipeline=periodic-weekly&skip=0 Emulation periodic job runs | 16:06 |
| bauzas | chateaulav: ^ ;) | 16:06 |
| bauzas | #info Please look at the gate failures and file a bug report with the gate-failure tag. | 16:06 |
| bauzas | #info STOP DOING BLIND RECHECKS aka. 'recheck' https://docs.openstack.org/project-team-guide/testing.html#how-to-handle-test-failures | 16:06 |
| dansmith | ++ | 16:06 |
| gmann | ++ | 16:07 |
| bauzas | nothing to tell about the gate CI ? | 16:07 |
| bauzas | chateaulav: looks like the periodic jobs work :) | 16:07 |
| chateaulav | sure do | 16:08 |
| bauzas | cool | 16:09 |
| bauzas | #topic Release Planning | 16:09 |
| bauzas | #link https://releases.openstack.org/zed/schedule.html | 16:09 |
| bauzas | #info Zed-1 is due in 4 weeks | 16:09 |
| bauzas | I'll ask next week for a spec review day | 16:09 |
| bauzas | start to think about it | 16:09 |
| bauzas | maybe sometimes around Zed-1 | 16:10 |
| bauzas | nothing to tell apart of this | 16:10 |
| bauzas | ok, moving on | 16:11 |
| bauzas | #topic Review priorities | 16:11 |
| bauzas | #link https://review.opendev.org/q/status:open+(project:openstack/nova+OR+project:openstack/placement+OR+project:openstack/os-traits+OR+project:openstack/os-resource-classes+OR+project:openstack/os-vif+OR+project:openstack/python-novaclient+OR+project:openstack/osc-placement)+label:Review-Priority%252B1 | 16:11 |
| bauzas | I'll continue to review stephenfin's changes for SQLA | 16:12 |
| bauzas | 2.0 | 16:12 |
| gibi | it is on my list too | 16:13 |
| bauzas | cool | 16:13 |
| bauzas | moving on again | 16:13 |
| bauzas | #topic Stable Branches | 16:13 |
| bauzas | elodilles: ^ | 16:13 |
| elodilles | #info xena branch is blocked until 'l-c drop' patches merge - https://review.opendev.org/q/I514f6b337ffefef90a0ce9ab0b4afd083caa277e | 16:14 |
| elodilles | #info victoria and older branches are blocked by devstack issue - this needs to be backported for older branches (?): https://review.opendev.org/q/I941ef5ea90970a0901236afe81c551aaf24ac1d8 | 16:14 |
| elodilles | #info stable/victoria Extended Maintenance transition is due ~ in a week (2022-04-27) - we don't have much time to land patches and release them (especially with broken gate) | 16:14 |
| elodilles | EOM | 16:14 |
| dansmith | elodilles: I'll be glad to +2 those :) | 16:14 |
| elodilles | dansmith: roger :) | 16:14 |
| gmann | elodilles: victoria should be good now | 16:14 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/xena: stable/xena: Fix lower-constraint job https://review.opendev.org/c/openstack/nova/+/838509 | 16:15 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/xena: Fix pre_live_migration rollback https://review.opendev.org/c/openstack/nova/+/836015 | 16:15 |
| opendevreview | Erlon R. Cruz proposed openstack/nova stable/xena: Adds regression test for bug LP#1944619 https://review.opendev.org/c/openstack/nova/+/838550 | 16:15 |
| elodilles | gmann: isn't that depend on ussuri? | 16:15 |
| gmann | stable/ussuri or less are broken, I will backport the fix in stable/ussuri as I need to pin tempest there. | 16:15 |
| elodilles | gmann: i mean, for stable/victoria gate to work in nova we should have devstack's stable/ussuri fixed | 16:16 |
| gmann | but table/train or less, someone need to backport if we want to fix or just time to EOL? | 16:16 |
| dansmith | is melwitt around? | 16:16 |
| dansmith | I'm thinking that since these are just job removals we could just +W them all and let them land, potentially out of order without a problem | 16:16 |
| gmann | elodilles: oh do we run nova grenade job in nova stable/victoria? | 16:16 |
| dansmith | it's not a regression like might normally be the case | 16:16 |
| gmann | dansmith: +1 | 16:16 |
| dansmith | elodilles: thoughts? | 16:16 |
| elodilles | dansmith: sounds good | 16:17 |
| elodilles | for the 'l-c drop' patches | 16:17 |
| dansmith | okay, melwitt said she was waiting to +2, but I think we should just slam them in | 16:17 |
| dansmith | you okay if I treat her +1 as a +2 and just +2+W straight away? | 16:18 |
| bauzas | if I can help, lmk | 16:18 |
| elodilles | gmann: i think in nova (and neutron) grenade is running back till queens | 16:18 |
| gmann | elodilles: for git issue, you are right stable/victoria in nova is blocked as we have nova grenade job running https://github.com/openstack/nova/blob/stable/victoria/.zuul.yaml#L466 | 16:18 |
| gmann | elodilles: let me backport in ussuri today but no motivation for stable/train or less | 16:19 |
| elodilles | gmann: understood, i can backport them further | 16:19 |
| gmann | elodilles: :) no option for EOL? | 16:19 |
| elodilles | gmann: well, the option is there :) | 16:20 |
| elodilles | gmann: what i see is that no one is interested in pike | 16:20 |
| gmann | ack | 16:20 |
| gmann | your call. | 16:20 |
| elodilles | gmann: so i'm anyway planning to propose pike EOL | 16:20 |
| gmann | sounds good, | 16:20 |
| elodilles | gmann: though i see patches for queens time to time | 16:20 |
| elodilles | gmann: but if no one else is interested, then of course, let's EOL | 16:21 |
| gmann | yeah, we have to do it at some point otherwise we end up spending time on those | 16:21 |
| bauzas | well, I'm not interested at least :) | 16:21 |
| bauzas | elodilles: but you can ask the community | 16:22 |
| gmann | anyways may be we can discuss that in TC or release about how many we can EOL | 16:22 |
| elodilles | bauzas: i'm planning to do that for pike (mass-EOL) | 16:22 |
| gmann | yeah, +1 on ML asking | 16:22 |
| elodilles | sure, will do | 16:23 |
| bauzas | ok, I guess we can move | 16:25 |
| bauzas | last item | 16:26 |
| bauzas | #topic Open discussion | 16:26 |
| bauzas | (artom) https://blueprints.launchpad.net/nova/+spec/libvirt-update-windows-englightenments | 16:26 |
| bauzas | artom_: not sure you can hear us :) | 16:26 |
| bauzas | but this is your time | 16:26 |
| artom_ | Yeah, I grew a tail since then | 16:26 |
| opendevreview | David Hill proposed openstack/nova master: Add disable_cpu_type_validation to skip cpu type validation. https://review.opendev.org/c/openstack/nova/+/838552 | 16:26 |
| artom_ | So, yeah, we talked about it at PTG, I filed the BP, just looking for a yay/nay on the specless BP aspect | 16:27 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Remove unavailable but not reported PCI devices at startup https://review.opendev.org/c/openstack/nova/+/838553 | 16:27 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Simulate bug 1969496 https://review.opendev.org/c/openstack/nova/+/838554 | 16:27 |
| bauzas | ok, this is a specless BP approval | 16:27 |
| artom_ | We're basically updating the XML we pass to libvirt when the guest is Windows to be nicer to the guest | 16:27 |
| artom_ | And mimic the kind of virtual HW that hyperv gives it | 16:27 |
| dansmith | only for windows guests or all? | 16:27 |
| bauzas | looks enough trivial for me and uncontroversial as libvirt supports those enlightments with our bare minimum version | 16:27 |
| bauzas | and we already add some of them | 16:28 |
| bauzas | dansmith: only for windows guests, yay | 16:28 |
| bauzas | dansmith: we have some conditional in the code that makes the windows guests half-smart | 16:28 |
| bauzas | half-smart, because we only enable some and not all the enlightments | 16:28 |
| 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:28 |
| dansmith | okay | 16:29 |
| bauzas | dansmith: the conditional is based on the image prop IIRC | 16:29 |
| dansmith | some image prop indicating windows or that windows enlightenments should be added? | 16:29 |
| bauzas | dansmith: so operators who don't want to expose such things don't have to mark the images accordingly | 16:29 |
| dansmith | if so, then cool | 16:29 |
| dansmith | ack | 16:30 |
| bauzas | dansmith: no, windows IIRC | 16:30 |
| bauzas | lemme try to see if I can dig the conditional | 16:30 |
| dansmith | so if it's just enabling more of those on the same conditional, then that seems fine for specless | 16:30 |
| artom_ | dansmith, so we currently already add some bits based on the image property | 16:30 |
| artom_ | This is just updating/adding some newer bits | 16:30 |
| bauzas | found | 16:30 |
| dansmith | ack, cool | 16:30 |
| bauzas | #link https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L6062 | 16:30 |
| dansmith | roger that | 16:31 |
| bauzas | ok, any concern for the specless approval ? | 16:31 |
| dansmith | nope | 16:31 |
| dansmith | I mean.. nope from me ;) | 16:31 |
| artom_ | dansmith, https://opendev.org/openstack/nova/src/branch/master/nova/virt/libvirt/driver.py#L6062-L6084 | 16:31 |
| artom_ | jinx :( | 16:32 |
| bauzas | artom_: please confirm those enlightments are already provided with our minim libvirt version ? | 16:32 |
| gibi | I'm OK to have this accepted as specless | 16:32 |
| artom_ | Is "Sean said they are" good enough? :) | 16:32 |
| bauzas | as 'yeah, bauzas, we don't need to bump our minimum supported libvirt version for the sake of such small change' | 16:32 |
| dansmith | we should confirm, and just say so in the bp | 16:32 |
| bauzas | sounds reasonable | 16:33 |
| 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 |
| bauzas | that's all we had for the meeting | 16:33 |
| bauzas | any other item before we call it a wrap ? | 16:34 |
| bauzas | looks not, | 16:34 |
| bauzas | thanks all ! | 16:34 |
| bauzas | #endmeeting | 16:34 |
| 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 |
| opendevmeet | Minutes: https://meetings.opendev.org/meetings/nova/2022/nova.2022-04-19-16.00.html | 16:34 |
| opendevmeet | Minutes (text): https://meetings.opendev.org/meetings/nova/2022/nova.2022-04-19-16.00.txt | 16:34 |
| opendevmeet | Log: https://meetings.opendev.org/meetings/nova/2022/nova.2022-04-19-16.00.log.html | 16:34 |
| gibi | o/ | 16:35 |
| * bauzas rushes off the keyboard | 16:36 | |
| 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:37 |
| * artom_ switches off his phone hotspot and back to his router, which should have properly reconnected by now | 16:38 | |
| 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:38 |
| dansmith | elodilles: didn't it merge? | 16:39 |
| 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:39 |
| 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 |
| gmann | dansmith: it need to be fixed in ussuru as nova stale/vicrotia has nova-grenade job running as voting | 16:40 |
| dansmith | gmann: ah | 16:40 |
| gmann | I am quashing those backport in ussuri also | 16:41 |
| gmann | squashing | 16:41 |
| elodilles | gmann: thanks! | 16:42 |
| opendevreview | Rico Lin proposed openstack/nova master: libvirt: Add vIOMMU device to guest https://review.opendev.org/c/openstack/nova/+/830646 | 16:52 |
| *** dasm is now known as dasm|off | 17: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 |
| 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:50 |
| melwitt | gmann: oh gotcha, cool | 17:52 |
| mfo | sean-k-mooney, hey! thanks for reviewing https://review.opendev.org/c/openstack/nova/+/828979 last week! | 18:18 |
| 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:18 |
| sean-k-mooney | mfo: melwitt is a stable core otherwise stephenfin has knowladge of that area | 18:19 |
| mfo | sean-k-mooney, thanks! | 18:19 |
| dansmith | I left some comments, but I'm not so sure it's reasonable a really old stable at this point | 18:28 |
| 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:38 |
| dansmith | sean-k-mooney: not sure I understand.. that path is in our code elsewhere? | 18:39 |
| sean-k-mooney | dansmith: did you make progress on the gate jobs you were working on | 18:39 |
| sean-k-mooney | dansmith: ya i think it was hardcoded in nova on older branches | 18:40 |
| 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 |
| mfo | wallaby and later used a refactor to rely on qemu's firmware descriptor files, which specify the paths. | 18:40 |
| sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/828979/4/nova/virt/libvirt/driver.py#143 | 18:41 |
| dansmith | mfo: it doesn't reference a variable where that path is stored, so it looks pretty one-off-ish | 18:41 |
| sean-k-mooney | that is where its defiend | 18:41 |
| mfo | dansmith, indeed, happy to change that, but the array is so small that i thought it wasnt a problem. | 18:41 |
| dansmith | sean-k-mooney: yeah, better to use that than just the random string in the file | 18:41 |
| sean-k-mooney | i dont think there is a way to override it form our config | 18:41 |
| dansmith | anyway, they're super old branches, this is stable-only and seems like a stretch on policy to me, but whatever | 18:42 |
| mfo | feedback appreciated; i'll send another version referencing the path from the path array. | 18:49 |
| mfo | oh, dan's comment just arrived. | 18:51 |
| 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 |
| dansmith | but that still won't change my opinion on suitability :) | 18:53 |
| 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 |
| mfo | yeah, understand. | 18:55 |
| 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 :) | 18:58 |
| mfo | and then people just hit this when they try and enable UEFI, which is already supported. | 19:01 |
| 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:02 |
| 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:04 |
| mfo | yup, secboot on x86 requires q35. | 19:05 |
| mfo | just maybe not have the secboot path there before wallaby/secboot support, as another option. | 19:05 |
| dansmith | yeah sounds like that would have been better, but outright removing it now would be a lot of change | 19:06 |
| dansmith | I guess it seems like it would be a lot clearer of a patch if you made the logic be: | 19:06 |
| dansmith | paths = [non-secboot, ..] | 19:06 |
| dansmith | if hw_platform == 'q35': paths += [secboot] | 19:06 |
| dansmith | instead of the flag and skip logic | 19:06 |
| mfo | i see. | 19:07 |
| 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 |
| mfo | i tried to have smaller changes, but that complicated review. | 19:07 |
| mfo | the reason was not to change much of what already existed, for the stable only / time past for these releases. | 19:08 |
| mfo | but i guess it was too much :/ | 19:08 |
| dansmith | no, I see the reasoning, it just doesn't feel like much of a fix the way you have it | 19:09 |
| mfo | got you. | 19:09 |
| 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 | 19:09 |
| mfo | that's really good feedback; thank you. | 19:09 |
| mfo | ok; meanwhile i'll try and come up w/ a fix as you suggested, anyway; it's certainly clearer / sounds more a like a simple fix. if sean is ok w/ that too, I guess this simpler style would make more sense. | 19:11 |
| mfo | and i can submit it if that's what looks best. thx again. | 19:12 |
| melwitt | elodilles, dansmith: looks like the check-cherry-picks script doesn't tolerate a change that has both "cherry picked from commit" line + [stable-only]. if it sees the former, it requires that the commit exist in some existing branch first https://zuul.opendev.org/t/openstack/build/cb49b82241b54cafa892a1fed4927e41 | 19:38 |
| elodilles | melwitt: so in any way, we need to merge them in order | 20:01 |
| melwitt | right | 20:02 |
| melwitt | I will follow them and recheck as each one lands | 20:02 |
| elodilles | and i'll continue in my morning if any remains :) | 20:04 |
| opendevreview | Merged openstack/nova stable/yoga: [stable-only] Drop lower-constraints job https://review.opendev.org/c/openstack/nova/+/838000 | 22:06 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!