* [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
* Re: [pve-devel] [PATCH storage] deprecate mkdir option for create_path and populate_directory
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
0 siblings, 1 reply; 3+ messages in thread
From: Fabian Grünbichler @ 2021-04-23 7:08 UTC (permalink / raw)
To: Proxmox VE development discussion
On January 4, 2021 4:30 pm, Aaron Lauterer wrote:
> 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>
Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
with two nits that could be addressed as follow-up or folded in, see
below
>
> [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 },
we have a preference for new parameters/properties to not use
underscores, and the storage.cfg already has both (is_mountpoint and
prune-backups), but we might want to fix this up before adding more
underscores ;)
I am not 100% sure about the naming either, since both options do very
similar stuff (one creates the top level dir, the other the sub dirs).
maybe
create-path
create-sub-dirs
is more straightforward? (and a bit shorter for the latter as well ;))
> 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});
this warning here would benefit from having '$storeid: ' as prefix.
>
> + 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
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH storage] deprecate mkdir option for create_path and populate_directory
2021-04-23 7:08 ` Fabian Grünbichler
@ 2021-04-26 13:13 ` Aaron Lauterer
0 siblings, 0 replies; 3+ messages in thread
From: Aaron Lauterer @ 2021-04-26 13:13 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
On 4/23/21 9:08 AM, Fabian Grünbichler wrote:
> On January 4, 2021 4:30 pm, Aaron Lauterer wrote:
>> 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>
>
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>
> with two nits that could be addressed as follow-up or folded in, see
> below
>
>>
>> [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 },
>
> we have a preference for new parameters/properties to not use
> underscores, and the storage.cfg already has both (is_mountpoint and
> prune-backups), but we might want to fix this up before adding more
> underscores ;)
good point!
>
> I am not 100% sure about the naming either, since both options do very
> similar stuff (one creates the top level dir, the other the sub dirs).
> maybe
>
> create-path
> create-sub-dirs
>
> is more straightforward? (and a bit shorter for the latter as well ;))
`create-sub-dirs` sounds also a bit funny to me, and `create-sub-directories` is also quite long...
Any other opinions?
>
>> 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});
>
> this warning here would benefit from having '$storeid: ' as prefix.
Thanks for catching this! As I think I will send in a v2 of this patch I will add it.
>
>>
>> + 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
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
^ 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