Do not fail volume attach or publish operation at kubelet if target path directory already exists on the node. #119735

akankshapanse · 2023-08-02T12:22:59Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR addresses issue reported at #119401. This allows kubelet to ignore if the staging target directory or parent publish directory already exists and let corresponding CSI driver handle the situation.

Testing done:
Reproduced original issue seen in the bug mentioned above and after replacing kubelet with this fix, control was successfully passed to CSI driver, instead of failing the volume attach at kubelet level.

Which issue(s) this PR fixes:

#119401

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Do not fail volume attach or publish operation at kubelet if target path directory already exists on the node.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-08-02T12:23:03Z

Please note that we're already in Test Freeze for the release-1.28 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.28.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Aug 2 10:20:21 UTC 2023.

k8s-ci-robot · 2023-08-02T12:23:07Z

Welcome @akankshapanse!

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 · 2023-08-02T12:23:08Z

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 · 2023-08-02T12:23:09Z

Hi @akankshapanse. 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.

xing-yang · 2023-08-02T14:41:51Z

/ok-to-test

xing-yang · 2023-08-02T17:15:06Z

/assign @msau42 @jsafrane @andyzhangx

pkg/volume/csi/csi_attacher.go

jsafrane

I am sorry if I confused you with my previous comments. I wanted just a function that checks for windows mount and then calls os.MkdirAll, just like your original code.

Or do I miss anything?

jsafrane · 2023-09-21T08:02:15Z

pkg/util/filesystem/defaultfs.go

+		// For windows NTFS, check if the path is symlink instead of directory.
+		if err1 == nil && (dir.IsDir() || (dir.Mode()&os.ModeSymlink != 0)) {
+			return nil
+		}


This was already checked above.

jsafrane · 2023-09-21T08:02:23Z

pkg/util/filesystem/defaultfs.go

+		}
+		return &os.PathError{Op: "mkdir", Path: path, Err: syscall.ENOTDIR}
+	}
+	// Slow path: make sure parent exists and then call Mkdir for path.


It should be safe safe to call os.MkdirAll here, no need to reimplement it here.

akankshapanse · 2023-09-25T08:53:30Z

I am sorry if I confused you with my previous comments. I wanted just a function that checks for windows mount and then calls os.MkdirAll, just like your original code.

Or do I miss anything?

@jsafrane do you mean the following?
we should continue to check if path exists (using os.Lstat() as original fix in this PR had proposed) and ensure the path is dir for linux (os.IsDir()) and symlink for windows (dir.Mode()&os.ModeSymlink != 0). If not, call os.MkdirAll() as usual.

jsafrane · 2023-10-02T10:35:31Z

@jsafrane do you mean the following? we should continue to check if path exists (using os.Lstat() as original fix in this PR had proposed) and ensure the path is dir for linux (os.IsDir()) and symlink for windows (dir.Mode()&os.ModeSymlink != 0). If not, call os.MkdirAll() as usual.

Yes. Just like the original fix, but in a common function.

… already exists on windows worker node.

akankshapanse · 2023-10-03T07:41:37Z

@jsafrane do you mean the following? we should continue to check if path exists (using os.Lstat() as original fix in this PR had proposed) and ensure the path is dir for linux (os.IsDir()) and symlink for windows (dir.Mode()&os.ModeSymlink != 0). If not, call os.MkdirAll() as usual.

Yes. Just like the original fix, but in a common function.

Updated the diff as per your suggestion.

jsafrane · 2023-10-03T10:59:37Z

Thanks for the patience!

/lgtm
/approve
/priority important-soon
/retest

k8s-ci-robot · 2023-10-03T10:59:43Z

LGTM label has been added.

Git tree hash: c67ff8c273fc0e9db0fc15250b6a8a9490d8a876

msau42 · 2023-10-06T20:45:35Z

/assign @dims

dims · 2023-10-06T21:31:02Z

/approve
/lgtm

thanks @akankshapanse !

k8s-ci-robot · 2023-10-06T21:31:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akankshapanse, dims, jsafrane

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:

~~pkg/OWNERS~~ [dims]

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

pacoxu · 2023-10-08T02:19:54Z

/retest-required

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels Aug 2, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 2, 2023

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 2, 2023

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 2, 2023

k8s-ci-robot requested review from mauriciopoppe and msau42 August 2, 2023 12:23

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 2, 2023

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 2, 2023

k8s-ci-robot assigned andyzhangx, jsafrane and msau42 Aug 2, 2023

jsafrane reviewed Aug 3, 2023

View reviewed changes

pkg/volume/csi/csi_attacher.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2023

akankshapanse mentioned this pull request Aug 18, 2023

In case of node reboot, pod running on that node goes to "Unknown" state, as kubelet fails to attach the PVC associated #119401

Closed

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2023

akankshapanse force-pushed the fix_kubelet_target_dir_issue branch from b6ab720 to cb600f2 Compare September 20, 2023 07:04

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 20, 2023

jsafrane reviewed Sep 21, 2023

View reviewed changes

Do not fail volume attach operation at kubelet if staging target path…

38935b7

… already exists on windows worker node.

akankshapanse force-pushed the fix_kubelet_target_dir_issue branch from cb600f2 to 38935b7 Compare October 3, 2023 07:38

k8s-ci-robot assigned dims Oct 6, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2023

k8s-ci-robot merged commit c486a08 into kubernetes:master Oct 8, 2023

k8s-ci-robot added this to the v1.29 milestone Oct 8, 2023

akankshapanse mentioned this pull request Oct 10, 2023

REQUEST: New membership for akankshapanse kubernetes/org#4511

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not fail volume attach or publish operation at kubelet if target path directory already exists on the node. #119735

Do not fail volume attach or publish operation at kubelet if target path directory already exists on the node. #119735

akankshapanse commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

xing-yang commented Aug 2, 2023

xing-yang commented Aug 2, 2023

jsafrane left a comment

jsafrane Sep 21, 2023

jsafrane Sep 21, 2023

akankshapanse commented Sep 25, 2023

jsafrane commented Oct 2, 2023

akankshapanse commented Oct 3, 2023

jsafrane commented Oct 3, 2023

k8s-ci-robot commented Oct 3, 2023

msau42 commented Oct 6, 2023

dims commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

pacoxu commented Oct 8, 2023

Do not fail volume attach or publish operation at kubelet if target path directory already exists on the node. #119735

Do not fail volume attach or publish operation at kubelet if target path directory already exists on the node. #119735

Conversation

akankshapanse commented Aug 2, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

k8s-ci-robot commented Aug 2, 2023

xing-yang commented Aug 2, 2023

xing-yang commented Aug 2, 2023

jsafrane left a comment

Choose a reason for hiding this comment

jsafrane Sep 21, 2023

Choose a reason for hiding this comment

jsafrane Sep 21, 2023

Choose a reason for hiding this comment

akankshapanse commented Sep 25, 2023

jsafrane commented Oct 2, 2023

akankshapanse commented Oct 3, 2023

jsafrane commented Oct 3, 2023

k8s-ci-robot commented Oct 3, 2023

msau42 commented Oct 6, 2023

dims commented Oct 6, 2023

k8s-ci-robot commented Oct 6, 2023

pacoxu commented Oct 8, 2023