[libvirt] [PATCH] qemu: match controller index for LIVE+CONFIG when doing hotplug
Martin Kletzander
mkletzan at redhat.com
Fri Jun 24 11:25:19 UTC 2016
On Thu, Jun 23, 2016 at 12:34:35PM -0400, Laine Stump wrote:
>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).
>
Even if the OS doesn't complain, we say we're trying to guarantee that
guest ABI will not change. Re-plugging some devices causes that (not
USB for example) and we should avoid it.
>>
>> 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.
>
it should be possible both ways: --live with one device and --config with
other on the same address, and the other way around as well. Checking
anything more would make the code way more complex just to tell the user
it's *maybe* not what they wanted. Don't forget you can boot a totally
different XML than the persistent one with CreateXML.
>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
>
This can be separated into two different issues. If you do
attach-interface, we generate an XML without address, so you should be
able to do the above and have 2 more interfaces live, the second one
would be identical to the only one added to config. However if you do
attach-device on a domain with XML that has the address specified, first
with only one of --live/--config and then with both, then the second
call should fail.
>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).
>
Definitely totally identical or just rollback and return error.
>(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".)
>
I think that controller will be added automatically even if it is not in
any of the definitions. However the fact that user/mgmt app treated
live and config definitions differently gives us the ability to just
simply handle them differently as well. We should not try to make
automatic additions way too much automatic IMHO.
>>
>> 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.
>
Add the definition in the config, do postparse, check if that definition
can be added into live, repeat with better data. Rolling back the
addition to config is easy. I haven't thought it through yet, though.
>>
>> 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)).
>
I had some ideas in my mind similar to this; it doesn't really matter if
the allocation is extracted from PostParse or not and I think lot of
stuff will change during the implementation =)
>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.
>
Friday, only small amount of coffee, well, it's not the right time to
think this through for me, but we definitely need to take a broader
approach to all the address assignments.
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160624/5b1c6e74/attachment-0001.sig>
More information about the libvir-list
mailing list