From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <a.lauterer@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; Mon,  4 Jan 2021 16:30:52 +0100 (CET)
From: Aaron Lauterer <a.lauterer@proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, 
 <mailto:pve-devel-request@lists.proxmox.com?subject=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 <a.lauterer@proxmox.com>

[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