[libvirt PATCH 0/7] external snapshot revert fixes
This fixes reverting external snapshots to not error out in cases where it should work and makes it correctly load the memory state when reverting to snapshot of running VM. Pavel Hrdina (7): qemu_saveimage: extract starting process to qemuSaveImageStartProcess qemuSaveImageStartProcess: allow setting reason for audit log qemuSaveImageStartProcess: add snapshot argument qemuSaveImageStartProcess: make it possible to use without header qemu_snapshot: fix reverting external snapshot when not all disks are included qemu_snapshot: correctly load the saved memory state file NEWS: document support for reverting external snapshots NEWS.rst | 8 +++ src/qemu/qemu_saveimage.c | 111 ++++++++++++++++++++++++++------------ src/qemu/qemu_saveimage.h | 14 +++++ src/qemu/qemu_snapshot.c | 90 ++++++++++++++++++++----------- 4 files changed, 158 insertions(+), 65 deletions(-) -- 2.41.0
Part of qemuSaveImageStartVM() function will be used when reverting external snapshots. To avoid duplicating code and logic extract the shared bits into separate function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 103 ++++++++++++++++++++++++++------------ src/qemu/qemu_saveimage.h | 12 +++++ 2 files changed, 83 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 41310d6a9a..86f31d1820 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -565,42 +565,46 @@ qemuSaveImageOpen(virQEMUDriver *driver, return ret; } +/** + * qemuSaveImageStartProcess: + * @conn: connection object + * @driver: qemu driver object + * @vm: domain object + * @fd: FD pointer of memory state file + * @path: path to memory state file + * @header: header from memory state file + * @cookie: cookie from memory state file + * @asyncJob: type of asynchronous job + * @start_flags: flags to start QEMU process with + * @started: boolean to store if QEMU process was started + * + * Start VM with existing memory state. Make sure that the stored memory state + * is correctly decompressed so it can be loaded by QEMU process. + * + * Returns 0 on success, -1 on error. + */ int -qemuSaveImageStartVM(virConnectPtr conn, - virQEMUDriver *driver, - virDomainObj *vm, - int *fd, - virQEMUSaveData *data, - const char *path, - bool start_paused, - bool reset_nvram, - virDomainAsyncJob asyncJob) +qemuSaveImageStartProcess(virConnectPtr conn, + virQEMUDriver *driver, + virDomainObj *vm, + int *fd, + const char *path, + virQEMUSaveHeader *header, + qemuDomainSaveCookie *cookie, + virDomainAsyncJob asyncJob, + unsigned int start_flags, + bool *started) { qemuDomainObjPrivate *priv = vm->privateData; - int ret = -1; - bool started = false; - virObjectEvent *event; VIR_AUTOCLOSE intermediatefd = -1; g_autoptr(virCommand) cmd = NULL; g_autofree char *errbuf = NULL; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - virQEMUSaveHeader *header = &data->header; - g_autoptr(qemuDomainSaveCookie) cookie = NULL; int rc = 0; - unsigned int start_flags = VIR_QEMU_PROCESS_START_PAUSED | - VIR_QEMU_PROCESS_START_GEN_VMID; - - if (reset_nvram) - start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; - - if (virSaveCookieParseString(data->cookie, (virObject **)&cookie, - virDomainXMLOptionGetSaveCookie(driver->xmlopt)) < 0) - goto cleanup; if ((header->version == 2) && (header->compressed != QEMU_SAVE_FORMAT_RAW)) { if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed))) - goto cleanup; + return -1; intermediatefd = *fd; *fd = -1; @@ -613,7 +617,7 @@ qemuSaveImageStartVM(virConnectPtr conn, if (virCommandRunAsync(cmd, NULL) < 0) { *fd = intermediatefd; intermediatefd = -1; - goto cleanup; + return -1; } } @@ -622,7 +626,7 @@ qemuSaveImageStartVM(virConnectPtr conn, */ if (cookie && qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) - goto cleanup; + return -1; if (cookie && !cookie->slirpHelper) priv->disableSlirp = true; @@ -631,12 +635,12 @@ qemuSaveImageStartVM(virConnectPtr conn, asyncJob, "stdio", *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, start_flags) == 0) - started = true; + *started = true; if (intermediatefd != -1) { virErrorPtr orig_err = NULL; - if (!started) { + if (!*started) { /* if there was an error setting up qemu, the intermediate * process will wait forever to write to stdout, so we * must manually kill it and ignore any error related to @@ -656,15 +660,50 @@ qemuSaveImageStartVM(virConnectPtr conn, rc = -1; } - virDomainAuditStart(vm, "restored", started); - if (!started || rc < 0) - goto cleanup; + virDomainAuditStart(vm, "restored", *started); + if (!*started || rc < 0) + return -1; /* qemuProcessStart doesn't unset the qemu error reporting infrastructure * in case of migration (which is used in this case) so we need to reset it * so that the handle to virtlogd is not held open unnecessarily */ qemuMonitorSetDomainLog(qemuDomainGetMonitor(vm), NULL, NULL, NULL); + return 0; +} + +int +qemuSaveImageStartVM(virConnectPtr conn, + virQEMUDriver *driver, + virDomainObj *vm, + int *fd, + virQEMUSaveData *data, + const char *path, + bool start_paused, + bool reset_nvram, + virDomainAsyncJob asyncJob) +{ + int ret = -1; + bool started = false; + virObjectEvent *event; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virQEMUSaveHeader *header = &data->header; + g_autoptr(qemuDomainSaveCookie) cookie = NULL; + unsigned int start_flags = VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_GEN_VMID; + + if (reset_nvram) + start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; + + if (virSaveCookieParseString(data->cookie, (virObject **)&cookie, + virDomainXMLOptionGetSaveCookie(driver->xmlopt)) < 0) + goto cleanup; + + if (qemuSaveImageStartProcess(conn, driver, vm, fd, path, header, cookie, + asyncJob, start_flags, &started) < 0) { + goto cleanup; + } + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_RESTORED); diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index 30cf4b1ee0..af30b7f2ec 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -57,6 +57,18 @@ qemuSaveImageUpdateDef(virQEMUDriver *driver, virDomainDef *def, const char *newxml); +int +qemuSaveImageStartProcess(virConnectPtr conn, + virQEMUDriver *driver, + virDomainObj *vm, + int *fd, + const char *path, + virQEMUSaveHeader *header, + qemuDomainSaveCookie *cookie, + virDomainAsyncJob asyncJob, + unsigned int start_flags, + bool *started); + int qemuSaveImageStartVM(virConnectPtr conn, virQEMUDriver *driver, -- 2.41.0
On Thu, Aug 31, 2023 at 16:55:00 +0200, Pavel Hrdina wrote:
Part of qemuSaveImageStartVM() function will be used when reverting external snapshots. To avoid duplicating code and logic extract the shared bits into separate function.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 103 ++++++++++++++++++++++++++------------ src/qemu/qemu_saveimage.h | 12 +++++ 2 files changed, 83 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 41310d6a9a..86f31d1820 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -565,42 +565,46 @@ qemuSaveImageOpen(virQEMUDriver *driver, return ret; }
+/** + * qemuSaveImageStartProcess: + * @conn: connection object + * @driver: qemu driver object + * @vm: domain object + * @fd: FD pointer of memory state file + * @path: path to memory state file + * @header: header from memory state file + * @cookie: cookie from memory state file + * @asyncJob: type of asynchronous job + * @start_flags: flags to start QEMU process with + * @started: boolean to store if QEMU process was started + * + * Start VM with existing memory state. Make sure that the stored memory state + * is correctly decompressed so it can be loaded by QEMU process.
In subsequent patches which add the ability to conditionally load the save image or not load it and load an internal snapshot, you never update this comment so it will not be known to the reader that the function gained additional modes. Please address that in the individual commits or separately at the end. I don't mind if you also mention it here right away before subsequent patches.
+ * + * Returns 0 on success, -1 on error. + */ int -qemuSaveImageStartVM(virConnectPtr conn, - virQEMUDriver *driver, - virDomainObj *vm, - int *fd, - virQEMUSaveData *data, - const char *path, - bool start_paused, - bool reset_nvram, - virDomainAsyncJob asyncJob) +qemuSaveImageStartProcess(virConnectPtr conn, + virQEMUDriver *driver, + virDomainObj *vm, + int *fd, + const char *path, + virQEMUSaveHeader *header, + qemuDomainSaveCookie *cookie, + virDomainAsyncJob asyncJob, + unsigned int start_flags, + bool *started) { qemuDomainObjPrivate *priv = vm->privateData; - int ret = -1; - bool started = false; - virObjectEvent *event; VIR_AUTOCLOSE intermediatefd = -1; g_autoptr(virCommand) cmd = NULL; g_autofree char *errbuf = NULL; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - virQEMUSaveHeader *header = &data->header; - g_autoptr(qemuDomainSaveCookie) cookie = NULL; int rc = 0; - unsigned int start_flags = VIR_QEMU_PROCESS_START_PAUSED | - VIR_QEMU_PROCESS_START_GEN_VMID; - - if (reset_nvram) - start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; - - if (virSaveCookieParseString(data->cookie, (virObject **)&cookie, - virDomainXMLOptionGetSaveCookie(driver->xmlopt)) < 0) - goto cleanup;
if ((header->version == 2) && (header->compressed != QEMU_SAVE_FORMAT_RAW)) { if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed))) - goto cleanup; + return -1;
intermediatefd = *fd; *fd = -1; @@ -613,7 +617,7 @@ qemuSaveImageStartVM(virConnectPtr conn, if (virCommandRunAsync(cmd, NULL) < 0) { *fd = intermediatefd; intermediatefd = -1; - goto cleanup; + return -1; } }
@@ -622,7 +626,7 @@ qemuSaveImageStartVM(virConnectPtr conn, */ if (cookie && qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) - goto cleanup; + return -1;
if (cookie && !cookie->slirpHelper) priv->disableSlirp = true; @@ -631,12 +635,12 @@ qemuSaveImageStartVM(virConnectPtr conn, asyncJob, "stdio", *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, start_flags) == 0) - started = true; + *started = true;
if (intermediatefd != -1) { virErrorPtr orig_err = NULL;
- if (!started) { + if (!*started) { /* if there was an error setting up qemu, the intermediate * process will wait forever to write to stdout, so we * must manually kill it and ignore any error related to @@ -656,15 +660,50 @@ qemuSaveImageStartVM(virConnectPtr conn, rc = -1; }
- virDomainAuditStart(vm, "restored", started); - if (!started || rc < 0) - goto cleanup; + virDomainAuditStart(vm, "restored", *started); + if (!*started || rc < 0) + return -1;
/* qemuProcessStart doesn't unset the qemu error reporting infrastructure * in case of migration (which is used in this case) so we need to reset it * so that the handle to virtlogd is not held open unnecessarily */ qemuMonitorSetDomainLog(qemuDomainGetMonitor(vm), NULL, NULL, NULL);
+ return 0; +} + +int +qemuSaveImageStartVM(virConnectPtr conn, + virQEMUDriver *driver, + virDomainObj *vm, + int *fd, + virQEMUSaveData *data, + const char *path, + bool start_paused, + bool reset_nvram, + virDomainAsyncJob asyncJob) +{ + int ret = -1; + bool started = false; + virObjectEvent *event; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virQEMUSaveHeader *header = &data->header; + g_autoptr(qemuDomainSaveCookie) cookie = NULL; + unsigned int start_flags = VIR_QEMU_PROCESS_START_PAUSED | + VIR_QEMU_PROCESS_START_GEN_VMID; + + if (reset_nvram) + start_flags |= VIR_QEMU_PROCESS_START_RESET_NVRAM; + + if (virSaveCookieParseString(data->cookie, (virObject **)&cookie, + virDomainXMLOptionGetSaveCookie(driver->xmlopt)) < 0) + goto cleanup;
You certainly want the save cookie to be parsed and processed even when reverting from snapshot. Since we have a cookie both in the definition and in the save image, I still think that the one in the save image is the more correct one as it's tied to the state. We have the cookie in the snapshot definition mostly for internal snapshots.
+ + if (qemuSaveImageStartProcess(conn, driver, vm, fd, path, header, cookie, + asyncJob, start_flags, &started) < 0) { + goto cleanup; + } + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_RESTORED);
IMO qemuSaveImageStartProcess outght to process the save image cookie from the image. This applies also for v2. With that addressed: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When called by snapshot code we will need to use different reason. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 6 ++++-- src/qemu/qemu_saveimage.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 86f31d1820..1eedc900b9 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -576,6 +576,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, * @cookie: cookie from memory state file * @asyncJob: type of asynchronous job * @start_flags: flags to start QEMU process with + * @reason: audit log reason * @started: boolean to store if QEMU process was started * * Start VM with existing memory state. Make sure that the stored memory state @@ -593,6 +594,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, qemuDomainSaveCookie *cookie, virDomainAsyncJob asyncJob, unsigned int start_flags, + const char *reason, bool *started) { qemuDomainObjPrivate *priv = vm->privateData; @@ -660,7 +662,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, rc = -1; } - virDomainAuditStart(vm, "restored", *started); + virDomainAuditStart(vm, reason, started); if (!*started || rc < 0) return -1; @@ -700,7 +702,7 @@ qemuSaveImageStartVM(virConnectPtr conn, goto cleanup; if (qemuSaveImageStartProcess(conn, driver, vm, fd, path, header, cookie, - asyncJob, start_flags, &started) < 0) { + asyncJob, start_flags, "restored", &started) < 0) { goto cleanup; } diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index af30b7f2ec..c6a701dcf5 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -67,6 +67,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, qemuDomainSaveCookie *cookie, virDomainAsyncJob asyncJob, unsigned int start_flags, + const char *reason, bool *started); int -- 2.41.0
On Thu, Aug 31, 2023 at 16:55:01 +0200, Pavel Hrdina wrote:
When called by snapshot code we will need to use different reason.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 6 ++++-- src/qemu/qemu_saveimage.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 86f31d1820..1eedc900b9 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -576,6 +576,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, * @cookie: cookie from memory state file * @asyncJob: type of asynchronous job * @start_flags: flags to start QEMU process with + * @reason: audit log reason
Put expected values into the comment.
* @started: boolean to store if QEMU process was started * * Start VM with existing memory state. Make sure that the stored memory state
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When called from snapshot code we will need to pass snapshot object in order to make internal snapshots work correctly. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 6 ++++-- src/qemu/qemu_saveimage.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 1eedc900b9..73115af42d 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -572,6 +572,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, * @vm: domain object * @fd: FD pointer of memory state file * @path: path to memory state file + * @snapshot: snapshot to load when starting QEMU process or NULL * @header: header from memory state file * @cookie: cookie from memory state file * @asyncJob: type of asynchronous job @@ -590,6 +591,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, virDomainObj *vm, int *fd, const char *path, + virDomainMomentObj *snapshot, virQEMUSaveHeader *header, qemuDomainSaveCookie *cookie, virDomainAsyncJob asyncJob, @@ -634,7 +636,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, priv->disableSlirp = true; if (qemuProcessStart(conn, driver, vm, cookie ? cookie->cpu : NULL, - asyncJob, "stdio", *fd, path, NULL, + asyncJob, "stdio", *fd, path, snapshot, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, start_flags) == 0) *started = true; @@ -701,7 +703,7 @@ qemuSaveImageStartVM(virConnectPtr conn, virDomainXMLOptionGetSaveCookie(driver->xmlopt)) < 0) goto cleanup; - if (qemuSaveImageStartProcess(conn, driver, vm, fd, path, header, cookie, + if (qemuSaveImageStartProcess(conn, driver, vm, fd, path, NULL, header, cookie, asyncJob, start_flags, "restored", &started) < 0) { goto cleanup; } diff --git a/src/qemu/qemu_saveimage.h b/src/qemu/qemu_saveimage.h index c6a701dcf5..c5ad50558e 100644 --- a/src/qemu/qemu_saveimage.h +++ b/src/qemu/qemu_saveimage.h @@ -63,6 +63,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, virDomainObj *vm, int *fd, const char *path, + virDomainMomentObj *snapshot, virQEMUSaveHeader *header, qemuDomainSaveCookie *cookie, virDomainAsyncJob asyncJob, -- 2.41.0
On Thu, Aug 31, 2023 at 16:55:02 +0200, Pavel Hrdina wrote:
When called from snapshot code we will need to pass snapshot object in order to make internal snapshots work correctly.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 6 ++++-- src/qemu/qemu_saveimage.h | 1 + 2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 1eedc900b9..73115af42d 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -572,6 +572,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, * @vm: domain object * @fd: FD pointer of memory state file * @path: path to memory state file + * @snapshot: snapshot to load when starting QEMU process or NULL
At this point the function comment must include what happens if you provide it. And more specifically that user must not do that if they are reloading an image. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When used with internal snapshots there is no header to be used and no memory state to be decompressed. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 73115af42d..2538732901 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -573,7 +573,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, * @fd: FD pointer of memory state file * @path: path to memory state file * @snapshot: snapshot to load when starting QEMU process or NULL - * @header: header from memory state file + * @header: header from memory state file or NULL * @cookie: cookie from memory state file * @asyncJob: type of asynchronous job * @start_flags: flags to start QEMU process with @@ -605,7 +605,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, g_autofree char *errbuf = NULL; int rc = 0; - if ((header->version == 2) && + if (header && (header->version == 2) && (header->compressed != QEMU_SAVE_FORMAT_RAW)) { if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed))) return -1; -- 2.41.0
On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
When used with internal snapshots there is no header to be used and no memory state to be decompressed.
I didn't yet have a look at the rest, but this made me curious. What are you actually doing with this with internal snapshots? There in fact isn't a save image at all with internal snapshots as the memory image is stored in the qcow2 image (in a different section than the data -> thus also the name internal) so I'm not exactly sure what you are refering to here in the commit message.
On Thu, Aug 31, 2023 at 05:43:35PM +0200, Peter Krempa wrote:
On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
When used with internal snapshots there is no header to be used and no memory state to be decompressed.
I didn't yet have a look at the rest, but this made me curious. What are you actually doing with this with internal snapshots?
There in fact isn't a save image at all with internal snapshots as the memory image is stored in the qcow2 image (in a different section than the data -> thus also the name internal) so I'm not exactly sure what you are refering to here in the commit message.
All of that is correct. In PATCH 6/7 this new function is called from qemuSnapshotRevertActive unconditionally. And it will handle reverting internal and external snapshots and do the correct thing based on what arguments are passed to it. In case of internal snapshots we were calling qemuProcessStart and passing virDomainMomentObj. To avoid code duplication this parameter was introduced in PATCH 3/7 to this new helper so it can start QEMU process when reverting to internal snapshot as well as when reverting to external snapshots.
On Thu, Aug 31, 2023 at 17:59:27 +0200, Pavel Hrdina wrote:
On Thu, Aug 31, 2023 at 05:43:35PM +0200, Peter Krempa wrote:
On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
When used with internal snapshots there is no header to be used and no memory state to be decompressed.
I didn't yet have a look at the rest, but this made me curious. What are you actually doing with this with internal snapshots?
There in fact isn't a save image at all with internal snapshots as the memory image is stored in the qcow2 image (in a different section than the data -> thus also the name internal) so I'm not exactly sure what you are refering to here in the commit message.
All of that is correct. In PATCH 6/7 this new function is called from qemuSnapshotRevertActive unconditionally. And it will handle reverting internal and external snapshots and do the correct thing based on what arguments are passed to it.
In case of internal snapshots we were calling qemuProcessStart and passing virDomainMomentObj. To avoid code duplication this parameter was introduced in PATCH 3/7 to this new helper so it can start QEMU process when reverting to internal snapshot as well as when reverting to external snapshots.
Given how complex snapshots are I don't think it would make it any cleaner if a single function is called that has two similar but semantically very distinct behaviours and the reader has to understand that it's based on the value of the arguments which behaviour is chosen. Thus if you have both function calls and a condition selecting one of them it will be easier for readers.
On Thu, Aug 31, 2023 at 16:55:03 +0200, Pavel Hrdina wrote:
When used with internal snapshots there is no header to be used and no memory state to be decompressed.
This ...
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_saveimage.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_saveimage.c b/src/qemu/qemu_saveimage.c index 73115af42d..2538732901 100644 --- a/src/qemu/qemu_saveimage.c +++ b/src/qemu/qemu_saveimage.c @@ -573,7 +573,7 @@ qemuSaveImageOpen(virQEMUDriver *driver, * @fd: FD pointer of memory state file * @path: path to memory state file * @snapshot: snapshot to load when starting QEMU process or NULL - * @header: header from memory state file + * @header: header from memory state file or NULL * @cookie: cookie from memory state file * @asyncJob: type of asynchronous job * @start_flags: flags to start QEMU process with
... here.
@@ -605,7 +605,7 @@ qemuSaveImageStartProcess(virConnectPtr conn, g_autofree char *errbuf = NULL; int rc = 0;
- if ((header->version == 2) && + if (header && (header->version == 2) && (header->compressed != QEMU_SAVE_FORMAT_RAW)) { if (!(cmd = qemuSaveImageGetCompressionCommand(header->compressed))) return -1;
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
We need to skip all disks that have snapshot type other than 'external'. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d943281e35..ff85d56572 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2058,6 +2058,9 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i]; virDomainDiskDef *domdisk = domdef->disks[i]; + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active, false) < 0) return -1; } @@ -2098,6 +2101,9 @@ qemuSnapshotRevertExternalActive(virDomainObj *vm, return -1; for (i = 0; i < tmpsnapdef->ndisks; i++) { + if (tmpsnapdef->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (qemuSnapshotDiskPrepareOne(snapctxt, vm->def->disks[i], tmpsnapdef->disks + i, @@ -2188,6 +2194,9 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm, for (i = 0; i < curdef->nrevertdisks; i++) { virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (virStorageSourceInit(snapdisk->src) < 0 || virStorageSourceUnlink(snapdisk->src) < 0) { VIR_WARN("Failed to remove snapshot image '%s'", @@ -2203,6 +2212,9 @@ qemuSnapshotRevertExternalFinish(virDomainObj *vm, for (i = 0; i < curdef->ndisks; i++) { virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]); + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + if (virStorageSourceInit(snapdisk->src) < 0 || virStorageSourceUnlink(snapdisk->src) < 0) { VIR_WARN("Failed to remove snapshot image '%s'", -- 2.41.0
Original code assumed that the memory state file is only migration stream but it has additional metadata stored by libvirt. To correctly load the memory state file we need to reuse code that is used when restoring domain from saved image. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 78 ++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index ff85d56572..869b46a9a8 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2004,6 +2004,21 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +typedef struct _qemuSnapshotRevertMemoryData { + int fd; + char *path; + virQEMUSaveData *data; +} qemuSnapshotRevertMemoryData; + +static void +qemuSnapshotClearRevertMemoryData(qemuSnapshotRevertMemoryData *memdata) +{ + VIR_FORCE_CLOSE(memdata->fd); + virQEMUSaveDataFree(memdata->data); +} +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuSnapshotRevertMemoryData, qemuSnapshotClearRevertMemoryData); + + /** * qemuSnapshotRevertExternalPrepare: * @vm: domain object @@ -2011,15 +2026,13 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, * @snap: snapshot object we are reverting to * @config: live domain definition * @inactiveConfig: offline domain definition - * memsnapFD: pointer to store memory state file FD or NULL - * memsnapPath: pointer to store memory state file path or NULL + * @memdata: struct with data to load memory state * * Prepare new temporary snapshot definition @tmpsnapdef that will * be used while creating new overlay files after reverting to snapshot * @snap. In case we are reverting to snapshot with memory state it will - * open it and pass FD via @memsnapFD and path to the file via - * @memsnapPath, caller is responsible for freeing both @memsnapFD and - * memsnapPath. + * open it and store necessary data in @memdata. Caller is responsible + * to clear the data by using qemuSnapshotClearRevertMemoryData(). * * Returns 0 in success, -1 on error. */ @@ -2029,8 +2042,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainMomentObj *snap, virDomainDef *config, virDomainDef *inactiveConfig, - int *memsnapFD, - char **memsnapPath) + qemuSnapshotRevertMemoryData *memdata) { size_t i; bool active = virDomainObjIsActive(vm); @@ -2065,12 +2077,21 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, return -1; } - if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + if (memdata && snapdef->memorysnapshotfile) { virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virDomainDef) savedef = NULL; - *memsnapPath = snapdef->memorysnapshotfile; - *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + memdata->path = snapdef->memorysnapshotfile; + memdata->fd = qemuSaveImageOpen(driver, NULL, memdata->path, + &savedef, &memdata->data, + false, NULL, + false, false); + + if (memdata->fd < 0) + return -1; + + if (!virDomainDefCheckABIStability(savedef, domdef, driver->xmlopt)) + return -1; } return 0; @@ -2254,13 +2275,13 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; virDomainMomentObj *loadSnap = NULL; - VIR_AUTOCLOSE memsnapFD = -1; - char *memsnapPath = NULL; int detail; bool defined = false; qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; int rc; g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_auto(qemuSnapshotRevertMemoryData) memdata = { -1, NULL, NULL }; + bool started = false; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; @@ -2284,7 +2305,7 @@ qemuSnapshotRevertActive(virDomainObj *vm, if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, *config, *inactiveConfig, - &memsnapFD, &memsnapPath) < 0) { + &memdata) < 0) { return -1; } } else { @@ -2298,28 +2319,24 @@ qemuSnapshotRevertActive(virDomainObj *vm, virDomainObjAssignDef(vm, config, true, NULL); - /* No cookie means libvirt which saved the domain was too old to - * mess up the CPU definitions. - */ - if (cookie && - qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) + if (qemuSaveImageStartProcess(snapshot->domain->conn, driver, vm, + &memdata.fd, memdata.path, loadSnap, + memdata.data ? &memdata.data->header : NULL, + cookie, VIR_ASYNC_JOB_SNAPSHOT, start_flags, + "from-snapshot", &started) < 0) { + if (started) { + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, + VIR_ASYNC_JOB_SNAPSHOT, + VIR_QEMU_PROCESS_STOP_MIGRATED); + } return -1; + } - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, - cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_SNAPSHOT, NULL, memsnapFD, - memsnapPath, loadSnap, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - start_flags); - virDomainAuditStart(vm, "from-snapshot", rc >= 0); detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, detail); virObjectEventStateQueue(driver->domainEventState, event); - if (rc < 0) - return -1; - if (virDomainSnapshotIsExternal(snap)) { if (qemuSnapshotRevertExternalActive(vm, tmpsnapdef) < 0) @@ -2427,8 +2444,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, return -1; if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, - NULL, *inactiveConfig, - NULL, NULL) < 0) { + NULL, *inactiveConfig, NULL) < 0) { return -1; } -- 2.41.0
On Thu, Aug 31, 2023 at 16:55:05 +0200, Pavel Hrdina wrote:
Original code assumed that the memory state file is only migration stream but it has additional metadata stored by libvirt. To correctly load the memory state file we need to reuse code that is used when restoring domain from saved image.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 78 ++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index ff85d56572..869b46a9a8 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2004,6 +2004,21 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, }
+typedef struct _qemuSnapshotRevertMemoryData { + int fd; + char *path; + virQEMUSaveData *data; +} qemuSnapshotRevertMemoryData; + +static void +qemuSnapshotClearRevertMemoryData(qemuSnapshotRevertMemoryData *memdata) +{ + VIR_FORCE_CLOSE(memdata->fd); + virQEMUSaveDataFree(memdata->data); +} +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuSnapshotRevertMemoryData, qemuSnapshotClearRevertMemoryData); + + /** * qemuSnapshotRevertExternalPrepare: * @vm: domain object @@ -2011,15 +2026,13 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, * @snap: snapshot object we are reverting to * @config: live domain definition * @inactiveConfig: offline domain definition - * memsnapFD: pointer to store memory state file FD or NULL - * memsnapPath: pointer to store memory state file path or NULL + * @memdata: struct with data to load memory state * * Prepare new temporary snapshot definition @tmpsnapdef that will * be used while creating new overlay files after reverting to snapshot * @snap. In case we are reverting to snapshot with memory state it will - * open it and pass FD via @memsnapFD and path to the file via - * @memsnapPath, caller is responsible for freeing both @memsnapFD and - * memsnapPath. + * open it and store necessary data in @memdata. Caller is responsible + * to clear the data by using qemuSnapshotClearRevertMemoryData(). * * Returns 0 in success, -1 on error. */ @@ -2029,8 +2042,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainMomentObj *snap, virDomainDef *config, virDomainDef *inactiveConfig, - int *memsnapFD, - char **memsnapPath) + qemuSnapshotRevertMemoryData *memdata) { size_t i; bool active = virDomainObjIsActive(vm); @@ -2065,12 +2077,21 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, return -1; }
- if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + if (memdata && snapdef->memorysnapshotfile) { virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autoptr(virDomainDef) savedef = NULL;
- *memsnapPath = snapdef->memorysnapshotfile; - *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + memdata->path = snapdef->memorysnapshotfile; + memdata->fd = qemuSaveImageOpen(driver, NULL, memdata->path, + &savedef, &memdata->data, + false, NULL, + false, false); + + if (memdata->fd < 0) + return -1; + + if (!virDomainDefCheckABIStability(savedef, domdef, driver->xmlopt)) + return -1; }
return 0; @@ -2254,13 +2275,13 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; virDomainMomentObj *loadSnap = NULL; - VIR_AUTOCLOSE memsnapFD = -1; - char *memsnapPath = NULL; int detail; bool defined = false; qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; int rc; g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_auto(qemuSnapshotRevertMemoryData) memdata = { -1, NULL, NULL }; + bool started = false;
start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
@@ -2284,7 +2305,7 @@ qemuSnapshotRevertActive(virDomainObj *vm,
if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, *config, *inactiveConfig, - &memsnapFD, &memsnapPath) < 0) { + &memdata) < 0) { return -1; } } else { @@ -2298,28 +2319,24 @@ qemuSnapshotRevertActive(virDomainObj *vm,
virDomainObjAssignDef(vm, config, true, NULL);
- /* No cookie means libvirt which saved the domain was too old to - * mess up the CPU definitions. - */ - if (cookie && - qemuDomainFixupCPUs(vm, &cookie->cpu) < 0) + if (qemuSaveImageStartProcess(snapshot->domain->conn, driver, vm, + &memdata.fd, memdata.path, loadSnap, + memdata.data ? &memdata.data->header : NULL, + cookie, VIR_ASYNC_JOB_SNAPSHOT, start_flags, + "from-snapshot", &started) < 0) {
I conceptually do not like the use of a function whose name implies that a save image is used to also start qemu without a save image.
On Thu, Aug 31, 2023 at 16:55:05 +0200, Pavel Hrdina wrote:
Original code assumed that the memory state file is only migration stream but it has additional metadata stored by libvirt. To correctly load the memory state file we need to reuse code that is used when restoring domain from saved image.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 78 ++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 31 deletions(-)
This one is indeed cleaner. Note my comments about the save image cookie though. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- NEWS.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index e40c8ac259..a3be76d6cc 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -28,6 +28,14 @@ v9.7.0 (unreleased) 2) pre-binding the variant driver using the ``--driver`` option of ``virsh nodedev-detach``. + * QEMU: implement reverting external snapshots + + Reverting external snapshots is now possible using the existing API + ``virDomainSnapshotRevert()``. Management application can check host + capabilities for ``<externalSnapshot/>`` element within the list of + guest features to see if the current libvirt supports both deleting + and reverting external snapshots. + * **Improvements** * **Bug fixes** -- 2.41.0
On Thu, Aug 31, 2023 at 04:54:59PM +0200, Pavel Hrdina wrote:
This fixes reverting external snapshots to not error out in cases where it should work and makes it correctly load the memory state when reverting to snapshot of running VM.
Forget to mention that we need this to be in 9.7.0 otherwise I will have to revert commit <de71573bfec7f3acd22ec74794318de121716e21> introducing capability to indicate that reverting external snapshots is usable. Pavel
Pavel Hrdina (7): qemu_saveimage: extract starting process to qemuSaveImageStartProcess qemuSaveImageStartProcess: allow setting reason for audit log qemuSaveImageStartProcess: add snapshot argument qemuSaveImageStartProcess: make it possible to use without header qemu_snapshot: fix reverting external snapshot when not all disks are included qemu_snapshot: correctly load the saved memory state file NEWS: document support for reverting external snapshots
NEWS.rst | 8 +++ src/qemu/qemu_saveimage.c | 111 ++++++++++++++++++++++++++------------ src/qemu/qemu_saveimage.h | 14 +++++ src/qemu/qemu_snapshot.c | 90 ++++++++++++++++++++----------- 4 files changed, 158 insertions(+), 65 deletions(-)
-- 2.41.0
On Thu, Aug 31, 2023 at 17:06:35 +0200, Pavel Hrdina wrote:
On Thu, Aug 31, 2023 at 04:54:59PM +0200, Pavel Hrdina wrote:
This fixes reverting external snapshots to not error out in cases where it should work and makes it correctly load the memory state when reverting to snapshot of running VM.
Forget to mention that we need this to be in 9.7.0 otherwise I will have to revert commit <de71573bfec7f3acd22ec74794318de121716e21> introducing capability to indicate that reverting external snapshots is usable.
Given that the release is tomorrow and I conceptually don't like using the 'save image' function to open iternal snapshots which do not have a save image at all (memory state is stored _internally_ in qcow2) I think the safest will be if you also send this variant.
participants (2)
-
Pavel Hrdina -
Peter Krempa