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 belowOn 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.eb...@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@adsf': Could not open '/dev/zvol/myzpool/vm-108-disk-0@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 pluginsPVE::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@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
