[libvirt] [PATCH 0/9] qemu: sav/restore inactive persistent config
This series address the same issue as seemingly abandomed patch series [1]. Implementation is written from scratch however as I don't see need to introduce new flags for the purpose of this patch series. However I read John's comments and addressed them where applied. The option for not reverting inactive config in case of restoring active persistent domain that Jiri suggested in [2] is new functionality actually and is left for future work. The purpuse of patch series is described in patch 3 "qemu: snapshot: save/restore inactive persistent config": 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. [1] [PATCH 0/7] Keep non-persistent changes alive in snapshot https://www.redhat.com/archives/libvir-list/2017-October/msg01333.html [2] [PATCH] qemu: snapshot: Keep non-persistent changes alive in snapshot https://www.redhat.com/archives/libvir-list/2017-October/msg01275.html Nikolay Shirokovskiy (9): qemu: release job on cleanup path qemu: snapshot revert: reuse common cleanup code qemu: snapshot: save/restore inactive persistent config schema: snapshot: add persistent domain config conf: snapshot: dump/parse persistent domain config docs: add persistent config to snapshot xml description conf: snapshot: check domain name on redefine conf: snapshot: support persistent config on redefine news: add notice for persistent config in snapshot docs/formatsnapshot.html.in | 7 ++ docs/news.xml | 11 +++ docs/schemas/domainsnapshot.rng | 10 +++ src/conf/snapshot_conf.c | 60 +++++++++++++++ src/conf/snapshot_conf.h | 2 + src/qemu/qemu_driver.c | 52 ++++++++++--- .../active_inactive_domain.xml | 85 ++++++++++++++++++++++ tests/domainsnapshotxml2xmltest.c | 1 + 8 files changed, 218 insertions(+), 10 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlout/active_inactive_domain.xml -- 1.8.3.1
Introduced-by: 6a6f6b91e qemu: "Fix CPU model broken by older libvirt" Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 90cbc02..3242a60 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16457,7 +16457,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, */ if (cookie && qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) - goto cleanup; + goto endjob; rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, -- 1.8.3.1
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Introduced-by: 6a6f6b91e qemu: "Fix CPU model broken by older libvirt"
Add a blank line here...
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John This is separate from the "core" of this and could be pushed without other changes.
Commit [1] changed cleanup code to first remove inactive domain and then release job so we can reuse common cleanup code. [1] 9115dcd83 qemu: Introduce and use qemuDomainRemoveInactiveJob Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3242a60..f7c1d6f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16533,8 +16533,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) { qemuDomainRemoveInactive(driver, vm); - qemuProcessEndJob(driver, vm); - goto cleanup; + goto endjob; } if (config) virDomainObjAssignDef(vm, config, false, NULL); @@ -16554,8 +16553,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { qemuDomainRemoveInactive(driver, vm); - qemuProcessEndJob(driver, vm); - goto cleanup; + goto endjob; } detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, -- 1.8.3.1
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Commit [1] changed cleanup code to first remove inactive domain and then release job so we can reuse common cleanup code.
[1] 9115dcd83 qemu: Introduce and use qemuDomainRemoveInactiveJob
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John Similarly separable patch that could be pushed now.
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. 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(-) 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 && + !(def->persistDom = qemuDomainDefCopy(driver, vm->newDef, + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE))) + goto endjob; + 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; + 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; + } /* 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) { + virDomainObjAssignDef(vm, persistConfig, false, NULL); + VIR_STEAL_PTR(newConfig, persistConfig); + } } /* 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; } -- 1.8.3.1
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.
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 we 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. 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.
+ !(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.
+ 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.
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?
+ }
/* 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); + } }
/* 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; }
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; }
On 12/21/18 3:23 AM, Nikolay Shirokovskiy wrote:
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.
I'm mostly 50/50 on this - leaning towards separation mainly because I think it is possible, but also because when you mention adding a new feature and fixing an existing memory leak in the commit message that generally results in someone asking for separation. Certainly makes it easier to bisect to a commit and not have to wonder did the new functionality or the bugfix cause some issue...
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.
Like I said - some of the magic and details are lost in accessors. Yes, the active is essentially saved to status... The whole def/newDef fetch, store, and manipulation gets hidden behind those *Domain.*Persistent* type helpers. The @newDef nomenclature has always struck me as a bit odd, but using a longer more descriptive name may not help me either.
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.
So we have an active persistent domain with a newDef that eventually is made non persistent... I'm sure there's a reason for it...
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?
I think my main concern would be vm->persistent == true and vm->newDef == NULL.
+ !(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
110% agree.
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.
and obviously I asked because I find the design of the flags is confusing and what each means with respect to what post parse processing or validation occurs - I just cannot keep it in memory.
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 :)
<sigh>... commit f24e67d24
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.
The case you're chasing though would have an id != -1 since it's an active guest, but we're saving the newDef/next config. Perhaps similar to how various qemu_driver commands can manipulate both active and config (attach, detach, change, etc). It's a "hypervisor dependent" action as to what happens.
+ 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.
OK - I was going explicit with the alternative to the code below for persistConfig/newConfig.
+ }
/* 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.
Nah, no way that's over complicated <eyeroll>. Touching it though means you get to own making it better... job security and all that. John
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; }
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/schemas/domainsnapshot.rng | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 2680887..405e614 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -84,6 +84,16 @@ </choice> </optional> <optional> + <element name='persistent'> + <!-- Nested grammar ensures that any of our overrides of + storagecommon/domaincommon defines do not conflict + with any domain.rng overrides. --> + <grammar> + <include href='domain.rng'/> + </grammar> + </element> + </optional> + <optional> <element name='parent'> <element name='name'> <text/> -- 1.8.3.1
Maybe a few words here about the option being used to hold a persistent config that could be different than the snapshot's running domain def. On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/schemas/domainsnapshot.rng | 10 ++++++++++ 1 file changed, 10 insertions(+)
Seems reasonable - although I wonder if this should be combined with the next two patches as well... That's not a big deal though... I actually prefer "persistent" over "newDef" or a/k/a "nextPersistentDef". Reviewed-by: John Ferlan <jferlan@redhat.com> John
Persistent in this case is a persistent config of active persistent domain. We need it for metadata to be complete as explained in previous patch. Config is saved like this: <domainsnapshot> <persistent> <domain> ... </domain> </persistent> </domainsnapshot> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 26 +++++++ .../active_inactive_domain.xml | 85 ++++++++++++++++++++++ tests/domainsnapshotxml2xmltest.c | 1 + 3 files changed, 112 insertions(+) create mode 100644 tests/domainsnapshotxml2xmlout/active_inactive_domain.xml diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 2e4a0b9..34fcf64 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -283,6 +283,20 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } else { VIR_WARN("parsing older snapshot that lacks domain"); } + + if (def->state == VIR_DOMAIN_RUNNING || + def->state == VIR_DOMAIN_PAUSED) { + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + xmlNodePtr domainNode; + + if ((domainNode = virXPathNode("./persistent/domain", ctxt)) && + !(def->persistDom = virDomainDefParseNode(ctxt->node->doc, + domainNode, caps, + xmlopt, NULL, + domainflags))) + goto cleanup; + } } else { def->creationTime = tv.tv_sec; } @@ -753,6 +767,18 @@ virDomainSnapshotDefFormat(const char *domain_uuid, virBufferAddLit(&buf, "</domain>\n"); } + if (def->persistDom) { + virBufferAddLit(&buf, "<persistent>\n"); + virBufferAdjustIndent(&buf, 2); + + if (virDomainDefFormatInternal(def->persistDom, + caps, flags, &buf, xmlopt) < 0) + goto error; + + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</persistent>\n"); + } + if (virSaveCookieFormatBuf(&buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; diff --git a/tests/domainsnapshotxml2xmlout/active_inactive_domain.xml b/tests/domainsnapshotxml2xmlout/active_inactive_domain.xml new file mode 100644 index 0000000..74025a3 --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/active_inactive_domain.xml @@ -0,0 +1,85 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <state>running</state> + <parent> + <name>earlier_snap</name> + </parent> + <creationTime>1272917631</creationTime> + <memory snapshot='internal'/> + <domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> + </domain> + <persistent> + <domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> + </domain> + </persistent> + <active>1</active> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c index 2a07fe0..e51373d 100644 --- a/tests/domainsnapshotxml2xmltest.c +++ b/tests/domainsnapshotxml2xmltest.c @@ -204,6 +204,7 @@ mymain(void) DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false); DO_TEST_OUT("metadata", "c7a5fdbd-edaf-9455-926a-d65c16db1809", false); DO_TEST_OUT("external_vm_redefine", "c7a5fdbd-edaf-9455-926a-d65c16db1809", false); + DO_TEST_OUT("active_inactive_domain", NULL, true); DO_TEST_INOUT("empty", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false, false); DO_TEST_INOUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8", false, false); -- 1.8.3.1
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Persistent in this case is a persistent config of active
of the active
persistent domain. We need it for metadata to be complete as explained in previous patch.
Hence why I think the two should be combined.
Config is saved like this:
<domainsnapshot> <persistent> <domain> ... </domain> </persistent> </domainsnapshot>
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 26 +++++++ .../active_inactive_domain.xml | 85 ++++++++++++++++++++++ tests/domainsnapshotxml2xmltest.c | 1 + 3 files changed, 112 insertions(+) create mode 100644 tests/domainsnapshotxml2xmlout/active_inactive_domain.xml
Seems reasonable to me... Reviewed-by: John Ferlan <jferlan@redhat.com> John
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/formatsnapshot.html.in | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index fbbecfd..1357f53 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -235,6 +235,13 @@ at the time of the snapshot (<span class="since">since 0.9.5</span>). Readonly. </dd> + <dt><code>persistent/domain</code></dt> + <dd>Inactive domain configuration for active persistent domain. + Such a domain have 2 distinct configs and here inactive is + stored. It is different from <code>domain</code> which more + presisely keeps "inactive portion" of active config. + (<span class="since">since 5.0.0</span>). + </dd> <dt><code>cookie</code></dt> <dd>Save image cookie containing additional data libvirt may need to properly restore a domain from an active snapshot when such data -- 1.8.3.1
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/formatsnapshot.html.in | 7 +++++++ 1 file changed, 7 insertions(+)
This probably should be merged with patch5 - nice to separate for review though. Although, like patch4 it's not that important to me.
diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index fbbecfd..1357f53 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -235,6 +235,13 @@ at the time of the snapshot (<span class="since">since 0.9.5</span>). Readonly. </dd> + <dt><code>persistent/domain</code></dt> + <dd>Inactive domain configuration for active persistent domain.
s/for/from an/
+ Such a domain have 2 distinct configs and here inactive is
Tough read grammar wise.
+ stored. It is different from <code>domain</code> which more + presisely keeps "inactive portion" of active config.
precisely
+ (<span class="since">since 5.0.0</span>). + </dd>
Hmmm... IIRC... isn't newDef the "next config"? Scour around for Domain*Persistent and see what I mean. I think you have this description backwards - hey I could be wrong, too... An active domain for which configuration specific changes have been made will store both the "active" and "next config" in the domain object and the <persistent> element will signify that config; whereas, the <domain> element signifies the active domain configuration. There is nothing different in the previous patch between the "<domain ... </domain>" and the "<persistent> ... </persistent>"; however, I would believe that if something changed as "config only" then it would/should show up in that persistDom. John
<dt><code>cookie</code></dt> <dd>Save image cookie containing additional data libvirt may need to properly restore a domain from an active snapshot when such data
On 20.12.2018 23:32, John Ferlan wrote:
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/formatsnapshot.html.in | 7 +++++++ 1 file changed, 7 insertions(+)
This probably should be merged with patch5 - nice to separate for review though. Although, like patch4 it's not that important to me.
diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index fbbecfd..1357f53 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -235,6 +235,13 @@ at the time of the snapshot (<span class="since">since 0.9.5</span>). Readonly. </dd> + <dt><code>persistent/domain</code></dt> + <dd>Inactive domain configuration for active persistent domain.
s/for/from an/
+ Such a domain have 2 distinct configs and here inactive is
Tough read grammar wise.
+ stored. It is different from <code>domain</code> which more + presisely keeps "inactive portion" of active config.
precisely
+ (<span class="since">since 5.0.0</span>). + </dd>
Hmmm... IIRC... isn't newDef the "next config"? Scour around for Domain*Persistent and see what I mean. I think you have this description backwards - hey I could be wrong, too...
An active domain for which configuration specific changes have been made will store both the "active" and "next config" in the domain object and the <persistent> element will signify that config; whereas, the <domain> element signifies the active domain configuration.
Naming is known to be hard. For active domain you can think of this configs as active/persistent or active/next one(new one). I think @newDef name is choosen because @persistentDef would be misleading - inactive domains store its persistent domain in @def and not @newDef (however I would prefer the opposite - inactive domains storing its definition in @persistentDef and having @def = NULL). If done from scratch I would suggest to use: - <domain> for inactive/transient domains - <persistent><domain> and <transient><domain> for active persistent domains or even only last option for all cases. But now for backward compat we need some name. Well I thought explaining next/new def is more complicated then stating that persistent is only defined for active persistent domains and that is keeps the persistent config. Nikolay
There is nothing different in the previous patch between the "<domain ... </domain>" and the "<persistent> ... </persistent>"; however, I would believe that if something changed as "config only" then it would/should show up in that persistDom.
John
<dt><code>cookie</code></dt> <dd>Save image cookie containing additional data libvirt may need to properly restore a domain from an active snapshot when such data
Renaming domain which has snapshots is prohibited. Also reverting to ABI compatible active domain with a different name can have issues later I guess. So let's prohibit changing domain name on snapshot metadata redefine as well. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 34fcf64..fa1cd6b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1307,6 +1307,14 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, goto cleanup; } + if (def->dom && + STRNEQ(def->dom->name, domain->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("definition for snapshot %s must use name %s"), + def->name, domain->name); + goto cleanup; + } + other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING || -- 1.8.3.1
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Renaming domain which has snapshots is prohibited. Also reverting to ABI compatible active domain with a different name can have issues later I guess. So let's prohibit changing domain name on snapshot metadata redefine as well.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+)
This is separable patch and I'm definitely not the expert here. I think it should go before you make patch4 and beyond changes.
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 34fcf64..fa1cd6b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1307,6 +1307,14 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, goto cleanup; }
+ if (def->dom && + STRNEQ(def->dom->name, domain->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("definition for snapshot %s must use name %s"), + def->name, domain->name); + goto cleanup; + } +
You could have also done the "if (def->dom) { ... } " type change... Not that it matters that much. Although it seems reasonable to me, hopefully Peter will pipe in as well. Reading the commit message makes me believe that this would not be possible, but then again the UUID *and* name are used as keys to look up the domain object so the names had better match. If they don't then we could be in a heap of trouble. Not sure how it would happen though. You have my R-By, but I'd certainly feel better with even more eyes and thoughts on this. Reviewed-by: John Ferlan <jferlan@redhat.com> John
other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING ||
On 21.12.2018 00:01, John Ferlan wrote:
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Renaming domain which has snapshots is prohibited. Also reverting to ABI compatible active domain with a different name can have issues later I guess. So let's prohibit changing domain name on snapshot metadata redefine as well.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 8 ++++++++ 1 file changed, 8 insertions(+)
This is separable patch and I'm definitely not the expert here. I think it should go before you make patch4 and beyond changes.
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 34fcf64..fa1cd6b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1307,6 +1307,14 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, goto cleanup; }
+ if (def->dom && + STRNEQ(def->dom->name, domain->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("definition for snapshot %s must use name %s"), + def->name, domain->name); + goto cleanup; + } +
You could have also done the "if (def->dom) { ... } " type change... Not that it matters that much.
Although it seems reasonable to me, hopefully Peter will pipe in as well. Reading the commit message makes me believe that this would not be possible, but then again the UUID *and* name are used as keys to look up the domain object so the names had better match. If they don't then we could be in a heap of trouble. Not sure how it would happen though.
Yeah it is a bit defensive check as redefine is supposed to be feed by xmls we get on dumping existing snapshot metadata so we should have consistent names and uuids. I don't think anyone sane is going to change uuid or name in xml so may be this check is excessive. UUID check along is fine as this way we won't use snapshot metadata from different domain by mistake. On the other hand it is good to have more checks as xmls are editable in the end and only limited set of editings are valid. Nikolay
You have my R-By, but I'd certainly feel better with even more eyes and thoughts on this.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING ||
This patch just adds basic checks for persistent domain config on snapshot metadata redefine. It also lets use previous version of config if it exists in previous version of metadata and not explicitly specified in new one just as in case of top level config. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index fa1cd6b..b003a07 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1315,6 +1315,22 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, goto cleanup; } + if (def->persistDom && + memcmp(def->persistDom->uuid, domain->uuid, VIR_UUID_BUFLEN)) { + virReportError(VIR_ERR_INVALID_ARG, + _("persistent definition for snapshot %s must use uuid %s"), + def->name, uuidstr); + goto cleanup; + } + + if (def->persistDom && + STRNEQ(def->persistDom->name, domain->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("persistent definition for snapshot %s must use name %s"), + def->name, domain->name); + goto cleanup; + } + other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING || @@ -1349,6 +1365,11 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } } + if (other->def->persistDom && !def->persistDom) { + def->persistDom = other->def->persistDom; + other->def->persistDom = NULL; + } + if (def->dom) { if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || virDomainSnapshotDefIsExternal(def)) { @@ -1363,6 +1384,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, other->def->dom = def->dom; def->dom = NULL; } + if (def->persistDom && !other->def->persistDom) { + other->def->persistDom = def->persistDom; + def->persistDom = NULL; + } goto cleanup; } } -- 1.8.3.1
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
This patch just adds basic checks for persistent domain config on snapshot metadata redefine. It also lets use previous version of config if it exists in previous version of metadata and not explicitly specified in new one just as in case of top level config.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
Should this actually be part of patch5? Although nice to have patches separated for review - functionality wise it's paired with managing the persistDom for the snapshot_conf.
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index fa1cd6b..b003a07 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1315,6 +1315,22 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, goto cleanup; }
+ if (def->persistDom && + memcmp(def->persistDom->uuid, domain->uuid, VIR_UUID_BUFLEN)) { + virReportError(VIR_ERR_INVALID_ARG, + _("persistent definition for snapshot %s must use uuid %s"), + def->name, uuidstr); + goto cleanup; + } + + if (def->persistDom && + STRNEQ(def->persistDom->name, domain->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("persistent definition for snapshot %s must use name %s"), + def->name, domain->name); + goto cleanup; + } +
Could go with the: if (def->persistDom) { } here too, but IDC that much. Based on any fallout from other comments received on patch7 and since this essentially "copies" things for the persistDom,
other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING || @@ -1349,6 +1365,11 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } }
+ if (other->def->persistDom && !def->persistDom) { + def->persistDom = other->def->persistDom; + other->def->persistDom = NULL; + }
Isn't this just VIR_STEAL_PTR(def->persistDom, other->def->persistDom); ?
+ if (def->dom) { if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || virDomainSnapshotDefIsExternal(def)) { @@ -1363,6 +1384,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, other->def->dom = def->dom; def->dom = NULL; } + if (def->persistDom && !other->def->persistDom) { + other->def->persistDom = def->persistDom; + def->persistDom = NULL; + }
Likewise...
goto cleanup; } }
Reviewed-by: John Ferlan <jferlan@redhat.com> John
On 21.12.2018 00:15, John Ferlan wrote:
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
This patch just adds basic checks for persistent domain config on snapshot metadata redefine. It also lets use previous version of config if it exists in previous version of metadata and not explicitly specified in new one just as in case of top level config.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
Should this actually be part of patch5? Although nice to have patches separated for review - functionality wise it's paired with managing the persistDom for the snapshot_conf.
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index fa1cd6b..b003a07 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1315,6 +1315,22 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, goto cleanup; }
+ if (def->persistDom && + memcmp(def->persistDom->uuid, domain->uuid, VIR_UUID_BUFLEN)) { + virReportError(VIR_ERR_INVALID_ARG, + _("persistent definition for snapshot %s must use uuid %s"), + def->name, uuidstr); + goto cleanup; + } + + if (def->persistDom && + STRNEQ(def->persistDom->name, domain->name)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("persistent definition for snapshot %s must use name %s"), + def->name, domain->name); + goto cleanup; + } +
Could go with the:
if (def->persistDom) { }
here too, but IDC that much.
Based on any fallout from other comments received on patch7 and since this essentially "copies" things for the persistDom,
other = virDomainSnapshotFindByName(vm->snapshots, def->name); if (other) { if ((other->def->state == VIR_DOMAIN_RUNNING || @@ -1349,6 +1365,11 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, } }
+ if (other->def->persistDom && !def->persistDom) { + def->persistDom = other->def->persistDom; + other->def->persistDom = NULL; + }
Isn't this just
VIR_STEAL_PTR(def->persistDom, other->def->persistDom);
?
+ if (def->dom) { if (def->state == VIR_DOMAIN_DISK_SNAPSHOT || virDomainSnapshotDefIsExternal(def)) { @@ -1363,6 +1384,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain, other->def->dom = def->dom; def->dom = NULL; } + if (def->persistDom && !other->def->persistDom) { + other->def->persistDom = def->persistDom; + def->persistDom = NULL; + }
Likewise...
goto cleanup;
Yeah, a bit of copy-paste. In this case I would like to add patch to use VIR_STEAL_PTR for existing code in virDomainSnapshotRedefinePrep too. Nikolay
} }
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index a599ff4..79ebce9 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -52,6 +52,17 @@ <section title="Improvements"> <change> <summary> + qemu: snapshot inactive config of active persistent domain too + </summary> + <description> + Previous libvirt versions do not store complete metadata for active + persistent domain snapshot. As a result on revert both active and + inactive configs are reverted to be the same. Now snapshot saves both + active and inactive configs. + </description> + </change> + <change> + <summary> qemu: Add support for ARMv6l guests </summary> </change> -- 1.8.3.1
On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John BTW: the explanation you give here matches my thoughts on this although what I read and commented on in previous patches wasn't as clear at least w/r/t how I read the patches...
participants (2)
-
John Ferlan -
Nikolay Shirokovskiy