[libvirt] [BUG] When PLUG a bridge interface to an active VM, the generated LIVE and CONFIG mac address are different

[Michal Privoznik]

On 8/28/19 9:55 AM, Xu Yandong wrote:
> 
> 
> On 2019/8/27 20:44, Ján Tomko wrote:
>> On Tue, Aug 27, 2019 at 02:13:28PM +0200, Michal Privoznik wrote:
>>> On 8/22/19 10:43 AM, Xu Yandong (Yandong Xu) wrote:
>>>> Hi,
>>>>
>>>> When plug a bridge interface to an active VM with both LIVE AND CONFIG flags,
>>>> libvirt generate different mac address to LIVE and CONFIG instance, so After
>>>> I reboot the VM, DHCP server doesn't assign the same IP address to the new
>>>> bridge interface.
>>>>
>>>
>>> This is kind of expected. In general, live and inactive XMLs can be
>>> significantly different.
>>
>> Well, if the live and inactive XMLs have sufficiently diverged, then
>> I don't see the point of calling AttachDevice with both AFFECT_LIVE and
>> AFFECT_CONFIG - you might as well use two different API calls.
>>
> Yes, when the live and inactive XMLs have sufficiently diverged, it's really
> difficult or even impossible to try to automatically fill in the missing data
> while applying to both live and inactive XML.
> 
>> However for a guest with the definitions in sync, quietly accepting a
>> both flags while attaching a device with different MAC addresses (so
>> essentially two different devices) feels wrong.
>>
>>> And I guess it's out of libvirt's scope to try and autofill missing data in
>>> such way that fits both libe and inactive XMLs. For instance, a PCI address
>>> can be taken/free in live XML because of hotplug/hotunplug but that might
>>> not be the case for inactive XML. Should libvirt try and find a slot that
>>> suits both XMLs?
>>>
>>
>> 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
> 
> Patch 55ce6564634 caused a problem fixed in 1e0534a7702, described as below:
> Some members are generated during XML parse (e.g. MAC address of
> an interface); However, with current implementation, if we
> are plugging a device both to persistent and live config,
> we parse given XML twice: first time for live, second for config.
> This is wrong then as the second time we are not guaranteed
> to generate same values as we did for the first time.
> 
> To solve that, we should use the same def when attaching a device
> config and live. And leave the current process unchanged when
> using --config and --live separately.
> 
> Signed-off-by: Wu Jing <wujing42 at huawei.com>
> ---
>   src/qemu/qemu_driver.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
> 
> 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?

Michal