[core] Check cgroupv2 mount status by dentiny · Pull Request #51141 · ray-project/ray

dentiny · 2025-03-06T22:23:46Z

This PR does two things:

Update the existing mount status check from linux command to C++ syscalls;
Add check it's cgroupv2 but not cgroupv1 gets mounted.

Additional check:

I checked with cgroupv2 mounted with read-only mode, result ok

[/workspaces/pg_duckdb] (hjiang/fix-httpfs-semantics) 
postgres@5959e7653aeb$ mount | grep cgroup
cgroup on /sys/fs/cgroup type cgroup2 (ro,nosuid,nodev,noexec,relatime)

Signed-off-by: dentiny <dentinyhao@gmail.com>

israbbani

Looks good. I left a few small comments

src/ray/common/cgroup/cgroup_setup.cc

Signed-off-by: dentiny <dentinyhao@gmail.com>

src/ray/common/cgroup/cgroup_setup.cc

src/ray/common/cgroup/cgroup_setup.h

edoakes · 2025-03-07T13:11:46Z

src/ray/common/cgroup/test/cgroup_v2_utils_test.cc

+TEST(CgroupV2UtilsTest, CheckCgroupV2Mount) {
+  EXPECT_TRUE(IsCgroupV2MountedAsRw("/sys/fs/cgroup"));
+  EXPECT_FALSE(IsCgroupV2MountedAsRw("/tmp/non_existent_folder"));
+}


there are more cases we should be testing here:

cgroup v2 is mounted but not readable

cgroup v2 is mounted but read only

cgroup v2 is mounted and rw but in a different path

cgroup v1 is mounted at the path

cgroup v1 and v2 are both mounted

per the suggestion above, I would also change the function to return a detail message about the reason the function returns false and validate those details in this test

let's also ensure that the test is running in an automated fashion. shouldn't require manual setup commands

+1 to more tests and automated testing.

We can't expect everyone's local box to have the correct environment setup for this. Is there an example we can follow for:

Tests that are only enabled in certain CI environments

Setting up new CI environments for this project. (Docker, K8S, VM)

there are more cases we should be testing here:

Is it something we should do in one unit test? That means you need to have root permission to mount and unmount cgroup folder.

can you propose a holistic testing plan that will cover these cases in a way you think makes sense?

Testing plan here: https://docs.google.com/document/d/10qM5J5rv3anKXr1S0OWDZIOk75e6auU5YF90u5EtRjo/edit?tab=t.0#heading=h.o0t94extwg3

Also discussed with Ibrahim offline on the testing cases and return values.

israbbani · 2025-03-07T18:21:47Z

For testing purposes it's probably better to return true/false from this function and add the check/fatal error at the callsite.

@edoakes, I think you get to write tests against the same binary behavior (true/false or failure/non-failure). For fatal errors, I prefer throwing the error as close to the problem as possible so you get more context for debugging.

edoakes · 2025-03-07T19:11:14Z

For testing purposes it's probably better to return true/false from this function and add the check/fatal error at the callsite.

@edoakes, I think you get to write tests against the same binary behavior (true/false or failure/non-failure). For fatal errors, I prefer throwing the error as close to the problem as possible so you get more context for debugging.

Fine w/ me

Signed-off-by: dentiny <dentinyhao@gmail.com>

edoakes

basically all style comments/nits

src/ray/common/cgroup/cgroup_setup.cc

src/ray/common/cgroup/cgroup_setup.h

src/ray/common/cgroup/cgroup_setup.cc

edoakes · 2025-03-19T19:04:45Z

src/ray/common/cgroup/test/cgroup_v2_utils_privileged_test.cc

+}
+
+TEST(CgroupV2UtilsTest, CgroupV2DirectoryNotExist) {
+  EXPECT_FALSE(IsCgroupV2Prepared("/tmp/non_existent_folder").ok());


do we have a RAY_ASSERT_NOT_OK to match RAY_ASSERT_OK? not sure actually

No at the moment: https://github.com/ray-project/ray/blob/master/src/ray/common/test/testing.h

The reason why I didn't wrote the NON_OK at first place is, for error status, we better check status code, instead of merely error or not. Updated to check error status in the latest commit.

src/ray/common/cgroup/test/cgroup_v2_utils_unprivileged_test.cc

edoakes · 2025-03-19T19:08:35Z

the change to return a more detailed error message in the status looks great btw

israbbani

Lgtm. A few small suggests around typos

src/ray/common/cgroup/test/BUILD

src/ray/common/cgroup/test/cgroup_v2_utils_unprivileged_test.cc

src/ray/common/cgroup/cgroup_setup.cc

Signed-off-by: dentiny <dentinyhao@gmail.com>

israbbani

🚢

Signed-off-by: dentiny <dentinyhao@gmail.com>

This PR does two things: - Update the existing mount status check from linux command to C++ syscalls; - Add check it's cgroupv2 but not cgroupv1 gets mounted. Additional check: - I checked with cgroupv2 mounted with read-only mode, result ok ``` [/workspaces/pg_duckdb] (hjiang/fix-httpfs-semantics) postgres@5959e7653aeb$ mount | grep cgroup cgroup on /sys/fs/cgroup type cgroup2 (ro,nosuid,nodev,noexec,relatime) ``` --------- Signed-off-by: dentiny <dentinyhao@gmail.com> Signed-off-by: Dhakshin Suriakannu <d_suriakannu@apple.com>

Check cgroupv2 mount status

2376dd7

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny added the go add ONLY when ready to merge, run all tests label Mar 6, 2025

dentiny requested review from edoakes, israbbani and jjyao March 6, 2025 22:23

dentiny assigned jjyao, israbbani and edoakes Mar 6, 2025

israbbani reviewed Mar 6, 2025

View reviewed changes

src/ray/common/cgroup/cgroup_setup.cc Outdated Show resolved Hide resolved

src/ray/common/cgroup/cgroup_setup.cc Outdated Show resolved Hide resolved

src/ray/common/cgroup/cgroup_setup.cc Outdated Show resolved Hide resolved

dentiny requested a review from israbbani March 7, 2025 00:17

dentiny mentioned this pull request Mar 7, 2025

[core] Implement utils class to setup and cleanup cgroup folder #49941

Merged

extract cgroup constant

b7df013

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny force-pushed the hjiang/check-cgroupv2-mount-status branch from ae73a1a to b7df013 Compare March 7, 2025 01:54

edoakes reviewed Mar 7, 2025

View reviewed changes

dentiny added 2 commits March 18, 2025 21:12

Merge branch 'master' into hjiang/check-cgroupv2-mount-status

e5021d6

refactor test

6df11a1

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny force-pushed the hjiang/check-cgroupv2-mount-status branch from 73b2585 to 6df11a1 Compare March 19, 2025 00:23

dentiny added 5 commits March 19, 2025 00:26

stale comment

3ec07ea

Signed-off-by: dentiny <dentinyhao@gmail.com>

merge tests

859c48a

Signed-off-by: dentiny <dentinyhao@gmail.com>

fix copyright

6effab4

Signed-off-by: dentiny <dentinyhao@gmail.com>

use status as return

371e825

Signed-off-by: dentiny <dentinyhao@gmail.com>

update comment

3cafea7

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny requested a review from edoakes March 19, 2025 01:05

edoakes reviewed Mar 19, 2025

View reviewed changes

Merge branch 'master' into hjiang/check-cgroupv2-mount-status

a915aff

israbbani reviewed Mar 19, 2025

View reviewed changes

address comments

6c744e9

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny added 2 commits March 19, 2025 20:45

test use status code

5c0a201

Signed-off-by: dentiny <dentinyhao@gmail.com>

no manual for unpriviledged test

b2062ac

Signed-off-by: dentiny <dentinyhao@gmail.com>

dentiny requested review from edoakes and israbbani March 19, 2025 21:01

israbbani approved these changes Mar 20, 2025

View reviewed changes

fix unit test for mac/windows

e365b03

Signed-off-by: dentiny <dentinyhao@gmail.com>

edoakes merged commit f7dfe9e into ray-project:master Mar 20, 2025
5 checks passed

hainesmichaelc added the community-backlog label May 22, 2025

Comments

Conversation

dentiny commented Mar 6, 2025

Uh oh!

israbbani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

israbbani commented Mar 7, 2025

Uh oh!

edoakes commented Mar 7, 2025

Uh oh!

edoakes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

edoakes commented Mar 19, 2025

Uh oh!

israbbani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

israbbani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants