Partially revert "CreateCgroupPath: only enable needed controllers" #2395
Conversation
|
CI error: I think this error is ignored in the past. |
|
Waiting for #2390 complete. |
| // pids (since kernel 4.5) | ||
| if _, ok := m.controllers["pids"]; ok { |
There was a problem hiding this comment.
I believe this function was the only user of controllers map. If there aren't any left, remove it altogether.
Maybe as a separate patch, describing why there's no need for this map and check.
There was a problem hiding this comment.
Oh, there is another user: GetStats!
|
Left a number of comments. It's better to split this into a few smaller patches as there are a few independent things being fixed. |
I quite agree with you. And the purpose of this patch is to let |
|
LGTM, can we also have tests? |
Yes, I think we should add some test cases. I'll add it later. |
lifubang
force-pushed
the
updateCgroupv2
branch
2 times, most recently
from
9e4e363
to
1ed174f
Compare
|
LGTM (if green) |
libcontainer/cgroups/fs2/create.go
Outdated
| if _, ok := avail[controller]; ok { | ||
| list = append(list, "+"+controller) | ||
| return true | ||
| } |
There was a problem hiding this comment.
nit
_, ok := avail[controller]
return ok
libcontainer/cgroups/fs2/fs2.go
Outdated
| @@ -76,8 +76,8 @@ func (m *manager) Apply(pid int) error { | |||
| // - "runc create (rootless + limits + no cgrouppath + no permission) fails with informative error" | |||
| if m.rootless { | |||
| if m.config.Path == "" { | |||
| cl, clErr := neededControllers(m.config) | |||
| if clErr == nil && len(cl) == 0 { | |||
| cl, clErr := needAnyControllers(m.config) | |||
There was a problem hiding this comment.
nit: cl used to mean "controller list". Now it's not so maybe rename to need or something.
ditto for clErr.
1. Partially revert "CreateCgroupPath: only enable needed controllers" If we update a resource which did not limited in the beginning, it will have no effective. 2. Returns err if we use an non enabled controller, or else the user may feel success, but actually there are no effective. Signed-off-by: lifubang <[email protected]>
Signed-off-by: lifubang <[email protected]>
|
LGTM @mrunalp @AkihiroSuda PTAL |
|
LGTM |
fix #2394
Because if we update a resource which did not limited in the beginning, it will
have no effective.
there are no effective.
For #2367 , I think we don't need fully revert 4b4bc99 , because rootless need some part in it to check errors.
Signed-off-by: lifubang [email protected]