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.

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

oci: add pouch default runtime spec #2411

Merged
merged 3 commits into from
Dec 20, 2018
Merged

oci: add pouch default runtime spec #2411

merged 3 commits into from
Dec 20, 2018

Conversation

Ace-Tang
Copy link
Contributor

@Ace-Tang Ace-Tang commented Nov 2, 2018

use pouch default runtime spec, instead of containerd default spec,
compare to containerd spec, we remove Root, Process(Env Cmd
NoNewPrivileges User Rlimits), Linux(CgroupsPath), this should not
be exist in default spec, remove Mount(/run), this is not used, add
allowed device, add cgroup mount.

Signed-off-by: Ace-Tang aceapril@126.com

Ⅰ. Describe what this PR did

use pouch default runtime spec, instead of containerd default spec,
compare to containerd spec, we remove Root, Process(Env Cmd
NoNewPrivileges User Rlimits), Linux(CgroupsPath), this should not
be exist in default spec, remove Mount(/run), this is not used, add
allowed device, add cgroup mount.

Ⅱ. Does this pull request fix one issue?

fix #2116 , and base on PR #2271close #2115

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

no.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

For the default runtime spec pouch used, I will post an explanation, if have doubt with the new spec , please wait some time.

@codecov
Copy link

codecov bot commented Nov 2, 2018

Codecov Report

Merging #2411 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2411      +/-   ##
==========================================
+ Coverage   69.11%   69.32%   +0.21%     
==========================================
  Files         278      279       +1     
  Lines       18689    18820     +131     
==========================================
+ Hits        12916    13047     +131     
- Misses       4297     4299       +2     
+ Partials     1476     1474       -2
Flag Coverage Δ
#criv1alpha1test 31.94% <97.29%> (+0.7%) ⬆️
#criv1alpha2test 36.23% <97.29%> (+0.65%) ⬆️
#integrationtest 41.06% <100%> (+0.29%) ⬆️
#nodee2etest 33.19% <97.29%> (+0.48%) ⬆️
#unittest 26.76% <0%> (+0.01%) ⬆️
Impacted Files Coverage Δ
daemon/mgr/system.go 67.93% <ø> (-5.35%) ⬇️
ctrd/utils.go 100% <ø> (ø) ⬆️
daemon/mgr/spec_mount.go 85.57% <100%> (+1.17%) ⬆️
oci/spec_default.go 100% <100%> (ø)
daemon/mgr/spec.go 66.66% <100%> (+4.76%) ⬆️
daemon/mgr/snapshot.go 89.85% <0%> (-4.35%) ⬇️
apis/server/utils.go 71.15% <0%> (-3.85%) ⬇️
daemon/mgr/events.go 96.29% <0%> (-3.71%) ⬇️
ctrd/client.go 68.45% <0%> (-2.02%) ⬇️
... and 11 more

@@ -0,0 +1,194 @@
package oci
Copy link
Collaborator

Choose a reason for hiding this comment

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

too many packages in pouch directory,can we merge them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Give some advise ? @rudyfly

oci/spec_default.go Outdated Show resolved Hide resolved
@rudyfly
Copy link
Collaborator

rudyfly commented Dec 3, 2018

I have checked capabilities and mounts, they are right.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 3, 2018
@fuweid
Copy link
Contributor

fuweid commented Dec 3, 2018

@rudyfly, any update here?

@rudyfly
Copy link
Collaborator

rudyfly commented Dec 4, 2018

@fuweid yes, it need update. I remove LGTM

@rudyfly rudyfly removed the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 4, 2018
@Ace-Tang
Copy link
Contributor Author

Ace-Tang commented Dec 4, 2018

/cc @rudyfly

@rudyfly
Copy link
Collaborator

rudyfly commented Dec 20, 2018

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 20, 2018
use pouch default runtime spec, instead of containerd default spec,
compare to containerd spec, we remove Root, Process(Env Cmd
NoNewPrivileges User Rlimits), Linux(CgroupsPath), this should not
be exist in default spec, remove Mount(/run), this is not used, add
allowed device, add cgroup mount.

Signed-off-by: Ace-Tang <aceapril@126.com>
change shm size in mount option instead of append one more size in
option.
remove set shm-size in user-define mount, since if the mount destination
duplicate with runtime spec, it cause a error.

Signed-off-by: Ace-Tang <aceapril@126.com>
Signed-off-by: Ace-Tang <aceapril@126.com>
@Ace-Tang
Copy link
Contributor Author

Update with fix

if sm.Destination == "/dev/shm" && c.HostConfig.ShmSize != nil &&
+                       *c.HostConfig.ShmSize != 0 {
+                       for idx, v := range sm.Options {
+                               if strings.Contains(v, "size=") {
+                                       sm.Options[idx] = fmt.Sprintf("size=%s", strconv.FormatInt(*c.HostConfig.ShmSize, 10))
+                               }
+                       }
                }

and add a test for default shm size, @rudyfly

@rudyfly rudyfly merged commit b680418 into AliyunContainerService:master Dec 20, 2018
@Ace-Tang Ace-Tang deleted the refactor_default_spec branch January 7, 2019 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/test LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[refactor] create runtime spec need refactor
4 participants