[pve-devel] applied-series: [PATCH v3 storage 1/5] Allow passing options to volume_has_feature
Fabian Ebner
f.ebner at proxmox.com
Tue Apr 7 14:19:42 CEST 2020
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.
Also, wouldn't removing the copy feature technically also break the API?
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.
> 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?
>> Otherwise one needs to add a check whether valid_target_formats includes
>> the format of the source for other plugins that implement clone_image.
>
> hm. it's all a bit of a mess with a fuzzy boundary between storage and
> qemu-server/pve-container, and probably requires more thought to clean
> up...
>
>>>> Changes from v2:
>>>> * Use new options parameter instead of adding more
>>>> dependency to PVE::Cluster
>>>> * storage API bump and now there is an inter-package dependency:
>>>> #2 container depends on #1 storage
>>>>
>>>> PVE/Storage.pm | 8 ++++----
>>>> PVE/Storage/Plugin.pm | 7 ++++++-
>>>> 2 files changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>>> index a46550c..fcfc6af 100755
>>>> --- a/PVE/Storage.pm
>>>> +++ b/PVE/Storage.pm
>>>> @@ -39,11 +39,11 @@ use PVE::Storage::DRBDPlugin;
>>>> use PVE::Storage::PBSPlugin;
>>>>
>>>> # Storage API version. Icrement it on changes in storage API interface.
>>>> -use constant APIVER => 3;
>>>> +use constant APIVER => 4;
>>>> # Age is the number of versions we're backward compatible with.
>>>> # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>>>> # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
>>>> -use constant APIAGE => 2;
>>>> +use constant APIAGE => 3;
>>>>
>>>> # load standard plugins
>>>> PVE::Storage::DirPlugin->register();
>>>> @@ -286,13 +286,13 @@ sub volume_snapshot_delete {
>>>> }
>>>>
>>>> sub volume_has_feature {
>>>> - my ($cfg, $feature, $volid, $snap, $running) = @_;
>>>> + my ($cfg, $feature, $volid, $snap, $running, $opts) = @_;
>>>>
>>>> my ($storeid, $volname) = parse_volume_id($volid, 1);
>>>> if ($storeid) {
>>>> my $scfg = storage_config($cfg, $storeid);
>>>> my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>>> - return $plugin->volume_has_feature($scfg, $feature, $storeid, $volname, $snap, $running);
>>>> + return $plugin->volume_has_feature($scfg, $feature, $storeid, $volname, $snap, $running, $opts);
>>>> } elsif ($volid =~ m|^(/.+)$| && -e $volid) {
>>>> return undef;
>>>> } else {
>>>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>>>> index 2232261..8c0dae1 100644
>>>> --- a/PVE/Storage/Plugin.pm
>>>> +++ b/PVE/Storage/Plugin.pm
>>>> @@ -828,7 +828,7 @@ sub storage_can_replicate {
>>>> }
>>>>
>>>> sub volume_has_feature {
>>>> - my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running) = @_;
>>>> + my ($class, $scfg, $feature, $storeid, $volname, $snapname, $running, $opts) = @_;
>>>>
>>>> my $features = {
>>>> snapshot => { current => { qcow2 => 1}, snap => { qcow2 => 1} },
>>>> @@ -841,6 +841,11 @@ sub volume_has_feature {
>>>> current => {qcow2 => 1, raw => 1, vmdk => 1} },
>>>> };
>>>>
>>>> + # clone_image creates a qcow2 volume
>>>> + return 0 if $feature eq 'clone' &&
>>>> + defined($opts->{valid_target_formats}) &&
>>>> + !(grep { $_ eq 'qcow2' } @{$opts->{valid_target_formats}});
>>>> +
>>>> my ($vtype, $name, $vmid, $basename, $basevmid, $isBase, $format) =
>>>> $class->parse_volname($volname);
>>>>
>>>> --
>>>> 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
>>>
>>
More information about the pve-devel
mailing list