[pve-devel] [PATCH storage] fix #2467 remove duplicate volumes & tag with correct content type

[Fabian Ebner]

On 11/19/19 12:05 PM, Tim Marx wrote:
> The bugfix for #2317 introduced a kind of odd api behavior, where each volume
> was returned twice from our api if a storage has both 'rootdir' & 'images' content
> types enabled. To give the content type of the volume an actual meaning, it is
> now inferred form the associated guest (qemu/lxc) and we now ignore 'rootdir'
> if both types are enabled. At the volume level, there is no option to list volumes
> based on content types, since the volumes do not know what type they are actually
> used for.
> 
> Signed-off-by: Tim Marx <t.marx at proxmox.com>
> ---
>   PVE/Storage.pm        |  5 +++++
>   PVE/Storage/Plugin.pm | 19 +++++++++++++++++--
>   2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index bb3b874..d597fcf 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -854,6 +854,11 @@ sub volume_list {
>       my $cts = $content ? [ $content ] : [ @ctypes ];
> 
>       my $scfg = PVE::Storage::storage_config($cfg, $storeid);
> +    # volumes aren't aware of their content type, in case both types are allowed
> +    # for a specific storage, we would return each volume twice.
> +    if (defined($scfg->{content}->{rootdir}) && defined($scfg->{content}->{images})) {
> +	$cts = [ grep (!/rootdir/, @$cts) ];
> +    }
> 
>       $cts = [ grep { defined($scfg->{content}->{$_}) } @$cts ];
>

Isn't it necessary to do this filtering inside 'list_volumes' and 
probably even for each volume?
Otherwise for a storage with content types rootdir and images:
'pvesm list local-lvm --content rootdir' doesn't list anything and 
'pvesm list local-lvm --content images' lists both container rootdirs 
and VM images.

> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 88da231..2f30388 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -937,7 +937,7 @@ sub list_volumes {
>       my ($class, $storeid, $scfg, $vmid, $content_types) = @_;
> 
>       my $res = [];
> -
> +    my $vmlist = PVE::Cluster::get_vmlist();
>       foreach my $ct (@$content_types) {
>   	my $data;
> 
> @@ -960,7 +960,22 @@ sub list_volumes {
>   	next if !$data;
> 
>   	foreach my $item (@$data) {
> -	    $item->{content} = $ct;
> +	    if ($ct eq 'images' || $ct eq 'rootdir') {
> +		my $vmtype = $vmlist->{ids}->{$item->{vmid}}->{type};
> +		if (defined($vmtype)) {
> +		    if ($vmtype eq 'lxc') {
> +			$item->{content} = 'rootdir';
> +		    }
> +		    if ($vmtype eq 'qemu') {
> +			$item->{content} = 'images';
> +		    }
> +		} else {
> +		    $item->{content} = 'unknown';
> +		}
> +	    } else {
> +		$item->{content} = $ct;
> +	    }
> +
>   	    push @$res, $item;
>   	}
>       }
> --
> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>