The `mkdir` option has two meanings[0][1] which are split up in `create-path` and `create-sub-dirs`.
The `create-path` option decides if the path to the storage is automatically created or not. The `create-sub-dirs` options decides if the default directory structure (dump, images, ...) at the storage location is created. The `mkdir` option is still working but will trigger a warning in the logs. As a side effect, this also fixes #3214 because the `create-path` option is now run after the `is_mountpoint` check in the `activate_storage` method in DirPlugin.pm. [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046575.html [1] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046576.html Reviewed-by: Fabian Grünbichler <f.gruenbich...@proxmox.com> Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com> --- This is the continuation of quite the old patch series [2]. We never applied it prior to the release of Proxmox VE 7. Hopefully we can get it into v8 so that we can think about dropping the 'mkdir' parameter in v9. I added the T-b: Fabian Grünbichler [3] but feel free to remove it if you think that it has been too long ago :) changes from v2: * rebased * reworked if conditions in 'should_create_path' for better readability * added ${storeid} to deprecation warning v1: (thx @fabian for the review [3]) * adjusted parameters to use - instead of _ * renamed populate_directory with create-sub-dirs One thing we should also check is how to handle default storages, mainly `local` which AFAICT has `mkdir 1` set which will trigger deprecation warnings in the syslog. [2] https://lists.proxmox.com/pipermail/pve-devel/2021-May/048381.html [3] https://lists.proxmox.com/pipermail/pve-devel/2021-April/047880.html PVE/Storage/CIFSPlugin.pm | 4 +++- PVE/Storage/CephFSPlugin.pm | 4 +++- PVE/Storage/DirPlugin.pm | 19 ++++++++++++++++--- PVE/Storage/GlusterfsPlugin.pm | 4 +++- PVE/Storage/NFSPlugin.pm | 4 +++- PVE/Storage/Plugin.pm | 22 ++++++++++++++++++++++ 6 files changed, 50 insertions(+), 7 deletions(-) diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm index 6e20f4b..506f55c 100644 --- a/PVE/Storage/CIFSPlugin.pm +++ b/PVE/Storage/CIFSPlugin.pm @@ -150,6 +150,8 @@ sub options { domain => { optional => 1}, smbversion => { optional => 1}, mkdir => { optional => 1 }, + 'create-path' => { optional => 1 }, + 'create-sub-dirs' => { optional => 1 }, bwlimit => { optional => 1 }, preallocation => { optional => 1 }, }; @@ -228,7 +230,7 @@ sub activate_storage { if (!cifs_is_mounted($scfg, $cache->{mountdata})) { - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); + mkpath $path if $class->should_create_path($scfg); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/PVE/Storage/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm index 944f0b8..415044a 100644 --- a/PVE/Storage/CephFSPlugin.pm +++ b/PVE/Storage/CephFSPlugin.pm @@ -146,6 +146,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-path' => { optional => 1 }, + 'create-sub-dirs' => { optional => 1 }, fuse => { optional => 1 }, bwlimit => { optional => 1 }, maxfiles => { optional => 1 }, @@ -214,7 +216,7 @@ sub activate_storage { if (!cephfs_is_mounted($scfg, $storeid, $cache->{mountdata})) { my $path = $scfg->{path}; - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); + mkpath $path if $class->should_create_path($scfg); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm index 9e305f5..6fc1f27 100644 --- a/PVE/Storage/DirPlugin.pm +++ b/PVE/Storage/DirPlugin.pm @@ -35,10 +35,22 @@ sub properties { type => 'string', format => 'pve-storage-path', }, mkdir => { + description => + "Deprecated, use the 'create-path' and " . + "'create-sub-dirs' options instead.", + type => 'boolean', + default => 'yes', + }, + 'create-path' => { description => "Create the directory if it doesn't exist.", type => 'boolean', default => 'yes', }, + 'create-sub-dirs' => { + description => "Populate the directory with the default structure.", + type => 'boolean', + default => 'yes', + }, is_mountpoint => { description => "Assume the given path is an externally managed mountpoint " . @@ -64,6 +76,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-path' => { optional => 1 }, + 'create-sub-dirs' => { optional => 1 }, is_mountpoint => { optional => 1 }, bwlimit => { optional => 1 }, preallocation => { optional => 1 }, @@ -213,9 +227,6 @@ sub activate_storage { my ($class, $storeid, $scfg, $cache) = @_; my $path = $scfg->{path}; - if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) { - mkpath $path; - } my $mp = parse_is_mountpoint($scfg); if (defined($mp) && !path_is_mounted($mp, $cache->{mountdata})) { @@ -223,6 +234,8 @@ sub activate_storage { "directory is expected to be a mount point but is not mounted: '$mp'\n"; } + mkpath $path if $class->should_create_path($scfg); + $class->SUPER::activate_storage($storeid, $scfg, $cache); } diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm index ad386d2..302fac0 100644 --- a/PVE/Storage/GlusterfsPlugin.pm +++ b/PVE/Storage/GlusterfsPlugin.pm @@ -137,6 +137,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-path' => { optional => 1 }, + 'create-sub-dirs' => { optional => 1 }, bwlimit => { optional => 1 }, preallocation => { optional => 1 }, }; @@ -303,7 +305,7 @@ sub activate_storage { if (!glusterfs_is_mounted($volume, $path, $cache->{mountdata})) { - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); + mkpath $path if $class->should_create_path($scfg); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm index c2d4176..5c7bd99 100644 --- a/PVE/Storage/NFSPlugin.pm +++ b/PVE/Storage/NFSPlugin.pm @@ -91,6 +91,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-path' => { optional => 1 }, + 'create-sub-dirs' => { optional => 1 }, bwlimit => { optional => 1 }, preallocation => { optional => 1 }, }; @@ -134,7 +136,7 @@ sub activate_storage { if (!nfs_is_mounted($server, $export, $path, $cache->{mountdata})) { # NOTE: only call mkpath when not mounted (avoid hang when NFS server is offline - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); + mkpath $path if $class->should_create_path($scfg); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm index ca7a0d4..87d4358 100644 --- a/PVE/Storage/Plugin.pm +++ b/PVE/Storage/Plugin.pm @@ -1357,7 +1357,13 @@ sub activate_storage { "directory '$path' does not exist or is unreachable\n"; } + warn "${storeid}: 'mkdir' option is deprecated. Use 'create-path' or 'create-sub-dirs' instead.\n" + if defined($scfg->{mkdir}); + return if defined($scfg->{'create-sub-dirs'}) + && !$scfg->{'create-sub-dirs'}; + + # FIXME The mkdir option is deprecated and create-sub-dirs should be used return if defined($scfg->{mkdir}) && !$scfg->{mkdir}; if (defined($scfg->{content})) { @@ -1693,4 +1699,20 @@ sub rename_volume { return "${storeid}:${base}${target_vmid}/${target_volname}"; } +sub should_create_path { + my ($class, $scfg) = @_; + + # FIXME the mkdir parameter is deprecated and create_path should be used + my $mkpath = 0; + if (!defined($scfg->{'create-path'}) && !defined($scfg->{mkdir})) { + $mkpath = 1; + } elsif (defined($scfg->{'create-path'}) && $scfg->{'create-path'}) { + $mkpath = 1; + } elsif ($scfg->{mkdir}) { + $mkpath = 1; + } + + return $mkpath; +} + 1; -- 2.30.2 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
