[pve-devel] [PATCH v2 storage 1/4] volume_has_feature: be aware that qcow2 does not work for lxc

[Fabian Ebner]

On 19.03.20 16:53, Fabian Grünbichler wrote:
> On March 19, 2020 1:37 pm, Fabian Ebner wrote:
>> Relevant for the 'clone' feature, because Plugin.pm's clone_image
>> always produces qcow2. Also fixed style for neighboring if/else block.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>
>> Previous discussion: https://pve.proxmox.com/pipermail/pve-devel/2020-March/042472.html
>>
>> Changes from v1:
>>      * As Fabian G. pointed out, templates are not impossible
>>        on directory based storages, but linked cloning (currently)
>>        is. So fix the storage backend and get rid of the wrong checks.
>>
>> This solution doesn't need an API change. It does need
>> PVE::Cluster which is used by list_images already.
> 
> I really don't like this - hence my comment on v1. I also did not like
> it for list_images ;) pve-storage should not be concerned with which
> guests support which formats.
> 
> if we don't want to extend has_feature with a target_format for
> copy/clone, then I'd rather we work around this in pve-container. it's a
> single call site there anyway.
> 

Ok, I hadn't caught your last reply before sending out this patch.

There's actually two places that would need an extra check in LXC:
1. In clone_vm in PVE/API2/LXC.pm
2. In the has_feature function in PVE/LXC/Config.pm
(AFAIU clone_vm doesn't use has_feature, since it does other things 
while iterating the mount points)

I'm thinking about adding an $opts parameter to volume_has_feature and 
using the option valid_target_formats (as a list of acceptable formats 
for the target of the clone operation) as you suggested.

>>
>>   PVE/Storage/Plugin.pm | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 2232261..8baa410 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -844,13 +844,19 @@ sub volume_has_feature {
>>       my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
>>   	$class->parse_volname($volname);
>>   
>> +    my $vmlist = PVE::Cluster::get_vmlist();
>> +    my $vminfo = $vmlist->{ids}->{$vmid};
>> +
>>       my $key = undef;
>> -    if($snapname){
>> -        $key = 'snap';
>> -    }else{
>> -        $key =  $isBase ? 'base' : 'current';
>> +    if ($snapname) {
>> +	$key = 'snap';
>> +    } else {
>> +	$key = $isBase ? 'base' : 'current';
>>       }
>>   
>> +    # clone_images produces a qcow2 image
>> +    return 0 if defined($vminfo) && $vminfo->{type} eq 'lxc' && $feature eq 'clone';
>> +
>>       return 1 if defined($features->{$feature}->{$key}->{$format});
>>   
>>       return undef;
>> -- 
>> 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
>