Re: [pve-devel] [PATCH v3 storage] deprecate mkdir option for create-path and create-sub-dirs

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 09/02/2023 um 14:18 schrieb Aaron Lauterer:
> The `mkdir` option has two meanings[0][1] which are split up in `create-path`
> and `create-sub-dirs`.
> 
> The `create-path` option decides if the path to the storage is
> automatically created or not.
> The `create-sub-dirs` 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.
> 
> [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046575.html
> [1] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046576.html
> 
> Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> This is the continuation of quite the old patch series [2]. We never
> applied it prior to the release of Proxmox VE 7. Hopefully we can get it
> into v8 so that we can think about dropping the 'mkdir' parameter in v9.
> 
> I added the T-b: Fabian Grünbichler [3] but feel free to remove it if
> you think that it has been too long ago :)

You added the R-b, not T-b, but that's actually the right one so OK. But you
added it first, but first should always be the authors S-o-b. I.e., trailers
should only be appended, not prefixed or the like.


But that's a nit and could be easily addressed on applying, and actually I
was close on doing so, but IMO it's a bit to late for this now, we should
rather add this early in the 8.x release cycle instead and then we could
actually think of removing it in 9.x - as it would be rather a to short
time span applying it now and dropping it for 9.0 already - especially as
dir based external plugins are affected by this too and dropping it might
even warrant an APIAGE reset.

Plus, I'd do the following changes on top:

diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 6fc1f27..560bb62 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -35,14 +35,13 @@ sub properties {
            type => 'string', format => 'pve-storage-path',
        },
        mkdir => {
-           description =>
-               "Deprecated, use the 'create-path' and " .
-               "'create-sub-dirs' options instead.",
+           description => "Create the directory if it doesn't exist and populate it with default sub-dirs."
+               ." NOTE: Deprecated, use the 'create-path' and 'create-sub-dirs' options instead.",
            type => 'boolean',
            default => 'yes',
        },
-       'create-path' => {
-           description => "Create the directory if it doesn't exist.",
+       'create-base-path' => {
+           description => "Create the base directory, if it doesn't exist.",
            type => 'boolean',
            default => 'yes',
        },

The s/create-path/create-base-path/ is obviously not finished, just to get the idea
over, and the should_create_path helper should be renamed too, as currently it's named
to generic and one would even expect it to take a path argument, as otherwise one asks
"create _what_ path?" should_create_base_path or directly doing a config_aware_base_mkdir
that also hosts the path creation directly (no hard feelings on that though)

Plus some adapting to newer style guideline stuff, e.g.:

diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 634f289..1674e36 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1365,8 +1365,7 @@ sub activate_storage {
     warn "${storeid}: 'mkdir' option is deprecated. Use 'create-path' or 'create-sub-dirs' instead.\n"
        if defined($scfg->{mkdir});
 
-    return if defined($scfg->{'create-sub-dirs'})
-       && !$scfg->{'create-sub-dirs'};
+    return if defined($scfg->{'create-sub-dirs'}) && !$scfg->{'create-sub-dirs'};
 
     # FIXME The mkdir option is deprecated and create-sub-dirs should be used
     return if defined($scfg->{mkdir}) && !$scfg->{mkdir};