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

[Fabian Grünbichler]

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 ;)

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