From 55ca131c2c8491ce9d2aed3d589136cbd3046f95 Mon Sep 17 00:00:00 2001 From: "Joshua M. Boniface" Date: Tue, 24 Oct 2023 00:23:12 -0400 Subject: [PATCH] Handle snapshots on restore and provide options Also rename the retain option to remove superfluous plural. --- api-daemon/pvcapid/flaskapi.py | 25 ++++++++++++++------ api-daemon/pvcapid/helper.py | 6 +++-- client-cli/pvc/cli/cli.py | 28 ++++++++++++++++------- client-cli/pvc/lib/vm.py | 11 +++++---- daemon-common/ceph.py | 21 +++++++++-------- daemon-common/vm.py | 42 ++++++++++++++++++++++------------ 6 files changed, 87 insertions(+), 46 deletions(-) diff --git a/api-daemon/pvcapid/flaskapi.py b/api-daemon/pvcapid/flaskapi.py index 1ab1364a..fd532800 100755 --- a/api-daemon/pvcapid/flaskapi.py +++ b/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 ) diff --git a/api-daemon/pvcapid/helper.py b/api-daemon/pvcapid/helper.py index ae5531c4..bc6c9726 100755 --- a/api-daemon/pvcapid/helper.py +++ b/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: diff --git a/client-cli/pvc/cli/cli.py b/client-cli/pvc/cli/cli.py index c32fe288..404909a0 100644 --- a/client-cli/pvc/cli/cli.py +++ b/client-cli/pvc/cli/cli.py @@ -1606,13 +1606,13 @@ def cli_vm_flush_locks(domain): ) @click.option( "-r", - "--retain-snapshots", - "retain_snapshots", + "--retain-snapshot", + "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.") diff --git a/client-cli/pvc/lib/vm.py b/client-cli/pvc/lib/vm.py index 9571d462..09ab6628 100644 --- a/client-cli/pvc/lib/vm.py +++ b/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) diff --git a/daemon-common/ceph.py b/daemon-common/ceph.py index 0cd54dd6..2be6e800 100644 --- a/daemon-common/ceph.py +++ b/daemon-common/ceph.py @@ -1113,23 +1113,24 @@ 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 - retcode, stdout, stderr = common.run_os_command( - "rbd snap create {}/{}@{}".format(pool, volume, name) - ) - if retcode: - return ( - False, - 'ERROR: Failed to create RBD snapshot "{}" of volume "{}" in pool "{}": {}'.format( - name, volume, pool, stderr - ), + if not zk_only: + retcode, stdout, stderr = common.run_os_command( + "rbd snap create {}/{}@{}".format(pool, volume, name) ) + if retcode: + return ( + False, + 'ERROR: Failed to create RBD snapshot "{}" of volume "{}" in pool "{}": {}'.format( + name, volume, pool, stderr + ), + ) # 2. Add the snapshot to Zookeeper zkhandler.write( diff --git a/daemon-common/vm.py b/daemon-common/vm.py index 73ca1b11..32abbf8b 100644 --- a/daemon-common/vm.py +++ b/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,11 +1617,16 @@ 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) - retcode, stdout, stderr = common.run_os_command( - f"rbd snap rm {pool}/{volume}@backup_{incremental_parent}" - ) - if retcode: - return False, f"ERROR: Failed to remove imported image snapshot for {parent_volume_file}: {stderr}" + 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}" + ) + if retcode: + return False, f"ERROR: Failed to remove imported image snapshot for {parent_volume_file}: {stderr}" retcode, stdout, stderr = common.run_os_command( f"rbd snap rm {pool}/{volume}@backup_{datestring}" ) @@ -1651,11 +1660,16 @@ 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) - retcode, stdout, stderr = common.run_os_command( - f"rbd snap rm {pool}/{volume}@backup_{datestring}" - ) - if retcode: - return False, f"ERROR: Failed to remove imported image snapshot for {volume_file}: {stderr}" + 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}" + ) + if retcode: + return False, f"ERROR: Failed to remove imported image snapshot for {volume_file}: {stderr}" # 5. Start VM retcode, retmsg = start_vm(zkhandler, domain)