Re: [pve-devel] [PATCH storage] config: add overrides for default directory locations

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

This is going to need multiple sets of eyes as I don't trust that we
don't have some rogue direct `$scfg->{path}` usage ruining this for some
case ;-)

As for the code, see inline comments:

On Thu, Dec 15, 2022 at 12:21:46PM +0100, Leo Nunner wrote:
> Allowing overrides for the default directory locations seems to
> integrate rather well into the existing system. Custom locations
> are specified using the "dirs" parameter as a comma-separated list
> of "vtype:/location" values.
> 
> For now, the option has been enabled for the Directory, CIFS and NFS
> backends.
> 
> Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
> ---
>  PVE/Storage/CIFSPlugin.pm |  1 +
>  PVE/Storage/DirPlugin.pm  |  1 +
>  PVE/Storage/NFSPlugin.pm  |  1 +
>  PVE/Storage/Plugin.pm     | 43 +++++++++++++++++++++++++++++++++++----
>  test/get_subdir_test.pm   |  7 +++++++
>  5 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index 982040a..4284c35 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -128,6 +128,7 @@ sub properties {
>  sub options {
>      return {
>  	path => { fixed => 1 },
> +	dirs => { optional => 1 },
>  	server => { fixed => 1 },
>  	share => { fixed => 1 },
>  	nodes => { optional => 1 },
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 8715a9d..3c907ca 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -54,6 +54,7 @@ sub properties {
>  sub options {
>      return {
>  	path => { fixed => 1 },
> +	dirs => { optional => 1 },
>  	nodes => { optional => 1 },
>  	shared => { optional => 1 },
>  	disable => { optional => 1 },
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 5bd7313..b7e8318 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -79,6 +79,7 @@ sub properties {
>  sub options {
>      return {
>  	path => { fixed => 1 },
> +	dirs => { optional => 1 },
>  	server => { fixed => 1 },
>  	export => { fixed => 1 },
>  	nodes => { optional => 1 },
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 8a41df1..de9227b 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -181,6 +181,11 @@ my $defaultData = {
>  	    default => 'metadata',
>  	    optional => 1,
>  	},
> +	dirs => {
> +	    description => "Overrides for default directories",
> +	    type => "string", format => "pve-volume-id-list",
> +	    optional => 1,
> +	},
>      },
>  };
>  
> @@ -205,6 +210,17 @@ sub valid_content_types {
>      return $def->{content}->[0];
>  }
>  
> +sub dirs_hash_to_string {
> +    my $hash = shift;
> +
> +    my @cta;

Better just build the string right away and skip the cryptically named
array ;-)
(can do a `$str .= ',' if length($str);` line for the comma part)

Alternatively you can use a more functional style with `map {}`, eg.:

    join(',', map { "$_:$hash->{$_}" } sort keys %$hash);

Finally: please `sort` the keys to avoid adding yet another case of
reordering happening on every single storage.cfg modification as perl
hashes are randomized ;-)

> +    foreach my $dir (keys %$hash) {
> +	push @cta, "$dir:$hash->{$dir}" if $hash->{$dir};
> +    }
> +
> +    return join(',', @cta);
> +}
> +
>  sub default_format {
>      my ($scfg) = @_;
>  
> @@ -405,6 +421,23 @@ sub decode_value {
>  	#    die "storage '$storeid' does not allow node restrictions\n";
>  	#}
>  
> +	return $res;
> +    } elsif ($key eq 'dirs') {
> +	my $valid_content = $def->{content}->[0];
> +	my $res = {};
> +
> +	foreach my $dir (PVE::Tools::split_list($value)) {
> +	    my ($c, $path) = parse_volume_id($dir);

Using `parse_volume_id` here can lead to confusing error messages.
Plus you verify each part below anyway, so IMO you can just do
    split(/:/, $dir, 2)

> +	    if (!$valid_content->{$c}) {
> +		warn "storage does not support content type '$c'\n";
> +		next;
> +	    } elsif (!verify_path($path, 1)) {
> +		warn "not a valid path: $path";
> +		next;
> +	    }
> +	    $res->{$c} = $path;
> +	}
> +
>  	return $res;
>      }
>  
> @@ -419,6 +452,9 @@ sub encode_value {
>      } elsif ($key eq 'content') {
>  	my $res = content_hash_to_string($value) || 'none';
>  	return $res;
> +    } elsif ($key eq 'dirs') {
> +	my $res = dirs_hash_to_string($value);
> +	return $res;
>      }
>  
>      return $value;
> @@ -610,12 +646,11 @@ sub get_subdir {
>      my $path = $scfg->{path};
>  
>      die "storage definition has no path\n" if !$path;
> +    die "unknown vtype '$vtype'\n" if !exists($vtype_subdirs->{$vtype});
>  
> -    my $subdir = $vtype_subdirs->{$vtype};
> -
> -    die "unknown vtype '$vtype'\n" if !defined($subdir);
> +    my $subdir = $scfg->{dirs}->{$vtype} // "/".$vtype_subdirs->{$vtype};
>  
> -    return "$path/$subdir";
> +    return $path.$subdir;
>  }
>  
>  sub filesystem_path {
> diff --git a/test/get_subdir_test.pm b/test/get_subdir_test.pm
> index 1e58350..26c08d5 100644
> --- a/test/get_subdir_test.pm
> +++ b/test/get_subdir_test.pm
> @@ -27,6 +27,13 @@ foreach my $type (keys %$vtype_subdirs) {
>      push @$tests, [ $scfg_with_path, $type, $path ];
>  }
>  
> +# creates additional tests for overrides
> +foreach my $type (keys %$vtype_subdirs) {
> +    my $override = "/${type}_override";
> +    my $scfg_with_override = { path => '/some/path', dirs => { $type => $override } };
> +    push @$tests, [ $scfg_with_override, $type, "$scfg_with_override->{path}$scfg_with_override->{dirs}->{$type}" ];
> +}
> +
>  plan tests => scalar @$tests;
>  
>  foreach my $tt (@$tests) {
> -- 
> 2.30.2