-
Notifications
You must be signed in to change notification settings - Fork 39.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue in kubelet getMountedVolumePathListFromDisk #87166
Conversation
93fae09
to
5dd339a
Compare
/test pull-kubernetes-integration |
/test pull-kubernetes-e2e-gce |
cc @msau42 |
@kubernetes/sig-storage-pr-reviews |
5dd339a
to
fbaba72
Compare
/retest |
fbaba72
to
e61fbb5
Compare
@msau, comments are addressed. PTAL. |
pkg/kubelet/kubelet_getters.go
Outdated
for _, volumeDir := range volumeDirs { | ||
path := filepath.Join(volumePluginPath, volumeDir) | ||
volumes = append(volumes, path) | ||
csimountpath := filepath.Join(path, "/mount") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse the logic in https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/csi/csi_mounter.go#L84? Maybe pull it out into a public function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually would it be possible to use mounter.GetPath() here for all plugins? Then we don't need plugin-specific logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to get a new mounter, it requires volume spec, and pod id etc. It will need go through the volume spec reconstruction, which is not very easy... I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make a helper function out of
kubernetes/pkg/volume/csi/csi_mounter.go
Line 84 in 468af72
dir := filepath.Join(getTargetPath(c.podUID, c.specVolumeID, c.plugin.host), "/mount") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where will the helper function be placed?
right now, the logic is depending on the plugin name, we define the path differently. You mean we have a helper function to contain this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I made a helper function in csi_util.go. PTAL
pkg/kubelet/kubelet_getters.go
Outdated
// to the list if the mounted path exists. | ||
for _, volumeDir := range volumeDirs { | ||
path := filepath.Join(volumePluginPath, volumeDir) | ||
volumes = append(volumes, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we append this path? Then we're going to have double the entries right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should remove it.
e61fbb5
to
5d8dd85
Compare
@msau, comments are addressed. PTAL. |
/assign |
This PR fixes issue kubernetes#74650. It adds the extra check for /mount dir under pod volume dir. It also adds the unit test for this function
5d8dd85
to
7012994
Compare
@msau42 PTAL |
/retest |
1 similar comment
/retest |
Lgtm as temporary workaround, however, the final solution should include #88759 too. As mentioned here, not all CSI volumes are mounts. Just presence of the volume directory indicates that the CSI volume plugin did not finish its cleanup and TearDown / NodeUnpublish should be called. The orphan pod cleaner should not check for mounts at all. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, jingxu97, msau42 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:
Approvers can indicate their approval by writing |
/release-note-none |
/retest Review the full test history for this PR. Silence the bot with an |
7 similar comments
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
This PR fixes issue #74650. It adds the extra check for /mount dir under
pod volume dir. It also adds the unit test for this function
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #74650
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: