[pve-devel] [PATCH-SERIES qemu-server] clone_disk-related improvments

[pve-devel] [PATCH-SERIES qemu-server] clone_disk-related improvments

[Fabian Ebner]

The second patch is fixing a concrete bug with vm_{resume,suspend} not being
issued for a full clone of a running VM without guest agent. Other patches are
minor improvements, mostly trying to prevent future bugs.

Fabian Ebner (4):
  drive mirror monitor: warn when suspend/resume/freeze/thaw calls fail
  api: clone: fork before locking
  api: move disk: fork before locking
  clone disk: don't modify drive parameter

 PVE/API2/Qemu.pm  | 382 ++++++++++++++++++++++++----------------------
 PVE/QemuServer.pm |  10 +-
 2 files changed, 205 insertions(+), 187 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/4] drive mirror monitor: warn when suspend/resume/freeze/thaw calls fail

[Fabian Ebner]

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0071a06..4f531f2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7466,9 +7466,11 @@ sub qemu_drive_mirror_monitor {
 		    if ($agent_running) {
 			print "freeze filesystem\n";
 			eval { mon_cmd($vmid, "guest-fsfreeze-freeze"); };
+			warn $@ if $@;
 		    } else {
 			print "suspend vm\n";
 			eval { PVE::QemuServer::vm_suspend($vmid, 1); };
+			warn $@ if $@;
 		    }
 
 		    # if we clone a disk for a new target vm, we don't switch the disk
@@ -7477,9 +7479,11 @@ sub qemu_drive_mirror_monitor {
 		    if ($agent_running) {
 			print "unfreeze filesystem\n";
 			eval { mon_cmd($vmid, "guest-fsfreeze-thaw"); };
+			warn $@ if $@;
 		    } else {
 			print "resume vm\n";
-			eval {  PVE::QemuServer::vm_resume($vmid, 1, 1); };
+			eval { PVE::QemuServer::vm_resume($vmid, 1, 1); };
+			warn $@ if $@;
 		    }
 
 		    last;
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/4] api: clone: fork before locking

[Fabian Ebner]

using the familiar early+repeated checks pattern from other API calls.
Only intended functional changes are with regard to locking/forking.

For a full clone of a running VM without guest agent, this also fixes
issuing vm_{resume,suspend} calls for drive mirror completion.
Previously, those just timed out, because of not getting the lock:

> create full clone of drive scsi0 (rbdkvm:vm-104-disk-0)
> Formatting '/var/lib/vz/images/105/vm-105-disk-0.raw', fmt=raw
> size=4294967296 preallocation=off
> drive mirror is starting for drive-scsi0
> drive-scsi0: transferred 2.0 MiB of 4.0 GiB (0.05%) in 0s
> drive-scsi0: transferred 635.0 MiB of 4.0 GiB (15.50%) in 1s
> drive-scsi0: transferred 1.6 GiB of 4.0 GiB (40.50%) in 2s
> drive-scsi0: transferred 3.6 GiB of 4.0 GiB (90.23%) in 3s
> drive-scsi0: transferred 4.0 GiB of 4.0 GiB (100.00%) in 4s, ready
> all 'mirror' jobs are ready
> suspend vm
> trying to acquire lock...
> can't lock file '/var/lock/qemu-server/lock-104.conf' - got timeout
> drive-scsi0: Cancelling block job
> drive-scsi0: Done.
> resume vm
> trying to acquire lock...
> can't lock file '/var/lock/qemu-server/lock-104.conf' - got timeout

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Best viewed with -w.

 PVE/API2/Qemu.pm | 220 ++++++++++++++++++++++++-----------------------
 1 file changed, 112 insertions(+), 108 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6992f6f..38e08c8 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3079,9 +3079,7 @@ __PACKAGE__->register_method({
 
 	my $running = PVE::QemuServer::check_running($vmid) || 0;
 
-	my $clonefn = sub {
-	    # do all tests after lock but before forking worker - if possible
-
+	my $load_and_check = sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 	    PVE::QemuConfig->check_lock($conf);
 
@@ -3091,7 +3089,7 @@ __PACKAGE__->register_method({
 	    die "snapshot '$snapname' does not exist\n"
 		if $snapname && !defined( $conf->{snapshots}->{$snapname});
 
-	    my $full = extract_param($param, 'full') // !PVE::QemuConfig->is_template($conf);
+	    my $full = $param->{full} // !PVE::QemuConfig->is_template($conf);
 
 	    die "parameter 'storage' not allowed for linked clones\n"
 		if defined($storage) && !$full;
@@ -3156,7 +3154,13 @@ __PACKAGE__->register_method({
 		}
 	    }
 
-            # auto generate a new uuid
+	    return ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone);
+	};
+
+	my $clonefn = sub {
+	    my ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone) = $load_and_check->();
+
+	    # auto generate a new uuid
 	    my $smbios1 = PVE::QemuServer::parse_smbios1($newconf->{smbios1} || '');
 	    $smbios1->{uuid} = PVE::QemuServer::generate_uuid();
 	    $newconf->{smbios1} = PVE::QemuServer::print_smbios1($smbios1);
@@ -3181,105 +3185,99 @@ __PACKAGE__->register_method({
 	    # FIXME use PVE::QemuConfig->create_and_lock_config and adapt code
 	    PVE::Tools::file_set_contents($conffile, "# qmclone temporary file\nlock: clone\n");
 
-	    my $realcmd = sub {
-		my $upid = shift;
-
-		my $newvollist = [];
-		my $jobs = {};
-
-		eval {
-		    local $SIG{INT} =
-			local $SIG{TERM} =
-			local $SIG{QUIT} =
-			local $SIG{HUP} = sub { die "interrupted by signal\n"; };
-
-		    PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
-
-		    my $bwlimit = extract_param($param, 'bwlimit');
-
-		    my $total_jobs = scalar(keys %{$drives});
-		    my $i = 1;
-
-		    foreach my $opt (sort keys %$drives) {
-			my $drive = $drives->{$opt};
-			my $skipcomplete = ($total_jobs != $i); # finish after last drive
-			my $completion = $skipcomplete ? 'skip' : 'complete';
-
-			my $src_sid = PVE::Storage::parse_volume_id($drive->{file});
-			my $storage_list = [ $src_sid ];
-			push @$storage_list, $storage if defined($storage);
-			my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', $storage_list, $bwlimit);
-
-			my $newdrive = PVE::QemuServer::clone_disk(
-			    $storecfg,
-			    $vmid,
-			    $running,
-			    $opt,
-			    $drive,
-			    $snapname,
-			    $newid,
-			    $storage,
-			    $format,
-			    $fullclone->{$opt},
-			    $newvollist,
-			    $jobs,
-			    $completion,
-			    $oldconf->{agent},
-			    $clonelimit,
-			    $oldconf
-			);
-
-			$newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
-
-			PVE::QemuConfig->write_config($newid, $newconf);
-			$i++;
-		    }
-
-		    delete $newconf->{lock};
-
-		    # do not write pending changes
-		    if (my @changes = keys %{$newconf->{pending}}) {
-			my $pending = join(',', @changes);
-			warn "found pending changes for '$pending', discarding for clone\n";
-			delete $newconf->{pending};
-		    }
-
-		    PVE::QemuConfig->write_config($newid, $newconf);
-
-                    if ($target) {
-			# always deactivate volumes - avoid lvm LVs to be active on several nodes
-			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
-			PVE::Storage::deactivate_volumes($storecfg, $newvollist);
-
-			my $newconffile = PVE::QemuConfig->config_file($newid, $target);
-			die "Failed to move config to node '$target' - rename failed: $!\n"
-			    if !rename($conffile, $newconffile);
-		    }
-
-		    PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
-		};
-		if (my $err = $@) {
-		    eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $jobs) };
-		    sleep 1; # some storage like rbd need to wait before release volume - really?
-
-		    foreach my $volid (@$newvollist) {
-			eval { PVE::Storage::vdisk_free($storecfg, $volid); };
-			warn $@ if $@;
-		    }
-
-		    PVE::Firewall::remove_vmfw_conf($newid);
-
-		    unlink $conffile; # avoid races -> last thing before die
-
-		    die "clone failed: $err";
-		}
-
-		return;
-	    };
-
 	    PVE::Firewall::clone_vmfw_conf($vmid, $newid);
 
-	    return $rpcenv->fork_worker('qmclone', $vmid, $authuser, $realcmd);
+	    my $newvollist = [];
+	    my $jobs = {};
+
+	    eval {
+		local $SIG{INT} =
+		    local $SIG{TERM} =
+		    local $SIG{QUIT} =
+		    local $SIG{HUP} = sub { die "interrupted by signal\n"; };
+
+		PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
+
+		my $bwlimit = extract_param($param, 'bwlimit');
+
+		my $total_jobs = scalar(keys %{$drives});
+		my $i = 1;
+
+		foreach my $opt (sort keys %$drives) {
+		    my $drive = $drives->{$opt};
+		    my $skipcomplete = ($total_jobs != $i); # finish after last drive
+		    my $completion = $skipcomplete ? 'skip' : 'complete';
+
+		    my $src_sid = PVE::Storage::parse_volume_id($drive->{file});
+		    my $storage_list = [ $src_sid ];
+		    push @$storage_list, $storage if defined($storage);
+		    my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', $storage_list, $bwlimit);
+
+		    my $newdrive = PVE::QemuServer::clone_disk(
+			$storecfg,
+			$vmid,
+			$running,
+			$opt,
+			$drive,
+			$snapname,
+			$newid,
+			$storage,
+			$format,
+			$fullclone->{$opt},
+			$newvollist,
+			$jobs,
+			$completion,
+			$oldconf->{agent},
+			$clonelimit,
+			$oldconf
+		    );
+
+		    $newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
+
+		    PVE::QemuConfig->write_config($newid, $newconf);
+		    $i++;
+		}
+
+		delete $newconf->{lock};
+
+		# do not write pending changes
+		if (my @changes = keys %{$newconf->{pending}}) {
+		    my $pending = join(',', @changes);
+		    warn "found pending changes for '$pending', discarding for clone\n";
+		    delete $newconf->{pending};
+		}
+
+		PVE::QemuConfig->write_config($newid, $newconf);
+
+		if ($target) {
+		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
+		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
+		    PVE::Storage::deactivate_volumes($storecfg, $newvollist);
+
+		    my $newconffile = PVE::QemuConfig->config_file($newid, $target);
+		    die "Failed to move config to node '$target' - rename failed: $!\n"
+			if !rename($conffile, $newconffile);
+		}
+
+		PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
+	    };
+	    if (my $err = $@) {
+		eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $jobs) };
+		sleep 1; # some storage like rbd need to wait before release volume - really?
+
+		foreach my $volid (@$newvollist) {
+		    eval { PVE::Storage::vdisk_free($storecfg, $volid); };
+		    warn $@ if $@;
+		}
+
+		PVE::Firewall::remove_vmfw_conf($newid);
+
+		unlink $conffile; # avoid races -> last thing before die
+
+		die "clone failed: $err";
+	    }
+
+	    return;
 	};
 
 	# Aquire exclusive lock lock for $newid
@@ -3287,12 +3285,18 @@ __PACKAGE__->register_method({
 	    return PVE::QemuConfig->lock_config_full($newid, 1, $clonefn);
 	};
 
-	# exclusive lock if VM is running - else shared lock is enough;
-	if ($running) {
-	    return PVE::QemuConfig->lock_config_full($vmid, 1, $lock_target_vm);
-	} else {
-	    return PVE::QemuConfig->lock_config_shared($vmid, 1, $lock_target_vm);
-	}
+	my $lock_source_vm = sub {
+	    # exclusive lock if VM is running - else shared lock is enough;
+	    if ($running) {
+		return PVE::QemuConfig->lock_config_full($vmid, 1, $lock_target_vm);
+	    } else {
+		return PVE::QemuConfig->lock_config_shared($vmid, 1, $lock_target_vm);
+	    }
+	};
+
+	$load_and_check->(); # early checks before forking/locking
+
+	return $rpcenv->fork_worker('qmclone', $vmid, $authuser, $lock_source_vm);
     }});
 
 __PACKAGE__->register_method({
-- 
2.30.2

[pve-devel] [PATCH qemu-server 3/4] api: move disk: fork before locking

[Fabian Ebner]

using the familiar early+repeated checks pattern from other API calls.
Only intended functional changes are with regard to locking/forking.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Best viewed with -w

 PVE/API2/Qemu.pm | 162 +++++++++++++++++++++++++----------------------
 1 file changed, 86 insertions(+), 76 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 38e08c8..59e083e 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3396,7 +3396,7 @@ __PACKAGE__->register_method({
 
 	my $storecfg = PVE::Storage::config();
 
-	my $move_updatefn =  sub {
+	my $load_and_check_move = sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 	    PVE::QemuConfig->check_lock($conf);
 
@@ -3416,8 +3416,8 @@ __PACKAGE__->register_method({
 		$oldfmt = $1;
 	    }
 
-	    die "you can't move to the same storage with same format\n" if $oldstoreid eq $storeid &&
-                (!$format || !$oldfmt || $oldfmt eq $format);
+	    die "you can't move to the same storage with same format\n"
+		if $oldstoreid eq $storeid && (!$format || !$oldfmt || $oldfmt eq $format);
 
 	    # this only checks snapshots because $disk is passed!
 	    my $snapshotted = PVE::QemuServer::Drive::is_volume_in_use(
@@ -3429,6 +3429,13 @@ __PACKAGE__->register_method({
 	    die "you can't move a disk with snapshots and delete the source\n"
 		if $snapshotted && $param->{delete};
 
+	    return ($conf, $drive, $oldstoreid, $snapshotted);
+	};
+
+	my $move_updatefn = sub {
+	    my ($conf, $drive, $oldstoreid, $snapshotted) = $load_and_check_move->();
+	    my $old_volid = $drive->{file};
+
 	    PVE::Cluster::log_msg(
 		'info',
 		$authuser,
@@ -3439,83 +3446,79 @@ __PACKAGE__->register_method({
 
 	    PVE::Storage::activate_volumes($storecfg, [ $drive->{file} ]);
 
-	    my $realcmd = sub {
-		my $newvollist = [];
+	    my $newvollist = [];
+
+	    eval {
+		local $SIG{INT} =
+		    local $SIG{TERM} =
+		    local $SIG{QUIT} =
+		    local $SIG{HUP} = sub { die "interrupted by signal\n"; };
+
+		warn "moving disk with snapshots, snapshots will not be moved!\n"
+		    if $snapshotted;
+
+		my $bwlimit = extract_param($param, 'bwlimit');
+		my $movelimit = PVE::Storage::get_bandwidth_limit(
+		    'move',
+		    [$oldstoreid, $storeid],
+		    $bwlimit
+		);
+
+		my $newdrive = PVE::QemuServer::clone_disk(
+		    $storecfg,
+		    $vmid,
+		    $running,
+		    $disk,
+		    $drive,
+		    undef,
+		    $vmid,
+		    $storeid,
+		    $format,
+		    1,
+		    $newvollist,
+		    undef,
+		    undef,
+		    undef,
+		    $movelimit,
+		    $conf,
+		);
+		$conf->{$disk} = PVE::QemuServer::print_drive($newdrive);
+
+		PVE::QemuConfig->add_unused_volume($conf, $old_volid) if !$param->{delete};
+
+		# convert moved disk to base if part of template
+		PVE::QemuServer::template_create($vmid, $conf, $disk)
+		    if PVE::QemuConfig->is_template($conf);
+
+		PVE::QemuConfig->write_config($vmid, $conf);
+
+		my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks');
+		if ($running && $do_trim && PVE::QemuServer::qga_check_running($vmid)) {
+		    eval { mon_cmd($vmid, "guest-fstrim") };
+		}
 
 		eval {
-		    local $SIG{INT} =
-			local $SIG{TERM} =
-			local $SIG{QUIT} =
-			local $SIG{HUP} = sub { die "interrupted by signal\n"; };
-
-		    warn "moving disk with snapshots, snapshots will not be moved!\n"
-			if $snapshotted;
-
-		    my $bwlimit = extract_param($param, 'bwlimit');
-		    my $movelimit = PVE::Storage::get_bandwidth_limit(
-			'move',
-			[$oldstoreid, $storeid],
-			$bwlimit
-		    );
-
-		    my $newdrive = PVE::QemuServer::clone_disk(
-			$storecfg,
-			$vmid,
-			$running,
-			$disk,
-			$drive,
-			undef,
-			$vmid,
-			$storeid,
-			$format,
-			1,
-			$newvollist,
-			undef,
-			undef,
-			undef,
-			$movelimit,
-			$conf,
-		    );
-		    $conf->{$disk} = PVE::QemuServer::print_drive($newdrive);
-
-		    PVE::QemuConfig->add_unused_volume($conf, $old_volid) if !$param->{delete};
-
-		    # convert moved disk to base if part of template
-		    PVE::QemuServer::template_create($vmid, $conf, $disk)
-			if PVE::QemuConfig->is_template($conf);
-
-		    PVE::QemuConfig->write_config($vmid, $conf);
-
-		    my $do_trim = PVE::QemuServer::get_qga_key($conf, 'fstrim_cloned_disks');
-		    if ($running && $do_trim && PVE::QemuServer::qga_check_running($vmid)) {
-			eval { mon_cmd($vmid, "guest-fstrim") };
-		    }
-
-		    eval {
-			# try to deactivate volumes - avoid lvm LVs to be active on several nodes
-			PVE::Storage::deactivate_volumes($storecfg, [ $newdrive->{file} ])
-			    if !$running;
-		    };
-		    warn $@ if $@;
+		    # try to deactivate volumes - avoid lvm LVs to be active on several nodes
+		    PVE::Storage::deactivate_volumes($storecfg, [ $newdrive->{file} ])
+			if !$running;
 		};
-		if (my $err = $@) {
-		    foreach my $volid (@$newvollist) {
-			eval { PVE::Storage::vdisk_free($storecfg, $volid) };
-			warn $@ if $@;
-		    }
-		    die "storage migration failed: $err";
-                }
-
-		if ($param->{delete}) {
-		    eval {
-			PVE::Storage::deactivate_volumes($storecfg, [$old_volid]);
-			PVE::Storage::vdisk_free($storecfg, $old_volid);
-		    };
-		    warn $@ if $@;
-		}
+		warn $@ if $@;
 	    };
+	    if (my $err = $@) {
+		foreach my $volid (@$newvollist) {
+		    eval { PVE::Storage::vdisk_free($storecfg, $volid) };
+		    warn $@ if $@;
+		}
+		die "storage migration failed: $err";
+	    }
 
-            return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd);
+	    if ($param->{delete}) {
+		eval {
+		    PVE::Storage::deactivate_volumes($storecfg, [$old_volid]);
+		    PVE::Storage::vdisk_free($storecfg, $old_volid);
+		};
+		warn $@ if $@;
+	    }
 	};
 
 	my $load_and_check_reassign_configs = sub {
@@ -3695,7 +3698,14 @@ __PACKAGE__->register_method({
 
 	    die "cannot move disk '$disk', only configured disks can be moved to another storage\n"
 		if $disk =~ m/^unused\d+$/;
-	    return PVE::QemuConfig->lock_config($vmid, $move_updatefn);
+
+	    $load_and_check_move->(); # early checks before forking/locking
+
+	    my $realcmd = sub {
+		PVE::QemuConfig->lock_config($vmid, $move_updatefn);
+	    };
+
+	    return $rpcenv->fork_worker('qmmove', $vmid, $authuser, $realcmd);
 	} else {
 	    my $msg = "both 'storage' and 'target-vmid' missing, either needs to be set";
 	    raise_param_exc({ 'target-vmid' => $msg, 'storage' => $msg });
-- 
2.30.2

[pve-devel] [PATCH qemu-server 4/4] clone disk: don't modify drive parameter

[Fabian Ebner]

While existing callers are not using the parameter after the call,
the modification is rather unexpected and could lead to bugs quickly.

Also avoid setting an undef value in the hash, but use delete instead.

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

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 4f531f2..8aa1946 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -7642,8 +7642,8 @@ sub clone_disk {
 no_data_clone:
     my ($size) = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 10) };
 
-    my $disk = $drive;
-    $disk->{format} = undef;
+    my $disk = dclone($drive);
+    delete $disk->{format};
     $disk->{file} = $newvolid;
     $disk->{size} = $size if defined($size);
 
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 2/4] api: clone: fork before locking

[Fabian Grünbichler]

On January 27, 2022 3:01 pm, Fabian Ebner wrote:
> using the familiar early+repeated checks pattern from other API calls.
> Only intended functional changes are with regard to locking/forking.

two questions:
- the FW config cloning happens inside the worker now, while it was 
  previously before forking the worker (LGTM, but might be called out 
  explicitly if intentional ;))
- there are some checks at the start of the endpoint (checking 
  storage/target), which are not repeated after the fork+lock - while 
  unlikely, our view of storage.cfg could change in-between (lock guest 
  config -> cfs_update). should those be moved in the check sub (or into 
  the check_storage_access_clone helper)?

rest of the series LGTM

> 
> For a full clone of a running VM without guest agent, this also fixes
> issuing vm_{resume,suspend} calls for drive mirror completion.
> Previously, those just timed out, because of not getting the lock:
> 
>> create full clone of drive scsi0 (rbdkvm:vm-104-disk-0)
>> Formatting '/var/lib/vz/images/105/vm-105-disk-0.raw', fmt=raw
>> size=4294967296 preallocation=off
>> drive mirror is starting for drive-scsi0
>> drive-scsi0: transferred 2.0 MiB of 4.0 GiB (0.05%) in 0s
>> drive-scsi0: transferred 635.0 MiB of 4.0 GiB (15.50%) in 1s
>> drive-scsi0: transferred 1.6 GiB of 4.0 GiB (40.50%) in 2s
>> drive-scsi0: transferred 3.6 GiB of 4.0 GiB (90.23%) in 3s
>> drive-scsi0: transferred 4.0 GiB of 4.0 GiB (100.00%) in 4s, ready
>> all 'mirror' jobs are ready
>> suspend vm
>> trying to acquire lock...
>> can't lock file '/var/lock/qemu-server/lock-104.conf' - got timeout
>> drive-scsi0: Cancelling block job
>> drive-scsi0: Done.
>> resume vm
>> trying to acquire lock...
>> can't lock file '/var/lock/qemu-server/lock-104.conf' - got timeout
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Best viewed with -w.
> 
>  PVE/API2/Qemu.pm | 220 ++++++++++++++++++++++++-----------------------
>  1 file changed, 112 insertions(+), 108 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6992f6f..38e08c8 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -3079,9 +3079,7 @@ __PACKAGE__->register_method({
>  
>  	my $running = PVE::QemuServer::check_running($vmid) || 0;
>  
> -	my $clonefn = sub {
> -	    # do all tests after lock but before forking worker - if possible
> -
> +	my $load_and_check = sub {
>  	    my $conf = PVE::QemuConfig->load_config($vmid);
>  	    PVE::QemuConfig->check_lock($conf);
>  
> @@ -3091,7 +3089,7 @@ __PACKAGE__->register_method({
>  	    die "snapshot '$snapname' does not exist\n"
>  		if $snapname && !defined( $conf->{snapshots}->{$snapname});
>  
> -	    my $full = extract_param($param, 'full') // !PVE::QemuConfig->is_template($conf);
> +	    my $full = $param->{full} // !PVE::QemuConfig->is_template($conf);
>  
>  	    die "parameter 'storage' not allowed for linked clones\n"
>  		if defined($storage) && !$full;
> @@ -3156,7 +3154,13 @@ __PACKAGE__->register_method({
>  		}
>  	    }
>  
> -            # auto generate a new uuid
> +	    return ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone);
> +	};
> +
> +	my $clonefn = sub {
> +	    my ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone) = $load_and_check->();
> +
> +	    # auto generate a new uuid
>  	    my $smbios1 = PVE::QemuServer::parse_smbios1($newconf->{smbios1} || '');
>  	    $smbios1->{uuid} = PVE::QemuServer::generate_uuid();
>  	    $newconf->{smbios1} = PVE::QemuServer::print_smbios1($smbios1);
> @@ -3181,105 +3185,99 @@ __PACKAGE__->register_method({
>  	    # FIXME use PVE::QemuConfig->create_and_lock_config and adapt code
>  	    PVE::Tools::file_set_contents($conffile, "# qmclone temporary file\nlock: clone\n");
>  
> -	    my $realcmd = sub {
> -		my $upid = shift;
> -
> -		my $newvollist = [];
> -		my $jobs = {};
> -
> -		eval {
> -		    local $SIG{INT} =
> -			local $SIG{TERM} =
> -			local $SIG{QUIT} =
> -			local $SIG{HUP} = sub { die "interrupted by signal\n"; };
> -
> -		    PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
> -
> -		    my $bwlimit = extract_param($param, 'bwlimit');
> -
> -		    my $total_jobs = scalar(keys %{$drives});
> -		    my $i = 1;
> -
> -		    foreach my $opt (sort keys %$drives) {
> -			my $drive = $drives->{$opt};
> -			my $skipcomplete = ($total_jobs != $i); # finish after last drive
> -			my $completion = $skipcomplete ? 'skip' : 'complete';
> -
> -			my $src_sid = PVE::Storage::parse_volume_id($drive->{file});
> -			my $storage_list = [ $src_sid ];
> -			push @$storage_list, $storage if defined($storage);
> -			my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', $storage_list, $bwlimit);
> -
> -			my $newdrive = PVE::QemuServer::clone_disk(
> -			    $storecfg,
> -			    $vmid,
> -			    $running,
> -			    $opt,
> -			    $drive,
> -			    $snapname,
> -			    $newid,
> -			    $storage,
> -			    $format,
> -			    $fullclone->{$opt},
> -			    $newvollist,
> -			    $jobs,
> -			    $completion,
> -			    $oldconf->{agent},
> -			    $clonelimit,
> -			    $oldconf
> -			);
> -
> -			$newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
> -
> -			PVE::QemuConfig->write_config($newid, $newconf);
> -			$i++;
> -		    }
> -
> -		    delete $newconf->{lock};
> -
> -		    # do not write pending changes
> -		    if (my @changes = keys %{$newconf->{pending}}) {
> -			my $pending = join(',', @changes);
> -			warn "found pending changes for '$pending', discarding for clone\n";
> -			delete $newconf->{pending};
> -		    }
> -
> -		    PVE::QemuConfig->write_config($newid, $newconf);
> -
> -                    if ($target) {
> -			# always deactivate volumes - avoid lvm LVs to be active on several nodes
> -			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
> -			PVE::Storage::deactivate_volumes($storecfg, $newvollist);
> -
> -			my $newconffile = PVE::QemuConfig->config_file($newid, $target);
> -			die "Failed to move config to node '$target' - rename failed: $!\n"
> -			    if !rename($conffile, $newconffile);
> -		    }
> -
> -		    PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
> -		};
> -		if (my $err = $@) {
> -		    eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $jobs) };
> -		    sleep 1; # some storage like rbd need to wait before release volume - really?
> -
> -		    foreach my $volid (@$newvollist) {
> -			eval { PVE::Storage::vdisk_free($storecfg, $volid); };
> -			warn $@ if $@;
> -		    }
> -
> -		    PVE::Firewall::remove_vmfw_conf($newid);
> -
> -		    unlink $conffile; # avoid races -> last thing before die
> -
> -		    die "clone failed: $err";
> -		}
> -
> -		return;
> -	    };
> -
>  	    PVE::Firewall::clone_vmfw_conf($vmid, $newid);
>  
> -	    return $rpcenv->fork_worker('qmclone', $vmid, $authuser, $realcmd);
> +	    my $newvollist = [];
> +	    my $jobs = {};
> +
> +	    eval {
> +		local $SIG{INT} =
> +		    local $SIG{TERM} =
> +		    local $SIG{QUIT} =
> +		    local $SIG{HUP} = sub { die "interrupted by signal\n"; };
> +
> +		PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
> +
> +		my $bwlimit = extract_param($param, 'bwlimit');
> +
> +		my $total_jobs = scalar(keys %{$drives});
> +		my $i = 1;
> +
> +		foreach my $opt (sort keys %$drives) {
> +		    my $drive = $drives->{$opt};
> +		    my $skipcomplete = ($total_jobs != $i); # finish after last drive
> +		    my $completion = $skipcomplete ? 'skip' : 'complete';
> +
> +		    my $src_sid = PVE::Storage::parse_volume_id($drive->{file});
> +		    my $storage_list = [ $src_sid ];
> +		    push @$storage_list, $storage if defined($storage);
> +		    my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', $storage_list, $bwlimit);
> +
> +		    my $newdrive = PVE::QemuServer::clone_disk(
> +			$storecfg,
> +			$vmid,
> +			$running,
> +			$opt,
> +			$drive,
> +			$snapname,
> +			$newid,
> +			$storage,
> +			$format,
> +			$fullclone->{$opt},
> +			$newvollist,
> +			$jobs,
> +			$completion,
> +			$oldconf->{agent},
> +			$clonelimit,
> +			$oldconf
> +		    );
> +
> +		    $newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
> +
> +		    PVE::QemuConfig->write_config($newid, $newconf);
> +		    $i++;
> +		}
> +
> +		delete $newconf->{lock};
> +
> +		# do not write pending changes
> +		if (my @changes = keys %{$newconf->{pending}}) {
> +		    my $pending = join(',', @changes);
> +		    warn "found pending changes for '$pending', discarding for clone\n";
> +		    delete $newconf->{pending};
> +		}
> +
> +		PVE::QemuConfig->write_config($newid, $newconf);
> +
> +		if ($target) {
> +		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
> +		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
> +		    PVE::Storage::deactivate_volumes($storecfg, $newvollist);
> +
> +		    my $newconffile = PVE::QemuConfig->config_file($newid, $target);
> +		    die "Failed to move config to node '$target' - rename failed: $!\n"
> +			if !rename($conffile, $newconffile);
> +		}
> +
> +		PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
> +	    };
> +	    if (my $err = $@) {
> +		eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $jobs) };
> +		sleep 1; # some storage like rbd need to wait before release volume - really?
> +
> +		foreach my $volid (@$newvollist) {
> +		    eval { PVE::Storage::vdisk_free($storecfg, $volid); };
> +		    warn $@ if $@;
> +		}
> +
> +		PVE::Firewall::remove_vmfw_conf($newid);
> +
> +		unlink $conffile; # avoid races -> last thing before die
> +
> +		die "clone failed: $err";
> +	    }
> +
> +	    return;
>  	};
>  
>  	# Aquire exclusive lock lock for $newid
> @@ -3287,12 +3285,18 @@ __PACKAGE__->register_method({
>  	    return PVE::QemuConfig->lock_config_full($newid, 1, $clonefn);
>  	};
>  
> -	# exclusive lock if VM is running - else shared lock is enough;
> -	if ($running) {
> -	    return PVE::QemuConfig->lock_config_full($vmid, 1, $lock_target_vm);
> -	} else {
> -	    return PVE::QemuConfig->lock_config_shared($vmid, 1, $lock_target_vm);
> -	}
> +	my $lock_source_vm = sub {
> +	    # exclusive lock if VM is running - else shared lock is enough;
> +	    if ($running) {
> +		return PVE::QemuConfig->lock_config_full($vmid, 1, $lock_target_vm);
> +	    } else {
> +		return PVE::QemuConfig->lock_config_shared($vmid, 1, $lock_target_vm);
> +	    }
> +	};
> +
> +	$load_and_check->(); # early checks before forking/locking
> +
> +	return $rpcenv->fork_worker('qmclone', $vmid, $authuser, $lock_source_vm);
>      }});
>  
>  __PACKAGE__->register_method({
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

Re: [pve-devel] [PATCH qemu-server 2/4] api: clone: fork before locking

[Fabian Ebner]

Am 31.01.22 um 13:34 schrieb Fabian Grünbichler:
> On January 27, 2022 3:01 pm, Fabian Ebner wrote:
>> using the familiar early+repeated checks pattern from other API calls.
>> Only intended functional changes are with regard to locking/forking.
> 
> two questions:
> - the FW config cloning happens inside the worker now, while it was 
>   previously before forking the worker (LGTM, but might be called out 
>   explicitly if intentional ;))

Honestly, I didn't think too much about it, so thanks for pointing that
out! But thinking about it now, I also don't see an obvious issue with
it and IMHO it feels more natural to be part of the worker since it
takes the firewall config lock and the cleanup also happens inside the
worker.

> - there are some checks at the start of the endpoint (checking 
>   storage/target), which are not repeated after the fork+lock - while 
>   unlikely, our view of storage.cfg could change in-between (lock guest 
>   config -> cfs_update). should those be moved in the check sub (or into 
>   the check_storage_access_clone helper)?
> 

Yes, for better consistency that should be done. Either way is fine with
me. Should I send a v2 or are you going to do a follow-up?

> rest of the series LGTM
> 
>>
>> For a full clone of a running VM without guest agent, this also fixes
>> issuing vm_{resume,suspend} calls for drive mirror completion.
>> Previously, those just timed out, because of not getting the lock:
>>
>>> create full clone of drive scsi0 (rbdkvm:vm-104-disk-0)
>>> Formatting '/var/lib/vz/images/105/vm-105-disk-0.raw', fmt=raw
>>> size=4294967296 preallocation=off
>>> drive mirror is starting for drive-scsi0
>>> drive-scsi0: transferred 2.0 MiB of 4.0 GiB (0.05%) in 0s
>>> drive-scsi0: transferred 635.0 MiB of 4.0 GiB (15.50%) in 1s
>>> drive-scsi0: transferred 1.6 GiB of 4.0 GiB (40.50%) in 2s
>>> drive-scsi0: transferred 3.6 GiB of 4.0 GiB (90.23%) in 3s
>>> drive-scsi0: transferred 4.0 GiB of 4.0 GiB (100.00%) in 4s, ready
>>> all 'mirror' jobs are ready
>>> suspend vm
>>> trying to acquire lock...
>>> can't lock file '/var/lock/qemu-server/lock-104.conf' - got timeout
>>> drive-scsi0: Cancelling block job
>>> drive-scsi0: Done.
>>> resume vm
>>> trying to acquire lock...
>>> can't lock file '/var/lock/qemu-server/lock-104.conf' - got timeout
>>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Best viewed with -w.
>>
>>  PVE/API2/Qemu.pm | 220 ++++++++++++++++++++++++-----------------------
>>  1 file changed, 112 insertions(+), 108 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 6992f6f..38e08c8 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -3079,9 +3079,7 @@ __PACKAGE__->register_method({
>>  
>>  	my $running = PVE::QemuServer::check_running($vmid) || 0;
>>  
>> -	my $clonefn = sub {
>> -	    # do all tests after lock but before forking worker - if possible
>> -
>> +	my $load_and_check = sub {
>>  	    my $conf = PVE::QemuConfig->load_config($vmid);
>>  	    PVE::QemuConfig->check_lock($conf);
>>  
>> @@ -3091,7 +3089,7 @@ __PACKAGE__->register_method({
>>  	    die "snapshot '$snapname' does not exist\n"
>>  		if $snapname && !defined( $conf->{snapshots}->{$snapname});
>>  
>> -	    my $full = extract_param($param, 'full') // !PVE::QemuConfig->is_template($conf);
>> +	    my $full = $param->{full} // !PVE::QemuConfig->is_template($conf);
>>  
>>  	    die "parameter 'storage' not allowed for linked clones\n"
>>  		if defined($storage) && !$full;
>> @@ -3156,7 +3154,13 @@ __PACKAGE__->register_method({
>>  		}
>>  	    }
>>  
>> -            # auto generate a new uuid
>> +	    return ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone);
>> +	};
>> +
>> +	my $clonefn = sub {
>> +	    my ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone) = $load_and_check->();
>> +
>> +	    # auto generate a new uuid
>>  	    my $smbios1 = PVE::QemuServer::parse_smbios1($newconf->{smbios1} || '');
>>  	    $smbios1->{uuid} = PVE::QemuServer::generate_uuid();
>>  	    $newconf->{smbios1} = PVE::QemuServer::print_smbios1($smbios1);
>> @@ -3181,105 +3185,99 @@ __PACKAGE__->register_method({
>>  	    # FIXME use PVE::QemuConfig->create_and_lock_config and adapt code
>>  	    PVE::Tools::file_set_contents($conffile, "# qmclone temporary file\nlock: clone\n");
>>  
>> -	    my $realcmd = sub {
>> -		my $upid = shift;
>> -
>> -		my $newvollist = [];
>> -		my $jobs = {};
>> -
>> -		eval {
>> -		    local $SIG{INT} =
>> -			local $SIG{TERM} =
>> -			local $SIG{QUIT} =
>> -			local $SIG{HUP} = sub { die "interrupted by signal\n"; };
>> -
>> -		    PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
>> -
>> -		    my $bwlimit = extract_param($param, 'bwlimit');
>> -
>> -		    my $total_jobs = scalar(keys %{$drives});
>> -		    my $i = 1;
>> -
>> -		    foreach my $opt (sort keys %$drives) {
>> -			my $drive = $drives->{$opt};
>> -			my $skipcomplete = ($total_jobs != $i); # finish after last drive
>> -			my $completion = $skipcomplete ? 'skip' : 'complete';
>> -
>> -			my $src_sid = PVE::Storage::parse_volume_id($drive->{file});
>> -			my $storage_list = [ $src_sid ];
>> -			push @$storage_list, $storage if defined($storage);
>> -			my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', $storage_list, $bwlimit);
>> -
>> -			my $newdrive = PVE::QemuServer::clone_disk(
>> -			    $storecfg,
>> -			    $vmid,
>> -			    $running,
>> -			    $opt,
>> -			    $drive,
>> -			    $snapname,
>> -			    $newid,
>> -			    $storage,
>> -			    $format,
>> -			    $fullclone->{$opt},
>> -			    $newvollist,
>> -			    $jobs,
>> -			    $completion,
>> -			    $oldconf->{agent},
>> -			    $clonelimit,
>> -			    $oldconf
>> -			);
>> -
>> -			$newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
>> -
>> -			PVE::QemuConfig->write_config($newid, $newconf);
>> -			$i++;
>> -		    }
>> -
>> -		    delete $newconf->{lock};
>> -
>> -		    # do not write pending changes
>> -		    if (my @changes = keys %{$newconf->{pending}}) {
>> -			my $pending = join(',', @changes);
>> -			warn "found pending changes for '$pending', discarding for clone\n";
>> -			delete $newconf->{pending};
>> -		    }
>> -
>> -		    PVE::QemuConfig->write_config($newid, $newconf);
>> -
>> -                    if ($target) {
>> -			# always deactivate volumes - avoid lvm LVs to be active on several nodes
>> -			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
>> -			PVE::Storage::deactivate_volumes($storecfg, $newvollist);
>> -
>> -			my $newconffile = PVE::QemuConfig->config_file($newid, $target);
>> -			die "Failed to move config to node '$target' - rename failed: $!\n"
>> -			    if !rename($conffile, $newconffile);
>> -		    }
>> -
>> -		    PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
>> -		};
>> -		if (my $err = $@) {
>> -		    eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $jobs) };
>> -		    sleep 1; # some storage like rbd need to wait before release volume - really?
>> -
>> -		    foreach my $volid (@$newvollist) {
>> -			eval { PVE::Storage::vdisk_free($storecfg, $volid); };
>> -			warn $@ if $@;
>> -		    }
>> -
>> -		    PVE::Firewall::remove_vmfw_conf($newid);
>> -
>> -		    unlink $conffile; # avoid races -> last thing before die
>> -
>> -		    die "clone failed: $err";
>> -		}
>> -
>> -		return;
>> -	    };
>> -
>>  	    PVE::Firewall::clone_vmfw_conf($vmid, $newid);
>>  
>> -	    return $rpcenv->fork_worker('qmclone', $vmid, $authuser, $realcmd);
>> +	    my $newvollist = [];
>> +	    my $jobs = {};
>> +
>> +	    eval {
>> +		local $SIG{INT} =
>> +		    local $SIG{TERM} =
>> +		    local $SIG{QUIT} =
>> +		    local $SIG{HUP} = sub { die "interrupted by signal\n"; };
>> +
>> +		PVE::Storage::activate_volumes($storecfg, $vollist, $snapname);
>> +
>> +		my $bwlimit = extract_param($param, 'bwlimit');
>> +
>> +		my $total_jobs = scalar(keys %{$drives});
>> +		my $i = 1;
>> +
>> +		foreach my $opt (sort keys %$drives) {
>> +		    my $drive = $drives->{$opt};
>> +		    my $skipcomplete = ($total_jobs != $i); # finish after last drive
>> +		    my $completion = $skipcomplete ? 'skip' : 'complete';
>> +
>> +		    my $src_sid = PVE::Storage::parse_volume_id($drive->{file});
>> +		    my $storage_list = [ $src_sid ];
>> +		    push @$storage_list, $storage if defined($storage);
>> +		    my $clonelimit = PVE::Storage::get_bandwidth_limit('clone', $storage_list, $bwlimit);
>> +
>> +		    my $newdrive = PVE::QemuServer::clone_disk(
>> +			$storecfg,
>> +			$vmid,
>> +			$running,
>> +			$opt,
>> +			$drive,
>> +			$snapname,
>> +			$newid,
>> +			$storage,
>> +			$format,
>> +			$fullclone->{$opt},
>> +			$newvollist,
>> +			$jobs,
>> +			$completion,
>> +			$oldconf->{agent},
>> +			$clonelimit,
>> +			$oldconf
>> +		    );
>> +
>> +		    $newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
>> +
>> +		    PVE::QemuConfig->write_config($newid, $newconf);
>> +		    $i++;
>> +		}
>> +
>> +		delete $newconf->{lock};
>> +
>> +		# do not write pending changes
>> +		if (my @changes = keys %{$newconf->{pending}}) {
>> +		    my $pending = join(',', @changes);
>> +		    warn "found pending changes for '$pending', discarding for clone\n";
>> +		    delete $newconf->{pending};
>> +		}
>> +
>> +		PVE::QemuConfig->write_config($newid, $newconf);
>> +
>> +		if ($target) {
>> +		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
>> +		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
>> +		    PVE::Storage::deactivate_volumes($storecfg, $newvollist);
>> +
>> +		    my $newconffile = PVE::QemuConfig->config_file($newid, $target);
>> +		    die "Failed to move config to node '$target' - rename failed: $!\n"
>> +			if !rename($conffile, $newconffile);
>> +		}
>> +
>> +		PVE::AccessControl::add_vm_to_pool($newid, $pool) if $pool;
>> +	    };
>> +	    if (my $err = $@) {
>> +		eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $jobs) };
>> +		sleep 1; # some storage like rbd need to wait before release volume - really?
>> +
>> +		foreach my $volid (@$newvollist) {
>> +		    eval { PVE::Storage::vdisk_free($storecfg, $volid); };
>> +		    warn $@ if $@;
>> +		}
>> +
>> +		PVE::Firewall::remove_vmfw_conf($newid);
>> +
>> +		unlink $conffile; # avoid races -> last thing before die
>> +
>> +		die "clone failed: $err";
>> +	    }
>> +
>> +	    return;
>>  	};
>>  
>>  	# Aquire exclusive lock lock for $newid
>> @@ -3287,12 +3285,18 @@ __PACKAGE__->register_method({
>>  	    return PVE::QemuConfig->lock_config_full($newid, 1, $clonefn);
>>  	};
>>  
>> -	# exclusive lock if VM is running - else shared lock is enough;
>> -	if ($running) {
>> -	    return PVE::QemuConfig->lock_config_full($vmid, 1, $lock_target_vm);
>> -	} else {
>> -	    return PVE::QemuConfig->lock_config_shared($vmid, 1, $lock_target_vm);
>> -	}
>> +	my $lock_source_vm = sub {
>> +	    # exclusive lock if VM is running - else shared lock is enough;
>> +	    if ($running) {
>> +		return PVE::QemuConfig->lock_config_full($vmid, 1, $lock_target_vm);
>> +	    } else {
>> +		return PVE::QemuConfig->lock_config_shared($vmid, 1, $lock_target_vm);
>> +	    }
>> +	};
>> +
>> +	$load_and_check->(); # early checks before forking/locking
>> +
>> +	return $rpcenv->fork_worker('qmclone', $vmid, $authuser, $lock_source_vm);
>>      }});
>>  
>>  __PACKAGE__->register_method({
>> -- 
>> 2.30.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 

Re: [pve-devel] [PATCH qemu-server 2/4] api: clone: fork before locking

[Fabian Grünbichler]

On February 3, 2022 10:31 am, Fabian Ebner wrote:
> Am 31.01.22 um 13:34 schrieb Fabian Grünbichler:
>> On January 27, 2022 3:01 pm, Fabian Ebner wrote:
>>> using the familiar early+repeated checks pattern from other API calls.
>>> Only intended functional changes are with regard to locking/forking.
>> 
>> two questions:
>> - the FW config cloning happens inside the worker now, while it was 
>>   previously before forking the worker (LGTM, but might be called out 
>>   explicitly if intentional ;))
> 
> Honestly, I didn't think too much about it, so thanks for pointing that
> out! But thinking about it now, I also don't see an obvious issue with
> it and IMHO it feels more natural to be part of the worker since it
> takes the firewall config lock and the cleanup also happens inside the
> worker.
> 
>> - there are some checks at the start of the endpoint (checking 
>>   storage/target), which are not repeated after the fork+lock - while 
>>   unlikely, our view of storage.cfg could change in-between (lock guest 
>>   config -> cfs_update). should those be moved in the check sub (or into 
>>   the check_storage_access_clone helper)?
>> 
> 
> Yes, for better consistency that should be done. Either way is fine with
> me. Should I send a v2 or are you going to do a follow-up?

I'll do a follow-up!

> 
>> rest of the series LGTM

[pve-devel] applied-series: [PATCH-SERIES qemu-server] clone_disk-related improvments

[Fabian Grünbichler]

with following addition on top (shown here with `-w`), to repeat even 
more of the checks after forking/locking:

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 59e083eb..a359d096 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3046,7 +3046,6 @@ __PACKAGE__->register_method({
 	my $vmid = extract_param($param, 'vmid');
 	my $newid = extract_param($param, 'newid');
 	my $pool = extract_param($param, 'pool');
-	$rpcenv->check_pool_exist($pool) if defined($pool);
 
         my $snapname = extract_param($param, 'snapname');
 	my $storage = extract_param($param, 'storage');
@@ -3059,6 +3058,10 @@ __PACKAGE__->register_method({
 	    undef $target;
 	}
 
+	my $running = PVE::QemuServer::check_running($vmid) || 0;
+
+	my $load_and_check = sub {
+	    $rpcenv->check_pool_exist($pool) if defined($pool);
 	    PVE::Cluster::check_node_exists($target) if $target;
 
 	    my $storecfg = PVE::Storage::config();
@@ -3071,15 +3074,13 @@ __PACKAGE__->register_method({
 		    PVE::Storage::storage_check_enabled($storecfg, $storage, $target);
 		    # clone only works if target storage is shared
 		    my $scfg = PVE::Storage::storage_config($storecfg, $storage);
-		die "can't clone to non-shared storage '$storage'\n" if !$scfg->{shared};
+		    die "can't clone to non-shared storage '$storage'\n"
+			if !$scfg->{shared};
 		}
 	    }
 
 	    PVE::Cluster::check_cfs_quorum();
 
-	my $running = PVE::QemuServer::check_running($vmid) || 0;
-
-	my $load_and_check = sub {
 	    my $conf = PVE::QemuConfig->load_config($vmid);
 	    PVE::QemuConfig->check_lock($conf);
 
@@ -3159,6 +3160,7 @@ __PACKAGE__->register_method({
 
 	my $clonefn = sub {
 	    my ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone) = $load_and_check->();
+	    my $storecfg = PVE::Storage::config();
 
 	    # auto generate a new uuid
 	    my $smbios1 = PVE::QemuServer::parse_smbios1($newconf->{smbios1} || '');

On January 27, 2022 3:01 pm, Fabian Ebner wrote:
> The second patch is fixing a concrete bug with vm_{resume,suspend} not being
> issued for a full clone of a running VM without guest agent. Other patches are
> minor improvements, mostly trying to prevent future bugs.
> 
> Fabian Ebner (4):
>   drive mirror monitor: warn when suspend/resume/freeze/thaw calls fail
>   api: clone: fork before locking
>   api: move disk: fork before locking
>   clone disk: don't modify drive parameter
> 
>  PVE/API2/Qemu.pm  | 382 ++++++++++++++++++++++++----------------------
>  PVE/QemuServer.pm |  10 +-
>  2 files changed, 205 insertions(+), 187 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>