cgroup,systemd: allow empty slice in cgroupsPath#1812
cgroup,systemd: allow empty slice in cgroupsPath#1812giuseppe merged 1 commit intocontainers:mainfrom
Conversation
Reviewer's GuideEnsure crun’s systemd cgroup driver defaults the slice to “system.slice” or “user.slice” when the slice component is empty in cgroupsPath, mirroring runc behavior, and add a test to validate this logic. Class diagram for updated systemd cgroup slice logicclassDiagram
class libcrun_cgroup_args {
+const char *id
+pid_t pid
+const char *cgroup_path
}
class CgroupSystemd {
+libcrun_cgroup_enter_systemd(args, err)
+get_systemd_scope_and_slice(id, user_slice, cgroup_path, scope, slice)
}
libcrun_cgroup_args <.. CgroupSystemd : uses
CgroupSystemd : +get_systemd_scope_and_slice now takes bool user_slice
CgroupSystemd : +get_systemd_scope_and_slice sets slice to "system.slice" or "user.slice" if empty
CgroupSystemd : +libcrun_cgroup_enter_systemd determines rootless for cgroup v2
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kolyshkin - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/libcrun/cgroup-systemd.c:140` </location>
<code_context>
*n = '\0';
+
+ /* Ref: https://github.com/opencontainers/runc/blob/main/docs/systemd.md#systemd-unit-name-and-placement */
+ if (is_empty_string (*slice))
+ {
+ if (user_slice)
+ *slice = xstrdup ("user.slice");
+ else
+ *slice = xstrdup ("system.slice");
+ }
}
</code_context>
<issue_to_address>
Potential memory leak if *slice was previously allocated.
Free the previous value of *slice before assigning a new value with xstrdup to prevent memory leaks.
</issue_to_address>
### Comment 2
<location> `tests/test_start.py:540` </location>
<code_context>
+
+ conf = base_config()
+ add_all_namespaces(conf)
+ conf['linux']['cgroupsPath'] = ':crun:123' # crun-123.scope
+
+ cid = None
</code_context>
<issue_to_address>
Consider adding a test for a fully empty cgroupsPath.
Also test with cgroupsPath set to an empty string or None to verify correct handling of these edge cases.
</issue_to_address>
### Comment 3
<location> `tests/test_start.py:549` </location>
<code_context>
+ scope = json.loads(state)['systemd-scope']
+
+ want='system.slice'
+ if is_rootless():
+ want='user.slice'
+
</code_context>
<issue_to_address>
Unreachable code or indentation issue.
Please check the indentation and logic to ensure all code paths are reachable and correct.
</issue_to_address>
### Comment 4
<location> `src/libcrun/cgroup-systemd.c:111` </location>
<code_context>
static void
-get_systemd_scope_and_slice (const char *id, const char *cgroup_path, char **scope, char **slice)
+get_systemd_scope_and_slice (const char *id, bool user_slice, const char *cgroup_path,
+ char **scope, char **slice)
{
</code_context>
<issue_to_address>
Consider refactoring get_systemd_scope_and_slice to accept a default_slice string instead of a user_slice boolean, and move rootless logic to the caller.
```c
// 1) Change get_systemd_scope_and_slice signature: drop `user_slice` and add
// const char *default_slice`
-static void get_systemd_scope_and_slice(
- const char *id,
- bool user_slice,
- const char *cgroup_path,
- char **scope,
- char **slice)
+static void get_systemd_scope_and_slice(
+ const char *id,
+ const char *cgroup_path,
+ const char *default_slice,
+ char **scope,
+ char **slice)
{
char *n;
if (!cgroup_path || !*cgroup_path) {
xasprintf(scope, "crun-%s.scope", id);
return;
}
// ... existing scope logic ...
if (slice) {
*slice = xstrdup(cgroup_path);
n = strchr(*slice, ':');
if (n) *n = '\0';
// simplified default‐slice logic
if (is_empty_string(*slice))
*slice = xstrdup(default_slice);
}
}
```
```c
// 2) In the caller, compute default_slice once
int rootless = 0;
if (cgroup_mode == CGROUP_MODE_UNIFIED) {
rootless = is_rootless(err);
if (rootless < 0)
return rootless;
}
const char *default_slice = rootless
? "user.slice"
: "system.slice";
get_systemd_scope_and_slice(
id,
cgroup_path,
default_slice,
&scope,
&slice);
```
Benefits:
- Eliminates the extra `bool user_slice` flag and inner `if/else`
- Moves all rootless logic into the caller
- Reduces nesting and keeps `get_systemd_scope_and_slice` focused on string mangling only
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| conf = base_config() | ||
| add_all_namespaces(conf) | ||
| conf['linux']['cgroupsPath'] = ':crun:123' # crun-123.scope |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for a fully empty cgroupsPath.
Also test with cgroupsPath set to an empty string or None to verify correct handling of these edge cases.
| scope = json.loads(state)['systemd-scope'] | ||
|
|
||
| want='system.slice' | ||
| if is_rootless(): |
There was a problem hiding this comment.
issue: Unreachable code or indentation issue.
Please check the indentation and logic to ensure all code paths are reachable and correct.
| } | ||
|
|
||
| static void | ||
| get_systemd_scope_and_slice (const char *id, const char *cgroup_path, char **scope, char **slice) |
There was a problem hiding this comment.
issue (complexity): Consider refactoring get_systemd_scope_and_slice to accept a default_slice string instead of a user_slice boolean, and move rootless logic to the caller.
// 1) Change get_systemd_scope_and_slice signature: drop `user_slice` and add
// const char *default_slice`
-static void get_systemd_scope_and_slice(
- const char *id,
- bool user_slice,
- const char *cgroup_path,
- char **scope,
- char **slice)
+static void get_systemd_scope_and_slice(
+ const char *id,
+ const char *cgroup_path,
+ const char *default_slice,
+ char **scope,
+ char **slice)
{
char *n;
if (!cgroup_path || !*cgroup_path) {
xasprintf(scope, "crun-%s.scope", id);
return;
}
// ... existing scope logic ...
if (slice) {
*slice = xstrdup(cgroup_path);
n = strchr(*slice, ':');
if (n) *n = '\0';
// simplified default‐slice logic
if (is_empty_string(*slice))
*slice = xstrdup(default_slice);
}
}// 2) In the caller, compute default_slice once
int rootless = 0;
if (cgroup_mode == CGROUP_MODE_UNIFIED) {
rootless = is_rootless(err);
if (rootless < 0)
return rootless;
}
const char *default_slice = rootless
? "user.slice"
: "system.slice";
get_systemd_scope_and_slice(
id,
cgroup_path,
default_slice,
&scope,
&slice);Benefits:
- Eliminates the extra
bool user_sliceflag and innerif/else - Moves all rootless logic into the caller
- Reduces nesting and keeps
get_systemd_scope_and_slicefocused on string mangling only
| if 'SYSTEMD' not in get_crun_feature_string(): | ||
| return 77 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if is_rootless(): | ||
| want='user.slice' |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if got != want: | ||
| got = subprocess.check_output(['systemctl', '--user', 'show','-PSlice', scope], close_fds=False).decode().strip() |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if got != want: | ||
| sys.stderr.write("# Got Slice %s, want %s\n" % got, want) | ||
| return 1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if cid is not None: | ||
| run_crun_command(["delete", "-f", cid]) |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if cid is not None: | ||
| run_crun_command(["delete", "-f", cid]) | ||
|
|
||
| if is_rootless(): |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
|
Ephemeral COPR build failed. @containers/packit-build please check. |
1 similar comment
|
Ephemeral COPR build failed. @containers/packit-build please check. |
While it may not be properly documented in runtime-spec, runc's systemd cgroup driver follows some rules when constructing the slice and scope from the linux.cgroupsPath (see [1]). One such rule is, when the slice is empty, it defaults to "system.slice", unless we have cgroup v2 and a rootless container, in which case it defaults to "user.slice". This is supported by runc and although it might be questionable, it makes sense for crun to be compatible. Add a test case. Fixes: 1811. [1]: https://github.com/opencontainers/runc/blob/main/docs/systemd.md#systemd-unit-name-and-placement Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
TMT tests failed. @containers/packit-build please check. |
2 participants
While it may not be properly documented in runtime-spec, runc's systemd cgroup driver follows some rules when constructing the slice and scope from the linux.cgroupsPath (see 1).
One such rule is, when the slice is empty, it defaults to "system.slice", unless we have cgroup v2 and a rootless container, in which case it defaults to "user.slice". This is supported by runc and although it might be questionable, it makes sense for crun to be compatible.
Add a test case.
Fixes: #1811.
Summary by Sourcery
Allow cgroup systemd driver to fall back to system.slice or user.slice for empty slices in cgroupsPath, aligning crun with runc behavior and ensuring compatibility.
New Features:
Enhancements:
Tests: