Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-09-18
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
10:55:22 sean-k-mooney well maybe not
10:55:27 sean-k-mooney we might need to backport something
10:55:37 sean-k-mooney for issue on older branches
10:55:43 lyarwood stephenfin: yeah odd, I wonder if it's something in https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainAttachDeviceFlags that defaults to attaching it to all configs?
10:56:17 stephenfin VIR_DOMAIN_AFFECT_CURRENT sounds reasonable
10:56:47 lyarwood https://github.com/openstack/nova/blob/c49cd3b9669da3f8da377369442bbe749fcb3c13/nova/virt/libvirt/driver.py#L1921 we also do that for disks FWIW
10:56:52 lyarwood provide persistent and live that is
10:57:22 sean-k-mooney lyarwood: ya i was wondering if currently we shoudl jsut set both to true
10:57:35 sean-k-mooney but was not sure if that would fail in some cases
10:57:56 stephenfin Or drop both flags and use VIR_DOMAIN_AFFECT_CURRENT (modify whatever state the instance is currently in)
11:01:40 lyarwood stephenfin: right but we would need to ensure that would attach or detach from both configs at the moment
11:01:54 stephenfin can the two get out of sync?
11:01:56 lyarwood stephenfin: if we dropped the inactive persistent config then yeah we could just use that
11:01:58 lyarwood stephenfin: yup

Earlier   Later