Use namespace in default cgroup path #1617
Conversation
47cd76f to
39650f5
Compare
Codecov Report
@@ Coverage Diff @@
## master #1617 +/- ##
=======================================
Coverage 46.44% 46.44%
=======================================
Files 26 26
Lines 3542 3542
=======================================
Hits 1645 1645
Misses 1519 1519
Partials 378 378Continue to review full report at Codecov.
|
| @@ -63,7 +65,7 @@ func BenchmarkContainerStart(b *testing.B) { | |||
| b.Error(err) | |||
| return | |||
| } | |||
| spec, err := GenerateSpec(ctx, client, nil, WithImageConfig(image), withTrue()) | |||
| spec, err := GenerateSpec(ctx, client, &containers.Container{ID: b.Name()}, WithImageConfig(image), withTrue()) | |||
There was a problem hiding this comment.
what's the significance of adding an "empty" container with the name filled in for ID? I only ask because our examples still show nil here and other than being passed to the options helpers, I don't see what GenerateSpec does with it.
There was a problem hiding this comment.
Caught my eyes too, I think it was only because it's part of SpecOpt signature. Maybe we should replace it with ID directly there.
There was a problem hiding this comment.
other spec opts use this for more info, like rootfs and snapshotter
There was a problem hiding this comment.
of course it panics.. my eyes were obviously not following the code path :) Anyway..I'm fine generating a separate PR to update the getting started and README.md examples that are passing nil if that's what we want to do.
There was a problem hiding this comment.
Updated the getting started doc
By default, the generated spec will place containers in cgroups by their ids, we need to use the namespace as the cgroup root to avoid containers with the same name being placed in the same cgroup. ``` 11:perf_event:/to/redis 10:freezer:/to/redis 9:memory:/to/redis 8:devices:/to/redis 7:net_cls,net_prio:/to/redis 6:pids:/to/redis 5:hugetlb:/to/redis 4:cpuset:/to/redis 3:blkio:/to/redis 2:cpu,cpuacct:/to/redis 1:name=systemd:/to/redis 11:perf_event:/te/redis 10:freezer:/te/redis 9:memory:/te/redis 8:devices:/te/redis 7:net_cls,net_prio:/te/redis 6:pids:/te/redis 5:hugetlb:/te/redis 4:cpuset:/te/redis 3:blkio:/te/redis 2:cpu,cpuacct:/te/redis 1:name=systemd:/te/redis ``` Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
|
@estesp i see you thumbs up but no merge? Is that how its going to be? |
|
Fine..merging 😆 |
|
@crosbymichael Can we make this an option? Or have another pure default spec function? |
|
@Random-Liu aren't you using |
|
@Random-Liu i thought you were overriding the cgroup path anyways |
By default, the generated spec will place containers in cgroups by their
ids, we need to use the namespace as the cgroup root to avoid
containers with the same name being placed in the same cgroup.
Signed-off-by: Michael Crosby crosbymichael@gmail.com