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

[pve-devel] [PATCH v2 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.

Changes from v1:
    - Change parameter verification from pve-volume-id to a custom
      format that also verifies the validity of the path and the given
      vtype.

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     | 49 +++++++++++++++++++++++++++++++++++----
 test/get_subdir_test.pm   |  7 ++++++
 5 files changed, 55 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 v2 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     | 49 +++++++++++++++++++++++++++++++++++----
 test/get_subdir_test.pm   |  7 ++++++
 5 files changed, 55 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..5cb3d90 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-dir-override-list",
+	    optional => 1,
+	},
     },
 };
 
@@ -205,6 +210,12 @@ sub valid_content_types {
     return $def->{content}->[0];
 }
 
+sub dirs_hash_to_string {
+    my $hash = shift;
+
+    return join(',', map { "$_:$hash->{$_}" } sort keys %$hash);
+}
+
 sub default_format {
     my ($scfg) = @_;
 
@@ -335,6 +346,18 @@ sub parse_volume_id {
     die "unable to parse volume ID '$volid'\n";
 }
 
+PVE::JSONSchema::register_format('pve-dir-override', \&verify_dir_override);
+sub verify_dir_override {
+    my ($value, $noerr) = @_;
+
+    if($value =~ m/^([a-z]+):(.+)$/ &&
+	verify_content($1, $noerr) && verify_path($2, $noerr)) {
+	return $value;
+    }
+
+    return undef if $noerr;
+    die "invalid override '$value'\n";
+}
 
 sub private {
     return $defaultData;
@@ -405,6 +428,22 @@ 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 ($content, $path) = split(/:/, $dir, 2);
+
+	    if (!$valid_content->{$content}) {
+		warn "storage does not support content type '$content'\n";
+		next;
+	    }
+
+	    $res->{$content} = $path;
+	}
+
 	return $res;
     }
 
@@ -419,6 +458,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 +652,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 v2 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

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

[Wolfgang Bumiller]

applied, thanks

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

[Wolfgang Bumiller]

applied, thanks

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

[Wolfgang Bumiller]

Actually, I just realized that these are actually *relative* paths
written "as absolute" paths, and if not, the error will now actually say
that it is not an absolute path as well.

Also the docs say absolute paths, but they are not.

So please add a followup which drops the use of `verify_path` (rather,
just allow all values or perhaps forbid it containing '..' (without even
bothering checking for slashes around).

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

[Thomas Lamprecht]

Am 05/01/2023 um 14:41 schrieb Wolfgang Bumiller:
> Actually, I just realized that these are actually *relative* paths

yeah that's the idea, relative paths from the storage MP.

> written "as absolute" paths, and if not, the error will now actually say
> that it is not an absolute path as well.

cosmetic, but yeah. What I rather find odd is the separation format, why
don't we use the quoted string format that the maintenance stuff in PBS
added?

> 
> Also the docs say absolute paths, but they are not.
> 
> So please add a followup which drops the use of `verify_path` (rather,
> just allow all values or perhaps forbid it containing '..' (without even
> bothering checking for slashes around).

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

[Thomas Lamprecht]

Am 02/01/2023 um 17:04 schrieb 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     | 49 +++++++++++++++++++++++++++++++++++----
>  test/get_subdir_test.pm   |  7 ++++++
>  5 files changed, 55 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..5cb3d90 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -181,6 +181,11 @@ my $defaultData = {
>  	    default => 'metadata',
>  	    optional => 1,
>  	},
> +	dirs => {

"content-dirs" for more clarity?

> +	    description => "Overrides for default directories",

"[...] default content type directories"

> +	    type => "string", format => "pve-dir-override-list",

please also add the default value here

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

[Wolfgang Bumiller]

On Thu, Jan 05, 2023 at 02:53:41PM +0100, Thomas Lamprecht wrote:
> Am 05/01/2023 um 14:41 schrieb Wolfgang Bumiller:
> > Actually, I just realized that these are actually *relative* paths
> 
> yeah that's the idea, relative paths from the storage MP.
> 
> > written "as absolute" paths, and if not, the error will now actually say
> > that it is not an absolute path as well.
> 
> cosmetic, but yeah. What I rather find odd is the separation format, why
> don't we use the quoted string format that the maintenance stuff in PBS
> added?

We just had a short off-list discussion about the formatting and will
also switch it to `=` instead of `:` to separate the vtype from the path
so it works more like a property string (since we don't usually use
`key:value` and it would also in theory allow switching to an actual
object/property-string based schema later on).

As for quoting, do we have this in PVE yet?
But I don't very much mind not being able to use any of ';', '=' or ','
for paths for the initial implementation.