YARN-11015. Decouple queue capacity with ability to run OPPORTUNISTIC container #3779
Conversation
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
* Adds queueing policies `BY_RESOURCES` and `BY_QUEUE_LEN` at the NM * If `BY_RESOURCES` is specified, the NM will queue as long as it has enough resources to run all pending + running containers, otherwise, it will reject the OPPORTUNISTIC container * If BY_QUEUE_LEN is specified, the NM will only accept as many containers as its queue capacity is configured * Restructure `TestContainerSchedulerQueueing` to accommodate different queueing policies at the NM
|
🎊 +1 overall
This message was automatically generated. |
goiri
reviewed
Dec 13, 2021
| queuePolicy = YarnConfiguration | ||
| .DEFAULT_NM_OPPORTUNISTIC_CONTAINERS_QUEUE_POLICY; | ||
| } else { | ||
| queuePolicy = context.getConf().get( |
There was a problem hiding this comment.
I think Configuration#getEnum() could be an option
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml
Show resolved
Hide resolved
...server/nodemanager/containermanager/scheduler/AllocationBasedResourceUtilizationTracker.java
Show resolved
Hide resolved
| (int) (pMemBytes >> 20) > | ||
| (int) (getContainersMonitor() | ||
| .getPmemAllocatedForContainers() >> 20)) { | ||
| (int) convertBytesToMB(pMemBytes) > |
...org/apache/hadoop/yarn/server/nodemanager/containermanager/scheduler/ContainerScheduler.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public void preTransition(ContainerImpl op, | ||
| org.apache.hadoop.yarn.server.nodemanager.containermanager.container. |
There was a problem hiding this comment.
Didnt you import this? ContainerState should be correct.
...a/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerSchedulerTest.java
Show resolved
Hide resolved
...a/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerSchedulerTest.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private static boolean isSuccessfulRun(final ContainerStatus containerStatus) { | ||
| return containerStatus.getContainerSubState() == ContainerSubState.RUNNING |
There was a problem hiding this comment.
Can we split this?
if (containerStatus.getContainerSubState() == ContainerSubState.RUNNING) {
return true;
}
...
|
|
||
| // Wait for the OContainer to get killed | ||
| BaseContainerManagerTest.waitForNMContainerState(containerManager, | ||
| createContainerId(1), ContainerState.DONE, |
There was a problem hiding this comment.
Single line instead of dropping the 40
|
@goiri thanks for the review! I've updated the PR to address your comments, please have another look. |
|
🎊 +1 overall
This message was automatically generated. |
aajisaka
changed the title
goiri
approved these changes
Jan 6, 2022
...a/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerSchedulerTest.java
Show resolved
Hide resolved
...a/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerSchedulerTest.java
Show resolved
Hide resolved
goiri
reviewed
Jan 6, 2022
| } | ||
|
|
||
| private static OpportunisticContainersQueuePolicy | ||
| getOppContainersQueuePolicyFromConf(final Context context) { |
There was a problem hiding this comment.
getOppContainersQueuePolicyFromConf(final Context context) {: 'getOppContainersQueuePolicyFromConf' has incorrect indentation level 2, expected level should be 6. [Indentation]
|
|
||
| // Wait for the OContainer to get killed | ||
| BaseContainerManagerTest.waitForNMContainerState(containerManager, | ||
| createContainerId(1), ContainerState.DONE,40); |
There was a problem hiding this comment.
createContainerId(1), ContainerState.DONE,40);:50: ',' is not followed by whitespace. [WhitespaceAfter]
|
@goiri updated to fix the checkstyle violations, thanks for the review! |
goiri
approved these changes
Jan 6, 2022
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
@afchung could you please provide details on the testing that was done apart from the unit tests? It would be great if you could add the details to the How was this patch tested section in description of this PR.
|
@GauthamBanasandra thanks for taking a look! In addition to unit tests, we also deployed the change to a production cluster. I've added more details on what we observed in our deployment to the description of the PR. Thanks! |
|
Thanks @afchung. Please allow me 2-3 days for reviewing this PR. |
GauthamBanasandra
requested changes
Jan 12, 2022
| */ | ||
| BY_QUEUE_LEN, | ||
| /** | ||
| * Determines wheether or not to run a container based on the amount of |
...a/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerSchedulerTest.java
Show resolved
Hide resolved
| if (subState == ContainerSubState.DONE) { | ||
| return state == | ||
| org.apache.hadoop.yarn.api.records.ContainerState.COMPLETE; | ||
| } |
There was a problem hiding this comment.
The if check here is redundant, since the condition is already satisfied by switch-case. You can just have
case DONE:
return state == org.apache.hadoop.yarn.api.records.ContainerState.COMPLETE;There was a problem hiding this comment.
Not necessarily, the subState can be COMPLETING, in which case we should not check state == org.apache.hadoop.yarn.api.records.ContainerState.COMPLETE
There was a problem hiding this comment.
I see. Thanks for clarifying. It seems a bit convoluted at first 😅. How about this?
case RUNNING:
case COMPLETING:
return true;
case DONE:
return state == org.apache.hadoop.yarn.api.records.ContainerState.COMPLETE;
default:
return false;
...r/nodemanager/containermanager/scheduler/TestContainerSchedulerOppContainersByResources.java
Show resolved
Hide resolved
...r/nodemanager/containermanager/scheduler/TestContainerSchedulerOppContainersByResources.java
Show resolved
Hide resolved
| @Test | ||
| public void testOpportunisticRunsWhenResourcesAvailable() throws Exception { | ||
| containerManager.start(); | ||
| List<StartContainerRequest> list = new ArrayList<>(); |
There was a problem hiding this comment.
Please rename the variable with a more specific one. The name list is quite generic.
|
|
||
| /** | ||
| * Tests that newly arrived containers after the resources are filled up | ||
| * get killed and never get killed. |
There was a problem hiding this comment.
Could you please the part get killed and never get killed a little more?
There was a problem hiding this comment.
Sorry, this was a typo.
| @Test | ||
| public void testKillOpportunisticWhenNoResourcesAvailable() throws Exception { | ||
| containerManager.start(); | ||
| List<StartContainerRequest> list = new ArrayList<>(); |
There was a problem hiding this comment.
Please use a more specific variable name here.
| */ | ||
| @Test | ||
| public void testOpportunisticRunsWhenResourcesAvailable() throws Exception { | ||
| containerManager.start(); |
There was a problem hiding this comment.
This is repeated in both the tests and can be moved to @before (the test setup method in junit).
...r/nodemanager/containermanager/scheduler/TestContainerSchedulerOppContainersByResources.java
Show resolved
Hide resolved
|
@GauthamBanasandra thanks for the review! I've updated the PR to address your comments, please have another pass. |
|
🎊 +1 overall
This message was automatically generated. |
minni31
reviewed
Jan 13, 2022
| return this.utilizationTracker.hasResourcesAvailable(container); | ||
| } | ||
|
|
||
| private boolean resourceAvailableToQueueOppContainer( |
There was a problem hiding this comment.
IIUC, we are doing this check after localization is done. Shouldn't this check be done at the time of container start to avoid wasting NM resources.
There was a problem hiding this comment.
IIRC I didn't do that since it's much more complicated and requires the ContainerScheduler to expose internal state.
Could you point me to where this can be done cleanly?
There was a problem hiding this comment.
I checked code and looks like it is same case with existing code also. I think there is scope of improvement there and that can be handle as separate jira. @goiri what do you think ?
There was a problem hiding this comment.
For this JIRA we can leave it as is for consistency with the rest.
We can open a JIRA to move it everywhere (it might have side effects).
minni31
reviewed
Jan 13, 2022
...a/org/apache/hadoop/yarn/server/nodemanager/containermanager/BaseContainerSchedulerTest.java
Show resolved
Hide resolved
|
@minni31 thanks for the review! Have replied to your comments, please have a look. |
GauthamBanasandra
requested changes
Jan 15, 2022
| if (subState == ContainerSubState.DONE) { | ||
| return state == | ||
| org.apache.hadoop.yarn.api.records.ContainerState.COMPLETE; | ||
| } |
There was a problem hiding this comment.
I see. Thanks for clarifying. It seems a bit convoluted at first 😅. How about this?
case RUNNING:
case COMPLETING:
return true;
case DONE:
return state == org.apache.hadoop.yarn.api.records.ContainerState.COMPLETE;
default:
return false;
|
@GauthamBanasandra have updated the PR to address your latest comments. Please have a look. |
|
@afchung I think the build failure is temporary and unrelated. I've retriggered the CI run now. |
|
It has got past the failed stage now. |
|
🎊 +1 overall
This message was automatically generated. |
GauthamBanasandra
approved these changes
Jan 20, 2022
minni31
approved these changes
Jan 21, 2022
goiri
approved these changes
Jan 24, 2022
| return this.utilizationTracker.hasResourcesAvailable(container); | ||
| } | ||
|
|
||
| private boolean resourceAvailableToQueueOppContainer( |
There was a problem hiding this comment.
For this JIRA we can leave it as is for consistency with the rest.
We can open a JIRA to move it everywhere (it might have side effects).
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
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.
Description of PR
BY_RESOURCESandBY_QUEUE_LENat the NMBY_RESOURCESis specified, the NM will queue as long as it hasenough resources to run all pending + running containers, otherwise, it
will reject the OPPORTUNISTIC container
BY_QUEUE_LENis specified, the NM will only accept as manycontainers as its queue capacity is configured
TestContainerSchedulerQueueingto accommodate differentqueueing policies at the NM
How was this patch tested?
BY_QUEUE_LEN, the behavior is the same as before and forBY_RESOURCES, we observe that the NM does not accept more containers than its resources can handle