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

[Fabian Grünbichler]

On April 2, 2020 6:34 pm, Thomas Lamprecht wrote:
> 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)

yes, for intra cluster it shouldn't be much trouble. I think completion 
for remote cluster will be ENOTIMPLEMENTED here, that one needs a proper 
multi-cluster client that then re-uses this API call ;)

>>              }),
>>  	    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.

yes, that makes sense :)

> 
>> +		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?

none other than habit :)

> 
>> +
>> +	    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< --]
> 
> 
>