Skip to content

Fixed subpath cleanup when /var/lib/kubelet is a symlink. #68741

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
Merged
k8s-ci-robot merged 1 commit into from
Sep 26, 2018
Merged

Fixed subpath cleanup when /var/lib/kubelet is a symlink. #68741

k8s-ci-robot merged 1 commit into from
Sep 26, 2018

Conversation

@jsafrane
Copy link
Member

@jsafrane jsafrane commented Sep 17, 2018

What this PR does / why we need it:
kubelet should resolve symlinks before checking /proc/mounts or /proc/1/mountinfo, just in case /var/lib/kubelet itself is a symlink,

Fixes #68740

Release note:

Fixed pod cleanup when /var/lib/kubelet is a symlink.

/sig storage
@cofyc @msau42 @gnufied PTAL

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 17, 2018
@jsafrane jsafrane mentioned this pull request Sep 17, 2018
Closed
gnufied
gnufied reviewed Sep 17, 2018
View reviewed changes
pkg/util/mount/mount.go Outdated
Copy link
Member

@gnufied gnufied Sep 17, 2018

Choose a reason for hiding this comment

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

err - tests may be?

Copy link
Member Author

@jsafrane jsafrane Sep 17, 2018

Choose a reason for hiding this comment

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

That's the problematic part. e2e test would require changing /var/lib/kubelet into a symlink somehow.

Copy link
Member

@gnufied gnufied Sep 17, 2018

Choose a reason for hiding this comment

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

Can we not write/update unit test for IsNotMountPoint, so as it considers symbolic links properly?

Copy link
Member Author

@jsafrane jsafrane Sep 18, 2018

Choose a reason for hiding this comment

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

Proper IsNotMountPoint unit test would require mounting / bind-mounting stuff, which is impossible in an unit test. I could fake all this, but then it would just test very little piece of code and I don't think it's worth all the effort.

@jsafrane
Copy link
Member Author

jsafrane commented Sep 17, 2018

I can see bunch of subpath tests failing, will investigate tomorrow.

cofyc
cofyc reviewed Sep 18, 2018
View reviewed changes
pkg/util/mount/mount.go Outdated
Copy link
Member

@cofyc cofyc Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should return true (not mounted) here. I think e2e tests failed because of this. In first volume setup, pod volume mount point does not exist.

Copy link
Member Author

@jsafrane jsafrane Sep 18, 2018

Choose a reason for hiding this comment

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

You're right, fixed.

I wonder why some code checks the returned bool without checking err...

@jsafrane
Copy link
Member Author

jsafrane commented Sep 18, 2018

/retest

@cofyc
Copy link
Member

cofyc commented Sep 18, 2018

/kind bug
/sig storage
/lgtm

@k8s-ci-robot k8s-ci-robot added 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. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 18, 2018
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cofyc, jsafrane

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@jsafrane
Copy link
Member Author

jsafrane commented Sep 19, 2018

@saad-ali, this is a bugfix, can we get it into 1.12?

@dims
Copy link
Member

dims commented Sep 19, 2018

cc @tpepper @guineveresaenger

@msau42
Copy link
Member

msau42 commented Sep 19, 2018

Is this a 1.12 regression?

@fejta-bot
Copy link

fejta-bot commented Sep 26, 2018

/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 comment for consistent failures.

@jsafrane
Copy link
Member Author

jsafrane commented Sep 26, 2018

It's not 1.12 regression, it was broken by our security subpath patches in all branches.

@jsafrane
Copy link
Member Author

jsafrane commented Sep 26, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 29cff0d into kubernetes:master Sep 26, 2018
This was referenced Oct 17, 2018
Merged
Merged
@klausenbusk klausenbusk mentioned this pull request Feb 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andyzhangx andyzhangx Awaiting requested review from andyzhangx

@jingxu97 jingxu97 Awaiting requested review from jingxu97

2 more reviewers

@gnufied gnufied gnufied left review comments

@cofyc cofyc cofyc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@cofyc cofyc

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pods stuck in Terminating status when subpath volume is used

7 participants

@jsafrane @cofyc @k8s-ci-robot @dims @msau42 @fejta-bot @gnufied