Add test code linux_cgroups_devices by sat0ken · Pull Request #3000 · youki-dev/youki

sat0ken · 2024-11-21T22:41:46Z

This implements the linux_cgroups_devices validation in #361

YJDoc2 · 2024-11-23T15:16:57Z

Hey @sat0ken thanks for the PR! I missed replying to your message on the issue, but I would have requested you to wait a bit before opening the PR. There is another PR which makes some refactor on test code, and that has been waiting for sometime to get merged. I plan to get that merged first and then ask authors of open test PRs to rebase on the main for the changes. I'll ping you once that is done, so you can rebase, and then I'll review this one.

I also request you to hold off on the relative_devices PR till that is done, but I have assigned that to you on the issue.

sat0ken · 2024-11-24T06:14:46Z

@YJDoc2 OK! I got it. thank you.

YJDoc2 · 2024-11-27T05:06:45Z

Hey @sat0ken , that PR is merged, so you can rebase this once and also work on the relative devices. However, in future I will request to only work on one PR at a time 😅

sat0ken · 2024-11-27T11:42:18Z

@YJDoc2 Thank you for notify.

However, in future I will request to only work on one PR at a time 😅

I'm sorry, I'll be careful from now on.

utam0k · 2024-12-02T12:12:07Z

I'm sorry, I'll be careful from now on.

There is no need to apologize; @YJDoc2 has just suggested a smooth way to develop. I often make this mistake myself 🙃

YJDoc2

Hey, there are some changes needed. Also can you confirm this is impl of linux_cgroups_devices and not linux_cgroups_relative_devices ?

YJDoc2 · 2024-12-03T13:24:47Z

tests/contest/contest/src/tests/cgroups/devices.rs

+        .access(allow.to_string())
+        .typ(dev_type)
+        .major(major)
+        .minor(minor)
+        .access(access)


Why are we setting access twice? Is one of them supposed to be allow instead?

It's my mistake. I fixed it.

0ea31ff

YJDoc2 · 2024-12-03T13:25:40Z

tests/contest/contest/src/tests/cgroups/devices.rs

+        linux_device_build(true, LinuxDeviceType::C, 10, 229, "rwm".to_string()),
+        linux_device_build(true, LinuxDeviceType::B, 8, 20, "rw".to_string()),
+        linux_device_build(true, LinuxDeviceType::B, 10, 200, "r".to_string()),


If we are setting true in all three, we can instead directly set true in the function, and not pass the parameter at all.

I fixed it.

0ea31ff

YJDoc2 · 2024-12-03T13:29:43Z

tests/contest/contest/src/tests/cgroups/devices.rs

+        test_result!(check_container_created(&data));
+        TestResult::Passed


We have to check container is created AND the devices are setup correctly using a function like this .
See the runtimeOutsideValidate call in the original test here , where we call the above function to validate the devices.

I fixed about like original test, read devices.list file, and compare to spec.

dc5a1c1

YJDoc2 · 2024-12-03T13:30:15Z

tests/contest/contest/src/tests/cgroups/devices.rs

+    if let TestResult::Failed(_) = test_result {
+        return test_result;
+    }
+    test_result


This is no-op, we can simply return the test_outside_container here as the last statement in the function block

I fixed it.

dc5a1c1

YJDoc2 · 2024-12-03T13:33:57Z

tests/contest/contest/src/tests/cgroups/devices.rs

+    let spec = SpecBuilder::default()
+        .linux(
+            LinuxBuilder::default()
+                .cgroups_path(Path::new("/runtime-test").join(cgroup_name))


I think this is incorrect. For absolute cgroups path, it should be single level at root like /cgrouptest . Joining another path here would make it relative.

Becouse, I check linux_cgroups_blkio test code, this code look like join path.
For absolute cgroups test, Shouldn't we join path?

https://github.com/youki-dev/youki/blob/main/tests/contest/contest/src/tests/cgroups/blkio.rs#L95

YJDoc2 · 2024-12-03T13:36:29Z

I'm sorry, I'll be careful from now on.

Hey, as @utam0k mentioned, no need to apologize, in fact I apologies if my comment seemed too harsh. We haven't mentioned this in the main issue as well, which we should, and I was suggesting that so it would be easier for us to review the PRs. 😅 😄

sat0ken · 2024-12-04T05:30:56Z

@utam0k @YJDoc2

Thank you review and kind comment!

I apologies if my comment seemed too harsh.

Don't worry about it.
I'm newbie about Rust and container, but I enjoying to contribute to youki. I will fix the code, wait please.

sat0ken · 2025-01-19T02:21:07Z

@YJDoc2

I fixed some code, could you please check it?
I don't know why failed e2e test and I have question.

YJDoc2 · 2025-01-22T14:07:03Z

Hey @sat0ken , apologies I haven't been able to take a look at this. I had been quite busy, and then got a bit sick, and currently just have a lot of things I need to work on 😅 I'll try my best to take a look at this as soon as possible.

Sincere apologies, I should have done better managing this 🙏

sat0ken · 2025-01-22T16:14:43Z

@YJDoc2
There's no need to apologies. Please take good care of yourself. Anytime is ok.

utam0k · 2025-09-07T06:39:58Z

@sat0ken I'll start the review. But please rebase from the main branch before starting to review.

sat0ken · 2025-09-10T12:58:18Z

@utam0k
Thank you for review. I did rebase from main.

utam0k · 2025-09-17T12:17:15Z

@utam0k Thank you for review. I did rebase from main.

It looks like you made a mistake when rebasing.

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

sat0ken · 2025-09-20T15:03:55Z

It looks like you made a mistake when rebasing.

OMG. Sorry, I fix it.

utam0k · 2025-10-17T00:20:00Z

tests/contest/contest/src/tests/cgroups/devices.rs

+            // read major, minor number
+            let major_minor: Vec<&str> = parts[1].split(':').collect();
+            if major_minor.len() != 2 {
+                // ignore invalid format


Why can we ignore invalid formats?

utam0k · 2025-10-17T00:21:52Z

tests/contest/contest/src/tests/cgroups/devices.rs

+    for spec_linux_device in spec_linux_devices {
+        if spec_linux_device.allow() {
+            let mut found = false;
+            for linux_device in linux_devices.clone() {


Do we really need clone()?

utam0k · 2025-10-17T00:23:08Z

tests/contest/contest/src/tests/cgroups/devices.rs

+            let major = if major_minor[0] == "*" {
+                None
+            } else {
+                major_minor[0].parse::<i64>().ok()
+            };
+            let minor = if major_minor[1] == "*" {
+                None
+            } else {
+                major_minor[1].parse::<i64>().ok()
+            };
+            // read access string
+            let access = parts[2].to_string();
+            devices.push(linux_device_build(
+                device_type,
+                major.unwrap(),
+                minor.unwrap(),


If * comes into this logic, I guess it will cause panic.

utam0k · 2025-10-17T00:20:00Z

tests/contest/contest/src/tests/cgroups/devices.rs

+            // read major, minor number
+            let major_minor: Vec<&str> = parts[1].split(':').collect();
+            if major_minor.len() != 2 {
+                // ignore invalid format


Why can we ignore invalid formats?

utam0k · 2025-10-17T00:21:52Z

tests/contest/contest/src/tests/cgroups/devices.rs

+    for spec_linux_device in spec_linux_devices {
+        if spec_linux_device.allow() {
+            let mut found = false;
+            for linux_device in linux_devices.clone() {


Do we really need clone()?

utam0k · 2025-10-17T00:23:08Z

tests/contest/contest/src/tests/cgroups/devices.rs

+            let major = if major_minor[0] == "*" {
+                None
+            } else {
+                major_minor[0].parse::<i64>().ok()
+            };
+            let minor = if major_minor[1] == "*" {
+                None
+            } else {
+                major_minor[1].parse::<i64>().ok()
+            };
+            // read access string
+            let access = parts[2].to_string();
+            devices.push(linux_device_build(
+                device_type,
+                major.unwrap(),
+                minor.unwrap(),


If * comes into this logic, I guess it will cause panic.

YJDoc2 added the kind/test label Nov 23, 2024

YJDoc2 requested changes Dec 3, 2024

View reviewed changes

sat0ken force-pushed the add-test-linux-cgroups-devices branch from 8979a4c to bab0815 Compare September 20, 2025 14:36

sat0ken added 7 commits September 20, 2025 23:48

add test code linux_cgroups_devices

db00767

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

fix format err

122fee4

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

fix linux_device_build func

1d3cf8c

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

read cgroup devices.list and check spec file

cc6a440

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

fix format err

682d46d

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

fix format err

a6aa7bb

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

change spec.clone() to &spec

5a8cf64

Signed-off-by: sat0ken <15720506+sat0ken@users.noreply.github.com>

sat0ken force-pushed the add-test-linux-cgroups-devices branch from 92f1eaa to 5a8cf64 Compare September 20, 2025 14:49

utam0k requested changes Oct 17, 2025

View reviewed changes

		test_result!(check_container_created(&data));
		TestResult::Passed

Conversation

sat0ken commented Nov 21, 2024

Uh oh!

YJDoc2 commented Nov 23, 2024

Uh oh!

sat0ken commented Nov 24, 2024

Uh oh!

YJDoc2 commented Nov 27, 2024

Uh oh!

sat0ken commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

utam0k commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YJDoc2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YJDoc2 commented Dec 3, 2024

Uh oh!

sat0ken commented Dec 4, 2024

Uh oh!

sat0ken commented Jan 19, 2025

Uh oh!

YJDoc2 commented Jan 22, 2025

Uh oh!

sat0ken commented Jan 22, 2025

Uh oh!

utam0k commented Sep 7, 2025

Uh oh!

sat0ken commented Sep 10, 2025

Uh oh!

utam0k commented Sep 17, 2025

Uh oh!

sat0ken commented Sep 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sat0ken commented Nov 27, 2024 •

edited

Loading

utam0k commented Dec 2, 2024 •

edited

Loading