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

[pve-devel] [PATCH storage 1/2] 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>

remove just style changes
---
Changes from RFC:

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 | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index fab6d57..2d78f40 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -27,7 +27,9 @@ my $add_pool_to_disk = sub {
 
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
 
-    return "$pool/$disk";
+    my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : "";
+
+    return "${pool}${namespace}/${disk}";
 };
 
 my $build_cmd = sub {
@@ -38,6 +40,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});
@@ -154,6 +164,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 $scfg->{namespace};
 
     my $raw = '';
     my $parser = sub { $raw .= shift };
@@ -281,6 +292,10 @@ sub properties {
 	    description => "Pool.",
 	    type => 'string',
 	},
+	namespace=> {
+	    description => "RBD Namespace.",
+	    type => 'string',
+	},
 	username => {
 	    description => "RBD Id.",
 	    type => 'string',
@@ -302,6 +317,7 @@ sub options {
 	disable => { optional => 1 },
 	monhost => { optional => 1},
 	pool => { optional => 1 },
+	namespace => { optional => 1 },
 	username => { optional => 1 },
 	content => { optional => 1 },
 	krbd => { optional => 1 },
@@ -349,9 +365,10 @@ sub path {
     $name .= '@'.$snapname if $snapname;
 
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
-    return ("/dev/rbd/$pool/$name", $vmid, $vtype) if $scfg->{krbd};
+    my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : "";
+    return ("/dev/rbd/${pool}${namespace}/${name}", $vmid, $vtype) if $scfg->{krbd};
 
-    my $path = "rbd:$pool/$name";
+    my $path = "rbd:${pool}${namespace}/${name}";
 
     $path .= ":conf=$cmd_option->{ceph_conf}" if $cmd_option->{ceph_conf};
     if (defined($scfg->{monhost})) {
@@ -370,6 +387,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 {
@@ -487,6 +505,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}) {
@@ -511,6 +530,7 @@ sub list_images {
 
     $cache->{rbd} = rbd_ls($scfg, $storeid) if !$cache->{rbd};
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
+    $pool .= "/$scfg->{namespace}" if $scfg->{namespace};
 
     my $res = [];
 
@@ -575,7 +595,9 @@ sub deactivate_storage {
 }
 
 my $get_kernel_device_name = sub {
-    my ($pool, $name) = @_;
+    my ($pool, $name, $namespace) = @_;
+
+    return "/dev/rbd/${pool}/${namespace}/${name}" if $namespace;
 
     return "/dev/rbd/$pool/$name";
 };
@@ -590,7 +612,7 @@ sub map_volume {
 
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
 
-    my $kerneldev = $get_kernel_device_name->($pool, $name);
+    my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace});
 
     return $kerneldev if -b $kerneldev; # already mapped
 
@@ -611,7 +633,7 @@ sub unmap_volume {
 
     my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
 
-    my $kerneldev = $get_kernel_device_name->($pool, $name);
+    my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace});
 
     if (-b $kerneldev) {
 	my $cmd = &$rbd_cmd($scfg, $storeid, 'unmap', $kerneldev);
-- 
2.20.1

[pve-devel] [PATCH storage 2/2] 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>
---

Could probably be done nicer here and there but it worked (except for
the sometimes racy rollback ...) and should help to test if anything
broke in newer Ceph versions.

 test/rbd_namespace.pl | 226 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 226 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..b8a5532
--- /dev/null
+++ b/test/rbd_namespace.pl
@@ -0,0 +1,226 @@
+#!/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 PVE::Tools qw(run_command);
+use Data::Dumper;
+use JSON;
+
+my $pool = "rbd";
+my $namespace = "testspace";
+my $showhelp = '';
+my $vmid = 999999;
+my $cleanup = undef;
+
+my $helpstring = "To override default values, set them as named parameters:
+
+--pool		pool name, default: ${pool}
+--namespace	rbd namespace, default: ${namespace}
+--vmid		VMID of the test VM, default: ${vmid}
+--cleanup	Remove the storage definitions, namespaces and VMs\n";
+
+GetOptions (
+	"pool=s" => \$pool,
+	"namespace=s" => \$namespace,
+	"vmid=i" => \$vmid,
+	"help" => \$showhelp,
+	"cleanup" => \$cleanup,
+) or die ($helpstring);
+
+die $helpstring if $showhelp;
+
+my $storage_name_rbd = "${pool}-${namespace}-rbd";
+
+my $vmid_clone = int($vmid) - 1;
+my $vmid_linked_clone = int($vmid) - 2;
+
+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) = @_;
+    system($cmd);
+    my $err = $? >> 8;
+
+    return 1 if $err == 0;
+    return 0;
+}
+
+sub prepare {
+    my $pools = run_cmd("/usr/bin/ceph osd pool ls --format json", 1);
+
+    my %poolnames = map {$_ => 1} @$pools;
+    die "Pool '$pool' does not exist!\n" if !exists($poolnames{$pool});
+
+    my $namespaces = run_cmd("/usr/bin/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("/usr/bin/rbd namespace create ${pool}/${namespace}");
+    }
+
+    my $storages = run_cmd("/usr/bin/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_rbd;
+
+	if ($rbd_found) {
+	    run_cmd("/usr/sbin/pvesm set ${storage_name_rbd} --krbd 0");
+	    die "Enable the storage '$stor->{storage}'!" if $stor->{disable};
+	}
+    }
+    die "No storage for pool '${pool}' found! Must have same name as pool!\n"
+	if !$pool_found;
+    # create PVE storages (librbd / krbd)
+    run_cmd("/usr/sbin/pvesm add rbd ${storage_name_rbd} --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("/usr/bin/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("/usr/sbin/qm stop ${vmid}", 0, 1);
+	    run_cmd("/usr/sbin/qm destroy ${vmid}");
+	}
+    }
+    run_cmd("/usr/sbin/qm create ${vmid} --bios ovmf --efidisk0 ${storage_name_rbd}:1 --scsi0 ${storage_name_rbd}:2");
+}
+
+
+sub cleanup {
+    print "Cleaning up test environment!\n";
+    print "Removing VMs\n";
+    run_cmd("/usr/sbin/qm stop ${vmid}", 0, 1);
+    run_cmd("/usr/sbin/qm stop ${vmid_linked_clone}", 0, 1);
+    run_cmd("/usr/sbin/qm stop ${vmid_clone}", 0, 1);
+    run_cmd("/usr/sbin/qm destroy ${vmid_linked_clone}", 0, 1);
+    run_cmd("/usr/sbin/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("/usr/sbin/qm unlock ${vmid}", 0, 1);
+    run_cmd("/usr/sbin/qm destroy ${vmid}", 0, 1);
+
+    print "Removing Storage definition for ${storage_name_rbd}\n";
+    run_cmd("/usr/sbin/pvesm remove ${storage_name_rbd}", 0, 1);
+
+    print "Removing RBD namespace '${pool}/${namespace}'\n";
+    run_cmd("/usr/bin/rbd namespace remove ${pool}/${namespace}", 0, 1);
+}
+
+sub run_tests {
+    my @tests = (
+	{name => "Start VM", cmd => "/usr/sbin/qm start ${vmid}"},
+	{name => "Snapshot VM", cmd => "/usr/sbin/qm snapshot ${vmid} test"},
+	{name => "Rollback VM to snapshot", cmd => "/usr/sbin/qm rollback ${vmid} test"},
+	{name => "Remove lock", cmd => "/usr/sbin/qm unlock ${vmid}", maintenance => 1},
+	{name => "Remove VM Snapshot", cmd => "/usr/sbin/qm delsnapshot ${vmid} test"},
+	{name => "Move Disk to pool w/o namespace", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${pool} --delete 1"},
+	{name => "Move Disk to pool w/ namespace", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${storage_name_rbd} --delete 1"},
+	{name => "Stop VM", cmd =>"/usr/sbin/qm stop ${vmid}", maintenance => 1},
+	{name => "Change to krbd", cmd =>"/usr/sbin/pvesm set ${storage_name_rbd} --krbd 1", maintenance => 1},
+	{name => "Start VM w/ krbd", cmd => "/usr/sbin/qm start ${vmid}"},
+	{name => "Move Disk to pool w/o namespace w/ krbd", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${pool} --delete 1"},
+	{name => "Move Disk to pool w/ namespace w/ krbd", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${storage_name_rbd} --delete 1"},
+	{name => "Snapshot VM w/ krbd", cmd => "/usr/sbin/qm snapshot ${vmid} test"},
+	{name => "Rollback VM to snapshot w/ krbd", cmd => "/usr/sbin/qm rollback ${vmid} test"},
+	{name => "Remove VM Snapshot w/ krbd", cmd => "/usr/sbin/qm delsnapshot ${vmid} test"},
+	{name => "Clone VM w/ krbd", cmd => "/usr/sbin/qm clone ${vmid} ${vmid_clone}"},
+	{name => "Stop VM", cmd =>"/usr/sbin/qm stop ${vmid}", maintenace => 1},
+	{name => "Change to non krbd", cmd =>"/usr/sbin/pvesm set ${storage_name_rbd} --krbd 0", maintenance => 1},
+	{name => "Convert to template", cmd =>"/usr/sbin/qm template ${vmid}"},
+	{name => "Create linked clone", cmd =>"/usr/sbin/qm clone ${vmid} ${vmid_linked_clone}"},
+	{name => "Start linked clone", cmd =>"/usr/sbin/qm start ${vmid_linked_clone}"},
+	{name => "Stop linked clone", cmd =>"/usr/sbin/qm stop ${vmid_linked_clone}"},
+	{name => "Change to krbd", cmd =>"/usr/sbin/pvesm set ${storage_name_rbd} --krbd 1", maintenance => 1},
+	{name => "Start linked clone w/ krbd", cmd =>"/usr/sbin/qm start ${vmid_linked_clone}"},
+	{name => "Stop linked clone w/ krbd", cmd =>"/usr/sbin/qm stop ${vmid_linked_clone}"},
+    );
+    print "Running tests:\n";
+
+    my $num_tests = 0;
+    for (@tests) {
+	$num_tests +=1 if !defined $_->{maintenance};
+    }
+
+    plan tests => $num_tests;
+
+    for my $test (@tests) {
+	print "$test->{name}\n";
+	if ($test->{maintenance}) {
+	    run_cmd($test->{cmd});
+	} else {
+	    ok(run_test_cmd($test->{cmd}), $test->{name});
+	}
+    }
+
+    done_testing();
+
+    if (Test::More->builder->is_passing()) {
+	cleanup();
+    }
+}
+
+if ($cleanup) {
+    cleanup();
+} else {
+    prepare();
+    run_tests();
+}
+
-- 
2.20.1

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

[Thomas Lamprecht]

On 01.04.21 15:40, 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>
> ---
> 
> Could probably be done nicer here and there but it worked (except for
> the sometimes racy rollback ...) and should help to test if anything
> broke in newer Ceph versions.

in general surely nice to have and thanks for tackling this, some comments
inline

> 
>  test/rbd_namespace.pl | 226 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 226 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..b8a5532
> --- /dev/null
> +++ b/test/rbd_namespace.pl
> @@ -0,0 +1,226 @@
> +#!/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 PVE::Tools qw(run_command);

nit: group use staements by perl-modules and proxmox-modules, so above should be below

> +use Data::Dumper;

avoid Dumper, it's a hog and actually not required if one has ...

> +use JSON;

.. json, I often just add a json print (jp) helper with wraps to_json with the
correct settings. Also it's nicer to just have a global $DEBUG flag to switch 
such prints on/off centraly.

E.g.:

sub jp {
    return if !$DEBUG;
    print to_json($_[0], { utf8 => 1, pretty => 1, canonical => 1} ) ."\n";
}


> +
> +my $pool = "rbd";
> +my $namespace = "testspace";
> +my $showhelp = '';
> +my $vmid = 999999;
> +my $cleanup = undef;
> +
> +my $helpstring = "To override default values, set them as named parameters:
> +
> +--pool		pool name, default: ${pool}
> +--namespace	rbd namespace, default: ${namespace}
> +--vmid		VMID of the test VM, default: ${vmid}
> +--cleanup	Remove the storage definitions, namespaces and VMs\n";
> +
> +GetOptions (
> +	"pool=s" => \$pool,
> +	"namespace=s" => \$namespace,
> +	"vmid=i" => \$vmid,
> +	"help" => \$showhelp,
> +	"cleanup" => \$cleanup,
> +) or die ($helpstring);
> +
> +die $helpstring if $showhelp;
> +
> +my $storage_name_rbd = "${pool}-${namespace}-rbd";
> +
> +my $vmid_clone = int($vmid) - 1;
> +my $vmid_linked_clone = int($vmid) - 2;
> +
> +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) = @_;
> +    system($cmd);
> +    my $err = $? >> 8;
> +
> +    return 1 if $err == 0;
> +    return 0;
> +}
> +
> +sub prepare {
> +    my $pools = run_cmd("/usr/bin/ceph osd pool ls --format json", 1);
> +
> +    my %poolnames = map {$_ => 1} @$pools;
> +    die "Pool '$pool' does not exist!\n" if !exists($poolnames{$pool});

why not `pveceph pool create <id>` ?
Actually I'd expect that the default is some random name, gets created here and
cleaned up at the end of this script (at least when we created it)

> +
> +    my $namespaces = run_cmd("/usr/bin/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("/usr/bin/rbd namespace create ${pool}/${namespace}");
> +    }
> +
> +    my $storages = run_cmd("/usr/bin/pvesh get storage --output-format json", 1);

avoid fixed paths for stuff in /(usr/)?s?bin
Also, please use arrays for commands also in test scripts - I do not want any (potential
later added) $param to shell inject anything in a test either.

> +    #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_rbd;
> +
> +	if ($rbd_found) {
> +	    run_cmd("/usr/sbin/pvesm set ${storage_name_rbd} --krbd 0");
> +	    die "Enable the storage '$stor->{storage}'!" if $stor->{disable};
> +	}
> +    }
> +    die "No storage for pool '${pool}' found! Must have same name as pool!\n"

same here, auto-create (and destory) it in that case? Then this could be more easily
run in a (planned) buildbot-triggered cluster test run.

> +	if !$pool_found;
> +    # create PVE storages (librbd / krbd)
> +    run_cmd("/usr/sbin/pvesm add rbd ${storage_name_rbd} --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("/usr/bin/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("/usr/sbin/qm stop ${vmid}", 0, 1);
> +	    run_cmd("/usr/sbin/qm destroy ${vmid}");
> +	}
> +    }
> +    run_cmd("/usr/sbin/qm create ${vmid} --bios ovmf --efidisk0 ${storage_name_rbd}:1 --scsi0 ${storage_name_rbd}:2");
> +}
> +
> +
> +sub cleanup {
> +    print "Cleaning up test environment!\n";
> +    print "Removing VMs\n";
> +    run_cmd("/usr/sbin/qm stop ${vmid}", 0, 1);

same again for above and below run_cmd, arrays and no absolute path please.

> +    run_cmd("/usr/sbin/qm stop ${vmid_linked_clone}", 0, 1);
> +    run_cmd("/usr/sbin/qm stop ${vmid_clone}", 0, 1);
> +    run_cmd("/usr/sbin/qm destroy ${vmid_linked_clone}", 0, 1);
> +    run_cmd("/usr/sbin/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("/usr/sbin/qm unlock ${vmid}", 0, 1);
> +    run_cmd("/usr/sbin/qm destroy ${vmid}", 0, 1);
> +
> +    print "Removing Storage definition for ${storage_name_rbd}\n";
> +    run_cmd("/usr/sbin/pvesm remove ${storage_name_rbd}", 0, 1);
> +
> +    print "Removing RBD namespace '${pool}/${namespace}'\n";
> +    run_cmd("/usr/bin/rbd namespace remove ${pool}/${namespace}", 0, 1);
> +}
> +
> +sub run_tests {
> +    my @tests = (
> +	{name => "Start VM", cmd => "/usr/sbin/qm start ${vmid}"},

Please define the tests out of the sub and use a less flat notation.

As those are quite order dependent I would call the strucure also "steps"?

Or have two-levels, I'd actually omit the single command names and log the
executed command in run_cmd, it actually has the info encoded in the name
already.

my $tests = [
   {
        name => "snapshot/rollback",
        steps => [
            ['qm', 'snapshot', $vmid, 'test_snap' ],
            ['qm', 'rollback', $vmid 'test_snap' ],
        ],
        cleanup => [ 'qm', 'unlock', $vmid ], # that whats now marked as "maintenance"
   },
   # ...
];


> +	{name => "Snapshot VM", cmd => "/usr/sbin/qm snapshot ${vmid} test"},
> +	{name => "Rollback VM to snapshot", cmd => "/usr/sbin/qm rollback ${vmid} test"},
> +	{name => "Remove lock", cmd => "/usr/sbin/qm unlock ${vmid}", maintenance => 1},
> +	{name => "Remove VM Snapshot", cmd => "/usr/sbin/qm delsnapshot ${vmid} test"},
> +	{name => "Move Disk to pool w/o namespace", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${pool} --delete 1"},
> +	{name => "Move Disk to pool w/ namespace", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${storage_name_rbd} --delete 1"},
> +	{name => "Stop VM", cmd =>"/usr/sbin/qm stop ${vmid}", maintenance => 1},
> +	{name => "Change to krbd", cmd =>"/usr/sbin/pvesm set ${storage_name_rbd} --krbd 1", maintenance => 1},
> +	{name => "Start VM w/ krbd", cmd => "/usr/sbin/qm start ${vmid}"},
> +	{name => "Move Disk to pool w/o namespace w/ krbd", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${pool} --delete 1"},
> +	{name => "Move Disk to pool w/ namespace w/ krbd", cmd =>"/usr/sbin/qm move_disk ${vmid} scsi0 ${storage_name_rbd} --delete 1"},
> +	{name => "Snapshot VM w/ krbd", cmd => "/usr/sbin/qm snapshot ${vmid} test"},
> +	{name => "Rollback VM to snapshot w/ krbd", cmd => "/usr/sbin/qm rollback ${vmid} test"},
> +	{name => "Remove VM Snapshot w/ krbd", cmd => "/usr/sbin/qm delsnapshot ${vmid} test"},
> +	{name => "Clone VM w/ krbd", cmd => "/usr/sbin/qm clone ${vmid} ${vmid_clone}"},
> +	{name => "Stop VM", cmd =>"/usr/sbin/qm stop ${vmid}", maintenace => 1},
> +	{name => "Change to non krbd", cmd =>"/usr/sbin/pvesm set ${storage_name_rbd} --krbd 0", maintenance => 1},
> +	{name => "Convert to template", cmd =>"/usr/sbin/qm template ${vmid}"},
> +	{name => "Create linked clone", cmd =>"/usr/sbin/qm clone ${vmid} ${vmid_linked_clone}"},
> +	{name => "Start linked clone", cmd =>"/usr/sbin/qm start ${vmid_linked_clone}"},
> +	{name => "Stop linked clone", cmd =>"/usr/sbin/qm stop ${vmid_linked_clone}"},
> +	{name => "Change to krbd", cmd =>"/usr/sbin/pvesm set ${storage_name_rbd} --krbd 1", maintenance => 1},
> +	{name => "Start linked clone w/ krbd", cmd =>"/usr/sbin/qm start ${vmid_linked_clone}"},
> +	{name => "Stop linked clone w/ krbd", cmd =>"/usr/sbin/qm stop ${vmid_linked_clone}"},
> +    );
> +    print "Running tests:\n";
> +
> +    my $num_tests = 0;
> +    for (@tests) {
> +	$num_tests +=1 if !defined $_->{maintenance};
> +    }
> +
> +    plan tests => $num_tests;
> +
> +    for my $test (@tests) {
> +	print "$test->{name}\n";
> +	if ($test->{maintenance}) {
> +	    run_cmd($test->{cmd});
> +	} else {
> +	    ok(run_test_cmd($test->{cmd}), $test->{name});
> +	}
> +    }
> +
> +    done_testing();
> +
> +    if (Test::More->builder->is_passing()) {
> +	cleanup();
> +    }
> +}
> +
> +if ($cleanup) {
> +    cleanup();
> +} else {
> +    prepare();
> +    run_tests();
> +}
> +
> 

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

[Thomas Lamprecht]

On 01.04.21 15:40, 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>
> 
> remove just style changes
> ---
> Changes from RFC:
> 
> 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 | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index fab6d57..2d78f40 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -27,7 +27,9 @@ my $add_pool_to_disk = sub {
>  
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  
> -    return "$pool/$disk";
> +    my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : "";

can I have "0" as namespace? as then above check may break. Maybe do:

my $namespace = '/' . ($scfg->{namespace} // '');

return "${pool}${namespace}${disk}";



> +
> +    return "${pool}${namespace}/${disk}";
>  };
>  
>  my $build_cmd = sub {
> @@ -38,6 +40,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});
> @@ -154,6 +164,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 $scfg->{namespace};
>  
>      my $raw = '';
>      my $parser = sub { $raw .= shift };
> @@ -281,6 +292,10 @@ sub properties {
>  	    description => "Pool.",
>  	    type => 'string',
>  	},
> +	namespace=> {
> +	    description => "RBD Namespace.",
> +	    type => 'string',
> +	},
>  	username => {
>  	    description => "RBD Id.",
>  	    type => 'string',
> @@ -302,6 +317,7 @@ sub options {
>  	disable => { optional => 1 },
>  	monhost => { optional => 1},
>  	pool => { optional => 1 },
> +	namespace => { optional => 1 },
>  	username => { optional => 1 },
>  	content => { optional => 1 },
>  	krbd => { optional => 1 },
> @@ -349,9 +365,10 @@ sub path {
>      $name .= '@'.$snapname if $snapname;
>  
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
> -    return ("/dev/rbd/$pool/$name", $vmid, $vtype) if $scfg->{krbd};
> +    my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : "";

same as above, a for perl falsy $ns value "breaks" this.

maybe we could use a `get_pool_ns_path($scfg)` (possible better name possible,
should be relatively short and not too misguiding) sub returning both of above? 

Or actually: maybe even a `get_rbd_path($scfg, $image)` returning
"${pool}${namespace}${image}"

(with $ns omitted/added depending on config)

There's seems to be more use for that one when looking at the changes made here.

> +    return ("/dev/rbd/${pool}${namespace}/${name}", $vmid, $vtype) if $scfg->{krbd};
>  
> -    my $path = "rbd:$pool/$name";
> +    my $path = "rbd:${pool}${namespace}/${name}";
>  
>      $path .= ":conf=$cmd_option->{ceph_conf}" if $cmd_option->{ceph_conf};
>      if (defined($scfg->{monhost})) {
> @@ -370,6 +387,7 @@ sub find_free_diskname {
>      my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
>  
>      my $cmd = &$rbd_cmd($scfg, $storeid, 'ls');
> +

unrelated new line added? (albeit this one may improve code readability, so fine for me)

>      my $disk_list = [];
>  
>      my $parser = sub {
> @@ -487,6 +505,7 @@ sub free_image {
>      my ($vtype, $name, $vmid, undef, undef, undef) =
>  	$class->parse_volname($volname);
>  
> +

bogus new lien added?

>      my $snaps = rbd_ls_snap($scfg, $storeid, $name);
>      foreach my $snap (keys %$snaps) {
>  	if ($snaps->{$snap}->{protected}) {
> @@ -511,6 +530,7 @@ sub list_images {
>  
>      $cache->{rbd} = rbd_ls($scfg, $storeid) if !$cache->{rbd};
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
> +    $pool .= "/$scfg->{namespace}" if $scfg->{namespace};
>  
>      my $res = [];
>  
> @@ -575,7 +595,9 @@ sub deactivate_storage {
>  }
>  
>  my $get_kernel_device_name = sub {
> -    my ($pool, $name) = @_;
> +    my ($pool, $name, $namespace) = @_;
> +
> +    return "/dev/rbd/${pool}/${namespace}/${name}" if $namespace;
>  
>      return "/dev/rbd/$pool/$name";
>  };
> @@ -590,7 +612,7 @@ sub map_volume {
>  
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  
> -    my $kerneldev = $get_kernel_device_name->($pool, $name);
> +    my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace});
>  
>      return $kerneldev if -b $kerneldev; # already mapped
>  
> @@ -611,7 +633,7 @@ sub unmap_volume {
>  
>      my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>  
> -    my $kerneldev = $get_kernel_device_name->($pool, $name);
> +    my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace});
>  
>      if (-b $kerneldev) {
>  	my $cmd = &$rbd_cmd($scfg, $storeid, 'unmap', $kerneldev);
> 

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

[Aaron Lauterer]

Thanks for the review. I took another look at the plugin and I believe that a `get_rbd_path` sub can be use in multiple places, even the `get_kernel_device` sub could use it AFAICS. I'll send in a v2 where I will introduce that, and then add the NS functionality.

On 4/1/21 4:48 PM, Thomas Lamprecht wrote:
> On 01.04.21 15:40, 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>
>>
>> remove just style changes
>> ---
>> Changes from RFC:
>>
>> 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 | 34 ++++++++++++++++++++++++++++------
>>   1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
>> index fab6d57..2d78f40 100644
>> --- a/PVE/Storage/RBDPlugin.pm
>> +++ b/PVE/Storage/RBDPlugin.pm
>> @@ -27,7 +27,9 @@ my $add_pool_to_disk = sub {
>>   
>>       my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>>   
>> -    return "$pool/$disk";
>> +    my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : "";
> 
> can I have "0" as namespace? as then above check may break. Maybe do:
> 
> my $namespace = '/' . ($scfg->{namespace} // '');
> 
> return "${pool}${namespace}${disk}";
> 
> 
> 
>> +
>> +    return "${pool}${namespace}/${disk}";
>>   };
>>   
>>   my $build_cmd = sub {
>> @@ -38,6 +40,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});
>> @@ -154,6 +164,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 $scfg->{namespace};
>>   
>>       my $raw = '';
>>       my $parser = sub { $raw .= shift };
>> @@ -281,6 +292,10 @@ sub properties {
>>   	    description => "Pool.",
>>   	    type => 'string',
>>   	},
>> +	namespace=> {
>> +	    description => "RBD Namespace.",
>> +	    type => 'string',
>> +	},
>>   	username => {
>>   	    description => "RBD Id.",
>>   	    type => 'string',
>> @@ -302,6 +317,7 @@ sub options {
>>   	disable => { optional => 1 },
>>   	monhost => { optional => 1},
>>   	pool => { optional => 1 },
>> +	namespace => { optional => 1 },
>>   	username => { optional => 1 },
>>   	content => { optional => 1 },
>>   	krbd => { optional => 1 },
>> @@ -349,9 +365,10 @@ sub path {
>>       $name .= '@'.$snapname if $snapname;
>>   
>>       my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>> -    return ("/dev/rbd/$pool/$name", $vmid, $vtype) if $scfg->{krbd};
>> +    my $namespace = $scfg->{namespace} ? "/$scfg->{namespace}" : "";
> 
> same as above, a for perl falsy $ns value "breaks" this.
> 
> maybe we could use a `get_pool_ns_path($scfg)` (possible better name possible,
> should be relatively short and not too misguiding) sub returning both of above?
> 
> Or actually: maybe even a `get_rbd_path($scfg, $image)` returning
> "${pool}${namespace}${image}"
> 
> (with $ns omitted/added depending on config)
> 
> There's seems to be more use for that one when looking at the changes made here.
> 
>> +    return ("/dev/rbd/${pool}${namespace}/${name}", $vmid, $vtype) if $scfg->{krbd};
>>   
>> -    my $path = "rbd:$pool/$name";
>> +    my $path = "rbd:${pool}${namespace}/${name}";
>>   
>>       $path .= ":conf=$cmd_option->{ceph_conf}" if $cmd_option->{ceph_conf};
>>       if (defined($scfg->{monhost})) {
>> @@ -370,6 +387,7 @@ sub find_free_diskname {
>>       my ($class, $storeid, $scfg, $vmid, $fmt, $add_fmt_suffix) = @_;
>>   
>>       my $cmd = &$rbd_cmd($scfg, $storeid, 'ls');
>> +
> 
> unrelated new line added? (albeit this one may improve code readability, so fine for me)
> 
>>       my $disk_list = [];
>>   
>>       my $parser = sub {
>> @@ -487,6 +505,7 @@ sub free_image {
>>       my ($vtype, $name, $vmid, undef, undef, undef) =
>>   	$class->parse_volname($volname);
>>   
>> +
> 
> bogus new lien added?
> 
>>       my $snaps = rbd_ls_snap($scfg, $storeid, $name);
>>       foreach my $snap (keys %$snaps) {
>>   	if ($snaps->{$snap}->{protected}) {
>> @@ -511,6 +530,7 @@ sub list_images {
>>   
>>       $cache->{rbd} = rbd_ls($scfg, $storeid) if !$cache->{rbd};
>>       my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>> +    $pool .= "/$scfg->{namespace}" if $scfg->{namespace};
>>   
>>       my $res = [];
>>   
>> @@ -575,7 +595,9 @@ sub deactivate_storage {
>>   }
>>   
>>   my $get_kernel_device_name = sub {
>> -    my ($pool, $name) = @_;
>> +    my ($pool, $name, $namespace) = @_;
>> +
>> +    return "/dev/rbd/${pool}/${namespace}/${name}" if $namespace;
>>   
>>       return "/dev/rbd/$pool/$name";
>>   };
>> @@ -590,7 +612,7 @@ sub map_volume {
>>   
>>       my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>>   
>> -    my $kerneldev = $get_kernel_device_name->($pool, $name);
>> +    my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace});
>>   
>>       return $kerneldev if -b $kerneldev; # already mapped
>>   
>> @@ -611,7 +633,7 @@ sub unmap_volume {
>>   
>>       my $pool =  $scfg->{pool} ? $scfg->{pool} : 'rbd';
>>   
>> -    my $kerneldev = $get_kernel_device_name->($pool, $name);
>> +    my $kerneldev = $get_kernel_device_name->($pool, $name, $scfg->{namespace});
>>   
>>       if (-b $kerneldev) {
>>   	my $cmd = &$rbd_cmd($scfg, $storeid, 'unmap', $kerneldev);
>>
>