[pve-devel] [PATCH storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property

[Fabian Grünbichler]

On November 6, 2019 1:46 pm, Fabian Ebner wrote:
> A new mountpoint property is added to the schema for ZFSPool storages.
> When needed for the first time, the current mount point is determined and
> written to the storage config.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
>  PVE/Storage/ZFSPoolPlugin.pm | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index 16fb0d6..44c8123 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -32,6 +32,10 @@ sub properties {
>  	    description => "use sparse volumes",
>  	    type => 'boolean',
>  	},
> +	mountpoint => {
> +	    description => "mount point",
> +	    type => 'string', format => 'pve-storage-path',
> +	},
>      };
>  }
>  
> @@ -44,6 +48,7 @@ sub options {
>  	disable => { optional => 1 },
>  	content => { optional => 1 },
>  	bwlimit => { optional => 1 },
> +	mountpoint => { optional => 1 },
>      };
>  }
>  
> @@ -148,11 +153,27 @@ sub path {
>      my ($vtype, $name, $vmid) = $class->parse_volname($volname);
>  
>      my $path = '';
> +    my $mountpoint = $scfg->{mountpoint};
> +
> +    # automatically write mountpoint to storage config if it is not present
> +    if (!$mountpoint) {
> +	$mountpoint = $class->zfs_request($scfg, undef, 'get', '-o', 'value',
> +					  'mountpoint', '-H', $scfg->{pool});
> +	chomp($mountpoint);
> +
> +	eval {
> +	    PVE::Storage::lock_storage_config(sub {
> +		my $cfg = PVE::Storage::config();
> +		$cfg->{ids}->{$storeid}->{mountpoint} = $mountpoint;
> +		PVE::Storage::write_config($cfg);
> +	    }, "update storage config failed");
> +	};
> +	warn $@ if $@;
> +    }
>  
>      if ($vtype eq "images") {
>  	if ($name =~ m/^subvol-/ || $name =~ m/^basevol-/) {
> -	    # fixme: we currently assume standard mount point?!
> -	    $path = "/$scfg->{pool}/$name";
> +	    $path = "$mountpoint/$name";
>  	} else {
>  	    $path = "/dev/zvol/$scfg->{pool}/$name";
>  	}

it would be great if this "get mountpoint property of dataset" (or better: 
even "get arbitrary properties of dataset") helper could be factored out. 
we already have two calls to "zfs get -Hp '...' $dataset" that could 
re-use such a helper.

then we could add a single check in activate_storage (maybe just for the 
branch where we actually just imported the pool?) to verify that the 
stored mountpoint value is correct, and if not update it (or warn about 
the problem). that check could have a very low timeout, and errors could
be ignored since it is just a double-check.