From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 482AD64EFF for ; Mon, 31 Jan 2022 13:34:51 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3952C244C3 for ; Mon, 31 Jan 2022 13:34:21 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id C357B244BA for ; Mon, 31 Jan 2022 13:34:19 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 971AD43CAB for ; Mon, 31 Jan 2022 13:34:19 +0100 (CET) Date: Mon, 31 Jan 2022 13:34:11 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220127140155.66141-1-f.ebner@proxmox.com> <20220127140155.66141-3-f.ebner@proxmox.com> In-Reply-To: <20220127140155.66141-3-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1643631853.hgjpywv6g4.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.219 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH qemu-server 2/4] api: clone: fork before locking X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 31 Jan 2022 12:34:51 -0000 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=20 previously before forking the worker (LGTM, but might be called out=20 explicitly if intentional ;)) - there are some checks at the start of the endpoint (checking=20 storage/target), which are not repeated after the fork+lock - while=20 unlikely, our view of storage.cfg could change in-between (lock guest=20 config -> cfs_update). should those be moved in the check sub (or into=20 the check_storage_access_clone helper)? rest of the series LGTM >=20 > 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: >=20 >> create full clone of drive scsi0 (rbdkvm:vm-104-disk-0) >> Formatting '/var/lib/vz/images/105/vm-105-disk-0.raw', fmt=3Draw >> size=3D4294967296 preallocation=3Doff >> 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 >=20 > Signed-off-by: Fabian Ebner > --- >=20 > Best viewed with -w. >=20 > PVE/API2/Qemu.pm | 220 ++++++++++++++++++++++++----------------------- > 1 file changed, 112 insertions(+), 108 deletions(-) >=20 > 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({ > =20 > my $running =3D PVE::QemuServer::check_running($vmid) || 0; > =20 > - my $clonefn =3D sub { > - # do all tests after lock but before forking worker - if possible > - > + my $load_and_check =3D sub { > my $conf =3D PVE::QemuConfig->load_config($vmid); > PVE::QemuConfig->check_lock($conf); > =20 > @@ -3091,7 +3089,7 @@ __PACKAGE__->register_method({ > die "snapshot '$snapname' does not exist\n" > if $snapname && !defined( $conf->{snapshots}->{$snapname}); > =20 > - my $full =3D extract_param($param, 'full') // !PVE::QemuConfig->is_= template($conf); > + my $full =3D $param->{full} // !PVE::QemuConfig->is_template($conf)= ; > =20 > die "parameter 'storage' not allowed for linked clones\n" > if defined($storage) && !$full; > @@ -3156,7 +3154,13 @@ __PACKAGE__->register_method({ > } > } > =20 > - # auto generate a new uuid > + return ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclon= e); > + }; > + > + my $clonefn =3D sub { > + my ($conffile, $newconf, $oldconf, $vollist, $drives, $fullclone) = =3D $load_and_check->(); > + > + # auto generate a new uuid > my $smbios1 =3D PVE::QemuServer::parse_smbios1($newconf->{smbios1} = || ''); > $smbios1->{uuid} =3D PVE::QemuServer::generate_uuid(); > $newconf->{smbios1} =3D 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"); > =20 > - my $realcmd =3D sub { > - my $upid =3D shift; > - > - my $newvollist =3D []; > - my $jobs =3D {}; > - > - eval { > - local $SIG{INT} =3D > - local $SIG{TERM} =3D > - local $SIG{QUIT} =3D > - local $SIG{HUP} =3D sub { die "interrupted by signal\n"; }; > - > - PVE::Storage::activate_volumes($storecfg, $vollist, $snapname); > - > - my $bwlimit =3D extract_param($param, 'bwlimit'); > - > - my $total_jobs =3D scalar(keys %{$drives}); > - my $i =3D 1; > - > - foreach my $opt (sort keys %$drives) { > - my $drive =3D $drives->{$opt}; > - my $skipcomplete =3D ($total_jobs !=3D $i); # finish after last drive > - my $completion =3D $skipcomplete ? 'skip' : 'complete'; > - > - my $src_sid =3D PVE::Storage::parse_volume_id($drive->{file}); > - my $storage_list =3D [ $src_sid ]; > - push @$storage_list, $storage if defined($storage); > - my $clonelimit =3D PVE::Storage::get_bandwidth_limit('clone', $storag= e_list, $bwlimit); > - > - my $newdrive =3D PVE::QemuServer::clone_disk( > - $storecfg, > - $vmid, > - $running, > - $opt, > - $drive, > - $snapname, > - $newid, > - $storage, > - $format, > - $fullclone->{$opt}, > - $newvollist, > - $jobs, > - $completion, > - $oldconf->{agent}, > - $clonelimit, > - $oldconf > - ); > - > - $newconf->{$opt} =3D PVE::QemuServer::print_drive($newdrive); > - > - PVE::QemuConfig->write_config($newid, $newconf); > - $i++; > - } > - > - delete $newconf->{lock}; > - > - # do not write pending changes > - if (my @changes =3D keys %{$newconf->{pending}}) { > - my $pending =3D 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 n= odes > - PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if != $running; > - PVE::Storage::deactivate_volumes($storecfg, $newvollist); > - > - my $newconffile =3D 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 =3D $@) { > - eval { PVE::QemuServer::qemu_blockjobs_cancel($vmid, $jobs) }; > - sleep 1; # some storage like rbd need to wait before release volum= e - 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); > =20 > - return $rpcenv->fork_worker('qmclone', $vmid, $authuser, $realcmd); > + my $newvollist =3D []; > + my $jobs =3D {}; > + > + eval { > + local $SIG{INT} =3D > + local $SIG{TERM} =3D > + local $SIG{QUIT} =3D > + local $SIG{HUP} =3D sub { die "interrupted by signal\n"; }; > + > + PVE::Storage::activate_volumes($storecfg, $vollist, $snapname); > + > + my $bwlimit =3D extract_param($param, 'bwlimit'); > + > + my $total_jobs =3D scalar(keys %{$drives}); > + my $i =3D 1; > + > + foreach my $opt (sort keys %$drives) { > + my $drive =3D $drives->{$opt}; > + my $skipcomplete =3D ($total_jobs !=3D $i); # finish after last dr= ive > + my $completion =3D $skipcomplete ? 'skip' : 'complete'; > + > + my $src_sid =3D PVE::Storage::parse_volume_id($drive->{file}); > + my $storage_list =3D [ $src_sid ]; > + push @$storage_list, $storage if defined($storage); > + my $clonelimit =3D PVE::Storage::get_bandwidth_limit('clone', $sto= rage_list, $bwlimit); > + > + my $newdrive =3D PVE::QemuServer::clone_disk( > + $storecfg, > + $vmid, > + $running, > + $opt, > + $drive, > + $snapname, > + $newid, > + $storage, > + $format, > + $fullclone->{$opt}, > + $newvollist, > + $jobs, > + $completion, > + $oldconf->{agent}, > + $clonelimit, > + $oldconf > + ); > + > + $newconf->{$opt} =3D PVE::QemuServer::print_drive($newdrive); > + > + PVE::QemuConfig->write_config($newid, $newconf); > + $i++; > + } > + > + delete $newconf->{lock}; > + > + # do not write pending changes > + if (my @changes =3D keys %{$newconf->{pending}}) { > + my $pending =3D 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 severa= l nodes > + PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) i= f !$running; > + PVE::Storage::deactivate_volumes($storecfg, $newvollist); > + > + my $newconffile =3D 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 =3D $@) { > + 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; > }; > =20 > # Aquire exclusive lock lock for $newid > @@ -3287,12 +3285,18 @@ __PACKAGE__->register_method({ > return PVE::QemuConfig->lock_config_full($newid, 1, $clonefn); > }; > =20 > - # 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_v= m); > - } > + my $lock_source_vm =3D 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_v= m); > }}); > =20 > __PACKAGE__->register_method({ > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20