Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-04-19
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 mfo that's really good feedback; thank 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
19:11:04 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:12:19 mfo and i can submit it if that's what looks best. thx again.
19:38:36 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
20:01:10 elodilles melwitt: so in any way, we need to merge them in order
20:02:08 melwitt right
20:02:10 melwitt I will follow them and recheck as each one lands
20:04:04 elodilles and i'll continue in my morning if any remains :)
22:06:59 opendevreview Merged openstack/nova stable/yoga: [stable-only] Drop lower-constraints job https://review.opendev.org/c/openstack/nova/+/838000
#openstack-nova - 2022-04-20
01:45:17 opendevreview Merged openstack/nova stable/xena: [stable-only] Drop lower-constraints job https://review.opendev.org/c/openstack/nova/+/838007
08:57:41 opendevreview Merged openstack/nova stable/xena: Cleanup old resize instances dir before resize https://review.opendev.org/c/openstack/nova/+/828407
08:57:48 opendevreview Merged openstack/nova stable/wallaby: [stable-only] Drop lower-constraints job https://review.opendev.org/c/openstack/nova/+/838030
09:54:46 opendevreview Balazs Gibizer proposed openstack/nova master: Remove unavailable but not reported PCI devices at startup https://review.opendev.org/c/openstack/nova/+/838553
09:54:47 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
09:54:47 opendevreview Balazs Gibizer proposed openstack/nova master: Simulate bug 1969496 https://review.opendev.org/c/openstack/nova/+/838554
10:39:40 gibi bauzas: I'm +2 on the whole SQLA 2.0 series on nova
10:40:05 bauzas gibi: ack, will take a look later (working on creating a devstack environment)
10:55:35 opendevreview Merged openstack/nova master: db: Narrow down deprecation warning filter https://review.opendev.org/c/openstack/nova/+/837154
10:55:43 opendevreview Merged openstack/nova master: db: Trivial rewrapping of warning filters https://review.opendev.org/c/openstack/nova/+/837155
12:02:10 opendevreview Erlon R. Cruz proposed openstack/nova stable/xena: Adds regression test for bug LP#1944619 https://review.opendev.org/c/openstack/nova/+/838550
12:02:11 opendevreview Erlon R. Cruz proposed openstack/nova stable/xena: Fix pre_live_migration rollback https://review.opendev.org/c/openstack/nova/+/836015
12:27:06 bauzas folks, I need to go outside for one hour, bbiab
13:36:09 erlon hey folks, could I get some reviews on: https://review.opendev.org/q/topic:bug/1944619+AND+-status:abandoned+AND+(branch:stable/xena+OR+branch:stable/yoga)
13:41:24 ricolin stephenfin: Hey, I update the patch set for IOMMU, and it will be great if you can take a look together :) https://review.opendev.org/c/openstack/nova/+/830646
13:44:39 sean-k-mooney erlon: ok so this will have ot be backported in order so the yoga backports will have to land before the xena ectra
13:44:48 sean-k-mooney erlon: so https://review.opendev.org/c/openstack/nova/+/836014 is the placse to start
13:45:21 sean-k-mooney erlon: looking at https://review.opendev.org/q/topic:bug%252F1944619
13:45:39 sean-k-mooney i dont see the regression test backported to yoga
13:45:52 erlon yes, I was trying to do all of then one chainng into the other, but, yes they need to land first
13:46:42 erlon sean-k-mooney: the regression test landed before the release date heh
13:46:57 sean-k-mooney ah ok
13:47:05 sean-k-mooney that makes sense then
14:37:27 elodilles erlon: yep, the regression test landed before the yoga release but not before yoga was branched o:)
14:37:49 elodilles erlon: that's why it's not present on stable/yoga
14:38:12 erlon elodilles: hmm, got it
14:38:46 erlon elodilles: so I should cherry pick the 2 patches similar to what I was trying to do for the xena branch
14:39:47 elodilles erlon: yes, cherry pick them please, i think those should cleanly apply
14:39:51 erlon elodilles: another thing, I was trying to backport both patches back to ussuri, but the unit tests base get very different specially in ussuri
14:40:36 erlon would it be possible to backport onlt the fix there? not the unit tests?
14:43:08 elodilles erlon: well, sometimes tests needs some refactor, i'll try to look at the patch later, and help with it if needed
14:43:41 elodilles erlon: as both should be backported
14:44:42 elodilles erlon: but first let's merge them on yoga, and then xena, and so on :)
14:48:16 erlon ok, yes, on wallaby I needed to do some refactor, but for ussuri it was a bit too much, but we can try when we get there
15:27:18 stephenfin melwitt: You'd probably be interested in https://review.opendev.org/c/openstack/nova/+/837162 as I think the issue I'm addressing is your code. Would like to confirm I'm not missing anything obvious
15:30:46 melwitt stephenfin: yes indeed, I will take a look, thanks for the heads up
16:02:41 opendevreview Merged openstack/os-traits master: Remove unnecessary unicode prefixes https://review.opendev.org/c/openstack/os-traits/+/838436
16:04:59 opendevreview Merged openstack/os-resource-classes master: Update python testing classifier https://review.opendev.org/c/openstack/os-resource-classes/+/822473
16:05:01 opendevreview Merged openstack/os-resource-classes master: Remove unnecessary unicode prefixes https://review.opendev.org/c/openstack/os-resource-classes/+/838448
16:21:02 opendevreview Merged openstack/nova master: db: Replace use of strings in join, defer operations https://review.opendev.org/c/openstack/nova/+/837156
16:21:10 opendevreview Merged openstack/nova master: db: Remove use of empty 'and_()' https://review.opendev.org/c/openstack/nova/+/837157
16:21:18 opendevreview Merged openstack/nova master: db: Replace use of Connection.connect() method https://review.opendev.org/c/openstack/nova/+/837158
16:28:49 gibi kashyap: I reviewed https://review.opendev.org/q/topic:bp%252Fcpu-selection-with-hypervisor-consideration and left comments inline. Let me know if I should expand on some of some of my comments.
16:29:07 gibi thank you for the split, it definitely helps the review
16:42:23 melwitt stephenfin: +W
16:46:09 stephenfin ty!
16:50:12 opendevreview Rico Lin proposed openstack/nova master: libvirt: Add vIOMMU device to guest https://review.opendev.org/c/openstack/nova/+/830646
20:45:01 opendevreview Erlon R. Cruz proposed openstack/nova stable/yoga: Fix pre_live_migration rollback https://review.opendev.org/c/openstack/nova/+/836014
20:45:02 opendevreview Erlon R. Cruz proposed openstack/nova stable/yoga: Adds regression test for bug LP#1944619 https://review.opendev.org/c/openstack/nova/+/838788

Earlier   Later