-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[core] [3/N] Check cgroupv2 mount status #48833
Conversation
Signed-off-by: dentiny <dentinyhao@gmail.com>
src/ray/raylet/cgroup_v2_utils.cc
Outdated
|
||
namespace ray { | ||
|
||
bool IsCgroupV2MountedAsRw() { |
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.
nits: I think you also need to check that the normal user in the container can modify the cgroup path, usually /sys/fs/cgroup
is owned by root
user even though it's writable, it is only writable by the root
.
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.
I think it's a good point, to update cgroupv2, mounted as rw is not enough, current user should also have the permission to write. Add a test to check.
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.
Another thought, another way we could do is we don't do any precheck before actually setting cgroup, which fails directly due to lacking permission.
// Precondition: cgroup V2 has already been mounted as rw. | ||
// | ||
// Setup command: | ||
// sudo umount /sys/fs/cgroup/unified | ||
// sudo mount -t cgroup2 cgroup2 /sys/fs/cgroup/unified -o rw |
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.
qq, I am not familiar but what requires to put these step in ray's cicd?
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.
Don't think it should be placed under CI, which is a group of unit test, but for cgroup related feature they should be tested as integration 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.
That's why you see me mark test as "manual".
Signed-off-by: dentiny <dentinyhao@gmail.com>
@rynewang This PR is ready to review. |
what's relationship of this PR with the other cgroup PRs? Can you give a roadmap of which PR does what? |
I think we just go through the implementation plan this morning? For cgroup setup preparation, there're a few things:
For setting up cgroup for attempt:
|
|
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
a483c05
to
c061931
Compare
Signed-off-by: dentiny <dentinyhao@gmail.com>
Before we prepare cgroup V2 environment at ray-project#48828 and setup cgroup for process at ray-project#48788, we should first check whether cgroupv2 is properly mounted. --------- Signed-off-by: dentiny <dentinyhao@gmail.com> Signed-off-by: Roshan Kathawate <roshankathawate@gmail.com>
Before we prepare cgroup V2 environment at ray-project#48828 and setup cgroup for process at ray-project#48788, we should first check whether cgroupv2 is properly mounted. --------- Signed-off-by: dentiny <dentinyhao@gmail.com> Signed-off-by: lielin.hyl <lielin.hyl@alibaba-inc.com>
Before we prepare cgroup V2 environment at #48828 and setup cgroup for process at #48788, we should first check whether cgroupv2 is properly mounted.