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

[Fabian Grünbichler]

On November 7, 2019 12:52 pm, Fabian Ebner wrote:
> 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
>> 
> 
> I noticed that the $scfg parameter in zfs_request is never used. Should 
> I clean that up? Otherwise I'd need to include the $scfg parameter in 
> the helper function as well, where it also isn't needed.

it's used by ZFS over iSCSI, where the plugin is derived from 
ZFSPoolPlugin, so it's not safe to remove ;)