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.

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? Otherwise one needs to add a check whether valid_target_formats includes the format of the source for other plugins that implement clone_image.


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

Reply via email to