Re: [libvirt] [BUG] When PLUG a bridge interface to an active VM, the generated LIVE and CONFIG mac address are different
On 8/22/19 10:43 AM, Xu Yandong (Yandong Xu) wrote: That possibly might be out of scope, but autofilling the mac address as early as virDomainNetDefParseXML also is not ideal.
We have pushed a patch bellow that can restore the situation to an older state.
Subject: [PATCH] qemu: use the same def when attaching device live and config
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 617d7d5..eca54d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8615,6 +8615,22 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup;
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG && + flags & VIR_DOMAIN_AFFECT_LIVE) { + /* If we are affecting both CONFIG and LIVE + * use the same xml of device preferentially + * to make the configuration consistent. + */ + devLive = virDomainDeviceDefParse(xml, vm->def,
This use of vm->def prefers live XML. This means that for instance PCI address are assigned based on current PCI layout in live XML which in general is different to inactive XML.
And it's not only PCI addresses, we autogenerate some out aspects of <interface/> (e.g. model) and even more for other devices. Just take a look at qemuDomainDeviceDefPostParse(). All functions there which take domain def as an argument do so because they are basing autogenerated value on domain definition. Plus there's more in parser code.
While your patch might work for your use case, it can break others.
BTW: have you read the original commit that caused this? I'm failing to see how we would not re-introduce the problem with your patch. If you're using live XML to validate/generate device addresses, how can we generate valid address for inactive XML?
This patch is a little different with the original commit, since we only prefer live XML when using LIVE AND CONFIG flags at the same time. Still parse separate device def if attach with only one flag. So this patch is kinda a combination of 1e0534a7702 and 55ce6564634, just to narrow down the problem influence. And I verified as the steps in https://bugzilla.redhat.com/show_bug.cgi?id=1559867, work as expected. Wu Jing
On 8/28/19 11:42 AM, wujing (O) wrote:
On 8/22/19 10:43 AM, Xu Yandong (Yandong Xu) wrote: That possibly might be out of scope, but autofilling the mac address as early as virDomainNetDefParseXML also is not ideal.
We have pushed a patch bellow that can restore the situation to an older state.
Subject: [PATCH] qemu: use the same def when attaching device live and config
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 617d7d5..eca54d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8615,6 +8615,22 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup;
+ if (flags & VIR_DOMAIN_AFFECT_CONFIG && + flags & VIR_DOMAIN_AFFECT_LIVE) { + /* If we are affecting both CONFIG and LIVE + * use the same xml of device preferentially + * to make the configuration consistent. + */ + devLive = virDomainDeviceDefParse(xml, vm->def,
This use of vm->def prefers live XML. This means that for instance PCI address are assigned based on current PCI layout in live XML which in general is different to inactive XML.
And it's not only PCI addresses, we autogenerate some out aspects of <interface/> (e.g. model) and even more for other devices. Just take a look at qemuDomainDeviceDefPostParse(). All functions there which take domain def as an argument do so because they are basing autogenerated value on domain definition. Plus there's more in parser code.
While your patch might work for your use case, it can break others.
BTW: have you read the original commit that caused this? I'm failing to see how we would not re-introduce the problem with your patch. If you're using live XML to validate/generate device addresses, how can we generate valid address for inactive XML?
This patch is a little different with the original commit, since we only prefer live XML when using LIVE AND CONFIG flags at the same time. Still parse separate device def if attach with only one flag. So this patch is kinda a combination of 1e0534a7702 and 55ce6564634, just to narrow down the problem influence.
And I verified as the steps in https://bugzilla.redhat.com/show_bug.cgi?id=1559867, work as expected.
Well, then try to coldplug <hostdev mode='subsystem' type='scsi'/> (that is only to inactive XML), and then try hotplug different <hostdev type'scsi'/> to both live and inactive XMLs and the bug will reproduce. Problem is, that inactive XML contains a device which is not contained in live XML and thus when generating device address based solely on live XML a conflicting address is generated. If we'd prefer inactive XML in your patch then the steps in my reproducer need to be swapped. At any rate, the issue is still there. Michal
participants (2)
-
Michal Privoznik -
wujing (O)