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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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