Re: [pve-devel] [PATCH pve-storage 4/5] storage: vdisk_free: remove external snapshots

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am 22.04.2025 13:51 CEST geschrieben:
> add a $include_snapshots param to free_image to
> remove the whole chain of snapshots when deleting the main image.

rbd, zfs, btrfs, lvmthin and qcow2 internal snapshots already all behave like this by default..

shouldn't we just implement this for external snapshots without the need to opt into
it?

> 
> Signed-off-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> ---
>  src/PVE/Storage.pm           |  2 +-
>  src/PVE/Storage/LVMPlugin.pm | 72 ++++++++++++++++++++++++------------
>  src/PVE/Storage/Plugin.pm    | 27 +++++++++++---
>  3 files changed, 71 insertions(+), 30 deletions(-)
> 
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index db9d190..55a9a43 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -1059,7 +1059,7 @@ sub vdisk_free {
>  
>  	my (undef, undef, undef, undef, undef, $isBase, $format) =
>  	    $plugin->parse_volname($volname);
> -	$cleanup_worker = $plugin->free_image($storeid, $scfg, $volname, $isBase, $format);
> +	$cleanup_worker = $plugin->free_image($storeid, $scfg, $volname, $isBase, $format, 1);
>      });
>  
>      return if !$cleanup_worker;
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 8ee337a..b20fe98 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -463,10 +463,34 @@ sub alloc_snap_image {
>  }
>  
>  sub free_image {
> -    my ($class, $storeid, $scfg, $volname, $isBase) = @_;
> +    my ($class, $storeid, $scfg, $volname, $isBase, $format, $include_snapshots) = @_;
>  
>      my $vg = $scfg->{vgname};
>  
> +    my $name = ($class->parse_volname($volname))[1];
> +
> +    #activate volumes && snapshot volumes
> +    my $path = $class->path($scfg, $volname, $storeid);
> +    $path = "\@pve-$name" if $format && $format eq 'qcow2';
> +    my $cmd = ['/sbin/lvchange', '-aly', $path];
> +    run_command($cmd, errmsg => "can't activate LV '$path' to zero-out its data");
> +    $cmd = ['/sbin/lvchange', '--refresh', $path];
> +    run_command($cmd, errmsg => "can't refresh LV '$path' to zero-out its data");
> +
> +    my $volnames = [];
> +    if ($include_snapshots) {
> +	my $snapshots =  $class->volume_snapshot_info($scfg, $storeid, $volname);
> +	for my $snapid (sort { $snapshots->{$b}->{order} <=> $snapshots->{$a}->{order} } keys %$snapshots) {
> +	    my $snap = $snapshots->{$snapid};
> +	    next if $snapid eq 'current';
> +	    next if !$snap->{volid};
> +	    next if !$snap->{ext};
> +	    my ($snap_storeid, $snap_volname) = PVE::Storage::parse_volume_id($snap->{volid});
> +	    push @$volnames, $snap_volname;
> +	}
> +    }
> +    push @$volnames, $volname;
> +
>      # we need to zero out LVM data for security reasons
>      # and to allow thin provisioning
>  
> @@ -478,40 +502,40 @@ sub free_image {
>  	if ($scfg->{saferemove_throughput}) {
>  		$throughput = $scfg->{saferemove_throughput};
>  	}
> -
> -	my $cmd = [
> +	for my $name (@$volnames) {
> +	    my $cmd = [
>  		'/usr/bin/cstream',
>  		'-i', '/dev/zero',
> -		'-o', "/dev/$vg/del-$volname",
> +		'-o', "/dev/$vg/del-$name",
>  		'-T', '10',
>  		'-v', '1',
>  		'-b', '1048576',
>  		'-t', "$throughput"
> -	];
> -	eval { run_command($cmd, errmsg => "zero out finished (note: 'No space left on device' is ok here)"); };
> -	warn $@ if $@;
> -
> -	$class->cluster_lock_storage($storeid, $scfg->{shared}, undef, sub {
> -	    my $cmd = ['/sbin/lvremove', '-f', "$vg/del-$volname"];
> -	    run_command($cmd, errmsg => "lvremove '$vg/del-$volname' error");
> -	});
> -	print "successfully removed volume $volname ($vg/del-$volname)\n";
> +	    ];
> +	    eval { run_command($cmd, errmsg => "zero out finished (note: 'No space left on device' is ok here)"); };
> +	    warn $@ if $@;
> +
> +	    $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";
> +	}
>      };
>  
> -    my $cmd = ['/sbin/lvchange', '-aly', "$vg/$volname"];
> -    run_command($cmd, errmsg => "can't activate LV '$vg/$volname' to zero-out its data");
> -    $cmd = ['/sbin/lvchange', '--refresh', "$vg/$volname"];
> -    run_command($cmd, errmsg => "can't refresh LV '$vg/$volname' to zero-out its data");
> -
>      if ($scfg->{saferemove}) {
> -	# avoid long running task, so we only rename here
> -	$cmd = ['/sbin/lvrename', $vg, $volname, "del-$volname"];
> -	run_command($cmd, errmsg => "lvrename '$vg/$volname' error");
> +	for my $name (@$volnames) {
> +	    # avoid long running task, so we only rename here
> +	    my $cmd = ['/sbin/lvrename', $vg, $name, "del-$name"];
> +	    run_command($cmd, errmsg => "lvrename '$vg/$name' error");
> +	}
>  	return $zero_out_worker;
>      } else {
> -	my $tmpvg = $scfg->{vgname};
> -	$cmd = ['/sbin/lvremove', '-f', "$tmpvg/$volname"];
> -	run_command($cmd, errmsg => "lvremove '$tmpvg/$volname' error");
> +	for my $name (@$volnames) {
> +	    my $tmpvg = $scfg->{vgname};
> +	    my $cmd = ['/sbin/lvremove', '-f', "$tmpvg/$name"];
> +	    run_command($cmd, errmsg => "lvremove '$tmpvg/$name' error");
> +	}
>      }
>  
>      return undef;
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 3f83fae..0319ab2 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -959,7 +959,7 @@ sub alloc_snap_image {
>  }
>  
>  sub free_image {
> -    my ($class, $storeid, $scfg, $volname, $isBase, $format) = @_;
> +    my ($class, $storeid, $scfg, $volname, $isBase, $format, $include_snapshots) = @_;
>  
>      die "cannot remove protected volume '$volname' on '$storeid'\n"
>  	if $class->get_volume_attribute($scfg, $storeid, $volname, 'protected');
> @@ -975,12 +975,29 @@ sub free_image {
>      if (defined($format) && ($format eq 'subvol')) {
>  	File::Path::remove_tree($path);
>      } else {
> -	if (!(-f $path || -l $path)) {
> -	    warn "disk image '$path' does not exist\n";
> -	    return undef;
> +
> +	my $volnames = [];
> +	if ($include_snapshots) {
> + 	    my $snapshots =  $class->volume_snapshot_info($scfg, $storeid, $volname);
> +	    for my $snapid (sort { $snapshots->{$b}->{order} <=> $snapshots->{$a}->{order} } keys %$snapshots) {
> + 		my $snap = $snapshots->{$snapid};
> +		next if $snapid eq 'current';
> +		next if !$snap->{volid};
> +		next if !$snap->{ext};
> +		my ($snap_storeid, $snap_volname) = PVE::Storage::parse_volume_id($snap->{volid});
> +		push @$volnames, $snap_volname;
> +	    }
>  	}
> +	push @$volnames, $volname;
>  
> -	unlink($path) || die "unlink '$path' failed - $!\n";
> +	for my $name (@$volnames) {
> +	    my $path = $class->filesystem_path($scfg, $name);
> +	    if (!(-f $path || -l $path)) {
> +		warn "disk image '$path' does not exist\n";
> +	    } else {
> +		unlink($path) || die "unlink '$path' failed - $!\n";
> +	    }
> +	}
>      }
>  
>      # try to cleanup directory to not clutter storage with empty $vmid dirs if
> -- 
> 2.39.5


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel