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 mount issues in containerized Kubelet #9976

Merged
merged 2 commits into from
Aug 1, 2015
Merged

Fix mount issues in containerized Kubelet #9976

merged 2 commits into from
Aug 1, 2015

Conversation

smarterclayton
Copy link
Contributor

  • nsenter was not located via path in the container
  • nsenter needed '--' to separate command from args
  • we need to search the parent FS at /rootfs in order to find mount/umount/findmnt

This was referenced Jun 17, 2015
Merged
Closed
@k8s-bot
Copy link

k8s-bot commented Jun 17, 2015

GCE e2e build/test passed for commit 4f6542529f7f12e5e3479a67f278a402b7747701.

kelseyhightower
kelseyhightower reviewed Jun 17, 2015
View reviewed changes
pkg/util/mount/nsenter_mount.go
// 4. The Kubelet process must have CAP_SYS_ADMIN (required by nsenter); at
// the present, this effectively means that the kubelet is running in a
// privileged container.
// 5. The volume path used by the Kubelet must be the same inside and outside
// the container and be writable by the container (to initialize volume)
// contents. TODO: remove this requirement.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we document the fact that the container image must contain mount, umount, and findmnt, or the rootfs you mount inside the container does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should

On Jun 17, 2015, at 7:13 PM, Kelsey Hightower notifications@github.com wrote:

In pkg/util/mount/nsenter_mount.go:

// 4. The Kubelet process must have CAP_SYS_ADMIN (required by nsenter); at
// the present, this effectively means that the kubelet is running in a
// privileged container.
+// 5. The volume path used by the Kubelet must be the same inside and outside
+// the container and be writable by the container (to initialize volume)
+// contents. TODO: remove this requirement.
Should we document the fact that the container image must contain mount, umount, and findmnt, or the rootfs you mount inside the container does?

—
Reply to this email directly or view it on GitHub.

@lavalamp
Copy link
Member

Minor comments.

@smarterclayton
Copy link
Contributor Author

Comments addressed - I search for each binary now in a list of paths, first path wins. If doesn't exist will default to /<binary> and the other mount commands will fail and log

@k8s-bot
Copy link

k8s-bot commented Jun 26, 2015

GCE e2e build/test passed for commit 93b14b9.

@lavalamp
Copy link
Member

Code LGTM. @dchen1107 to evaluate for 1.0 & second LGTM.

@lavalamp lavalamp assigned dchen1107 and unassigned lavalamp Jun 26, 2015
@kelseyhightower
Copy link
Contributor

LGTM

@dchen1107
Copy link
Member

@smarterclayton Can we hold this one for a few weeks, right after 1.0 release?

@smarterclayton
Copy link
Contributor Author

Sure.

@smarterclayton smarterclayton mentioned this pull request Jul 2, 2015
Merged
@pmorie
Copy link
Member

pmorie commented Jul 8, 2015

@dchen1107 should this have the 1.0-post label?

@smarterclayton
Copy link
Contributor Author

Docker 1.7 is broken in a container (except on Red Hat distros) because mount propagation is PRIVATE, not SHARED. Mount-prop fix is being tracked in libcontainer, will probably require docker changes as well. The other option may be to start with the hosts mount namespace.

@erictune erictune added this to the v1.0 milestone Jul 14, 2015
@smarterclayton
Copy link
Contributor Author

@dchen1107 second review?

@lvlv
Copy link
Contributor

lvlv commented Jul 31, 2015

My centos 6.5 + docker 1.7.1 + hyperkube + Kubernetes v1.0.1 is suffering this issue. Is it possible to at least merge "nsenter needed '--' to separate command from args" to release branch?

Here is the log:
I0731 07:52:48.829233 1 nsenter_mount.go:79] nsenter Mounting tmpfs /var/lib/kubelet/pods/a9d8d942-3729-11e5-abda-080027ce083d/volumes/kubernetes.iosecret/dns-token tmpfs []
I0731 07:52:48.829242 1 nsenter_mount.go:82] Mount command: /nsenter [--mount=/rootfs/proc/1/ns/mnt /usr/bin/mount -t tmpfs -o tmpfs /var/lib/kubelet/pods/a9d8d942-3729-11e5-abda-080027ce083d/volumes/kubernetes.iosecret/dns-token]
I0731 07:52:48.830048 1 nsenter_mount.go:86] Output from mount command: nsenter: failed to parse pid: 'tmpfs'
E0731 07:52:48.830076 1 kubelet.go:1225] Unable to mount volumes for pod "kube-dns-fqgan_default": exit status 1; skipping pod

@smarterclayton
Copy link
Contributor Author

I think we already got 1 LGTM, just needs a sign off from Dawn if she wants
to review it.

On Jul 31, 2015, at 5:42 AM, Lv Lv notifications@github.com wrote:

My centos 6.5 + docker 1.7.1 + hyperkube + Kubernetes v1.0.1 is suffering
this issue. Is it possible to at least merge "nsenter needed '--' to
separate command from args" to release branch?

Here is the log:
I0731 07:52:48.829233 1 nsenter_mount.go:79] nsenter Mounting tmpfs
/var/lib/kubelet/pods/a9d8d942-3729-11e5-abda-080027ce083d/volumes/
kubernetes.iosecret/dns-token tmpfs []
I0731 07:52:48.829242 1 nsenter_mount.go:82] Mount command: /nsenter
[--mount=/rootfs/proc/1/ns/mnt /usr/bin/mount -t tmpfs -o tmpfs
/var/lib/kubelet/pods/a9d8d942-3729-11e5-abda-080027ce083d/volumes/
kubernetes.iosecret/dns-token]
I0731 07:52:48.830048 1 nsenter_mount.go:86] Output from mount command:
nsenter: failed to parse pid: 'tmpfs'
E0731 07:52:48.830076 1 kubelet.go:1225] Unable to mount volumes for pod
"kube-dns-fqgan_default": exit status 1; skipping pod

—
Reply to this email directly or view it on GitHub
#9976 (comment)
.

@timothysc timothysc mentioned this pull request Jul 31, 2015
Merged
@timothysc
Copy link
Member

This is very useful for automating scale out setups.

/cc @wojtek-t @rrati @jbeda

@dchen1107 dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2015
@dchen1107
Copy link
Member

Sorry for delaying the review for this PR. LGTM

@dchen1107 dchen1107 removed this from the v1.0 milestone Jul 31, 2015
mikedanese added a commit that referenced this pull request Aug 1, 2015
@mikedanese
Merge pull request #9976 from smarterclayton/nsenter_mount_broken
12b9da8 
Fix mount issues in containerized Kubelet
@mikedanese mikedanese merged commit 12b9da8 into kubernetes:master Aug 1, 2015
@DirectXMan12 DirectXMan12 mentioned this pull request Sep 9, 2015
Merged
brendandburns added a commit that referenced this pull request Sep 10, 2015
@brendandburns
Merge pull request #13739 from DirectXMan12/automated-cherry-pick-of-…
f6d7478 
…#9976-upstream-release-1.0

Automated cherry pick of #9976
@brendandburns brendandburns mentioned this pull request Sep 10, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dchen1107 dchen1107

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet