Re: [pve-devel] [RFC storage 2/7] add disk rename feature

[Aaron Lauterer <a.lauterer@proxmox.com>]

Thanks for the review :)

On 6/2/21 10:36 AM, Fabian Ebner wrote:
> Am 01.06.21 um 18:10 schrieb Aaron Lauterer:
>> Functionality has been added for the following storage types:
>>
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index 93d09ce..6936abd 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -40,11 +40,11 @@ use PVE::Storage::ZFSPlugin;
>>   use PVE::Storage::PBSPlugin;
>>   # Storage API version. Increment it on changes in storage API interface.
>> -use constant APIVER => 8;
>> +use constant APIVER => 9;
>>   # Age is the number of versions we're backward compatible with.
>>   # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>>   # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
>> -use constant APIAGE => 7;
>> +use constant APIAGE => 8;
>>   # load standard plugins
>>   PVE::Storage::DirPlugin->register();
>> @@ -355,6 +355,7 @@ sub volume_snapshot_needs_fsfreeze {
>>   #            snapshot - taking a snapshot is possible
>>   #            sparseinit - volume is sparsely initialized
>>   #            template - conversion to base image is possible
>> +#            rename - renaming volumes is possible
>>   # $snap - check if the feature is supported for a given snapshot
>>   # $running - if the guest owning the volume is running
>>   # $opts - hash with further options:
>> @@ -1849,6 +1850,20 @@ sub complete_volume {
>>       return $res;
>>   }
>> +sub rename_volume {
>> +    my ($cfg, $source_volid, $source_vmid, $target_volname, $target_vmid) = @_;
> 
> Can't the vmid arguments be dropped and extracted directly from the volid/volname instead? Would prevent potential mismatch for careless callers.

Good point

> 
>> +
>> +    my ($storeid) = parse_volume_id($source_volid);
>> +    my (undef, $source_volname, undef, $base_name, $base_vmid, $isBase, $format) = parse_volname($cfg, $source_volid);
>> +
>> +    activate_storage($cfg, $storeid);
>> +
>> +    my $scfg = storage_config($cfg, $storeid);
>> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>> +
> 
> Check that target_volname is valid (by parsing) either here or within each plugin's implementation?

Will do

> 
>> +    return $plugin->rename_volume($scfg, $storeid, $source_volname, $source_vmid, $target_volname, $target_vmid, $base_name, $base_vmid);
> 
> Similar here, not all plugins need all of those parameters. Isn't taking $scfg, $storeid, $source_volid, $target_volname enough and let the plugin itself extract additional information if needed?

I'll look into it

> 
>> +}
>> +

>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 4a10a1f..4e6e288 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -939,6 +939,7 @@ sub volume_has_feature {
>>             snap => {qcow2 => 1} },
>>       sparseinit => { base => {qcow2 => 1, raw => 1, vmdk => 1},
>>               current => {qcow2 => 1, raw => 1, vmdk => 1} },
>> +    rename => { current =>{qcow2 => 1, raw => 1, vmdk => 1} },
>>       };
>>       # clone_image creates a qcow2 volume
>> @@ -946,6 +947,10 @@ sub volume_has_feature {
>>           defined($opts->{valid_target_formats}) &&
>>           !(grep { $_ eq 'qcow2' } @{$opts->{valid_target_formats}});
>> +    return 0 if $feature eq 'rename'
>> +    && $class->can('api')
>> +    && $class->api() < 9;
> 
> Style-nit: multiline post-if
> 
>> +
>>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
>>       $class->parse_volname($volname);
>> @@ -1463,4 +1468,28 @@ sub volume_import_formats {
>>       return ();
>>   }
>> +sub rename_volume {
>> +    my ($class, $scfg, $storeid, $source_volname, $source_vmid, $target_volname, $target_vmid, $base_name, $base_vmid) = @_;
>> +    die "not implemented in storage plugin '$class'\n" if $class->can('api') && $class->api() < 9;
>> +
>> +    my $basedir = $class->get_subdir($scfg, 'images');
>> +    my $sourcedir = "${basedir}/${source_vmid}";
>> +    my $targetdir = "${basedir}/${target_vmid}";
>> +
>> +    mkpath $targetdir;
>> +
>> +    my (undef, $format, undef) = parse_name_dir($source_volname);
>> +    $target_volname = "${target_volname}.${format}";
> 
> Ideally, the $target_volname already contains the format suffix at this point. Otherwise, passing a $target_volname with a format suffix results in a second suffix.
> 

I need to look into that as well and do a few tests.