Fix mounting NFS resources in IPv6 bare-metal environment #101066 #101067

Elbehery · 2021-04-13T12:10:04Z

What type of PR is this?

/kind bug
/kind storage
/label nfs
/label ipv6

What this PR does / why we need it:

Fix mounting nfs volumes in ipv6 environment.

Which issue(s) this PR fixes:

Fixes #101066

Special notes for your reviewer:

Enclose only ipv6 within [ ].

Fixed mounting of NFS volumes when IPv6 address is used as a server.

k8s-ci-robot · 2021-04-13T12:10:06Z

@Elbehery: The label(s) `/label nfs
, /label ipv6

cannot be applied. These labels are supported:api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda`

In response to this:

What type of PR is this?

/kind bug
/kind storage
/label nfs
/label ipv6

What this PR does / why we need it:

Fix mounting nfs volumes in ipv6 environment.

Which issue(s) this PR fixes:

Fixes #101066

Special notes for your reviewer:

Enclose only ipv6 within [ ].

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-04-13T12:10:11Z

@Elbehery: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-04-13T12:10:12Z

Welcome @Elbehery!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-04-13T12:10:12Z

Hi @Elbehery. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Elbehery · 2021-04-13T12:15:16Z

In addition to the testing procedure described in the contribute docs, I have tested it locally according to the reproducing steps described in #101066, below are the logs :-

$kubectl get svc                
NAME         TYPE        CLUSTER-IP         EXTERNAL-IP   PORT(S)                      AGE
kubernetes   ClusterIP   fd00:10:96::1      <none>        443/TCP                      3h35m
nfs-server   ClusterIP   fd00:10:96::63f1   <none>        2049/TCP,20048/TCP,111/TCP   89s

$kubectl get pod -w                   
NAME                READY   STATUS              RESTARTS   AGE
nfs-busybox-ppv7h   0/1     ContainerCreating   0          2s
nfs-busybox-xqrqp   0/1     ContainerCreating   0          2s
nfs-server-x86wb    1/1     Running             0          2m34s
nfs-busybox-ppv7h   1/1     Running             0          7s
nfs-busybox-xqrqp   1/1     Running             0          8s

jsafrane · 2021-04-13T12:26:43Z

/ok-to-test
/sig storage

Elbehery · 2021-04-13T14:44:28Z

@jsafrane added a test case with literal ipv4

built a new binary, tested upon ipv4, ipv6, and dualcluster configurations, all containersrunningandnfs` volumes mounted

jsafrane · 2021-04-13T16:06:23Z

pkg/volume/nfs/nfs_test.go

+		Name:         "vol1",
+		VolumeSource: v1.VolumeSource{NFS: &v1.NFSVolumeSource{Server: "0:0:0:0:0:0:0:1", Path: "/somepath", ReadOnly: false}},
+	}
+	doTestPlugin(t, volume.NewSpecFromVolume(vol))


doTestPlugin does not test that the plugin added [ ] around the IP address, you need to add it there.

@jsafrane nfsMounter does not expose server field, so that I can assert upon it.

I have been trying to find an interface implemented by nfsMounter that exposes the field, but I could not. I can :-

func (nfsMounter *nfsMounter) getServer() string { return nfsMounter.server }

What do u think ?

I meant something like this:

--- a/pkg/volume/nfs/nfs_test.go +++ b/pkg/volume/nfs/nfs_test.go @@ -96,7 +96,7 @@ func TestRecycler(t *testing.T) { } } -func doTestPlugin(t *testing.T, spec *volume.Spec) { +func doTestPlugin(t *testing.T, spec *volume.Spec, expectedDevice string) { tmpDir, err := utiltesting.MkTmpdir("nfs_test") if err != nil { t.Fatalf("error creating temp dir: %v", err) @@ -146,9 +146,6 @@ func doTestPlugin(t *testing.T, spec *volume.Spec) { if mntDevs[0].Type != "nfs" { t.Errorf("unexpected type of mounted devices. expected: %v, got %v", "nfs", mntDevs[0].Type) } - src, _, _ := getVolumeSource(spec) - srvr := getServerFromSource(src) - expectedDevice := fmt.Sprintf("%s:%s", srvr, src.Path) if mntDevs[0].Device != expectedDevice { t.Errorf("unexpected nfs device, expected %q, got: %q", expectedDevice, mntDevs[0].Device) } @@ -203,7 +200,7 @@ func TestIPV6VolumeSource(t *testing.T) { Name: "vol1", VolumeSource: v1.VolumeSource{NFS: &v1.NFSVolumeSource{Server: "0:0:0:0:0:0:0:1", Path: "/somepath", ReadOnly: false}}, } - doTestPlugin(t, volume.NewSpecFromVolume(vol)) + doTestPlugin(t, volume.NewSpecFromVolume(vol), "[0:0:0:0:0:0:0:1]:/somepath") } func TestIPV4VolumeSource(t *testing.T) {

(+ you need to update all other doTestPlugin calls)
This way you can explicitly specify that [] should be added to IPv6 addresses and not to ipv4.

@jsafrane added in the last commit 👍🏽

Elbehery · 2021-04-13T19:02:13Z

pkg/volume/nfs/nfs_test.go

@@ -118,6 +119,18 @@ func doTestPlugin(t *testing.T, spec *volume.Spec) {
 	if mounter == nil {
 		t.Errorf("Got a nil Mounter")
 	}
+	if nfsm, ok := mounter.(*nfsMounter); ok {


@jsafrane I avoided changing anything in the pkg itself for testing, so i came up with this way, I hope it suffice

@aojea do you have a comment on this mock ? .. I tried to avoid adding a method just to assert un-exported field

well, to be honest, the test has exactly the same code that the functionality, the only way to fail is if one of those diverge in the future :)

just throwing an idea, what if the fix goes here

kubernetes/pkg/volume/nfs/nfs.go

Line 258 in 4f6c595

source := fmt.Sprintf("%s:%s", nfsMounter.server, nfsMounter.exportPath)

it seems a better place that current one , if IPv6 you build the source with brackets for the server.
This way, you can mock nfsMounter.mounter.MountSensitiveWithoutSystemd(

kubernetes/pkg/volume/nfs/nfs.go

Line 191 in 4f6c595

mounter mount.Interface

and assert the server ip in mountOptions

kubernetes/pkg/volume/nfs/nfs.go

Line 264 in 4f6c595

err = nfsMounter.mounter.MountSensitiveWithoutSystemd(source, dir, "nfs", mountOptions, nil)

@aojea this way, the fix will affect only the mounter when it is being setUp not created.

The problem arose in the first place, when pods consuming nfs could not find the resource, and
I searched the usage of both setUp which affects the mounter only when used. On the other hand, newMounter is used by kubelet so when the resource is actually allocated.

Please correct me if I am wrong 🙏🏽

jsafrane · 2021-04-29T12:44:16Z

/lgtm
/retest

Elbehery · 2021-04-29T15:05:14Z

/retest

fejta-bot · 2021-04-29T17:45:16Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.