[pve-devel] [PATCH qemu-server 5/8] migrate: allow arbitrary source->target storage maps

[Thomas Lamprecht]

On 3/30/20 1:41 PM, Fabian Grünbichler wrote:
> the syntax is backwards compatible, providing a single storage ID or '1'
> works like before. the new helper ensures consistent behaviour at all
> call sites.

looks OK in general, some small things/nits inside (best to read them
bottom up ^^)

> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> 
> Notes:
>     needs a versioned dep on pve-common with the new format and parse_idmap
> 
>  PVE/API2/Qemu.pm   | 41 ++++++++++++++++++++++++++++-------------
>  PVE/QemuMigrate.pm | 24 +++++++++++-------------
>  PVE/QemuServer.pm  | 34 ++++++++++++++++++++++++++++------
>  3 files changed, 67 insertions(+), 32 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 788db93..6eba8d0 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> [ -- 8< -- snip -- 8< --]
> @@ -2067,8 +2063,15 @@ __PACKAGE__->register_method({
>  	my $migration_network = $get_root_param->('migration_network');
>  	my $targetstorage = $get_root_param->('targetstorage');
>  
> -	raise_param_exc({ targetstorage => "targetstorage can only by used with migratedfrom." })
> -	    if $targetstorage && !$migratedfrom;
> +	my $storagemap;
> +
> +	if ($targetstorage) {
> +	    raise_param_exc({ targetstorage => "targetstorage can only by used with migratedfrom." })
> +		if !$migratedfrom;

see below for above also

> +	    $storagemap = eval { PVE::JSONSchema::parse_idmap($targetstorage, 'pve-storage-id') };
> +	    raise_param_exc({ targetstorage => "failed to parse targetstorage map: $@" })
> +		if $@;

see below about duplicated "targetstorage" in raised exception message.
(reviewed this one bottom up, sorry ^^)

> +	}
>  
>  	# read spice ticket from STDIN
>  	my $spice_ticket;

> @@ -3385,9 +3388,7 @@ __PACKAGE__->register_method({
>  		description => "Enable live storage migration for local disk",
>  		optional => 1,
>  	    },
> -            targetstorage => get_standard_option('pve-storage-id', {
> -		description => "Default target storage.",
> -		optional => 1,
> +            targetstorage => get_standard_option('pve-targetstorage', {
>  		completion => \&PVE::QemuServer::complete_migration_storage,

Doing a cool completion handler for this will be fun ;) Actually, it should
be to hard (and for intra cluster fast enough)

>              }),
>  	    bwlimit => {
> @@ -3451,8 +3452,22 @@ __PACKAGE__->register_method({
>  
>  	my $storecfg = PVE::Storage::config();
>  
> -	if( $param->{targetstorage}) {
> -	    PVE::Storage::storage_check_node($storecfg, $param->{targetstorage}, $target);
> +	if (my $targetstorage = $param->{targetstorage}) {
> +	    my $storagemap = eval { PVE::JSONSchema::parse_idmap($targetstorage, 'pve-storage-id') };
> +	    raise_param_exc({ targetstorage => "failed to parse targetstorage map: $@" })

This results in:

> 400 Parameter verification failed.
> targetstorage: failed to parse targetstorage map: $@

You could thus do a s/targetstorage map/storage map/ do avoid to much
redundancy in the raised error message.

> +		if $@;
> +
> +	    foreach my $source (keys %{$storagemap->{entries}}) {
> +		PVE::Storage::storage_check_node($storecfg, $storagemap->{entries}->{$source}, $target);
> +	    }

Was there any reason to not iterate over `values %hash`, like:

foreach my $source (values %{$storagemap->{entries}}) {
    PVE::Storage::storage_check_node($storecfg, $source, $target);
}

for above?

> +
> +	    PVE::Storage::storage_check_node($storecfg, $storagemap->{default}, $target)
> +		if $storagemap->{default};
> +
> +	    PVE::QemuServer::check_storage_availability($storecfg, $conf, $target)
> +		if $storagemap->{identity};
> +
> +	    $param->{storagemap} = $storagemap;
>          } else {
>  	    PVE::QemuServer::check_storage_availability($storecfg, $conf, $target);
>  	}
> [ -- 8< -- snip -- 8< --]> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 1ac8643..cd534f4 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -97,6 +97,28 @@ PVE::JSONSchema::register_standard_option('pve-qemu-machine', {
>  	optional => 1,
>  });
>  
> +
> +sub map_storage {
> +    my ($map, $source) = @_;
> +
> +    return $source if !defined($map);
> +
> +    return $map->{entries}->{$source}
> +	if defined($map->{entries}) && $map->{entries}->{$source};

should/could be:

    if $map->{entries} && exists $map->{entries}->{$source};

> +
> +    return $map->{default} if $map->{default};
> +
> +    # identity (fallback)
> +    return $source;
> +}
> [ -- 8< -- snip -- 8< --]