Re: [libvirt] [PATCH] qemu: match controller index for LIVE+CONFIG when doing hotplug

On 06/23/2016 08:26 AM, Ján Tomko wrote: > On Wed, Jun 22, 2016 at 03:00:47PM -0400, Laine Stump wrote: >> An attempt to attach a new scsi controller with both --live and >> --config but without specifying an index, e.g.: >> >> <controller model="virtio-scsi" type="scsi" /> >> >> led to this error: >> >> internal error: Cannot parse controller index -1 >> >> This was because unspecified indexes are auto-assigned during >> virDomainDefPostParse(), which doesn't happen for hotplugged devices >> until after the device has been added to the domainDef, but >> qemuDomainAttachFlags() makes a copy of the device (for feeding to >> qemuDomainAttachDeviceLive() *before* it's added to the config, and >> the copying function actually formats the device object and then >> re-parses it into a new object. >> >> Since qemuDomainAttachDeviceConfig() consumes the device object >> pointer (i.e. sets it to NULL in the original virDomainDeviceDef) we >> can't just wait to make the copy. Instead, we need to make a *shallow* >> copy of the virDomainDeviceDef prior to >> qemuDomainAttachDeviceConfig(), then make a deep copy of the shallow >> copy. >> >> This resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344899 >> --- >> src/qemu/qemu_driver.c | 31 ++++++++++++++++++++----------- >> 1 file changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index fa05046..f3e17e2 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -8199,6 +8199,7 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, >> virDomainObjPtr vm = NULL; >> virDomainDefPtr vmdef = NULL; >> virDomainDeviceDefPtr dev = NULL, dev_copy = NULL; >> + virDomainDeviceDef dev_shallow; >> int ret = -1; >> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | >> VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; >> @@ -8237,22 +8238,19 @@ qemuDomainAttachDeviceFlags(virDomainPtr dom, >> if (dev == NULL) >> goto endjob; >> >> - if (flags & VIR_DOMAIN_AFFECT_CONFIG && >> - flags & VIR_DOMAIN_AFFECT_LIVE) { >> - /* If we are affecting both CONFIG and LIVE >> - * create a deep copy of device as adding >> - * to CONFIG takes one instance. >> - */ >> - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, >> driver->xmlopt); >> - if (!dev_copy) >> - goto endjob; >> - } >> - >> if (priv->qemuCaps) >> qemuCaps = virObjectRef(priv->qemuCaps); >> else if (!(qemuCaps = >> virQEMUCapsCacheLookup(driver->qemuCapsCache, vm->def->emulator))) >> goto endjob; >> >> + /* Save away the pointer to the device object before it is >> + * potentially swallowed up by qemuDomainAttachDeviceConfig(). >> + * This will allow us to make a copy of the device after any >> + * modifications made by virDomainDefPostParse() (which is called >> + * after the new device is added to the config >> + */ >> + dev_shallow = *dev; >> + > > This looks fragile and complicated, and I've already managed to break it > with the XML you provided: > > $ virsh attach-device f24 cont --live > Device attached successfully > > $ virsh attach-device f24 cont --live --config > error: Failed to attach device from cont > error: operation failed: target scsi:1 already exists Yeah, I noticed that too just before I posted, and am still thinking about how to solve it, but I still think the situation is one step better with this patch in place (although it does have a similar effect on --live + --config attachment of PCI devices - changing from one improper behavior to another. Sigh.) This all comes down to a more general problem that I offhandledly mentioned in an email with Tomasz Flendrich (the GSoC student working on "device address abstraction") - other than the live state starting off as a copy of the config, we make no attempt to maintain consistency of device attributes between the two, and could easily end up with conflicting things between the two. Normally this is mostly transparent to the user, but could cause serious complaints from guest OSes when they are later restarted and all their nice cozy devices previously added with "--live --config" are suddenly moved (this may not make sense now, but there are some further examples below).

> > AFAIK the reason we create a deep copy instead of parsing it again is But a "deep copy" is just formatting the object and then parsing the resulting XML again. > our generation of MAC addresses in the parser: > commit 1e0534a770208be6b848c961785db20467deb2fc > qemu: Don't parse device twice in attach/detach Well, that's a bit misleading. It *is* parsing twice; it's just that after the patch, the 2nd parse is done on the result of formatting the result of the first parse rather than parsing the original xml again. Because the MAC address is auto-generated as a part of the parsing (rather than during post-parse), we're able to get a MAC address consistent between live and persistent without calling virDomainDefPostParse(). Anything set in the post-parse doesn't have this same happy existence though. (side-note - when I made the patch to allow auto-generating the index, I originally put it directly in the parser rather than post-parse, but this was (rightfully) NACKed by Cole: https://www.redhat.com/archives/libvir-list/2016-May/msg01065.html Doing it the original way would have avoided the "already exists" error, but not the more general problem of diverging/conflicting live state and persistent config.) > So the question is: how should we treat the combination of --live and > --config? Not just that, but what should we do with --live by itself? Should we allow someone to hotplug a device --live with attributes that would have failed if given to --config (i.e. if they specify a PCI address that is currently unused in the live domain, but is in use in the config)? I could see a valid use case for this - if the device was added with --live and the user later wanted to make that device permanent - but allowing it could also lead to undesired/unexpected device address changes.

And what about the current bad behavior when you do this? virsh attach interface f24 --type network --source default --live virsh attach interface f24 --type network --source default --live --config

Without my patch, the 2nd device ends up with a different PCI address in the live and persistent object. With my patch, you end up with a similar error to above (complaining about attempting to use a PCI address that's already in use). So which evil is worse? Refusing to give a device different info for live vs. config (arguably not *as bad* for a PCI address vs. a MAC address or controller index, but still very undesirable)? Or silently allowing the evil, and damn the consequences? The latter would just cause some extra bookkeeping in the guest for a PCI address (the next time you start it, the guest would think the original device had disappeared and been replaced with another at a different address, with varying levels of whinging), but if you change the index of a SCSI controller, you could end up with disks plugged into the wrong controller (hmm, I guess that's not a horrible thing either, unless the device name in the guest is based on the controller's position in the devices, and that device name is used in the OS config somewhere... Yuck - better to make sure it doesn't change.) > Try to make the persistent device as close as the live one? > Or try to make it match the persistent config (while the live one would > match the live config?) I think we should make the two identical. For that to be true 100% of the time, we need to consider both the live and persistent config any time we're adding a device to either (or maybe just when we're adding it to both? I need to think or a minute, and I still haven't made my coffee).

(BTW, we also should consider what happens when you add a scsi disk with --live --config, attaching it to a controller that was added with just --live. I guess the controller should be automatically added to the config, but "dunno, haven't tried".)

> > Either way, it would be nicer to get the device definition stable > before we even try to add it to the persistent definition. The problem is that all of the stuff that "stabilizes" the device definition is done in the post-parse callback, and that requires that the device already be inserted into the domain definition.

> > Also, calling virDomainDefPostParse after device coldplug is strange, > we should be adding a device that does not need ajdustments. Again, we don't have the information necessary to adjust it until we at least have the domain def available (and all the post-parse callback infrastructure assumes/requires that the device already be inserted into the domain). But if we don't do the *entire* virDomainDefPostParse() then we are in danger of missing something during a hotplug. Maybe we need to rethink how we can do the post-parse stuff so that: 1) both the persistent config and (optionally) live state are supplied to the device callbacks (with assurances that the live state is ignored if it's NULL) 2) address and index assignment aren't done with a single call in the domain callback that assigns everything, but instead are done one-by-one in the device post-parse callback, assuring that newly assigned addresses aren't in use in either the persistent config or (if present) the live state. (I think the best way to do this may actually be to resurrect the idea of the pciaddrs "cache" rather than killing it (as is currently being discussed)).

And now I find myself all out of words, but without a solution or sufficient motivation to look for one. Hopefully something will come up out of the conversation.