Node Publish Mount Idempotent #1019

nirmalaagash · 2021-08-10T22:26:10Z

Is this a bug fix or adding new feature?
Issue Fix #955 - Node publish mount idempotent
Issue Fix #1007 - Duplicate Mount Entries.

What is this PR about? / Why do we need it?
The PR adds mount idempotent to the CSI driver which provides multiple requests of 'NodePublishVolume' with the same request parameters would not throw error. We need it for the idempotency in mount.

What testing is done?
Unit testing.
Manual testing is done using csc tool. Below are the screenshots which shows the NodePublishVolume call of same request made multiple times. At first, it publishes the volume at the target and later request provides the information that the target is already mounted.

For the issue #1007, manual testing results.

For filesystem volume,

For block volumes,

k8s-ci-robot · 2021-08-10T22:26:17Z

Hi @nirmalaagash. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

AndyXiangLi · 2021-08-11T18:31:34Z

/ok-to-test

nirmalaagash · 2021-08-11T18:37:41Z

/test pull-aws-ebs-csi-driver-verify

vdhanan · 2021-08-11T21:20:41Z

pkg/driver/node.go

+		//Checking if the path exists and error is related to Corrupted Mount, in that case, the system could unmount and mount.
+		_, pathErr := mountutils.PathExists(target)
+		if pathErr != nil && mountutils.IsCorruptedMnt(pathErr) {
+			klog.V(4).Infof("Target path %q is a corrupted directory", target)


are we not returning an error if target path is a corrupted directory?

Chances are that the device mounted or the mount point is corrupted because of improper unmounting. Hence in the next phase we try to read the directory and unmount it.

vdhanan · 2021-08-11T21:23:23Z

pkg/driver/node.go

 	}

+	if !notMnt {
+		//Return error when any of the directory inside is not readable which means that a device could be mounted.


could you clarify what this comment means? not sure i'm following

if a device is already mounted in the target directory, the directories inside the directory would not be accessible until the device is unmounted from the target directory. Hence in that case, it would return an error.

vdhanan · 2021-08-11T21:25:17Z

are all the new code paths (i.e. the if/else conditions) covered by the existing unit tests? i feel like we probably want to add a couple new ones, right?

bertinatto

Thank you for working on this, @nirmalaagash.

Does your fix also work block volumes? It seems we're only covering filesystem volumes.

nirmalaagash · 2021-08-12T17:40:04Z

Thank you for working on this, @nirmalaagash.

Does your fix also work block volumes? It seems we're only covering filesystem volumes.

@bertinatto Yes. This PR only covers the filesystem volumes.

bertinatto · 2021-08-12T18:35:57Z

pkg/driver/node.go

+		2. false, nil when the path is already mounted with a device.
+		3. true, nil when the path is not mounted with any device.
+	*/
+	notMnt, err := d.mounter.IsLikelyNotMountPoint(target)


An easy way to cover block devices as well is to move the check right below the in flight check.

You may want to take a look at the solutions both GCP PD and Azure Disk implemented.

@bertinatto Thanks. New commit covering block devices is pushed.

nirmalaagash · 2021-08-18T02:48:38Z

are all the new code paths (i.e. the if/else conditions) covered by the existing unit tests? i feel like we probably want to add a couple new ones, right?

@vdhanan Commit include Unit Test Cases covering the conditions implemented. Thanks.

ialidzhikov · 2021-08-24T08:46:17Z

/sig storage
/area provider/aws
/kind bug
/priority critical-urgent

vdhanan · 2021-08-25T16:33:15Z

/lgtm

vdhanan · 2021-08-25T16:51:43Z

/approve

k8s-ci-robot · 2021-08-25T16:51:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nirmalaagash, vdhanan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdhanan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nirmalaagash · 2021-08-25T17:06:48Z

/retest

nirmalaagash · 2021-08-25T19:13:28Z

/retest

ialidzhikov · 2021-08-25T21:10:09Z

The verify job fails on hack/verify-gofmt:

Verifying gofmt
diff -u ./pkg/cloud/aws_metrics.go.orig ./pkg/cloud/aws_metrics.go
--- ./pkg/cloud/aws_metrics.go.orig	2021-08-25 17:39:13.750184141 +0000
+++ ./pkg/cloud/aws_metrics.go	2021-08-25 17:39:13.750184141 +0000
@@ -1,3 +1,4 @@
+//go:build !providerless
 // +build !providerless
 
 /*
diff -u ./pkg/mounter/safe_mounter_unix.go.orig ./pkg/mounter/safe_mounter_unix.go
--- ./pkg/mounter/safe_mounter_unix.go.orig	2021-08-25 17:39:13.758184690 +0000
+++ ./pkg/mounter/safe_mounter_unix.go	2021-08-25 17:39:13.758184690 +0000
@@ -1,3 +1,4 @@
+//go:build linux || darwin
 // +build linux darwin
 
 /*
diff -u ./pkg/mounter/safe_mounter_windows.go.orig ./pkg/mounter/safe_mounter_windows.go
--- ./pkg/mounter/safe_mounter_windows.go.orig	2021-08-25 17:39:13.762184964 +0000
+++ ./pkg/mounter/safe_mounter_windows.go	2021-08-25 17:39:13.762184964 +0000
@@ -1,3 +1,4 @@
+//go:build windows
 // +build windows
 
 /*
diff -u ./pkg/driver/mount_windows.go.orig ./pkg/driver/mount_windows.go
--- ./pkg/driver/mount_windows.go.orig	2021-08-25 17:39:13.770185513 +0000
+++ ./pkg/driver/mount_windows.go	2021-08-25 17:39:13.770185513 +0000
@@ -1,3 +1,4 @@
+//go:build windows
 // +build windows
 
 /*
diff -u ./pkg/driver/node_windows.go.orig ./pkg/driver/node_windows.go
--- ./pkg/driver/node_windows.go.orig	2021-08-25 17:39:13.786186611 +0000
+++ ./pkg/driver/node_windows.go	2021-08-25 17:39:13.786186611 +0000
@@ -1,3 +1,4 @@
+//go:build windows
 // +build windows
 
 /*
diff -u ./pkg/driver/node_linux.go.orig ./pkg/driver/node_linux.go
--- ./pkg/driver/node_linux.go.orig	2021-08-25 17:39:13.818188805 +0000
+++ ./pkg/driver/node_linux.go	2021-08-25 17:39:13.818188805 +0000
@@ -1,3 +1,4 @@
+//go:build linux
 // +build linux
 
 /*
diff -u ./pkg/driver/mount_linux.go.orig ./pkg/driver/mount_linux.go
--- ./pkg/driver/mount_linux.go.orig	2021-08-25 17:39:13.858191550 +0000
+++ ./pkg/driver/mount_linux.go	2021-08-25 17:39:13.858191550 +0000
@@ -1,3 +1,4 @@
+//go:build linux
 // +build linux
 
 /*

Please run hack/update-gofmt to fix the issue(s)
make: *** [Makefile:143: verify] Error 1
+ EXIT_VALUE=2
+ set +o xtrace

New //go:build tag is added. For some reason it is using gofmt@v1.17. See similar issue kubernetes-sigs/controller-tools#594.
cc @wongma7

k8s-ci-robot · 2021-08-25T21:19:01Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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. I understand the commands that are listed here.

ialidzhikov · 2021-08-25T21:21:38Z

@nirmalaagash , aca3620 looks good. This is also how k/k handled it - see https://github.com/kubernetes/kubernetes/pull/103692/files.

PS: you have to fix the author of commit aca3620.

ialidzhikov · 2021-08-25T21:51:37Z

@nirmalaagash , there is still commit aca3620 with wrong author. I think you can drop this commit from your change as there is already 77cc317.

nirmalaagash · 2021-08-25T23:25:01Z

/check-cla

ialidzhikov · 2021-08-26T05:21:06Z

ping @bertinatto @vdhanan
for review

vdhanan · 2021-08-27T19:33:10Z

/lgtm

ialidzhikov · 2021-08-27T19:55:04Z

Hi folks 👋,

ls it possible to backport this fix to the supported version? Thanks in advance

…019-origin-release-1.2 [release-1.2] Automated cherry pick of #1019: Added Mount Idempotency

…019-origin-release-1.1 [release-1.1] Automated cherry pick of #1019: Added Mount Idempotency

Added Mount Idempotency

3bafcd1

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 10, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 10, 2021

k8s-ci-robot requested review from ayberk and ddebroy August 10, 2021 22:26

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 10, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2021

fixed:g-format error

36ffbfe

vdhanan reviewed Aug 11, 2021

View reviewed changes

bertinatto reviewed Aug 12, 2021

View reviewed changes

bertinatto mentioned this pull request Aug 12, 2021

Restart of kubelet leads to duplicated mount entries #1007

Closed

Added Mount Idempotency for block device

ac4a32e

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 18, 2021

nirmalaagash mentioned this pull request Aug 24, 2021

Pods using ephemeral volumes stuck terminating after kubelet restart #1027

Closed

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2021

Merge branch 'kubernetes-sigs:master' into master

da8dbf3

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2021

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 25, 2021

go formatt error fixed using go v1.17

38d60d9

nirmalaagash force-pushed the master branch from 4f8be26 to 38d60d9 Compare August 25, 2021 22:54

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021

k8s-ci-robot merged commit edb4fbe into kubernetes-sigs:master Aug 27, 2021

This was referenced Sep 1, 2021

[release-1.2] Automated cherry pick of #1019: Added Mount Idempotency #1037

Merged

[release-1.1] Automated cherry pick of #1019: Added Mount Idempotency #1039

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 2, 2021

Merge pull request #1037 from ialidzhikov/automated-cherry-pick-of-#1…

4498bbb

…019-origin-release-1.2 [release-1.2] Automated cherry pick of #1019: Added Mount Idempotency

k8s-ci-robot added a commit that referenced this pull request Sep 2, 2021

Merge pull request #1039 from ialidzhikov/automated-cherry-pick-of-#1…

d275c06

…019-origin-release-1.1 [release-1.1] Automated cherry pick of #1019: Added Mount Idempotency

nirmalaagash mentioned this pull request Sep 10, 2021

REQUEST: New membership for nirmalaagash kubernetes/org#2968

Closed

7 tasks

wongma7 mentioned this pull request Mar 25, 2022

Check if publish target path already mounted to be idempotent kubernetes-sigs/aws-efs-csi-driver#651

Closed

dobsonj mentioned this pull request May 16, 2023

idempotent unmount from NodeUnstageVolume / NodeUnpublishVolume #1605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node Publish Mount Idempotent #1019

Node Publish Mount Idempotent #1019

nirmalaagash commented Aug 10, 2021 •

edited

Loading

k8s-ci-robot commented Aug 10, 2021

AndyXiangLi commented Aug 11, 2021

nirmalaagash commented Aug 11, 2021

vdhanan Aug 11, 2021

nirmalaagash Aug 11, 2021

vdhanan Aug 11, 2021

nirmalaagash Aug 11, 2021

vdhanan commented Aug 11, 2021

bertinatto left a comment

nirmalaagash commented Aug 12, 2021

bertinatto Aug 12, 2021

nirmalaagash Aug 18, 2021

nirmalaagash commented Aug 18, 2021 •

edited

Loading

ialidzhikov commented Aug 24, 2021

vdhanan commented Aug 25, 2021

vdhanan commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

nirmalaagash commented Aug 25, 2021

nirmalaagash commented Aug 25, 2021

ialidzhikov commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

ialidzhikov commented Aug 25, 2021

ialidzhikov commented Aug 25, 2021

nirmalaagash commented Aug 25, 2021

ialidzhikov commented Aug 26, 2021

vdhanan commented Aug 27, 2021

ialidzhikov commented Aug 27, 2021

Node Publish Mount Idempotent #1019

Node Publish Mount Idempotent #1019

Conversation

nirmalaagash commented Aug 10, 2021 • edited Loading

k8s-ci-robot commented Aug 10, 2021

AndyXiangLi commented Aug 11, 2021

nirmalaagash commented Aug 11, 2021

vdhanan Aug 11, 2021

Choose a reason for hiding this comment

nirmalaagash Aug 11, 2021

Choose a reason for hiding this comment

vdhanan Aug 11, 2021

Choose a reason for hiding this comment

nirmalaagash Aug 11, 2021

Choose a reason for hiding this comment

vdhanan commented Aug 11, 2021

bertinatto left a comment

Choose a reason for hiding this comment

nirmalaagash commented Aug 12, 2021

bertinatto Aug 12, 2021

Choose a reason for hiding this comment

nirmalaagash Aug 18, 2021

Choose a reason for hiding this comment

nirmalaagash commented Aug 18, 2021 • edited Loading

ialidzhikov commented Aug 24, 2021

vdhanan commented Aug 25, 2021

vdhanan commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

nirmalaagash commented Aug 25, 2021

nirmalaagash commented Aug 25, 2021

ialidzhikov commented Aug 25, 2021

k8s-ci-robot commented Aug 25, 2021

ialidzhikov commented Aug 25, 2021

ialidzhikov commented Aug 25, 2021

nirmalaagash commented Aug 25, 2021

ialidzhikov commented Aug 26, 2021

vdhanan commented Aug 27, 2021

ialidzhikov commented Aug 27, 2021

nirmalaagash commented Aug 10, 2021 •

edited

Loading

nirmalaagash commented Aug 18, 2021 •

edited

Loading