[PATCH 8/9] qemu: support instant snapshots

[Peter Krempa]

On Thu, Nov 11, 2021 at 11:55:53 +0300, Nikolay Shirokovskiy wrote:
> Usual snapshot with memory of a running domain with first saves domain
> memory to disk and then make a disk snapshot. As result we get snapshot
> at moment in time much later then client asked for snapshot if domain
> memory is large. So basically you need to wait several minutes or even
> several tens of minutes before making guest unsafe changes.

Note that the API contract states that making changes is safe ONLY after
the virDomainSnapshotCreate API returns success, and ...

> 
> This patch adds instant mode to snapshot with memory of a running
> domain. In this mode snapshot is done almost at the moment of client
> request. It does not depends of domain memory size. So client can
> proceed with unsafe changes immediately (We need an event to notify
> client though as snapshot API itself is still synchronous and API
> will finish when domain memory will be stored to disk).

... this doesn't really change that. This gives users a point after
which changes done to the VM can be reverted IF the snapshot completes
successfully. It can still fail and the point in time the user wished to
capture will be lost forever.

Users will still need to wait for the API to finish to be sure. This
must me emphasised in the documentation.

Also for users of the new mode won't have any benefit as virsh will be
still waiting for the API to finish.

In general the new mode is definitely better, but we must not oversell
it.

> I dared to call this snapshot mode instant instead of background as it
> named in QEMU. IMHO in case of libvirt API name background be confused
> with asynchronous snapshot API which is not true.

This is okay. Many features have different names in libvirt.

> Instant mode basically just stops guest CPUs, makes disks snapshot and
> then start QEMU's background memory snapshot. Background here means
> if guest writes some region in memory then this memory first is written
> to disk so that eventually domain memory written to disk corresponds to
> moment of starting background snapshot.
> 
> Note that background snapshot starts guest CPUs right after snapshot
> start and do not stop CPUs after snapshot is finished unlikely to usual
> memory snapshots or migration. Nevertheless
> qemuSnapshotCreateActiveExternal calls qemuProcessStartCPUs in instant
> mode in order to lock domain images and other tasks we do not do in
> resume handler.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>  include/libvirt/libvirt-domain-snapshot.h |  2 +
>  src/qemu/qemu_snapshot.c                  | 68 ++++++++++++++++++-----
>  2 files changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
> index 90673ed0fb..2661ba2556 100644
> --- a/include/libvirt/libvirt-domain-snapshot.h
> +++ b/include/libvirt/libvirt-domain-snapshot.h
> @@ -73,6 +73,8 @@ typedef enum {
>                                                            running */
>      VIR_DOMAIN_SNAPSHOT_CREATE_VALIDATE    = (1 << 9), /* validate the XML
>                                                            against the schema */
> +    VIR_DOMAIN_SNAPSHOT_CREATE_INSTANT     = (1 << 10),/* snapshot at the moment
> +                                                          of call */
>  } virDomainSnapshotCreateFlags;
>  

We generally require that additions to public API are in a separate
patch. Additionally you must also add a broader description of the
implications of the flag in the function docs for virDomainSnapshotCreateXML
in libvirt-domain-snapshot.c.


>  /* Take a snapshot of the current VM state */
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index b521634f2a..14c4a64d52 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -1398,6 +1398,7 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
>  {
>      virObjectEvent *event;
>      bool resume = false;
> +    bool instant = false;
>      int ret = -1;
>      qemuDomainObjPrivate *priv = vm->privateData;
>      virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
> @@ -1442,13 +1443,25 @@ qemuSnapshotCreateActiveExternal(virQEMUDriver *driver,
>      if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PMSUSPENDED) {
>          pmsuspended = true;
>      } else if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
> +        if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_INSTANT) {
> +            if (!qemuMigrationCapsGet(vm, QEMU_MIGRATION_CAP_BACKGROUND_SNAPSHOT)) {
> +                virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> +                               _("Migration option 'background-snapshot'"
> +                                 " is not supported by QEMU binary"));

Don't linebreak error messages. Also users don't need to know which qemu
feature we are using and that it's a migration.

"Instant snapshots are not supported by this QEMU binary".

Also we'll probably need a check rejecting known-unsupported VM configs
such as when hugepages are in use as I remember the error from qemu
being extremely cryptic.

> +                goto cleanup;
> +            }
> +
> +            instant = true;
> +        }
> +