Re: [PATCH storage v2 2/2] nfs: use rpcinfo as a showmounts fallback for nfs3

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Fiona Ebner <f.ebner@proxmox.com> writes:

> Am 19.03.26 um 11:42 AM schrieb Maximiliano Sandoval:
>> The check_connection call must succeed in order for the NFS storage to
>> be activated. In a setup in which the NFS server does not list exports
>> and which only supports NFSv3 this would result in the storage not being
>> usable (not without manually mounting the NFS share and using the
>> Directory storage instead). We add a fallback check with rpcinfo for
>> this case.
>> 
>> The rpcinfo check was replaced with showmounts in f8b0d82fe. At the time
>> the rationale was that rpcinfo did not have support for ipv6
>> connections, which is not the case at the moment.
>> 
>> In order to not break compatibility with setups that already activate
>> correctly, we add a rpcinfo check as a fallback for NFSv3-only setups
>> only after the showmounts command fails.
>> 
>> Potential pitfalls:
>> - Some NFSv3 servers might only support UDP [RFC 1813]
>> - The two timeouts add up to 20 seconds which might be too much
>> - Some NFS servers might not have rpcbind as described in acff89540
>> 
>> [RFC 1813]: https://datatracker.ietf.org/doc/html/rfc1813#section-2.3
>> 
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>  src/PVE/Storage/NFSPlugin.pm | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
>> index 062b3f32..0203c0a1 100644
>> --- a/src/PVE/Storage/NFSPlugin.pm
>> +++ b/src/PVE/Storage/NFSPlugin.pm
>> @@ -193,10 +193,12 @@ sub check_connection {
>>  
>>      my $cmd;
>>  
>> +    my $is_v3 = defined($opts) && $opts =~ /vers=3.*/;
>
> What if the version is not explicitly set? Shouldn't we still do the
> fallback then? If yes, we can drop the variable here and just do the
> fallback in the else branch (i.e. if !$is_v4) since the showmount
> command, for which the fallback is intended, also happens in the
> !$is_v4 branch.

For the sake of keeping the current state as much as possible I
intentionally made the fallback happen only if the storage is explicitly
configured with vers=3.

Is there a way to check whether the NFS storage only supports version 3?
one could use this instead as a check.

>>      my $is_v4 = defined($opts) && $opts =~ /vers=4.*/;
>>      if ($is_v4) {
>>          $cmd = tcp_rpcinfo_cmd($server, '4');
>>      } else {
>> +        # TODO Consider switching to rpcinfo altogether for Proxmox VE 10.
>
> Usually, these messages use 'PVE 10' as a prefix, e.g.
> # TODO PVE 10 - actual todo message

Noted.

>>          $cmd = ['/sbin/showmount', '--no-headers', '--exports', $server];
>>      }
>>  
>> @@ -213,6 +215,12 @@ sub check_connection {
>>              return 0 if $port == 0;
>>  
>>              return PVE::Network::tcp_ping($server, $port, 2);
>> +        } elsif ($is_v3) {
>> +            $cmd = tcp_rpcinfo_cmd($server, '3');
>> +            eval {
>> +                run_command($cmd, timeout => 10, outfunc => sub { }, errfunc => sub { });
>> +            };
>> +            return 1 if !$@;
>>          }
>>          return 0;
>>      }

-- 
Maximiliano