[pve-devel] [RFC qemu-server] more flexible restore

[pve-devel] [RFC qemu-server] more flexible restore

[Fabian Ebner]

Allows keeping around certain disks and overriding VM settings upon
restore.

Only gave this some very rudimentary testing, because I'm not fully
restored yet after getting sick earlier this week. Didn't test the
combniation with live-restore at all yet.

First few patches are related improvements/cleanups.

Last patch depends on
https://lists.proxmox.com/pipermail/pve-devel/2022-April/052558.html


Fabian Ebner (6):
  restore: cleanup oldconf: also clean up snapshots from kept volumes
  restore destroy volumes: remove check for absolute path
  restore deactivate volumes: never die
  restore: also deactivate/destroy cloud-init disk upon error
  api: create: refactor parameter check logic
  restore: allow overriding settings upon restore

 PVE/API2/Qemu.pm  | 48 ++++++++++++++---------
 PVE/QemuServer.pm | 98 +++++++++++++++++++++++++++++++++--------------
 2 files changed, 99 insertions(+), 47 deletions(-)

-- 
2.30.2

[pve-devel] [RFC qemu-server 1/6] restore: cleanup oldconf: also clean up snapshots from kept volumes

[Fabian Ebner]

---
 PVE/QemuServer.pm | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0be6be90..f221d93c 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6228,6 +6228,8 @@ sub restore_file_archive {
 my $restore_cleanup_oldconf = sub {
     my ($storecfg, $vmid, $oldconf, $virtdev_hash) = @_;
 
+    my $kept_disks = {};
+
     PVE::QemuConfig->foreach_volume($oldconf, sub {
 	my ($ds, $drive) = @_;
 
@@ -6246,11 +6248,13 @@ my $restore_cleanup_oldconf = sub {
 	    if (my $err = $@) {
 		warn $err;
 	    }
+	} else {
+	    $kept_disks->{$volid} = 1;
 	}
     });
 
-    # delete vmstate files, after the restore we have no snapshots anymore
-    foreach my $snapname (keys %{$oldconf->{snapshots}}) {
+    # after the restore we have no snapshots anymore
+    for my $snapname (keys $oldconf->{snapshots}->%*) {
 	my $snap = $oldconf->{snapshots}->{$snapname};
 	if ($snap->{vmstate}) {
 	    eval { PVE::Storage::vdisk_free($storecfg, $snap->{vmstate}); };
@@ -6258,6 +6262,11 @@ my $restore_cleanup_oldconf = sub {
 		warn $err;
 	    }
 	}
+
+	for my $volid (keys $kept_disks->%*) {
+	    eval { PVE::Storage::volume_snapshot_delete($storecfg, $volid, $snapname); };
+	    warn $@ if $@;
+	}
     }
 };
 
-- 
2.30.2

[pve-devel] [RFC qemu-server 2/6] restore destroy volumes: remove check for absolute path

[Fabian Ebner]

Only a result from vdisk_alloc is assigned as a volid and that's never
an absolute path.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index f221d93c..be0694f9 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6470,11 +6470,7 @@ my $restore_destroy_volumes = sub {
 	my $volid = $devinfo->{$devname}->{volid};
 	next if !$volid;
 	eval {
-	    if ($volid =~ m|^/|) {
-		unlink $volid || die 'unlink failed\n';
-	    } else {
-		PVE::Storage::vdisk_free($storecfg, $volid);
-	    }
+	    PVE::Storage::vdisk_free($storecfg, $volid);
 	    print STDERR "temporary volume '$volid' sucessfuly removed\n";
 	};
 	print STDERR "unable to cleanup '$volid' - $@" if $@;
-- 
2.30.2

[pve-devel] [RFC qemu-server 3/6] restore deactivate volumes: never die

[Fabian Ebner]

Such an error shouldn't abort the whole operation.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index be0694f9..9a16b617 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6460,7 +6460,8 @@ my $restore_deactivate_volumes = sub {
 	push @$vollist, $volid if $volid;
     }
 
-    PVE::Storage::deactivate_volumes($storecfg, $vollist);
+    eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); };
+    print STDERR $@ if $@;
 };
 
 my $restore_destroy_volumes = sub {
-- 
2.30.2

[pve-devel] [RFC qemu-server 4/6] restore: also deactivate/destroy cloud-init disk upon error

[Fabian Ebner]

by re-using the same hash that's used when allocating/activating the
disks in the helpers doing the opposite.

Also in preparation to allow skipping certain disks upon restore.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/QemuServer.pm | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 9a16b617..741a5e89 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6452,12 +6452,11 @@ sub restore_update_config_line {
 }
 
 my $restore_deactivate_volumes = sub {
-    my ($storecfg, $devinfo) = @_;
+    my ($storecfg, $virtdev_hash) = @_;
 
     my $vollist = [];
-    foreach my $devname (keys %$devinfo) {
-	my $volid = $devinfo->{$devname}->{volid};
-	push @$vollist, $volid if $volid;
+    for my $dev (values $virtdev_hash->%*) {
+	push $vollist->@*, $dev->{volid} if $dev->{volid};
     }
 
     eval { PVE::Storage::deactivate_volumes($storecfg, $vollist); };
@@ -6465,11 +6464,10 @@ my $restore_deactivate_volumes = sub {
 };
 
 my $restore_destroy_volumes = sub {
-    my ($storecfg, $devinfo) = @_;
+    my ($storecfg, $virtdev_hash) = @_;
 
-    foreach my $devname (keys %$devinfo) {
-	my $volid = $devinfo->{$devname}->{volid};
-	next if !$volid;
+    for my $dev (values $virtdev_hash->%*) {
+	my $volid = $dev->{volid} or next;
 	eval {
 	    PVE::Storage::vdisk_free($storecfg, $volid);
 	    print STDERR "temporary volume '$volid' sucessfuly removed\n";
@@ -6655,7 +6653,8 @@ sub restore_proxmox_backup_archive {
     my $new_conf_raw = '';
 
     my $rpcenv = PVE::RPCEnvironment::get();
-    my $devinfo = {};
+    my $devinfo = {}; # info about drives included in backup
+    my $virtdev_hash = {}; # info about allocated drives
 
     eval {
 	# enable interrupts
@@ -6710,7 +6709,7 @@ sub restore_proxmox_backup_archive {
 	my $fh = IO::File->new($cfgfn, "r") ||
 	    die "unable to read qemu-server.conf - $!\n";
 
-	my $virtdev_hash = $parse_backup_hints->($rpcenv, $user, $storecfg, $fh, $devinfo, $options);
+	$virtdev_hash = $parse_backup_hints->($rpcenv, $user, $storecfg, $fh, $devinfo, $options);
 
 	# fixme: rate limit?
 
@@ -6771,13 +6770,13 @@ sub restore_proxmox_backup_archive {
     my $err = $@;
 
     if ($err || !$options->{live}) {
-	$restore_deactivate_volumes->($storecfg, $devinfo);
+	$restore_deactivate_volumes->($storecfg, $virtdev_hash);
     }
 
     rmtree $tmpdir;
 
     if ($err) {
-	$restore_destroy_volumes->($storecfg, $devinfo);
+	$restore_destroy_volumes->($storecfg, $virtdev_hash);
 	die $err;
     }
 
@@ -6947,7 +6946,8 @@ sub restore_vma_archive {
     my $oldtimeout;
     my $timeout = 5;
 
-    my $devinfo = {};
+    my $devinfo = {}; # info about drives included in backup
+    my $virtdev_hash = {}; # info about allocated drives
 
     my $rpcenv = PVE::RPCEnvironment::get();
 
@@ -6974,7 +6974,7 @@ sub restore_vma_archive {
 	    PVE::Tools::file_copy($fwcfgfn, "${pve_firewall_dir}/$vmid.fw");
 	}
 
-	my $virtdev_hash = $parse_backup_hints->($rpcenv, $user, $cfg, $fh, $devinfo, $opts);
+	$virtdev_hash = $parse_backup_hints->($rpcenv, $user, $cfg, $fh, $devinfo, $opts);
 
 	foreach my $info (values %{$virtdev_hash}) {
 	    my $storeid = $info->{storeid};
@@ -7080,14 +7080,14 @@ sub restore_vma_archive {
 
     alarm($oldtimeout) if $oldtimeout;
 
-    $restore_deactivate_volumes->($cfg, $devinfo);
+    $restore_deactivate_volumes->($cfg, $virtdev_hash);
 
     close($fifofh) if $fifofh;
     unlink $mapfifo;
     rmtree $tmpdir;
 
     if ($err) {
-	$restore_destroy_volumes->($cfg, $devinfo);
+	$restore_destroy_volumes->($cfg, $virtdev_hash);
 	die $err;
     }
 
-- 
2.30.2

[pve-devel] [RFC qemu-server 5/6] api: create: refactor parameter check logic

[Fabian Ebner]

In preparation to allow passing along certain parameters together with
'archive'. Moving the parameter checks to after the
conflicts-with-'archive' to ensure that the more telling error will
trigger first.

All check helpers should handle empty params fine, but check first
just to make sure and to avoid all the superfluous function calls.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3af21325..976d1bd6 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -822,22 +822,7 @@ __PACKAGE__->register_method({
 	    raise_perm_exc();
 	}
 
-	if (!$archive) {
-	    &$resolve_cdrom_alias($param);
-
-	    &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $storage);
-
-	    &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]);
-
-	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
-	    &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
-
-	    &$check_cpu_model_access($rpcenv, $authuser, $param);
-
-	    $check_drive_param->($param, $storecfg);
-
-	    PVE::QemuServer::add_random_macs($param);
-	} else {
+	if ($archive) {
 	    my $keystr = join(' ', keys %$param);
 	    raise_param_exc({ archive => "option conflicts with other options ($keystr)"}) if $keystr;
 
@@ -858,6 +843,23 @@ __PACKAGE__->register_method({
 	    }
 	}
 
+	if (scalar(keys $param->%*) > 0) {
+	    &$resolve_cdrom_alias($param);
+
+	    &$check_storage_access($rpcenv, $authuser, $storecfg, $vmid, $param, $storage);
+
+	    &$check_vm_modify_config_perm($rpcenv, $authuser, $vmid, $pool, [ keys %$param]);
+
+	    &$check_vm_create_serial_perm($rpcenv, $authuser, $vmid, $pool, $param);
+	    &$check_vm_create_usb_perm($rpcenv, $authuser, $vmid, $pool, $param);
+
+	    &$check_cpu_model_access($rpcenv, $authuser, $param);
+
+	    $check_drive_param->($param, $storecfg);
+
+	    PVE::QemuServer::add_random_macs($param);
+	}
+
 	my $emsg = $is_restore ? "unable to restore VM $vmid -" : "unable to create VM $vmid -";
 
 	eval { PVE::QemuConfig->create_and_lock_config($vmid, $force) };
-- 
2.30.2

[pve-devel] [RFC qemu-server 6/6] restore: allow overriding settings upon restore

[Fabian Ebner]

Specifying a drive parameter causes the restore operation to skip it
and keep the current drive instead.

TODO add some limits to what can be changed?

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/Qemu.pm  | 14 +++++++++++---
 PVE/QemuServer.pm | 44 ++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 976d1bd6..08b1a635 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -823,9 +823,6 @@ __PACKAGE__->register_method({
 	}
 
 	if ($archive) {
-	    my $keystr = join(' ', keys %$param);
-	    raise_param_exc({ archive => "option conflicts with other options ($keystr)"}) if $keystr;
-
 	    if ($archive eq '-') {
 		die "pipe requires cli environment\n" if $rpcenv->{type} ne 'cli';
 		$archive = { type => 'pipe' };
@@ -873,6 +870,16 @@ __PACKAGE__->register_method({
 
 	    die "$emsg vm is running\n" if PVE::QemuServer::check_running($vmid);
 
+	    for my $opt (sort keys $param->%*) {
+		next if !PVE::QemuServer::Drive::is_valid_drivename($opt);
+		my $drive = PVE::QemuServer::Drive::parse_drive($opt, $conf->{$opt});
+		my $newdrive = PVE::QemuServer::Drive::parse_drive($opt, $param->{$opt});
+		die "$opt - adding new drive during restore is not implemented\n" if !$drive;
+		die "$opt - unable to parse\n" if !$newdrive;
+		die "$opt - need to pass same volid to keep existing drive rather than restoring\n"
+		    if $drive->{file} ne $newdrive->{file};
+	    }
+
 	    my $realcmd = sub {
 		my $restore_options = {
 		    storage => $storage,
@@ -880,6 +887,7 @@ __PACKAGE__->register_method({
 		    unique => $unique,
 		    bwlimit => $bwlimit,
 		    live => $live_restore,
+		    override_conf => $param,
 		};
 		if ($archive->{type} eq 'file' || $archive->{type} eq 'pipe') {
 		    die "live-restore is only compatible with backup images from a Proxmox Backup Server\n"
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 741a5e89..95ca0444 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6295,6 +6295,7 @@ my $parse_backup_hints = sub {
     while (defined(my $line = <$fh>)) {
 	if ($line =~ m/^\#qmdump\#map:(\S+):(\S+):(\S*):(\S*):$/) {
 	    my ($virtdev, $devname, $storeid, $format) = ($1, $2, $3, $4);
+
 	    die "archive does not contain data for drive '$virtdev'\n"
 		if !$devinfo->{$devname};
 
@@ -6309,6 +6310,8 @@ my $parse_backup_hints = sub {
 	    $devinfo->{$devname}->{format} = $format;
 	    $devinfo->{$devname}->{storeid} = $storeid;
 
+	    next if $options->{override_conf}->{$virtdev}; # not being restored
+
 	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
 	    $check_storage->($storeid, $scfg); # permission and content type check
 
@@ -6476,6 +6479,17 @@ my $restore_destroy_volumes = sub {
     }
 };
 
+my $restore_merge_config = sub {
+    my ($filename, $backup_conf_raw, $override_conf) = @_;
+
+    my $backup_conf = parse_vm_config($filename, $backup_conf_raw);
+    for my $key (keys $override_conf->%*) {
+	$backup_conf->{$key} = $override_conf->{$key};
+    }
+
+    return $backup_conf;
+};
+
 sub scan_volids {
     my ($cfg, $vmid) = @_;
 
@@ -6785,8 +6799,10 @@ sub restore_proxmox_backup_archive {
 	$new_conf_raw .= "\nlock: create";
     }
 
-    PVE::Tools::file_set_contents($conffile, $new_conf_raw);
+    my $new_conf = $restore_merge_config->($conffile, $new_conf_raw, $options->{override_conf});
+    PVE::QemuConfig->write_config($vmid, $new_conf);
 
+    # TODO still needed with write_config?
     PVE::Cluster::cfs_update(); # make sure we read new file
 
     eval { rescan($vmid, 1); };
@@ -6808,6 +6824,12 @@ sub restore_proxmox_backup_archive {
 	# these special drives are already restored before start
 	delete $devinfo->{'drive-efidisk0'};
 	delete $devinfo->{'drive-tpmstate0-backup'};
+
+	for my $key (keys $options->{override_conf}->%*) {
+	    next if !is_valid_drivename($key);
+	    delete $devinfo->{"drive-$key"};
+	}
+
 	pbs_live_restore($vmid, $conf, $storecfg, $devinfo, $repo, $keyfile, $pbs_backup_name);
 
 	PVE::QemuConfig->remove_lock($vmid, "create");
@@ -7000,8 +7022,15 @@ sub restore_vma_archive {
 	my $map = $restore_allocate_devices->($cfg, $virtdev_hash, $vmid);
 
 	# print restore information to $fifofh
-	foreach my $virtdev (sort keys %$virtdev_hash) {
-	    my $d = $virtdev_hash->{$virtdev};
+	for my $devname (sort keys $devinfo->%*) {
+	    my $d = $devinfo->{$devname};
+
+	    if (!$virtdev_hash->{$d->{virtdev}}) { # skipped
+		print $fifofh "format=raw:0:$d->{devname}=/dev/null\n";
+		print "not restoring '$d->{devname}'\n";
+		next;
+	    }
+
 	    next if $d->{is_cloudinit}; # no need to restore cloudinit
 
 	    my $storeid = $d->{storeid};
@@ -7091,8 +7120,10 @@ sub restore_vma_archive {
 	die $err;
     }
 
-    PVE::Tools::file_set_contents($conffile, $new_conf_raw);
+    my $new_conf = $restore_merge_config->($conffile, $new_conf_raw, $opts->{override_conf});
+    PVE::QemuConfig->write_config($vmid, $new_conf);
 
+    # TODO still needed with write_config?
     PVE::Cluster::cfs_update(); # make sure we read new file
 
     eval { rescan($vmid, 1); };
@@ -7104,6 +7135,11 @@ sub restore_vma_archive {
 sub restore_tar_archive {
     my ($archive, $vmid, $user, $opts) = @_;
 
+    if (scalar(keys $opts->{override_conf}->%*) > 0) {
+	my $keystring = join(' ', keys $opts->{override_conf}->%*);
+	die "cannot pass along options ($keystring) when restoring from tar archive\n";
+    }
+
     if ($archive ne '-') {
 	my $firstfile = tar_archive_read_firstfile($archive);
 	die "ERROR: file '$archive' does not look like a QemuServer vzdump backup\n"
-- 
2.30.2