Handle snapshots on restore and provide options

Also rename the retain option to remove superfluous plural.

api-daemon/pvcapid/flaskapi.py

@@ -2307,7 +2307,7 @@ class API_VM_Backup(Resource):
                 "required": False,
             },
             {
-                "name": "retain_snapshots",
+                "name": "retain_snapshot",
                 "required": False,
             },
         ]
@@ -2331,11 +2331,11 @@ class API_VM_Backup(Resource):
             required: false
             description: A previous backup datestamp to use as an incremental parent; if unspecified a full backup is taken
           - in: query
-            name: retain_snapshots
+            name: retain_snapshot
             type: boolean
             required: false
             default: false
-            description: Whether or not to retain this backup's volume snapshots to use as a future incremental parent
+            description: Whether or not to retain this backup's volume snapshots to use as a future incremental parent; full backups only
         responses:
           200:
             description: OK
@@ -2355,9 +2355,9 @@ class API_VM_Backup(Resource):
         """
         target_path = reqargs.get("target_path", None)
         incremental_parent = reqargs.get("incremental_parent", None)
-        retain_snapshots = bool(strtobool(reqargs.get("retain_snapshots", "false")))
+        retain_snapshot = bool(strtobool(reqargs.get("retain_snapshot", "false")))
         return api_helper.vm_backup(
-            vm, target_path, incremental_parent, retain_snapshots
+            vm, target_path, incremental_parent, retain_snapshot
         )
 
 
@@ -2377,7 +2377,11 @@ class API_VM_Restore(Resource):
                 "name": "backup_datestring",
                 "required": True,
                 "helptext": "A backup datestring must be specified",
-            }
+            },
+            {
+                "name": "retain_snapshot",
+                "required": False,
+            },
         ]
     )
     @Authenticator
@@ -2398,6 +2402,12 @@ class API_VM_Restore(Resource):
             type: string
             required: true
             description: The backup datestring identifier (e.g. 20230102030405)
+          - in: query
+            name: retain_snapshot
+            type: boolean
+            required: false
+            default: true
+            description: Whether or not to retain the (parent, if incremental) volume snapshot after restore
         responses:
           200:
             description: OK
@@ -2417,8 +2427,9 @@ class API_VM_Restore(Resource):
         """
         target_path = reqargs.get("target_path", None)
         backup_datestring = reqargs.get("backup_datestring", None)
+        retain_snapshot = bool(strtobool(reqargs.get("retain_snapshot", "true")))
         return api_helper.vm_restore(
-            vm, target_path, backup_datestring
+            vm, target_path, backup_datestring, retain_snapshot
         )
 
 

api-daemon/pvcapid/helper.py

@@ -476,7 +476,7 @@ def vm_backup(
     domain,
     target_path,
     incremental_parent=None,
-    retain_snapshots=False,
+    retain_snapshot=False,
 ):
     """
     Back up a VM to a local (primary coordinator) filesystem path.
@@ -486,7 +486,7 @@ def vm_backup(
         domain,
         target_path,
         incremental_parent,
-        retain_snapshots,
+        retain_snapshot,
     )
 
     if retflag:
@@ -504,6 +504,7 @@ def vm_restore(
     domain,
     target_path,
     datestring,
+    retain_snapshot=False,
 ):
     """
     Restore a VM from a local (primary coordinator) filesystem path.
@@ -513,6 +514,7 @@ def vm_restore(
         domain,
         target_path,
         datestring,
+        retain_snapshot,
     )
 
     if retflag:

client-cli/pvc/cli/cli.py

@@ -1606,13 +1606,13 @@ def cli_vm_flush_locks(domain):
 )
 @click.option(
     "-r",
-    "--retain-snapshots",
+    "--retain-snapshot",
-    "retain_snapshots",
+    "retain_snapshot",
     is_flag=True,
     default=False,
-    help="Retain volume snapshots for future incremental use.",
+    help="Retain volume snapshot for future incremental use (full only).",
 )
-def cli_vm_backup(domain, target_path, incremental_parent, retain_snapshots):
+def cli_vm_backup(domain, target_path, incremental_parent, retain_snapshot):
     """
     Create a backup of virtual machine DOMAIN to TARGET_PATH on the cluster primary coordinator. DOMAIN may be a UUID or name.
 
@@ -1622,7 +1622,7 @@ def cli_vm_backup(domain, target_path, incremental_parent, retain_snapshots):
 
     The virtual machine DOMAIN may be running, and due to snapshots the backup should be crash-consistent, but will be in an unclean state and this must be considered when restoring from backups.
 
-    Incremental snapshots are possible by specifying the "-i"/"--incremental" option along with a source backup datestring. The snapshots from that source backup must have been retained using the "-r"/"--retain-snapshots" option. Arbitrary snapshots, assuming they are valid for all attached RBD volumes, may also be used, as long as they are prefixed with "backup_". Retaining snapshots of incremental backups is supported, though it is not recommended to "chain" incremental backups in this way as it can make managing restores more difficult.
+    Incremental snapshots are possible by specifying the "-i"/"--incremental" option along with a source backup datestring. The snapshots from that source backup must have been retained using the "-r"/"--retain-snapshots" option. Retaining snapshots of incremental backups is not supported as incremental backups cannot be chained.
 
     Full backup volume images are sparse-allocated, however it is recommended for safety to consider their maximum allocated size when allocated space for the TARGET_PATH. Incremental volume images are generally small but are dependent entirely on the rate of data change in each volume.
     """
@@ -1633,7 +1633,7 @@ def cli_vm_backup(domain, target_path, incremental_parent, retain_snapshots):
         newline=False,
     )
     retcode, retmsg = pvc.lib.vm.vm_backup(
-        CLI_CONFIG, domain, target_path, incremental_parent, retain_snapshots
+        CLI_CONFIG, domain, target_path, incremental_parent, retain_snapshot
     )
     if retcode:
         echo(CLI_CONFIG, "done.")
@@ -1650,7 +1650,15 @@ def cli_vm_backup(domain, target_path, incremental_parent, retain_snapshots):
 @click.argument("domain")
 @click.argument("backup_datestring")
 @click.argument("target_path")
-def cli_vm_restore(domain, backup_datestring, target_path):
+@click.option(
+    "-r/-R",
+    "--retain-snapshot/--remove-snapshot",
+    "retain_snapshot",
+    is_flag=True,
+    default=True,
+    help="Retain or remove restored (parent, if incremental) snapshot.",
+)
+def cli_vm_restore(domain, backup_datestring, target_path, retain_snapshot):
     """
     Restore the backup BACKUP_DATESTRING of virtual machine DOMAIN stored in TARGET_PATH on the cluster primary coordinator. DOMAIN may be a UUID or name.
 
@@ -1659,6 +1667,10 @@ def cli_vm_restore(domain, backup_datestring, target_path):
     The restore will import the VM configuration, metainfo, and the point-in-time snapshot of all attached RBD volumes. Incremental backups will be automatically handled.
 
     A VM named DOMAIN must not exist; if the VM already exists, it must be removed before restoring. Renaming is not sufficient as the UUID will remain the same.
+
+    If the "-r"/"--retain-snapshot" option is specified (the default), for incremental restores, only the parent snapshot is kept; for full restores, the restored snapshot is kept. If the "-R"/"--remove-snapshot" option is specified, the imported snapshot is removed.
+
+    WARNING: The "-R"/"--remove-snapshot" option will invalidate any existing incremental backups based on the same incremental parent for the restored VM.
     """
 
     echo(
@@ -1667,7 +1679,7 @@ def cli_vm_restore(domain, backup_datestring, target_path):
         newline=False,
     )
     retcode, retmsg = pvc.lib.vm.vm_restore(
-        CLI_CONFIG, domain, target_path, backup_datestring
+        CLI_CONFIG, domain, target_path, backup_datestring, retain_snapshot
     )
     if retcode:
         echo(CLI_CONFIG, "done.")

client-cli/pvc/lib/vm.py

@@ -433,18 +433,18 @@ def vm_locks(config, vm):
     return retstatus, response.json().get("message", "")
 
 
-def vm_backup(config, vm, target_path, incremental_parent=None, retain_snapshots=False):
+def vm_backup(config, vm, target_path, incremental_parent=None, retain_snapshot=False):
     """
     Create a backup of {vm} and its volumes to a local primary coordinator filesystem path
 
     API endpoint: POST /vm/{vm}/backup
-    API arguments: target_path={target_path}, incremental_parent={incremental_parent}, retain_snapshots={retain_snapshots}
+    API arguments: target_path={target_path}, incremental_parent={incremental_parent}, retain_snapshot={retain_snapshot}
     API schema: {"message":"{data}"}
     """
     params = {
         "target_path": target_path,
         "incremental_parent": incremental_parent,
-        "retain_snapshots": retain_snapshots,
+        "retain_snapshot": retain_snapshot,
     }
     response = call_api(config, "post", "/vm/{vm}/backup".format(vm=vm), params=params)
 
@@ -454,17 +454,18 @@ def vm_backup(config, vm, target_path, incremental_parent=None, retain_snapshots
         return True, response.json().get("message", "")
 
 
-def vm_restore(config, vm, target_path, backup_datestring):
+def vm_restore(config, vm, target_path, backup_datestring, retain_snapshot=False):
     """
     Restore a backup of {vm} and its volumes from a local primary coordinator filesystem path
 
     API endpoint: POST /vm/{vm}/restore
-    API arguments: target_path={target_path}, backup_datestring={backup_datestring}
+    API arguments: target_path={target_path}, backup_datestring={backup_datestring}, retain_snapshot={retain_snapshot}
     API schema: {"message":"{data}"}
     """
     params = {
         "target_path": target_path,
         "backup_datestring": backup_datestring,
+        "retain_snapshot": retain_snapshot,
     }
     response = call_api(config, "post", "/vm/{vm}/restore".format(vm=vm), params=params)
 

daemon-common/ceph.py

@@ -1113,13 +1113,14 @@ def getCephSnapshots(zkhandler, pool, volume):
     return snapshot_list
 
 
-def add_snapshot(zkhandler, pool, volume, name):
+def add_snapshot(zkhandler, pool, volume, name, zk_only=False):
     if not verifyVolume(zkhandler, pool, volume):
         return False, 'ERROR: No volume with name "{}" is present in pool "{}".'.format(
             volume, pool
         )
 
     # 1. Create the snapshot
+    if not zk_only:
         retcode, stdout, stderr = common.run_os_command(
             "rbd snap create {}/{}@{}".format(pool, volume, name)
         )

daemon-common/vm.py

@@ -1307,12 +1307,16 @@ def get_list(
 
 
 def backup_vm(
-    zkhandler, domain, target_path, incremental_parent=None, retain_snapshots=False
+    zkhandler, domain, target_path, incremental_parent=None, retain_snapshot=False
 ):
 
     tstart = time.time()
 
     # 0. Validations
+    # Disallow retaining snapshots with an incremental parent
+    if incremental_parent is not None and retain_snapshot:
+        return False, 'ERROR: Retaining snapshots of incremental backups is not supported!'
+
     # Validate that VM exists in cluster
     dom_uuid = getDomainUUID(zkhandler, domain)
     if not dom_uuid:
@@ -1466,7 +1470,7 @@ def backup_vm(
     is_snapshot_remove_failed = False
     which_snapshot_remove_failed = list()
     msg_snapshot_remove_failed = list()
-    if not retain_snapshots:
+    if not retain_snapshot:
         for pool, volume, _ in vm_volumes:
             if ceph.verifySnapshot(zkhandler, pool, volume, snapshot_name):
                 retcode, retmsg = ceph.remove_snapshot(
@@ -1485,7 +1489,7 @@ def backup_vm(
         retlines.append(f"WARNING: Failed to remove snapshot as requested for volume(s) {', '.join(which_snapshot_remove_failed)}: {', '.join(msg_snapshot_remove_failed)}")
 
     myhostname = gethostname().split(".")[0]
-    if retain_snapshots:
+    if retain_snapshot:
         retlines.append(f"Successfully backed up VM '{domain}' ({backup_type}@{datestring}, snapshots retained) to '{myhostname}:{target_path}' in {ttot}s.")
     else:
         retlines.append(f"Successfully backed up VM '{domain}' ({backup_type}@{datestring}) to '{myhostname}:{target_path}' in {ttot}s.")
@@ -1493,7 +1497,7 @@ def backup_vm(
     return True, '\n'.join(retlines)
 
 
-def restore_vm(zkhandler, domain, source_path, datestring):
+def restore_vm(zkhandler, domain, source_path, datestring, retain_snapshot=False):
 
     tstart = time.time()
 
@@ -1613,6 +1617,11 @@ def restore_vm(zkhandler, domain, source_path, datestring):
                 return False, f"ERROR: Failed to import incremental backup image {volume_file}: {stderr}"
 
             # Finally we remove the parent and child snapshots (no longer required required)
+            if retain_snapshot:
+                retcode, retmsg = ceph.add_snapshot(zkhandler, pool, volume, f"backup_{incremental_parent}", zk_only=True)
+                if not retcode:
+                    return False, f"ERROR: Failed to add imported image snapshot for {parent_volume_file}: {retmsg}"
+            else:
                 retcode, stdout, stderr = common.run_os_command(
                     f"rbd snap rm {pool}/{volume}@backup_{incremental_parent}"
                 )
@@ -1651,6 +1660,11 @@ def restore_vm(zkhandler, domain, source_path, datestring):
                 return False, f"ERROR: Failed to import backup image {volume_file}: {stderr}"
 
             # Finally we remove the source snapshot (not required)
+            if retain_snapshot:
+                retcode, retmsg = ceph.add_snapshot(zkhandler, pool, volume, f"backup_{incremental_parent}", zk_only=True)
+                if not retcode:
+                    return False, f"ERROR: Failed to add imported image snapshot for {volume_file}: {retmsg}"
+            else:
                 retcode, stdout, stderr = common.run_os_command(
                     f"rbd snap rm {pool}/{volume}@backup_{datestring}"
                 )