Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix issue in kubelet getMountedVolumePathListFromDisk #87166

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Fix issue in kubelet getMountedVolumePathListFromDisk #87166

merged 1 commit into from
Jun 25, 2020

Conversation

jingxu97
Copy link
Contributor

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?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

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.:

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 13, 2020
@jingxu97 jingxu97 added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 13, 2020
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jan 13, 2020
@jingxu97 jingxu97 force-pushed the Jan/mountcheckfix branch 2 times, most recently from 93fae09 to 5dd339a Compare January 14, 2020 00:42
@jingxu97
Copy link
Contributor Author

/test pull-kubernetes-integration

@jingxu97
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@jingxu97 jingxu97 requested a review from msau42 January 16, 2020 17:50
@jingxu97
Copy link
Contributor Author

cc @msau42

@msau42
Copy link
Member

msau42 commented Jan 16, 2020

@kubernetes/sig-storage-pr-reviews

@jingxu97
Copy link
Contributor Author

/retest

@jingxu97
Copy link
Contributor Author

@msau, comments are addressed. PTAL.

msau42
msau42 reviewed Feb 1, 2020
View reviewed changes
pkg/kubelet/kubelet_getters.go Outdated
for _, volumeDir := range volumeDirs {
path := filepath.Join(volumePluginPath, volumeDir)
volumes = append(volumes, path)
csimountpath := filepath.Join(path, "/mount")
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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")
that doesn't depend on mounter so that we don't have two places that define the path?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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)
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should remove it.

@jingxu97
Copy link
Contributor Author

@msau, comments are addressed. PTAL.

@msau42
Copy link
Member

msau42 commented May 6, 2020

/assign

@jingxu97
Fix issue in kubelet getMountedVolumePathListFromDisk
7012994 
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
@jingxu97
Copy link
Contributor Author

@msau42 PTAL

@jingxu97
Copy link
Contributor Author

/retest

1 similar comment
@jingxu97
Copy link
Contributor Author

/retest

@jsafrane
Copy link
Member

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.

@msau42
Copy link
Member

msau42 commented Jun 23, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2020
@dashpole
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2020
@msau42
Copy link
Member

msau42 commented Jun 23, 2020

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 23, 2020
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

7 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 14c6964 into kubernetes:master Jun 25, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@dashpole dashpole Awaiting requested review from dashpole

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

Invalid path to check in volume existence test for CSI volumes
6 participants
@jingxu97 @msau42 @jsafrane @dashpole @k8s-ci-robot @fejta-bot