Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: howels The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @howels! |
|
Hi @howels. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
Hello, please let me know if anything else is needed here. |
|
/ok-to-test |
9005e01 to
81c1682
Compare
|
Rebased to latest master. |
|
Looks like the Windows units tests are not maintained - using 1.16 and there is some issue with the tests themselves. Trivy scan fails due to usage limits on Trivy. Otherwise this appears to pass tests. |
|
Please can I get a review here? @ZeroMagic @andyzhangx My own testing shows that the additional credentials and checks make the csi-smb driver much more robust. This would be a quality-of-life improvement and reduce the likelihood of stuck mounts. |
There was a problem hiding this comment.
🟡 Changes recommended
Recovery correctness, synchronization, Kerberos credential handling, and Windows behavior need correction before approval.
5 open issues
What changed in this PR
Adds SMB staging recovery during volume publication to address missing globalmount failures reported in #737.
Changes:
- Detects missing staging mounts and attempts recovery before publishing.
- Caches Stage credentials and adds optional recovery Secret lookup.
- Adds recovery tests and configuration guidance.
| File | Description |
|---|---|
| pkg/smb/stage_secrets.go | Adds credential cache helpers. |
| pkg/smb/stage_secrets_test.go | Tests cache isolation and deletion. |
| pkg/smb/smb.go | Initializes the credential cache. |
| pkg/smb/nodeserver.go | Adds staging recovery and stale-bind handling. |
| pkg/smb/nodeserver_test.go | Tests recovery and staging detection. |
| known-issues.md | Documents missing staging mounts and workarounds. |
| docs/driver-parameters.md | Documents recovery Secret options. |
Review effort: Balanced
Static PVs set nodeStageSecretRef only. NodePublishVolume has no credentials of its own. Store a process-local copy keyed by volume ID and staging path. Never log it. Drop it on NodeUnstageVolume.
mkdir of the staging path is not a fix. Point nodePublishSecretRef at the same Secret as nodeStageSecretRef so NodePublishVolume can restage after a node-plugin restart.
Use optional secretname and secretnamespace only when Publish must rebuild a missing staging mount after other credential sources are lost.
Releasing the lock after restage lets NodeUnstageVolume remove the share before the pod bind. Keep it through the stale-bind check and the bind.
A missing mount-table entry is a lookup failure. Treating it as a stale bind unmounts a healthy pod mount.
The previous note said a stale bind is unmounted only after restage. Publish checks the mounted target on every call.
Unrelated Secret data must not enter the process cache. Keep username, password, domain, and krb5cc_<uid>.
a636c61 to
ce6f3a0
Compare
|
/retest |
|
@howels: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |


What type of PR is this?
What this PR does / why we need it:
When using this driver I wanted a robust SMB mount experience but when Pods died I was seeing issues with the
globalmountdirectory missing and also with some failed mounts not able to recover if network conditions interrupted the SMB connection or if the local driver Pod experienced an error.This PR solves these issues in my local testing.
Missing globalmount: kubelet can call NodePublishVolume while the staged CIFS mount is gone. The driver used to bind an empty directory into the pod. It now remounts CIFS at the staging path, or returns FailedPrecondition instead of publishing emptiness.
Plugin restart with no Stage secrets: kubelet does not pass nodeStageSecretRef on Publish. The process cache is lost when plugin dies. Publish now loads the optional secretname / secretnamespace volume attributes and fetches that Secret so restage still works after a crash. Existing PVs without those fields are unchanged.
Which issue(s) this PR fixes:
Fixes #
#737
Requirements:
Special notes for your reviewer:
Release note: