[libvirt] [PATCHv3 0/5] Add initial support for reverting snapshots
This series adds basic support for reverting to a snapshot. For more advanced support we need the creation of snapshot branches. As of now, we will be able just to invalidate the snapshot tree from the point we're reverting to. I'll follow up with patches that add ability to delete children snapshots when reverting to a deeper tree. Peter Krempa (5): qemu: Split out guts of qemuDomainSaveImageStartVM() to allow reuse snapshot: qemu: Fix detection of external snapshots when deleting snapshot: Add flag VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED snapshot: Add flag to allow hypervisor restart when reverting snapshots snapshot: qemu: Implement reverting of external snapshots include/libvirt/libvirt.h.in | 3 + src/conf/snapshot_conf.c | 17 ++ src/conf/snapshot_conf.h | 2 + src/libvirt.c | 36 ++-- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 383 ++++++++++++++++++++++++++++++++++++++++--- tools/virsh-snapshot.c | 7 + tools/virsh.pod | 19 ++- 8 files changed, 423 insertions(+), 45 deletions(-) -- 1.8.0
The workhorse part of qemuDomainSaveImageStartVM can be reused while loading external snapshots. This patch splits the code out into a new function qemuDomainSaveImageLoad that is free of setting lifecycle events. --- Previous version ACKed. Just a repost. --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5556f1e..2ddf63a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4956,19 +4956,19 @@ error: return -1; } -static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) -qemuDomainSaveImageStartVM(virConnectPtr conn, - struct qemud_driver *driver, - virDomainObjPtr vm, - int *fd, - const struct qemud_save_header *header, - const char *path, - bool start_paused) +/* this helper loads the save image into a new qemu process */ +static int +qemuDomainSaveImageLoad(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + int *fd, + const struct qemud_save_header *header, + const char *path) + { - int ret = -1; - virDomainEventPtr event; int intermediatefd = -1; virCommandPtr cmd = NULL; + int ret = -1; if (header->version == 2) { const char *prog = qemudSaveCompressionTypeToString(header->compressed); @@ -4976,7 +4976,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, virReportError(VIR_ERR_OPERATION_FAILED, _("Invalid compressed save format %d"), header->compressed); - goto out; + goto cleanup; } if (header->compressed != QEMUD_SAVE_FORMAT_RAW) { @@ -4992,7 +4992,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, _("Failed to start decompression binary %s"), prog); *fd = intermediatefd; - goto out; + goto cleanup; } } } @@ -5022,6 +5022,30 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, ret = -1; } +cleanup: + virCommandFree(cmd); + if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, + vm->def, path) < 0) + VIR_WARN("failed to restore save state label on %s", path); + + + return ret; +} + +static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6) +qemuDomainSaveImageStartVM(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + int *fd, + const struct qemud_save_header *header, + const char *path, + bool start_paused) +{ + virDomainEventPtr event; + int ret = -1; + + ret = qemuDomainSaveImageLoad(conn, driver, vm, fd, header, path); + if (ret < 0) { virDomainAuditStart(vm, "restored", false); goto out; @@ -5062,11 +5086,6 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, ret = 0; out: - virCommandFree(cmd); - if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager, - vm->def, path) < 0) - VIR_WARN("failed to restore save state label on %s", path); - return ret; } -- 1.8.0
On 11/12/2012 10:49 AM, Peter Krempa wrote:
The workhorse part of qemuDomainSaveImageStartVM can be reused while loading external snapshots. This patch splits the code out into a new function qemuDomainSaveImageLoad that is free of setting lifecycle events. --- Previous version ACKed. Just a repost. --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 17 deletions(-)
ACK still stands. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds a helper to determine if snapshots are external and uses the helper to fix detection of those in snapshot deletion code. Snapshots are external if they have an external memory image or if the disk locations are external. As mixed snapshots are forbidden for now we need to check just one disk to know. --- - Squashed the patch adding the helper with the actual fix. - Fixed detection of external snapshots --- src/conf/snapshot_conf.c | 17 +++++++++++++++++ src/conf/snapshot_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 6 +++--- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 691950a..aa2b526 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -1023,3 +1023,20 @@ cleanup: } return ret; } + + +bool +virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap) +{ + int i; + + if (snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + return true; + + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + return true; + } + + return false; +} diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 00775ae..b5c6e25 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -155,6 +155,8 @@ int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotPtr **snaps, unsigned int flags); +bool virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap); + VIR_ENUM_DECL(virDomainSnapshotLocation) VIR_ENUM_DECL(virDomainSnapshotState) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e94b478..5a07139 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1083,6 +1083,7 @@ virDomainSnapshotFindByName; virDomainSnapshotForEach; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; +virDomainSnapshotIsExternal; virDomainSnapshotLocationTypeFromString; virDomainSnapshotLocationTypeToString; virDomainSnapshotObjListGetNames; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2ddf63a..c33080d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2025,7 +2025,7 @@ cleanup: } -/* Count how many snapshots in a set have external disk snapshots. */ +/* Count how many snapshots in a set are external snapshots or checkpoints. */ static void qemuDomainSnapshotCountExternal(void *payload, const void *name ATTRIBUTE_UNUSED, @@ -2034,7 +2034,7 @@ qemuDomainSnapshotCountExternal(void *payload, virDomainSnapshotObjPtr snap = payload; int *count = data; - if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + if (virDomainSnapshotIsExternal(snap)) (*count)++; } @@ -12531,7 +12531,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY)) { if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && - snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + virDomainSnapshotIsExternal(snap)) external++; if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) virDomainSnapshotForEachDescendant(snap, -- 1.8.0
On 11/12/2012 10:49 AM, Peter Krempa wrote:
This patch adds a helper to determine if snapshots are external and uses the helper to fix detection of those in snapshot deletion code.
Snapshots are external if they have an external memory image or if the disk locations are external. As mixed snapshots are forbidden for now we need to check just one disk to know. --- - Squashed the patch adding the helper with the actual fix. - Fixed detection of external snapshots --- src/conf/snapshot_conf.c | 17 +++++++++++++++++ src/conf/snapshot_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 6 +++--- 4 files changed, 23 insertions(+), 3 deletions(-)
ACK; this can be applied now, in part because I need this particular patch for my next post. I still need to look at the rest of your series, though. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/13/12 20:06, Eric Blake wrote:
On 11/12/2012 10:49 AM, Peter Krempa wrote:
This patch adds a helper to determine if snapshots are external and uses the helper to fix detection of those in snapshot deletion code.
Snapshots are external if they have an external memory image or if the disk locations are external. As mixed snapshots are forbidden for now we need to check just one disk to know. --- - Squashed the patch adding the helper with the actual fix. - Fixed detection of external snapshots --- src/conf/snapshot_conf.c | 17 +++++++++++++++++ src/conf/snapshot_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 6 +++--- 4 files changed, 23 insertions(+), 3 deletions(-)
ACK; this can be applied now, in part because I need this particular patch for my next post. I still need to look at the rest of your series, though.
Thanks; I pushed this one to ease the review and merge of your series. Peter
The current snapshot reverting api supported changing the state of the machine after the snapshot was reverted to either started or paused. This patch adds the ability to revert the state but to stopped state. --- - fixed libvirt.c and virsh.pod docs. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 31 ++++++++++++++++--------------- tools/virsh-snapshot.c | 3 +++ tools/virsh.pod | 15 +++++++++------ 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index bf584a0..f5bbc89 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3873,6 +3873,7 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */ VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */ + VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED = 1 << 3, /* Revert into stopped state */ } virDomainSnapshotRevertFlags; /* Revert the domain to a point-in-time snapshot. The diff --git a/src/libvirt.c b/src/libvirt.c index bdb1dc6..b6b885c 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18638,17 +18638,17 @@ error: * * Revert the domain to a given snapshot. * - * Normally, the domain will revert to the same state the domain was - * in while the snapshot was taken (whether inactive, running, or - * paused), except that disk snapshots default to reverting to - * inactive state. Including VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING in - * @flags overrides the snapshot state to guarantee a running domain - * after the revert; or including VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED in - * @flags guarantees a paused domain after the revert. These two - * flags are mutually exclusive. While a persistent domain does not - * need either flag, it is not possible to revert a transient domain - * into an inactive state, so transient domains require the use of one - * of these two flags. + * Normally, the domain will revert to the same state the domain was in while + * the snapshot was taken (whether inactive, running, or paused), except that + * disk snapshots default to reverting to inactive state. Including + * VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING in @flags overrides the snapshot state to + * guarantee a running domain after the revert; or including + * VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED in @flags guarantees a paused domain after + * the revert. With VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED contained in the flags + * the domain's memory state is not restored. These three flags are mutually + * exclusive. While a persistent domain does not need either flag, it is not + * possible to revert a transient domain into an inactive state, so transient + * domains require the use of one of these two flags. * * Reverting to any snapshot discards all configuration changes made since * the last snapshot. Additionally, reverting to a snapshot from a running @@ -18697,11 +18697,12 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto error; } - if ((flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && - (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + if ((!!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) + + !!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) + + !!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED)) > 1) { virReportInvalidArg(flags, - _("running and paused flags in %s are mutually exclusive"), - __FUNCTION__); + _("running, paused and stopped flags in %s are " + "mutually exclusive"), __FUNCTION__); goto error; } diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 4281109..952dec5 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1517,6 +1517,7 @@ static const vshCmdOptDef opts_snapshot_revert[] = { {"current", VSH_OT_BOOL, 0, N_("revert to current snapshot")}, {"running", VSH_OT_BOOL, 0, N_("after reverting, change state to running")}, {"paused", VSH_OT_BOOL, 0, N_("after reverting, change state to paused")}, + {"stopped", VSH_OT_BOOL, 0, N_("after reverting, change state to stopped")}, {"force", VSH_OT_BOOL, 0, N_("try harder on risky reverts")}, {NULL, 0, 0, NULL} }; @@ -1536,6 +1537,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING; if (vshCommandOptBool(cmd, "paused")) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED; + if (vshCommandOptBool(cmd, "stopped")) + flags |= VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED; /* We want virsh snapshot-revert --force to work even when talking * to older servers that did the unsafe revert by default but * reject the flag, so we probe without the flag, and only use it diff --git a/tools/virsh.pod b/tools/virsh.pod index 0984e6e..ce5d010 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2822,7 +2822,7 @@ Output the name of the parent snapshot, if any, for the given I<snapshot>, or for the current snapshot with I<--current>. =item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} -[{I<--running> | I<--paused>}] [I<--force>] +[{I<--running> | I<--paused> | I<--stopped>}] [I<--force>] Revert the given domain to the snapshot specified by I<snapshot>, or to the current snapshot with I<--current>. Be aware @@ -2833,11 +2833,14 @@ the original snapshot was taken. Normally, reverting to a snapshot leaves the domain in the state it was at the time the snapshot was created, except that a disk snapshot with -no vm state leaves the domain in an inactive state. Passing either the -I<--running> or I<--paused> flag will perform additional state changes -(such as booting an inactive domain, or pausing a running domain). Since -transient domains cannot be inactive, it is required to use one of these -flags when reverting to a disk snapshot of a transient domain. +no vm state leaves the domain in an inactive state. Passing one of the +I<--running>, I<--paused> or I<--stopped> flag will perform additional +state changes such as booting an inactive domain, pausing a running domain +or shutting the domain down after the snapshot is reverted. Since +transient domains cannot be inactive, it is required to use one of +I<--running> or I<--paused> flags when reverting to a disk snapshot of a +transient domain. The I<--stopped> flag cannot be used on snapshots +of transient domains. There are two cases where a snapshot revert involves extra risk, which requires the use of I<--force> to proceed. One is the case of a -- 1.8.0
On 11/12/2012 10:49 AM, Peter Krempa wrote:
The current snapshot reverting api supported changing the state of the machine after the snapshot was reverted to either started or paused.
This patch adds the ability to revert the state but to stopped state. --- - fixed libvirt.c and virsh.pod docs. --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 31 ++++++++++++++++--------------- tools/virsh-snapshot.c | 3 +++ tools/virsh.pod | 15 +++++++++------ 4 files changed, 29 insertions(+), 21 deletions(-)
+ * Normally, the domain will revert to the same state the domain was in while + * the snapshot was taken (whether inactive, running, or paused), except that + * disk snapshots default to reverting to inactive state. Including + * VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING in @flags overrides the snapshot state to + * guarantee a running domain after the revert; or including + * VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED in @flags guarantees a paused domain after + * the revert. With VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED contained in the flags + * the domain's memory state is not restored. These three flags are mutually + * exclusive. While a persistent domain does not need either flag, it is not + * possible to revert a transient domain into an inactive state, so transient + * domains require the use of one of these two flags.
Change the last sentence: While a persistent domain does not need any of these flags, it is not possible to revert a transient domain into an inactive state, so transient domains require the use of either the running or paused flag.
=item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} -[{I<--running> | I<--paused>}] [I<--force>] +[{I<--running> | I<--paused> | I<--stopped>}] [I<--force>]
Revert the given domain to the snapshot specified by I<snapshot>, or to the current snapshot with I<--current>. Be aware @@ -2833,11 +2833,14 @@ the original snapshot was taken.
Normally, reverting to a snapshot leaves the domain in the state it was at the time the snapshot was created, except that a disk snapshot with -no vm state leaves the domain in an inactive state. Passing either the -I<--running> or I<--paused> flag will perform additional state changes -(such as booting an inactive domain, or pausing a running domain). Since -transient domains cannot be inactive, it is required to use one of these -flags when reverting to a disk snapshot of a transient domain. +no vm state leaves the domain in an inactive state. Passing one of the +I<--running>, I<--paused> or I<--stopped> flag will perform additional +state changes such as booting an inactive domain, pausing a running domain +or shutting the domain down after the snapshot is reverted. Since +transient domains cannot be inactive, it is required to use one of +I<--running> or I<--paused> flags when reverting to a disk snapshot of a +transient domain. The I<--stopped> flag cannot be used on snapshots +of transient domains.
But here you did fine. ACK with the one tweak in libvirt.c. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Some hypervisors require a respawn of the hypervisor to allow reverting to some snapshot states. This patch adds flag to remove the default safe approach to not allow this. When this flag is specified the hypervisor driver should re-emit events to allow management apps to reconnect. This flag is meant as a lesser way to enforce the restart of the hypervisor, that is a fairly common possibility compared to other meanings that the existing force flag has. --- New in series. Please see the next patch on actual use semantics. I was considering how to split the very powerful meaning of VIR_DOMAIN_SNAPSHOT_REVERT_FORCE to something less powerful but still useful. (I could add a flag that would allow invalidating of snapshot/image but that wouldn't be used that much) --- include/libvirt/libvirt.h.in | 2 ++ src/libvirt.c | 5 +++++ tools/virsh-snapshot.c | 4 ++++ tools/virsh.pod | 6 +++++- 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f5bbc89..2656a58 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3874,6 +3874,8 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */ VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED = 1 << 3, /* Revert into stopped state */ + VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN = 1 << 4, /* Allow restarting of the + hypervisor */ } virDomainSnapshotRevertFlags; /* Revert the domain to a point-in-time snapshot. The diff --git a/src/libvirt.c b/src/libvirt.c index b6b885c..098fe06 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -18650,6 +18650,11 @@ error: * possible to revert a transient domain into an inactive state, so transient * domains require the use of one of these two flags. * + * Some snapshot operations may require a restart of the hypervisor to complete + * successfuly. This is normally not allowed. To override this behavior add + * VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN to @flags. The hypervisor driver should + * re-emit the appropriate events to allow reconnect of management applications. + * * Reverting to any snapshot discards all configuration changes made since * the last snapshot. Additionally, reverting to a snapshot from a running * domain is a form of data loss, since it discards whatever is in the diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 952dec5..a67cbd8 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1519,6 +1519,8 @@ static const vshCmdOptDef opts_snapshot_revert[] = { {"paused", VSH_OT_BOOL, 0, N_("after reverting, change state to paused")}, {"stopped", VSH_OT_BOOL, 0, N_("after reverting, change state to stopped")}, {"force", VSH_OT_BOOL, 0, N_("try harder on risky reverts")}, + {"allow-respawn", VSH_OT_BOOL, 0, + N_("allow respawn of hypervisor on certain operations")}, {NULL, 0, 0, NULL} }; @@ -1539,6 +1541,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED; if (vshCommandOptBool(cmd, "stopped")) flags |= VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED; + if (vshCommandOptBool(cmd, "allow-respawn")) + flags |= VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN; /* We want virsh snapshot-revert --force to work even when talking * to older servers that did the unsafe revert by default but * reject the flag, so we probe without the flag, and only use it diff --git a/tools/virsh.pod b/tools/virsh.pod index ce5d010..29e955b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2822,7 +2822,7 @@ Output the name of the parent snapshot, if any, for the given I<snapshot>, or for the current snapshot with I<--current>. =item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>} -[{I<--running> | I<--paused> | I<--stopped>}] [I<--force>] +[{I<--running> | I<--paused> | I<--stopped>}] [I<--force>] [I<--respawn>] Revert the given domain to the snapshot specified by I<snapshot>, or to the current snapshot with I<--current>. Be aware @@ -2842,6 +2842,10 @@ I<--running> or I<--paused> flags when reverting to a disk snapshot of a transient domain. The I<--stopped> flag cannot be used on snapshots of transient domains. +Some snapshot revert approaches may require a respawn of the hypervisor +process. This is not allowed by default. You may specify I<--allow-respawn> +to override this limit. + There are two cases where a snapshot revert involves extra risk, which requires the use of I<--force> to proceed. One is the case of a snapshot that lacks full domain information for reverting -- 1.8.0
On 11/12/12 18:49, Peter Krempa wrote:
Some hypervisors require a respawn of the hypervisor to allow reverting to some snapshot states. This patch adds flag to remove the default safe approach to not allow this. When this flag is specified the hypervisor driver should re-emit events to allow management apps to reconnect.
This flag is meant as a lesser way to enforce the restart of the hypervisor, that is a fairly common possibility compared to other meanings that the existing force flag has. --- New in series. Please see the next patch on actual use semantics. I was considering how to split the very powerful meaning of VIR_DOMAIN_SNAPSHOT_REVERT_FORCE to something less powerful but still useful. (I could add a flag that would allow invalidating of snapshot/image but that wouldn't be used that much) --- include/libvirt/libvirt.h.in | 2 ++ src/libvirt.c | 5 +++++ tools/virsh-snapshot.c | 4 ++++ tools/virsh.pod | 6 +++++- 4 files changed, 16 insertions(+), 1 deletion(-)
I should have added code to libvirt.c that will set the _RESPAWN flag when force is specified to save hassle. I will do it in the next version if this idea doesn't get NACKed to oblivion. Peter
On 11/13/2012 08:28 AM, Peter Krempa wrote:
I should have added code to libvirt.c that will set the _RESPAWN flag when force is specified to save hassle. I will do it in the next version if this idea doesn't get NACKed to oblivion.
I'm still thinking about the original patch, but _don't_ add code to libvirt.c that turns on one flag when another is present. Remember, virsh linked to libvirt.so 1.0.1 can call into libvirtd linked to libvirt.so 1.0.0, and if libvirt.c auto-sets the _RESPAWN flag in 1.0.1, then the RPC call will fail because 1.0.0 will reject it. Any automatic handling of assuming _RESPAWN if _FORCE is present must be done in the individual drivers, such as qemu_driver.c. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds support for reverting of external snapshots. The support is somewhat limited yet (you can only revert to a snapshot that has no children or delete the children that would have their image chains invalidated). While reverting an external snapshot, the domain has to be taken offline as there's no possibility to start a migration from file on a running machine. This poses a few difficulties when the user has virt-viewer attached as appropriate events need to be re-emitted even if the machine doesn't change states. --- The qemuDomainSnapshotCreateEvent helper func can't be split out as it's used by code added in this patch only. tweaks: - fixed typos - removed PMSUSPEND handling capability - removed support for deleting guests - reordered code to make more bulletproof - fixed potential segfault when image is missing - and a few others I can't remember. --- src/qemu/qemu_driver.c | 331 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 327 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c33080d..6cc77b7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12116,6 +12116,316 @@ qemuDomainSnapshotRevertInactive(struct qemud_driver *driver, return ret > 0 ? -1 : ret; } +/* helper to create lifecycle for possible outcomes of reverting snapshots + * the reemit parameter is used when specific events need to be reemitted + * so that virt-viewer for example is able to re-connect */ +static int +qemuDomainSnapshotCreateEvent(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainState old_state, + virDomainState new_state, + bool reemit) +{ + int reason = 0; /* generic unknown reason */ + int type = -1; + virDomainEventPtr event; + + switch (new_state) { + case VIR_DOMAIN_RUNNING: + switch (old_state) { + case VIR_DOMAIN_RUNNING: + if (!reemit) + break; + /* fallthrough */ + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_SHUTOFF: + case VIR_DOMAIN_CRASHED: + case VIR_DOMAIN_BLOCKED: + case VIR_DOMAIN_SHUTDOWN: + type = VIR_DOMAIN_EVENT_STARTED; + reason = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + break; + + case VIR_DOMAIN_PMSUSPENDED: + case VIR_DOMAIN_PAUSED: + type = VIR_DOMAIN_EVENT_RESUMED; + reason = VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT; + break; + + case VIR_DOMAIN_LAST: + /* no event */ + break; + } + break; + + case VIR_DOMAIN_PAUSED: + switch (old_state) { + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_SHUTOFF: + case VIR_DOMAIN_CRASHED: + /* the machine was started here, so we need an additional event */ + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + if (!event) + goto no_memory; + qemuDomainEventQueue(driver, event); + /* fallthrough! */ + case VIR_DOMAIN_PMSUSPENDED: + case VIR_DOMAIN_RUNNING: + case VIR_DOMAIN_SHUTDOWN: + case VIR_DOMAIN_BLOCKED: + type = VIR_DOMAIN_EVENT_SUSPENDED; + reason = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + break; + + case VIR_DOMAIN_PAUSED: + case VIR_DOMAIN_LAST: + /* no event */ + break; + } + break; + + case VIR_DOMAIN_SHUTOFF: + switch (old_state) { + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_BLOCKED: + case VIR_DOMAIN_SHUTDOWN: + case VIR_DOMAIN_PAUSED: + case VIR_DOMAIN_RUNNING: + case VIR_DOMAIN_PMSUSPENDED: + type = VIR_DOMAIN_EVENT_STOPPED; + reason = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; + break; + + case VIR_DOMAIN_SHUTOFF: + case VIR_DOMAIN_CRASHED: + case VIR_DOMAIN_LAST: + /* no event */ + break; + } + break; + + /* fallthrough */ + case VIR_DOMAIN_NOSTATE: + case VIR_DOMAIN_BLOCKED: + case VIR_DOMAIN_SHUTDOWN: + case VIR_DOMAIN_CRASHED: + case VIR_DOMAIN_LAST: + case VIR_DOMAIN_PMSUSPENDED: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Suspicious domain state '%d' after snapshot"), + new_state); + return -1; + } + + if (!(event = virDomainEventNewFromObj(vm, type, reason))) + goto no_memory; + + qemuDomainEventQueue(driver, event); + + return 0; + +no_memory: + virReportOOMError(); + return -1; +} + + +/* The domain and driver is expected to be locked */ +static int +qemuDomainSnapshotRevertExternal(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + unsigned int flags) +{ + int ret = -1; + int loadfd = -1; + + virDomainDefPtr save_def = NULL; + struct qemud_save_header header; + virFileWrapperFdPtr wrapperFd = NULL; + + virDomainState old_state = virDomainObjGetState(vm, NULL); + virDomainDefPtr config = NULL; + + /* check if domain is running and should be running afterwards: + * qemu has to be re-started to allow the migration from file + */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN || + flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE) && + virDomainObjIsActive(vm) && + !(snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT || + snap->def->state == VIR_DOMAIN_SHUTOFF)) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, + _("Reverting of snapshot '%s' needs to restart the " + "hypervisor."), snap->def->name); + goto cleanup; + } + + /* check if snapshot has children */ + if (snap->nchildren > 0) { + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { + virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", + _("Reverting of snapshots with children " + "is not implemented yet.")); + goto cleanup; + } else { + VIR_WARN("Invalidating image chain of snapshot '%s'.", + snap->def->name); + } + } + + if (vm->current_snapshot) { + vm->current_snapshot->def->current = false; + if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, + driver->snapshotDir) < 0) + goto cleanup; + vm->current_snapshot = NULL; + /* XXX Should we restore vm->current_snapshot after this point + * in the failure cases where we know there was no change? */ + } + + /* Prepare to copy the snapshot inactive xml as the config of this + * domain. Easiest way is by a round trip through xml. + * + * XXX Should domain snapshots track live xml rather + * than inactive xml? */ + snap->def->current = true; + if (snap->def->dom) { + char *xml; + if (!(xml = qemuDomainDefFormatXML(driver, + snap->def->dom, + VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_MIGRATABLE))) + goto cleanup; + + config = virDomainDefParseString(driver->caps, xml, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE); + VIR_FREE(xml); + if (!config) + goto cleanup; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Domain definition not found in snapshot '%s'"), + snap->def->name); + goto cleanup; + } + + /* try to open the memory save image if one exists and it's needed */ + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED) && + (snap->def->state == VIR_DOMAIN_RUNNING || + snap->def->state == VIR_DOMAIN_PAUSED || + flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING || + flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + + if (snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + if (!snap->def->file) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Memory image path does nott exist for " + "the snapshot")); + goto cleanup; + } + + if (!virFileExists(snap->def->file)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("memory save file '%s' does not exist."), + snap->def->file); + goto cleanup; + } + + /* open the save image file */ + if ((loadfd = qemuDomainSaveImageOpen(driver, snap->def->file, + &save_def, &header, + false, &wrapperFd, NULL, + -1, false, false)) < 0) + goto cleanup; + + /* opening succeeded, there's a big probability the revert will work. + * We can now get rid of the active qemu, if it runs */ + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Can't revert snapshot to running state: " + "Memory image not found")); + goto cleanup; + } + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + /* Now we destroy the (possibly) running qemu process. + * Unfortunately, the guest can be restored only using incoming migration. + */ + if (virDomainObjIsActive(vm)) { + qemuProcessStop(driver, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0); + virDomainAuditStop(vm, "from-snapshot"); + } + + /* assign domain definition */ + virDomainObjAssignDef(vm, config, false); + config = NULL; + + /* wipe and re-create disk images - qemu-img doesn't care if it exists*/ + if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, false) < 0) + goto endjob; + + /* save the config files */ + if (virDomainSaveConfig(driver->configDir, vm->def) < 0) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Write to config file failed")); + goto endjob; + } + + /* re-start the guest if necessary */ + if (loadfd > 0) { + if ((ret = qemuDomainSaveImageLoad(conn, driver, + vm, &loadfd, + &header, snap->def->file)) < 0) { + virDomainAuditStart(vm, "from-snapshot", false); + goto endjob; + } + + virDomainAuditStart(vm, "from-snapshot", true); + + if ((snap->def->state == VIR_DOMAIN_RUNNING && + !(flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) || + flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) { + if (qemuProcessStartCPUs(driver, vm, conn, + VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + } + } + + /* create corresponding life cycle events */ + if (qemuDomainSnapshotCreateEvent(driver, vm, old_state, + virDomainObjGetState(vm, NULL), + !!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN)) < 0) + goto endjob; + + ret = 0; +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) { + vm = NULL; + } else if (ret < 0 && !vm->persistent) { + qemuDomainRemoveInactive(driver, vm); + vm = NULL; + } + +cleanup: + virDomainDefFree(config); + virDomainDefFree(save_def); + VIR_FORCE_CLOSE(loadfd); + virFileWrapperFdFree(wrapperFd); + + return ret; +} + static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) { @@ -12133,6 +12443,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED | + VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED | + VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN | VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1); /* We have the following transitions, which create the following events: @@ -12176,12 +12488,23 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, "to revert to inactive snapshot")); goto cleanup; } - if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + + /* Check if reverting an external snapshot */ + if (virDomainSnapshotIsExternal(snap)) { + ret = qemuDomainSnapshotRevertExternal(snapshot->domain->conn, + driver, vm, snap, flags); + goto cleanup; + } + + if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED | + VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("revert to external disk snapshot not supported " - "yet")); + _("VIR_DOMAIN_SNAPSHOT_REVERT_STOPPED and " + "VIR_DOMAIN_SNAPSHOT_REVERT_RESPAWN are unsupported " + "with internal snapshots")); goto cleanup; } + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { if (!snap->def->dom) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, @@ -12200,7 +12523,6 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } - if (vm->current_snapshot) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, @@ -12233,6 +12555,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } + /* Internal snapshot revert code starts below */ if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; -- 1.8.0
participants (2)
-
Eric Blake -
Peter Krempa