[pve-devel] [RFC storage 0/3] rbd: use image-/snap-spec instead of --pool/..

[pve-devel] [RFC storage 0/3] rbd: use image-/snap-spec instead of --pool/..

[Fabian Grünbichler]

the 'rbd' CLI tool has two ways of specifying which images/snapshots to
operate on:
- the deprecated --(dest-)pool, --namespace, --image and --snap parameters
- the new style 'spec'-based variant, passing [pool/[namespace/]]image[@snapshot]

we are currently using a mix of both (passing pool, namespace and snap
as needed via parameters, and the image-name as argument). this patch
series switches everything to use image or snap specs, except for `ls`,
which can't be switched over, because it doesn't operate on images at
all.

`unmap` doesn't support `--namespace` (but doesn't need it, since it
takes the kernel dev as argument). `import` doesn't either, but does
require it when a namespace is used as import target (this bug prompted
the whole excursion resulting in this patch series).

I smoke-tested the changes, but it's possible I missed some operation or
corner case when converting, so additional testing is highly
appreciated.

Fabian Grünbichler (3):
  rbd: extend get_rbd_(dev_)path helpers with $snap parameter
  fix #6338: rbd: use image-/snap-spec consistently
  rbd: add protect/unprotect helpers

 src/PVE/Storage/RBDPlugin.pm | 167 ++++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 81 deletions(-)

-- 
2.39.5



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

[pve-devel] [PATCH storage 1/3] rbd: extend get_rbd_(dev_)path helpers with $snap parameter

[Fabian Grünbichler]

this helper effectively converts a storage config entry and a
volume/image name into an RBD image-spec as specified in rbd(8):

       image-spec      is [pool-name/[namespace-name/]]image-name

by extending it with an optional $snap parameter it can also convert to
a snap-spec:

       snap-spec       is [pool-name/[namespace-name/]]image-name@snap-name

this is a needed preparatory step for dropping the (mostly) deprecated
`--pool ..`, `--namespace ..` and `--snap ..` parameters when invoking
the `rbd` CLI tool.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/Storage/RBDPlugin.pm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 73bc97e..8c67a37 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -42,15 +42,16 @@ my $librados_connect = sub {
 };
 
 my sub get_rbd_path {
-    my ($scfg, $volume) = @_;
+    my ($scfg, $volume, $snap) = @_;
     my $path = $scfg->{pool} ? $scfg->{pool} : 'rbd';
     $path .= "/$scfg->{namespace}" if defined($scfg->{namespace});
     $path .= "/$volume" if defined($volume);
+    $path .= "\@$snap" if defined($snap);
     return $path;
 };
 
 my sub get_rbd_dev_path {
-    my ($scfg, $storeid, $volume) = @_;
+    my ($scfg, $storeid, $volume, $snap) = @_;
 
     my $cluster_id = '';
     if ($scfg->{fsid}) {
@@ -70,7 +71,7 @@ my sub get_rbd_dev_path {
 	die "cluster fsid has invalid format\n";
     }
 
-    my $rbd_path = get_rbd_path($scfg, $volume);
+    my $rbd_path = get_rbd_path($scfg, $volume, $snap);
     my $pve_path = "/dev/rbd-pve/${cluster_id}/${rbd_path}";
     my $path = "/dev/rbd/${rbd_path}";
 
-- 
2.39.5



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

[pve-devel] [PATCH storage 2/3] fix #6338: rbd: use image-/snap-spec consistently

[Fabian Grünbichler]

this is the recommended way upstream to pass this information, and all commands
(other than those not operating on images/snapshots) consume them (rbd(8)):

>       image-spec      is [pool-name/[namespace-name/]]image-name
>       snap-spec       is [pool-name/[namespace-name/]]image-name@snap-name
> [..]
> You may specify each name individually, using --pool, --namespace, --image,
> and --snap options, but this is discouraged in favor of the above spec
> syntax.

We already had a few places calling `get_rbd_path` previously (which meant
passing the pool and namespace information twice to Ceph).

It also allows us to replace the custom import and unmap handling with just
custom `ls` handling, reducing the workarounds in rbd_cmd.

Fixes: #6338 , because `rbd import` doesn't handle a separate `--namespace ..`
at all..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
we could probably switch some more only-technically-public subs over to
take a spec instead of image name or image + snapshot name, if desired..

alternatively, we could also just special case the import call, and/or convince
upstream to implement --namespace handling there as well or send a patch doing
that their way..

 src/PVE/Storage/RBDPlugin.pm | 133 ++++++++++++++++++-----------------
 1 file changed, 69 insertions(+), 64 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 8c67a37..3bb5807 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -89,22 +89,17 @@ my $rbd_cmd = sub {
     my ($scfg, $storeid, $op, @options) = @_;
 
     my $cmd_option = PVE::CephConfig::ceph_connect_option($scfg, $storeid);
-    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
 
     my $cmd = ['/usr/bin/rbd'];
-    if ($op eq 'import') {
-	push $cmd->@*, '--dest-pool', $pool;
-    } else {
+    # `ls` doesn't take an image-spec, otherwise pool and namespace should be specified there
+    if ($op eq 'ls') {
+	my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd';
 	push $cmd->@*, '-p', $pool;
+	if (defined($scfg->{namespace})) {
+	    push @$cmd, '--namespace', $cfg->{namespace};
+	}
     }
 
-    if (defined(my $namespace = $scfg->{namespace})) {
-	# some subcommands will fail if the --namespace parameter is present
-	my $no_namespace_parameter = {
-	    unmap => 1,
-	};
-	push @$cmd, '--namespace', "$namespace" if !$no_namespace_parameter->{$op};
-    }
     push @$cmd, '-c', $cmd_option->{ceph_conf} if ($cmd_option->{ceph_conf});
     push @$cmd, '-m', $cmd_option->{mon_host} if ($cmd_option->{mon_host});
     push @$cmd, '--auth_supported', $cmd_option->{auth_supported} if ($cmd_option->{auth_supported});
@@ -144,9 +139,11 @@ my $krbd_feature_update = sub {
     my $to_disable = join(',', grep {  $active_features->{$_} } @disable);
     my $to_enable  = join(',', grep { !$active_features->{$_} } @enable );
 
+    my $image_spec = get_rbd_path($scfg, $name);
+
     if ($to_disable) {
 	print "disable RBD image features this kernel RBD drivers is not compatible with: $to_disable\n";
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'feature', 'disable', $name, $to_disable);
+	my $cmd = $rbd_cmd->($scfg, $storeid, 'feature', 'disable', $image_spec, $to_disable);
 	run_rbd_command(
 	    $cmd,
 	    errmsg => "could not disable krbd-incompatible image features '$to_disable' for rbd image: $name",
@@ -155,7 +152,7 @@ my $krbd_feature_update = sub {
     if ($to_enable) {
 	print "enable RBD image features this kernel RBD drivers supports: $to_enable\n";
 	eval {
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'feature', 'enable', $name, $to_enable);
+	    my $cmd = $rbd_cmd->($scfg, $storeid, 'feature', 'enable', $image_spec, $to_enable);
 	    run_rbd_command(
 		$cmd,
 		errmsg => "could not enable krbd-compatible image features '$to_enable' for rbd image: $name",
@@ -234,9 +231,9 @@ sub rbd_ls {
 }
 
 sub rbd_ls_snap {
-    my ($scfg, $storeid, $name) = @_;
+    my ($scfg, $storeid, $image_spec) = @_;
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'ls', $name, '--format', 'json');
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'ls', $image_spec, '--format', 'json');
 
     my $raw = '';
     run_rbd_command($cmd, errmsg => "rbd error", errfunc => sub {}, outfunc => sub { $raw .= shift; });
@@ -244,9 +241,9 @@ sub rbd_ls_snap {
     my $list;
     if ($raw =~ m/^(\[.*\])$/s) { # untaint
 	$list = eval { JSON::decode_json($1) };
-	die "invalid JSON output from 'rbd snap ls $name': $@\n" if $@;
+	die "invalid JSON output from 'rbd snap ls $image_spec': $@\n" if $@;
     } else {
-	die "got unexpected data from 'rbd snap ls $name': '$raw'\n";
+	die "got unexpected data from 'rbd snap ls $image_spec': '$raw'\n";
     }
 
     $list = [] if !defined($list);
@@ -269,11 +266,9 @@ sub rbd_volume_info {
     my ($scfg, $storeid, $volname, $snap) = @_;
 
     my $cmd = undef;
+    my $spec = get_rbd_path($scfg, $volname, $snap);
 
-    my @options = ('info', $volname, '--format', 'json');
-    if ($snap) {
-	push @options, '--snap', $snap;
-    }
+    my @options = ('info', $spec, '--format', 'json');
 
     $cmd = $rbd_cmd->($scfg, $storeid, @options);
 
@@ -300,7 +295,8 @@ sub rbd_volume_info {
 sub rbd_volume_du {
     my ($scfg, $storeid, $volname) = @_;
 
-    my @options = ('du', $volname, '--format', 'json');
+    my $spec = get_rbd_path($scfg, $volname);
+    my @options = ('du', $spec, '--format', 'json');
     my $cmd = $rbd_cmd->($scfg, $storeid, @options);
 
     my $raw = '';
@@ -471,14 +467,13 @@ sub path {
 
     my $cmd_option = PVE::CephConfig::ceph_connect_option($scfg, $storeid);
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
-    $name .= '@'.$snapname if $snapname;
 
     if ($scfg->{krbd}) {
-	my $rbd_dev_path = get_rbd_dev_path($scfg, $storeid, $name);
+	my $rbd_dev_path = get_rbd_dev_path($scfg, $storeid, $name, $snapname);
 	return ($rbd_dev_path, $vmid, $vtype);
     }
 
-    my $rbd_path = get_rbd_path($scfg, $name);
+    my $rbd_path = get_rbd_path($scfg, $name, $snapname);
     my $path = "rbd:${rbd_path}";
 
     $path .= ":conf=$cmd_option->{ceph_conf}" if $cmd_option->{ceph_conf};
@@ -560,8 +555,9 @@ sub create_base {
     my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $newname, $snap);
 
     if (!$protected){
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $newname, '--snap', $snap);
-	run_rbd_command($cmd, errmsg => "rbd protect $newname snap '$snap' error");
+	my $snap_spec = get_rbd_path($scfg, $newname, $snap);
+	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
+	run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
     }
 
     return $newvolname;
@@ -588,8 +584,9 @@ sub clone_image {
 	my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $volname, $snapname);
 
 	if (!$protected) {
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $volname, '--snap', $snapname);
-	    run_rbd_command($cmd, errmsg => "rbd protect $volname snap $snapname error");
+	    my $snap_spec = get_rbd_path($scfg, $volname, $snapname);
+	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
+	    run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
 	}
     }
 
@@ -597,8 +594,7 @@ sub clone_image {
     $newvol = $name if length($snapname);
 
     my @options = (
-	get_rbd_path($scfg, $basename),
-	'--snap', $snap,
+	get_rbd_path($scfg, $basename, $snap),
     );
     push @options, ('--data-pool', $scfg->{'data-pool'}) if $scfg->{'data-pool'};
 
@@ -623,7 +619,7 @@ sub alloc_image {
     );
     push @options, ('--data-pool', $scfg->{'data-pool'}) if $scfg->{'data-pool'};
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'create', @options, $name);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'create', @options, get_rbd_path($scfg, $name));
     run_rbd_command($cmd, errmsg => "rbd create '$name' error");
 
     return $name;
@@ -635,22 +631,23 @@ sub free_image {
     my ($vtype, $name, $vmid, undef, undef, undef) =
 	$class->parse_volname($volname);
 
-
-    my $snaps = rbd_ls_snap($scfg, $storeid, $name);
+    my $image_spec = get_rbd_path($scfg, $name);
+    my $snaps = rbd_ls_snap($scfg, $storeid, $image_spec);
     foreach my $snap (keys %$snaps) {
 	if ($snaps->{$snap}->{protected}) {
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $name, '--snap', $snap);
-	    run_rbd_command($cmd, errmsg => "rbd unprotect $name snap '$snap' error");
+	    my $snap_spec = get_rbd_path($scfg, $name, $snap);
+	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $snap_spec);
+	    run_rbd_command($cmd, errmsg => "rbd unprotect $snap_spec error");
 	}
     }
 
     $class->deactivate_volume($storeid, $scfg, $volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'purge',  $name);
-    run_rbd_command($cmd, errmsg => "rbd snap purge '$name' error");
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'purge',  $image_spec);
+    run_rbd_command($cmd, errmsg => "rbd snap purge '$image_spec' error");
 
-    $cmd = $rbd_cmd->($scfg, $storeid, 'rm', $name);
-    run_rbd_command($cmd, errmsg => "rbd rm '$name' error");
+    $cmd = $rbd_cmd->($scfg, $storeid, 'rm', $image_spec);
+    run_rbd_command($cmd, errmsg => "rbd rm '$image_spec' error");
 
     return undef;
 }
@@ -725,20 +722,18 @@ sub deactivate_storage {
 sub map_volume {
     my ($class, $storeid, $scfg, $volname, $snapname) = @_;
 
-    my ($vtype, $img_name, $vmid) = $class->parse_volname($volname);
+    my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $name = $img_name;
-    $name .= '@'.$snapname if $snapname;
-
-    my $kerneldev = get_rbd_dev_path($scfg, $storeid, $name);
+    my $kerneldev = get_rbd_dev_path($scfg, $storeid, $name, $snapname);
 
     return $kerneldev if -b $kerneldev; # already mapped
 
     # features can only be enabled/disabled for image, not for snapshot!
-    $krbd_feature_update->($scfg, $storeid, $img_name);
+    $krbd_feature_update->($scfg, $storeid, $name);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $name);
-    run_rbd_command($cmd, errmsg => "can't map rbd volume $name");
+    my $spec = get_rbd_path($scfg, $name, $snapname);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'map', $spec);
+    run_rbd_command($cmd, errmsg => "can't map rbd volume '$spec'");
 
     return $kerneldev;
 }
@@ -747,9 +742,8 @@ sub unmap_volume {
     my ($class, $storeid, $scfg, $volname, $snapname) = @_;
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
-    $name .= '@'.$snapname if $snapname;
 
-    my $kerneldev = get_rbd_dev_path($scfg, $storeid, $name);
+    my $kerneldev = get_rbd_dev_path($scfg, $storeid, $name, $snapname);
 
     if (-b $kerneldev) {
 	my $cmd = $rbd_cmd->($scfg, $storeid, 'unmap', $kerneldev);
@@ -791,8 +785,10 @@ sub volume_resize {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', int(ceil($size/1024/1024)), $name);
-    run_rbd_command($cmd, errmsg => "rbd resize '$volname' error");
+    my $image_spec = get_rbd_path($scfg, $volname);
+    $size = int(ceil($size/1024/1024));
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'resize', '--size', $size, $image_spec);
+    run_rbd_command($cmd, errmsg => "rbd resize '$image_spec' error");
     return undef;
 }
 
@@ -801,8 +797,9 @@ sub volume_snapshot {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'create', '--snap', $snap, $name);
-    run_rbd_command($cmd, errmsg => "rbd snapshot '$volname' error");
+    my $snap_spec = get_rbd_path($scfg, $name, $snap);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'create', $snap_spec);
+    run_rbd_command($cmd, errmsg => "rbd snapshot '$snap_spec' error");
     return undef;
 }
 
@@ -811,8 +808,9 @@ sub volume_snapshot_rollback {
 
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'rollback', '--snap', $snap, $name);
-    run_rbd_command($cmd, errmsg => "rbd snapshot $volname to '$snap' error");
+    my $snap_spec = get_rbd_path($scfg, $name, $snap);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'rollback', $snap_spec);
+    run_rbd_command($cmd, errmsg => "rbd snapshot rollback to '$snap_spec' error");
 }
 
 sub volume_snapshot_delete {
@@ -823,14 +821,16 @@ sub volume_snapshot_delete {
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
     my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $name, $snap);
+    my $snap_spec = get_rbd_path($scfg, $volname, $snap);
+
     if ($protected){
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $name, '--snap', $snap);
-	run_rbd_command($cmd, errmsg => "rbd unprotect $name snap '$snap' error");
+	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $snap_spec);
+	run_rbd_command($cmd, errmsg => "rbd unprotect '$snap_spec' error");
     }
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'rm', '--snap', $snap, $name);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'rm', $snap_spec);
 
-    run_rbd_command($cmd, errmsg => "rbd snapshot '$volname' error");
+    run_rbd_command($cmd, errmsg => "rbd snapshot delete '$snap_spec' error");
 
     return undef;
 }
@@ -890,7 +890,9 @@ sub volume_export {
 
     my ($size) = $class->volume_size_info($scfg, $storeid, $volname);
     PVE::Storage::Plugin::write_common_header($fh, $size);
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'export', '--export-format', '1', $volname, '-');
+
+    my $snap_spec = get_rbd_path($scfg, $volname, $snapshot);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'export', '--export-format', '1', $snap_spec, '-');
     run_rbd_command(
 	$cmd,
 	errmsg => 'could not export image',
@@ -938,8 +940,9 @@ sub volume_import {
     my ($size) = PVE::Storage::Plugin::read_common_header($fh);
     $size = PVE::Storage::Common::align_size_up($size, 1024) / 1024;
 
+    my $image_spec = get_rbd_path($scfg, $volname);
     eval {
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'import', '--export-format', '1', '-', $volname);
+	my $cmd = $rbd_cmd->($scfg, $storeid, 'import', '--export-format', '1', '-', $image_spec);
 	run_rbd_command(
 	    $cmd,
 	    errmsg => 'could not import image',
@@ -976,11 +979,13 @@ sub rename_volume {
     die "target volume '${target_volname}' already exists\n"
 	if rbd_volume_exists($scfg, $storeid, $target_volname);
 
-    my $cmd = $rbd_cmd->($scfg, $storeid, 'rename', $source_image, $target_volname);
+    my $source_spec = get_rbd_path($scfg, $source_volname);
+    my $target_spec = get_rbd_path($scfg, $target_volname);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'rename', $source_spec, $target_spec);
 
     run_rbd_command(
 	$cmd,
-	errmsg => "could not rename image '${source_image}' to '${target_volname}'",
+	errmsg => "could not rename image '${source_spec}' to '${target_spec}'",
     );
 
     eval { $class->unmap_volume($storeid, $scfg, $source_volname); };
-- 
2.39.5



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

[pve-devel] [PATCH storage 3/3] rbd: add protect/unprotect helpers

[Fabian Grünbichler]

this is a bit repetitive otherwise, no functional changes intended.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/Storage/RBDPlugin.pm | 55 ++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 3bb5807..6604bc3 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -96,7 +96,7 @@ my $rbd_cmd = sub {
 	my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd';
 	push $cmd->@*, '-p', $pool;
 	if (defined($scfg->{namespace})) {
-	    push @$cmd, '--namespace', $cfg->{namespace};
+	    push @$cmd, '--namespace', $scfg->{namespace};
 	}
     }
 
@@ -262,6 +262,25 @@ sub rbd_ls_snap {
     return $res;
 }
 
+my sub rbd_protect_snap {
+    my ($scfg, $storeid, $image, $snap) = @_;
+    my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $image, $snap);
+
+    if (!$protected){
+	my $snap_spec = get_rbd_path($scfg, $image, $snap);
+	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
+	run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
+    }
+}
+
+my sub rbd_unprotect_snap {
+    my ($scfg, $storeid, $image, $snap) = @_;
+
+    my $snap_spec = get_rbd_path($scfg, $image, $snap);
+    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $snap_spec);
+    run_rbd_command($cmd, errmsg => "rbd unprotect '$snap_spec' error");
+}
+
 sub rbd_volume_info {
     my ($scfg, $storeid, $volname, $snap) = @_;
 
@@ -552,13 +571,7 @@ sub create_base {
 
     $class->volume_snapshot($scfg, $storeid, $newname, $snap, $running);
 
-    my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $newname, $snap);
-
-    if (!$protected){
-	my $snap_spec = get_rbd_path($scfg, $newname, $snap);
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
-	run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
-    }
+    rbd_protect_snap($scfg, $storeid, $newname, $snap);
 
     return $newvolname;
 
@@ -580,15 +593,7 @@ sub clone_image {
 
     warn "clone $volname: $basename snapname $snap to $name\n";
 
-    if (length($snapname)) {
-	my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $volname, $snapname);
-
-	if (!$protected) {
-	    my $snap_spec = get_rbd_path($scfg, $volname, $snapname);
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
-	    run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
-	}
-    }
+    rbd_protect_snap($scfg, $storeid, $volname, $snap) if length($snapname);
 
     my $newvol = "$basename/$name";
     $newvol = $name if length($snapname);
@@ -634,11 +639,8 @@ sub free_image {
     my $image_spec = get_rbd_path($scfg, $name);
     my $snaps = rbd_ls_snap($scfg, $storeid, $image_spec);
     foreach my $snap (keys %$snaps) {
-	if ($snaps->{$snap}->{protected}) {
-	    my $snap_spec = get_rbd_path($scfg, $name, $snap);
-	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $snap_spec);
-	    run_rbd_command($cmd, errmsg => "rbd unprotect $snap_spec error");
-	}
+	rbd_unprotect_snap($scfg, $storeid, $name, $snap)
+	    if $snaps->{$snap}->{protected};
     }
 
     $class->deactivate_volume($storeid, $scfg, $volname);
@@ -821,13 +823,10 @@ sub volume_snapshot_delete {
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
 
     my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $name, $snap);
+
+    rbd_unprotect_snap($scfg, $storeid, $volname, $snap) if $protected;
+
     my $snap_spec = get_rbd_path($scfg, $volname, $snap);
-
-    if ($protected){
-	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'unprotect', $snap_spec);
-	run_rbd_command($cmd, errmsg => "rbd unprotect '$snap_spec' error");
-    }
-
     my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'rm', $snap_spec);
 
     run_rbd_command($cmd, errmsg => "rbd snapshot delete '$snap_spec' error");
-- 
2.39.5



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

Re: [pve-devel] [RFC storage 0/3] rbd: use image-/snap-spec instead of --pool/..

[Aaron Lauterer]

Tested this patch series by:

* create a new VM on ceph RBD
* took & removed snapshot (with memory)
* cloned VM
* converted to template (rename from vm-… to base-… worked)
* created linked clone
* destroy template -> fail due to linked clone
* destroy linked clone
* destroy template

namespace tests:
* create rbd namespace
* move-disk of VM to namespace (main disk, EFI, tpm)
* remote migrate from rbd namespace to other node with local-lvm
* remote migrate back from local-lvm to rbd namespace storage


The code changes look good and I didn't notice anything there.

So with this, consider this series

Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>

On  2025-04-23  15:59, Fabian Grünbichler wrote:
> the 'rbd' CLI tool has two ways of specifying which images/snapshots to
> operate on:
> - the deprecated --(dest-)pool, --namespace, --image and --snap parameters
> - the new style 'spec'-based variant, passing [pool/[namespace/]]image[@snapshot]
> 
> we are currently using a mix of both (passing pool, namespace and snap
> as needed via parameters, and the image-name as argument). this patch
> series switches everything to use image or snap specs, except for `ls`,
> which can't be switched over, because it doesn't operate on images at
> all.
> 
> `unmap` doesn't support `--namespace` (but doesn't need it, since it
> takes the kernel dev as argument). `import` doesn't either, but does
> require it when a namespace is used as import target (this bug prompted
> the whole excursion resulting in this patch series).
> 
> I smoke-tested the changes, but it's possible I missed some operation or
> corner case when converting, so additional testing is highly
> appreciated.
> 
> Fabian Grünbichler (3):
>    rbd: extend get_rbd_(dev_)path helpers with $snap parameter
>    fix #6338: rbd: use image-/snap-spec consistently
>    rbd: add protect/unprotect helpers
> 
>   src/PVE/Storage/RBDPlugin.pm | 167 ++++++++++++++++++-----------------
>   1 file changed, 86 insertions(+), 81 deletions(-)
> 



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

Re: [pve-devel] [PATCH storage 2/3] fix #6338: rbd: use image-/snap-spec consistently

[Fiona Ebner]

Am 23.04.25 um 15:59 schrieb Fabian Grünbichler:
> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
> index 8c67a37..3bb5807 100644
> --- a/src/PVE/Storage/RBDPlugin.pm
> +++ b/src/PVE/Storage/RBDPlugin.pm
> @@ -89,22 +89,17 @@ my $rbd_cmd = sub {
>      my ($scfg, $storeid, $op, @options) = @_;
>  
>      my $cmd_option = PVE::CephConfig::ceph_connect_option($scfg, $storeid);
> -    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  
>      my $cmd = ['/usr/bin/rbd'];
> -    if ($op eq 'import') {
> -	push $cmd->@*, '--dest-pool', $pool;
> -    } else {
> +    # `ls` doesn't take an image-spec, otherwise pool and namespace should be specified there
> +    if ($op eq 'ls') {
> +	my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  	push $cmd->@*, '-p', $pool;
> +	if (defined($scfg->{namespace})) {
> +	    push @$cmd, '--namespace', $cfg->{namespace};

Typo here that's fixed in next commit

> @@ -890,7 +890,9 @@ sub volume_export {
>  
>      my ($size) = $class->volume_size_info($scfg, $storeid, $volname);
>      PVE::Storage::Plugin::write_common_header($fh, $size);
> -    my $cmd = $rbd_cmd->($scfg, $storeid, 'export', '--export-format', '1', $volname, '-');
> +
> +    my $snap_spec = get_rbd_path($scfg, $volname, $snapshot);
> +    my $cmd = $rbd_cmd->($scfg, $storeid, 'export', '--export-format', '1', $snap_spec, '-');
>      run_rbd_command(
>  	$cmd,
>  	errmsg => 'could not export image',

Oh, so a snapshot was never exported before, but always the image? Mea
culpa. Can only be reached via manual CLI luckily, but still good to get
fixed. You'll need to adapt getting the size too in the lines above,
that still uses just $volname.

Other than those, looks good to me.


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

Re: [pve-devel] [PATCH storage 3/3] rbd: add protect/unprotect helpers

[Fiona Ebner]

Am 23.04.25 um 15:59 schrieb Fabian Grünbichler:
> this is a bit repetitive otherwise, no functional changes intended.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/PVE/Storage/RBDPlugin.pm | 55 ++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
> index 3bb5807..6604bc3 100644
> --- a/src/PVE/Storage/RBDPlugin.pm
> +++ b/src/PVE/Storage/RBDPlugin.pm
> @@ -96,7 +96,7 @@ my $rbd_cmd = sub {
>  	my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  	push $cmd->@*, '-p', $pool;
>  	if (defined($scfg->{namespace})) {
> -	    push @$cmd, '--namespace', $cfg->{namespace};
> +	    push @$cmd, '--namespace', $scfg->{namespace};
>  	}
>      }
>  

Should be squashed into previous commit.

> @@ -262,6 +262,25 @@ sub rbd_ls_snap {
>      return $res;
>  }
>  
> +my sub rbd_protect_snap {
> +    my ($scfg, $storeid, $image, $snap) = @_;
> +    my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $image, $snap);
> +
> +    if (!$protected){

Style nit: missing space between '){'

> +	my $snap_spec = get_rbd_path($scfg, $image, $snap);
> +	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
> +	run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
> +    }
> +}
> +


> @@ -580,15 +593,7 @@ sub clone_image {
>  
>      warn "clone $volname: $basename snapname $snap to $name\n";
>  
> -    if (length($snapname)) {
> -	my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $volname, $snapname);
> -
> -	if (!$protected) {
> -	    my $snap_spec = get_rbd_path($scfg, $volname, $snapname);
> -	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
> -	    run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
> -	}
> -    }
> +    rbd_protect_snap($scfg, $storeid, $volname, $snap) if length($snapname);

Last argument should be $snapname

>  
>      my $newvol = "$basename/$name";
>      $newvol = $name if length($snapname);


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

Re: [pve-devel] [PATCH storage 3/3] rbd: add protect/unprotect helpers

[Fabian Grünbichler]

On May 5, 2025 4:24 pm, Fiona Ebner wrote:
> Am 23.04.25 um 15:59 schrieb Fabian Grünbichler:
>> this is a bit repetitive otherwise, no functional changes intended.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>  src/PVE/Storage/RBDPlugin.pm | 55 ++++++++++++++++++------------------
>>  1 file changed, 27 insertions(+), 28 deletions(-)
>> 
>> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
>> index 3bb5807..6604bc3 100644
>> --- a/src/PVE/Storage/RBDPlugin.pm
>> +++ b/src/PVE/Storage/RBDPlugin.pm
>> @@ -96,7 +96,7 @@ my $rbd_cmd = sub {
>>  	my $pool = $scfg->{pool} ? $scfg->{pool} : 'rbd';
>>  	push $cmd->@*, '-p', $pool;
>>  	if (defined($scfg->{namespace})) {
>> -	    push @$cmd, '--namespace', $cfg->{namespace};
>> +	    push @$cmd, '--namespace', $scfg->{namespace};
>>  	}
>>      }
>>  
> 
> Should be squashed into previous commit.

ack

> 
>> @@ -262,6 +262,25 @@ sub rbd_ls_snap {
>>      return $res;
>>  }
>>  
>> +my sub rbd_protect_snap {
>> +    my ($scfg, $storeid, $image, $snap) = @_;
>> +    my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $image, $snap);
>> +
>> +    if (!$protected){
> 
> Style nit: missing space between '){'

ack

> 
>> +	my $snap_spec = get_rbd_path($scfg, $image, $snap);
>> +	my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
>> +	run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
>> +    }
>> +}
>> +
> 
> 
>> @@ -580,15 +593,7 @@ sub clone_image {
>>  
>>      warn "clone $volname: $basename snapname $snap to $name\n";
>>  
>> -    if (length($snapname)) {
>> -	my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $volname, $snapname);
>> -
>> -	if (!$protected) {
>> -	    my $snap_spec = get_rbd_path($scfg, $volname, $snapname);
>> -	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
>> -	    run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
>> -	}
>> -    }
>> +    rbd_protect_snap($scfg, $storeid, $volname, $snap) if length($snapname);
> 
> Last argument should be $snapname

not really - this is saying "protect the snapshot $snap if it is not the
(already protected) '__base__' snapshot". probably the whole logic
should be adapted to make this explicit though, e.g., something like:

diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
index 6604bc3..5c1a8e2 100644
--- a/src/PVE/Storage/RBDPlugin.pm
+++ b/src/PVE/Storage/RBDPlugin.pm
@@ -580,23 +580,26 @@ sub create_base {
 sub clone_image {
     my ($class, $scfg, $storeid, $volname, $vmid, $snapname) = @_;
 
-    my $snap = '__base__';
-    $snap = $snapname if length $snapname;
-
     my ($vtype, $basename, $basevmid, undef, undef, $isBase) =
         $class->parse_volname($volname);
 
-    die "$volname is not a base image and snapname is not provided\n" 
-	if !$isBase && !length($snapname);
+    my $snap;
+
+    if ($snapname) {
+	$snap = $snapname;
+    } elsif ($isBase) {
+	$snap = '__base__';
+    } else {
+	die "$volname is not a base image and snapname is not provided\n";
+    }
 
     my $name = $class->find_free_diskname($storeid, $scfg, $vmid);
 
     warn "clone $volname: $basename snapname $snap to $name\n";
 
-    rbd_protect_snap($scfg, $storeid, $volname, $snap) if length($snapname);
+    rbd_protect_snap($scfg, $storeid, $volname, $snap) if $snap ne '__base__';
 
-    my $newvol = "$basename/$name";
-    $newvol = $name if length($snapname);
+    my $newvol = $snapname ? $name : "$basename/$name";
 
     my @options = (
 	get_rbd_path($scfg, $basename, $snap),



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

Re: [pve-devel] [PATCH storage 3/3] rbd: add protect/unprotect helpers

[Fiona Ebner]

Am 06.05.25 um 13:07 schrieb Fabian Grünbichler:
>>> @@ -580,15 +593,7 @@ sub clone_image {
>>>  
>>>      warn "clone $volname: $basename snapname $snap to $name\n";
>>>  
>>> -    if (length($snapname)) {
>>> -	my (undef, undef, undef, $protected) = rbd_volume_info($scfg, $storeid, $volname, $snapname);
>>> -
>>> -	if (!$protected) {
>>> -	    my $snap_spec = get_rbd_path($scfg, $volname, $snapname);
>>> -	    my $cmd = $rbd_cmd->($scfg, $storeid, 'snap', 'protect', $snap_spec);
>>> -	    run_rbd_command($cmd, errmsg => "rbd protect '$snap_spec' error");
>>> -	}
>>> -    }
>>> +    rbd_protect_snap($scfg, $storeid, $volname, $snap) if length($snapname);
>>
>> Last argument should be $snapname
> 
> not really - this is saying "protect the snapshot $snap if it is not the
> (already protected) '__base__' snapshot". probably the whole logic
> should be adapted to make this explicit though, e.g., something like:

Oh right, it doesn't matter semantically, because there is
$snap = $snapname if length $snapname;
further above. I just compared with the old code that used $snapname as
the argument, which you don't anymore.

> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
> index 6604bc3..5c1a8e2 100644
> --- a/src/PVE/Storage/RBDPlugin.pm
> +++ b/src/PVE/Storage/RBDPlugin.pm
> @@ -580,23 +580,26 @@ sub create_base {
>  sub clone_image {
>      my ($class, $scfg, $storeid, $volname, $vmid, $snapname) = @_;
>  
> -    my $snap = '__base__';
> -    $snap = $snapname if length $snapname;
> -
>      my ($vtype, $basename, $basevmid, undef, undef, $isBase) =
>          $class->parse_volname($volname);
>  
> -    die "$volname is not a base image and snapname is not provided\n" 
> -	if !$isBase && !length($snapname);
> +    my $snap;
> +
> +    if ($snapname) {
> +	$snap = $snapname;
> +    } elsif ($isBase) {
> +	$snap = '__base__';
> +    } else {
> +	die "$volname is not a base image and snapname is not provided\n";
> +    }
>  
>      my $name = $class->find_free_diskname($storeid, $scfg, $vmid);
>  
>      warn "clone $volname: $basename snapname $snap to $name\n";
>  
> -    rbd_protect_snap($scfg, $storeid, $volname, $snap) if length($snapname);
> +    rbd_protect_snap($scfg, $storeid, $volname, $snap) if $snap ne '__base__';
>  
> -    my $newvol = "$basename/$name";
> -    $newvol = $name if length($snapname);
> +    my $newvol = $snapname ? $name : "$basename/$name";
>  
>      my @options = (
>  	get_rbd_path($scfg, $basename, $snap),
> 

Looks fine to me at a glance.


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