| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-07-29 | |||
| 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 | |
| 14:34:37 | bauzas | sean-k-mooney: can you summarize the mdev issue you said ? | |
| 14:34:51 | bauzas | -ETOOMUCHCONTENTTODIGEST | |
| 14:35:35 | bauzas | fwiw, I need to resume my mtty series now that devstack supports 24.04 | |
| 14:36:58 | sean-k-mooney | bauzas: it appears that if a gues has an mdev and a cinder volume we cant detach t he cinder volume because the get_device_by_alis code cant hanel the fact that we dont have an aias field in the class that modeles the mdev device in the guest xml | |
| 14:37:10 | sean-k-mooney | i.e. you cant detach a cinder volume for a guest with a vgpu | |
| 14:37:34 | sean-k-mooney | we can fix that in the way propsoed in the patch but it missed the underly ing bug and did not include a repoducer | |
| 14:37:53 | bauzas | i see | |
| 14:38:06 | sean-k-mooney | the underlying bug is the get_device_by_alias code assumes all geust devices have an alias filed | |
| 14:38:18 | bauzas | so people need to rebuild the instance with a flavor without VGPU first before detaching | |
| 14:38:19 | sean-k-mooney | so we shoudl fix that | |
| 14:38:29 | bauzas | agreed | |
| 14:38:47 | bauzas | is this https://review.opendev.org/c/openstack/nova/+/925037 ? | |
| 14:38:49 | sean-k-mooney | bauzas: they might just need to stop the instnace | |
| 14:39:01 | sean-k-mooney | yep | |
| 14:39:13 | sean-k-mooney | that patch will work but it does not have enouch test coverage | |
| 14:39:30 | sean-k-mooney | and as i said the bug is reallly here https://github.com/openstack/nova/blob/7a7427691e0bd4818bb7a2c5f5371e0244addbbb/nova/virt/libvirt/guest.py#L419 | |
| 14:40:03 | bauzas | the real problem is that we didn't had any functest for that | |
| 14:40:06 | sean-k-mooney | that if assumes all devices in get_all_devices will have an alias field | |
| 14:40:19 | bauzas | because mdev support predates alias | |
| 14:40:24 | sean-k-mooney | yep | |