Skip to content

[core] Creating non-linux implementation for sysfs_cgroup_driver. #56483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Sep 15, 2025
Merged

[core] Creating non-linux implementation for sysfs_cgroup_driver. #56483

merged 10 commits into from
Sep 15, 2025

Conversation

@israbbani
Copy link
Contributor

@israbbani israbbani commented Sep 12, 2025 •
edited
Loading

MacOS build is failing on post-merge. The problem was introduced by #56260.

This PR provides a noop implementation for the sysfs_cgroup_driver target for non-linux systems.

@israbbani israbbani added core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests labels Sep 12, 2025
@israbbani israbbani marked this pull request as ready for review September 12, 2025 17:07
@israbbani israbbani requested a review from a team as a code owner September 12, 2025 17:07
@israbbani israbbani changed the title [core] attempting to fix mac-os build [core] Creating non-linux implementation for sysfs_cgroup_driver. Sep 12, 2025
@israbbani
Copy link
Contributor Author

israbbani commented Sep 12, 2025

Windows test failures are unrelated. MacOS build passes in https://buildkite.com/ray-project/premerge/builds/48663

israbbani
israbbani commented Sep 12, 2025
View reviewed changes
src/ray/common/cgroup2/sysfs_cgroup_driver.cc
Comment on lines +40 to +45
// Used to identify if a filesystem is mounted using cgroupv2.
// See: https://docs.kernel.org/admin-guide/cgroup-v2.html#mounting
#ifndef CGROUP2_SUPER_MAGIC
#define CGROUP2_SUPER_MAGIC 0x63677270
#endif

Copy link
Contributor Author

@israbbani israbbani Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved here to make the header file support non-linux systems.

can-anyscale
can-anyscale reviewed Sep 12, 2025
View reviewed changes
Copy link
Contributor

@can-anyscale can-anyscale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/ray/common/cgroup2/sysfs_cgroup_driver.h
* @param mount_file_path only used for testing.
*/
explicit SysFsCgroupDriver(std::string mount_file_path = MOUNTED)
explicit SysFsCgroupDriver(std::string mount_file_path = kMountFilePath)
Copy link
Contributor

@can-anyscale can-anyscale Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was /etc/mtab on line 45 and now it's /mnt/mtab? - also some comments about MOUNTED are not longer valid

Copy link
Contributor Author

@israbbani israbbani Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I've deleted the comments and changed it to /etc/mtab. I need to figure out why the cgroup tests passed with the wrong mount path.

Copy link
Contributor Author

@israbbani israbbani Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the relevant integration tests are introduced in a later PR. This would break tests in #56352.

@can-anyscale can-anyscale self-requested a review September 12, 2025 17:39
@can-anyscale
Copy link
Contributor

can-anyscale commented Sep 12, 2025

the bazel build refactoring looks good to me

@can-anyscale can-anyscale enabled auto-merge (squash) September 12, 2025 18:12
@israbbani
Copy link
Contributor Author

israbbani commented Sep 12, 2025

Summarizing my offline discussions with @jjyao here. I think the true-north for cross-platform development is to have

  1. Public headers from a module that needs to provide cross-platform support (e.g. CgroupManagerInterface)
  2. A factory that creates the right concrete implementation for each platform.
  3. Separate implementation files (linux_cgroup_manager.cc vs noop_cgroup_manager.cc).

This has a few benefits:

  1. All of the implementation details are hidden from the callers. No more platform-specific ifdefs scattered through the code.
  2. The callers program against interfaces only which makes code cleaner and speeds up compilation times.
  3. The binaries for each platform are not bloated with irrelevant code.

For the Cgroups module, after the feature is code complete, I'll refactor this to

  1. Export only two public interfaces (that hide all implementation details): the names are not set in stone but something to the effect of ProcessIsolationFactory and ProcessIsolationManagerInterface.
  2. The factory will compile with the correct dependencies based on the platform (following the example here).
  3. There will be no need for a NoopCgroupDriver because the Manager's interface will not call it if Cgroups are not supported.

Once the pattern is tested and well established, I plan on trying to use it on our other process management code in process.{h|cc}. We can further use this to also provide separate implementations for different kernel versions for Linux.

@github-actions github-actions bot disabled auto-merge September 14, 2025 18:48
@dayshah dayshah enabled auto-merge (squash) September 14, 2025 18:50
@dayshah dayshah disabled auto-merge September 15, 2025 02:17
@dayshah dayshah merged commit f896b68 into master Sep 15, 2025
5 checks passed
@dayshah dayshah deleted the irabbani/fixing-cgroup-driver-include branch September 15, 2025 09:18
edoakes pushed a commit to edoakes/ray that referenced this pull request Sep 15, 2025
@israbbani @jjyao @edoakes
[core] Creating non-linux implementation for sysfs_cgroup_driver. (ra…
b225301 
…y-project#56483)

Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: israbbani <[email protected]>
Co-authored-by: Ibrahim Rabbani <[email protected]>
Co-authored-by: Jiajun Yao <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
@israbbani israbbani mentioned this pull request Sep 15, 2025
Open
16 tasks
@pcmoritz pcmoritz mentioned this pull request Sep 16, 2025
Closed
ZacAttack pushed a commit to ZacAttack/ray that referenced this pull request Sep 24, 2025
@israbbani @jjyao @ZacAttack
[core] Creating non-linux implementation for sysfs_cgroup_driver. (ra…
adb9d56 
…y-project#56483)

Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: israbbani <[email protected]>
Co-authored-by: Ibrahim Rabbani <[email protected]>
Co-authored-by: Jiajun Yao <[email protected]>
Signed-off-by: zac <[email protected]>
marcostephan pushed a commit to marcostephan/ray that referenced this pull request Sep 24, 2025
@israbbani @jjyao @marcostephan
[core] Creating non-linux implementation for sysfs_cgroup_driver. (ra…
002e38c 
…y-project#56483)

Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: israbbani <[email protected]>
Co-authored-by: Ibrahim Rabbani <[email protected]>
Co-authored-by: Jiajun Yao <[email protected]>
Signed-off-by: Marco Stephan <[email protected]>
dstrodtman pushed a commit that referenced this pull request Oct 6, 2025
@israbbani @jjyao @dstrodtman
[core] Creating non-linux implementation for sysfs_cgroup_driver. (#5…
9b02d01 
…6483)

Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: israbbani <[email protected]>
Co-authored-by: Ibrahim Rabbani <[email protected]>
Co-authored-by: Jiajun Yao <[email protected]>
Signed-off-by: Douglas Strodtman <[email protected]>
justinyeh1995 pushed a commit to justinyeh1995/ray that referenced this pull request Oct 20, 2025
@israbbani @jjyao @justinyeh1995
[core] Creating non-linux implementation for sysfs_cgroup_driver. (ra…
03e8e39 
…y-project#56483)

Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: israbbani <[email protected]>
Co-authored-by: Ibrahim Rabbani <[email protected]>
Co-authored-by: Jiajun Yao <[email protected]>
landscapepainter pushed a commit to landscapepainter/ray that referenced this pull request Nov 17, 2025
@israbbani @jjyao @landscapepainter
[core] Creating non-linux implementation for sysfs_cgroup_driver. (ra…
92f0aca 
…y-project#56483)

Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: israbbani <[email protected]>
Co-authored-by: Ibrahim Rabbani <[email protected]>
Co-authored-by: Jiajun Yao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjyao jjyao jjyao approved these changes

@can-anyscale can-anyscale can-anyscale approved these changes

Assignees

No one assigned

Labels

core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@israbbani @can-anyscale @jjyao @dayshah