[pve-devel] [PATCH qemu-server v2 1/1] fix 1734: clone VM: if deactivation fails demote error to warning

[pve-devel] [PATCH qemu-server v2 1/1] fix 1734: clone VM: if deactivation fails demote error to warning

[Hannes Duerr]

When a template with disks on LVM is cloned to another node, the volumes
are first activated, then cloned and deactivated again after cloning.

However, if clones of this template are now created in parallel to other
nodes, it can happen that one of the tasks can no longer deactivate the
logical volume because it is still in use.  The reason for this is that
we use a shared lock.
Since the failed deactivation does not necessarily have consequences, we
downgrade the error to a warning, which means that the clone tasks will
continue to be completed successfully.

Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
---
changes since v1:
- fix nits and spelling

 PVE/API2/Qemu.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 69c5896..1ff5abe 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -48,6 +48,7 @@ use PVE::DataCenterConfig;
 use PVE::SSHInfo;
 use PVE::Replication;
 use PVE::StorageTunnel;
+use PVE::RESTEnvironment qw(log_warn);
 
 BEGIN {
     if (!$ENV{PVE_GENERATING_DOCS}) {
@@ -3820,7 +3821,11 @@ __PACKAGE__->register_method({
 
 		if ($target) {
 		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
-		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
+		    eval {
+			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
+		    };
+		    log_warn($@) if ($@);
+
 		    PVE::Storage::deactivate_volumes($storecfg, $newvollist);
 
 		    my $newconffile = PVE::QemuConfig->config_file($newid, $target);
-- 
2.39.2

Re: [pve-devel] [PATCH qemu-server v2 1/1] fix 1734: clone VM: if deactivation fails demote error to warning

[Friedrich Weber]

Tested-by: Friedrich Weber <f.weber@proxmox.com>

Can confirm the patch fixes the issue of parallel qmclones failing
occasionally due to a LVM deactivation error, and the extra \n in the
task log from v1 is gone.

One tiny comment inline:

On 06/03/2024 15:08, Hannes Duerr wrote:
> When a template with disks on LVM is cloned to another node, the volumes
> are first activated, then cloned and deactivated again after cloning.
> 
> However, if clones of this template are now created in parallel to other
> nodes, it can happen that one of the tasks can no longer deactivate the
> logical volume because it is still in use.  The reason for this is that
> we use a shared lock.
> Since the failed deactivation does not necessarily have consequences, we
> downgrade the error to a warning, which means that the clone tasks will
> continue to be completed successfully.
> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
> changes since v1:
> - fix nits and spelling
> 
>  PVE/API2/Qemu.pm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 69c5896..1ff5abe 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -48,6 +48,7 @@ use PVE::DataCenterConfig;
>  use PVE::SSHInfo;
>  use PVE::Replication;
>  use PVE::StorageTunnel;
> +use PVE::RESTEnvironment qw(log_warn);
>  
>  BEGIN {
>      if (!$ENV{PVE_GENERATING_DOCS}) {
> @@ -3820,7 +3821,11 @@ __PACKAGE__->register_method({
>  
>  		if ($target) {
>  		    # always deactivate volumes - avoid lvm LVs to be active on several nodes
> -		    PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
> +		    eval {
> +			PVE::Storage::deactivate_volumes($storecfg, $vollist, $snapname) if !$running;
> +		    };
> +		    log_warn($@) if ($@);

I suppose the parentheses in the post-if condition `if ($@)` are not
strictly necessary, but since ` ... if ($@);` does appear a couple of
times in our existing code, so probably not a problem.

> +
>  		    PVE::Storage::deactivate_volumes($storecfg, $newvollist);
>  
>  		    my $newconffile = PVE::QemuConfig->config_file($newid, $target);

[pve-devel] applied: [PATCH qemu-server v2 1/1] fix 1734: clone VM: if deactivation fails demote error to warning

[Thomas Lamprecht]

Am 06/03/2024 um 15:08 schrieb Hannes Duerr:
> When a template with disks on LVM is cloned to another node, the volumes
> are first activated, then cloned and deactivated again after cloning.
> 
> However, if clones of this template are now created in parallel to other
> nodes, it can happen that one of the tasks can no longer deactivate the
> logical volume because it is still in use.  The reason for this is that
> we use a shared lock.
> Since the failed deactivation does not necessarily have consequences, we
> downgrade the error to a warning, which means that the clone tasks will
> continue to be completed successfully.
> 
> Signed-off-by: Hannes Duerr <h.duerr@proxmox.com>
> ---
> changes since v1:
> - fix nits and spelling
> 
>  PVE/API2/Qemu.pm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
>

applied with Friedrich's T-b, thanks!

I also made a small follow-up commit trying to address some smaller
code-style nits.