Fixed mounting with containerized kubelet #23435
Conversation
Kubelet was not able to mount volumes when running inside a container and using nsenter mounter, NsenterMounter.IsLikelyNotMountPoint() should return ErrNotExist when the checked directory does not exists as the regular mounted does this and some volume plugins depend on this behavior.
|
@kubernetes/sig-storage @kubernetes/rh-storage |
|
Labelling this PR as size/XS |
k8s-github-robot
added
the
size/XS
|
@jsafrane what does findmnt return when you give it a target that doesn't exist? |
|
ping @fgrzadkowski |
|
findmnt just returns exit status 1, I don't think we should depend on that. |
|
@jsafrane I get it now. |
| @@ -170,6 +170,12 @@ func (n *NsenterMounter) IsLikelyNotMountPoint(file string) (bool, error) { | |||
| return true, err | |||
| } | |||
|
|
|||
| // Check the directory exists | |||
| if _, err = os.Stat(file); os.IsNotExist(err) { | |||
There was a problem hiding this comment.
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.
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#L75
There was a problem hiding this comment.
Let's imagine we have a directory /var/lib/kubelet/test which is a mount point which contains directory foo. Now, I think that if we call IsLikelyNotMountPoint for it var/lib/kubelet/test/foo we'd get false for newer versions of findmnt (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.
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 return ErrNotExist to tell the caller to create the directory before mounting.
There was a problem hiding this comment.
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.
@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.
|
GCE e2e build/test passed for commit 92f1811. |
|
This LGTM.. @pmorie are there outstanding issues here ? |
|
LGTM |
pmorie
added
lgtm
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
Removing LGTM because the release note process has not been followed. |
k8s-github-robot
removed
the
lgtm
pmorie
added
the
release-note-none
|
GCE e2e build/test passed for commit 92f1811. |
pmorie
added
the
lgtm
|
Have you tested this code on platforms that do not have |
|
@luxas we haven't -- this code has always used |
|
GCE e2e build/test passed for commit 92f1811. |
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
GCE e2e build/test passed for commit 92f1811. |
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
GCE e2e build/test passed for commit 92f1811. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
@k8s-bot test this issue: #IGNORE Tests have been pending for 24 hours |
|
GCE e2e build/test passed for commit 92f1811. |
|
Automatic merge from submit-queue |
|
GCE e2e build/test passed for commit 92f1811. |
|
@kubernetes/sig-storage should we consider this a cherry-pick candidate for 1.2.x? |
|
Is |
|
@saad-ali yes, I'll flip it to release-note |
ncdc
added
release-note
roberthbailey
added
the
cherry-pick-approved
…35-upstream-release-1.2 Automated cherry pick of #23435
…ck-of-#23435-upstream-release-1.2 Automated cherry pick of kubernetes#23435
…ck-of-#23435-upstream-release-1.2 Automated cherry pick of kubernetes#23435
NsenterMounter.IsLikelyNotMountPoint()should returnErrNotExistwhen thechecked directory does not exists - the regular mounted does this and
some volume plugins depend on this behavior.
See for example: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/aws_ebs/aws_util.go#L72