[libvirt] [PATCH] qemu: match controller index for LIVE+CONFIG when doing hotplug
Laine Stump
laine at laine.org
Thu Jun 23 16:34:35 UTC 2016
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.
More information about the libvir-list
mailing list