You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/01/27 13:27:38 UTC
[GitHub] [hadoop] 9uapaw commented on a change in pull request #3938: YARN-11069. Dynamic Queue ACL handling in Legacy and Flexible Auto Created Queues
9uapaw commented on a change in pull request #3938:
URL: https://github.com/apache/hadoop/pull/3938#discussion_r793513418
##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
##########
@@ -417,6 +420,23 @@ protected void setDynamicQueueProperties() {
.getConfiguredNodeLabelsForAllQueues()
.setLabelsByQueue(getQueuePath(), new HashSet<>(parentNodeLabels));
}
+
+ AutoCreatedQueueTemplate aqc = new AutoCreatedQueueTemplate(
Review comment:
Is it necessary to create a new AQC template object? Parent already has it.
##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractCSQueue.java
##########
@@ -417,6 +420,23 @@ protected void setDynamicQueueProperties() {
.getConfiguredNodeLabelsForAllQueues()
.setLabelsByQueue(getQueuePath(), new HashSet<>(parentNodeLabels));
}
+
+ AutoCreatedQueueTemplate aqc = new AutoCreatedQueueTemplate(
+ queueContext.getConfiguration(),
+ parent.getQueuePathObject());
+
+ if (this instanceof ParentQueue) {
+ acls.putAll(getACLsForFlexibleAutoCreatedParentQueue(aqc));
+ }
+
+ if (this instanceof AbstractLeafQueue) {
+ if (parent instanceof AbstractManagedParentQueue) {
+ acls.putAll(getACLsForLegacyAutoCreatedLeafQueue(
+ queueContext.getConfiguration(), parent.getQueuePath()));
+ } else {
+ acls.putAll(getACLsForFlexibleAutoCreatedLeafQueue(aqc));
+ }
+ }
Review comment:
These belong to their corresponding classes. eg. an abstract method setDynamicQueueACL, that is overridden both in ParentQueue and AbstractLeafQueue as well.
##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
##########
@@ -469,26 +481,61 @@ private RMAppImpl createAndPopulateNewRMApp(
submissionContext.getQueue() : placementContext.getFullQueuePath();
String appName = submissionContext.getApplicationName();
- CSQueue csqueue = ((CapacityScheduler) scheduler).getQueue(queueName);
+
+ CapacityScheduler cs = (CapacityScheduler) scheduler;
+ CapacitySchedulerConfiguration csConf = cs.getConfiguration();
+
+ CSQueue csqueue = cs.getQueue(queueName);
+ PrivilegedEntity privilegedEntity = getPrivilegedEntity(queueName);
if (csqueue == null && placementContext != null) {
- //could be an auto created queue through queue mapping. Validate
- // parent queue exists and has valid acls
- String parentQueueName = placementContext.getParentQueue();
- csqueue = ((CapacityScheduler) scheduler).getQueue(parentQueueName);
+ List<Permission> permissions = new ArrayList<>();
Review comment:
I think the whole CS ACL check logic could be extracted to somewhere else.
##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
##########
@@ -469,26 +481,61 @@ private RMAppImpl createAndPopulateNewRMApp(
submissionContext.getQueue() : placementContext.getFullQueuePath();
String appName = submissionContext.getApplicationName();
- CSQueue csqueue = ((CapacityScheduler) scheduler).getQueue(queueName);
+
+ CapacityScheduler cs = (CapacityScheduler) scheduler;
+ CapacitySchedulerConfiguration csConf = cs.getConfiguration();
+
+ CSQueue csqueue = cs.getQueue(queueName);
+ PrivilegedEntity privilegedEntity = getPrivilegedEntity(queueName);
if (csqueue == null && placementContext != null) {
- //could be an auto created queue through queue mapping. Validate
- // parent queue exists and has valid acls
- String parentQueueName = placementContext.getParentQueue();
- csqueue = ((CapacityScheduler) scheduler).getQueue(parentQueueName);
+ List<Permission> permissions = new ArrayList<>();
+
+ final QueuePath parentCandidate = new QueuePath(placementContext.getParentQueue());
+ AutoCreatedQueueTemplate aqc = new AutoCreatedQueueTemplate(csConf, parentCandidate);
+
+ CSQueue parentQueue = cs.getQueue(parentCandidate.getFullPath());
+ if (parentQueue == null) {
+ final QueuePath parent = new QueuePath(parentCandidate.getParent());
+ parentQueue = cs.getQueue(parent.getFullPath());
+ if (parentQueue == null) {
+ throw RPCUtil.getRemoteException(new AccessControlException(
+ "Access denied for invalid queue " + submissionContext.getQueue() +
+ " parent is not found " + parent.getFullPath() +
+ " user " + user + " applicationId " + applicationId));
+ }
+
Review comment:
I presume this logic is made because of multi level auto queue creation. In that case, it is not correct anymore, because YARN-10632 made the maximum queue depth configurable, therefore I suggest to use CSQueueManager#determineMissingParents to acquire queues to be dynamically created.
##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java
##########
@@ -941,6 +941,92 @@ private static String getAclKey(AccessType acl) {
return "acl_" + StringUtils.toLowerCase(acl.toString());
}
+ /**
+ * Creates a mapping of queue ACLs for a Legacy Auto Created Leaf Queue.
+ *
+ * @param csConf the CapacitySchedulerConfiguration
+ * @param parentQueuePath the parent's queue path
+ * @return A mapping of the queue ACLs.
+ */
+ public static Map<AccessType, AccessControlList> getACLsForLegacyAutoCreatedLeafQueue(
Review comment:
I think this should either be:
1. A non static method, because defining a static method in CSConfig and then receiving a CSConfig as an argument looks strange
2. Put this logic in an ACL specific class as helper methods
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org