[pve-devel] applied-series: [PATCH v3 storage 1/5] Allow passing options to volume_has_feature

[Fabian Grünbichler]

On April 7, 2020 2:19 pm, Fabian Ebner wrote:
> On 06.04.20 11:52, Fabian Grünbichler wrote:
>> On April 6, 2020 10:46 am, Fabian Ebner wrote:
>>> On 27.03.20 10:09, Fabian Grünbichler wrote:
>>>> with a small follow-up for vzdump (see separate mail), and comment below
>>>>
>>>> On March 23, 2020 12:18 pm, Fabian Ebner wrote:
>>>>> With the option valid_target_formats it's possible
>>>>> to let the caller specify possible formats for the target
>>>>> of an operation.
>>>>> [0]: If the option is not set, assume that every format is valid.
>>>>>
>>>>> In most cases the format of the the target and the format
>>>>> of the source will agree (and therefore assumption [0] is
>>>>> not actually assuming very much and ensures backwards
>>>>> compatability). But when cloning a volume on a storage
>>>>> using Plugin.pm's implementation (e.g. directory based
>>>>> storages), the result is always a qcow2 image.
>>>>>
>>>>> When cloning containers, the new option can be used to detect
>>>>> that qcow2 is not valid and hence the clone feature is not
>>>>> available.
>>>>>
>>>>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>>>>> ---
>>>>
>>>> it would be a good follow-up to think which other storage plugins should
>>>> also implement this. e.g. plugins supporting subvol and raw/qcow2/vmdk -
>>>> can they copy/clone from one to the other? future people might only look
>>>> at Plugin.pm, see that such an option exists, and assume it works for all
>>>> plugins/features ;)
>>>>
>>>
>>> The copy operations are actually implemented as the non-full clone_disk
>>> for QEMU and as copy_volume in LXC. AFAIU they should always work.
>> 
>> right, those are actually not implemented in the storage layer
>> (anymore?), so checking there does not make much sense. I wonder whether
>> we even need the 'copy' feature then? export/import uses it's own format
>> matching, and the full clone stuff is already in
>> pve-container/qemu-server with lots of special handling.
>> 
> 
> [2]: There is a single caller (except for the has_feature API calls) in 
> API2/Qemu.pm's clone_vm.
> 
> ("allowed" in the next paragraph means has_feature returns 1)
> Looking through the individual plugins, copy with "current" is always 
> allowed and whenever the storage (and for Plugin.pm format) supports 
> snapshots respectively base images, then copy with "snap" respectively 
> "base" is allowed. That is, except for ZFS, where copy with "snap" is 
> not allowed.
> 
> But this another difference between LXC/QEMU. For containers there is no 
> equivalent check like [2] and making a full clone from a snapshot 
> actually works. When I comment out the check for QEMU, I get:
> 
> qemu-img: Could not open '/dev/zvol/myzpool/vm-108-disk-0 at adsf': Could 
> not open '/dev/zvol/myzpool/vm-108-disk-0 at adsf': No such file or directory
> 
> So there is a purpose for checking the copy feature, although not the 
> best one, since copying from a ZFS snapshot isn't impossible in itself 
> as the LXC case shows.

we could implement copy from zvol snapshot as well - we'd need to query 
and potentially set the snapdev property (akin to a 'map the snapshot' 
action), and then reset it back to the original value ('unmap the 
snapshot').

> Also, wouldn't removing the copy feature technically also break the API?

yes. but we could remove it in the sense of 'we no longer use it for 
anything', and then drop it with 7.x for example. the question is 
whether it makes sense to keep it to encode the 'mapping/accessing a 
snapshot/base volume for a full clone is possible' feature, since the 
assumption that you can always copy the current one is probably true 
(and format conversion for cloning happens a level above the storage, or 
can fallback to storage_migrate with its mechanism).

> If we really want to remove it, we could replace the check in QEMU by an 
> explicit check for "full copy of ZFS from snapshot" or otherwise 
> implement that feature and drop the check entirely.

see above.

>> there's also still some issue/missing check for subvols of size 0 which
>> can't be moved to a sized volume, but that is unrelated.
>> 
>>>
>>> For clone operations, there is vdisk_clone in the storage module, but it
>>> doesn't take a format parameter. Except for GlusterfsPlugin.pm (which
>>> doesn't have its own volume_has_feature) and Plugin.pm the format of the
>>> target is the same as the format of the source. Shouldn't that be
>>> considered valid regardless of the valid_target_formats option?
>> 
>> you can't say it's valid per se - since for example raw->raw is not
>> possible, but raw->qcow2 is.
>> 
> 
> Yes, raw->raw is not possible, but raw would be a valid target format 
> from the caller's perspective. Since the caller apparently can handle 
> volumes with the format of the source, the storage backend can assume 
> it's fine if the target is in that format as well. That does not mean 
> that the operation will result in that format, just that it would be 
> something the caller could deal with.
> 
> But it's true that this is an additional assumption:
> [0]: If the option is not set, assume that every format is valid.
> [1]: Regardless of whether the option is set, the format of the source 
> is valid.
> (valid again means something the caller can deal with).
> 
> [0] and [1] together are sufficient to make the current 
> implementation(s) of volume_has_feature correct. If we drop [1], we need 
> additional checks for clone in volume_has_feature (for LvmThin/RBD/ZFS), 
> i.e. check if the format of the source (which will be the format of the 
> target) appears in the list the caller gave us.
> 
> Should I add a comment with both assumptions or only the first one and 
> add the additional checks?

I guess documenting both assumptions is a good choice for now - we can 
always revisit if we find another issue.