[pve-devel] [PATCH v2 storage 0/3] ceph: add namespace support

[pve-devel] [PATCH v2 storage 0/3] ceph: add namespace support

[Aaron Lauterer]

This series introduces namespace support for Ceph RBD on the PVE side.

The first patch reworks the RBD storage plugin to use one central sub to
create the RBD paths for <pool>/<image>. With this patch applied, adding
support for namespaces involves very little changes to the RBD plugin.

v1 -> v2:
* new patch (centralize rbd path concatenation) has been
  introduced
* namespace checks now avoid matching the contents of the
  $scfg->{namespace} but check if it is defined. This is done to avoid
  evaluating a namespace called '0' for example, to evaluate to false.
* integration test has been reworked according to feedback

Aaron Lauterer (3):
  rbd: centralize rbd path concatenation
  rbd: fix #3286 add namespace support
  rbd: add integration test for namespace handling

 PVE/Storage/RBDPlugin.pm |  63 +++++--
 test/rbd_namespace.pl    | 370 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 415 insertions(+), 18 deletions(-)
 create mode 100755 test/rbd_namespace.pl

-- 
2.20.1

[pve-devel] [PATCH v2 storage 1/3] rbd: centralize rbd path concatenation

[Aaron Lauterer]

The <pool>/<image> paths are needed in quite a lot of places. Having one
single place where they are created helps to reduce duplicate code and
makes it easier to introduce new features.

The 'add_pool_to_disk' sub was already doing that but the name was not
really fitting. This commit renames it to the more general
'get_rbd_path' and changes the second parameter to the more widely used
$volume instead of $disk.

Furthermore, all occurences where "$pool/$volume" has been concatenated
have been replaced with a call to get_rbd_path.

Plus some minor code style cleanups for long function calls that were
touched.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 PVE/Storage/RBDPlugin.pm | 46 +++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index fab6d57..421539f 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -22,12 +22,10 @@ my $get_parent_image_name = sub {
     return $parent->{image} . "@" . $parent->{snapshot};
 };
 
-my $add_pool_to_disk = sub {
-    my ($scfg, $disk) = @_;
-
+my $get_rbd_path = sub {
+    my ($scfg, $volume) = @_;
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
-
-    return "$pool/$disk";
+    return "${pool}/${volume}";
 };
 
 my $build_cmd = sub {
@@ -348,10 +346,10 @@ sub path {
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
     $name .= '@'.$snapname if $snapname;
 
-    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
-    return ("/dev/rbd/$pool/$name", $vmid, $vtype) if $scfg->{krbd};
+    my $rbd_path = &$get_rbd_path($scfg, $name);
+    return ("/dev/rbd/${rbd_path}", $vmid, $vtype) if $scfg->{krbd};
 
-    my $path = "rbd:$pool/$name";
+    my $path = "rbd:${rbd_path}";
 
     $path .= ":conf=$cmd_option->{ceph_conf}" if $cmd_option->{ceph_conf};
     if (defined($scfg->{monhost})) {
@@ -412,7 +410,13 @@ sub create_base {
 
     my $newvolname = $basename ? "$basename/$newname" : "$newname";
 
-    my $cmd = &$rbd_cmd($scfg, $storeid, 'rename', &$add_pool_to_disk($scfg, $name), &$add_pool_to_disk($scfg, $newname));
+    my $cmd = &$rbd_cmd(
+	$scfg,
+	$storeid,
+	'rename',
+	&$get_rbd_path($scfg, $name),
+	&$get_rbd_path($scfg, $newname),
+    );
     run_rbd_command($cmd, errmsg => "rbd rename '$name' error");
 
     my $running  = undef; #fixme : is create_base always offline ?
@@ -458,8 +462,15 @@ sub clone_image {
     my $newvol = "$basename/$name";
     $newvol = $name if length($snapname);
 
-    my $cmd = &$rbd_cmd($scfg, $storeid, 'clone', &$add_pool_to_disk($scfg, $basename), 
-			'--snap', $snap, &$add_pool_to_disk($scfg, $name));
+    my $cmd = &$rbd_cmd(
+	$scfg,
+	$storeid,
+	'clone',
+	&$get_rbd_path($scfg, $basename),
+	'--snap',
+	$snap,
+	&$get_rbd_path($scfg, $name),
+    );
 
     run_rbd_command($cmd, errmsg => "rbd clone '$basename' error");
 
@@ -575,9 +586,10 @@ sub deactivate_storage {
 }
 
 my $get_kernel_device_name = sub {
-    my ($pool, $name) = @_;
+    my ($scfg, $name) = @_;
 
-    return "/dev/rbd/$pool/$name";
+    my $rbd_path = &$get_rbd_path($scfg, $name);
+    return "/dev/rbd/${rbd_path}";
 };
 
 sub map_volume {
@@ -588,9 +600,7 @@ sub map_volume {
     my $name = $img_name;
     $name .= '@'.$snapname if $snapname;
 
-    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
-
-    my $kerneldev = $get_kernel_device_name->($pool, $name);
+    my $kerneldev = $get_kernel_device_name->($scfg, $name);
 
     return $kerneldev if -b $kerneldev; # already mapped
 
@@ -609,9 +619,7 @@ sub unmap_volume {
     my ($vtype, $name, $vmid) = $class->parse_volname($volname);
     $name .= '@'.$snapname if $snapname;
 
-    my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
-
-    my $kerneldev = $get_kernel_device_name->($pool, $name);
+    my $kerneldev = $get_kernel_device_name->($scfg, $name);
 
     if (-b $kerneldev) {
 	my $cmd = &$rbd_cmd($scfg, $storeid, 'unmap', $kerneldev);
-- 
2.20.1

[pve-devel] [PATCH v2 storage 2/3] rbd: fix #3286 add namespace support

[Aaron Lauterer]

This patch introduces support for Cephs RBD namespaces.

A new storage config parameter 'namespace' defines the namespace to be
used for the RBD storage.

The namespace must already exist in the Ceph cluster as it is not
automatically created.

The main intention is to use this for external Ceph clusters. With
namespaces, each PVE cluster can get its own namespace and will not
conflict with other PVE clusters.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>

---
v1 -> v2:
use `defined` to check if a namespace if configured instead of
evaluating the value which would wrongly evaluate to false if the pool
would be called '0'. Thx @Thomas for pointing this out.

much less changes since the 'centralize rbd path concatenation' patch
took care of most instances.

removed empty new lines that I did not catch in the previous version

rvc -> v1:
add --namespace parameter centrally in sub $build_cmd. All commands
except one (rbd unmap) support it. To handle commands that don't support
it, a hash with them was introduced.

In a few places paths (FS, Ceph hierarchy) are needed. These are the
places scattered throughout the plugin where the namespace is inserted
if it is configured.

 PVE/Storage/RBDPlugin.pm | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index 421539f..02950be 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -25,6 +25,8 @@ my $get_parent_image_name = sub {
 my $get_rbd_path = sub {
     my ($scfg, $volume) = @_;
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
+
+    return "${pool}/$scfg->{namespace}/${volume}" if defined($scfg->{namespace});
     return "${pool}/${volume}";
 };
 
@@ -36,6 +38,14 @@ my $build_cmd = sub {
 
     my $cmd = [$binary, '-p', $pool];
 
+    # some subcommands will fail if the --namespace parameter is present
+    my $no_namespace_parameter = {
+	unmap => 1,
+    };
+
+    push @$cmd, '--namespace', $scfg->{namespace}
+	if ($scfg->{namespace} && !$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});
@@ -152,6 +162,7 @@ sub rbd_ls {
 
     my $cmd = &$rbd_cmd($scfg, $storeid, 'ls', '-l', '--format', 'json');
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
+    $pool .= "/$scfg->{namespace}" if defined($scfg->{namespace});
 
     my $raw = '';
     my $parser = sub { $raw .= shift };
@@ -279,6 +290,10 @@ sub properties {
 	    description => "Pool.",
 	    type => 'string',
 	},
+	namespace=> {
+	    description => "RBD Namespace.",
+	    type => 'string',
+	},
 	username => {
 	    description => "RBD Id.",
 	    type => 'string',
@@ -300,6 +315,7 @@ sub options {
 	disable => { optional => 1 },
 	monhost => { optional => 1},
 	pool => { optional => 1 },
+	namespace => { optional => 1 },
 	username => { optional => 1 },
 	content => { optional => 1 },
 	krbd => { optional => 1 },
@@ -368,6 +384,7 @@ sub find_free_diskname {
     my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
 
     my $cmd = &$rbd_cmd($scfg, $storeid, 'ls');
+
     my $disk_list = [];
 
     my $parser = sub {
@@ -498,6 +515,7 @@ sub free_image {
     my ($vtype, $name, $vmid, undef, undef, undef) =
 	$class->parse_volname($volname);
 
+
     my $snaps = rbd_ls_snap($scfg, $storeid, $name);
     foreach my $snap (keys %$snaps) {
 	if ($snaps->{$snap}->{protected}) {
@@ -522,6 +540,7 @@ sub list_images {
 
     $cache->{rbd} = rbd_ls($scfg, $storeid) if !$cache->{rbd};
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
+    $pool .= "/$scfg->{namespace}" if defined($scfg->{namespace});
 
     my $res = [];
 
-- 
2.20.1

[pve-devel] [PATCH v2 storage 3/3] rbd: add integration test for namespace handling

[Aaron Lauterer]

This test is intended to be run on a hyperconverged PVE cluster to test
the most common operations of VMs using a namespaced Ceph RBD pool.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v1 -> v2:
reworked the test from the feedback I got [0].

* tests are now defined in deeper hashes/arrays and can contain testing
  steps as well as preparation and cleanup steps where needed.
* command calls don't use fixed paths
* command calls use arrays
* a new Ceph pool will be created and removed at the end by default. Can
  be overriden with the '--use_existing' parameter. Most likely in
  combination with the '--pool' parameter.
* debug flag has been introduced to be used if some debug printing via
  the `jp` sub is needed. Currenctly not used but might be useful when
  tinkering with the test in the future. Thx @Thomas for the hint


[0] https://lists.proxmox.com/pipermail/pve-devel/2021-April/047472.html

 test/rbd_namespace.pl | 370 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 370 insertions(+)
 create mode 100755 test/rbd_namespace.pl

diff --git a/test/rbd_namespace.pl b/test/rbd_namespace.pl
new file mode 100755
index 0000000..c6a9468
--- /dev/null
+++ b/test/rbd_namespace.pl
@@ -0,0 +1,370 @@
+#!/usr/bin/perl
+
+# This script is meant to be run manually on hyperconverged PVE server with a
+# Ceph cluster. It tests how PVE handles RBD namespaces.
+#
+# The pool (default: rbd) must already exist. The namespace and VMs will be
+# created.
+#
+# Parameters like names for the pool an namespace and the VMID can be
+# configured.  The VMIDs for the clones is $vmid -1 and $vmid -2.
+#
+# Cleanup is done after a successful run. Cleanup can also be called manually.
+#
+# Known issues:
+#
+# * Snapshot rollback can sometimes be racy with stopping the VM and Ceph
+#  recognizing that the disk image is not in use anymore.
+
+use strict;
+use warnings;
+
+use Test::More;
+use Getopt::Long;
+use JSON;
+
+use PVE::Tools qw(run_command);
+
+my $pool = "testpool";
+my $use_existing= undef;
+my $namespace = "testspace";
+my $showhelp = '';
+my $vmid = 999999;
+my $cleanup = undef;
+my $DEBUG = 0;
+
+my $helpstring = "To override default values, set them as named parameters:
+
+--pool		pool name, default: ${pool}
+--use_existing  use existing pool, default: 0, needs --pool set
+--namespace	rbd namespace, default: ${namespace}
+--vmid		VMID of the test VM, default: ${vmid}
+--cleanup	Remove the storage definitions, namespaces and VMs
+--debug		Enable debug output\n";
+
+GetOptions (
+	"pool=s" => \$pool,
+	"use_existing" => \$use_existing,
+	"namespace=s" => \$namespace,
+	"vmid=i" => \$vmid,
+	"help" => \$showhelp,
+	"cleanup" => \$cleanup,
+	"debug" => \$DEBUG,
+) or die ($helpstring);
+
+die $helpstring if $showhelp;
+
+my $storage_name = "${pool}-${namespace}";
+
+my $vmid_clone = int($vmid) - 1;
+my $vmid_linked_clone = int($vmid) - 2;
+
+sub jp {
+    return if !$DEBUG;
+    print to_json($_[0], { utf8 => 8, pretty => 1, canonical => 1 }) . "\n";
+}
+
+sub run_cmd {
+    my ($cmd, $json, $ignore_errors) = @_;
+
+    my $raw = '';
+    my $parser = sub {$raw .= shift;};
+
+    eval {
+	run_command($cmd, outfunc => $parser);
+    };
+    if (my $err = $@) {
+	die $err if !$ignore_errors;
+    }
+
+    if ($json) {
+	my $result;
+	if ($raw eq '') {
+	    $result = [];
+	} elsif ($raw =~ m/^(\[.*\])$/s) { # untaint
+	    $result = JSON::decode_json($1);
+	} else {
+	    die "got unexpected data from command: '$cmd' -> '$raw'\n";
+	}
+	return $result;
+	}
+    return $raw;
+}
+
+sub run_test_cmd {
+    my ($cmd) = @_;
+
+    my $raw = '';
+    my $out = sub {
+	my $line = shift;
+	$raw .= "${line}\n";
+    };
+
+    eval {
+	run_command($cmd, outfunc => $out);
+    };
+    if (my $err = $@) {
+	print $raw;
+	print $err;
+	return 0;
+    }
+    print $raw;
+    return 1;
+}
+
+sub prepare {
+    print "Preparing test environent\n";
+
+    my $pools = run_cmd("ceph osd pool ls --format json", 1);
+
+    my %poolnames = map {$_ => 1} @$pools;
+    die "Pool '$pool' does not exist!\n"
+	if !exists($poolnames{$pool}) && $use_existing;
+
+    run_cmd(['pveceph', 'pool', 'create', ${pool}, '--add_storages', 1])
+	if !$use_existing;
+
+    my $namespaces = run_cmd(['rbd', '-p', ${pool}, 'namespace', 'ls', '--format', 'json'], 1);
+    my $ns_found = 0;
+    for my $i (@$namespaces) {
+	#print Dumper $i;
+	$ns_found = 1 if $i->{name} eq $namespace;
+    }
+
+    if (!$ns_found) {
+	print "Create namespace '${namespace}' in pool '${pool}'\n";
+	run_cmd(['rbd', 'namespace', 'create', "${pool}/${namespace}"]);
+    }
+
+    my $storages = run_cmd(['pvesh', 'get', 'storage', '--output-format', 'json'], 1);
+    #print Dumper $storages;
+    my $rbd_found = 0;
+    my $pool_found = 0;
+
+    print "Create storage definition\n";
+    for my $stor (@$storages) {
+	$pool_found = 1 if $stor->{storage} eq $pool;
+	$rbd_found = 1 if $stor->{storage} eq $storage_name;
+
+	if ($rbd_found) {
+	    run_cmd(['pvesm', 'set', ${storage_name}, '--krbd', '0']);
+	    die "Enable the storage '$stor->{storage}'!" if $stor->{disable};
+	}
+    }
+    if (!$pool_found) {
+	die "No storage for pool '${pool}' found! Must have same name as pool!\n"
+	    if $use_existing;
+
+	run_cmd(['pvesm', 'add', 'rbd', $pool, '--pool', $pool, '--content', 'images,rootdir']);
+    }
+    # create PVE storages (librbd / krbd)
+    run_cmd(['pvesm', 'add', 'rbd', ${storage_name}, '--krbd', '0', '--pool', ${pool}, '--namespace', ${namespace}, '--content', 'images,rootdir'])
+	if !$rbd_found;
+
+
+    # create test VM
+    print "Create test VM ${vmid}\n";
+    my $vms = run_cmd(['pvesh', 'get', 'cluster/resources', '--type', 'vm', '--output-format', 'json'], 1);
+    for my $vm (@$vms) {
+	# TODO: introduce a force flag to make this behaviour configurable
+
+	if ($vm->{vmid} eq $vmid) {
+	    print "Test VM '${vmid}' already exists. It will be removed and recreated!\n";
+	    run_cmd(['qm', 'stop', ${vmid}], 0, 1);
+	    run_cmd(['qm', 'destroy', ${vmid}]);
+	}
+    }
+    run_cmd(['qm', 'create', ${vmid}, '--bios', 'ovmf', '--efidisk0', "${storage_name}:1", '--scsi0', "${storage_name}:2"]);
+}
+
+
+sub cleanup {
+    print "Cleaning up test environment!\n";
+    print "Removing VMs\n";
+    run_cmd(['qm', 'stop', ${vmid}], 0, 1);
+    run_cmd(['qm', 'stop', ${vmid_linked_clone}], 0, 1);
+    run_cmd(['qm', 'stop', ${vmid_clone}], 0, 1);
+    run_cmd(['qm', 'destroy', ${vmid_linked_clone}], 0, 1);
+    run_cmd(['qm', 'destroy', ${vmid_clone}], 0, 1);
+    run_cmd(['for', 'i', 'in', "/dev/rbd/${pool}/${namespace}/*;", 'do', '/usr/bin/rbd', 'unmap', '\$i;', 'done'], 0, 1);
+    run_cmd(['qm', 'unlock', ${vmid}], 0, 1);
+    run_cmd(['qm', 'destroy', ${vmid}], 0, 1);
+
+    print "Removing Storage definition for ${storage_name}\n";
+    run_cmd(['pvesm', 'remove', ${storage_name}], 0, 1);
+
+    print "Removing RBD namespace '${pool}/${namespace}'\n";
+    run_cmd(['rbd', 'namespace', 'remove', "${pool}/${namespace}"], 0, 1);
+
+    if (!$use_existing) {
+	print "Removing Storage definition for ${pool}\n";
+	run_cmd(['pvesm', 'remove', ${pool}], 0, 1);
+	print "Removing test pool\n";
+	run_cmd(['pveceph', 'pool', 'destroy', $pool]);
+    }
+}
+
+my $tests = [
+    # Example structure for tests
+    # {
+    #     name => "name of test section",
+    #     preparations => [
+    #         ['some', 'prep', 'command'],
+    #     ],
+    #     steps => [
+    #         ['test', 'cmd', $vmid],
+    #         ['second', 'step', $vmid],
+    #     ],
+    #     cleanup => [
+    #         ['cleanup', 'command'],
+    #     ],
+    # },
+    {
+	name => 'first VM start',
+	steps => [
+	    ['qm', 'start', $vmid],
+	],
+    },
+    {
+	name => 'snapshot/rollback',
+	steps => [
+	    ['qm', 'snapshot', $vmid, 'test'],
+	    ['qm', 'rollback', $vmid, 'test'],
+	],
+	cleanup => [
+	    ['qm', 'unlock', $vmid],
+	],
+    },
+    {
+	name => 'remove snapshot',
+	steps => [
+	    ['qm', 'delsnapshot', $vmid, 'test'],
+	],
+    },
+    {
+	name => 'moving disk between namespaces',
+	steps => [
+	    ['qm', 'move_disk', $vmid, 'scsi0', $pool, '--delete', 1],
+	    ['qm', 'move_disk', $vmid, 'scsi0', $storage_name, '--delete', 1],
+	],
+    },
+    {
+	name => 'switch to krbd',
+	preparations => [
+	    ['qm', 'stop', $vmid],
+	    ['pvesm', 'set', $storage_name, '--krbd', 1]
+	],
+    },
+    {
+	name => 'start VM with krbd',
+	steps => [
+	    ['qm', 'start', $vmid],
+	],
+    },
+    {
+	name => 'snapshot/rollback with krbd',
+	steps => [
+	    ['qm', 'snapshot', $vmid, 'test'],
+	    ['qm', 'rollback', $vmid, 'test'],
+	],
+	cleanup => [
+	    ['qm', 'unlock', $vmid],
+	],
+    },
+    {
+	name => 'remove snapshot with krbd',
+	steps => [
+	    ['qm', 'delsnapshot', $vmid, 'test'],
+	],
+    },
+    {
+	name => 'moving disk between namespaces with krbd',
+	steps => [
+	    ['qm', 'move_disk', $vmid, 'scsi0', $pool, '--delete', 1],
+	    ['qm', 'move_disk', $vmid, 'scsi0', $storage_name, '--delete', 1],
+	],
+    },
+    {
+	name => 'clone VM with krbd',
+	steps => [
+	    ['qm', 'clone', $vmid, $vmid_clone],
+	],
+    },
+    {
+	name => 'switch to non krbd',
+	preparations => [
+	    ['qm', 'stop', $vmid],
+	    ['qm', 'stop', $vmid_clone],
+	    ['pvesm', 'set', $storage_name, '--krbd', 0]
+	],
+    },
+    {
+	name => 'templates and linked clone',
+	steps => [
+	    ['qm', 'template', $vmid],
+	    ['qm', 'clone', $vmid, $vmid_linked_clone],
+	    ['qm', 'start', $vmid_linked_clone],
+	    ['qm', 'stop', $vmid_linked_clone],
+	],
+    },
+    {
+	name => 'start linked clone with krbd',
+	preparations => [
+	    ['pvesm', 'set', $storage_name, '--krbd', 1]
+	],
+	steps => [
+	    ['qm', 'start', $vmid_linked_clone],
+	    ['qm', 'stop', $vmid_linked_clone],
+	],
+    },
+];
+
+sub run_prep_cleanup {
+    my ($cmds) = @_;
+
+    for (@$cmds) {
+	print join(' ', @$_). "\n";
+	run_cmd($_);
+    }
+}
+
+sub run_steps {
+    my ($steps) = @_;
+
+    for (@$steps) {
+	ok(run_test_cmd($_), join(' ', @$_));
+    }
+}
+
+sub run_tests {
+    print "Running tests:\n";
+
+    my $num_tests = 0;
+    for (@$tests) {
+	$num_tests += scalar(@{$_->{steps}}) if defined $_->{steps};
+    }
+
+    print("Tests: $num_tests\n");
+    plan tests => $num_tests;
+
+    for my $test (@$tests) {
+	print "Section: $test->{name}\n";
+	run_prep_cleanup($test->{preparations}) if defined $test->{preparations};
+	run_steps($test->{steps}) if defined $test->{steps};
+	run_prep_cleanup($test->{cleanup}) if defined $test->{cleanup};
+    }
+
+    done_testing();
+
+    if (Test::More->builder->is_passing()) {
+	cleanup();
+    }
+}
+
+if ($cleanup) {
+    cleanup();
+} else {
+    prepare();
+    run_tests();
+}
+
-- 
2.20.1

[pve-devel] applied: [PATCH v2 storage 1/3] rbd: centralize rbd path concatenation

[Thomas Lamprecht]

On 07.04.21 16:22, Aaron Lauterer wrote:
> The <pool>/<image> paths are needed in quite a lot of places. Having one
> single place where they are created helps to reduce duplicate code and
> makes it easier to introduce new features.
> 
> The 'add_pool_to_disk' sub was already doing that but the name was not
> really fitting. This commit renames it to the more general
> 'get_rbd_path' and changes the second parameter to the more widely used
> $volume instead of $disk.
> 
> Furthermore, all occurences where "$pool/$volume" has been concatenated
> have been replaced with a call to get_rbd_path.
> 
> Plus some minor code style cleanups for long function calls that were
> touched.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
>  PVE/Storage/RBDPlugin.pm | 46 +++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
>

applied, thanks!

I made the $volume optional too in a follow-up, so that it could be reused
for an additional case.

[pve-devel] applied: [PATCH v2 storage 2/3] rbd: fix #3286 add namespace support

[Thomas Lamprecht]

On 07.04.21 16:22, Aaron Lauterer wrote:
> This patch introduces support for Cephs RBD namespaces.
> 
> A new storage config parameter 'namespace' defines the namespace to be
> used for the RBD storage.
> 
> The namespace must already exist in the Ceph cluster as it is not
> automatically created.
> 
> The main intention is to use this for external Ceph clusters. With
> namespaces, each PVE cluster can get its own namespace and will not
> conflict with other PVE clusters.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> 
> ---
> v1 -> v2:
> use `defined` to check if a namespace if configured instead of
> evaluating the value which would wrongly evaluate to false if the pool
> would be called '0'. Thx @Thomas for pointing this out.
> 
> much less changes since the 'centralize rbd path concatenation' patch
> took care of most instances.
> 
> removed empty new lines that I did not catch in the previous version
> 
> rvc -> v1:
> add --namespace parameter centrally in sub $build_cmd. All commands
> except one (rbd unmap) support it. To handle commands that don't support
> it, a hash with them was introduced.
> 
> In a few places paths (FS, Ceph hierarchy) are needed. These are the
> places scattered throughout the plugin where the namespace is inserted
> if it is configured.
> 
>  PVE/Storage/RBDPlugin.pm | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

applied, but addressed one little issue (see below). thanks!

> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 421539f..02950be 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -25,6 +25,8 @@ my $get_parent_image_name = sub {
>  my $get_rbd_path = sub {
>      my ($scfg, $volume) = @_;
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
> +
> +    return "${pool}/$scfg->{namespace}/${volume}" if defined($scfg->{namespace});
>      return "${pool}/${volume}";
>  };
>  
> @@ -36,6 +38,14 @@ my $build_cmd = sub {
>  
>      my $cmd = [$binary, '-p', $pool];
>  
> +    # some subcommands will fail if the --namespace parameter is present
> +    my $no_namespace_parameter = {
> +	unmap => 1,
> +    };
> +
> +    push @$cmd, '--namespace', $scfg->{namespace}
> +	if ($scfg->{namespace} && !$no_namespace_parameter->{$op});

above check still failed for "falsy" namespace values, I fixed that in a follow-up

> +
>      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});

[pve-devel] applied: [PATCH v2 storage 3/3] rbd: add integration test for namespace handling

[Thomas Lamprecht]

On 07.04.21 16:22, Aaron Lauterer wrote:
> This test is intended to be run on a hyperconverged PVE cluster to test
> the most common operations of VMs using a namespaced Ceph RBD pool.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> v1 -> v2:
> reworked the test from the feedback I got [0].
> 
> * tests are now defined in deeper hashes/arrays and can contain testing
>   steps as well as preparation and cleanup steps where needed.
> * command calls don't use fixed paths
> * command calls use arrays
> * a new Ceph pool will be created and removed at the end by default. Can
>   be overriden with the '--use_existing' parameter. Most likely in
>   combination with the '--pool' parameter.
> * debug flag has been introduced to be used if some debug printing via
>   the `jp` sub is needed. Currenctly not used but might be useful when
>   tinkering with the test in the future. Thx @Thomas for the hint
> 
> 
> [0] https://lists.proxmox.com/pipermail/pve-devel/2021-April/047472.html
> 
>  test/rbd_namespace.pl | 370 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 370 insertions(+)
>  create mode 100755 test/rbd_namespace.pl
> 
>

applied, thanks!

I made three small follow-ups though:

* switched --use_existing to --use-existing
* exit cleanly on --help, its not an error after all!
* allow also the short-opt version for help, -h