-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Fixed mounting with containerized kubelet #23435
Merged
k8s-github-robot
merged 1 commit into
kubernetes:master
from
jsafrane:devel/fix-nsenter-mkdir
Apr 13, 2016
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
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.
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.
This is going to run in the container's mount namespace -- don't we need to do this check in the host's mount ns?
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.
Kubernetes runs with /var/lib/kubernetes mounted to the same directory in the container so it works.
If you look e.g. into AWS volume plugin, it just calls
mkdir()
there without any nsenter. https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/aws_ebs/aws_util.go#L75There 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.
Isn't it
/var/lib/kubelet
?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.
Let's imagine we have a directory
/var/lib/kubelet/test
which is a mount point which contains directoryfoo
. Now, I think that if we callIsLikelyNotMountPoint
for itvar/lib/kubelet/test/foo
we'd get false for newer versions offindmnt
(see comment in line 186). If that's the case (I haven't tested this) your change would introduce a semantic change as now we'd return true.I'm not saying it's a bad change, but we should say it explicitly.
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.
yes, it's
/var/lib/kubelet
, sorry.@fgrzadkowski, I don't understand your point. If
var/lib/kubelet/test/foo
exist, this patch does not change a thing. Only if it doesn't we returnErrNotExist
to tell the caller to create the directory before mounting.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.
I agree. The change only affects the situation when the directory does not exist. I just want to make sure we are clear about semantics.
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.
@fgrzadkowski Personally I think this change is correct, and that the semantics are clear. Prior to this patch it is ambiguous how the dir-doesn't-exist case is handled; this makes nsenter_mount.go consistent with mount.go and documents the semantics.