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

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

[Maximiliano Sandoval]

See the respective commit messages.

This came up in enterprise support. In this case the NFS appliance only
supported NFSv3 and the showmounts command failed, but rpcbind
succeeded (with -T tcp).

This possibility was discussed with Friedrich and Fiona off-list.

Tested:
 - Created a NFS share on host "B"
 - On host "A" a NFS storage was added (using B's share) with --options vers=3
 - Ran `ss -tulpn4 |grep mountd` on host B to see ports used by showmounts
 - The ports listed were blocked on host "A", so that showmount fails:

   /sbin/showmount --no-headers --exports $B
   rpc mount export: RPC: Unable to send; errno = Operation not permitted

 - Ran `pvesm status` on A and checked that the NFS storage is available
 - Created a VM on A with a disk image in the the NFS storage

The later two operations would have failed without the changes here.

Differences from v1:
 - the name of the helper function was fixed

Maximiliano Sandoval (2):
  nfs: create a helper to run rpcinfo commands
  nfs: use rpcinfo as a showmounts fallback for nfs3

 src/PVE/Storage/NFSPlugin.pm | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

-- 
2.47.3

[PATCH storage v2 1/2] nfs: create a helper to run rpcinfo commands

[Maximiliano Sandoval]

Will be used in the next command as a fallback in the NFSv3 case.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/PVE/Storage/NFSPlugin.pm | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
index 4cc02c9e..062b3f32 100644
--- a/src/PVE/Storage/NFSPlugin.pm
+++ b/src/PVE/Storage/NFSPlugin.pm
@@ -171,6 +171,20 @@ sub deactivate_storage {
     }
 }
 
+sub tcp_rpcinfo_cmd {
+    my ($server, $version) = @_;
+
+    my $ip = PVE::JSONSchema::pve_verify_ip($server, 1);
+    if (!defined($ip)) {
+        $ip = PVE::Network::get_ip_from_hostname($server);
+    }
+
+    my $transport = PVE::JSONSchema::pve_verify_ipv4($ip, 1) ? 'tcp' : 'tcp6';
+
+    # nfsv4 uses a pseudo-filesystem always beginning with / no exports are listed
+    return ['/usr/sbin/rpcinfo', '-T', $transport, $ip, 'nfs', $version];
+}
+
 sub check_connection {
     my ($class, $storeid, $scfg) = @_;
 
@@ -181,16 +195,7 @@ sub check_connection {
 
     my $is_v4 = defined($opts) && $opts =~ /vers=4.*/;
     if ($is_v4) {
-        my $ip = PVE::JSONSchema::pve_verify_ip($server, 1);
-        if (!defined($ip)) {
-            $ip = PVE::Network::get_ip_from_hostname($server);
-        }
-
-        my $transport = PVE::JSONSchema::pve_verify_ipv4($ip, 1) ? 'tcp' : 'tcp6';
-
-        # nfsv4 uses a pseudo-filesystem always beginning with /
-        # no exports are listed
-        $cmd = ['/usr/sbin/rpcinfo', '-T', $transport, $ip, 'nfs', '4'];
+        $cmd = tcp_rpcinfo_cmd($server, '4');
     } else {
         $cmd = ['/sbin/showmount', '--no-headers', '--exports', $server];
     }
-- 
2.47.3

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

[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.*/;
     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.
         $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;
     }
-- 
2.47.3

Re: [PATCH storage v2 1/2] nfs: create a helper to run rpcinfo commands

[Fiona Ebner]

Am 19.03.26 um 11:42 AM schrieb Maximiliano Sandoval:
> Will be used in the next command as a fallback in the NFSv3 case.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/PVE/Storage/NFSPlugin.pm | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
> index 4cc02c9e..062b3f32 100644
> --- a/src/PVE/Storage/NFSPlugin.pm
> +++ b/src/PVE/Storage/NFSPlugin.pm
> @@ -171,6 +171,20 @@ sub deactivate_storage {
>      }
>  }
>  
> +sub tcp_rpcinfo_cmd {

The helper could be private.

I wouldn't use tcp as a prefix (if using it, rather later in the name).
Other plugins use the plugin name as the prefix for intra-module
helpers. Maybe something like nfs_get_rpcinfo_command?

> +    my ($server, $version) = @_;
> +
> +    my $ip = PVE::JSONSchema::pve_verify_ip($server, 1);
> +    if (!defined($ip)) {
> +        $ip = PVE::Network::get_ip_from_hostname($server);
> +    }
> +
> +    my $transport = PVE::JSONSchema::pve_verify_ipv4($ip, 1) ? 'tcp' : 'tcp6';
> +
> +    # nfsv4 uses a pseudo-filesystem always beginning with / no exports are listed

I feel like putting it on the same line makes it harder to read. There
should be a comma or  dot after the slash. But actually, the comment
does not belong inside the function here, but to the call site. It's the
clarification for why rpcinfo is used rather than listing exports.

> +    return ['/usr/sbin/rpcinfo', '-T', $transport, $ip, 'nfs', $version];
> +}
> +
>  sub check_connection {
>      my ($class, $storeid, $scfg) = @_;
>  
> @@ -181,16 +195,7 @@ sub check_connection {
>  
>      my $is_v4 = defined($opts) && $opts =~ /vers=4.*/;
>      if ($is_v4) {
> -        my $ip = PVE::JSONSchema::pve_verify_ip($server, 1);
> -        if (!defined($ip)) {
> -            $ip = PVE::Network::get_ip_from_hostname($server);
> -        }
> -
> -        my $transport = PVE::JSONSchema::pve_verify_ipv4($ip, 1) ? 'tcp' : 'tcp6';
> -
> -        # nfsv4 uses a pseudo-filesystem always beginning with /
> -        # no exports are listed
> -        $cmd = ['/usr/sbin/rpcinfo', '-T', $transport, $ip, 'nfs', '4'];
> +        $cmd = tcp_rpcinfo_cmd($server, '4');
>      } else {
>          $cmd = ['/sbin/showmount', '--no-headers', '--exports', $server];
>      }

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

[Fiona Ebner]

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.

>      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

>          $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;
>      }

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

[Maximiliano Sandoval]

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

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

[Fiona Ebner]

Am 20.03.26 um 2:59 PM schrieb Maximiliano Sandoval:
> 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.

Are you concerned about the fallback not working for even older NFS
versions? Or is it something else?

'man nfs' states:
"Depending on kernel configuration, the Linux NFS client may support NFS
versions 3, 4.0, 4.1, or 4.2."

AFAICS, if there is no explicit vers=3 in the config, the situation is
as follows:

1) if we do the fallback:
1a) if it works (for example because it is version 3), great, we detect
more storages as online
1b) if it doesn't work, we still don't detect the storage, also not
worse than before

2) if we don't do the fallback, we don't detect any more storages