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

[Fabian Grünbichler]

On April 6, 2020 10:46 am, Fabian Ebner wrote:
> 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.

right, those are actually not implemented in the storage layer 
(anymore?), so checking there does not make much sense. I wonder whether 
we even need the 'copy' feature then? export/import uses it's own format 
matching, and the full clone stuff is already in 
pve-container/qemu-server with lots of special handling.

there's also still some issue/missing check for subvols of size 0 which 
can't be moved to a sized volume, but that is unrelated.

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

you can't say it's valid per se - since for example raw->raw is not 
possible, but raw->qcow2 is.

> Otherwise one needs to add a check whether valid_target_formats includes 
> the format of the source for other plugins that implement clone_image.

hm. it's all a bit of a mess with a fuzzy boundary between storage and 
qemu-server/pve-container, and probably requires more thought to clean 
up...

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