[pve-devel] [PATCH pve-storage v2] storage plugin: new list_volumes plugin method

Fabian Grünbichler f.gruenbichler at proxmox.com
Fri Jul 19 11:17:56 CEST 2019


On Fri, Jul 19, 2019 at 10:40:52AM +0200, Dietmar Maurer wrote:
> This cleanup improve code reuse, and allows plugins to override list_volumes.
> 
> Note: This changes the template_list return value into an array.
> ---
> 
> Changes in v2:
> 
> - remove debug statements
> - move implementaion to Plugin.pm for max. compatibility with old code
> - cleanup regex (use i flag)
> - bump APIVER an d APIAGE
> - sort result inside volume_list (for all plugins)
> - only list supported/enabled content
> 
> We need to adopt the template_list call in
> - pveam,

pveam uses volume_list only?

> - PVE/QemuServer.pm 7316f
> - PVE/LXC.pm 1832f

see below

> 
> 
>  PVE/Storage.pm                 | 155 ++++-----------------------------
>  PVE/Storage/DirPlugin.pm       |   4 +-
>  PVE/Storage/Plugin.pm          |  77 ++++++++++++++++
>  test/run_test_zfspoolplugin.pl |   2 +-
>  4 files changed, 98 insertions(+), 140 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 5925c69..c438374 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -36,11 +36,11 @@ use PVE::Storage::ZFSPlugin;
>  use PVE::Storage::DRBDPlugin;
>  
>  # Storage API version. Icrement it on changes in storage API interface.
> -use constant APIVER => 2;
> +use constant APIVER => 3;
>  # 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 => 1;
> +use constant APIAGE => 2;
>  
>  # load standard plugins
>  PVE::Storage::DirPlugin->register();
> @@ -769,116 +769,6 @@ sub vdisk_free {
>      $rpcenv->fork_worker('imgdel', undef, $authuser, $cleanup_worker);
>  }
>  
> -# lists all files in the snippets directory
> -sub snippets_list {
> -    my ($cfg, $storeid) = @_;
> -
> -    my $ids = $cfg->{ids};
> -
> -    storage_check_enabled($cfg, $storeid) if ($storeid);
> -
> -    my $res = {};
> -
> -    foreach my $sid (keys %$ids) {
> -	next if $storeid && $storeid ne $sid;
> -	next if !storage_check_enabled($cfg, $sid, undef, 1);
> -
> -	my $scfg = $ids->{$sid};
> -	next if !$scfg->{content}->{snippets};
> -
> -	activate_storage($cfg, $sid);
> -
> -	if ($scfg->{path}) {
> -	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> -	    my $path = $plugin->get_subdir($scfg, 'snippets');
> -
> -	    foreach my $fn (<$path/*>) {
> -		next if -d $fn;
> -
> -		push @{$res->{$sid}}, {
> -		    volid => "$sid:snippets/". basename($fn),
> -		    format => 'snippet',
> -		    size => -s $fn // 0,
> -		};
> -	    }
> -	}
> -
> -	if ($res->{$sid}) {
> -	    @{$res->{$sid}} = sort {$a->{volid} cmp $b->{volid} } @{$res->{$sid}};
> -	}
> -    }
> -
> -    return $res;
> -}
> -
> -#list iso or openvz template ($tt = <iso|vztmpl|backup>)
> -sub template_list {
> -    my ($cfg, $storeid, $tt) = @_;
> -
> -    die "unknown template type '$tt'\n"
> -	if !($tt eq 'iso' || $tt eq 'vztmpl' || $tt eq 'backup');
> -
> -    my $ids = $cfg->{ids};
> -
> -    storage_check_enabled($cfg, $storeid) if ($storeid);
> -
> -    my $res = {};
> -
> -    # query the storage
> -
> -    foreach my $sid (keys %$ids) {
> -	next if $storeid && $storeid ne $sid;
> -
> -	my $scfg = $ids->{$sid};
> -	my $type = $scfg->{type};
> -
> -	next if !storage_check_enabled($cfg, $sid, undef, 1);
> -
> -	next if $tt eq 'iso' && !$scfg->{content}->{iso};
> -	next if $tt eq 'vztmpl' && !$scfg->{content}->{vztmpl};
> -	next if $tt eq 'backup' && !$scfg->{content}->{backup};
> -
> -	activate_storage($cfg, $sid);
> -
> -	if ($scfg->{path}) {
> -	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> -
> -	    my $path = $plugin->get_subdir($scfg, $tt);
> -
> -	    foreach my $fn (<$path/*>) {
> -
> -		my $info;
> -
> -		if ($tt eq 'iso') {
> -		    next if $fn !~ m!/([^/]+\.[Ii][Ss][Oo])$!;
> -
> -		    $info = { volid => "$sid:iso/$1", format => 'iso' };
> -
> -		} elsif ($tt eq 'vztmpl') {
> -		    next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
> -
> -		    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
> -
> -		} elsif ($tt eq 'backup') {
> -		    next if $fn !~ m!/([^/]+\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo))$!;
> -
> -		    $info = { volid => "$sid:backup/$1", format => $2 };
> -		}
> -
> -		$info->{size} = -s $fn // 0;
> -
> -		push @{$res->{$sid}}, $info;
> -	    }
> -
> -	}
> -
> -	@{$res->{$sid}} = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @{$res->{$sid}} if $res->{$sid};
> -    }
> -
> -    return $res;
> -}
> -
> -
>  sub vdisk_list {
>      my ($cfg, $storeid, $vmid, $vollist) = @_;
>  
> @@ -923,6 +813,15 @@ sub vdisk_list {
>      return $res;
>  }
>  
> +sub template_list {
> +    my ($cfg, $storeid, $tt) = @_;
> +
> +    die "unknown template type '$tt'\n"
> +	if !($tt eq 'iso' || $tt eq 'vztmpl' || $tt eq 'backup' || $tt eq 'snippets');
> +
> +    return volume_list($cfg, $storeid, undef, $tt);
> +}

sorry for not catching this in v1 already, but this previously allowed
listing templates from all storages if $storeid was undefined, now it
requires $storeid. since this is only used for completion, where this
behaviour is actually nice for

pct create 12345 <TAB>

completing all available templates on all storages, why not keep the old
output format, and add the usual loop like so:

----8<----

die "unknown template type '$tt'\n"
    if !($tt eq 'iso' || $tt eq 'vztmpl' || $tt eq 'backup' || $tt eq 'snippets');

my $ids = $cfg->{ids};

storage_check_enabled($cfg, $storeid) if ($storeid);

my $res = {};

foreach my $sid (keys %$ids) {
    next if $storeid && $storeid ne $sid;
    next if !storage_check_enabled($cfg, $sid, undef, 1);
    $res->{$storeid} = volume_list($cfg, $storeid, undef, $ tt);
}

return $res;

---->8----

this would keep the interface for the completion helpers identical,
which also keeps the changes limited to a single repo.

> +
>  sub volume_list {
>      my ($cfg, $storeid, $vmid, $content) = @_;
>  
> @@ -932,33 +831,15 @@ sub volume_list {
>  
>      my $scfg = PVE::Storage::storage_config($cfg, $storeid);
>  
> -    my $res = [];
> -    foreach my $ct (@$cts) {
> -	my $data;
> -	if ($ct eq 'images') {
> -	    $data = vdisk_list($cfg, $storeid, $vmid);
> -	} elsif ($ct eq 'iso' && !defined($vmid)) {
> -	    $data = template_list($cfg, $storeid, 'iso');
> -	} elsif ($ct eq 'vztmpl'&& !defined($vmid)) {
> -	    $data = template_list ($cfg, $storeid, 'vztmpl');
> -	} elsif ($ct eq 'backup') {
> -	    $data = template_list ($cfg, $storeid, 'backup');
> -	    foreach my $item (@{$data->{$storeid}}) {
> -		if (defined($vmid)) {
> -		    @{$data->{$storeid}} = grep { $_->{volid} =~ m/\S+-$vmid-\S+/ } @{$data->{$storeid}};
> -		}
> -	    }
> -	} elsif ($ct eq 'snippets') {
> -	    $data = snippets_list($cfg, $storeid);
> -	}
> +    $cts = [ grep { defined($scfg->{content}->{$_}) } @$cts ];
> +
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>  
> -	next if !$data || !$data->{$storeid};
> +    activate_storage($cfg, $storeid);
>  
> -	foreach my $item (@{$data->{$storeid}}) {
> -	    $item->{content} = $ct;
> -	    push @$res, $item;
> -	}
> -    }
> +    my $res = $plugin->list_volumes($storeid, $scfg, $vmid, $cts);
> +
> +    @$res = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @$res;
>  
>      return $res;
>  }
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index e24ee09..39760a8 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -21,7 +21,7 @@ sub plugindata {
>  		     { images => 1,  rootdir => 1 }],
>  	format => [ { raw => 1, qcow2 => 1, vmdk => 1, subvol => 1 } , 'raw' ],
>      };
> -}   
> +}
>  
>  sub properties {
>      return {
> @@ -114,7 +114,7 @@ sub activate_storage {
>  	    "directory is expected to be a mount point but is not mounted: '$mp'\n";
>      }
>  
> -    $class->SUPER::activate_storage($storeid, $scfg, $cache);    
> +    $class->SUPER::activate_storage($storeid, $scfg, $cache);
>  }
>  
>  sub check_config {
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 842b4d2..eb2d86a 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -902,6 +902,83 @@ sub list_images {
>      return $res;
>  }
>  
> +# list templates ($tt = <iso|vztmpl|backup|snippets>)
> +my $template_list = sub {

if you send a v3 with the above change, we could also give this a better
name. what it actually does is list content in a subdir of the storage?

subdir_list_content
subdir_list_files
list_subdir_files
subdir_file_list
get_subdir_files (since we already of get_subdir)

> +    my ($sid, $path, $tt, $vmid) = @_;
> +
> +    my $res = [];
> +
> +    foreach my $fn (<$path/*>) {
> +
> +	next if -d $fn;
> +
> +	my $info;
> +
> +	if ($tt eq 'iso') {
> +	    next if $fn !~ m!/([^/]+\.iso)$!i;
> +
> +	    $info = { volid => "$sid:iso/$1", format => 'iso' };
> +
> +	} elsif ($tt eq 'vztmpl') {
> +	    next if $fn !~ m!/([^/]+\.tar\.([gx]z))$!;
> +
> +	    $info = { volid => "$sid:vztmpl/$1", format => "t$2" };
> +
> +	} elsif ($tt eq 'backup') {
> +	    next if $fn !~ m!/([^/]+\.(tar|tar\.gz|tar\.lzo|tgz|vma|vma\.gz|vma\.lzo))$!;
> +	    next if defined($vmid) && $fn !~  m/\S+-$vmid-\S+/;
> +
> +	    $info = { volid => "$sid:backup/$1", format => $2 };
> +
> +	} elsif ($tt eq 'snippets') {
> +
> +	    $info = {
> +		volid => "$sid:snippets/". basename($fn),
> +		format => 'snippet',
> +	    };
> +	}
> +
> +	$info->{size} = -s $fn // 0;
> +
> +	push @$res, $info;
> +    }
> +
> +    return $res;
> +};
> +
> +sub list_volumes {
> +    my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> +
> +    my $res = [];
> +
> +    foreach my $ct (@$content_types) {
> +	my $data;
> +
> +	my $path = $class->get_subdir($scfg, $ct);
> +
> +	if ($ct eq 'images') {
> +	    $data = $class->list_images($storeid, $scfg, $vmid);
> +	} elsif ($ct eq 'iso' && !defined($vmid)) {
> +	    $data = $template_list->($storeid, $path, 'iso');
> +	} elsif ($ct eq 'vztmpl'&& !defined($vmid)) {
> +	    $data = $template_list->($storeid, $path, 'vztmpl');
> +	} elsif ($ct eq 'backup') {
> +	    $data = $template_list->($storeid, $path, 'backup', $vmid);
> +	} elsif ($ct eq 'snippets') {
> +	    $data = $template_list->($storeid, $path, 'snippets');
> +	}
> +
> +	next if !$data;
> +
> +	foreach my $item (@$data) {
> +	    $item->{content} = $ct;
> +	    push @$res, $item;
> +	}
> +    }
> +
> +    return $res;
> +}
> +
>  sub status {
>      my ($class, $storeid, $scfg, $cache) = @_;
>  
> diff --git a/test/run_test_zfspoolplugin.pl b/test/run_test_zfspoolplugin.pl
> index 63b1456..2058508 100755
> --- a/test/run_test_zfspoolplugin.pl
> +++ b/test/run_test_zfspoolplugin.pl
> @@ -309,7 +309,7 @@ my $test15 = sub {
>  
>      print "\nrun test15 \"template_list and vdisk_list\"\n";
>  
> -    my $hash = Dumper {};
> +    my $hash = Dumper [];

this change would become unnecessary

>  
>      my $res = Dumper PVE::Storage::template_list($cfg, $storagename, "vztmpl");
>      if ( $hash ne $res ) {
> -- 
> 2.20.1
> 
> _______________________________________________
> 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