* [pve-devel] [PATCH storage docs] Allow overrides for default directories
@ 2022-12-15 11:21 Leo Nunner
2022-12-15 11:21 ` [pve-devel] [PATCH storage] config: add overrides for default directory locations Leo Nunner
2022-12-15 11:21 ` [pve-devel] [PATCH docs] docs: document 'dirs' parameter for directory storage Leo Nunner
0 siblings, 2 replies; 4+ messages in thread
From: Leo Nunner @ 2022-12-15 11:21 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* [pve-devel] [PATCH storage] config: add overrides for default directory locations 2022-12-15 11:21 [pve-devel] [PATCH storage docs] Allow overrides for default directories Leo Nunner @ 2022-12-15 11:21 ` Leo Nunner 2022-12-16 10:29 ` Wolfgang Bumiller 2022-12-15 11:21 ` [pve-devel] [PATCH docs] docs: document 'dirs' parameter for directory storage Leo Nunner 1 sibling, 1 reply; 4+ messages in thread From: Leo Nunner @ 2022-12-15 11:21 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH storage] config: add overrides for default directory locations 2022-12-15 11:21 ` [pve-devel] [PATCH storage] config: add overrides for default directory locations Leo Nunner @ 2022-12-16 10:29 ` Wolfgang Bumiller 0 siblings, 0 replies; 4+ messages in thread From: Wolfgang Bumiller @ 2022-12-16 10:29 UTC (permalink / raw) To: Leo Nunner; +Cc: pve-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [pve-devel] [PATCH docs] docs: document 'dirs' parameter for directory storage 2022-12-15 11:21 [pve-devel] [PATCH storage docs] Allow overrides for default directories Leo Nunner 2022-12-15 11:21 ` [pve-devel] [PATCH storage] config: add overrides for default directory locations Leo Nunner @ 2022-12-15 11:21 ` Leo Nunner 1 sibling, 0 replies; 4+ messages in thread From: Leo Nunner @ 2022-12-15 11:21 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-16 10:29 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-15 11:21 [pve-devel] [PATCH storage docs] Allow overrides for default directories Leo Nunner 2022-12-15 11:21 ` [pve-devel] [PATCH storage] config: add overrides for default directory locations Leo Nunner 2022-12-16 10:29 ` Wolfgang Bumiller 2022-12-15 11:21 ` [pve-devel] [PATCH docs] docs: document 'dirs' parameter for directory storage Leo Nunner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.Service provided by Proxmox Server Solutions GmbH | Privacy | Legal