[libvirt] [PATCH] qemu: snapshot: Keep non-persistent changes alive in snapshot
Restoring to a snapshot should not overwrite the persistent configuration xml of a snapshot as a side effect. This patch fixes the same. Currently, virDomainSnapshotDef only saves active domain definition of the guest. And on restore the active domain definition is used as both active and inactive domain definitions. Thiswill make the non-persistent changes persistent in snapshot image. This patch allows to save inactive domain definition as well and on snapshot-revert non-persistent configuration is restored as is. Currently, snapshot-revert is making non-presistent changes as persistent. Here are the steps to reproduce. Step1: virsh define $dom Step2: virsh attach-device $dom $memory-device.xml --live Step3: virsh snapshot-create $dom Step4: virsh destroy $dom Step5: virsh snapshot-revert $dom $snapshot-name Step6: virsh destroy $dom Step7: virsh start $dom Here we still have $memory-device attached in Step2. This patch is attempting to solve this issue. Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com> --- src/conf/snapshot_conf.c | 1 + src/conf/snapshot_conf.h | 1 + src/qemu/qemu_driver.c | 23 +++++++++++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f0e852c..e32fb4d 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->newDom); virObjectUnref(def->cookie); VIR_FREE(def); } diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 1d663c7..0bc915f 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -75,6 +75,7 @@ struct _virDomainSnapshotDef { virDomainSnapshotDiskDef *disks; virDomainDefPtr dom; + virDomainDefPtr newDom; virObjectPtr cookie; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 74fdfdb..c7cdb43 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15035,6 +15035,15 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob; + if (vm->newDef) { + if (!(xml = qemuDomainDefFormatLive(driver, vm->newDef, priv->origCPU, + true, true)) || + !(def->newDom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + goto endjob; + } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; @@ -15567,6 +15576,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; virDomainDefPtr config = NULL; + virDomainDefPtr newConfig = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; bool was_running = false; @@ -15678,6 +15688,13 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } + if (snap->def->newDom) { + newConfig = virDomainDefCopy(snap->def->newDom, caps, + driver->xmlopt, NULL, true); + if (!newConfig) + goto endjob; + } + cookie = (qemuDomainSaveCookiePtr) snap->def->cookie; switch ((virDomainState) snap->def->state) { @@ -15775,12 +15792,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virCPUDefFree(priv->origCPU); VIR_STEAL_PTR(priv->origCPU, origCPU); } + if (newConfig) + vm->newDef = newConfig; } else { /* Transitions 2, 3 */ load: was_stopped = true; if (config) virDomainObjAssignDef(vm, config, false, NULL); + if (newConfig) + vm->newDef = newConfig; /* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. @@ -15874,6 +15895,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } if (config) virDomainObjAssignDef(vm, config, false, NULL); + if (newConfig) + vm->newDef = newConfig; if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { -- 1.8.3.1
On Fri, Oct 27, 2017 at 14:02:23 +0530, Kothapally Madhu Pavan wrote:
Restoring to a snapshot should not overwrite the persistent configuration xml of a snapshot as a side effect. This patch fixes the same. Currently, virDomainSnapshotDef only saves active domain definition of the guest. And on restore the active domain definition is used as both active and inactive domain definitions. Thiswill make the non-persistent changes persistent in snapshot image. This patch allows to save inactive domain definition as well and on snapshot-revert non-persistent configuration is restored as is.
Currently, snapshot-revert is making non-presistent changes as persistent. Here are the steps to reproduce. Step1: virsh define $dom Step2: virsh attach-device $dom $memory-device.xml --live Step3: virsh snapshot-create $dom Step4: virsh destroy $dom Step5: virsh snapshot-revert $dom $snapshot-name Step6: virsh destroy $dom Step7: virsh start $dom Here we still have $memory-device attached in Step2.
I think this is actually a bit more complicated. When reverting to a snapshot, when reverting a snapshot we should revert both active and inactive configuration for backward compatibility and also because it makes sense. Imagine you made a snapshot of a running domain, played with the domain configuration and then reverted the state of the domain to the snapshot. Once you shutdown the domain and start it again you'd get a completely different machine. Of course, you actually may want such behavior. Thus we can't really guess whether a user wants to revert both active and inactive configuration or just one of them. The user should be able to tell us what to do (and we should revert both configs if no preference is given). However, for this to be really useful we need to store both active and inactive configurations when creating a snapshot of a running domain. Jirka
On 10/27/2017 02:51 PM, Jiri Denemark wrote:
On Fri, Oct 27, 2017 at 14:02:23 +0530, Kothapally Madhu Pavan wrote:
Restoring to a snapshot should not overwrite the persistent configuration xml of a snapshot as a side effect. This patch fixes the same. Currently, virDomainSnapshotDef only saves active domain definition of the guest. And on restore the active domain definition is used as both active and inactive domain definitions. Thiswill make the non-persistent changes persistent in snapshot image. This patch allows to save inactive domain definition as well and on snapshot-revert non-persistent configuration is restored as is.
Currently, snapshot-revert is making non-presistent changes as persistent. Here are the steps to reproduce. Step1: virsh define $dom Step2: virsh attach-device $dom $memory-device.xml --live Step3: virsh snapshot-create $dom Step4: virsh destroy $dom Step5: virsh snapshot-revert $dom $snapshot-name Step6: virsh destroy $dom Step7: virsh start $dom Here we still have $memory-device attached in Step2. I think this is actually a bit more complicated. When reverting to a snapshot, when reverting a snapshot we should revert both active and inactive configuration for backward compatibility and also because it makes sense. Imagine you made a snapshot of a running domain, played with the domain configuration and then reverted the state of the domain to the snapshot. Once you shutdown the domain and start it again you'd get a completely different machine. Of course, you actually may want such behavior. Thus we can't really guess whether a user wants to revert both active and inactive configuration or just one of them. The user should be able to tell us what to do (and we should revert both configs if no preference is given).
However, for this to be really useful we need to store both active and inactive configurations when creating a snapshot of a running domain. With the current behavior that I see from snapshot-list, I understood we categorize the snapshots as "Active (running, paused)" or "config(shutoff)" depending if the snapshot was taken on an active or inactive domain. With my use case what I observed was that the revert of active snapshot actually overwriting inactive domain configuration. I thought only the "config" snapshot alone can overwrite the inactive domain configuration. Hence this patch. Are you suggesting we should have both active and inactive domain configurations to be saved for an active guest and restore (both by default) OR (provide options to select)?
Madhu
Jirka
On Fri, Oct 27, 2017 at 18:47:51 +0530, Madhu Pavan wrote:
On 10/27/2017 02:51 PM, Jiri Denemark wrote:
I think this is actually a bit more complicated. When reverting to a snapshot, when reverting a snapshot we should revert both active and inactive configuration for backward compatibility and also because it makes sense. Imagine you made a snapshot of a running domain, played with the domain configuration and then reverted the state of the domain to the snapshot. Once you shutdown the domain and start it again you'd get a completely different machine. Of course, you actually may want such behavior. Thus we can't really guess whether a user wants to revert both active and inactive configuration or just one of them. The user should be able to tell us what to do (and we should revert both configs if no preference is given).
However, for this to be really useful we need to store both active and inactive configurations when creating a snapshot of a running domain. With the current behavior that I see from snapshot-list, I understood we categorize the snapshots as "Active (running, paused)" or "config(shutoff)" depending if the snapshot was taken on an active or inactive domain. With my use case what I observed was that the revert of active snapshot actually overwriting inactive domain configuration. I thought only the "config" snapshot alone can overwrite the inactive domain configuration. Hence this patch. Are you suggesting we should have both active and inactive domain configurations to be saved for an active guest and restore (both by default) OR (provide options to select)?
Yes, exactly. Jirka
On 10/27/2017 06:51 PM, Jiri Denemark wrote:
On Fri, Oct 27, 2017 at 18:47:51 +0530, Madhu Pavan wrote:
On 10/27/2017 02:51 PM, Jiri Denemark wrote:
I think this is actually a bit more complicated. When reverting to a snapshot, when reverting a snapshot we should revert both active and inactive configuration for backward compatibility and also because it makes sense. Imagine you made a snapshot of a running domain, played with the domain configuration and then reverted the state of the domain to the snapshot. Once you shutdown the domain and start it again you'd get a completely different machine. Of course, you actually may want such behavior. Thus we can't really guess whether a user wants to revert both active and inactive configuration or just one of them. The user should be able to tell us what to do (and we should revert both configs if no preference is given).
However, for this to be really useful we need to store both active and inactive configurations when creating a snapshot of a running domain. With the current behavior that I see from snapshot-list, I understood we categorize the snapshots as "Active (running, paused)" or "config(shutoff)" depending if the snapshot was taken on an active or inactive domain. With my use case what I observed was that the revert of active snapshot actually overwriting inactive domain configuration. I thought only the "config" snapshot alone can overwrite the inactive domain configuration. Hence this patch. Are you suggesting we should have both active and inactive domain configurations to be saved for an active guest and restore (both by default) OR (provide options to select)? Yes, exactly. I have sent a new patchset considering your suggestions. Here is the link to it https://www.redhat.com/archives/libvir-list/2017-October/msg01333.html
Madhu
participants (3)
-
Jiri Denemark -
Kothapally Madhu Pavan -
Madhu Pavan