| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-07-29 | |||
| 11:22:54 | sean-k-mooney | can fail https://bugs.launchpad.net/nova/+bug/2074219 | |
| 11:23:43 | sean-k-mooney | bauzas: we should create a functional repoducer and or see if we can replicate this in your devstack mdev series | |
| 11:26:36 | sean-k-mooney | my guess is they work for Indiana University and just decieded that actully fixing this properly upstream was too much effrot and they are just going to carry the patch downstream | |
| 11:27:12 | sean-k-mooney | but if this is a real problem i wouls like to actully fix it for everyone properly | |
| 11:27:31 | sean-k-mooney | however we shoudl acutlly create a repoducer before doing that | |
| 11:43:31 | elodilles | sean-k-mooney: actually i'm back from today on, though i just started picking up the pace o:) | |
| 11:54:17 | sean-k-mooney | elodilles: well welcome back and dont burn yourself out. | |
| 12:00:47 | elodilles | thx, i will try no to o:) | |
| 12:21:15 | opendevreview | Rajesh Tailor proposed openstack/nova master: Add regression test case for bug 1978573 https://review.opendev.org/c/openstack/nova/+/914653 | |
| 12:21:16 | opendevreview | Rajesh Tailor proposed openstack/nova master: Restore original AZ when unshelve fails https://review.opendev.org/c/openstack/nova/+/911108 | |
| 12:26:28 | sean-k-mooney | elodilles: i dont really want to delay the release for the cve too long but if there are any backports you think we should include in the release im happy to review | |
| 12:31:33 | kashyap | sean-k-mooney: On that MDEV patch review, for a new contributor, I wouldn't use language like: "any attempt to rely on something like that would not pass code review." | |
| 12:32:12 | sean-k-mooney | kashyap: they are not really new htey have submitted patches before over a few releases and abandoned most of them | |
| 12:32:56 | sean-k-mooney | kashyap: but they were also implying that we were not following best pratices and relying on bad coding style. | |
| 12:33:12 | kashyap | I see; I've just seen that they've also contributed in the past a patch. | |
| 12:34:03 | sean-k-mooney | when i didnt recognise the name i looked to see if they were new and saw they ahd previous patches but i tought it was odd they abandoned most of them | |
| 12:34:21 | kashyap | Yeah, I saw that comment. Maybe they could've said, "_if_ we're relying on missing attributes ..." | |
| 12:35:09 | sean-k-mooney | perhaps they seams to ignore the content of my comment hosever and get very defensive | |
| 12:36:45 | sean-k-mooney | i on 2 ocations asked them to create a repoducer to demonstrate the orgainal bug and confrim there patch actully fixed it | |
| 12:36:53 | kashyap | But is parsing the alias field here wrong? I'm not 100% sure yet | |
| 12:45:15 | sean-k-mooney | the bug is that we dont check that all the objects have it before using it | |
| 12:45:18 | sean-k-mooney | and only some of them do | |
| 12:45:44 | sean-k-mooney | you can only request MDEVs via the flavor | |
| 12:46:00 | sean-k-mooney | so we dont actuly supprot detaching or attaching them to a running domain | |
| 12:46:09 | sean-k-mooney | which is why we did not add the alias field to them | |
| 12:46:21 | sean-k-mooney | now we can just do that and that will fix the current bug | |
| 12:46:45 | sean-k-mooney | we could also fix this in other ways | |
| 12:47:06 | sean-k-mooney | without a repoducer its hard to prevent a futrue regressions | |
| 12:47:21 | kashyap | Yeah, I see that bit - that we don't support detach/attach mdevs to a running domain. | |
| 12:47:38 | kashyap | I agree; repro will help here, and you've asked as much on the review | |
| 12:48:03 | sean-k-mooney | so the fact we dont supprot it is just an aside, its why we didnt think it was required when we started using the aliases | |
| 12:48:10 | sean-k-mooney | if we fix this by adding the filed | |
| 12:48:31 | sean-k-mooney | then we have a wackamole problem fo doing this for each possibel object that could be returened | |
| 12:48:52 | sean-k-mooney | if we fix this but only checking the aliase if the object has one then we fix it for all device types | |
| 12:49:28 | sean-k-mooney | in the previous case of a PF it was correct to add teh alias filed because we do supprot attaching/detaching those to a running guest | |
| 12:49:44 | sean-k-mooney | so there patch is not wrong jsut incompelte | |
| 12:50:24 | sean-k-mooney | kashyap: is this somehting your interested in fixing? | |
| 12:51:03 | sean-k-mooney | i was trying to highlight it to bauzas in case they wanted to fix it or where seeign it in there mtty serise | |
| 12:51:20 | kashyap | sean-k-mooney: I'm not sufficiantly clued into it; I was just curious based on your remark here. (Reading back your comments) | |
| 12:51:34 | kashyap | Hmm, you do raise a good point on the whack-a-mole problem of each object | |
| 12:51:50 | kashyap | Pff; I'd probably not risk to introduce new regressions at this stage, and given capacity. | |
| 12:51:56 | sean-k-mooney | ah ok, so the underliying problem is we actully woudl ideally have a new api call in libvirt | |
| 12:52:05 | kashyap | Hmm, really? | |
| 12:52:19 | kashyap | Ah, "ideally", you said. What's the current API call? | |
| 12:52:38 | sean-k-mooney | ya so ideally libvirt would have an api to get a domain device element given an alias | |
| 12:52:52 | sean-k-mooney | since it does not we get all the devices and do the filtering ourselves | |
| 12:54:25 | kashyap | Let me check on #virt (OFTC) if they're open to it | |
| 12:54:34 | sean-k-mooney | so this is where the actly bug is in nova https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L419 | |
| 12:54:53 | sean-k-mooney | dev.alias is only in some of our devices not all | |
| 12:55:14 | sean-k-mooney | so we coudl fix it in the base clase we use or we shoudl do a hasattr check before using it | |
| 12:55:40 | kashyap | Do we know a list of devices that libvirt _does_ return here? | |
| 12:56:18 | sean-k-mooney | its all devices in the domain after we parse it | |
| 12:56:25 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/guest.py#L419-L452 | |
| 12:56:47 | kashyap | I'm looking at get_all_devices() | |
| 12:57:41 | sean-k-mooney | so we could fix this by adding alias to https://github.com/openstack/nova/blob/master/nova/virt/libvirt/config.py#L3000 | |
| 12:58:13 | sean-k-mooney | these are not really object returned form libvirt | |
| 12:58:20 | sean-k-mooney | they are our config objects | |
| 12:58:29 | sean-k-mooney | we construct them form parsing the xml | |
| 12:58:56 | sean-k-mooney | but we check the libvirt pyton binding before doing this to see if there was a fucntion we could use more directly and didnt find one at the time | |
| 12:59:24 | sean-k-mooney | what we would do ideally is just as libvirt for the singel device element based on the alias | |
| 13:00:08 | sean-k-mooney | instead of pulling the entire domain. and parsing out all the devics to try and find the specific one | |
| 13:02:16 | kashyap | Yeah, I know these (the config.py stuff) are all libvirt guest-config classes. | |
| 13:02:35 | kashyap | Maybe you want to summarize / copy paste your comment from here on the change for the record. | |
| 13:03:11 | sean-k-mooney | not really :) i could but since they have abandoned it i feel like they are nto going to work on it any more | |
| 13:03:20 | sean-k-mooney | so i wanted to see if one of use wanted to pick it up | |
| 13:03:39 | sean-k-mooney | kashyap: im expectign bauzas mtty patches to repodcue this | |
| 13:03:55 | sean-k-mooney | so as part of using mtty to test in the gate we will likely need to fix this anyway | |
| 13:03:56 | kashyap | You think so? | |
| 13:04:01 | kashyap | Got a link? | |
| 13:04:48 | sean-k-mooney | so if this is failing the way they reported it woudl cause volume detach to fail if libvirt report mdevs | |
| 13:05:03 | sean-k-mooney | so i woudl expect the volume tempest test to be failin on https://review.opendev.org/q/topic:%22mtty_support%22 | |
| 13:06:26 | sean-k-mooney | the fact it does not makes me want the repodcures even more | |
| 13:06:48 | sean-k-mooney | oh... | |
| 13:07:06 | sean-k-mooney | ok so i wonder if this only failes if you have both a vgpu and a cinder volume | |
| 13:07:11 | kashyap | Hmm | |
| 13:07:12 | sean-k-mooney | on the same vm | |
| 13:08:22 | sean-k-mooney | we dont have result anymore but it did actully fail | |
| 13:08:24 | sean-k-mooney | https://zuul.opendev.org/t/openstack/build/42a251fe1d174a2abef7016b96fe0dc4 | |
| 13:10:46 | kashyap | Ohh, so the vGPU + Cinder volume is the combo | |
| 13:14:28 | opendevreview | Merged openstack/nova stable/2024.1: Remove AMI snapshot format special case https://review.opendev.org/c/openstack/nova/+/924982 | |
| 13:21:43 | sean-k-mooney | kashyap: i think so but again they didnt provide a clear repoducer in the bug so its hard to tell exactly | |
| 13:21:50 | sean-k-mooney | but i think that has to be the case looking at the code | |
| 13:22:09 | sean-k-mooney | we are looping over the devices in the guest domain and the error is in the mdev class | |
| 13:22:21 | sean-k-mooney | so it must only be an issue when comibining both features | |
| 13:22:34 | sean-k-mooney | which would expliain why we dont see this today | |
| 13:30:59 | dansmith | sean-k-mooney: gibi: I thought we were into "written by a core, backported by a core, can be single-vote-landed by a core" on stable now | |
| 13:31:44 | sean-k-mooney | dansmith: i guess, it does not come up often | |
| 13:32:12 | sean-k-mooney | but in either case i was going to proxy your/my +2 by end of day if noone else said go for it | |
| 13:34:22 | dansmith | ack | |
| 13:45:09 | opendevreview | Rajesh Tailor proposed openstack/nova master: Fix KeyError on assisted snapshot call https://review.opendev.org/c/openstack/nova/+/900783 | |
| 13:57:06 | opendevreview | Rajesh Tailor proposed openstack/nova master: Handle InstanceExists exception for duplicate instance https://review.opendev.org/c/openstack/nova/+/860938 | |
| 14:00:39 | elodilles | sean-k-mooney: i'm OK to propose stable releases anytime & i don't have any important backport on my radar | |
| 14:04:37 | sean-k-mooney | elodilles: bauzas asked me to include https://review.opendev.org/c/openstack/nova/+/924514 and its backports | |
| 14:05:06 | sean-k-mooney | i think we can include https://review.opendev.org/c/openstack/nova/+/923689 also on 2024.1 | |
| 14:06:03 | elodilles | ACK, I'll wait for them then o/ | |
| 14:06:10 | bauzas | cool | |
| 14:07:45 | sean-k-mooney | elodilles: i have not realy looked at the older branch contet much | |
| 14:07:58 | sean-k-mooney | i do see some nice to haves so i may do some stable review today | |
| 14:08:17 | sean-k-mooney | but i dont see anythign super urgent | |
| 14:09:08 | elodilles | ACK, i'll check them too | |