Fix NFS VolumeStore FQDN not working #8244

YanzhaoLi · 2018-08-30T09:46:16Z

When docker using a nfs VolumeStore as a volume, the mount action will fail if the nfs-server url is a FQDN. A dotted ip url works well. When the linux syscall mount(source, target, flag, mountOptions) using for nfs mount, its mountOptions should contain 'addr=serverip' and the serverip must be an ip(ps. the url in source is useless).

Thus the fix resolve the FQDN and replace mountOptions's 'addr=fqdn' to 'addr=ip'. The mountOptions is generated as 'addr=host' at CreateMountSpec when creating the endpointVM. But this fix choose to do the resolve just before the mounting action.

Fix #8043
Fix #8357

[specific ci=1-19-Docker-Volume-Create]

zjs · 2018-08-30T18:24:56Z

lib/tether/ops_linux.go

+
+	log.Debugf("Looking up host %s", source.Hostname())
+	ips, err := net.LookupIP(source.Hostname())
+	if err != nil {
 		log.Errorf("mounting %s on %s failed: %s", source.String(), target, err)


Could we update this error message to specify looking up the IP for the specified hostname failed? It seems like that might be useful context to provide to aid in debugging, as that might lead to different troubleshooting steps than focusing on the mounting process itself.

I've thought that the variable 'err' contains corresponding looking-up failure info

zjs · 2018-08-30T18:28:04Z

lib/tether/ops_linux.go

+			return nil
+		}
+	}
+	log.Errorf("mounting %s on %s failed: %s", source.String(), target, err)


If there are multiple IPs, we'll only log the last error to occur. Maybe it would be helpful to log each error (and the associated IP address) at DEBUG within the loop, in addition to this ERROR-level log message here.

zjs · 2018-08-30T18:32:16Z

lib/tether/ops_linux.go

+		//We resolve the ip address nearest the mounting action.
+		//An alternative place is nfs.CreateMountSpec function in /lib/portlayer/storage/nfs/vm.go
+		mountOptionsFQDN2IP := strings.Replace(mountOptions, source.Hostname(), ip.String(), -1)
+		if err = Sys.Syscall.Mount(rawSource.String(), target, nfsFileSystemType, syscall.MS_NOATIME, mountOptionsFQDN2IP); err == nil {


Could we extract this call to Sys.Syscall.Mount into a private method that takes rawSource, target, nfsFileSystemType, and the mountOptions*? This would let us avoid duplication with the identical call above and, more importantly, ensure that we keep the behavior between IP- and hostname-based handling consistent (e.g., if we ever replaced syscall.MS_NOATIME or made it configurable — as the comment above suggests we might).

zjs · 2018-08-30T18:39:34Z

lib/tether/ops_linux.go

+		//NOTE: the mountOptions of syscall mount only accept addr=ip; addr=FQDN doesn't work
+		//We resolve the ip address nearest the mounting action.
+		//An alternative place is nfs.CreateMountSpec function in /lib/portlayer/storage/nfs/vm.go
+		mountOptionsFQDN2IP := strings.Replace(mountOptions, source.Hostname(), ip.String(), -1)


It seems like rawSource.String() will also contain the hostname. Does that need to be replaced too?

Actually hostname in rawSource.String() doesn't affect sys.syscall.mount.

hickeng · 2018-09-07T13:33:30Z

The fix looks good. I've been trying to think of a way to add testing before approving.

zjs · 2018-09-07T18:20:09Z

lib/tether/ops_linux.go

+	for _, ip := range ips {
+		//NOTE: the mountOptions of syscall mount only accept addr=ip. addr=FQDN doesn't work
+		//We resolve the ip address nearest the mounting action.
+		mountOptionsIP := strings.Replace(mountOptions, source.Hostname(), ip.String(), -1)


Are there edge cases where mountOptions could contain the source.Hostname() multiple times? Could someone provide a source.Hostname() like addr (a valid hostname, but not a valid FQDN) that actually appears as a key in addition to being a value)?

Maybe prefixing the old/new with addr= would reduce risk here?

strings.Replace(mountOptions, "addr="+source.Hostname(), "addr="+ip.String(), -1)

Yeah, that's really an awesome advice.
Btw, currently the mountOptions is not configurable.
It is always nfsMountOpitons + ",addr=" + host.Host, and the nfsMountOptions is a hard-coded constant.

Btw, currently the mountOptions is not configurable.

I see! That's good context to have. That certainly limits the scope of any issues here right now, but I think this change is still useful to make to avoid potential issues in the future.

YanzhaoLi · 2018-11-02T08:32:14Z

The fix looks good. I've been trying to think of a way to add testing before approving.

I've created an issue #8357 aiming to expect test cases for this pr.

When docker using a nfs VolumeStore as a volume, the mount action will fail if the nfs-server url is a FQDN. A dotted ip url works well. When the linux syscall mount(source, target, flag, mountOptions) using for nfs mount, its mountOptions should contain 'addr=serverip' and the serverip must be an ip(ps. the url in source is useless). Thus the fix resolve the FQDN and give its mountOptions the 'addr=ip'. The mountOptions is generated as 'addr=host' at CreateMountSpec when creating the endpointVM. The fix choose to do the resolve just before the mounting action. Resolve: vmware#8043

hickeng

Confirmed test functions as expected from CI https://ci-vic.vmware.com/vmware/vic/20465 logs.

YanzhaoLi requested a review from a team as a code owner August 30, 2018 09:46

vmwclabot added the cla-not-required label Aug 30, 2018

YanzhaoLi requested review from hickeng and zjs August 30, 2018 09:47

zjs reviewed Aug 30, 2018

View reviewed changes

YanzhaoLi force-pushed the topic/fix-nfsmount-fqdn branch from cc4f6f6 to 17f04ff Compare September 4, 2018 06:12

zjs reviewed Sep 7, 2018

View reviewed changes

zjs assigned YanzhaoLi Oct 26, 2018

zjs added this to the Sprint 36 milestone Oct 26, 2018

YanzhaoLi mentioned this pull request Nov 2, 2018

Need to add test cases for FQDN Volume of pr 8244 #8357

Closed

3 tasks

YanzhaoLi force-pushed the topic/fix-nfsmount-fqdn branch 2 times, most recently from 94c5f45 to 21c1b96 Compare November 5, 2018 08:14

YanzhaoLi force-pushed the topic/fix-nfsmount-fqdn branch 9 times, most recently from 7bc9bf6 to 2153a31 Compare November 22, 2018 08:58

YanzhaoLi requested review from wjun, renmaosheng and DanielXiao November 22, 2018 09:00

renmaosheng approved these changes Nov 26, 2018

View reviewed changes

YanzhaoLi added 3 commits November 25, 2018 22:49

fixup! Fix NFS VolumeStore FQDN not working

00902d1

fixup! Fix NFS VolumeStore FQDN not working

96fb767

fixup! Fix NFS VolumeStore FQDN not working

2009b83

YanzhaoLi force-pushed the topic/fix-nfsmount-fqdn branch from 2153a31 to e912dce Compare November 26, 2018 06:50

fixup! Fix NFS VolumeStore FQDN not working

1d54af6

YanzhaoLi force-pushed the topic/fix-nfsmount-fqdn branch from e912dce to 1d54af6 Compare November 26, 2018 11:09

hickeng approved these changes Nov 26, 2018

View reviewed changes

wjun approved these changes Nov 27, 2018

View reviewed changes

YanzhaoLi merged commit 44d6f81 into vmware:master Dec 17, 2018

YanzhaoLi added the impact/doc/note Requires creation of or changes to an official release note label Jan 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NFS VolumeStore FQDN not working #8244

Fix NFS VolumeStore FQDN not working #8244

YanzhaoLi commented Aug 30, 2018 •

edited

Loading

zjs Aug 30, 2018

YanzhaoLi Sep 4, 2018

zjs Aug 30, 2018

zjs Aug 30, 2018

zjs Aug 30, 2018

YanzhaoLi Sep 4, 2018

hickeng commented Sep 7, 2018

zjs Sep 7, 2018

YanzhaoLi Sep 10, 2018

zjs Sep 25, 2018 •

edited

Loading

YanzhaoLi commented Nov 2, 2018

hickeng left a comment

Fix NFS VolumeStore FQDN not working #8244

Fix NFS VolumeStore FQDN not working #8244

Conversation

YanzhaoLi commented Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hickeng commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjs Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

YanzhaoLi commented Nov 2, 2018

hickeng left a comment

Choose a reason for hiding this comment

YanzhaoLi commented Aug 30, 2018 •

edited

Loading

zjs Sep 25, 2018 •

edited

Loading