[pve-devel] [PATCH storage docs] Allow overrides for default directories

[pve-devel] [PATCH storage docs] Allow overrides for default directories

[Leo Nunner]

This patch adds a parameter to the storage configuration that allows the
default directory structure to be overridden. For now, I allowed the
parameter for the directory, CIFS and NFS plugins.

I tested the following things:
    - Create VMs/CTs
    - Upload ISOs/container templates
    - Create/restore backups
    - Upload snippets
    - Move images between storages with different configurations

All of which posed no problems with the UI and were put into the correct
(custom) location. I also did migrations with a few different
configurations, which also worked correctly.

storage:

Leo Nunner (1):
  config: add overrides for default directory locations

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

docs:

Leo Nunner (1):
  docs: document 'dirs' parameter for directory storage

 pve-storage-cifs.adoc |  4 ++++
 pve-storage-dir.adoc  | 19 ++++++++++++++-----
 pve-storage-nfs.adoc  |  4 ++++
 3 files changed, 22 insertions(+), 5 deletions(-)

-- 
2.30.2

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

[Leo Nunner]

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

[pve-devel] [PATCH docs] docs: document 'dirs' parameter for directory storage

[Leo Nunner]

Signed-off-by: Leo Nunner <l.nunner@proxmox.com>
---
 pve-storage-cifs.adoc |  4 ++++
 pve-storage-dir.adoc  | 19 ++++++++++++++-----
 pve-storage-nfs.adoc  |  4 ++++
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/pve-storage-cifs.adoc b/pve-storage-cifs.adoc
index bb4b902..4d93727 100644
--- a/pve-storage-cifs.adoc
+++ b/pve-storage-cifs.adoc
@@ -57,6 +57,10 @@ path::
 
 The local mount point. Optional, defaults to `/mnt/pve/<STORAGE_ID>/`.
 
+dirs::
+
+Overrides for the default directory layout. Optional.
+
 .Configuration Example (`/etc/pve/storage.cfg`)
 ----
 cifs: backup
diff --git a/pve-storage-dir.adoc b/pve-storage-dir.adoc
index 4116fe5..ec1b957 100644
--- a/pve-storage-dir.adoc
+++ b/pve-storage-dir.adoc
@@ -46,9 +46,18 @@ storage backends.
 Configuration
 ~~~~~~~~~~~~~
 
-This backend supports all common storage properties, and adds an
-additional property called `path` to specify the directory. This
-needs to be an absolute file system path.
+This backend supports all common storage properties, and adds two
+additional properties. The `path` property is used to specify the
+directory. This needs to be an absolute file system path.
+
+The optional `dirs` property allows for the default layout to be
+changed. It consists of a comma-separated list of identifiers in
+the following format:
+
+ vtype:path
+
+Where `vtype` is one of the allowed content types for the storage, and
+`path` is an absolute file system path.
 
 .Configuration Example (`/etc/pve/storage.cfg`)
 ----
@@ -57,12 +66,12 @@ dir: backup
         content backup
         prune-backups keep-last=7
         max-protected-backups 3
+        dirs backup:/custom/backup/dir
 ----
 
 The above configuration defines a storage pool called `backup`. That pool can be
 used to store up to 7 regular backups (`keep-last=7`) and 3 protected backups
-per VM. The real path for the backup files is `/mnt/backup/dump/...`.
-
+per VM. The real path for the backup files is `/mnt/backup/custom/backup/dir/...`.
 
 File naming conventions
 ~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/pve-storage-nfs.adoc b/pve-storage-nfs.adoc
index 9a90057..95ee45d 100644
--- a/pve-storage-nfs.adoc
+++ b/pve-storage-nfs.adoc
@@ -40,6 +40,10 @@ path::
 
 The local mount point (defaults to `/mnt/pve/<STORAGE_ID>/`).
 
+dirs::
+
+Overrides for the default directory layout. Optional.
+
 options::
 
 NFS mount options (see `man nfs`).
-- 
2.30.2

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

[Wolfgang Bumiller]

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