[pve-devel] applied-series: [PATCH v3 storage 1/5] Allow passing options to volume_has_feature

[Fabian Ebner]

On 27.03.20 10:09, Fabian Grünbichler wrote:
> with a small follow-up for vzdump (see separate mail), and comment below
> 
> On March 23, 2020 12:18 pm, Fabian Ebner wrote:
>> With the option valid_target_formats it's possible
>> to let the caller specify possible formats for the target
>> of an operation.
>> [0]: If the option is not set, assume that every format is valid.
>>
>> In most cases the format of the the target and the format
>> of the source will agree (and therefore assumption [0] is
>> not actually assuming very much and ensures backwards
>> compatability). But when cloning a volume on a storage
>> using Plugin.pm's implementation (e.g. directory based
>> storages), the result is always a qcow2 image.
>>
>> When cloning containers, the new option can be used to detect
>> that qcow2 is not valid and hence the clone feature is not
>> available.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
> 
> it would be a good follow-up to think which other storage plugins should
> also implement this. e.g. plugins supporting subvol and raw/qcow2/vmdk -
> can they copy/clone from one to the other? future people might only look
> at Plugin.pm, see that such an option exists, and assume it works for all
> plugins/features ;)
> 

The copy operations are actually implemented as the non-full clone_disk 
for QEMU and as copy_volume in LXC. AFAIU they should always work.

For clone operations, there is vdisk_clone in the storage module, but it 
doesn't take a format parameter. Except for GlusterfsPlugin.pm (which 
doesn't have its own volume_has_feature) and Plugin.pm the format of the 
target is the same as the format of the source. Shouldn't that be 
considered valid regardless of the valid_target_formats option? 
Otherwise one needs to add a check whether valid_target_formats includes 
the format of the source for other plugins that implement clone_image.


>> Changes from v2:
>>      * Use new options parameter instead of adding more
>>        dependency to PVE::Cluster
>>      * storage API bump and now there is an inter-package dependency:
>>        #2 container depends on #1 storage
>>
>>   PVE/Storage.pm        | 8 ++++----
>>   PVE/Storage/Plugin.pm | 7 ++++++-
>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index a46550c..fcfc6af 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -39,11 +39,11 @@ use PVE::Storage::DRBDPlugin;
>>   use PVE::Storage::PBSPlugin;
>>   
>>   # Storage API version. Icrement it on changes in storage API interface.
>> -use constant APIVER => 3;
>> +use constant APIVER => 4;
>>   # 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 => 2;
>> +use constant APIAGE => 3;
>>   
>>   # load standard plugins
>>   PVE::Storage::DirPlugin->register();
>> @@ -286,13 +286,13 @@ sub volume_snapshot_delete {
>>   }
>>   
>>   sub volume_has_feature {
>> -    my ($cfg, $feature, $volid, $snap, $running) = @_;
>> +    my ($cfg, $feature, $volid, $snap, $running, $opts) = @_;
>>   
>>       my ($storeid, $volname) = parse_volume_id($volid, 1);
>>       if ($storeid) {
>>           my $scfg = storage_config($cfg, $storeid);
>>           my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>> -        return $plugin->volume_has_feature($scfg, $feature, $storeid, $volname, $snap, $running);
>> +        return $plugin->volume_has_feature($scfg, $feature, $storeid, $volname, $snap, $running, $opts);
>>       } elsif ($volid =~ m|^(/.+)$| && -e $volid) {
>>   	return undef;
>>       } else {
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 2232261..8c0dae1 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -828,7 +828,7 @@ sub storage_can_replicate {
>>   }
>>   
>>   sub volume_has_feature {
>> -    my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
>> +    my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
>>   
>>       my $features = {
>>   	snapshot => { current => { qcow2 => 1}, snap => { qcow2 => 1} },
>> @@ -841,6 +841,11 @@ sub volume_has_feature {
>>   			current => {qcow2 => 1, raw => 1, vmdk => 1} },
>>       };
>>   
>> +    # clone_image creates a qcow2 volume
>> +    return 0 if $feature eq 'clone' &&
>> +		defined($opts->{valid_target_formats}) &&
>> +		!(grep { $_ eq 'qcow2' } @{$opts->{valid_target_formats}});
>> +
>>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
>>   	$class->parse_volname($volname);
>>   
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>