Skip to content

Remove mount namespace from shim#1856

Merged
stevvooe merged 1 commit intocontainerd:masterfrom
crosbymichael:mountns
Dec 2, 2017
Merged

Remove mount namespace from shim#1856
stevvooe merged 1 commit intocontainerd:masterfrom
crosbymichael:mountns

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Dec 1, 2017

Signed-off-by: Michael Crosby [email protected]

@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Dec 1, 2017

@Random-Liu can you try this PR to see if it fixes #1785

I was able to, kinda reproduce, this type of error in the stress tests but it wasn't rootfs related, it was mount related.

containerd-stress -d 10m -c 5                                                          
INFO[0000] pulling docker.io/library/alpine:latest         
INFO[0000] generating spec from image                      
INFO[0000] starting stress test run...                     
ERRO[0496] running container 2-1774                      error="OCI runtime create failed: container_linux.go:296: starting container process caused "process_linux.go:279: applying cgroup configuration for process caused \"open /sys/fs/cgroup/cpuset.cpus: no such file or directory\"": unknown"  
INFO[0600] worker 2 finished                               
INFO[0600] worker 0 finished                               
INFO[0600] worker 1 finished                               
INFO[0600] worker 3 finished                               
INFO[0600] worker 4 finished                               
INFO[0600] ending test run in 600.161 seconds              
INFO[0600] create/start/delete 10739 containers in 600.161 seconds (17.894 c/sec) or (0.056 sec/c)  failures=1

@crosbymichael crosbymichael force-pushed the mountns branch 2 times, most recently from 9f4b3ce to fd0af30 Compare December 1, 2017 21:58
@mlaventure
Copy link
Copy Markdown
Contributor

mlaventure commented Dec 1, 2017

There's already an option for this, why removing the feature entirely?

In which scenarios the mountns causes the issue on a recent kernel?

@crosbymichael
Remove mount namespace from shim
fd2e3cd 
Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Dec 1, 2017

@mlaventure if you remember how we had this initially, we had extra code to disconnect that mounts in the new ns but that broke propagation. We removed that but left the namespace.

However, we are seeing more issues, the zfs issue and I ran into a couple during stress tests with leaking mounts. This fixes my stress test findings and hopefully fixes #1785

@crosbymichael crosbymichael changed the title WIP: Remove mount namespace from shim Remove mount namespace from shim Dec 1, 2017
@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 1, 2017

Codecov Report

Merging #1856 into master will increase coverage by 4.93%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1856      +/-   ##
==========================================
+ Coverage   44.25%   49.18%   +4.93%     
==========================================
  Files          67       86      +19     
  Lines        7105     8244    +1139     
==========================================
+ Hits         3144     4055     +911     
- Misses       3468     3519      +51     
- Partials      493      670     +177
Flag Coverage Δ
#linux 52.6% <ø> (?)
#windows 44.25% <ø> (ø) ⬆️
Impacted Files Coverage Δ
oci/spec_unix.go 98.37% <0%> (ø)
fs/diff_unix.go 56.52% <0%> (ø)
fs/du_unix.go 0% <0%> (ø)
content/local/store_unix.go 80% <0%> (ø)
fs/dtype_linux.go 50% <0%> (ø)
mount/mount.go 0% <0%> (ø)
oci/spec.go 50% <0%> (ø)
fs/hardlink_unix.go 60% <0%> (ø)
mount/mount_linux.go 0% <0%> (ø)
oci/spec_opts.go 0% <0%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6df9f6...fd2e3cd. Read the comment docs.

@mlaventure
Copy link
Copy Markdown
Contributor

mlaventure commented Dec 1, 2017

😞 Shame, it makes the world so much easier not to have to deal with the mounts cleanup

@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Dec 1, 2017

Ya, but you did add all that mount cleanup code already ;)

@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Dec 1, 2017

LGTM

@stevvooe stevvooe merged commit 53c892d into containerd:master Dec 2, 2017
@crosbymichael crosbymichael deleted the mountns branch December 2, 2017 00:01
This was referenced Dec 2, 2017
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@crosbymichael @mlaventure @codecov-io @stevvooe