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