[pve-devel] [PATCH/RFC storage 1/1] lvmplugin: improve error handling for saferemove

[Fiona Ebner]

Ensure that volumes are not automatically removed after a failure to
zero-out to give an admin the possibility to do so manually. The
downside is that "del-.*" volumes might remain left-over, the upside
is that data does not leak upon error.

Also ensure that activation failure of a single volume does not error
out early, but continue with removal of other volumes.

Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
---
 src/PVE/Storage/LVMPlugin.pm | 59 ++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 275e84a..5e9fd83 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -10,6 +10,7 @@ use IO::File;
 use PVE::Tools qw(run_command file_read_firstline trim);
 use PVE::Storage::Plugin;
 use PVE::JSONSchema qw(get_standard_option);
+use PVE::RESTEnvironment qw(log_warn);
 
 use PVE::Storage::Common;
 
@@ -297,8 +298,7 @@ my sub free_lvm_volumes {
         if ($bdev && $bdev =~ m|^/dev/(dm-\d+)|) {
             $sysdir = "/sys/block/$1";
         } else {
-            warn "skip zero-out for volume '$lvmpath' - no device mapper link\n";
-            return;
+            die "no device mapper link found\n";
         }
 
         my $write_zeroes_max_bytes =
@@ -345,43 +345,50 @@ my sub free_lvm_volumes {
                 $stepsize = $write_zeroes_max_bytes;
             }
 
-            my $cmd = ['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"];
-            eval { run_command($cmd); };
-            warn $@ if $@;
+            run_command(['blkdiscard', $lvmpath, '-v', '--zeroout', '--step', "${stepsize}"]);
         }
     };
 
     # we need to zero out LVM data for security reasons
     # and to allow thin provisioning
     my $zero_out_worker = sub {
+        my $error_count = 0;
         for my $name (@$volnames) {
             my $lvmpath = "/dev/$vg/del-$name";
             print "zero-out data on image $name ($lvmpath)\n";
 
-            my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath];
-            run_command(
-                $cmd_activate,
-                errmsg => "can't activate LV '$lvmpath' to zero-out its data",
-            );
-            $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath];
-            run_command(
-                $cmd_activate,
-                errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
-            );
+            eval {
+                my $cmd_activate = ['/sbin/lvchange', '-aly', $lvmpath];
+                run_command(
+                    $cmd_activate,
+                    errmsg => "can't activate LV '$lvmpath' to zero-out its data",
+                );
+                $cmd_activate = ['/sbin/lvchange', '--refresh', $lvmpath];
+                run_command(
+                    $cmd_activate,
+                    errmsg => "can't refresh LV '$lvmpath' to zero-out its data",
+                );
 
-            $secure_delete_cmd->($lvmpath);
+                $secure_delete_cmd->($lvmpath);
 
-            $class->cluster_lock_storage(
-                $storeid,
-                $scfg->{shared},
-                undef,
-                sub {
-                    my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$name"];
-                    run_command($cmd, errmsg => "lvremove '$vg/del-$name' error");
-                },
-            );
-            print "successfully removed volume $name ($vg/del-$name)\n";
+                $class->cluster_lock_storage(
+                    $storeid,
+                    $scfg->{shared},
+                    undef,
+                    sub {
+                        my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$name"];
+                        run_command($cmd, errmsg => "lvremove '$vg/del-$name' error");
+                    },
+                );
+                print "successfully removed volume $name ($vg/del-$name)\n";
+            };
+            if (my $err = $@) {
+                chomp($err);
+                log_warn("cannot zero-out '$lvmpath' - $err - please zero-out and remove manually");
+                $error_count++;
+            }
         }
+        die "failed to zero-out $error_count volume(s) - check log\n" if $error_count > 0;
     };
 
     if ($scfg->{saferemove}) {
-- 
2.47.3