From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id AFA809C2B3 for ; Wed, 31 May 2023 14:46:42 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 88592A173 for ; Wed, 31 May 2023 14:46:12 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 31 May 2023 14:46:10 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id AFD3F47ECA for ; Wed, 31 May 2023 14:46:10 +0200 (CEST) From: Aaron Lauterer To: pve-devel@lists.proxmox.com Date: Wed, 31 May 2023 14:46:09 +0200 Message-Id: <20230531124609.928686-1-a.lauterer@proxmox.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.344 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URI_NOVOWEL 0.5 URI hostname has long non-vowel sequence Subject: [pve-devel] [PATCH v4 storage] deprecate mkdir option for create-base-path and create-subdirs X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 May 2023 12:46:42 -0000 The `mkdir` option has two meanings[0][1] which are split up in `create-path` and `create-sub-dirs`. The `create-base-path` option decides if the path to the storage is automatically created or not. The `create-subdirs` 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-base-path` option is now run after the `is_mountpoint` check in the `activate_storage` method in DirPlugin.pm. The 'mkpath' command has been moved into a new helper function that first determines if the conditions to create the path is true, called 'config_aware_base_mkdir'. [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046575.html [1] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046576.html Signed-off-by: Aaron Lauterer Reviewed-by: Fabian Grünbichler --- changes since v3: renamed parameters: 'create-path' -> 'create-base-path' 'create-sub-dirs' -> 'create-subdirs' The latter due to @Wolfgang's suggestion. Renamed the 'should_create_path' function to 'config_aware_base_mkdir' and it now also creates the path it needed. Adapted the storage plugins to the changes and added them to the BTRFSPlugin. Some smaller style changes. I hope I hot them all. Did a quick test for all storage plugins: 1. add the storage disabled 2. add the 'mkdir 0' option 3. enable it -> check if nothing is created. 4. set 'mkdir 1' or remove it -> check if base path + subdirs are created 5. disable storage, rm folders and umount it if needed 6. add 'create-base-path 0' and 'create-subdirs 0' options 7. enable storage -> check, nothing should be created 8. set the 'create-base-path 1' option -> check if base dir is created, but empty 9. set 'create-subdirs 1' -> check if subdirs are created src/PVE/Storage/BTRFSPlugin.pm | 6 +++--- src/PVE/Storage/CIFSPlugin.pm | 4 +++- src/PVE/Storage/CephFSPlugin.pm | 4 +++- src/PVE/Storage/DirPlugin.pm | 19 +++++++++++++++---- src/PVE/Storage/GlusterfsPlugin.pm | 5 +++-- src/PVE/Storage/NFSPlugin.pm | 4 +++- src/PVE/Storage/Plugin.pm | 20 ++++++++++++++++++++ 7 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/PVE/Storage/BTRFSPlugin.pm b/src/PVE/Storage/BTRFSPlugin.pm index 1db4e4f..542f7f0 100644 --- a/src/PVE/Storage/BTRFSPlugin.pm +++ b/src/PVE/Storage/BTRFSPlugin.pm @@ -74,6 +74,8 @@ sub options { is_mountpoint => { optional => 1 }, nocow => { optional => 1 }, mkdir => { optional => 1 }, + 'create-base-path' => { optional => 1 }, + 'create-subdirs' => { optional => 1 }, preallocation => { optional => 1 }, # TODO: The new variant of mkdir with `populate` vs `create`... }; @@ -118,9 +120,7 @@ sub activate_storage { my ($class, $storeid, $scfg, $cache) = @_; my $path = $scfg->{path}; - if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) { - mkpath $path; - } + $class->config_aware_base_mkdir($scfg, $path); my $mp = PVE::Storage::DirPlugin::parse_is_mountpoint($scfg); if (defined($mp) && !PVE::Storage::DirPlugin::path_is_mounted($mp, $cache->{mountdata})) { diff --git a/src/PVE/Storage/CIFSPlugin.pm b/src/PVE/Storage/CIFSPlugin.pm index e03226d..e40641a 100644 --- a/src/PVE/Storage/CIFSPlugin.pm +++ b/src/PVE/Storage/CIFSPlugin.pm @@ -150,6 +150,8 @@ sub options { domain => { optional => 1}, smbversion => { optional => 1}, mkdir => { optional => 1 }, + 'create-base-path' => { optional => 1 }, + 'create-subdirs' => { 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}); + $class->config_aware_base_mkdir($scfg, $path); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/src/PVE/Storage/CephFSPlugin.pm b/src/PVE/Storage/CephFSPlugin.pm index db0c2f6..8aad518 100644 --- a/src/PVE/Storage/CephFSPlugin.pm +++ b/src/PVE/Storage/CephFSPlugin.pm @@ -147,6 +147,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-base-path' => { optional => 1 }, + 'create-subdirs' => { 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}); + $class->config_aware_base_mkdir($scfg, $path); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm index 9e305f5..37e322b 100644 --- a/src/PVE/Storage/DirPlugin.pm +++ b/src/PVE/Storage/DirPlugin.pm @@ -35,7 +35,18 @@ sub properties { type => 'string', format => 'pve-storage-path', }, mkdir => { - description => "Create the directory if it doesn't exist.", + description => "Create the directory if it doesn't exist and populate it with default sub-dirs." + ." NOTE: Deprecated, use the 'create-base-path' and 'create-subdirs' options instead.", + type => 'boolean', + default => 'yes', + }, + 'create-base-path' => { + description => "Create the base directory if it doesn't exist.", + type => 'boolean', + default => 'yes', + }, + 'create-subdirs' => { + description => "Populate the directory with the default structure.", type => 'boolean', default => 'yes', }, @@ -64,6 +75,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-base-path' => { optional => 1 }, + 'create-subdirs' => { optional => 1 }, is_mountpoint => { optional => 1 }, bwlimit => { optional => 1 }, preallocation => { optional => 1 }, @@ -213,9 +226,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 +233,7 @@ sub activate_storage { "directory is expected to be a mount point but is not mounted: '$mp'\n"; } + $class->config_aware_base_mkdir($scfg, $path); $class->SUPER::activate_storage($storeid, $scfg, $cache); } diff --git a/src/PVE/Storage/GlusterfsPlugin.pm b/src/PVE/Storage/GlusterfsPlugin.pm index ad386d2..2b7f9e1 100644 --- a/src/PVE/Storage/GlusterfsPlugin.pm +++ b/src/PVE/Storage/GlusterfsPlugin.pm @@ -137,6 +137,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-base-path' => { optional => 1 }, + 'create-subdirs' => { optional => 1 }, bwlimit => { optional => 1 }, preallocation => { optional => 1 }, }; @@ -302,8 +304,7 @@ sub activate_storage { my $volume = $scfg->{volume}; if (!glusterfs_is_mounted($volume, $path, $cache->{mountdata})) { - - mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); + $class->config_aware_base_mkdir($scfg, $path); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm index c2d4176..6e92bdd 100644 --- a/src/PVE/Storage/NFSPlugin.pm +++ b/src/PVE/Storage/NFSPlugin.pm @@ -91,6 +91,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + 'create-base-path' => { optional => 1 }, + 'create-subdirs' => { 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}); + $class->config_aware_base_mkdir($scfg, $path); die "unable to activate storage '$storeid' - " . "directory '$path' does not exist\n" if ! -d $path; diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm index c323085..34d830e 100644 --- a/src/PVE/Storage/Plugin.pm +++ b/src/PVE/Storage/Plugin.pm @@ -1362,7 +1362,12 @@ sub activate_storage { "directory '$path' does not exist or is unreachable\n"; } + warn "${storeid}: 'mkdir' option is deprecated. Use 'create-base-path' or 'create-subdirs' instead.\n" + if defined($scfg->{mkdir}); + return if defined($scfg->{'create-subdirs'}) && !$scfg->{'create-subdirs'}; + + # FIXME The mkdir option is deprecated. Remove with PVE 9? return if defined($scfg->{mkdir}) && !$scfg->{mkdir}; if (defined($scfg->{content})) { @@ -1698,4 +1703,19 @@ sub rename_volume { return "${storeid}:${base}${target_vmid}/${target_volname}"; } +sub config_aware_base_mkdir { + my ($class, $scfg, $path) = @_; + + # FIXME the mkdir parameter is deprecated and create-base-path should be used + my $mkpath = 0; + if (!defined($scfg->{'create-base-path'}) && !defined($scfg->{mkdir})) { + $mkpath = 1; + } elsif (defined($scfg->{'create-base-path'}) && $scfg->{'create-base-path'}) { + $mkpath = 1; + } elsif ($scfg->{mkdir}) { + $mkpath = 1; + } + mkpath $path if $mkpath; +} + 1; -- 2.30.2