| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-18 | |||
| 10:19:16 | stephenfin | I want to see if libvirt complains and if the guest handles it when it's resumed | |
| 10:19:18 | stephenfin | ack, thanks :) | |
| 10:20:42 | vinay_m | hi , regarding this bug : https://bugs.launchpad.net/nova/+bug/1893238 | |
| 10:20:42 | openstack | Launchpad bug 1893238 in OpenStack Compute (nova) "nova document search result links not working" [Medium,Triaged] | |
| 10:20:58 | stephenfin | (reason being that we call attach_device without the live or persistent flags, so I suspect libvirt might crash and burn without them https://github.com/openstack/nova/blob/f47bde539c8/nova/virt/libvirt/driver.py#L4425) | |
| 10:21:48 | vinay_m | is search implementation is diff/seperate from nova service ? | |
| 10:22:09 | vinay_m | means like is it not involved with nova code part | |
| 10:24:36 | sean-k-mooney | stephenfin: i kind of figured out why the func test failed. its not the fakelibvirt changes | |
| 10:24:52 | sean-k-mooney | at least not directly | |
| 10:25:00 | stephenfin | vinay_m: the search functionality is a Sphinx feature so no, it's not a nova issue | |
| 10:25:29 | sean-k-mooney | the source_dev elemnt is different in one of the comarisons so it does not find the device to detach | |
| 10:27:30 | gibi | stephenfin: boot, pause, attach, unpause. No error, the running guest sees the new device | |
| 10:28:41 | stephenfin | gibi: Okay, cool. I wonder what the point of those flags is so :-\ | |
| 10:28:52 | stephenfin | gibi: Could you run one final check? If you attach the device then stop and start the guest, does it stick around? | |
| 10:29:12 | sean-k-mooney | stephenfin: im really thinking we might want to remove tehm and pass true in all cases | |
| 10:29:52 | gibi | detaching while paused leads to a failure and a retry loop | |
| 10:30:01 | sean-k-mooney | gibi: yep it should | |
| 10:30:29 | vinay_m | stephenfin: thx | |
| 10:30:39 | sean-k-mooney | libvirt will wait for the guest to respond to the acpi hot unplug interupt | |
| 10:30:44 | sean-k-mooney | and since its paused it wont | |
| 10:32:05 | stephenfin | sean-k-mooney: Yeah, I'm not even sure what the point of them is. By passing 'live=True', we'll be passing the 'VIR_DOMAIN_AFFECT_LIVE' flag to the 'attachDeviceFlags' call | |
| 10:32:42 | stephenfin | libvirt says that "VIR_DOMAIN_AFFECT_LIVE specifies that the device shall be allocated to the active domain instance only and is not added to the persisted domain configuration." | |
| 10:33:04 | stephenfin | but if the instance is shutoff, then by definition there isn't an active domain so that should fail | |
| 10:33:24 | stephenfin | "Note that the target hypervisor must return an error if unable to satisfy flags." | |
| 10:33:36 | sean-k-mooney | ah ok | |
| 10:33:46 | sean-k-mooney | i didnt know about the error requiremetn | |
| 10:34:21 | sean-k-mooney | well i did but not with that wording | |
| 10:34:46 | stephenfin | my thinking was simply to set the 'VIR_DOMAIN_AFFECT_LIVE' and 'VIR_DOMAIN_AFFECT_CONFIG' flags based on checking 'domain.isActive()' and 'domain.isPersistent()' respectively | |
| 10:35:00 | stephenfin | but if it's that simple, why do you even need the flags | |
| 10:35:02 | sean-k-mooney | ya | |
| 10:35:21 | sean-k-mooney | i think that should work | |
| 10:35:32 | sean-k-mooney | can you add a patch to try it | |
| 10:35:47 | gibi | stephenfin: boot, attach, stop, start: guest keeps the attached device | |
| 10:35:59 | sean-k-mooney | as i said i know we have had some issues where the live and persistent domains get out of sync | |
| 10:36:15 | stephenfin | weeeeeird | |
| 10:36:23 | stephenfin | might have to ask danpb about this one | |
| 10:36:30 | sean-k-mooney | gibi: start recreated the xml form start so its going to work in the nova cases | |
| 10:36:39 | sean-k-mooney | even if we dont update the persistent domain | |
| 10:37:07 | sean-k-mooney | i kindof wish we did not have a persistnet domain in nova | |
| 10:37:13 | sean-k-mooney | just use teh traisiant one | |
| 10:37:38 | sean-k-mooney | it would remove a subset of bugs | |
| 10:37:43 | stephenfin | sean-k-mooney: ah, that's a fair point | |
| 10:37:58 | lyarwood | sean-k-mooney: I thought that was the plan in W? | |
| 10:38:07 | lyarwood | sean-k-mooney: to look into removing the persistent domain that is | |
| 10:38:13 | stephenfin | I was thinking of reboot in the libvirt sense, where you have to have a persistent domain | |
| 10:38:14 | sean-k-mooney | lyarwood: add it to the ptg/spec list | |
| 10:38:26 | sean-k-mooney | lyarwood: that is what we talked about after that CVE | |
| 10:38:33 | lyarwood | yup doing it now | |
| 10:38:34 | sean-k-mooney | did you bring it up upstream yet | |
| 10:39:20 | sean-k-mooney | lyarwood: stephenfin is looking at the device attach/detach flag we use and tryign to figure out why we pass live/persited true/false in places | |
| 10:39:26 | lyarwood | I thought we had talked about it here but obviously not sorry | |
| 10:39:46 | lyarwood | ah | |
| 10:39:48 | lyarwood | so | |
| 10:39:53 | lyarwood | that's because the initial attempt to detach | |
| 10:40:04 | lyarwood | might only detach from the inactive config | |
| 10:40:24 | lyarwood | after that we only want to try the live config | |
| 10:40:33 | stephenfin | lyarwood: not for PCI devices | |
| 10:40:36 | lyarwood | that's async and can fail if the guestos doesn't work with us | |
| 10:40:53 | stephenfin | we're not using the detach_device_with_retry helper for those | |
| 10:40:54 | stephenfin | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4396 | |
| 10:41:04 | stephenfin | vs https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L2306-L2309 | |
| 10:41:15 | stephenfin | also not for mdev devices https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L7512 | |
| 10:41:48 | lyarwood | what's the reason for not using the helper with PCI devices? | |
| 10:41:50 | lyarwood | or mdevs? | |
| 10:42:03 | lyarwood | shouldn't the semantics be the same | |
| 10:42:20 | stephenfin | not a clue :) that's what I was thinking but I can't find proof | |
| 10:42:36 | stephenfin | nor can I find proof that what we do for the other cases is even correct | |
| 10:42:43 | sean-k-mooney | so pci device detach was really only done in suspend until recently | |
| 10:42:44 | lyarwood | yeah I would think they should be the same tbh, virtio-blk devices are PCI devices after all | |
| 10:42:52 | lyarwood | oh right | |
| 10:42:57 | stephenfin | for example, we don't pass the live or persistent flags to the attach_device call for PCI devices | |
| 10:42:58 | stephenfin | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4425 | |
| 10:43:02 | lyarwood | hotunplug wasn't always a thing with PCI passthrough | |
| 10:43:17 | stephenfin | yet we do for other devices like network interfaces https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L2247 | |
| 10:43:23 | stephenfin | why? I don't know | |
| 10:43:38 | sean-k-mooney | for pci passthough its still not a thing for neutron sriov port it partly worked btu it never freed the devices | |
| 10:43:43 | sean-k-mooney | in the pci tracker | |
| 10:43:51 | lyarwood | kk then that might be why | |
| 10:44:02 | lyarwood | a single call would be enough if the domain was paused | |
| 10:44:08 | lyarwood | and iirc live == running or paused right | |
| 10:44:21 | stephenfin | yeah | |
| 10:44:27 | sean-k-mooney | a paused domian is runnign yes | |
| 10:44:33 | sean-k-mooney | well live | |
| 10:44:34 | stephenfin | I want to replace that with a call to isActive | |
| 10:44:38 | stephenfin | same thing, fwict | |
| 10:44:48 | sean-k-mooney | its still defiend and qemu is running but the cpus are stopped | |
| 10:45:14 | lyarwood | is this all from the dom.xml(PAUSED) patch btw stephenfin ? | |
| 10:45:30 | stephenfin | yup :( I'm way down this rabbit hole now | |
| 10:45:37 | lyarwood | kk | |
| 10:45:48 | lyarwood | well just to confuse you even more | |
| 10:46:03 | lyarwood | https://review.opendev.org/#/c/749929/ is how we should be doing this | |
| 10:46:06 | lyarwood | eventually | |
| 10:46:30 | lyarwood | that's incomplete but events are apparently a better way of checking when things are actually detached | |
| 10:46:48 | stephenfin | oh, very nice | |
| 10:49:53 | lyarwood | urgh I want to rewrite detach_device_with_retry so much in W | |
| 10:51:42 | lyarwood | stephenfin: so sorry I lost track of what the issue is here now | |
| 10:52:31 | stephenfin | The question is whether we should be setting the live parameter here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L4425 | |
| 10:53:19 | stephenfin | gibi's testing suggests it's not necessary, in which case I guess the question is do we need to be setting it here https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L2247 | |
| 10:54:21 | sean-k-mooney | stephenfin: if we just remove the persistnt domain then this is a moot point | |
| 10:54:29 | stephenfin | true | |