backup: Prevent snapshots and checkpoints at same time

Earlier patches mentioned that the initial implementation will prevent
snapshots and checkpoints from being used on the same domain at once.
However, the actual restriction is done in this separate patch to make
it easier to lift that restriction via a revert, when we are finally
ready to tackle that integration in the future.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

docs/formatcheckpoint.html.in

@@ -36,7 +36,9 @@
       now, libvirt exposes enough support to create disk checkpoints
       independently from a backup operation
       via <code>virDomainCheckpointCreateXML()</code> <span class="since">since
-      5.6.0</span>.
+      5.6.0</span>.  Likewise, the creation of checkpoints when
+      external snapshots exist is currently forbidden, although future
+      work will make it possible to integrate these two concepts.
     </p>
     <p>
       Attributes of libvirt checkpoints are stored as child elements

docs/formatsnapshot.html.in

@@ -93,7 +93,9 @@
       sets that snapshot as current, and the prior current snapshot is
       the parent of the new snapshot.  Branches in the hierarchy can
       be formed by reverting to a snapshot with a child, then creating
-      another snapshot.
+      another snapshot.  For now, the creation of external snapshots
+      when checkpoints exist is forbidden, although future work will
+      make it possible to integrate these two concepts.
     </p>
     <p>
       The top-level <code>domainsnapshot</code> element may contain

src/qemu/qemu_driver.c

@@ -15777,6 +15777,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
     if (!(vm = qemuDomObjFromDomain(domain)))
         goto cleanup;
 
+    if (virDomainListCheckpoints(vm->checkpoints, NULL, domain, NULL, 0) > 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot create snapshot while checkpoint exists"));
+        goto cleanup;
+    }
+
     cfg = virQEMUDriverGetConfig(driver);
 
     if (virDomainSnapshotCreateXMLEnsureACL(domain->conn, vm->def, flags) < 0)
@@ -18515,6 +18521,12 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base,
     if (virDomainBlockRebaseEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
+    if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot perform block rebase while checkpoint exists"));
+        goto cleanup;
+    }
+
     /* For normal rebase (enhanced blockpull), the common code handles
      * everything, including vm cleanup. */
     if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY))
@@ -18599,6 +18611,12 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml,
     if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
+    if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot perform block copy while checkpoint exists"));
+        goto cleanup;
+    }
+
     for (i = 0; i < nparams; i++) {
         virTypedParameterPtr param = &params[i];
 
@@ -18661,6 +18679,13 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth,
         return -1;
     }
 
+    if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot perform block pull while checkpoint exists"));
+        virDomainObjEndAPI(&vm);
+        return -1;
+    }
+
     return qemuDomainBlockPullCommon(dom->conn->privateData,
                                      vm, path, NULL, bandwidth, flags);
 }
@@ -18711,6 +18736,12 @@ qemuDomainBlockCommit(virDomainPtr dom,
     if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
+    if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, 0) > 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot perform block commit while checkpoint exists"));
+        goto cleanup;
+    }
+
     if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
         goto cleanup;
 
@@ -22431,6 +22462,12 @@ static int qemuDomainRename(virDomainPtr dom,
         goto endjob;
     }
 
+    if (virDomainListCheckpoints(vm->checkpoints, NULL, dom, NULL, flags) > 0) {
+        virReportError(VIR_ERR_OPERATION_INVALID,
+                       "%s", _("cannot rename domain with checkpoints"));
+        goto endjob;
+    }
+
     if (virDomainObjListRename(driver->domains, vm, new_name, flags,
                                qemuDomainRenameCallback, driver) < 0)
         goto endjob;

src/test/test_driver.c

@@ -7698,6 +7698,12 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
     if (!(vm = testDomObjFromDomain(domain)))
         goto cleanup;
 
+    if (virDomainListCheckpoints(vm->checkpoints, NULL, domain, NULL, 0) > 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot create snapshot while checkpoint exists"));
+        goto cleanup;
+    }
+
     if (!vm->persistent && (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                        _("cannot halt after transient domain snapshot"));
@@ -8158,6 +8164,12 @@ testDomainCheckpointCreateXML(virDomainPtr domain,
     if (!(vm = testDomObjFromDomain(domain)))
         goto cleanup;
 
+    if (virDomainSnapshotObjListNum(vm->snapshots, NULL, 0) > 0) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot create checkpoint while snapshot exists"));
+        goto cleanup;
+    }
+
     if (!virDomainObjIsActive(vm)) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("cannot create checkpoint for inactive domain"));

tests/virsh-checkpoint

@@ -38,6 +38,10 @@ $abs_top_builddir/tools/virsh --connect test:///default >out 2>err '
   checkpoint-create-as test c1
   checkpoint-create-as test c3
   checkpoint-create-as test c2
+  # snapshots cannot be created while checkpoints exist
+  echo --err marker
+  snapshot-create-as test s1
+  echo --err marker
   # Checking tree view (siblings sorted alphabetically)
   checkpoint-list test --tree
   # Demonstrate list filtering
@@ -77,6 +81,9 @@ Domain checkpoint c1 created
 
 Domain checkpoint c3 created
 Domain checkpoint c2 created
+
+
+
 c1
   |
   +- c3
@@ -126,6 +133,9 @@ compare exp out.cooked || fail=1
 
 cat <<EOF > exp || fail=1
 error: operation failed: domain moment c1 already exists
+error: marker
+error: Operation not supported: cannot create snapshot while checkpoint exists
+error: marker
 EOF
 compare exp err || fail=1
 

tests/virsh-snapshot

@@ -45,6 +45,10 @@ $abs_top_builddir/tools/virsh --connect test:///default >out 2>err '
   snapshot-revert test s1
   snapshot-create-as test s7
   snapshot-create-as test s8
+  # checkpoints cannot be created while snapshots exist
+  echo --err marker
+  checkpoint-create-as test c1
+  echo --err marker
   # Checking tree view (siblings sorted alphabetically)
   snapshot-list test --tree
   # Current was last one created, but we can change that
@@ -102,6 +106,9 @@ Domain snapshot s4 created
 
 Domain snapshot s7 created
 Domain snapshot s8 created
+
+
+
 s1
   |
   +- s3
@@ -175,6 +182,9 @@ compare exp out.cooked || fail=1
 cat <<EOF > exp || fail=1
 error: operation failed: domain moment s1 already exists
 error: marker
+error: Operation not supported: cannot create checkpoint while snapshot exists
+error: marker
+error: marker
 error: domain 'test' has no current snapshot
 error: marker
 EOF

tools/virsh.pod

@@ -4669,6 +4669,10 @@ a persistent domain.  However, for transient domains, snapshot
 metadata is silently lost when the domain quits running (whether
 by command such as B<destroy> or by internal guest action).
 
+For now, it is not possible to create snapshots in a domain that has
+checkpoints, although this restriction will be lifted in a future
+release.
+
 =item B<snapshot-create-as> I<domain> {[I<--print-xml>]
 [I<--no-metadata>] [I<--halt>] [I<--reuse-external>]} [I<name>]
 [I<description>] [I<--disk-only> [I<--quiesce>]] [I<--atomic>]
@@ -4735,6 +4739,10 @@ If I<--live> is specified, libvirt takes the snapshot while the guest is
 running. This increases the size of the memory image of the external
 snapshot. This is currently supported only for external full system snapshots.
 
+For now, it is not possible to create snapshots in a domain that has
+checkpoints, although this restriction will be lifted in a future
+release.
+
 =item B<snapshot-current> I<domain> {[I<--name>] | [I<--security-info>]
 | [I<snapshotname>]}
 
@@ -4943,6 +4951,10 @@ a persistent domain.  However, for transient domains, checkpoint
 metadata is silently lost when the domain quits running (whether
 by command such as B<destroy> or by internal guest action).
 
+For now, it is not possible to create checkpoints in a domain that has
+snapshots, although this restriction will be lifted in a future
+release.
+
 =item B<checkpoint-create-as> I<domain> [I<--print-xml>]
 [I<name>] [I<description>] [I<--quiesce>] [I<--diskspec>] B<diskspec>]...
 
@@ -4966,6 +4978,10 @@ If I<--quiesce> is specified, libvirt will try to use guest agent
 to freeze and unfreeze domain's mounted file systems. However,
 if domain has no guest agent, checkpoint creation will fail.
 
+For now, it is not possible to create checkpoints in a domain that has
+snapshots, although this restriction will be lifted in a future
+release.
+
 =item B<checkpoint-edit> I<domain> I<checkpointname>
 
 Edit the XML configuration file for I<checkpointname> of a domain.