Fall back to the mount table when NodeUnpublishVolume cannot stat the target path - #1187
Open
notsrch wants to merge 2 commits into
Open
Fall back to the mount table when NodeUnpublishVolume cannot stat the target path#1187notsrch wants to merge 2 commits into
notsrch wants to merge 2 commits into
Conversation
… target path Stat on an NFS mount point returns EACCES when the export rule no longer admits the node. Before this change, that error was returned as-is, so the unpublish failed before umount was attempted. kubelet retried forever and the VolumeAttachment stayed pinned. The mount-table check already used for non-directory targets reads /proc/self/mountinfo and does not touch the filesystem behind the path, so it works even when the target path cannot be stat'd. Any stat error other than not-found now falls back to that check instead of failing outright; ESTALE and EIO on a stale NFS mount are handled the same way. Fixes NetApp#1184 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change description
NodeUnpublishVolume(core/node/unmount.go,unmountGeneric) stats the target path beforeit unmounts, to decide between the directory heuristic and the mount-table lookup. Only
not-found was tolerated; any other
staterror was returned ascodes.Internalbeforeumountwas attempted.On an NFS mount whose export rule no longer admits the node,
staton the mount point itselfreturns
EACCES. kubelet then retries the unpublish forever with the same result: the pod neverleaves Terminating, the node keeps the volume in
status.volumesInUse, and the VolumeAttachmentis never detached. Force-deleting the pod does not help, and the attach/detach controller did not
force-detach within 10 minutes on Kubernetes v1.35. Issue #1184 has the details.
Fix: when
statfails with anything other than not-found, log a warning with the error and takethe existing mount-table branch (
IsMountedreads/proc/self/mountinfoand never touches thefilesystem behind the path). Everything after that point is unchanged:
umount, best-effortremoval of the target path, and tracking-file cleanup. Not-found still returns success as before.
The fallback covers every
staterror rather thanEACCESalone becauseESTALEandEIOon astale share hit the same pre-check and are equally irrelevant to
umount(2); the warning recordswhich one it was.
Why it matters beyond tidiness: with
autoExportPolicy, a per-volume export rule that goesmissing is only written back by a fresh
ControllerPublishVolume(#1181), which Kubernetes onlyissues after the VolumeAttachment is gone. This bug pinned the VolumeAttachment for exactly the
node that needed the republish, so the missing rule denied I/O, the denied I/O failed the
unpublish, the failed unpublish held the attachment, and the held attachment blocked the repair.
The only exit was editing the export policy on the array.
Fixes #1184.
Project tracking
Testing
Summary: three new unit tests cover the fallback and fail against master; the bug was reproduced
deterministically on a live ONTAP cluster, on two 26.06.0-based builds, and the failing call is
the pre-check alone (the same unpublish succeeded on its next retry once access was restored).
With this change built into the same image the same step completes: one warning, unmount, pod
gone in 30 s, attachment released in 33 s.
Unit tests
go build ./core/node/,go vet ./core/node/,gofmt -l core/nodeclean.go test ./core/node/ -count=1green on the branch.core/node/unmount_test.go, all three fail against master'sunmount.go:TestUnmountGeneric_StatError_Mounted_FallsBackToMountTableAndUnmounts:statreturnsEACCES, the mount table says mounted,umountis called, cleanup runs.TestUnmountGeneric_StatError_NotMounted_SkipsUmount:statfails, the mount table saysnot mounted, no
umount, cleanup runs.TestUnmountGeneric_StatError_MountTableError_Wrapped:statfails and the mount-tablelookup also fails; the error is returned wrapped, as the existing non-directory branch does.
Live reproduction (fails on 26.06.0-based builds)
Environment: 3-node RKE2 cluster, Kubernetes v1.35.4, Trident installed by the operator with
enableConcurrency: true. Oneontap-nas-economybackend on ONTAP 9.17.1P1 (REST),autoExportPolicy: true, NFSv3. One RWX PVC mounted by a DaemonSet, so every node holds apublication and a VolumeAttachment. ONTAP queried and edited over REST from inside the cluster.
The node plugin under test was not stock 26.06.0. It was v26.06.0 plus the fixes for #1179
(from PR #1183) and #1181 (registration-driven export rule repair). Neither of those changes
touches
core/node/or the CSI node server, so the unpublish path was stock in both runs. Thecombined build matters for the recovery observations below, not for the failure.
Trigger, from the issue: delete node-b's rule from the volume's qtree export policy on the array
(
DELETE /api/protocols/nfs/export-policies/{id}/rules/{index}, HTTP 200).Run 1 (v26.06.0 + #1179 fix only):
Permission denied.GRPC error: rpc error: code = Internal desc = could not check if the target path (/var/lib/kubelet/pods/<uid>/volumes/kubernetes.io~csi/<pv>/mount) is a directory; stat ...: permission denied. Nodestatus.volumesInUsestill lists the volume; VolumeAttachment still attached.kubectl delete pod --force --grace-period=0.volumesInUseand the VolumeAttachment unchanged.POST .../rules, HTTP 201).Run 2 (v26.06.0 + #1179 + #1181 fixes):
stat ...: permission deniedunpublish failure in the first 35 s; VolumeAttachment held. Identical to run 1.What the two runs show:
statpre-check alone. Nothing else changed between "fails on every retry"and "succeeds on the next retry" except that
statstopped returningEACCES.periodic loop, so a pod whose export access was revoked stays stuck until someone restarts the
node's Trident pod or the node reboots. The unpublish should not depend on that; it should
unmount a path it cannot read.
Retest with this change (passes)
Same cluster, backend, volume and DaemonSet as the reproduction. Image: v26.06.0 plus the #1179
fix, the #1181 fix, and this change. Clean state confirmed first: 3 rules in both policies, 3
publications, 3 VolumeAttachments, all pods Running.
Permission denied, as before.level=warning msg="Could not stat target path; checking the mount table instead." error="stat /var/lib/kubelet/pods/<uid>/volumes/kubernetes.io~csi/<pv>/mount: permission denied".could not check if the target patherrors.No node registration happened during this run, so the #1181 repair path did not take part; the
recovery is this change alone. The not-found path was not separately exercised here; it is
covered by the existing unit test and is unchanged.
Comparison, same step on three builds:
Checklist
AI assistance
Assisted-by: Claude Fable 5 noreply@anthropic.com