[core] (cgroups 17/n) Removing controller check when adding a constraint#57731
Merged
[core] (cgroups 17/n) Removing controller check when adding a constraint#57731
Conversation
cpu.weight. It needs to be enabled in the parent cgroup. Python integration tests were not running in CI. Signed-off-by: irabbani <israbbani@gmail.com>
Contributor
Author
|
All tests are now running properly |
israbbani
commented
Oct 15, 2025
Comment on lines
-339
to
-351
| // Check if the required controller for the constraint is enabled. | ||
| StatusOr<std::unordered_set<std::string>> available_controllers_s = | ||
| GetEnabledControllers(cgroup_path); | ||
| RAY_RETURN_NOT_OK(available_controllers_s.status()); | ||
| const auto &controllers = available_controllers_s.value(); | ||
| if (controllers.find(controller) == controllers.end()) { | ||
| return Status::InvalidArgument(absl::StrFormat( | ||
| "Failed to apply %s to cgroup %s. To use %s, enable the %s controller.", | ||
| constraint, | ||
| cgroup_path, | ||
| constraint, | ||
| controller)); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
When I wrote this, I assumed that the controller needs to be enabled on the cgroup to apply a constraint. This isn't always the case (e.g. the cpu controller needs to be enabled on a parent cgroup) and the constraint is applied to the child cgroup.
I could keep the check and complicate the API (e.g. pass in the path of the cgroup on which to check that the controller is enabled), but I think it's cleaner for the caller to check first and then enable the constraint. This is what CgroupManager does.
edoakes
approved these changes
Oct 15, 2025
15 tasks
justinyeh1995
pushed a commit
to justinyeh1995/ray
that referenced
this pull request
Oct 20, 2025
…int (ray-project#57731) In ray-project#57269, I introduced a bug: * the `cpu` controller is not enabled on the system and user cgroups because it only needs to be enabled on the parent. * however, CgroupDriver::AddConstraint checked that for every constraint, the matching controller was enabled. This was not caught in CI because the python integration tests were not running. For the time being, I've removed the manual tag and excluded `cgroup` from all other python tests. We need to fix this properly because tests shouldn't exclude other tests but rather include the right targets. --------- Signed-off-by: irabbani <israbbani@gmail.com>
xinyuangui2
pushed a commit
to xinyuangui2/ray
that referenced
this pull request
Oct 22, 2025
…int (ray-project#57731) In ray-project#57269, I introduced a bug: * the `cpu` controller is not enabled on the system and user cgroups because it only needs to be enabled on the parent. * however, CgroupDriver::AddConstraint checked that for every constraint, the matching controller was enabled. This was not caught in CI because the python integration tests were not running. For the time being, I've removed the manual tag and excluded `cgroup` from all other python tests. We need to fix this properly because tests shouldn't exclude other tests but rather include the right targets. --------- Signed-off-by: irabbani <israbbani@gmail.com> Signed-off-by: xgui <xgui@anyscale.com>
elliot-barn
pushed a commit
that referenced
this pull request
Oct 23, 2025
…int (#57731) In #57269, I introduced a bug: * the `cpu` controller is not enabled on the system and user cgroups because it only needs to be enabled on the parent. * however, CgroupDriver::AddConstraint checked that for every constraint, the matching controller was enabled. This was not caught in CI because the python integration tests were not running. For the time being, I've removed the manual tag and excluded `cgroup` from all other python tests. We need to fix this properly because tests shouldn't exclude other tests but rather include the right targets. --------- Signed-off-by: irabbani <israbbani@gmail.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
landscapepainter
pushed a commit
to landscapepainter/ray
that referenced
this pull request
Nov 17, 2025
…int (ray-project#57731) In ray-project#57269, I introduced a bug: * the `cpu` controller is not enabled on the system and user cgroups because it only needs to be enabled on the parent. * however, CgroupDriver::AddConstraint checked that for every constraint, the matching controller was enabled. This was not caught in CI because the python integration tests were not running. For the time being, I've removed the manual tag and excluded `cgroup` from all other python tests. We need to fix this properly because tests shouldn't exclude other tests but rather include the right targets. --------- Signed-off-by: irabbani <israbbani@gmail.com>
Aydin-ab
pushed a commit
to Aydin-ab/ray-aydin
that referenced
this pull request
Nov 19, 2025
…int (ray-project#57731) In ray-project#57269, I introduced a bug: * the `cpu` controller is not enabled on the system and user cgroups because it only needs to be enabled on the parent. * however, CgroupDriver::AddConstraint checked that for every constraint, the matching controller was enabled. This was not caught in CI because the python integration tests were not running. For the time being, I've removed the manual tag and excluded `cgroup` from all other python tests. We need to fix this properly because tests shouldn't exclude other tests but rather include the right targets. --------- Signed-off-by: irabbani <israbbani@gmail.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
Future-Outlier
pushed a commit
to Future-Outlier/ray
that referenced
this pull request
Dec 7, 2025
…int (ray-project#57731) In ray-project#57269, I introduced a bug: * the `cpu` controller is not enabled on the system and user cgroups because it only needs to be enabled on the parent. * however, CgroupDriver::AddConstraint checked that for every constraint, the matching controller was enabled. This was not caught in CI because the python integration tests were not running. For the time being, I've removed the manual tag and excluded `cgroup` from all other python tests. We need to fix this properly because tests shouldn't exclude other tests but rather include the right targets. --------- Signed-off-by: irabbani <israbbani@gmail.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
In #57269, I introduced a bug:
cpucontroller is not enabled on the system and user cgroups because it only needs to be enabled on the parent.This was not caught in CI because the python integration tests were not running. For the time being, I've removed the manual tag and excluded
cgroupfrom all other python tests.We need to fix this properly because tests shouldn't exclude other tests but rather include the right targets.