-
Notifications
You must be signed in to change notification settings - Fork 7k
[core] (cgroups 8/n) Wiring CgroupManager into the raylet. #56297
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
Conversation
and core worker Signed-off-by: irabbani <[email protected]>
Signed-off-by: irabbani <[email protected]>
a noop build target for non-linux platforms. Signed-off-by: irabbani <[email protected]>
Signed-off-by: irabbani <[email protected]>
Signed-off-by: irabbani <[email protected]>
Signed-off-by: irabbani <[email protected]>
| #ifndef __linux__ | ||
| RAY_LOG(WARNING) | ||
| << "Resource isolation with cgroups is only supported in linux. Please set " | ||
| "enable_resource_isolation to false. This is likely a misconfiguration."; | ||
| #endif |
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 thought it would be useful to add a warning when resource isolation is enabled on non-linux platforms. It's likely a misconfiguration.
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: Ibrahim Rabbani <[email protected]>
Signed-off-by: irabbani <[email protected]>
| DEFINE_string(labels, | ||
| "", | ||
| "Define the key-value format of node labels, which is a serialized JSON."); | ||
| DEFINE_bool( |
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.
Thinking out loud, do we need this boolean flag? Could we infer it to be true if cgroup_path and other configs are present?
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.
Fair point. I see the argument for both. More flags means more configuration possibilities. In this case, I picked an enable_ flag because the config is complex and it simplifies invariant checking and understanding the user's intent.
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.
Ok. I'm not particularly opinionated one way or the other, mostly I guess probing to see if there was a reason to this rhyme and it sounds like you had one in mind. So we can keep this for now on the notion that it'll make it easier to extend later should that come along.
| << cgroup_manager.ToString(); | ||
|
|
||
| #ifndef __linux__ | ||
| RAY_LOG(WARNING) |
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.
Do we need to do a system check to see if cgroups are enabled on the host?
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 warning is for if the user is trying to enable resource isolation on a non-linux system. So Cgroups will not be enabled.
The line above this will create the appropriate target of CgroupManager based on the platform. If it's linux, it will fail if cgroups are not enabled on the host.
RAY_CHECK(cgroup_manager.ok())
<< "Failed to start raylet. Could not create CgroupManager because of "
<< cgroup_manager.ToString();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 misspoke. The following is implemented in 9/n:
The line above this will create the appropriate target of CgroupManager based on the platform.
In this PR, we still do a system check if cgroups are enabled on the host in CgroupManager::Create.
#56352) This PR stacks on #56297. For more details about the resource isolation project see #54703. This PR wires the resource isolation config (introduced here #51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]>
ray-project#56352) This PR stacks on ray-project#56297. For more details about the resource isolation project see ray-project#54703. This PR wires the resource isolation config (introduced here ray-project#51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]> Signed-off-by: Zhiqiang Ma <[email protected]>
ray-project#56352) This PR stacks on ray-project#56297. For more details about the resource isolation project see ray-project#54703. This PR wires the resource isolation config (introduced here ray-project#51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]> Signed-off-by: zac <[email protected]>
#56352) This PR stacks on #56297. For more details about the resource isolation project see #54703. This PR wires the resource isolation config (introduced here #51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]> Signed-off-by: elliot-barn <[email protected]>
ray-project#56352) This PR stacks on ray-project#56297. For more details about the resource isolation project see ray-project#54703. This PR wires the resource isolation config (introduced here ray-project#51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]> Signed-off-by: Marco Stephan <[email protected]>
#56352) This PR stacks on #56297. For more details about the resource isolation project see #54703. This PR wires the resource isolation config (introduced here #51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]> Signed-off-by: elliot-barn <[email protected]>
ray-project#56352) This PR stacks on ray-project#56297. For more details about the resource isolation project see ray-project#54703. This PR wires the resource isolation config (introduced here ray-project#51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]> Signed-off-by: Douglas Strodtman <[email protected]>
ray-project#56352) This PR stacks on ray-project#56297. For more details about the resource isolation project see ray-project#54703. This PR wires the resource isolation config (introduced here ray-project#51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]>
Original PR #56297 by israbbani Original: ray-project/ray#56297
… the raylet. Merged from original PR #56297 Original: ray-project/ray#56297
ray-project#56352) This PR stacks on ray-project#56297. For more details about the resource isolation project see ray-project#54703. This PR wires the resource isolation config (introduced here ray-project#51865) from ray cli (ray start) and the ray sdk (ray.init) into the raylet. Notable changes include: 1. A separate python test target for running test_resource_isolation related unit and integration tests. This unifies all cgroup related tests under one buildkite target and removes the need for `--except-tags cgroup` everywhere else. 2. Modification to the cgroup hierarchy. This was an oversight on my part. The "no internal processes" constraint says that a non-root cgroup can either have controllers enabled or have processes. Therefore, the new hierarchy looks like: ``` // base_cgroup_path (e.g. /sys/fs/cgroup) // | // ray_node_<node_id> // | | // system application // | | // leaf leaf // ``` where the leaf nodes contain all processes and the system/application cgroups apply cpu.weight and memory.min constraints. 3. CgroupManager now has a move ctor/assignment operator that allows ownership and lifecycle to be managed by the NodeManager. 4. CgroupManager is now owned by NodeManager. 5. An end-to-end integration test in `python/ray/tests/resource_isolation/test_resource_isolation_integration.py`. 6. Moved all previous integration tests from test_ray_init and test_cli into test_resource_isolation_integration.py. These tests have TODOs to finish them up once the rest of cgroup features are implemented. --------- Signed-off-by: irabbani <[email protected]> Co-authored-by: Edward Oakes <[email protected]>
This PR stacks on #56285.
For more details about the resource isolation project see #54703.
This PR integrates the CgroupManager with the raylet startup in
main.cc. It includesI've left comments on the PR to highlight parts that could use some feedback/discussion.