-
Notifications
You must be signed in to change notification settings - Fork 7k
[ci] Enable Cgroup support in CI for core #51454
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
849370a
[WIP] Cgroups in CI
israbbani 7b14643
Fixing a minor bug. Adding some debugging log lines
israbbani 94e25e5
Fixing test
israbbani d82dc73
Fixing the test again. Adding the right magic words!
israbbani 51adfd2
Adding a shell test to CI to make sure CI groupv2 is mounted
israbbani 3ad76ea
Adding a proper CI integration test for docker
israbbani 0fe192f
Removing unused code
israbbani 59bbda2
Fixing the CI test
israbbani 7d92f16
Adding --privileged-container flag to CI test
israbbani da908f9
Fixing CI test dependencies
israbbani 216d578
One more
israbbani 5e810e6
hacking the test
israbbani bbc7a14
Skip building ray when using cgroups
israbbani 58123b3
Merge branch 'master' into irabbani/ci-adventures
israbbani 9df5448
Removing a log line and fixing a typo
israbbani 1ae9cd6
Renaming flag to --privileged. Fixing a typo.
israbbani 801ae0c
Changed test scoping for CI
israbbani 1ba8495
Adding a sleep to debug on machine
israbbani a386ce4
One more typo
israbbani 37dd164
Sleeping
israbbani 86cfd09
YAMLLLLL
israbbani 3718e8a
or true!
israbbani f3c5df3
Adding some logging to debug
israbbani 49fc60b
The logs did show me!
israbbani 5b75e72
Moving the --privileged flag before the other flags
israbbani 15e4d89
it would be nice to test these locally
israbbani db50587
Adding an eternal timeout to the test so
israbbani 7f5074e
another one!
israbbani 625b27c
Cleanup
israbbani a0bf010
Debugging again
israbbani a326ebc
And we should be done!
israbbani c7e32af
Final cleanup!
israbbani d8c3e4d
Addressing comments
israbbani 0d18f89
Moving privileged flag back into tester_container
israbbani a57748e
Removing parallel-workers
israbbani e10ce69
Responding to feedback
israbbani File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| from typing import List, Tuple, Optional | ||
|
|
||
|
|
||
| _CUDA_COPYRIGHT = """ | ||
| ========== | ||
| == CUDA == | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import os | ||
| import pytest | ||
| import sys | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| # In privileged containers, we expect the following | ||
| # cgroupv1 is disabled | ||
| # cgroupv2 is enabled and mounted on /sys/fs/cgroup | ||
| # the user running tests has read and write access to the cgroup subtree | ||
| # memory and cpu controllers are enabled | ||
|
|
||
| _MOUNT_FILE_PATH = "/proc/mounts" | ||
| _CGROUP2_PATH = "/sys/fs/cgroup" | ||
| _CTRL_FILE = "cgroup.controllers" | ||
| _EXPECTED_CTRLS = ["memory", "cpu"] | ||
|
|
||
|
|
||
| # mount file format: | ||
| # cgroup /sys/fs/cgroup cgroup2 rw,nosuid,nodev,noexec,relatime 0 0 | ||
| def test_only_cgroupv2_mounted_rw(): | ||
| found_cgroupv2 = False | ||
| found_cgroupv1 = False | ||
| with open(Path(_MOUNT_FILE_PATH)) as f: | ||
| for line in f: | ||
| c = line.split() | ||
| found_cgroupv2 = found_cgroupv2 or ( | ||
| c[2] == "cgroup2" and c[1] == _CGROUP2_PATH and "rw" in c[3] | ||
dentiny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
| found_cgroupv1 = found_cgroupv1 or (c[2] == "cgroup") | ||
| assert found_cgroupv2 and not found_cgroupv1 | ||
|
|
||
|
|
||
| def test_cgroupv2_rw_for_test_user(): | ||
| assert os.access(_CGROUP2_PATH, os.R_OK) and os.access(_CGROUP2_PATH, os.W_OK) | ||
|
|
||
|
|
||
| def test_cgroupv2_controllers_enabled(): | ||
dentiny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| with open(os.path.join(_CGROUP2_PATH, _CTRL_FILE)) as f: | ||
| enabled = f.readlines() | ||
| assert len(enabled) == 1 | ||
| enabled_ctrls = enabled[0].split() | ||
| for expected_ctrl in _EXPECTED_CTRLS: | ||
| assert ( | ||
| expected_ctrl in enabled_ctrls | ||
| ), f"Expected {expected_ctrl} to be enabled for cgroups2, but it is not" | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(pytest.main(["-v", __file__])) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious what is
--config=llvm?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it means inherit
build:llvmThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's defined in
.bazelrcfile. I'm not sure if this is the right one for what we want. @aslonnie maybe--config=clangis better because it's consistent with our other cpp tests?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename can happen in another follow up.
@israbbani maybe checkin the change of this
.bazelrcfile first (and I can approve it), and then your iteration on this PR can have much better cache hit rateThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for some reason the cache hit rate has improved on its own. Maybe one of the microchecks passed and published the docker image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion, my real question is why do we need the config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aslonnie asked me to include it as the base config for C++ testing. It adds the following properties:
My understanding is that it's needed if we're going to use the
cgroupconfig to run C++ tests. Are you asking why we need thecgroupconfig?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm just curious why do we need these llvm config.
Because I don't see it in all C++ related configs, for example
ray/.bazelrc
Lines 128 to 134 in 5e05c2f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. We may need to audit these and removes one that we don't need.