Skip to content

Conversation

@fuweid
Copy link
Contributor

@fuweid fuweid commented May 14, 2018

Signed-off-by: Wei Fu [email protected]

Ⅰ. Describe what this PR did

Mount /sys/fs/cgroup into the container.

Ⅱ. Does this pull request fix one issue?

#1247

Ⅲ. Describe how you did it

Use oci.Spec and containerd will help us to do mount.

Ⅳ. Describe how to verify it

➜  pouch git:(bugfix_add_mount_spec_in_oci) pouch run -it registry.hub.docker.com/library/busybox:1.28 sh
/ # ls -al /sys/fs/cgroup/
total 0
dr-xr-xr-x   13 root     root           340 May 14 09:16 .
drwxr-xr-x    9 root     root             0 May 14 08:20 ..
drwxr-xr-x    2 root     root             0 May 14 09:16 blkio
lrwxrwxrwx    1 root     root            11 May 14 09:16 cpu -> cpu,cpuacct
drwxr-xr-x    2 root     root             0 May 14 09:16 cpu,cpuacct
lrwxrwxrwx    1 root     root            11 May 14 09:16 cpuacct -> cpu,cpuacct
drwxr-xr-x    2 root     root             0 May 14 09:16 cpuset
drwxr-xr-x    2 root     root             0 May 14 09:16 devices
drwxr-xr-x    2 root     root             0 May 14 09:16 freezer
drwxr-xr-x    2 root     root             0 May 14 09:16 hugetlb
drwxr-xr-x    2 root     root             0 May 14 09:16 memory
lrwxrwxrwx    1 root     root            16 May 14 09:16 net_cls -> net_cls,net_prio
drwxr-xr-x    2 root     root             0 May 14 09:16 net_cls,net_prio
lrwxrwxrwx    1 root     root            16 May 14 09:16 net_prio -> net_cls,net_prio
drwxr-xr-x    2 root     root             0 May 14 09:16 perf_event
drwxr-xr-x    2 root     root             0 May 14 09:16 pids
drwxr-xr-x    2 root     root             0 May 14 09:16 systemd

Ⅴ. Special notes for reviews

@fuweid fuweid requested review from Ace-Tang and allencloud May 14, 2018 09:17
@pouchrobot pouchrobot added areas/test kind/bug This is bug report for project size/M labels May 14, 2018
//
// TODO: The github.com/containerd/containerd/oci provides oci.WithMounts
// in v1.1.0. Before we upgrade it, we use assign way to handle the case.
container.Spec.Mounts = newOCILinuxMounts()
Copy link
Contributor

Choose a reason for hiding this comment

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

@fuweid , can you put these logic into file like daemon/mgr/spec_mount.go, since this is oci spec related ?

@fuweid fuweid changed the title bugfix: mount host's dir into container by oci.Specs [WIP] bugfix: mount host's dir into container by oci.Specs May 14, 2018
@pouchrobot pouchrobot added size/S and removed size/M labels May 14, 2018
@fuweid fuweid changed the title [WIP] bugfix: mount host's dir into container by oci.Specs [WIP] bugfix: mount /sys/fs/cgroup into container May 14, 2018
@fuweid fuweid changed the title [WIP] bugfix: mount /sys/fs/cgroup into container bugfix: mount /sys/fs/cgroup into container May 14, 2018
@fuweid
Copy link
Contributor Author

fuweid commented May 14, 2018

@Ace-Tang I have updated and Please take a look. Thanks

@codecov-io
Copy link

Codecov Report

Merging #1314 into master will decrease coverage by 0.99%.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #1314    +/-   ##
========================================
- Coverage    17.4%   16.41%    -1%     
========================================
  Files         189      183     -6     
  Lines       11767    11314   -453     
========================================
- Hits         2048     1857   -191     
+ Misses       9572     9321   -251     
+ Partials      147      136    -11
Impacted Files Coverage Δ
ctrd/container.go 0% <ø> (ø) ⬆️
daemon/mgr/spec_mount.go 0% <0%> (ø) ⬆️
pkg/utils/utils.go 69.79% <0%> (-2.6%) ⬇️
apis/server/container_bridge.go 0% <0%> (ø) ⬆️
daemon/mgr/container.go 0% <0%> (ø) ⬆️
apis/server/router.go 0% <0%> (ø) ⬆️
daemon/mgr/container_logs.go
daemon/logger/jsonfile/utils.go
daemon/containerio/jsonfile.go
daemon/logger/jsonfile/jsonfile_read.go
... and 4 more

@Ace-Tang
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 14, 2018
@Ace-Tang Ace-Tang merged commit de46ffa into AliyunContainerService:master May 14, 2018
@allencloud
Copy link
Collaborator

I think we could add an integration test case for this feature, right? @fuweid

@fuweid fuweid deleted the bugfix_add_mount_spec_in_oci branch May 14, 2018 13:52
@fuweid
Copy link
Contributor Author

fuweid commented May 14, 2018

@allencloud , yes. I will do that in the following PR.But Reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

areas/test kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants