Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support NFSv4 only environment #617

Merged
merged 4 commits into from
Sep 25, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions drivers/NFSSR.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,13 @@ def validate_remotepath(self, scan):
def check_server(self):
try:
if PROBEVERSION in self.dconf:
sv = nfs.get_supported_nfs_versions(self.remoteserver)
sv = nfs.get_supported_nfs_versions(self.remoteserver, self.transport)
if len(sv):
self.nfsversion = sv[0]
else:
nfs.check_server_tcp(self.remoteserver, self.nfsversion)
if not nfs.check_server_tcp(self.remoteserver, self.transport, self.nfsversion):
raise nfs.NfsException("Unsupported NFS version: %s" % self.nfsversion)

except nfs.NfsException as exc:
raise xs_errors.XenError('NFSVersion',
opterr=exc.errstr)
Expand Down Expand Up @@ -163,7 +165,7 @@ def probe(self):

self.mount(temppath, self.remotepath)
try:
return nfs.scan_srlist(temppath, self.dconf)
return nfs.scan_srlist(temppath, self.transport, self.dconf)
finally:
try:
nfs.unmount(temppath, True)
Expand Down Expand Up @@ -250,7 +252,7 @@ def vdi(self, uuid, loadLocked=False):

def scan_exports(self, target):
util.SMlog("scanning2 (target=%s)" % target)
dom = nfs.scan_exports(target)
dom = nfs.scan_exports(target, self.transport)
print(dom.toprettyxml(), file=sys.stderr)

def set_transport(self):
Expand Down
155 changes: 115 additions & 40 deletions drivers/nfs.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@

RPCINFO_BIN = "/usr/sbin/rpcinfo"
SHOWMOUNT_BIN = "/usr/sbin/showmount"
NFS_STAT = "/usr/sbin/nfsstat"

DEFAULT_NFSVERSION = '3'

Expand All @@ -50,34 +51,41 @@
NFS_SERVICE_WAIT = 30
NFS_SERVICE_RETRY = 6

NFS4_PSEUDOFS = "/"
NFS4_TMP_MOUNTPOINT = "/tmp/mnt"

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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@MarkSymsCtx MarkSymsCtx Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Make sure that NFS over TCP/IP V3 is supported on the server.

Returns True if everything is OK
False otherwise.
"""

try:
sv = get_supported_nfs_versions(server)
return (True if nfsversion in sv else False)
sv = get_supported_nfs_versions(server, transport)
return (nfsversion[0] in sv)
except util.CommandException as inst:
raise NfsException("rpcinfo failed or timed out: return code %d" %
inst.code)


def check_server_service(server):
def check_server_service(server, transport):
"""Ensure NFS service is up and available on the remote server.

Returns False if fails to detect service after
NFS_SERVICE_RETRY * NFS_SERVICE_WAIT
"""

sv = get_supported_nfs_versions(server, transport)
# Services are not present in NFS4 only, this doesn't mean there's no NFS
if sv == ['4']:
return True
benjamreis marked this conversation as resolved.
Show resolved Hide resolved

retries = 0
errlist = [errno.EPERM, errno.EPIPE, errno.EIO]

Expand Down Expand Up @@ -129,18 +137,6 @@ def soft_mount(mountpoint, remoteserver, remotepath, transport, useroptions='',
raise NfsException("Failed to make directory: code is %d" %
inst.code)


# Wait for NFS service to be available
try:
if not check_server_service(remoteserver):
raise util.CommandException(
code=errno.EOPNOTSUPP,
reason='No NFS service on server: `%s`' % remoteserver
)
except util.CommandException as inst:
raise NfsException("Failed to detect NFS service on server `%s`"
% remoteserver)

mountcommand = 'mount.nfs'

options = "soft,proto=%s,vers=%s" % (
Expand Down Expand Up @@ -186,17 +182,16 @@ def unmount(mountpoint, rmmountpoint):
raise NfsException("rmdir failed with error '%s'" % inst.strerror)


def scan_exports(target):
"""Scan target and return an XML DOM with target, path and accesslist."""
util.SMlog("scanning")
def _scan_exports_nfs3(target, dom, element):
""" Scan target and return an XML DOM with target, path and accesslist.
Using NFS3 services.
"""

cmd = [SHOWMOUNT_BIN, "--no-headers", "-e", target]
dom = xml.dom.minidom.Document()
element = dom.createElement("nfs-exports")
dom.appendChild(element)
for val in util.pread2(cmd).split('\n'):
if not len(val):
continue
entry = dom.createElement('Export')
entry = dom.createElement("Export")
element.appendChild(entry)

subentry = dom.createElement("Target")
Expand All @@ -218,11 +213,61 @@ def scan_exports(target):
entry.appendChild(subentry)
textnode = dom.createTextNode(access)
subentry.appendChild(textnode)
return dom


def _scan_exports_nfs4_only(target, transport, dom, element):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :)

""" Scan target and return an XML DOM with target, path and accesslist.
Using NFS4 only pseudo FS.
"""

mountpoint = "%s/%s" % (NFS4_TMP_MOUNTPOINT, target)
soft_mount(mountpoint, target, NFS4_PSEUDOFS, transport, nfsversion="4")
paths = os.listdir(mountpoint)
unmount(mountpoint, NFS4_PSEUDOFS)
for path in paths:
entry = dom.createElement("Export")
element.appendChild(entry)

subentry = dom.createElement("Target")
entry.appendChild(subentry)
textnode = dom.createTextNode(target)
subentry.appendChild(textnode)
subentry = dom.createElement("Path")
entry.appendChild(subentry)
textnode = dom.createTextNode(path)
subentry.appendChild(textnode)

subentry = dom.createElement("Accesslist")
entry.appendChild(subentry)
# Assume everyone as we do not have any info about it
textnode = dom.createTextNode("*")
subentry.appendChild(textnode)
return dom


def scan_srlist(path, dconf):
def scan_exports(target, transport):
"""Scan target and return an XML DOM with target, path and accesslist."""
util.SMlog("scanning")
dom = xml.dom.minidom.Document()
element = dom.createElement("nfs-exports")
dom.appendChild(element)
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, to a lesser extent, get_supported_nfs_versions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can split in methods for NFS3 and NFS4Only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

return _scan_exports_nfs3(target, dom, element)
except Exception:
util.SMlog("Unable to scan exports with %s, trying NFSv4" % SHOWMOUNT_BIN)

# NFSv4 only
try:
return _scan_exports_nfs4_only(target, transport, dom, element)
except Exception:
util.SMlog("Unable to scan exports with NFSv4 pseudo FS mount")

raise NfsException("Failed to read NFS export paths from server %s" %
(target))


def scan_srlist(path, transport, dconf):
"""Scan and report SR, UUID."""
dom = xml.dom.minidom.Document()
element = dom.createElement("SRlist")
Expand All @@ -245,7 +290,7 @@ def scan_srlist(path, dconf):
if PROBEVERSION in dconf:
util.SMlog("Add supported nfs versions to sr-probe")
try:
supported_versions = get_supported_nfs_versions(dconf.get('server'))
supported_versions = get_supported_nfs_versions(dconf.get('server'), transport)
supp_ver = dom.createElement("SupportedVersions")
element.appendChild(supp_ver)

Expand All @@ -261,22 +306,52 @@ def scan_srlist(path, dconf):
return dom.toprettyxml()


def get_supported_nfs_versions(server):
"""Return list of supported nfs versions."""
valid_versions = set(['3', '4'])
def _get_supported_nfs_version_rpcinfo(server):
""" Return list of supported nfs versions.
Using NFS3 services.
"""

valid_versions = set(["3", "4"])
cv = set()
ns = util.pread2([RPCINFO_BIN, "-s", "%s" % server])
ns = ns.split("\n")
for i in range(len(ns)):
if ns[i].find("nfs") > 0:
cvi = ns[i].split()[1].split(",")
for j in range(len(cvi)):
cv.add(cvi[j])
return sorted(cv & valid_versions)


def _is_nfs4_supported(server, transport):
""" Return list of supported nfs versions.
Using NFS4 pseudo FS.
"""

try:
ns = util.pread2([RPCINFO_BIN, "-s", "%s" % server])
ns = ns.split("\n")
for i in range(len(ns)):
if ns[i].find("nfs") > 0:
cvi = ns[i].split()[1].split(",")
for j in range(len(cvi)):
cv.add(cvi[j])
return sorted(cv & valid_versions)
except:
util.SMlog("Unable to obtain list of valid nfs versions")
raise NfsException('Failed to read supported NFS version from server %s' %
mountpoint = "%s/%s" % (NFS4_TMP_MOUNTPOINT, server)
soft_mount(mountpoint, server, NFS4_PSEUDOFS, transport, nfsversion='4')
util.pread2([NFS_STAT, "-m"])
unmount(mountpoint, NFS4_PSEUDOFS)
return True
except Exception:
return False


def get_supported_nfs_versions(server, transport):
"""Return list of supported nfs versions."""
try:
return _get_supported_nfs_version_rpcinfo(server)
except Exception:
util.SMlog("Unable to obtain list of valid nfs versions with %s, trying NFSv4" % RPCINFO_BIN)

# NFSv4 only
if _is_nfs4_supported(server, transport):
return ["4"]
else:
util.SMlog("Unable to obtain list of valid nfs versions with NFSv4 pseudo FS mount")

raise NfsException("Failed to read supported NFS version from server %s" %
(server))


Expand Down
2 changes: 1 addition & 1 deletion tests/test_NFSSR.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def test_attach(self, validate_nfsversion, check_server_tcp, _testhost,

nfssr.attach(None)

check_server_tcp.assert_called_once_with('aServer',
check_server_tcp.assert_called_once_with('aServer', 'tcp',
'aNfsversionChanged')
soft_mount.assert_called_once_with('/var/run/sr-mount/UUID',
'aServer',
Expand Down
Loading