runtime: merged ValidCgroupPath method #8931
Conversation
|
/test |
There was a problem hiding this comment.
Thx @yaoyinnan could you please split the PR into more one commits to make it clearer ? IMO, the code related to test should be moved into one commit ?
| // see https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#cgroups-path | ||
| func ValidCgroupPathV1(path string, systemdCgroup bool) (string, error) { | ||
| func ValidCgroupPath(path string, v2 bool) (string, error) { |
There was a problem hiding this comment.
Looks like the logic was changed due to systemdCgroup removal
- before: the function throws an error if
systemdCgroup == trueand the path is not in the format of systemd cgroup. - now: giving a new path without any errors.
There was a problem hiding this comment.
@justxuewei Thank you for your review, this issue is indeed worthy of discussion.
The IsSystemdCgroup() is repeated. When calling the ValidCgroupPath() method, the incoming systemdCgroup is obtained through IsSystemdCgroup(path).
This logic seems to be intended to handle the following situation: when calling the ValidCgroupPath(), the parameter systemdCgroup==true is actively passed in, but in fact, IsSystemdCgroup(path) is judged to be false inside the function.
I'm not sure if I want to keep this design from before, please give advice, thanks.
There was a problem hiding this comment.
IsSystemdCgroup(path)is judged to be false inside the function.
I still think they are different. Let me list all possible cases here.
| index | path | systemdCgroup | result |
|---|---|---|---|
| 1 | systemd | true | (origin path, nil) |
| 2 | systemd | false | (origin path, nil) |
| 3 | non-systemd | true | ("", error) |
| 4 | non-systemd | false | (new path, nil) |
Case 3 means I expect the path is in the format of systemd, but it isn't actually. Then, an error is required to be thrown. The case is unreachable if systemdCgroup is removed. In other words, no errors will be thrown in the current ValidCgroupPath(). Wdyt?
There was a problem hiding this comment.
I fixed this question and the email issue.
|
Btw, your email in |
yaoyinnan
force-pushed
the
8930/feat/merge-ValidCgroupPath
branch
from
9d09a35
to
b11419c
Compare
yaoyinnan
force-pushed
the
8930/feat/merge-ValidCgroupPath
branch
2 times, most recently
from
55e61c7
to
d535f6f
Compare
|
/test |
Merged ValidCgroupPath method to handle cgroupv1 and cgroupv2. Fixes: kata-containers#8930 Signed-off-by: yaoyinnan <35447132+yaoyinnan@users.noreply.github.com>
Modify ValidCgroupPath unit test. Fixes: kata-containers#8930 Signed-off-by: yaoyinnan <35447132+yaoyinnan@users.noreply.github.com>
yaoyinnan
force-pushed
the
8930/feat/merge-ValidCgroupPath
branch
from
d535f6f
to
b0b8523
Compare
|
/test |
Merged ValidCgroupPath method and modified unit tests.
Fixes: #8930