[libvirt] [PATCH v2 2/2] qemu: Use the correct vm def on cold attach

[John Ferlan]

On 06/13/2018 07:15 AM, Ján Tomko wrote:
> On Tue, Jun 12, 2018 at 06:06:02PM -0400, Laine Stump wrote:
>> On 06/12/2018 10:32 AM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>>>
>>> When attaching a device to the domain we need to be sure
>>> to use the correct domain definition (vm->def or vm->newDef)
>>> when calling virDomainDeviceDefParse because the post parse
>>> processing algorithms that may assign an address for the
>>> device will use whatever domain definition was passed in.
>>>
>>> Additionally, some devices (SCSI hostdev and SCSI disk) use
>>> algorithms that rely on knowing what already exists of the
>>> other type when generating the new device's address. Using
>>> the wrong VM definition could result in duplicated addresses.
>>>
>>> In the case of the bz, two hostdev's with no domain address
>>> provided were added to the running domain's config only.
>>> However, the parsing algorithm used the live domain in
>>> order to figure out the host device address resulting in
>>> the same address being used and a subsequent start failing
>>> due to duplicate address.
>>>
>>> Fix this by separating the checks/code into CONFIG and LIVE
>>> processing using the correct definition for each block and
>>> peforming cleanup for both options as necessary.
> 
> IMO this is a step into the right direction - rather than tuning up the
> device to be compatible with the live definition and hoping it will work
> in the persistent definition is just naive.
> 
> But I feel like I should read the whole parser and post-parser to
> confidently
> review this change. Also, even though it's 2018 we still generate the
> MAC address for interfaces in the parser, so this would result in two
> different interfaces being added into the definitions.
> 
As noted in my response to Laine - the virsh documentation states:

"Note: using of partial device definition XML files may lead to
unexpected results as some fields may be autogenerated and thus match
devices other than expected."

So, IOW, WYSIWYG.

If a consumer *chooses* to update LIVE for one device and LIVE and
CONFIG for another, then can we "assume" that they know what they are
doing and that they don't want the first device in the "next" guest.

To a degree we are merely doing what we're told.

Of course we could shift gears here and require the address to be
provided, but for someone that might be an unwelcome change in
philosophy. I don't think avoiding address assignment for attach-device
--config would be a good idea especially considering the complex
relationship with the disk attachment by name that could then conflict
with a hostdev.

>>
>> So what happens if someone requests only LIVE for the hotplug of one
>> device, and then both LIVE and CONFIG for another? Does the 2nd device
>> get a different address in the running guest than it has in the
>> persistent config?
>>
> 
> For that case, we should be considering both domain definitions when
> generating the addreess - at least for PCI addresses, we rely on calling
> virDomainDefPostParse in qemuDomainAttachDeviceConfig (this in turn
> calls the per-domain address assignment callback), which seems an
> overkill if we only added one device. Maybe we need a way to assign the
> address to just one device that would take both definitions into
> consideration? That would also let us get rid of that
> virDomainDefPostParse call.

While possible to consider both, it could be impractical. For some
devices there's lots of considerations to be made - for others it's a
simple does this already exist. In the long run it's all about
understanding each device's particular conflict resolution and capacity
comparison issues.

> 
> Or we could focus our energy elsehwere.
> 

I like this option...  Fix the one offs when/if they show up and move
on. If someone, some day has the desire to write patches that will
revamp device address assignment, good luck!


John


> Jano
> 
>>>
>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>> ---
>>>  src/qemu/qemu_driver.c | 52
>>> ++++++++++++++++++++++----------------------------
>>>  1 file changed, 23 insertions(+), 29 deletions(-)
>>>
> 
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>