Support NFSv4 only environment #617

benjamreis · 2023-05-02T13:22:17Z

NFSv4 only environments don't support rpcinfo
and showmount calls as they're missing NFSv3
services.

When rpcinfo or showmount fails, try to mount
NFSv4 pseudo FS '/' to add '4' to supported NFS versions
and run ls on the mounted pseudo FS to offer the first
folder level of exports.

See: #551
And: xcp-ng/xcp#135

benjamreis · 2023-05-02T13:26:50Z

What can be done next:

Attempt a mount on / with nfs4 and:

run nfsstat -m to get the version of the mount and add it to the supported version
run ls in the mounted / to get some exported paths BUT what if there's multiple level of folders?

What do you think @MarkSymsCtx?

EDIT: Done.

MarkSymsCtx · 2023-05-02T13:41:36Z

What can be done next:

Attempt a mount on / with nfs4 and:

run nfsstat -m to get the version of the mount and add it to the supported version

run ls in the mounted / to get some exported paths BUT what if there's multiple level of folders?

What do you think @MarkSymsCtx?

It's all generally horrible with NFSv4 but the above may help in a significant set of sub-cases.

benjamreis · 2023-05-02T13:43:30Z

Okay, i'll try that then, it's still better than nothing.
Would you prefer to merge this PR 1st and I'll do a 2nd one on top or wait and do everything in this PR?

stormi · 2023-05-02T13:53:50Z

run nfsstat -m to get the version of the mount and add it to the supported version

I don't know exactly how this works, but the current version of mount.nfs4 in XS/XCP-ng dom0 defaulted to 4.1 for a server that only supports 4.1 and 4.2. Another client on a more recent computer defaulted to 4.2 for the same server.

This means that the version we can extract from nfsstat -m depends on the client and doesn't give much information about what the server supports, so maybe probe should just reply 4 as the supported version when there's no NFS v3 (Option A).

Another approach would be, after a first successful mount of /, to attempt to mount / with every known version of the v4 protocol (4.0, 4.1, 4.2), and keep only the working values (Option B).

Yet another approach would be always listing all version of NFSv4, and let it fail at sr-create time when the user attempts a not-supported version (Option C).

benjamreis · 2023-05-02T14:01:44Z

Yet another approach would be always listing all version of NFSv4, and let it fail at sr-create time when the user attempts a not-supported version (Option C).

I don't like this option, it can be deceptive to show supported versions and then fail when trying to use them.

IMHO only displaying 4 is enough and let the mount decide on the version?

drivers/nfs.py

benjamreis · 2023-05-05T08:32:45Z

Successfull tests on NFS3 & 4.

olivierlambert · 2023-06-30T06:08:33Z

Hey :) Any news on this? Is there anything left to do or simply getting validated?

MarkSymsCtx · 2023-06-30T09:44:45Z

drivers/nfs.py

-
-
-def scan_srlist(path, dconf):
+    try:


I don't particularly like how long this method has now become, it makes it quite cognitively hard to follow. Would be good to pull some of these long open coded blocks out into well named methods which describe what they're doing and then each one can be reviewed and understood in isolation (or ignored if not of interest to the current needs).

Similarly, to a lesser extent, get_supported_nfs_versions below.

I can split in methods for NFS3 and NFS4Only.

NFSv4 only environments don't support `rpcinfo` and `showmount` calls as they're missing NFSv3 services. When `rpcinfo` or `showmount` fails, try to mount NFSv4 pseudo FS '/' to add '4' to supported NFS versions and run `ls` on the mounted pseudo FS to offer the first folder level of exports. See: xapi-project#551 And: xcp-ng/xcp#135 Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

MarkSymsCtx

Just a minor nit on method names.

Is there anything "useful" to see in the logging for this that will show it working?

MarkSymsCtx · 2023-07-25T10:30:06Z

drivers/nfs.py

+    return dom
+
+
+def _scan_exports_nfs4_only(target, transport, dom, element):


given that we have _scan_exports_nfs3 this should probably just be _scan_exports_nfs4 for consistency.

I hesitated to add the _only or not, both are okay for me so I can remove it if you prefer.

benjamreis · 2023-07-25T11:46:19Z

Is there anything "useful" to see in the logging for this that will show it working?

In a NFS3/NFS3-4 env you should see the same logs as before since NFS4 only is a fallback in case of failure.
In NFS4 only env I added logs saying the 1st method fails and we're trying using the NFS4 pseudo FS
https://github.com/xapi-project/sm/pull/617/files#diff-9ca8a7292c88a517d48db9a9d25ee2d14ffd83f7f40d818c1c4121ce2e275b35R258
https://github.com/xapi-project/sm/pull/617/files#diff-9ca8a7292c88a517d48db9a9d25ee2d14ffd83f7f40d818c1c4121ce2e275b35R346

I also added logs in case both methods failed before throwing an exception.

Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

MarkSymsCtx · 2023-09-05T11:53:02Z

drivers/nfs.py


 class NfsException(Exception):

    def __init__(self, errstr):
        self.errstr = errstr

-
-def check_server_tcp(server, nfsversion=DEFAULT_NFSVERSION):
+def check_server_tcp(server, transport, nfsversion=DEFAULT_NFSVERSION):


The method is explicit (or is expected to be) in using TCP, so it should not have the transport argument. If get_suppoted_nfs_versions needs a transport it should be passed as a literal from this method without changing the signature of the method.

Indeed but transport can be tcp or tcp6 in the case of IPv6.

Would you prefer an IPv6 boolean and the the transport given to get_supported_nfs_versions would be 'tcp6' if ipv6 else 'tcp'?

Ah, I see, that was not clear. Might have been better for the IPv6 bits to be at least a commit on their own right to make it clear. But would be difficult to split now so leave it as it is.

olivierlambert · 2023-09-06T11:54:18Z

I think we need another validated reviewer to be able to merge this. Anyone in mind @MarkSymsCtx ?

MarkSymsCtx · 2023-09-06T12:33:34Z

@olivierlambert I'd already added the extra reviewer

olivierlambert · 2023-09-13T16:09:46Z

Can you merge? Thanks!

KevinLCtx · 2023-09-14T08:41:11Z

Can you merge? Thanks!

Mark is on vacation I will see if I can find somebody else who has permission to merge it.

See: xapi-project#617 Signed-off-by: Benjamin Reis <benjamin.reis@vates.tech>

benjamreis force-pushed the support-nfsv4-only branch from b7cc0d2 to f093ab0 Compare May 2, 2023 13:34

MarkSymsCtx previously approved these changes May 2, 2023

View reviewed changes

benjamreis dismissed MarkSymsCtx’s stale review via e00c477 May 3, 2023 09:29