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

bugfix: fix cgroup-parent can not set from config #1361

Merged
merged 1 commit into from
May 21, 2018
Merged

bugfix: fix cgroup-parent can not set from config #1361

merged 1 commit into from
May 21, 2018

Conversation

Ace-Tang
Copy link
Contributor

@Ace-Tang Ace-Tang commented May 21, 2018

remove initial value in cgroup-parent flag, or this flag
can never be set from config file when create container
with pouch cli.

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

Ⅰ. Describe what this PR did

func (mgr *ContainerManager) createContainerdContainer(ctx context.Context, c *Container) error {
    // CgroupParent from HostConfig will be first priority to use,
    // then will be value from mgr.Config.CgroupParent
    if c.HostConfig.CgroupParent == "" { 
        c.HostConfig.CgroupParent = mgr.Config.CgroupParent
    }    

If pouch cli give cgroup-parent a default value, the c.HostConfig.CgroupParent will never be empty, and it can never use CgroupParent set from daemon config file.

The default cgroup parent we already set in

    // same with containerd use. or make it a variable
    cgroupsParent := "default"
    if c.HostConfig.CgroupParent != "" {
        cgroupsParent = c.HostConfig.CgroupParent
    }   

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

remove initial value in cgroup-parent flag, or this flag
can never be set from config file when create container
with pouch cli.

Signed-off-by: Ace-Tang <aceapril@126.com>
@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels May 21, 2018
@codecov-io
Copy link

Codecov Report

Merging #1361 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1361   +/-   ##
=======================================
  Coverage   17.34%   17.34%           
=======================================
  Files         189      189           
  Lines       11832    11832           
=======================================
  Hits         2052     2052           
  Misses       9633     9633           
  Partials      147      147
Impacted Files Coverage Δ
cli/common_flags.go 0% <0%> (ø) ⬆️

@HusterWan
Copy link
Contributor

Reasonable !!!, Thanks a lot @Ace-Tang

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 21, 2018
@HusterWan HusterWan merged commit b8b7c01 into AliyunContainerService:master May 21, 2018
@Ace-Tang Ace-Tang deleted the fix_cgroup_parent_set_from_config branch May 21, 2018 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants