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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
