[pve-devel] [PATCH pve-storage] PVE/Storage/Plugin.pm: new list_volumes plugin method
Fabian Grünbichler
f.gruenbichler at proxmox.com
Fri Jul 19 09:24:18 CEST 2019
meta: not tested, just read.. comments inline
AFAICT this is not only an API change, but a breaking one.
On Fri, Jul 19, 2019 at 08:02:34AM +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.
doesn't this break existing users? e.g.,
PVE/QemuServer.pm 7316f:
my $data = PVE::Storage::template_list($cfg, $storeid, 'backup');
my $res = [];
foreach my $id (keys %$data) {
foreach my $item (@{$data->{$id}}) {
PVE/LXC.pm 1832f:
my $data = PVE::Storage::template_list($cfg, $storeid, $vtype);
my $res = [];
foreach my $id (keys %$data) {
foreach my $item (@{$data->{$id}}) {
(both are "only" completion helpers, but still..)
> ---
> PVE/Storage.pm | 147 +++------------------------------
> PVE/Storage/DirPlugin.pm | 84 ++++++++++++++++++-
> PVE/Storage/Plugin.pm | 23 ++++++
> test/run_test_zfspoolplugin.pl | 2 +-
> 4 files changed, 118 insertions(+), 138 deletions(-)
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 5925c69..18ce8b6 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -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 {
would be good to explicitly mention that this gets dropped, there are no
users anyway besides Storage.pm
> - 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);
> +}
> +
> sub volume_list {
> my ($cfg, $storeid, $vmid, $content) = @_;
>
> @@ -932,33 +831,11 @@ 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);
> - }
> + 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);
1) PVE::Storage::list_volumes is used in pveam, but also exposed for arbitrary content
types over the API and CLI.
>
> return $res;
> }
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index e24ee09..6d5fb6d 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 {
> @@ -127,4 +127,84 @@ sub check_config {
> return $opts;
> }
>
> +# list templates ($tt = <iso|vztmpl|backup|snippets>)
> +my $template_list = sub {
> + 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!/([^/]+\.[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))$!;
> + 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) = @_;
> +
> + use Data::Dumper;
> + 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;
> + }
> + }
> +
> + @$res = sort {lc($a->{volid}) cmp lc ($b->{volid}) } @$res;
> +
> + return $res;
> +}
2) but this fallback to the behaviour previously exhibited by all plugins via Storage.pm is now only used for DirPlugin.pm (and derived plugins)
> +
> 1;
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 842b4d2..e940195 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -902,6 +902,29 @@ sub list_images {
> return $res;
> }
>
> +sub list_volumes {
> + my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> +
> + my $res = [];
> +
> + foreach my $ct (@$content_types) {
> + my $data;
> +
> + # default implementation just considers images
> + if ($ct eq 'images') {
> + $data = $class->list_images($storeid, $scfg, $vmid);
> + }
> + next if !$data;
> +
> + foreach my $item (@$data) {
> + $item->{content} = $ct;
> + push @$res, $item;
> + }
> + }
> +
> + return $res;
> +}
3) so any external plugin deriving from the base Plugin.pm will now no
longer be able to list anything besides images via the API, either over
storage/{storage}/content (pvesm list) or pveam index? seems like a
breaking API change to me..
> +
> 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 [];
>
> 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