Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Unmount dev shm and cleanup container when stop/remove sandbox #77

Merged
merged 3 commits into from
Jun 16, 2017
Merged

Unmount dev shm and cleanup container when stop/remove sandbox #77

merged 3 commits into from
Jun 16, 2017

Conversation

Random-Liu
Copy link
Member

Based on #73. The last 2 commits are new.

  1. The 2nd commit unmount sandbox /dev/shm when stop sandbox.
  2. The 3rd commit stop/remove containers inside sandbox when stop/remove sandbox.

@Random-Liu Random-Liu changed the title Unmount dev shm Unmount dev shm and cleanup container when stop/remove sandbox Jun 14, 2017
@Random-Liu Random-Liu added this to the v0.1.0-alpha.1 milestone Jun 14, 2017
@Random-Liu
Copy link
Member Author

Random-Liu commented Jun 14, 2017 •
edited
Loading

With all my recent PRs merged, we could reliably pass 30/36 cri validation test now:

Summarizing 6 Failures:

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support attach [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:183

[Fail] [k8s.io] Security Context runtime should support container with security context [It] runtime should support RunAsUser [security context] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/container.go:326

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support exec [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:127

[Fail] [k8s.io] Streaming runtime should support streaming interfaces [It] runtime should support portforward [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/streaming.go:231

[Fail] [k8s.io] Security Context runtime should support container with security context [It] runtime should support RunAsUserName [security context] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/container.go:326

[Fail] [k8s.io] Networking runtime should support networking [It] runtime should support port mapping with host port and container port [Conformance] 
/home/lantaol/workspace/src/github.com/kubernetes-incubator/cri-tools/pkg/validate/networking.go:206

Ran 36 of 36 Specs in 98.358 seconds
FAIL! -- 30 Passed | 6 Failed | 0 Pending | 0 Skipped --- FAIL: TestE2ECRI (98.36s)
FAIL

Ginkgo ran 1 suite in 1m38.432169568s
Test Suite Failed
exit status 1

All failed tests are caused by known missing features which we are not targeting for the alpha version.
/cc @mikebrow @yujuhong @xlgao-zju @heartlock @Crazykev @mozhuli @kubernetes-incubator/maintainers-cri-containerd

The biggest blockers now are:

  1. Switching to new containerd api. WIP: Switch to containerd client #68
  2. Containerd schema 1 image support. Add schema1 conversion on pull containerd#968

@Random-Liu Random-Liu mentioned this pull request Jun 14, 2017
Closed
42 tasks
runcom
runcom reviewed Jun 14, 2017
View reviewed changes
pkg/server/sandbox_run.go
if !config.GetLinux().GetSecurityContext().GetNamespaceOptions().GetHostIpc() {
if err := c.os.Unmount(getSandboxDevShm(rootDir), unix.MNT_DETACH); err != nil && os.IsNotExist(err) {
glog.Errorf("failed to unmount sandbox shm: %v", err)
if err := c.os.Unmount(getSandboxDevShm(rootDir), unix.MNT_DETACH); err != nil && !os.IsNotExist(err) {
Copy link

Choose a reason for hiding this comment

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

@Random-Liu any reason why this needs to be umounted (if any) at sandbox stop? we do this at sandbox stop and we didn't run into any issues yet

Copy link
Member Author

@Random-Liu Random-Liu Jun 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@runcom I feel like either stop or remove is ok.

The reason why we unmount in sandbox stop now is:

  1. I tried docker, after docker stop, shm is unmounted.
  2. Kubelet may garbage collect (remove) sandbox long after sandbox is stopped, we may not want to keep the shm during that. :)

Copy link

Choose a reason for hiding this comment

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

makes sense, thanks @Random-Liu !!!

Copy link
Member Author

Choose a reason for hiding this comment

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

@runcom Np. :)

@Random-Liu Random-Liu mentioned this pull request Jun 14, 2017
Merged
runcom
runcom reviewed Jun 14, 2017
View reviewed changes
pkg/os/os.go Outdated
func (RealOS) Unmount(target string, flags int) error {
if mounted, err := mount.Mounted(target); err != nil || !mounted {
Copy link

Choose a reason for hiding this comment

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

be careful with the check here, if shm path is under /var/run/... but the mount in the /proc/self/mountinfo is in the form of /run/... you'll get mounted = false, err = nil.
For instance, in CRI-O we store shm under /var/run/CONTID/shm but if you pass that to mount.Mounted it'll say that's not mounted because /proc/self/mountinfo only contains /run/CONTID/shm and that doesn't match.
We do resolve the symlink first.

Copy link
Member Author

Choose a reason for hiding this comment

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

We put everything in /var/lib now. :p May need to consider the directory structure more later.
Will add a TODO here to take care of this in the future.

@Random-Liu Random-Liu force-pushed the unmount-dev-shm branch 2 times, most recently from ecaafe0 to 91638cf Compare June 14, 2017 21:11
@Random-Liu Random-Liu mentioned this pull request Jun 16, 2017
Merged
@Random-Liu
Unmount /dev/shm when stop sandbox.
7f9e026 
Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu
Stop/remove all containers when stop/remove sandbox.
53367bb 
Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu
Update godeps.
57b8b43 
Signed-off-by: Lantao Liu <lantaol@google.com>
mikebrow
mikebrow approved these changes Jun 16, 2017
View reviewed changes
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

nice! LGTM

@mikebrow mikebrow added the lgtm label Jun 16, 2017
@Random-Liu
Copy link
Member Author

Apply LGTM based on #77 (review).

@Random-Liu Random-Liu merged commit 9658159 into containerd:master Jun 16, 2017
@Random-Liu Random-Liu deleted the unmount-dev-shm branch June 16, 2017 20:59
lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017
@Random-Liu
Merge pull request containerd#77 from Random-Liu/unmount-dev-shm
b37c179 
Unmount dev shm and cleanup container when stop/remove sandbox
dcantah pushed a commit to dcantah/cri that referenced this pull request Oct 21, 2020
@kevpar
Merge pull request containerd#77 from dcantah/cp-removesandbox
6d8b78d 
Cherry-pick: Forcibly stop running containers before removal
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@runcom runcom runcom left review comments

@mikebrow mikebrow mikebrow approved these changes

Assignees

@yujuhong yujuhong

@mikebrow mikebrow

Labels
cncf-cla: yes lgtm
Projects
None yet
Milestone
v0.1.0-alpha.1
Development

Successfully merging this pull request may close these issues.
5 participants
@Random-Liu @runcom @mikebrow @yujuhong @k8s-ci-robot