YARN-11692. Support mixed cgroup v1/v2 controller structure #6821
Conversation
This file contains 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
|
💔 -1 overall
This message was automatically generated. |
brumi1024
reviewed
May 13, 2024
| @@ -63,35 +63,54 @@ public class ResourceHandlerModule { | |||
| * as resource metrics functionality. We need to ensure that the same | |||
| * instance is used for both. | |||
| */ | |||
| private static volatile CGroupsHandler cGroupsV1Handler; | |||
There was a problem hiding this comment.
Nit, since we're renaming both variables. According to the official terminology:
“cgroup” stands for “control group” and is never capitalized. The singular form is used to designate the whole feature and also as a qualifier as in “cgroup controllers”. When explicitly referring to multiple individual control groups, the plural form “cgroups” is used.
I think cGroupsV1Handler/cGroupsV2Handler should actually be cgroupV1Handler/cgroupV2Handler. I wanted to do this in YARN-11672, but since I haven't touched the original handler variable postponed it - this seems like a good place to change it, if you agree.
| * Returns an initialized, thread-safe CGroupsHandler instance. | ||
| */ | ||
| private static CGroupsHandler getInitializedCGroupsHandler(Configuration conf) | ||
| private static void initializeCGroupsHandlers(Configuration conf) throws ResourceHandlerException { |
There was a problem hiding this comment.
Nit: based on the comment above: initializeCGroupsHandlers -> initializeCgroupHandlers, and the same applies for the individual init methods.
There was a problem hiding this comment.
Fixed the naming here as well.
| ? new CGroupsV2HandlerImpl(conf, PrivilegedOperationExecutor.getInstance(conf)) | ||
| : new CGroupsHandlerImpl(conf, PrivilegedOperationExecutor.getInstance(conf)); | ||
| LOG.debug("Value of CGroupsHandler is: {}", cGroupsHandler); | ||
| if (cGroupsV1Handler == null) { |
There was a problem hiding this comment.
Do we need the null check twice?
There was a problem hiding this comment.
It catched my eye too, but then I found that it is implemented based on the double-checked locking, so I left it there: https://www.baeldung.com/java-singleton-double-checked-locking
| @@ -101,18 +120,18 @@ private static CGroupsHandler getInitializedCGroupsHandler(Configuration conf) | |||
| */ | |||
|
|
|||
| public static CGroupsHandler getCGroupsHandler() { | |||
| return cGroupsHandler; | |||
| return cgroupsV2Enabled ? cGroupsV2Handler : cGroupsV1Handler; | |||
There was a problem hiding this comment.
getCGroupsHandler method is called from DefaultOOMHandler, CGroupElasticMemoryController, which seemingly rely on v1 memory limits, so the related changes should be addressed in YARN-11678. It's also called from the CGroupsResourceCalculator, which will handle the v1/v2 differences separately.
It's called from OCIContainerRuntime, but that class doesn't use it cgroupHandler, DockerLinuxContainerRuntime calls it to check resource limits, and RuncContainerRuntime calls it for checking and storing cpu limits. Having this return the v1 or v2 handler based on the config will result in issues, even in mixed mode, where for example the CPU is mounted in a v1 structure, so I suggest leaving this as is for now, and updating it once DockerLinuxContainerRuntime and RuncContainerRuntime is updated to handle the v2 files.
There was a problem hiding this comment.
Sure, I changed it to return v1 handler only.
| This parameter is optional, and needed to be set only in mixed mode, | ||
| when CGroups v2 is mounted in a different path than Cgroups v1. | ||
| For example, in hybrid mode, CGroups v1 controllers can be mounted in | ||
| /sys/fs/cgroup, while v2 can be mounted in /sys/fs/cgroup/unified folder. |
There was a problem hiding this comment.
v1 controllers can be mounted in /sys/fs/cgroup
Might be a bit misleading in this format, maybe say something like this:
"v1 controllers can be mounted under /sys/fs/cgroup/ (for example /sys/fs/cgroup/cpu,cpuacct)". Otherwise it can seem like the cgroup v2 FS is mounted under a cgroup v1 FS.
There was a problem hiding this comment.
It happened to be a bit misleading indeed, I fixed the phrasing. Thanks for the suggestion!
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: I10a024b3fa88aaeb5dfcd9173f53200b00ccc7df
Change-Id: I1de48e26ce0f827be16a7c435f240c14487d2baa
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: I6b76ff4c8afb2249fa38ff43195e66b446b212c6
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Change-Id: Ib7673c9ba18fd7a4f7edde63972fc1ff66bca7de
|
Thanks @brumi1024 for the review, I updated the patch based on your suggestions, and added unit tests as well. |
Change-Id: I944acfc508bd89391282e262e0b3b9dd398aa2f9
|
💔 -1 overall
This message was automatically generated. |
Change-Id: I6de7b4d683444747662fbf6bfd65581f014b08ce
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
brumi1024
approved these changes
May 15, 2024
brumi1024
approved these changes
May 15, 2024
K0K0V0K
pushed a commit
to K0K0V0K/hadoop
that referenced
this pull request
May 17, 2024
K0K0V0K
pushed a commit
to K0K0V0K/hadoop
that referenced
this pull request
May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change-Id: I10a024b3fa88aaeb5dfcd9173f53200b00ccc7df
Description of PR
There were heavy changes on the device side in cgroup v2. To keep supporting FGPAs and GPUs short term, mixed structures where some of the cgroup controllers are from v1 while others from v2 should be supported. More info: https://dropbear.xyz/2023/05/23/devices-with-cgroup-v2/
How was this patch tested?
Unit tests
Tested on a cluster in mixed mode, where CPU was mounted in v1 and memory was in v2. When running a job, both v1 and v2 control files contained the correct limits.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?