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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 25.03.26 um 1:46 PM 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 a
> 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

Typo: s/showmounts/showmount/

> the rationale was that rpcinfo did not have support for ipv6
> connections, which does not hold anymore.
> 
> 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.

Typo: s/showmounts/showmount/

> 
> Potential pitfalls:
> - Some NFSv3 servers might only support UDP [RFC 1813]

Also relevant for switching to only using rpcinfo. Could you extend the
TODO PVE 10 comment with this pitfall?

> - The two timeouts add up to 20 seconds

Is the timeout for the first command hit in case the server does not
list exports? Or does the command fail differently? If it's the former
case, it would be nice to avoid that, since a 10 second delay on every
activate_storage() is still bad from a user perspective.

Were there good reasons for the customer's server not to list the
exports? If not, we could also just document the requirement for NFSv3?

> - Some NFS servers might not have rpcbind as described in acff89540

AFAIK only NFSv4 ones [0], and that's why we have the tcp_ping()
fallback there.

[0]: "The following section only applies to NFSv3 implementations that
require the rpcbind service for backward compatibility." - from
https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/7/html/storage_administration_guide/s2-nfs-methodology-portmap

> 
> [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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/PVE/Storage/NFSPlugin.pm b/src/PVE/Storage/NFSPlugin.pm
> index 56dbd9ec..78f27c33 100644
> --- a/src/PVE/Storage/NFSPlugin.pm
> +++ b/src/PVE/Storage/NFSPlugin.pm
> @@ -198,6 +198,7 @@ sub check_connection {
>          # no exports are listed
>          $cmd = $nfs_get_rpcinfo_command->($server, '4');
>      } else {
> +        # TODO PVE 10 Consider switching to rpcinfo altogether.

Nit: could use the usual format: # FIXME: MAJOR VERSION: ...

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