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

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

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