[pve-devel] [PATCH storage] fix 3214: DirPlugin: die early if mountpoint not mounted

[pve-devel] [PATCH storage] fix 3214: DirPlugin: die early if mountpoint not mounted

[Aaron Lauterer]

If the `is_mountpoint` option is enabled, it is sensible to first check
for the mountpoint before proceeding any further in the storage
activation.

The old behaviour would create the path to the mountpoint if the `mdkir`
option wasn't set to false. This resulted in mountpoints further up
the path to not mount anymore, e.g. nested ZFS datasets.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

There is another patch[0] already for this bug report but it deals with
a related, but different problem of not creating the internal storage
directory structure if `mkdir 0` was set.

[0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046572.html

 PVE/Storage/DirPlugin.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
index 2267f11..81aaf2b 100644
--- a/PVE/Storage/DirPlugin.pm
+++ b/PVE/Storage/DirPlugin.pm
@@ -132,17 +132,17 @@ sub status {
 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})) {
 	die "unable to activate storage '$storeid' - " .
 	    "directory is expected to be a mount point but is not mounted: '$mp'\n";
     }
 
+    my $path = $scfg->{path};
+    if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) {
+	mkpath $path;
+    }
+
     $class->SUPER::activate_storage($storeid, $scfg, $cache);
 }
 
-- 
2.20.1

Re: [pve-devel] [PATCH storage] fix 3214: DirPlugin: die early if mountpoint not mounted

[Wolfgang Bumiller]

Here our `mkdir` meaning is `create the path to the storage`.

I'd propose the following:

Apply neither #3214 patch. Keep the `mkdir` code as it is. Deprecate the
`mkdir` option and split it up into 2 new options:

`create_path` and `populate_directory`

It should be harder to confuse those two in the future (plus they can
both be set explicitly simultaneously).

On Tue, Dec 22, 2020 at 01:18:52PM +0100, Aaron Lauterer wrote:
> If the `is_mountpoint` option is enabled, it is sensible to first check
> for the mountpoint before proceeding any further in the storage
> activation.
> 
> The old behaviour would create the path to the mountpoint if the `mdkir`
> option wasn't set to false. This resulted in mountpoints further up
> the path to not mount anymore, e.g. nested ZFS datasets.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> There is another patch[0] already for this bug report but it deals with
> a related, but different problem of not creating the internal storage
> directory structure if `mkdir 0` was set.
> 
> [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046572.html
> 
>  PVE/Storage/DirPlugin.pm | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index 2267f11..81aaf2b 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -132,17 +132,17 @@ sub status {
>  sub activate_storage {
>      my ($class, $storeid, $scfg, $cache) = @_;
>  
> -    my $path = $scfg->{path};
> -    if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) {
> -	mkpath $path;
> -    }

Depending on whether we want to also "fix" the defaults (`mkdir` not
present meaning it is not *used*, rather than it is `on`), the above
should either happen `if !defined($scfg->{create_path})` (not explicitly
using `create_path`), or changed to `if ($scfg->{mkdir})` (which should
generate a warning).

> -
>      my $mp = parse_is_mountpoint($scfg);
>      if (defined($mp) && !path_is_mounted($mp, $cache->{mountdata})) {
>  	die "unable to activate storage '$storeid' - " .
>  	    "directory is expected to be a mount point but is not mounted: '$mp'\n";
>      }
>  

the `is_mountpoint` check would stay here after `mkdir` handling for
legacy behavior.
and below here is where we'd add the hunk below with
`s/mkdir/populate_directory/`;

The other patch would do use `populate_directory` if it is explicitly
set, otherwise fall through to the old `mkdir` based code with a
warning that `populate_directory` should be explicitly set in the
config.


> +    my $path = $scfg->{path};
> +    if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) {
> +	mkpath $path;
> +    }
> +
>      $class->SUPER::activate_storage($storeid, $scfg, $cache);
>  }
>  
> -- 
> 2.20.1

Re: [pve-devel] [PATCH storage] fix 3214: DirPlugin: die early if mountpoint not mounted

[Aaron Lauterer]

On 12/22/20 2:39 PM, Wolfgang Bumiller wrote:
> Here our `mkdir` meaning is `create the path to the storage`.
> 
> I'd propose the following:
> 
> Apply neither #3214 patch. Keep the `mkdir` code as it is. Deprecate the
> `mkdir` option and split it up into 2 new options:
> 
> `create_path` and `populate_directory`
> 
> It should be harder to confuse those two in the future (plus they can
> both be set explicitly simultaneously).

sounds good!

> 
> On Tue, Dec 22, 2020 at 01:18:52PM +0100, Aaron Lauterer wrote:
>> If the `is_mountpoint` option is enabled, it is sensible to first check
>> for the mountpoint before proceeding any further in the storage
>> activation.
>>
>> The old behaviour would create the path to the mountpoint if the `mdkir`
>> option wasn't set to false. This resulted in mountpoints further up
>> the path to not mount anymore, e.g. nested ZFS datasets.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>> ---
>>
>> There is another patch[0] already for this bug report but it deals with
>> a related, but different problem of not creating the internal storage
>> directory structure if `mkdir 0` was set.
>>
>> [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046572.html
>>
>>   PVE/Storage/DirPlugin.pm | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
>> index 2267f11..81aaf2b 100644
>> --- a/PVE/Storage/DirPlugin.pm
>> +++ b/PVE/Storage/DirPlugin.pm
>> @@ -132,17 +132,17 @@ sub status {
>>   sub activate_storage {
>>       my ($class, $storeid, $scfg, $cache) = @_;
>>   
>> -    my $path = $scfg->{path};
>> -    if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) {
>> -	mkpath $path;
>> -    }
> 
> Depending on whether we want to also "fix" the defaults (`mkdir` not
> present meaning it is not *used*, rather than it is `on`), the above
> should either happen `if !defined($scfg->{create_path})` (not explicitly
> using `create_path`), or changed to `if ($scfg->{mkdir})` (which should
> generate a warning).

AFAIU this sounds like a breaking change and should probably only be done on a major version release. If we actually want to change that.

> 
>> -
>>       my $mp = parse_is_mountpoint($scfg);
>>       if (defined($mp) && !path_is_mounted($mp, $cache->{mountdata})) {
>>   	die "unable to activate storage '$storeid' - " .
>>   	    "directory is expected to be a mount point but is not mounted: '$mp'\n";
>>       }
>>   
> 
> the `is_mountpoint` check would stay here after `mkdir` handling for
> legacy behavior.
> and below here is where we'd add the hunk below with
> `s/mkdir/populate_directory/`;
> 
> The other patch would do use `populate_directory` if it is explicitly
> set, otherwise fall through to the old `mkdir` based code with a
> warning that `populate_directory` should be explicitly set in the
> config.
> 
> 
>> +    my $path = $scfg->{path};
>> +    if (!defined($scfg->{mkdir}) || $scfg->{mkdir}) {
>> +	mkpath $path;
>> +    }
>> +
>>       $class->SUPER::activate_storage($storeid, $scfg, $cache);
>>   }
>>   
>> -- 
>> 2.20.1