[pve-devel] [PATCH qemu-server 1/2] add tests for qemu_img_convert

[Thomas Lamprecht]

On 10/16/19 11:27 AM, Dominik Csapak wrote:
> storage config is motly copied from the config2command tests
> 

some general info about what and *how* you test would be great here.

looks OK in general, and thanks for more test, some comments inline

> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  test/Makefile                      |   5 +-
>  test/run_qemu_img_convert_tests.pl | 186 +++++++++++++++++++++++++++++
>  2 files changed, 190 insertions(+), 1 deletion(-)
>  create mode 100755 test/run_qemu_img_convert_tests.pl
> 
> diff --git a/test/Makefile b/test/Makefile
> index 902d9e3..d88cbd2 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -1,6 +1,6 @@
>  all: test
>  
> -test: test_snapshot test_ovf test_cfg_to_cmd test_pci_addr_conflicts
> +test: test_snapshot test_ovf test_cfg_to_cmd test_pci_addr_conflicts test_qemu_img_convert
>  
>  test_snapshot: run_snapshot_tests.pl
>  	./run_snapshot_tests.pl
> @@ -12,5 +12,8 @@ test_ovf: run_ovf_tests.pl
>  test_cfg_to_cmd: run_config2command_tests.pl cfg2cmd/*.conf
>  	perl -I../ ./run_config2command_tests.pl
>  
> +test_qemu_img_convert: run_qemu_img_convert_tests.pl
> +	perl -I../ ./run_qemu_img_convert_tests.pl
> +
>  test_pci_addr_conflicts: run_pci_addr_checks.pl
>  	./run_pci_addr_checks.pl
> diff --git a/test/run_qemu_img_convert_tests.pl b/test/run_qemu_img_convert_tests.pl
> new file mode 100755
> index 0000000..fafb58a
> --- /dev/null
> +++ b/test/run_qemu_img_convert_tests.pl
> @@ -0,0 +1,186 @@
> +#!/usr/bin/env perl 

besides the trailing whitespace error I'd like to keep this in sync with
the other tests and use "/usr/bin/perl" as #!

> +
> +use strict;
> +use warnings;
> +
> +use lib qw(..);
> +
> +use Test::More;
> +use Test::MockModule;
> +
> +use PVE::QemuServer;
> +
> +my $vmid = 8006;
> +my $storage_config = {
> +    ids => {
> +	local => {
> +	    content => {
> +		images => 1,
> +	    },
> +	    path => "/var/lib/vz",
> +	    type => "dir",
> +	    shared => 0,
> +	},
> +	"rbd-store" => {
> +	    monhost => "127.0.0.42,127.0.0.21,::1",
> +	    content => {
> +		images => 1
> +	    },
> +	    type => "rbd",
> +	    pool => "cpool",
> +	    username => "admin",
> +	    shared => 1
> +	},
> +	"local-lvm" => {
> +	    vgname => "pve",
> +	    bwlimit => "restore=1024",
> +	    type => "lvmthin",
> +	    thinpool => "data",
> +	    content => {
> +		images => 1,
> +	    }
> +	},
> +	"zfs-over-iscsi" => {
> +	    type => "zfs",
> +	    iscsiprovider => "LIO",
> +	    lio_tpg => "tpg1",
> +	    portal => "127.0.0.1",
> +	    target => "iqn.2019-10.org.test:foobar",
> +	    pool => "tank",
> +	}
> +    }
> +};
> +
> +my $tests = {
> +    qcow2raw => [
> +	"local:$vmid/vm-$vmid-disk-0.qcow2", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "qcow2", "-O", "raw",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.qcow2", "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw"
> +	]
> +    ],

can e have an explicit separation of command and expected result already in
the test data structure representation?

something like:

qcow2raw => {
  cmd => [],
  expected => ...,
}

> +    raw2qcow2 => [
> +	"local:$vmid/vm-$vmid-disk-0.raw", "local:$vmid/vm-$vmid-disk-0.qcow2", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "qcow2",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw", "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.qcow2"
> +	]
> +    ],
> +    local2rbd => [
> +	"local:$vmid/vm-$vmid-disk-0.raw", "rbd-store:vm-$vmid-disk-0", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "raw",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw", "rbd:cpool/vm-$vmid-disk-0:mon_host=127.0.0.42;127.0.0.21;[\\:\\:1]:auth_supported=none"
> +	]
> +    ],
> +    rbd2local => [
> +	"rbd-store:vm-$vmid-disk-0", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "raw",
> +	    "rbd:cpool/vm-$vmid-disk-0:mon_host=127.0.0.42;127.0.0.21;[\\:\\:1]:auth_supported=none", "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw"
> +	]
> +    ],
> +    local2zos => [
> +	"local:$vmid/vm-$vmid-disk-0.raw", "zfs-over-iscsi:vm-$vmid-disk-0", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "--target-image-opts",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> +	    "file.driver=iscsi,file.transport=tcp,file.initiator-name=foobar,file.portal=127.0.0.1,file.target=iqn.2019-10.org.test:foobar,file.lun=1,driver=raw"
> +	]
> +    ],
> +    zos2local => [
> +	"zfs-over-iscsi:vm-$vmid-disk-0", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "--image-opts", "-O", "raw",
> +	    "file.driver=iscsi,file.transport=tcp,file.initiator-name=foobar,file.portal=127.0.0.1,file.target=iqn.2019-10.org.test:foobar,file.lun=1,driver=raw",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> +	]
> +    ],
> +    zos2rbd => [
> +	"zfs-over-iscsi:vm-$vmid-disk-0", "rbd-store:vm-$vmid-disk-0", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "--image-opts", "-O", "raw",
> +	    "file.driver=iscsi,file.transport=tcp,file.initiator-name=foobar,file.portal=127.0.0.1,file.target=iqn.2019-10.org.test:foobar,file.lun=1,driver=raw",
> +	    "rbd:cpool/vm-$vmid-disk-0:mon_host=127.0.0.42;127.0.0.21;[\\:\\:1]:auth_supported=none"
> +	]
> +    ],
> +    rbd2zos => [
> +	"rbd-store:vm-$vmid-disk-0", "zfs-over-iscsi:vm-$vmid-disk-0", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "--target-image-opts",
> +	    "rbd:cpool/vm-$vmid-disk-0:mon_host=127.0.0.42;127.0.0.21;[\\:\\:1]:auth_supported=none",
> +	    "file.driver=iscsi,file.transport=tcp,file.initiator-name=foobar,file.portal=127.0.0.1,file.target=iqn.2019-10.org.test:foobar,file.lun=1,driver=raw",
> +	]
> +    ],
> +    local2lvmthin => [
> +	"local:$vmid/vm-$vmid-disk-0.raw", "local-lvm:vm-$vmid-disk-0", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "raw",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> +	    "/dev/pve/vm-$vmid-disk-0",
> +	]
> +    ],
> +    lvmthin2local => [
> +	"local-lvm:vm-$vmid-disk-0", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 0, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "raw",
> +	    "/dev/pve/vm-$vmid-disk-0",
> +	    "/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> +	]
> +    ],
> +    zeroinit => [
> +	"local-lvm:vm-$vmid-disk-0", "local:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 1, [
> +	    "/usr/bin/qemu-img", "convert", "-p", "-n", "-f", "raw", "-O", "raw",
> +	    "/dev/pve/vm-$vmid-disk-0",
> +	    "zeroinit:/var/lib/vz/images/$vmid/vm-$vmid-disk-0.raw",
> +	]
> +    ],
> +    notexistingstorage => [
> +	"local-lvm:vm-$vmid-disk-0", "not-existing:$vmid/vm-$vmid-disk-0.raw", 1024*10, undef, 1,
> +	"storage 'not-existing' does not exists\n",
> +    ],
> +};
> +
> +my $command;
> +
> +my $storage_module = Test::MockModule->new("PVE::Storage");
> +$storage_module->mock(
> +    config => sub {
> +	return $storage_config;
> +    },
> +    activate_volumes => sub {
> +	return 1;
> +    }
> +);
> +
> +my $lio_module = Test::MockModule->new("PVE::Storage::LunCmd::LIO");
> +$lio_module->mock(
> +    run_lun_command => sub {
> +	return 1;
> +    }
> +);
> +
> +# we use the exported run_command so we have to mock it there
> +my $zfsplugin_module = Test::MockModule->new("PVE::Storage::ZFSPlugin");
> +$zfsplugin_module->mock(
> +    run_command => sub {
> +	return 1;
> +    }
> +);
> +
> +# we use the exported run_command so we have to mock it there
> +my $qemu_server_module = Test::MockModule->new("PVE::QemuServer");
> +$qemu_server_module->mock(
> +    run_command => sub {
> +	$command = shift;
> +    },
> +    get_initiator_name => sub {
> +	return "foobar";
> +    }
> +);
> +
> +foreach my $name (sort keys %$tests) {
> +    my $res = $tests->{$name}->[-1]; # last element

/if/ we would have res and cmd mixed, then I'd at least do 
my $res = pop @{$tests->{$name}};

so that it does not gets expanded in the qemu_img_convert call below..

> +    eval { PVE::QemuServer::qemu_img_convert(@{$tests->{$name}}) };
> +    if (my $err = $@) {
> +	is ($err, $res, $name);
> +    } elsif (defined($command)) {
> +	is_deeply($command, $res, $name);
> +	$command = undef;
> +    } else {
> +	fail($name);
> +	note("no command")
> +    }
> +}
> +
> +done_testing();
>