[core] (cgroups 6/n) CgroupManager cleans up the entire cgroup hierarchy in reverse order when destroyed. #56260
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
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 |
|---|---|---|
|
|
@@ -113,48 +113,65 @@ StatusOr<std::unique_ptr<CgroupManager>> CgroupManager::Create( | |
| return cgroup_manager; | ||
| } | ||
|
|
||
| void CgroupManager::RegisterDeleteCgroup(const std::string &cgroup_path) { | ||
| cleanup_operations_.emplace_back([this, cgroup = cgroup_path]() { | ||
| Status s = this->cgroup_driver_->DeleteCgroup(cgroup); | ||
| if (!s.ok()) { | ||
| RAY_LOG(WARNING) << absl::StrFormat( | ||
| "Failed to delete cgroup %s with error %s.", cgroup, s.ToString()); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| void CgroupManager::RegisterMoveAllProcesses(const std::string &from, | ||
| const std::string &to) { | ||
| cleanup_operations_.emplace_back([this, from_cgroup = from, to_cgroup = to]() { | ||
| Status s = this->cgroup_driver_->MoveAllProcesses(from_cgroup, to_cgroup); | ||
| if (!s.ok()) { | ||
| RAY_LOG(WARNING) << absl::StrFormat( | ||
|
Contributor
There was a problem hiding this comment. nit: WARNING vs. ERROR? Also, what is the potential implication of this failing? If we're unable to move processes into their appropriate cgroup, then.... what, they'll just run without the resource limits we'd otherwise like imposted? Should we keep running in that scenario or bail out?
Contributor
There was a problem hiding this comment. I guess this line of questioning applies to delete too, but.... process move seems more concerning.
Contributor
Author
There was a problem hiding this comment. This is during clean up. When CgroupManager starts up with a The reason for doing this is that the default cgroup path, and the most commonly anticipated pattern, will be to run this with However, the root cgroup for the container is not the actual root cgroup of the operating system. This means that it is subject to the No Internal Processes constraint. It cannot have child cgroups and a controller enabled if it has processes inside it. The implication for clean up is that the cgroup hierarchy will not be deleted properly. This is mostly fine when it's running inside a container. Given this, maybe ERROR makes more sense? |
||
| "Failed to move all processes from %s to %s with error %s", | ||
| from_cgroup, | ||
| to_cgroup, | ||
| s.ToString()); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| template <typename T> | ||
| void CgroupManager::RegisterRemoveConstraint(const std::string &cgroup, | ||
| const Constraint<T> &constraint) { | ||
| cleanup_operations_.emplace_back( | ||
| [this, constrained_cgroup = cgroup, constraint_to_remove = constraint]() { | ||
| std::string default_value = std::to_string(constraint_to_remove.default_value_); | ||
| Status s = this->cgroup_driver_->AddConstraint(constrained_cgroup, | ||
| constraint_to_remove.controller_, | ||
| constraint_to_remove.name_, | ||
| default_value); | ||
| if (!s.ok()) { | ||
| RAY_LOG(WARNING) << absl::StrFormat( | ||
| "Failed to set constraint %s=%s to default value for cgroup %s with error " | ||
| "%s.", | ||
| constraint_to_remove.name_, | ||
| default_value, | ||
| constrained_cgroup, | ||
| s.ToString()); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| void CgroupManager::RegisterDisableController(const std::string &cgroup_path, | ||
| const std::string &controller) { | ||
| cleanup_operations_.emplace_back( | ||
| [this, cgroup = cgroup_path, controller_to_disable = controller]() { | ||
| Status s = this->cgroup_driver_->DisableController(cgroup, controller_to_disable); | ||
| if (!s.ok()) { | ||
| RAY_LOG(WARNING) << absl::StrFormat( | ||
| "Failed to disable controller %s for cgroup %s with error %s", | ||
| controller_to_disable, | ||
| cgroup, | ||
| s.ToString()); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| Status CgroupManager::Initialize(int64_t system_reserved_cpu_weight, | ||
|
|
@@ -168,11 +185,11 @@ Status CgroupManager::Initialize(int64_t system_reserved_cpu_weight, | |
| cpu_weight_constraint_.Max() - system_reserved_cpu_weight; | ||
|
|
||
| RAY_LOG(INFO) << absl::StrFormat( | ||
| "Initializing CgroupManager at base cgroup at '%s'. Ray's cgroup " | ||
| "hierarchy will under the node cgroup at '%s'. The %s controllers will be " | ||
| "enabled. " | ||
| "The system cgroup at '%s' will have constraints [%s=%lld, %s=%lld]. " | ||
| "The application cgroup '%s' will have constraints [%s=%lld].", | ||
| base_cgroup_path_, | ||
| node_cgroup_path_, | ||
| supported_controllers, | ||
|
|
||
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
Oops, something went wrong.
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.
Out of curiosity, why remove the log? (I won't insist on bringing the log back, just curious to the motivation)
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 left it in by mistake from the previous PR. I was using it to debug a test.