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

[Fabian Grünbichler]

On March 19, 2020 10:43 am, Fabian Ebner wrote:
> 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).

if we do it there, I'd rather see it with $format (we don't want 
guest-specifics inside pve-storage if avoidable), and in this case we 
actually want to ask 'can we create a linked clone with format raw or 
subvol', not 'can we create a linked clone as container'.

> The other is doing an additional check in LXC's clone_vm and not only 
> rely on volume_has_feature.

this would be a valid workaround as well. first check whether cloning is 
possible in theory via has_feature, and then check if the storage is a 
dir-based storage and die accordingly.

the proper solution has the advantage that if we ever implement raw 
linked clone support on a dir based storage (however that would look 
like :-P), we'd just need to do it in pve-storage and pve-container 
would magically work. it might also be nice to have this API extension 
for other stuff/features, but from the top of my head I don't have any 
concrete example. if you can think of something, that would probably 
push the needle towards solution 1 ;)

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