bugfix: remove pids-limit initial value #1354
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1354 +/- ##
==========================================
- Coverage 17.34% 17.32% -0.02%
==========================================
Files 189 189
Lines 11832 11822 -10
==========================================
- Hits 2052 2048 -4
+ Misses 9633 9627 -6
Partials 147 147
|
cli/common_flags.go
Outdated
| @@ -84,7 +84,7 @@ func addCommonFlags(flagSet *pflag.FlagSet) *container { | |||
|
|
|||
| flagSet.StringVarP(&c.workdir, "workdir", "w", "", "Set the working directory in a container") | |||
| flagSet.Var(&c.ulimit, "ulimit", "Set container ulimit") | |||
| flagSet.Int64Var(&c.pidsLimit, "pids-limit", -1, "Set container pids limit, -1 for unlimited") | |||
| flagSet.Int64Var(&c.pidsLimit, "pids-limit", 0, "Set container pids limit, -1 for unlimited") | |||
There was a problem hiding this comment.
I check the document and I think the unlimited setting should be max.
It's correct?
I check this in my env:
➜ ~ cat /sys/fs/cgroup/pids/user.slice/pids.max
max
There was a problem hiding this comment.
And please update the comment for the pidsLimit.
There was a problem hiding this comment.
you are right, comment maybe confused, -1 here just means in runc, we would not set value for pid.max , it will jump the set. Maybe we can add more comment, but -1 here is really for unlimit set, @fuweid
if cgroup.Resources.PidsLimit != 0 {
31 // "max" is the fallback value.
32 limit := "max"
33
34 if cgroup.Resources.PidsLimit > 0 {
35 limit = strconv.FormatInt(cgroup.Resources.PidsLimit, 10)
36 }
37
38 if err := writeFile(path, "pids.max", limit); err != nil {
39 return err
40 }
41 }
There was a problem hiding this comment.
I think I get something wrong.
There was a problem hiding this comment.
Oh, -1 means will still set max for pids.max, but if pids cgroup is not mount, it will go error, let me re-write the comment.
pids-limit should have zero as initial value, or runtime will fail to set if kernel not support pid-limit, that will fail to start container. Signed-off-by: Ace-Tang <aceapril@126.com>
|
LGTM |
|
BTW, I think we should add some tests for the cgroup settings in the next step. |
|
Yes, cgroup file existing should be check first |
pids-limit should have zero as initial value, or runtime
will fail to set if kernel not support pid-limit, that will
fail to start container.
Signed-off-by: Ace-Tang aceapril@126.com
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews