[pve-devel] [PATCH container 1/2] fix #1904: convert to base image when moving a volume of a template

[Fabian Ebner]

On 19.03.20 09:01, Fabian Grünbichler wrote:
> On March 18, 2020 2:02 pm, Fabian Ebner wrote:
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>
>> For VMs this already happens.
>>
>> When adding volumes to templates no such conversion to
>> base images happens yet (affects both VM/LXC). Because
>> templates are more-or-less supposed to be read-only it
>> probably makes sense to disallow adding volumes altogether.
>> Or should we bother and do conversion instead?
>>
>> When a volume with linked clones is moved with 'delete source'
>> then the volume will be copied over and the source volume does
>> not get deleted, but the source volume disappears from the config.
>> With the second patch it gets added as an unused volume instead.
>>
>>   src/PVE/API2/LXC.pm | 23 +++++++++++++++++++----
>>   1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
>> index a5aa5fc..9eb52dc 100644
>> --- a/src/PVE/API2/LXC.pm
>> +++ b/src/PVE/API2/LXC.pm
>> @@ -468,6 +468,13 @@ __PACKAGE__->register_method({
>>   	return $rpcenv->fork_worker($workername, $vmid, $authuser, $realcmd);
>>       }});
>>   
>> +sub assert_storeid_supports_templates {
>> +    my ($scfg, $sid) = @_;
>> +
>> +    die "Warning: Directory storage '$sid' does not support container templates!\n"
>> +	if $scfg->{ids}->{$sid}->{path};
>> +}
>> +
> 
> so this lead me down quite a rabbit hole back to
> 
> https://bugzilla.proxmox.com/show_bug.cgi?id=1778
> 
> I think the original fix that lead to this "dir storages don't support
> CT templates" was wrong - what we should do is error out early on
> attempts to do a linked clone of a volume on a directory storage (as
> that would create a qcow2 image, which containers don't support).
> 
> having a template on directory storage is perfectly fine - you just can
> only do full clones, since .raw does not support anything else.
> 
> could you take a look at how involved cleaning this up would be?
> 

I think the problem is that volume_has_feature cannot distinguish 
between a raw volume of a VM and a raw volume of a container. And the 
clone feature is not actually present in the latter case.

I see two approaches:
One is to extend volume_has_feature with $vmtype (would be a 
non-breaking API change).
The other is doing an additional check in LXC's clone_vm and not only 
rely on volume_has_feature.

>>   sub check_storage_supports_templates {
>>       my ($conf) = @_;
>>   
>> @@ -477,8 +484,7 @@ sub check_storage_supports_templates {
>>   	    my ($ms, $mp) = @_;
>>   
>>   	    my ($sid) = PVE::Storage::parse_volume_id($mp->{volume}, 0);
>> -	    die "Warning: Directory storage '$sid' does not support container templates!\n"
>> -		if $scfg->{ids}->{$sid}->{path};
>> +	    assert_storeid_supports_templates($scfg, $sid);
>>   	});
>>       };
>>       return $@
>> @@ -1805,6 +1811,8 @@ __PACKAGE__->register_method({
>>   
>>   	my ($mpdata, $old_volid);
>>   
>> +	my $storage_cfg = PVE::Storage::config();
>> +
>>   	PVE::LXC::Config->lock_config($vmid, sub {
>>   	    my $conf = PVE::LXC::Config->load_config($vmid);
>>   	    PVE::LXC::Config->check_lock($conf);
>> @@ -1823,6 +1831,8 @@ __PACKAGE__->register_method({
>>   	    die "you can't move a volume with snapshots and delete the source\n"
>>   		if $param->{delete} && PVE::LXC::Config->is_volume_in_use_by_snapshots($conf, $old_volid);
>>   
>> +	    assert_storeid_supports_templates($storage_cfg, $storage) if PVE::LXC::Config->is_template($conf);
>> +
>>   	    PVE::Tools::assert_if_modified($param->{digest}, $conf->{digest});
>>   
>>   	    PVE::LXC::Config->set_lock($vmid, $lockname);
>> @@ -1833,7 +1843,6 @@ __PACKAGE__->register_method({
>>   		PVE::Cluster::log_msg('info', $authuser, "move volume CT $vmid: move --volume $mpkey --storage $storage");
>>   
>>   		my $conf = PVE::LXC::Config->load_config($vmid);
>> -		my $storage_cfg = PVE::Storage::config();
>>   
>>   		my $new_volid;
>>   
>> @@ -1843,7 +1852,13 @@ __PACKAGE__->register_method({
>>   		    my $source_storage = PVE::Storage::parse_volume_id($old_volid);
>>   		    my $movelimit = PVE::Storage::get_bandwidth_limit('move', [$source_storage, $storage], $bwlimit);
>>   		    $new_volid = PVE::LXC::copy_volume($mpdata, $vmid, $storage, $storage_cfg, $conf, undef, $movelimit);
>> -		    $mpdata->{volume} = $new_volid;
>> +		    if (PVE::LXC::Config->is_template($conf)) {
>> +			PVE::Storage::activate_volumes($storage_cfg, [$new_volid]);
>> +			my $template_volid = PVE::Storage::vdisk_create_base($storage_cfg, $new_volid);
>> +			$mpdata->{volume} = $template_volid;
>> +		    } else {
>> +			$mpdata->{volume} = $new_volid;
>> +		    }
>>   
>>   		    PVE::LXC::Config->lock_config($vmid, sub {
>>   			my $digest = $conf->{digest};
>> -- 
>> 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
>