From 6f5f5330107e3d96abb00d66b99773b8bceadf28 Mon Sep 17 00:00:00 2001 From: bgoerlitz <2602635+bgoerlitz@users.noreply.github.com> Date: Fri, 29 Sep 2023 12:56:45 -0500 Subject: [PATCH 1/4] YARN-11584. Safely fail on leaf queue with empty name Prevents auto-creation of a leaf queue with an empty string as the shortname in CapacityScheduler, which caused FATAL exception in RM. --- .../capacity/AbstractParentQueue.java | 6 +++ .../CapacitySchedulerQueueManager.java | 6 +++ ...estCapacitySchedulerAutoQueueCreation.java | 38 +++++++++++++++++++ ...CapacitySchedulerNewQueueAutoCreation.java | 7 ++++ 4 files changed, 57 insertions(+) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java index 50516dd2bc5fa..54f373113ba28 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java @@ -490,6 +490,12 @@ public CSQueue createNewQueue(String childQueuePath, boolean isLeaf) String queueShortName = childQueuePath.substring( childQueuePath.lastIndexOf(".") + 1); + if (StringUtils.isEmpty(queueShortName)) { + throw new SchedulerDynamicEditException( + "Trying to create new queue=" + childQueuePath + + ", which has empty leaf shortname."); + } + if (isLeaf) { childQueue = new LeafQueue(queueContext, queueShortName, this, null, true); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java index 9e4dc55708d55..352c1817f2c25 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java @@ -509,6 +509,12 @@ public AbstractLeafQueue createQueue(QueuePath queue) String leafQueueName = queue.getLeafName(); String parentQueueName = queue.getParent(); + if (StringUtils.isEmpty(leafQueueName)) { + throw new SchedulerDynamicEditException( + "Trying to create new queue=" + queue.getFullPath() + + ", which has empty leaf shortname."); + } + if (!StringUtils.isEmpty(parentQueueName)) { CSQueue parentQueue = getQueue(parentQueueName); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java index ca5069d22133f..b835c1047440c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java @@ -109,6 +109,8 @@ public class TestCapacitySchedulerAutoQueueCreation private static final Logger LOG = LoggerFactory.getLogger( TestCapacitySchedulerAutoQueueCreation.class); + private static final String SPECIFIED_QUEUE_MAPPING = "%specified"; + private static final String CURRENT_USER_MAPPING = "%user"; private static final Resource TEMPLATE_MAX_RES = Resource.newInstance(16 * @@ -1054,4 +1056,40 @@ public RMNodeLabelsManager createNodeLabelManager() { } } } + + @Test(timeout = 10000) + public void testAutoCreateLeafQueueFailsWithSpecifiedEmptyStringLeafQueue() + throws Exception { + + final String INVALID_QUEUE = ""; + + MockRM newMockRM = setupSchedulerInstance(); + CapacityScheduler newCS = + (CapacityScheduler) newMockRM.getResourceScheduler(); + + //queue mapping to place app in queue specified by user + setupQueueMapping(newCS, "app_user", "root.c", SPECIFIED_QUEUE_MAPPING); + newCS.updatePlacementRules(); + + try { + //submitting to root.c. should fail WITHOUT crashing the RM + submitApp(newCS, "app_user", INVALID_QUEUE, "root.c"); + + RMContext rmContext = mock(RMContext.class); + when(rmContext.getDispatcher()).thenReturn(dispatcher); + newCS.setRMContext(rmContext); + + ApplicationId appId = BuilderUtils.newApplicationId(1, 1); + SchedulerEvent addAppEvent = new AppAddedSchedulerEvent( + appId, "root.c." + INVALID_QUEUE, "app_user"); + newCS.handle(addAppEvent); + + RMAppEvent event = new RMAppEvent(appId, RMAppEventType.APP_REJECTED, + "error"); + dispatcher.spyOnNextEvent(event, 10000); + } finally { + ((CapacityScheduler) newMockRM.getResourceScheduler()).stop(); + newMockRM.stop(); + } + } } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java index d2891379c47e0..c1673b0232f0a 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java @@ -1277,6 +1277,13 @@ public void testAutoCreateInvalidParent() throws Exception { () -> createQueue("invalidQueue")); } + @Test(expected = SchedulerDynamicEditException.class) + public void testAutoCreateQueueShouldFailWhenLeafEmptyString() + throws Exception { + startScheduler(); + createQueue("root.a."); + } + protected AbstractLeafQueue createQueue(String queuePath) throws YarnException, IOException { return autoQueueHandler.createQueue(new QueuePath(queuePath)); From 335d167781efaae88e6d3ecfad928e67fc3eb85e Mon Sep 17 00:00:00 2001 From: bgoerlitz <2602635+bgoerlitz@users.noreply.github.com> Date: Thu, 5 Oct 2023 15:25:41 -0500 Subject: [PATCH 2/4] Checkstyle fix --- .../capacity/TestCapacitySchedulerAutoQueueCreation.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java index b835c1047440c..55be35c0aa41d 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java @@ -1061,7 +1061,7 @@ public RMNodeLabelsManager createNodeLabelManager() { public void testAutoCreateLeafQueueFailsWithSpecifiedEmptyStringLeafQueue() throws Exception { - final String INVALID_QUEUE = ""; + final String INVALIDQUEUE = ""; MockRM newMockRM = setupSchedulerInstance(); CapacityScheduler newCS = @@ -1073,7 +1073,7 @@ public void testAutoCreateLeafQueueFailsWithSpecifiedEmptyStringLeafQueue() try { //submitting to root.c. should fail WITHOUT crashing the RM - submitApp(newCS, "app_user", INVALID_QUEUE, "root.c"); + submitApp(newCS, "app_user", INVALIDQUEUE, "root.c"); RMContext rmContext = mock(RMContext.class); when(rmContext.getDispatcher()).thenReturn(dispatcher); @@ -1081,7 +1081,7 @@ public void testAutoCreateLeafQueueFailsWithSpecifiedEmptyStringLeafQueue() ApplicationId appId = BuilderUtils.newApplicationId(1, 1); SchedulerEvent addAppEvent = new AppAddedSchedulerEvent( - appId, "root.c." + INVALID_QUEUE, "app_user"); + appId, "root.c." + INVALIDQUEUE, "app_user"); newCS.handle(addAppEvent); RMAppEvent event = new RMAppEvent(appId, RMAppEventType.APP_REJECTED, From 68685e2e9d592a118339ad82d173747953e8e4da Mon Sep 17 00:00:00 2001 From: bgoerlitz <2602635+bgoerlitz@users.noreply.github.com> Date: Thu, 5 Oct 2023 20:36:38 -0500 Subject: [PATCH 3/4] Fixed checkstyle warning in new test --- .../capacity/TestCapacitySchedulerAutoQueueCreation.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java index 55be35c0aa41d..590b0a4b7668d 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java @@ -1061,7 +1061,7 @@ public RMNodeLabelsManager createNodeLabelManager() { public void testAutoCreateLeafQueueFailsWithSpecifiedEmptyStringLeafQueue() throws Exception { - final String INVALIDQUEUE = ""; + final String invalidQueue = ""; MockRM newMockRM = setupSchedulerInstance(); CapacityScheduler newCS = @@ -1073,7 +1073,7 @@ public void testAutoCreateLeafQueueFailsWithSpecifiedEmptyStringLeafQueue() try { //submitting to root.c. should fail WITHOUT crashing the RM - submitApp(newCS, "app_user", INVALIDQUEUE, "root.c"); + submitApp(newCS, "app_user", invalidQueue, "root.c"); RMContext rmContext = mock(RMContext.class); when(rmContext.getDispatcher()).thenReturn(dispatcher); @@ -1081,7 +1081,7 @@ public void testAutoCreateLeafQueueFailsWithSpecifiedEmptyStringLeafQueue() ApplicationId appId = BuilderUtils.newApplicationId(1, 1); SchedulerEvent addAppEvent = new AppAddedSchedulerEvent( - appId, "root.c." + INVALIDQUEUE, "app_user"); + appId, "root.c." + invalidQueue, "app_user"); newCS.handle(addAppEvent); RMAppEvent event = new RMAppEvent(appId, RMAppEventType.APP_REJECTED, From 5d7604ba55c168481eb0226d0273c685bf694fc7 Mon Sep 17 00:00:00 2001 From: bgoerlitz <2602635+bgoerlitz@users.noreply.github.com> Date: Thu, 19 Oct 2023 15:35:51 -0500 Subject: [PATCH 4/4] Revised approach to ensure that QueuePath.hasEmptyPart() returns true for empty leaf shortname As QueuePath.hasEmptyPart() is already called in previously patched code paths, the explicit checks for empty leaf shortname are removed. Additionally, the Unit Test for CapacityScheudulerQueueManager.createQueue(QueuePath) is unnecessary, so was removed. --- .../scheduler/capacity/AbstractParentQueue.java | 6 ------ .../scheduler/capacity/CapacitySchedulerQueueManager.java | 6 ------ .../resourcemanager/scheduler/capacity/QueuePath.java | 5 +++++ .../TestCapacitySchedulerNewQueueAutoCreation.java | 7 ------- .../resourcemanager/scheduler/capacity/TestQueuePath.java | 2 ++ 5 files changed, 7 insertions(+), 19 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java index 54f373113ba28..50516dd2bc5fa 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java @@ -490,12 +490,6 @@ public CSQueue createNewQueue(String childQueuePath, boolean isLeaf) String queueShortName = childQueuePath.substring( childQueuePath.lastIndexOf(".") + 1); - if (StringUtils.isEmpty(queueShortName)) { - throw new SchedulerDynamicEditException( - "Trying to create new queue=" + childQueuePath - + ", which has empty leaf shortname."); - } - if (isLeaf) { childQueue = new LeafQueue(queueContext, queueShortName, this, null, true); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java index 352c1817f2c25..9e4dc55708d55 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java @@ -509,12 +509,6 @@ public AbstractLeafQueue createQueue(QueuePath queue) String leafQueueName = queue.getLeafName(); String parentQueueName = queue.getParent(); - if (StringUtils.isEmpty(leafQueueName)) { - throw new SchedulerDynamicEditException( - "Trying to create new queue=" + queue.getFullPath() - + ", which has empty leaf shortname."); - } - if (!StringUtils.isEmpty(parentQueueName)) { CSQueue parentQueue = getQueue(parentQueueName); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueuePath.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueuePath.java index 440742b908927..692be1132eac6 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueuePath.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/QueuePath.java @@ -96,6 +96,11 @@ private void setFromFullPath(String fullPath) { * @return true if there is at least one empty part of the path */ public boolean hasEmptyPart() { + // iterator will not contain an empty leaf queue, so check directly + if (leaf.isEmpty()) { + return true; + } + for (String part : this) { if (part.isEmpty()) { return true; diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java index c1673b0232f0a..d2891379c47e0 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreation.java @@ -1277,13 +1277,6 @@ public void testAutoCreateInvalidParent() throws Exception { () -> createQueue("invalidQueue")); } - @Test(expected = SchedulerDynamicEditException.class) - public void testAutoCreateQueueShouldFailWhenLeafEmptyString() - throws Exception { - startScheduler(); - createQueue("root.a."); - } - protected AbstractLeafQueue createQueue(String queuePath) throws YarnException, IOException { return autoQueueHandler.createQueue(new QueuePath(queuePath)); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueuePath.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueuePath.java index 7eb577d9c1271..91171966c119c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueuePath.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestQueuePath.java @@ -48,9 +48,11 @@ public void testCreation() { @Test public void testEmptyPart() { QueuePath queuePathWithEmptyPart = new QueuePath("root..level_2"); + QueuePath queuePathWithEmptyLeaf = new QueuePath("root.level_1."); QueuePath queuePathWithoutEmptyPart = new QueuePath(TEST_QUEUE); Assert.assertTrue(queuePathWithEmptyPart.hasEmptyPart()); + Assert.assertTrue(queuePathWithEmptyLeaf.hasEmptyPart()); Assert.assertFalse(queuePathWithoutEmptyPart.hasEmptyPart()); }