all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH storage] deprecate mkdir option for create_path and populate_directory
@ 2021-01-04 15:30 Aaron Lauterer
  2021-04-23  7:08 ` Fabian Grünbichler
  0 siblings, 1 reply; 3+ messages in thread
From: Aaron Lauterer @ 2021-01-04 15:30 UTC (permalink / raw)
  To: pve-devel

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





^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-26 13:14 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-04 15:30 [pve-devel] [PATCH storage] deprecate mkdir option for create_path and populate_directory Aaron Lauterer
2021-04-23  7:08 ` Fabian Grünbichler
2021-04-26 13:13   ` Aaron Lauterer

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