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

[Thomas Lamprecht]

On 11/18/19 11:45 AM, Fabian Ebner wrote:
> When adding a zfspool storage with 'pvesm add' the mount point is now added
> automatically to the storage configuration if it can be determined.
> path() does not assume the default mountpoint anymore, fixing 2085.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
> 
> Changes from v3:
>     * create a new 'mounpoint' property instead of using 'path', since
>       'path' is used to check whether storage is filesystem based
> 

applied, but removed the useless check_format call, as it was silenced
by the eval anyway.. Lets assume that when the mountpoint property is
invalid, ZFS cannot mount it anyway..

>  PVE/Storage/ZFSPoolPlugin.pm   | 31 +++++++++++++++++++++++++++++--
>  test/run_test_zfspoolplugin.pl | 26 ++++++++++++++------------
>  2 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index b8adf1c..507795b 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 },
>      };
>  }
>  
> @@ -142,17 +147,39 @@ sub parse_volname {
>  
>  # virtual zfs methods (subclass can overwrite them)
>  
> +sub on_add_hook {
> +    my ($class, $storeid, $scfg, %param) = @_;
> +
> +    my $cfg_mountpoint = $scfg->{mountpoint};
> +    my $mountpoint;
> +
> +    # ignore failure, pool might currently not be imported
> +    eval {
> +	$mountpoint = $class->zfs_get_properties($scfg, 'mountpoint', $scfg->{pool}, 1);
> +	PVE::JSONSchema::check_format(properties()->{mountpoint}->{format}, $mountpoint);

.. ^^^^ was removed

> +    };
> +
> +    if (defined($cfg_mountpoint)) {
> +	if (defined($mountpoint) && !($cfg_mountpoint =~ m|^\Q$mountpoint\E/?$|)) {
> +	    warn "warning for $storeid - mountpoint: $cfg_mountpoint " .
> +		 "does not match current mount point: $mountpoint\n";
> +	}
> +    } else {
> +	$scfg->{mountpoint} = $mountpoint;
> +    }
> +}
> +
>  sub path {
>      my ($class, $scfg, $volname, $storeid, $snapname) = @_;
>  
>      my ($vtype, $name, $vmid) = $class->parse_volname($volname);
>  
>      my $path = '';
> +    my $mountpoint = $scfg->{mountpoint} // "/$scfg->{pool}";
>  
>      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";
>  	}