[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/9] qemu: snapshot: save/restore inactive persistent config




On 20.12.2018 23:31, John Ferlan wrote:
> 
> 
> On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
>> In case of active persistent domain snapshot metadata is
>> not complete. We save only active configuration and on
>> restore use it both for active and inactive configuration.
>> Let's fix it and save and restore both in this case.
>>
>> In case of active transient or inactive domain we have
>> only one config and thus everything is good.
>>
>> By the way this patch fixes config memleak on error paths.
> 
> use @config to make it clearer...
> 
> I see how/why you combined things and although extracting out the
> memleak is a bit difficult, I think it should be done.
> 
> Perhaps in the cleanup code to *SaveConfig, you just set @config = NULL
> "temporarily" since we know/assume virDomainObjAssignDef was successful
> and we wouldn't want the subsequently added virDomainDefFree(config) to
> free it on those paths, but we would want it to be free'd on error
> paths.  Then by using @newConfig in this patch, it's even more obvious
> and of course the temporary config = NULL would be removed...  Hope this
> makes sense.

Ok. I thought of a distinct patch too, but having in mind that adding/removing
code in subsequent patches is not preferred I decided to just note this moment
in the message.

> 
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy virtuozzo com>
>> ---
>>  src/conf/snapshot_conf.c |  1 +
>>  src/conf/snapshot_conf.h |  2 ++
>>  src/qemu/qemu_driver.c   | 44 +++++++++++++++++++++++++++++++++++++++-----
>>  3 files changed, 42 insertions(+), 5 deletions(-)
>>
> 
> Not my area of expertise, but rather than have it sit unreviewed...
> 
> 
>> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
>> index 5a511b4..2e4a0b9 100644
>> --- a/src/conf/snapshot_conf.c
>> +++ b/src/conf/snapshot_conf.c
>> @@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
>>          virDomainSnapshotDiskDefClear(&def->disks[i]);
>>      VIR_FREE(def->disks);
>>      virDomainDefFree(def->dom);
>> +    virDomainDefFree(def->persistDom);
>>      virObjectUnref(def->cookie);
>>      VIR_FREE(def);
>>  }
>> diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
>> index 20a42bd..3da204a 100644
>> --- a/src/conf/snapshot_conf.h
>> +++ b/src/conf/snapshot_conf.h
>> @@ -75,6 +75,8 @@ struct _virDomainSnapshotDef {
>>      virDomainSnapshotDiskDef *disks;
>>  
>>      virDomainDefPtr dom;
>> +    /* inactive domain config in case of active persistent domain */
>> +    virDomainDefPtr persistDom;
>>  
>>      virObjectPtr cookie;
>>  
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index f7c1d6f..7e0585d 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -15687,6 +15687,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>>                                                   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
>>              goto endjob;
>>  
>> +        if (virDomainObjIsActive(vm) && vm->persistent &&
> 
> As it's easy to get lost or forget... IIRC vm->persistent means that w> have used the Define functions to provide a "config" at some point in

> time. Then eventually when/if we modify the "active" object to make a
> configuration specific change, then the @newDef gets populated with the
> config change(s); otherwise, the changes are made to the "active" def
> and eventually/possibly saved.  All the magic is hidden away in
> accessors that I don't think about all that often any more.

If by otherwise you mean "if we make changes to active config" - then 
such changes are saved to status files but they can't be written 
directly to config file.

> 
> So, should the vm->persistent be vm->newDef instead? Either way if the
> following call is made and vm->newDef == NULL, then eventually in
> virDomainDefFormatInternal we would core when @def was NULL.

Well there is at least one case when vm->newDef != NULL but domain is not persistent
- if we undefine domain vm->newDef is not released. This can be fixed but
I'm not sure there are no other cases. So checking vm->persisent is more reliable.

I think this is currenly invariant that if virDomainObjIsActive(vm) && vm->persistent
then vm->newDef != NULL but we can add explicit check.

The best option I think is to fix all cases when vm->newDef != NULL for non persistent
active domain and replace check as you suggested. What do you think?


> 
> 
>> +            !(def->persistDom = qemuDomainDefCopy(driver, vm->newDef,
>> +                                                  VIR_DOMAIN_XML_SECURE |
>> +                                                  VIR_DOMAIN_XML_MIGRATABLE)))
> 
> Do we need VIR_DOMAIN_XML_INACTIVE too since this is the "next"
> persistent config def?  A variant for parse is used later in patch5.

This is one of the hardest questions - what combinations of flags one should
use for one's purpuses :) I'm not kidding, there are no good definitions of 
VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_DEF_FORMAT_INACTIVE. So I use same
combination migration uses to pass persistent config to destination -
check qemuMigrationSrcRun.

Hmm, virDomainDefFormatInternal has next hunk at the beginning:

    if (def->id == -1)                                                                                              
        flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;  

and def->id == -1 for inactive config (@id is set to -1 on parsing
inactive configs and on domain start @def is copied to @newDef before
allocating @id - check qemuProcessInit). So we have this flag anyway :)

Also even the above fact does not have the case  AFAIU having this flag for inactive config 
does not have much meaning - random hunks of code that I checked filter
dumping info that gained by config on domain start. However I run across a 
kind of exception too - @id is not dumped at all if VIR_DOMAIN_DEF_FORMAT_INACTIVE
is set however this is not make much difference.

> 
>> +            goto endjob;
>> +
> 
> Since we are doing this as "best chance" and aren't requiring it, what
> are your (and perhaps others) thoughts related to making this really
> optional. As in if the qemuDomainDefCopy fails, then we just throw a
> VIR_INFO, clear the last error, and continue on?
> 
> I don't have a preference, but I'm just throwing it out there as an idea.

I should say I'm not looking at this step as "best chance" - if we can
not revert exact state I'd prefer to fail here.

> 
>>          if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
>>              align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
>>              align_match = false;
>> @@ -16219,6 +16225,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>      qemuDomainObjPrivatePtr priv;
>>      int rc;
>>      virDomainDefPtr config = NULL;
>> +    virDomainDefPtr persistConfig = NULL;
>>      virQEMUDriverConfigPtr cfg = NULL;
>>      virCapsPtr caps = NULL;
>>      bool was_stopped = false;
>> @@ -16226,6 +16233,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>      virCPUDefPtr origCPU = NULL;
>>      unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID;
>>      qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START;
>> +    virDomainDefPtr newConfig = NULL;
>>  
>>      virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>>                    VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
>> @@ -16332,6 +16340,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>              goto endjob;
>>      }
>>  
>> +    if (snap->def->persistDom &&
>> +        !(persistConfig = virDomainDefCopy(snap->def->persistDom, caps,
>> +                                           driver->xmlopt, NULL, true)))
>> +        goto endjob;
>> +
> 
> Based only on my previous comment, for this part I agree especially
> since if we were able to save a config, then we'd want to be sure to
> restore it.
> 
>>      cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
>>  
>>      switch ((virDomainState) snap->def->state) {
>> @@ -16440,8 +16453,18 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>                   * failed loadvm attempt? */
>>                  goto endjob;
>>              }
>> -            if (config) {
>> -                virDomainObjAssignDef(vm, config, false, NULL);
>> +
>> +            /* Older versions do not save inactive config in metadata, instead
>> +             * they use active config for this purpose, so keep this behaviour
>> +             * for backward compat.
>> +             */
>> +            if (persistConfig)
>> +                VIR_STEAL_PTR(newConfig, persistConfig);
>> +            else
>> +                VIR_STEAL_PTR(newConfig, config);
>> +
>> +            if (newConfig) {
>> +                virDomainObjAssignDef(vm, newConfig, false, NULL);
>>                  virCPUDefFree(priv->origCPU);
>>                  VIR_STEAL_PTR(priv->origCPU, origCPU);
>>              }
>> @@ -16449,8 +16472,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>              /* Transitions 2, 3 */
>>          load:
>>              was_stopped = true;
>> -            if (config)
>> +            if (config) {
>>                  virDomainObjAssignDef(vm, config, false, NULL);
>> +                config = NULL;
> 
> This "makes sense" except for the case where @persistConfig == NULL,
> then we wouldn't virDomainSaveConfig below as we currently do. So maybe
> this should be:
> 
>     if (!persistConfig)
>         VIR_STEAL_PTR(newConfig, config);
>     else
>         config = NULL;
> 
> That way we won't virDomainDefFree @config below (which is what you're
> trying to avoid), but we will save @config as we did before unless of
> course @persistConfig is going to be used.
> 
> Make sense?

Good catch! I think I'll change to:

            if (config) {
                virDomainObjAssignDef(vm, config, false, NULL);
                VIR_STEAL_PTR(newConfig, config);
            }

because if we have persistConfig but fail to assing it - means
we are on error path and we won't save newConfig anyway.

> 
>> +            }
>>  
>>              /* No cookie means libvirt which saved the domain was too old to
>>               * mess up the CPU definitions.
>> @@ -16471,6 +16496,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>                                               detail);
>>              if (rc < 0)
>>                  goto endjob;
>> +
>> +            if (persistConfig) {
> 
> Perhaps we should point out:
> 
>    /* This will save the @persistConfig in the vm->newDef where it
>     * was originally copied from. */
> 
> John
> 
>> +                virDomainObjAssignDef(vm, persistConfig, false, NULL);
>> +                VIR_STEAL_PTR(newConfig, persistConfig);
>> +            }
>>          }


This makes me wonder if virDomainObjAssignDef is too overcomplicated
if we need such comments.

Nikolay

>>  
>>          /* Touch up domain state.  */
>> @@ -16535,8 +16565,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>              qemuDomainRemoveInactive(driver, vm);
>>              goto endjob;
>>          }
>> -        if (config)
>> +        if (config) {
>>              virDomainObjAssignDef(vm, config, false, NULL);
>> +            VIR_STEAL_PTR(newConfig, config);
>> +        }
>>  
>>          if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>>                       VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
>> @@ -16600,7 +16632,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>      } else if (snap) {
>>          snap->def->current = false;
>>      }
>> -    if (ret == 0 && config && vm->persistent &&
>> +    if (ret == 0 && newConfig && vm->persistent &&
>>          !(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
>>                                      vm->newDef ? vm->newDef : vm->def))) {
>>          detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT;
>> @@ -16616,6 +16648,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>>      virObjectUnref(cfg);
>>      virNWFilterUnlockFilterUpdates();
>>      virCPUDefFree(origCPU);
>> +    virDomainDefFree(config);
>> +    virDomainDefFree(persistConfig);
>>  
>>      return ret;
>>  }
>>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]