bugfix: container can not write cgroup with privileged #2552
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2552 +/- ##
==========================================
+ Coverage 69.03% 69.13% +0.09%
==========================================
Files 278 278
Lines 18581 18582 +1
==========================================
+ Hits 12828 12847 +19
+ Misses 4272 4264 -8
+ Partials 1481 1471 -10
|
| res := command.PouchRun("run", "--name", name1, busyboxImage, "sh", "-c", "mkdir /sys/fs/cgroup/cpu/test") | ||
| defer DelContainerForceMultyTime(c, name1) | ||
|
|
||
| if res.ExitCode == 0 { |
There was a problem hiding this comment.
The if condition is duplicated with c.Assert(util.PartialEqual(res.Combined(), "Read-only file system"), check.IsNil) . I think that we can remove the if condition. WDYT?
There was a problem hiding this comment.
Copy from other test in test/cli_run_with_privileged_test.go, author may think error stdout is not enough to judge error, I agree with he. But I do not mind to remove it.
There was a problem hiding this comment.
If you do not mind, I will do this in next pr, and fix all tests in test/cli_run_with_privileged_test.go
|
I think we must refact the function if c.HostConfig.Privileged {
if !s.Root.Readonly {
// Clear readonly for /sys.
for i := range s.Mounts {
if s.Mounts[i].Destination == "/sys" {
clearReadonly(&s.Mounts[i])
}
}
}
// Clear readonly for cgroup
for i := range s.Mounts {
if s.Mounts[i].Type == "cgroup" {
clearReadonly(&s.Mounts[i])
}
}
}change it: if c.HostConfig.Privileged {
for i := range s.Mounts {
// Clear readonly for /sys.
if s.Mounts[i].Destination == "/sys" && !s.Root.Readonly {
clearReadonly(&s.Mounts[i])
}
// Clear readonly for cgroup
if s.Mounts[i].Type == "cgroup" {
clearReadonly(&s.Mounts[i])
}
}
}As the same reason, we also can merge this for loop with above code. Can you refact this function? |
clear ro in mount option when container get privileged, make cgroup writable, add test for it. Signed-off-by: Ace-Tang <[email protected]>
|
@rudyfly , updated |
4 participants
clear ro in mount option when container get privileged, make cgroup
writable, add test for it.
Signed-off-by: Ace-Tang [email protected]
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
fixes #2553
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
add test.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews