YARN-3762. FairScheduler: CME on FSParentQueue#getQueueUserAclInfo. (kasha)

(cherry picked from commit edb9cd0f7a)

hadoop-yarn-project/CHANGES.txt

@@ -434,6 +434,8 @@ Release 2.8.0 - UNRELEASED
     YARN-3751. Fixed AppInfo to check if used resources are null. (Sunil G via
     zjshen)
 
+    YARN-3762. FairScheduler: CME on FSParentQueue#getQueueUserAclInfo. (kasha)
+
 Release 2.7.1 - UNRELEASED
 
   INCOMPATIBLE CHANGES

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/FSParentQueue.java

@@ -23,6 +23,9 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -44,59 +47,103 @@ public class FSParentQueue extends FSQueue {
   private static final Log LOG = LogFactory.getLog(
       FSParentQueue.class.getName());
 
-  private final List<FSQueue> childQueues = 
+  private final List<FSQueue> childQueues = new ArrayList<>();
-      new ArrayList<FSQueue>();
   private Resource demand = Resources.createResource(0);
   private int runnableApps;
 
+  private ReadWriteLock rwLock = new ReentrantReadWriteLock();
+  private Lock readLock = rwLock.readLock();
+  private Lock writeLock = rwLock.writeLock();
+
   public FSParentQueue(String name, FairScheduler scheduler,
       FSParentQueue parent) {
     super(name, scheduler, parent);
   }
   
   public void addChildQueue(FSQueue child) {
+    writeLock.lock();
+    try {
       childQueues.add(child);
+    } finally {
+      writeLock.unlock();
+    }
+  }
+
+  public void removeChildQueue(FSQueue child) {
+    writeLock.lock();
+    try {
+      childQueues.remove(child);
+    } finally {
+      writeLock.unlock();
+    }
   }
 
   @Override
   public void recomputeShares() {
+    readLock.lock();
+    try {
       policy.computeShares(childQueues, getFairShare());
       for (FSQueue childQueue : childQueues) {
         childQueue.getMetrics().setFairShare(childQueue.getFairShare());
         childQueue.recomputeShares();
       }
+    } finally {
+      readLock.unlock();
+    }
   }
 
   public void recomputeSteadyShares() {
+    readLock.lock();
+    try {
       policy.computeSteadyShares(childQueues, getSteadyFairShare());
       for (FSQueue childQueue : childQueues) {
-      childQueue.getMetrics().setSteadyFairShare(childQueue.getSteadyFairShare());
+        childQueue.getMetrics()
+            .setSteadyFairShare(childQueue.getSteadyFairShare());
         if (childQueue instanceof FSParentQueue) {
           ((FSParentQueue) childQueue).recomputeSteadyShares();
         }
       }
+    } finally {
+      readLock.unlock();
+    }
   }
 
   @Override
   public void updatePreemptionVariables() {
     super.updatePreemptionVariables();
     // For child queues
+
+    readLock.lock();
+    try {
       for (FSQueue childQueue : childQueues) {
         childQueue.updatePreemptionVariables();
       }
+    } finally {
+      readLock.unlock();
+    }
   }
 
   @Override
   public Resource getDemand() {
-    return demand;
+    readLock.lock();
+    try {
+      return Resource.newInstance(demand.getMemory(), demand.getVirtualCores());
+    } finally {
+      readLock.unlock();
+    }
   }
 
   @Override
   public Resource getResourceUsage() {
     Resource usage = Resources.createResource(0);
+    readLock.lock();
+    try {
       for (FSQueue child : childQueues) {
         Resources.addTo(usage, child.getResourceUsage());
       }
+    } finally {
+      readLock.unlock();
+    }
     return usage;
   }
 
@@ -106,6 +153,8 @@ public class FSParentQueue extends FSQueue {
     // Limit demand to maxResources
     Resource maxRes = scheduler.getAllocationConfiguration()
         .getMaxResources(getName());
+    writeLock.lock();
+    try {
       demand = Resources.createResource(0);
       for (FSQueue childQueue : childQueues) {
         childQueue.updateDemand();
@@ -121,40 +170,41 @@ public class FSParentQueue extends FSQueue {
           break;
         }
       }
+    } finally {
+      writeLock.unlock();
+    }
     if (LOG.isDebugEnabled()) {
       LOG.debug("The updated demand for " + getName() + " is " + demand +
           "; the max is " + maxRes);
     }    
   }
   
-  private synchronized QueueUserACLInfo getUserAclInfo(
+  private QueueUserACLInfo getUserAclInfo(UserGroupInformation user) {
-      UserGroupInformation user) {
+    List<QueueACL> operations = new ArrayList<>();
-    QueueUserACLInfo userAclInfo = 
-      recordFactory.newRecordInstance(QueueUserACLInfo.class);
-    List<QueueACL> operations = new ArrayList<QueueACL>();
     for (QueueACL operation : QueueACL.values()) {
       if (hasAccess(operation, user)) {
         operations.add(operation);
       } 
     }
-
+    return QueueUserACLInfo.newInstance(getQueueName(), operations);
-    userAclInfo.setQueueName(getQueueName());
-    userAclInfo.setUserAcls(operations);
-    return userAclInfo;
   }
   
   @Override
-  public synchronized List<QueueUserACLInfo> getQueueUserAclInfo(
+  public List<QueueUserACLInfo> getQueueUserAclInfo(UserGroupInformation user) {
-      UserGroupInformation user) {
     List<QueueUserACLInfo> userAcls = new ArrayList<QueueUserACLInfo>();
     
     // Add queue acls
     userAcls.add(getUserAclInfo(user));
     
     // Add children queue acls
+    readLock.lock();
+    try {
       for (FSQueue child : childQueues) {
         userAcls.addAll(child.getQueueUserAclInfo(user));
       }
+    } finally {
+      readLock.unlock();
+    }
  
     return userAcls;
   }
@@ -168,13 +218,33 @@ public class FSParentQueue extends FSQueue {
       return assigned;
     }
 
+    // Hold the write lock when sorting childQueues
+    writeLock.lock();
+    try {
       Collections.sort(childQueues, policy.getComparator());
+    } finally {
+      writeLock.unlock();
+    }
+
+    /*
+     * We are releasing the lock between the sort and iteration of the
+     * "sorted" list. There could be changes to the list here:
+     * 1. Add a child queue to the end of the list, this doesn't affect
+     * container assignment.
+     * 2. Remove a child queue, this is probably good to take care of so we
+     * don't assign to a queue that is going to be removed shortly.
+     */
+    readLock.lock();
+    try {
       for (FSQueue child : childQueues) {
         assigned = child.assignContainer(node);
         if (!Resources.equals(assigned, Resources.none())) {
           break;
         }
       }
+    } finally {
+      readLock.unlock();
+    }
     return assigned;
   }
 
@@ -185,12 +255,18 @@ public class FSParentQueue extends FSQueue {
     // Find the childQueue which is most over fair share
     FSQueue candidateQueue = null;
     Comparator<Schedulable> comparator = policy.getComparator();
+
+    readLock.lock();
+    try {
       for (FSQueue queue : childQueues) {
         if (candidateQueue == null ||
             comparator.compare(queue, candidateQueue) > 0) {
           candidateQueue = queue;
         }
       }
+    } finally {
+      readLock.unlock();
+    }
 
     // Let the selected queue choose which of its container to preempt
     if (candidateQueue != null) {
@@ -201,7 +277,12 @@ public class FSParentQueue extends FSQueue {
 
   @Override
   public List<FSQueue> getChildQueues() {
-    return childQueues;
+    readLock.lock();
+    try {
+      return Collections.unmodifiableList(childQueues);
+    } finally {
+      readLock.unlock();
+    }
   }
 
   @Override
@@ -218,24 +299,44 @@ public class FSParentQueue extends FSQueue {
   }
   
   public void incrementRunnableApps() {
+    writeLock.lock();
+    try {
       runnableApps++;
+    } finally {
+      writeLock.unlock();
+    }
   }
   
   public void decrementRunnableApps() {
+    writeLock.lock();
+    try {
       runnableApps--;
+    } finally {
+      writeLock.unlock();
+    }
   }
 
   @Override
   public int getNumRunnableApps() {
+    readLock.lock();
+    try {
       return runnableApps;
+    } finally {
+      readLock.unlock();
+    }
   }
 
   @Override
   public void collectSchedulerApplications(
       Collection<ApplicationAttemptId> apps) {
+    readLock.lock();
+    try {
       for (FSQueue childQueue : childQueues) {
         childQueue.collectSchedulerApplications(apps);
       }
+    } finally {
+      readLock.unlock();
+    }
   }
   
   @Override

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/fair/QueueManager.java

@@ -304,7 +304,8 @@ public class QueueManager {
       }
     }
     queues.remove(queue.getName());
-    queue.getParent().getChildQueues().remove(queue);
+    FSParentQueue parent = queue.getParent();
+    parent.removeChildQueue(queue);
   }
   
   /**