From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Aaron Lauterer <a.lauterer@proxmox.com>
Subject: Re: [pve-devel] [PATCH storage 2/2] rbd: add integration test for namespace handling
Date: Thu, 1 Apr 2021 16:34:47 +0200 [thread overview]
Message-ID: <f4a3082d-d872-d0d2-6c7b-f2a9329ee80d@proxmox.com> (raw)
In-Reply-To: <20210401134057.26305-2-a.lauterer@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();
> +}
> +
>
next prev parent reply other threads:[~2021-04-01 14:35 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-01 13:40 [pve-devel] [PATCH storage 1/2] rbd: fix #3286 add namespace support Aaron Lauterer
2021-04-01 13:40 ` [pve-devel] [PATCH storage 2/2] rbd: add integration test for namespace handling Aaron Lauterer
2021-04-01 14:34 ` Thomas Lamprecht [this message]
2021-04-01 14:48 ` [pve-devel] [PATCH storage 1/2] rbd: fix #3286 add namespace support Thomas Lamprecht
2021-04-02 12:20 ` Aaron Lauterer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f4a3082d-d872-d0d2-6c7b-f2a9329ee80d@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=a.lauterer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox