1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/2] qemu: don't update cpu unconditionally for migratable flag
  4. View patch

[libvirt] [PATCH 2/2] qemu: don't update cpu unconditionally for migratable flag

Nikolay Shirokovskiy posted 2 patches 8 years, 4 months ago
[libvirt] [PATCH 2/2] qemu: don't update cpu unconditionally for migratable flag
Posted by Nikolay Shirokovskiy 8 years, 4 months ago 
Imagine if we use 'virsh dumpxml --migratable' for offline domain
to get config to tweak before migration. Currenly cpu section will
be expanded, host-cpu mode turns into custom and migration fails
because of ABI check.

Looks like ABI check does not make much sence for offline migration
but we don't want host-cpu mode to turn into custom in the first place.
Using --migratable is reasonable in this case because this flags
makes changes for inactive configs too like removing automatically
added parts that old versions can not handle.

I suggest not to update cpu for inactive configs. This appoach is
coherent with the way migration works for inactive configs in case
it is not specified externally to migration: function qemuMigrationCookieXMLFormat
don't expand cpu too.

Adding flag for active configs is useless because qemuDomainFormatXML
will clear it in this case. Thus we can skip adding update flag
altogether.

---

I suppose next usage of libvirt in case one need to migrate with tweaked configs:

inactive domain:

    virsh dumpxml DOM --migratable > dom.xml
    virsh migrate DOM --offline --persistent --xml dom.xml

active domain:

    virsh dumpxml DOM --migratable > active.xml
    virsh dumpxml DOM --migratable --inactive > inactive.xml
    virsh migrate DOM --persistent --xml active.xml --persistent-xml inactive.xml


 src/qemu/qemu_driver.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d9dff93..3223554 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6993,9 +6993,6 @@ static char
     if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0)
         goto cleanup;
 
-    if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
-        flags |= VIR_DOMAIN_XML_UPDATE_CPU;
-
     ret = qemuDomainFormatXML(driver, vm, flags);
 
  cleanup:
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't update cpu unconditionally for migratable flag
Posted by Jiri Denemark 8 years, 4 months ago 
On Thu, Sep 21, 2017 at 16:39:39 +0300, Nikolay Shirokovskiy wrote:
> Imagine if we use 'virsh dumpxml --migratable' for offline domain
> to get config to tweak before migration. Currenly cpu section will
> be expanded, host-cpu mode turns into custom and migration fails
> because of ABI check.
> 
> Looks like ABI check does not make much sence for offline migration
> but we don't want host-cpu mode to turn into custom in the first place.
> Using --migratable is reasonable in this case because this flags
> makes changes for inactive configs too like removing automatically
> added parts that old versions can not handle.
> 
> I suggest not to update cpu for inactive configs. This appoach is
> coherent with the way migration works for inactive configs in case
> it is not specified externally to migration: function qemuMigrationCookieXMLFormat
> don't expand cpu too.
> 
> Adding flag for active configs is useless because qemuDomainFormatXML
> will clear it in this case.

Not anymore, see commit v3.7.0-151-g06f75ff2cb.

...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d9dff93..3223554 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6993,9 +6993,6 @@ static char
>      if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0)
>          goto cleanup;
>  
> -    if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
> -        flags |= VIR_DOMAIN_XML_UPDATE_CPU;
> -
>      ret = qemuDomainFormatXML(driver, vm, flags);
>  
>   cleanup:

This area of code changed recently and it looks like the following now:

    if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
        flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;

    /* The CPU is already updated in the domain's live definition, we need to
     * ignore the VIR_DOMAIN_XML_UPDATE_CPU flag.
     */
    if (virDomainObjIsActive(vm) &&
        !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
        flags &= ~VIR_DOMAIN_XML_UPDATE_CPU;

But even before the changes, the two lines you are removing were

    if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
        flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;

since commit v0.10.2-133-g28f8dfdccc, i.e., for the last 5 years. I
guess you have some local changes applied to the libvirt tree.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't update cpu unconditionally for migratable flag
Posted by Nikolay Shirokovskiy 8 years, 4 months ago 

On 21.09.2017 17:24, Jiri Denemark wrote:
> On Thu, Sep 21, 2017 at 16:39:39 +0300, Nikolay Shirokovskiy wrote:
>> Imagine if we use 'virsh dumpxml --migratable' for offline domain
>> to get config to tweak before migration. Currenly cpu section will
>> be expanded, host-cpu mode turns into custom and migration fails
>> because of ABI check.
>>
>> Looks like ABI check does not make much sence for offline migration
>> but we don't want host-cpu mode to turn into custom in the first place.
>> Using --migratable is reasonable in this case because this flags
>> makes changes for inactive configs too like removing automatically
>> added parts that old versions can not handle.
>>
>> I suggest not to update cpu for inactive configs. This appoach is
>> coherent with the way migration works for inactive configs in case
>> it is not specified externally to migration: function qemuMigrationCookieXMLFormat
>> don't expand cpu too.
>>
>> Adding flag for active configs is useless because qemuDomainFormatXML
>> will clear it in this case.
> 
> Not anymore, see commit v3.7.0-151-g06f75ff2cb.

Is g06f75ff2cb commit in public libvirt tree? I can't see it even on https://libvirt.org/git/?p=libvirt.git;a=summary.


> 
> ...
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d9dff93..3223554 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -6993,9 +6993,6 @@ static char
>>      if (qemuDomainUpdateCurrentMemorySize(driver, vm) < 0)
>>          goto cleanup;
>>  
>> -    if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
>> -        flags |= VIR_DOMAIN_XML_UPDATE_CPU;
>> -
>>      ret = qemuDomainFormatXML(driver, vm, flags);
>>  
>>   cleanup:
> 
> This area of code changed recently and it looks like the following now:
> 
>     if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
>         flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;
> 
>     /* The CPU is already updated in the domain's live definition, we need to
>      * ignore the VIR_DOMAIN_XML_UPDATE_CPU flag.
>      */
>     if (virDomainObjIsActive(vm) &&
>         !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
>         flags &= ~VIR_DOMAIN_XML_UPDATE_CPU;
> 
> But even before the changes, the two lines you are removing were
> 
>     if ((flags & VIR_DOMAIN_XML_MIGRATABLE))
>         flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS;
> 
> since commit v0.10.2-133-g28f8dfdccc, i.e., for the last 5 years. I
> guess you have some local changes applied to the libvirt tree.
> 

))) The previous commit in series touch this place.

Nikolay
 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't update cpu unconditionally for migratable flag
Posted by Peter Krempa 8 years, 4 months ago 
On Thu, Sep 21, 2017 at 17:32:33 +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 21.09.2017 17:24, Jiri Denemark wrote:
> > On Thu, Sep 21, 2017 at 16:39:39 +0300, Nikolay Shirokovskiy wrote:
> >> Imagine if we use 'virsh dumpxml --migratable' for offline domain
> >> to get config to tweak before migration. Currenly cpu section will
> >> be expanded, host-cpu mode turns into custom and migration fails
> >> because of ABI check.
> >>
> >> Looks like ABI check does not make much sence for offline migration
> >> but we don't want host-cpu mode to turn into custom in the first place.
> >> Using --migratable is reasonable in this case because this flags
> >> makes changes for inactive configs too like removing automatically
> >> added parts that old versions can not handle.
> >>
> >> I suggest not to update cpu for inactive configs. This appoach is
> >> coherent with the way migration works for inactive configs in case
> >> it is not specified externally to migration: function qemuMigrationCookieXMLFormat
> >> don't expand cpu too.
> >>
> >> Adding flag for active configs is useless because qemuDomainFormatXML
> >> will clear it in this case.
> > 
> > Not anymore, see commit v3.7.0-151-g06f75ff2cb.
> 
> Is g06f75ff2cb commit in public libvirt tree? I can't see it even on https://libvirt.org/git/?p=libvirt.git;a=summary.

Note that 'g' isn't really a hex digit as it's usual in commit hashes.
That's a tricky part of the git describe string.

https://libvirt.org/git/?p=libvirt.git;a=commit;h=06f75ff2cb


commit 06f75ff2cb292e2658b4f2f6949c700550006272
Author:     Jiri Denemark <jdenemar@redhat.com>
AuthorDate: Fri Jun 30 16:55:20 2017 +0200
Commit:     Jiri Denemark <jdenemar@redhat.com>
CommitDate: Thu Sep 21 15:27:39 2017 +0200

    qemu: Don't update CPU when formatting live def
    
    Since commit v2.2.0-199-g7ce711a30e libvirt stores an updated guest CPU
    in domain's live definition and there's no need to update it every time
    we want to format the definition. The commit itself tried to address
    this in qemuDomainFormatXML, but forgot to fix qemuDomainDefFormatLive.
    Not to mention that masking a previously set flag is only acceptable if
    the flag was set by a public API user. Internally, libvirt should have
    never set the flag in the first place.
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1485022
    
    Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemu: don't update cpu unconditionally for migratable flag
Posted by Nikolay Shirokovskiy 8 years, 4 months ago 

On 21.09.2017 18:50, Peter Krempa wrote:
> On Thu, Sep 21, 2017 at 17:32:33 +0300, Nikolay Shirokovskiy wrote:
>>
>>
>> On 21.09.2017 17:24, Jiri Denemark wrote:
>>> On Thu, Sep 21, 2017 at 16:39:39 +0300, Nikolay Shirokovskiy wrote:
>>>> Imagine if we use 'virsh dumpxml --migratable' for offline domain
>>>> to get config to tweak before migration. Currenly cpu section will
>>>> be expanded, host-cpu mode turns into custom and migration fails
>>>> because of ABI check.
>>>>
>>>> Looks like ABI check does not make much sence for offline migration
>>>> but we don't want host-cpu mode to turn into custom in the first place.
>>>> Using --migratable is reasonable in this case because this flags
>>>> makes changes for inactive configs too like removing automatically
>>>> added parts that old versions can not handle.
>>>>
>>>> I suggest not to update cpu for inactive configs. This appoach is
>>>> coherent with the way migration works for inactive configs in case
>>>> it is not specified externally to migration: function qemuMigrationCookieXMLFormat
>>>> don't expand cpu too.
>>>>
>>>> Adding flag for active configs is useless because qemuDomainFormatXML
>>>> will clear it in this case.
>>>
>>> Not anymore, see commit v3.7.0-151-g06f75ff2cb.
>>
>> Is g06f75ff2cb commit in public libvirt tree? I can't see it even on https://libvirt.org/git/?p=libvirt.git;a=summary.
> 
> Note that 'g' isn't really a hex digit as it's usual in commit hashes.
> That's a tricky part of the git describe string.
> 
> https://libvirt.org/git/?p=libvirt.git;a=commit;h=06f75ff2cb
> 
> 
> commit 06f75ff2cb292e2658b4f2f6949c700550006272
> Author:     Jiri Denemark <jdenemar@redhat.com>
> AuthorDate: Fri Jun 30 16:55:20 2017 +0200
> Commit:     Jiri Denemark <jdenemar@redhat.com>
> CommitDate: Thu Sep 21 15:27:39 2017 +0200
> 
>     qemu: Don't update CPU when formatting live def
>     
>     Since commit v2.2.0-199-g7ce711a30e libvirt stores an updated guest CPU
>     in domain's live definition and there's no need to update it every time
>     we want to format the definition. The commit itself tried to address
>     this in qemuDomainFormatXML, but forgot to fix qemuDomainDefFormatLive.
>     Not to mention that masking a previously set flag is only acceptable if
>     the flag was set by a public API user. Internally, libvirt should have
>     never set the flag in the first place.
>     
>     https://bugzilla.redhat.com/show_bug.cgi?id=1485022
>     
>     Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> 

I see, thanx!

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.