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.eb...@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@pve.proxmox.com
>>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
