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

[Fabian Ebner]

On 11/7/19 9:34 AM, Fabian Grünbichler wrote:
> 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.
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 

Yes, that does sound like a better place to do it.
I'll work on a v3 with your suggestions, so just ignore the v2.