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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 92B5E65D10 for ; Mon, 4 Jan 2021 16:31:26 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 848421EF65 for ; Mon, 4 Jan 2021 16:30:56 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (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 id 054311EF45 for ; Mon, 4 Jan 2021 16:30:53 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C41284506E for ; Mon, 4 Jan 2021 16:30:52 +0100 (CET) From: Aaron Lauterer To: pve-devel@lists.proxmox.com Date: Mon, 4 Jan 2021 16:30:51 +0100 Message-Id: <20210104153051.27815-1-a.lauterer@proxmox.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.021 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pve-devel] [PATCH storage] deprecate mkdir option for create_path and populate_directory 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: Mon, 04 Jan 2021 15:31:26 -0000 The `mkdir` option has two meanings[0][1] which are split up in `create_path` and `populate_directory`. The `create_path` option decides if the path to the storage is automatically created or not. The `populate_directory` 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. Signed-off-by: Aaron Lauterer [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046575.html [1] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046576.html --- To not be too spammy I only created a warning for mkdir in Plugin::activate_storage. I wasn't sure if this warrants a 'fix #3214' in the commit message as the fix is only a side effect. 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 be06cc7..5ee3190 100644 --- a/PVE/Storage/CIFSPlugin.pm +++ b/PVE/Storage/CIFSPlugin.pm @@ -142,6 +142,8 @@ sub options { domain => { optional => 1}, smbversion => { optional => 1}, mkdir => { optional => 1 }, + create_path => { optional => 1 }, + populate_directory => { optional => 1 }, bwlimit => { optional => 1 }, }; } @@ -225,7 +227,7 @@ sub activate_storage { if (!cifs_is_mounted($server, $share, $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/CephFSPlugin.pm b/PVE/Storage/CephFSPlugin.pm index 8eb7c70..3d6309d 100644 --- a/PVE/Storage/CephFSPlugin.pm +++ b/PVE/Storage/CephFSPlugin.pm @@ -147,6 +147,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + create_path => { optional => 1 }, + populate_directory => { optional => 1 }, fuse => { optional => 1 }, bwlimit => { optional => 1 }, maxfiles => { optional => 1 }, @@ -203,7 +205,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 2267f11..22bdeff 100644 --- a/PVE/Storage/DirPlugin.pm +++ b/PVE/Storage/DirPlugin.pm @@ -30,10 +30,22 @@ sub properties { type => 'string', format => 'pve-storage-path', }, mkdir => { + description => + "Deprecated, use the 'create_path' and " . + "'populate_directory' options instead.", + type => 'boolean', + default => 'yes', + }, + create_path => { description => "Create the directory if it doesn't exist.", type => 'boolean', default => 'yes', }, + populate_directory => { + description => "Populate the directory with the default structure.", + type => 'boolean', + default => 'yes', + }, is_mountpoint => { description => "Assume the given path is an externally managed mountpoint " . @@ -57,6 +69,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + create_path => { optional => 1 }, + populate_directory => { optional => 1 }, is_mountpoint => { optional => 1 }, bwlimit => { optional => 1 }, }; @@ -133,9 +147,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})) { @@ -143,6 +154,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 2dd414d..4ad632f 100644 --- a/PVE/Storage/GlusterfsPlugin.pm +++ b/PVE/Storage/GlusterfsPlugin.pm @@ -136,6 +136,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + create_path => { optional => 1 }, + populate_directory => { optional => 1 }, bwlimit => { optional => 1 }, }; } @@ -300,7 +302,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 e8e27c0..a32b707 100644 --- a/PVE/Storage/NFSPlugin.pm +++ b/PVE/Storage/NFSPlugin.pm @@ -87,6 +87,8 @@ sub options { content => { optional => 1 }, format => { optional => 1 }, mkdir => { optional => 1 }, + create_path => { optional => 1 }, + populate_directory => { optional => 1 }, bwlimit => { optional => 1 }, }; } @@ -129,7 +131,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 57c58a9..c77acb4 100644 --- a/PVE/Storage/Plugin.pm +++ b/PVE/Storage/Plugin.pm @@ -1158,7 +1158,13 @@ sub activate_storage { "directory '$path' does not exist or is unreachable\n"; } + warn "'mkdir' option is deprecated. Use 'create_path' or 'populate_directory' instead.\n" + if defined($scfg->{mkdir}); + return if defined($scfg->{populate_directory}) + && !$scfg->{populate_directory}; + + # FIXME The mkdir option is deprecated and populate_directory should be used return if defined($scfg->{mkdir}) && !$scfg->{mkdir}; if (defined($scfg->{content})) { @@ -1454,4 +1460,20 @@ sub volume_import_formats { return (); } +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})) { + $mkpath = 1 if $scfg->{create_path}; + } else { + $mkpath = 1 if $scfg->{mkdir}; + } + + return $mkpath; +} + 1; -- 2.20.1