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

[Fabian Grünbichler]

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?

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