[libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM

Pavel Hrdina posted 11 patches 4 years, 2 months ago
[libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM
Posted by Pavel Hrdina 4 years, 2 months ago
Our compatibility check code isn't complete and there are cases where it
fails to detect incompatible configuration and the revert fails. In
addition future support for external snapshot will always require
restarting the QEMU process.

To unify the behavior drop the compatibility check code and always
restart the QEMU process.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/qemu/qemu_snapshot.c | 66 ++++++----------------------------------
 1 file changed, 10 insertions(+), 56 deletions(-)

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 3d6ec490ab..661f74146c 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1989,62 +1989,16 @@ qemuSnapshotRevert(virDomainObj *vm,
          * to have finer control.  */
         if (virDomainObjIsActive(vm)) {
             /* Transitions 5, 6, 8, 9 */
-            /* Check for ABI compatibility. We need to do this check against
-             * the migratable XML or it will always fail otherwise */
-            bool compatible;
-
-            /* Replace the CPU in config and put the original one in priv
-             * once we're done. When we have the updated CPU def in the
-             * cookie, we don't want to replace the CPU in migratable def
-             * when doing ABI checks to make sure the current CPU exactly
-             * matches the one used at the time the snapshot was taken.
-             */
-            if (cookie && cookie->cpu && config->cpu) {
-                origCPU = config->cpu;
-                if (!(config->cpu = virCPUDefCopy(cookie->cpu)))
-                    goto endjob;
-
-                compatible = qemuDomainDefCheckABIStability(driver,
-                                                            priv->qemuCaps,
-                                                            vm->def,
-                                                            config);
-            } else {
-                compatible = qemuDomainCheckABIStability(driver, vm, config);
-            }
-
-            /* If using VM GenID, there is no way currently to change
-             * the genid for the running guest, so set an error,
-             * mark as incompatible, and don't allow change of genid
-             * if the revert force flag would start the guest again. */
-            if (compatible && config->genidRequested) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                               _("domain genid update requires restart"));
-                compatible = false;
-                start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID;
-            }
-
-            if (!compatible) {
-                virErrorPtr err = virGetLastError();
-
-                if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) {
-                    /* Re-spawn error using correct category. */
-                    if (err->code == VIR_ERR_CONFIG_UNSUPPORTED)
-                        virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s",
-                                       err->str2);
-                    goto endjob;
-                }
-                virResetError(err);
-                qemuProcessStop(driver, vm,
-                                VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
-                                QEMU_ASYNC_JOB_START, 0);
-                virDomainAuditStop(vm, "from-snapshot");
-                detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
-                event = virDomainEventLifecycleNewFromObj(vm,
-                                                 VIR_DOMAIN_EVENT_STOPPED,
-                                                 detail);
-                virObjectEventStateQueue(driver->domainEventState, event);
-                goto load;
-            }
+            qemuProcessStop(driver, vm,
+                            VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
+                            QEMU_ASYNC_JOB_START, 0);
+            virDomainAuditStop(vm, "from-snapshot");
+            detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
+            event = virDomainEventLifecycleNewFromObj(vm,
+                                                      VIR_DOMAIN_EVENT_STOPPED,
+                                                      detail);
+            virObjectEventStateQueue(driver->domainEventState, event);
+            goto load;
 
             if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
                 /* Transitions 5, 6 */
-- 
2.31.1

Re: [libvirt PATCH 02/11] qemu_snapshot: revert: always restart QEMU process for running VM
Posted by Peter Krempa 4 years, 2 months ago
On Mon, Nov 15, 2021 at 17:22:45 +0100, Pavel Hrdina wrote:
> Our compatibility check code isn't complete and there are cases where it
> fails to detect incompatible configuration and the revert fails. In
> addition future support for external snapshot will always require
> restarting the QEMU process.
> 
> To unify the behavior drop the compatibility check code and always
> restart the QEMU process.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 66 ++++++----------------------------------
>  1 file changed, 10 insertions(+), 56 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 3d6ec490ab..661f74146c 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -1989,62 +1989,16 @@ qemuSnapshotRevert(virDomainObj *vm,

[...]

>              /* Transitions 5, 6, 8, 9 */
> -            /* If using VM GenID, there is no way currently to change
> -             * the genid for the running guest, so set an error,
> -             * mark as incompatible, and don't allow change of genid
> -             * if the revert force flag would start the guest again. */
> -            if (compatible && config->genidRequested) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                               _("domain genid update requires restart"));
> -                compatible = false;
> -                start_flags &= ~VIR_QEMU_PROCESS_START_GEN_VMID;

We still need this bit. If genid is requested we must ensure that the
new start of the VM will regenerate it to ensure that the guest can
detect the reversion.

Apart from that I agree that the "feature" of not restarting qemu for
some reversions was more of a gimmick and micro-optimization and getting
rid of it for code clarity is worthwhile even when it results in the
users losing/needing reconnect the remote viewer connections.

Once you ensure that the genid is always regenerated when present:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>