add resource validation after delete #654
Conversation
|
From a quick look, this PR looks good.
I've not tried it on my own though. |
Signed-off-by: Zhou Hao <[email protected]>
zhouhao3
force-pushed
the
resources-validation
branch
from
1626388 to
6c943e8
Compare
zhouhao3
changed the title
|
@dongsupark thanks for your suggestion. |
| util.Fatal(err) | ||
| } | ||
| // Add the container to the cgroup | ||
| ioutil.WriteFile(filepath.Join(pidsPath, "cgrouptset/tasks"), []byte(strconv.Itoa(state.Pid)), 0644) |
There was a problem hiding this comment.
typo: "cgrouptset" != "cgrouptest" from line 26. You could check the return value of ioutil.WriteFile and use a constant for that directory.
| // Create a cgroup | ||
| cgPath := "/sys/fs/cgroup" | ||
| pidsPath := filepath.Join(cgPath, "pids") | ||
| os.Mkdir(filepath.Join(pidsPath, "cgrouptest"), 0755) |
There was a problem hiding this comment.
The test should cleanup (rmdir) that directory after the test is completed.
|
|
||
| // Create a cgroup | ||
| cgPath := "/sys/fs/cgroup" | ||
| pidsPath := filepath.Join(cgPath, "pids") |
There was a problem hiding this comment.
This will not work with cgroup-v2 because the paths are different but that's a larger in OCI.
There was a problem hiding this comment.
I don't know much about cgroup-v2, so should we do validation here in two different cases?
I will learn about cgroup-v2 later. There is still much work to be done about cgroup-v2 in the runtime-tool.
There was a problem hiding this comment.
A lot about the spec is cgroup-v1 only. I don't think we can do add support for v1 and v2 everywhere at once. So I don't think this should block this PR.
| @@ -0,0 +1,84 @@ | |||
| package main | |||
There was a problem hiding this comment.
typos in the filename:
validation/deleate_only_create_resources/delte_only_create_resources.go
^^^^^ ^^^^^
Signed-off-by: Zhou Hao <[email protected]>
zhouhao3
force-pushed
the
resources-validation
branch
from
041521f to
caa32a1
Compare
|
The two commit have resolved the remaining issues in |
|
ping @liangchenye @mrunalp |
|
@liangchenye @vbatts @mrunalp PTAL |
|
I've tested it, it works well. |
|
LGTM |
At present,
DeleteResImplementis implemented, but it is not yet clear how to implementDeleteOnlyCreatedRes. (Write the container id to the existingcgroup tasks?)@liangchenye @dongsupark @wking WDYT?
Signed-off-by: Zhou Hao [email protected]