Details
Description
This will allow different policies to customize how/if configuration changes should be applied (for example, a policy might restrict whether a configuration change by a certain user is allowed). This will be enforced by the MutableCSConfigurationProvider.
Attachments
Attachments
- YARN-5949-YARN-5734.001.patch
- 20 kB
- Jonathan Hung
- YARN-5949-YARN-5734.002.patch
- 33 kB
- Jonathan Hung
- YARN-5949-YARN-5734.003.patch
- 39 kB
- Jonathan Hung
- YARN-5949-YARN-5734.004.patch
- 41 kB
- Jonathan Hung
- YARN-5949-YARN-5734.005.patch
- 49 kB
- Jonathan Hung
Activity
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 19s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 26s | Maven dependency ordering for branch |
+1 | mvninstall | 17m 20s | |
-1 | compile | 4m 52s | hadoop-yarn in |
+1 | checkstyle | 1m 2s | |
+1 | mvnsite | 1m 23s | |
+1 | mvneclipse | 0m 55s | |
+1 | findbugs | 2m 34s | |
+1 | javadoc | 1m 7s | |
0 | mvndep | 0m 11s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 8s | the patch passed |
-1 | compile | 5m 10s | hadoop-yarn in the patch failed. |
-1 | javac | 5m 10s | hadoop-yarn in the patch failed. |
-0 | checkstyle | 0m 57s | hadoop-yarn-project/hadoop-yarn: The patch generated 3 new + 284 unchanged - 0 fixed = 287 total (was 284) |
+1 | mvnsite | 1m 21s | the patch passed |
+1 | mvneclipse | 0m 46s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | findbugs | 3m 3s | the patch passed |
-1 | javadoc | 0m 31s | hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 897 unchanged - 0 fixed = 898 total (was 897) |
-1 | unit | 0m 37s | hadoop-yarn-api in the patch failed. |
+1 | unit | 40m 18s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 44s | The patch does not generate ASF License warnings. |
94m 14s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.conf.TestYarnConfigurationFields |
This message was automatically generated.
Thanks for the patch. jhung
Overall looks good.
In the same patch, we could use this new QueueAdminConfigurationMutationPolicy in the REST API for changing YARN scheduler configurations ? So, we could have the end-to-end story for the MutableCSConfigurationProvider ?
Thanks xgong for the review.
Right now the default policy is DefaultConfigurationMutationPolicy. I was thinking the policy would be enforced in MutableCSConfigurationProvider instead of the REST API, so if they want to use the queue admin-based policy they can just configure yarn.scheduler.configuration.mutation.policy.class to QueueAdminConfigurationMutationPolicy.
Thanks jhung for your patch, overall looks good to me as well, few comments/suggestions:
- Not caused by this patch, there's some place of RMWebServices#updateSchedulerConfiguration hardcoded to use CapacityScheduler, could you update to MutableConfScheduler instead? Similarly, we should avoid directly call CapacityScheduler in removeQueue/addQueue/updateQueue as well. There're some cleanups can be done for emoveQueue/addQueue/updateQueue, there're some duplicated logics.
- QueueAdminConfigurationMutationPolicy use regex to parse the configs and get existing queue names, we should avoid doing this. YarnScheduler has a getQueueInfo API, by using that we can get all queues.
- Also we should get existing queues when necessary, I think we can add an API to reinitialize ConfigurationMutationPolicy.
- Please make sure QueueAdminConfigurationMutationPolicy can handle invalid configs (like "x.y.z" and x is not a root queue)
- Is it possible to change a global config (such as yarn.scheduler.capacity.maximum-applications), if so, is following check enough?
while (queue == null) { // We are adding a queue (whose parent we are possibly also adding). // Check ACL of lowest parent queue which already exists. parentPath = parentPath.substring(0, parentPath.lastIndexOf('.')); String parentName = parentPath.lastIndexOf('.') != -1 ? parentPath.substring(parentPath.lastIndexOf('.') + 1) : parentPath; queue = ((CapacityScheduler) rmContext.getScheduler()) .getQueue(parentName); } if (!queue.hasAccess(QueueACL.ADMINISTER_QUEUE, user)) { return false; }
If a global config is updated by admin, should we request admin permission?
Thanks for the comments leftnoteasy, had a few questions/comments:
we should avoid directly call CapacityScheduler in removeQueue/addQueue/updateQueue as well
Seems reasonable, we can also use the getQueueInfo API here, this seems to be the only way to replicate the "getQueue/setQueues" functionality from CapacityScheduler/CapacitySchedulerConfiguration.
QueueAdminConfigurationMutationPolicy use regex to parse the configs and get existing queue names, we should avoid doing this.
This makes sense, should just be able to either grab the second-to-last component of the key, or the component before "accessible-node-labels"/"ordering-policy". I think we can also search for "root", if it is not there then assume it is a global config change.
Also we should get existing queues when necessary, I think we can add an API to reinitialize ConfigurationMutationPolicy.
As long as we access these queues via YarnScheduler#getQueueInfo, is this API still necessary? When the scheduler is reinitialized and the next mutation comes in, it will check against the queues from the most recent reinitialization.
Please make sure QueueAdminConfigurationMutationPolicy can handle invalid configs (like "x.y.z" and x is not a root queue)
This is not implemented yet, but I was thinking of handling this in RMWebServices, there are some cases that have not been handled (e.g. updating config for a queue which doesn't exist shouldn't be allowed, right now it "succeeds" silently). So we can address these cases in a separate jira.
Is it possible to change a global config (such as yarn.scheduler.capacity.maximum-applications), if so, is following check enough?
Yes you're right, this is not handled yet, in fact there is still some handling we need to do in RMWebServices for global configs, we can address this in a separate jira as well. If global config is found then we should check for admin permission as you mentioned.
Attached 002 patch which uses MutableConfScheduler instead of CapacityScheduler. (right now there are still references to CapacitySchedulerConfiguration since the full key is needed, e.g. yarn.scheduler.capacity)
Also changed the way the queue path is extracted from the configuration key in QueueAdminConfigurationMutationPolicy
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 14s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 11s | Maven dependency ordering for branch |
+1 | mvninstall | 17m 48s | |
+1 | compile | 11m 17s | |
+1 | checkstyle | 0m 57s | |
+1 | mvnsite | 1m 16s | |
+1 | mvneclipse | 0m 45s | |
-1 | findbugs | 1m 7s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager in |
+1 | javadoc | 0m 58s | |
0 | mvndep | 0m 9s | Maven dependency ordering for patch |
+1 | mvninstall | 0m 57s | the patch passed |
+1 | compile | 8m 23s | the patch passed |
+1 | javac | 8m 23s | the patch passed |
-0 | checkstyle | 0m 53s | hadoop-yarn-project/hadoop-yarn: The patch generated 8 new + 324 unchanged - 0 fixed = 332 total (was 324) |
+1 | mvnsite | 1m 11s | the patch passed |
+1 | mvneclipse | 0m 41s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
-1 | findbugs | 1m 15s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager generated 1 new + 8 unchanged - 0 fixed = 9 total (was 8) |
-1 | javadoc | 0m 28s | hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 880 unchanged - 0 fixed = 881 total (was 880) |
-1 | unit | 0m 38s | hadoop-yarn-api in the patch failed. |
-1 | unit | 40m 10s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 50s | The patch does not generate ASF License warnings. |
101m 31s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager |
Dead store to queueInfo in org.apache.hadoop.yarn.server.resourcemanager.webapp.RMWebServices.addQueue(QueueConfigInfo, Map, Map) At RMWebServices.java:org.apache.hadoop.yarn.server.resourcemanager.webapp.RMWebServices.addQueue(QueueConfigInfo, Map, Map) At RMWebServices.java:[line 2752] | |
Failed junit tests | hadoop.yarn.conf.TestYarnConfigurationFields |
hadoop.yarn.server.resourcemanager.security.TestDelegationTokenRenewer |
This message was automatically generated.
Thanks jhung,
This makes sense, should just be able to either grab the second-to-last component of the key, or the component before "accessible-node-labels"/"ordering-policy". I think we can also search for "root", if it is not there then assume it is a global config change.
I think we can handle it in this way:
There're a set of known queue paths like {"root.queueA", "root.queueA.A1}. For a given config key to change, first we need to remove the common prefix ("yarn.scheduler.capacity."), do the longest prefix match in the known queue paths.
- If we can find any non-empty common prefix, check the queue's accessibilities.
- If we cannot find, this is a global config, check admin permission.
This approach doesn't need to handle special options like "accessible-node-labels", and don't need to use "root" to index starting of queue path, to me it is not a safe approach.
As long as we access these queues via YarnScheduler#getQueueInfo, is this API still necessary? When the scheduler is reinitialized and the next mutation comes in, it will check against the queues from the most recent reinitialization.
We may have to call getQueueInfo for everytime when config mutation request comes in, the frequency of mutation request should not super high, I think the stateless approach should be fine.
This is not implemented yet, but I was thinking of handling this in RMWebServices, there are some cases that have not been handled (e.g. updating config for a queue which doesn't exist shouldn't be allowed, right now it "succeeds" silently). So we can address these cases in a separate jira.
Agree, it will add a never used option, it doesn't sound like a critical issue, we can handle it in a separate JIRA.
Yes you're right, this is not handled yet, in fact there is still some handling we need to do in RMWebServices for global configs, we can address this in a separate jira as well.
If non-trivial effort need to take for this, I'm OK to move it to a separate JIRA. This is quite important to me. In fact, I think we should not assume any scheduler-specific configurations inside RMWebServices (like add special logics to handle "yarn.scheduler.capacity.").
Thoughts?
Discussed this offline, will upload a patch addressing scheduler agnostic configuration mutation.
Also created YARN-6575 for supporting global scheduler configuration mutation.
Attached 003 patch which pushes the client request -> key value map transformation down from RMWebServices to MutableCSConfigurationProvider, to avoid weird parsing logic.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 15s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 11s | Maven dependency ordering for branch |
+1 | mvninstall | 16m 18s | |
+1 | compile | 12m 56s | |
+1 | checkstyle | 0m 51s | |
+1 | mvnsite | 1m 22s | |
+1 | mvneclipse | 0m 50s | |
-1 | findbugs | 1m 5s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager in |
+1 | javadoc | 0m 56s | |
0 | mvndep | 0m 10s | Maven dependency ordering for patch |
+1 | mvninstall | 0m 54s | the patch passed |
+1 | compile | 9m 12s | the patch passed |
+1 | javac | 9m 12s | the patch passed |
-0 | checkstyle | 0m 43s | hadoop-yarn-project/hadoop-yarn: The patch generated 8 new + 325 unchanged - 0 fixed = 333 total (was 325) |
+1 | mvnsite | 1m 4s | the patch passed |
+1 | mvneclipse | 0m 45s | the patch passed |
-1 | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
+1 | findbugs | 2m 21s | the patch passed |
-1 | javadoc | 0m 30s | hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager generated 1 new + 880 unchanged - 0 fixed = 881 total (was 880) |
-1 | unit | 0m 35s | hadoop-yarn-api in the patch failed. |
-1 | unit | 39m 49s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 26s | The patch does not generate ASF License warnings. |
102m 0s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.conf.TestYarnConfigurationFields |
hadoop.yarn.server.resourcemanager.TestRMRestart | |
hadoop.yarn.server.resourcemanager.scheduler.fair.TestFairSchedulerPreemption |
This message was automatically generated.
004 fixes checkstyle/javadoc/whitespace/TestYarnConfigurationFields unit test.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 30s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
0 | mvndep | 0m 14s | Maven dependency ordering for branch |
+1 | mvninstall | 18m 14s | |
+1 | compile | 14m 23s | |
+1 | checkstyle | 0m 57s | |
+1 | mvnsite | 1m 55s | |
+1 | mvneclipse | 1m 10s | |
-1 | findbugs | 1m 6s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common in |
-1 | findbugs | 1m 8s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager in |
+1 | javadoc | 1m 30s | |
0 | mvndep | 0m 9s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 27s | the patch passed |
+1 | compile | 11m 31s | the patch passed |
+1 | javac | 11m 31s | the patch passed |
+1 | checkstyle | 0m 55s | the patch passed |
+1 | mvnsite | 1m 51s | the patch passed |
+1 | mvneclipse | 1m 4s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | xml | 0m 1s | The patch has no ill-formed XML file. |
+1 | findbugs | 4m 13s | the patch passed |
+1 | javadoc | 1m 28s | the patch passed |
+1 | unit | 0m 32s | hadoop-yarn-api in the patch passed. |
+1 | unit | 2m 30s | hadoop-yarn-common in the patch passed. |
+1 | unit | 44m 26s | hadoop-yarn-server-resourcemanager in the patch passed. |
+1 | asflicense | 0m 34s | The patch does not generate ASF License warnings. |
121m 55s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:0ac17dc |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12867417/YARN-5949-YARN-5734.004.patch |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml |
uname | Linux 7eee1d2426af 3.13.0-108-generic #155-Ubuntu SMP Wed Jan 11 16:58:52 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | |
Default Java | 1.8.0_121 |
findbugs | v3.1.0-RC1 |
findbugs | https://builds.apache.org/job/PreCommit-YARN-Build/15897/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-common-warnings.html |
findbugs | https://builds.apache.org/job/PreCommit-YARN-Build/15897/artifact/patchprocess/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-warnings.html |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/15897/testReport/ |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager U: hadoop-yarn-project/hadoop-yarn |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/15897/console |
Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks jhung for updating the patch,
Few comments:
1) Do you think is it better to rename "...scheduler.configuration.mutation.policy.class" to "...scheduler.configuration.mutation.acl-policy.class"? "policy" is too general to me. If you think it is better, I suggest to rename all related classes/fields.
2) In yarn-default.xml, it's better to add a note to say "DefaultConfigurationMutationPolicy" is using admin-acl config?
3) Is there any test added with the patch? Is it possible to add basic tests?
Thanks leftnoteasy! Seems reasonable. Attached 005 to address these comments.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 14m 57s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 2 new or modified test files. |
0 | mvndep | 1m 8s | Maven dependency ordering for branch |
+1 | mvninstall | 15m 19s | |
+1 | compile | 16m 21s | |
+1 | checkstyle | 1m 24s | |
+1 | mvnsite | 2m 19s | |
+1 | mvneclipse | 1m 16s | |
-1 | findbugs | 1m 13s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common in |
-1 | findbugs | 1m 15s | hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager in |
+1 | javadoc | 1m 35s | |
0 | mvndep | 0m 11s | Maven dependency ordering for patch |
+1 | mvninstall | 1m 39s | the patch passed |
+1 | compile | 9m 40s | the patch passed |
+1 | javac | 9m 40s | the patch passed |
-0 | checkstyle | 0m 54s | hadoop-yarn-project/hadoop-yarn: The patch generated 3 new + 324 unchanged - 0 fixed = 327 total (was 324) |
+1 | mvnsite | 1m 52s | the patch passed |
+1 | mvneclipse | 1m 5s | the patch passed |
-1 | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
+1 | xml | 0m 2s | The patch has no ill-formed XML file. |
+1 | findbugs | 3m 40s | the patch passed |
+1 | javadoc | 1m 26s | the patch passed |
+1 | unit | 0m 33s | hadoop-yarn-api in the patch passed. |
+1 | unit | 2m 26s | hadoop-yarn-common in the patch passed. |
-1 | unit | 42m 16s | hadoop-yarn-server-resourcemanager in the patch failed. |
+1 | asflicense | 0m 34s | The patch does not generate ASF License warnings. |
133m 31s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.server.resourcemanager.TestRMRestart |
This message was automatically generated.
Latest patch LGTM, will commit tomorrow if no opposite opinions. Thanks jhung.
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #13057 (See https://builds.apache.org/job/Hadoop-trunk-Commit/13057/)
YARN-5949. Add pluggable configuration ACL policy interface and (jhung: rev a4e62530469e4c3d5b339a06adeac2146fc15fa5)
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java
- (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/QueueAdminConfigurationMutationACLPolicy.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/TestMutableCSConfigurationProvider.java
- (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/DefaultConfigurationMutationACLPolicy.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/MutableCSConfigurationProvider.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/MutableConfigurationProvider.java
- (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ConfigurationMutationACLPolicy.java
- (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/TestConfigurationMutationACLPolicies.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/MutableConfScheduler.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/webapp/RMWebServices.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml
- (add) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/ConfigurationMutationACLPolicyFactory.java
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacityScheduler.java
Added 001 patch which creates a policy interface, a default policy (checks if user is part of yarn.admin.acl), and a policy that checks if user is part of acl_administer_queue for all queues s/he is modifying.
Will add unit tests soon...
xgong, leftnoteasy do you mind taking a look? Thanks!