tomicooler commented on code in PR #5871: URL: https://github.com/apache/hadoop/pull/5871#discussion_r1271263326
########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreationMixedMode.java: ########## @@ -0,0 +1,226 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity; + +import org.apache.hadoop.yarn.conf.YarnConfiguration; +import org.apache.hadoop.yarn.exceptions.YarnException; +import org.apache.hadoop.yarn.nodelabels.CommonNodeLabelsManager; +import org.apache.hadoop.yarn.server.resourcemanager.MockRM; +import org.apache.hadoop.yarn.server.resourcemanager.nodelabels.NullRMNodeLabelsManager; +import org.apache.hadoop.yarn.server.resourcemanager.nodelabels.RMNodeLabelsManager; +import org.apache.hadoop.yarn.server.resourcemanager.scheduler.ResourceScheduler; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; + +public class TestCapacitySchedulerNewQueueAutoCreationMixedMode + extends TestCapacitySchedulerAutoCreatedQueueBase { + public static final int GB = 1024; + private static final String NL = CommonNodeLabelsManager.NO_LABEL; + private MockRM mockRM = null; + private CapacityScheduler cs; + private CapacitySchedulerConfiguration csConf; + private CapacitySchedulerQueueManager autoQueueHandler; + + public CapacityScheduler getCs() { + return cs; + } + + /* + Create the following structure: + root + / | \ + a b e + / + a1 + */ + @Before + public void setUp() throws Exception { + csConf = new CapacitySchedulerConfiguration(); + csConf.setClass(YarnConfiguration.RM_SCHEDULER, CapacityScheduler.class, + ResourceScheduler.class); + csConf.setLegacyQueueModeEnabled(false); + } + + @After + public void tearDown() { + if (mockRM != null) { + mockRM.stop(); + } + } + + protected void startScheduler() throws Exception { + RMNodeLabelsManager mgr = new NullRMNodeLabelsManager(); + mgr.init(csConf); + mockRM = new MockRM(csConf) { + protected RMNodeLabelsManager createNodeLabelManager() { + return mgr; + } + }; + cs = (CapacityScheduler) mockRM.getResourceScheduler(); + cs.updatePlacementRules(); + mockRM.start(); + cs.start(); + autoQueueHandler = cs.getCapacitySchedulerQueueManager(); + mockRM.registerNode("h1:1234", 32000, 32); // label = x + } + + private void createMixedConfig() { + // root + // - a [memory=10%, vcores=1] + // - a1 [memory=100%, vcores=100%] + // - a2-auto [memory=2048, vcores=2] + // - b [memory=1024, vcores=10%] + // - c-auto [memory=1w, vcores=1w] + // - d [memory=20%, vcores=20%] + // - d1-auto [memory=10%, vcores=1] + // - e-auto [memory=1w, vcores=10] + // - e1-auto [memory=2048, vcores=2] + + // Set up static queues + csConf.setQueues(CapacitySchedulerConfiguration.ROOT, new String[]{"a", "b", "d"}); + csConf.setCapacityVector(A, NL, "[memory=10%, vcores=2]"); + csConf.setCapacityVector(B, NL, "[memory=2000, vcores=10%]"); + csConf.setCapacityVector(D, NL, "[memory=10%, vcores=10%]"); + csConf.setQueues(A, new String[]{"a1"}); + csConf.setCapacityVector(A1, NL, "[memory=100%, vcores=100%]"); + csConf.setAutoQueueCreationV2Enabled(CapacitySchedulerConfiguration.ROOT, true); + csConf.setAutoQueueCreationV2Enabled(A, true); + csConf.setAutoQueueCreationV2Enabled(D, true); + + // Set up dynamic queue templates + csConf.set(getTemplateKey(CapacitySchedulerConfiguration.ROOT, "capacity"), "[memory=2w, vcores=5w]"); + csConf.set(getParentTemplateKey(CapacitySchedulerConfiguration.ROOT, "capacity"), "[memory=2w, vcores=10]"); + csConf.set(getLeafTemplateKey(CapacitySchedulerConfiguration.ROOT + ".*", "capacity"), "[memory=2000, vcores=2]"); + csConf.set(getLeafTemplateKey(D, "capacity"), "[memory=1000, vcores=1]"); + + } + /* + Create and validate the following structure with mixed resource vectors and non-legacy queue mode: + + root + ┌─────┬────────┬─────┴─────┬─────────┐ + a b c-auto e-auto d + | | | + a1 e1-auto d1-auto + */ + @Test + public void testAutoCreateQueueRefresh() throws Exception { + createMixedConfig(); + startScheduler(); + + createQueue("root.c-auto"); + + // Check if queue c-auto got created + CSQueue cAuto = cs.getQueue("root.c-auto"); + // At this point queues a, b, d exists, ant c-autp was just created + // b takes 2000 MB from the cluster, a and d take up 10 + 10 = 20% (6000 MB, 6 vcore), so c-auto should get the rest + // (24000 MB, 24 vcore) because it's the only one with configured weights + Assert.assertEquals(24 / 32f, cAuto.getAbsoluteCapacity(), 1e-4); + Assert.assertEquals(-1f, cAuto.getQueueCapacities().getWeight(), 1e-6); Review Comment: an existing epsilon could be used here (or extract this to a variable) ########## hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerNewQueueAutoCreationMixedMode.java: ########## @@ -0,0 +1,226 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity; + +import org.apache.hadoop.yarn.conf.YarnConfiguration; +import org.apache.hadoop.yarn.exceptions.YarnException; +import org.apache.hadoop.yarn.nodelabels.CommonNodeLabelsManager; +import org.apache.hadoop.yarn.server.resourcemanager.MockRM; +import org.apache.hadoop.yarn.server.resourcemanager.nodelabels.NullRMNodeLabelsManager; +import org.apache.hadoop.yarn.server.resourcemanager.nodelabels.RMNodeLabelsManager; +import org.apache.hadoop.yarn.server.resourcemanager.scheduler.ResourceScheduler; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; + +public class TestCapacitySchedulerNewQueueAutoCreationMixedMode + extends TestCapacitySchedulerAutoCreatedQueueBase { + public static final int GB = 1024; + private static final String NL = CommonNodeLabelsManager.NO_LABEL; + private MockRM mockRM = null; + private CapacityScheduler cs; + private CapacitySchedulerConfiguration csConf; + private CapacitySchedulerQueueManager autoQueueHandler; + + public CapacityScheduler getCs() { + return cs; + } + + /* + Create the following structure: + root + / | \ + a b e + / + a1 + */ + @Before + public void setUp() throws Exception { + csConf = new CapacitySchedulerConfiguration(); + csConf.setClass(YarnConfiguration.RM_SCHEDULER, CapacityScheduler.class, + ResourceScheduler.class); + csConf.setLegacyQueueModeEnabled(false); + } + + @After + public void tearDown() { + if (mockRM != null) { + mockRM.stop(); + } + } + + protected void startScheduler() throws Exception { + RMNodeLabelsManager mgr = new NullRMNodeLabelsManager(); + mgr.init(csConf); + mockRM = new MockRM(csConf) { + protected RMNodeLabelsManager createNodeLabelManager() { + return mgr; + } + }; + cs = (CapacityScheduler) mockRM.getResourceScheduler(); + cs.updatePlacementRules(); + mockRM.start(); + cs.start(); + autoQueueHandler = cs.getCapacitySchedulerQueueManager(); + mockRM.registerNode("h1:1234", 32000, 32); // label = x + } + + private void createMixedConfig() { + // root + // - a [memory=10%, vcores=1] + // - a1 [memory=100%, vcores=100%] + // - a2-auto [memory=2048, vcores=2] + // - b [memory=1024, vcores=10%] + // - c-auto [memory=1w, vcores=1w] + // - d [memory=20%, vcores=20%] + // - d1-auto [memory=10%, vcores=1] + // - e-auto [memory=1w, vcores=10] + // - e1-auto [memory=2048, vcores=2] + + // Set up static queues + csConf.setQueues(CapacitySchedulerConfiguration.ROOT, new String[]{"a", "b", "d"}); + csConf.setCapacityVector(A, NL, "[memory=10%, vcores=2]"); + csConf.setCapacityVector(B, NL, "[memory=2000, vcores=10%]"); + csConf.setCapacityVector(D, NL, "[memory=10%, vcores=10%]"); + csConf.setQueues(A, new String[]{"a1"}); + csConf.setCapacityVector(A1, NL, "[memory=100%, vcores=100%]"); + csConf.setAutoQueueCreationV2Enabled(CapacitySchedulerConfiguration.ROOT, true); + csConf.setAutoQueueCreationV2Enabled(A, true); + csConf.setAutoQueueCreationV2Enabled(D, true); + + // Set up dynamic queue templates + csConf.set(getTemplateKey(CapacitySchedulerConfiguration.ROOT, "capacity"), "[memory=2w, vcores=5w]"); + csConf.set(getParentTemplateKey(CapacitySchedulerConfiguration.ROOT, "capacity"), "[memory=2w, vcores=10]"); + csConf.set(getLeafTemplateKey(CapacitySchedulerConfiguration.ROOT + ".*", "capacity"), "[memory=2000, vcores=2]"); + csConf.set(getLeafTemplateKey(D, "capacity"), "[memory=1000, vcores=1]"); + + } + /* + Create and validate the following structure with mixed resource vectors and non-legacy queue mode: + + root + ┌─────┬────────┬─────┴─────┬─────────┐ + a b c-auto e-auto d + | | | + a1 e1-auto d1-auto + */ + @Test + public void testAutoCreateQueueRefresh() throws Exception { + createMixedConfig(); + startScheduler(); + + createQueue("root.c-auto"); + + // Check if queue c-auto got created + CSQueue cAuto = cs.getQueue("root.c-auto"); + // At this point queues a, b, d exists, ant c-autp was just created Review Comment: typo: c-auto ########## 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: ########## @@ -448,7 +448,7 @@ protected void setDynamicQueueProperties() { // Set properties from parent template if (parent instanceof AbstractParentQueue && isDynamicQueue()) { ((AbstractParentQueue) parent).getAutoCreatedQueueTemplate() - .setTemplateEntriesForChild(queueContext.getConfiguration(), getQueuePath()); + .setTemplateEntriesForChild(queueContext.getConfiguration(), getQueuePath(), this instanceof AbstractLeafQueue); Review Comment: NIT: after a rebase he first commit won't be needed here. ########## 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: ########## @@ -463,12 +463,14 @@ protected void setDynamicQueueProperties() { queueContext.getQueueManager().getConfiguredNodeLabelsForAllQueues() .setLabelsByQueue(getQueuePath(), new HashSet<>(parentNodeLabels)); } + Map<String, QueueCapacityVector> templateConfiguredResourceVector = queueContext.getConfiguration() + .parseConfiguredResourceVector(queuePath.getFullPath(), parentNodeLabels); + Review Comment: I'm missing the part where the configuredCapacityVector (and configuredMaxCapacityVector) is updated with this information. Are those already have the correctly parsed capacity vectors at the setupQueueConfigs phrase, if so there is no need for parsing them again. Anyway the field member configurecCapacityVector should have the correct value set somewhere. Some e2e test with JSON asserts should verify it later. Maybe this new UT could also assert it. -- 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]
