From 54bb5e7ba718cffc57c4cc0299448456486797fc Mon Sep 17 00:00:00 2001 From: Szilard Nemeth Date: Mon, 20 Jun 2022 17:42:49 +0200 Subject: [PATCH 1/5] Add new testcases to repro the issue --- .../TestCSMappingPlacementRule.java | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java index 3e614bcbc967a..626f6c87350a7 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java @@ -18,6 +18,9 @@ package org.apache.hadoop.yarn.server.resourcemanager.placement.csmappingrule; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.commons.logging.impl.Log4JLogger; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.security.GroupMappingServiceProvider; import org.apache.hadoop.thirdparty.com.google.common.collect.ImmutableMap; @@ -33,6 +36,7 @@ import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration; import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerQueueManager; import org.apache.hadoop.yarn.util.Records; +import org.apache.log4j.Level; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -857,6 +861,59 @@ public List getMappingRules() { assertPlace(engine, app, user, "root.man.testGroup0"); } + @Test + public void testUserNameSetDefaultAndPlaceWith2Rules() throws IOException { + Log log = LogFactory.getLog("org.apache.hadoop.yarn.server.resourcemanager.placement"); + if (log instanceof Log4JLogger) { + org.apache.log4j.Logger logger = ((Log4JLogger) log).getLogger(); + logger.setLevel(Level.DEBUG); + } + + List rules = new ArrayList<>(); + rules.add( + new MappingRule( + MappingRuleMatchers.createUserMatcher("test.user"), + (MappingRuleActions.createUpdateDefaultAction("root.user")) + .setFallbackSkip())); + rules.add(new MappingRule( + MappingRuleMatchers.createUserMatcher("test.user"), + (MappingRuleActions.createPlaceToDefaultAction()) + .setFallbackReject())); + + CSMappingPlacementRule engine = setupEngine(true, rules); + ApplicationSubmissionContext app = createApp("app"); + assertPlace( + "test.user should be placed to root.user", + engine, app, "test.user", "root.user"); + } + + @Test + public void testUserNameSetDefaultAndPlaceWith2RulesUsernameReplacedWithDot() throws IOException { + Log log = LogFactory.getLog("org.apache.hadoop.yarn.server.resourcemanager.placement"); + if (log instanceof Log4JLogger) { + org.apache.log4j.Logger logger = ((Log4JLogger) log).getLogger(); + logger.setLevel(Level.DEBUG); + } + + + ArrayList rules = new ArrayList<>(); + rules.add( + new MappingRule( + MappingRuleMatchers.createUserMatcher("test_dot_user"), + (MappingRuleActions.createUpdateDefaultAction("root.user.bob")) + .setFallbackSkip())); + rules.add(new MappingRule( + MappingRuleMatchers.createUserMatcher("test_dot_user"), + (MappingRuleActions.createPlaceToDefaultAction()) + .setFallbackReject())); + + CSMappingPlacementRule engine = setupEngine(true, rules); + ApplicationSubmissionContext app = createApp("app"); + assertPlace( + "test.user should be placed to root.user.bob", + engine, app, "test.user", "root.user.bob"); + } + private CSMappingPlacementRule initPlacementEngine(CapacityScheduler cs) throws IOException { CSMappingPlacementRule engine = new CSMappingPlacementRule(); engine.setFailOnConfigError(true); From b3e3eb2a1cdcd18fa40e1621e501145b7079f89f Mon Sep 17 00:00:00 2001 From: Szilard Nemeth Date: Sun, 8 Jan 2023 13:01:45 +0100 Subject: [PATCH 2/5] Fix bug --- .../placement/CSMappingPlacementRule.java | 8 ++- .../placement/VariableContext.java | 10 ++++ .../csmappingrule/MappingRuleMatchers.java | 6 ++ .../TestCSMappingPlacementRule.java | 56 ++++++++++--------- 4 files changed, 51 insertions(+), 29 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/placement/CSMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java index cefed1dd9fd85..e0fab39b053b8 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/CSMappingPlacementRule.java @@ -228,7 +228,11 @@ private VariableContext createVariableContext( ApplicationSubmissionContext asc, String user) { VariableContext vctx = new VariableContext(); - vctx.put("%user", cleanName(user)); + String cleanedName = cleanName(user); + if (!user.equals(cleanedName)) { + vctx.putOriginal("%user", user); + } + vctx.put("%user", cleanedName); //If the specified matches the default it means NO queue have been specified //as per ClientRMService#submitApplication which sets the queue to default //when no queue is provided. @@ -239,7 +243,7 @@ private VariableContext createVariableContext( //Adding specified as empty will prevent it to be undefined and it won't //try to place the application to a queue named '%specified', queue path //validation will reject the empty path or the path with empty parts, - //so we sill still hit the fallback action of this rule if no queue + //so we still hit the fallback action of this rule if no queue //is specified vctx.put("%specified", ""); } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java index d60e7b5630ae1..d47678a6da2f9 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java @@ -39,6 +39,7 @@ public class VariableContext { * This is our actual variable store. */ private Map variables = new HashMap<>(); + private Map originalVariables = new HashMap<>(); /** * This is our conditional variable store. @@ -124,6 +125,10 @@ public VariableContext put(String name, String value) { return this; } + public void putOriginal(String name, String value) { + originalVariables.put(name, value); + } + /** * This method is used to add a conditional variable to the variable context. * @param name Name of the variable @@ -149,6 +154,11 @@ public String get(String name) { String ret = variables.get(name); return ret == null ? "" : ret; } + + public String getOriginal(String name) { + String ret = originalVariables.get(name); + return ret; + } /** * Adds a set to the context, each name can only be added once. The extra diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java index 9d56e89121c4d..ae3ca88b5d6e8 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java @@ -87,6 +87,12 @@ public boolean match(VariableContext variables) { } String substituted = variables.replaceVariables(value); + + String originalVariableValue = variables.getOriginal(variable); + if (originalVariableValue != null) { + return substituted.equals(originalVariableValue); + } + return substituted.equals(variables.get(variable)); } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java index 626f6c87350a7..291129ec7ab6f 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java @@ -37,6 +37,7 @@ import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerQueueManager; import org.apache.hadoop.yarn.util.Records; import org.apache.log4j.Level; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -69,6 +70,11 @@ public class TestCSMappingPlacementRule { @Rule public TemporaryFolder folder = new TemporaryFolder(); + + @Before + public void setUp() { + setLoggerToDebug(); + } private Map> userGroups = ImmutableMap.>builder() @@ -89,6 +95,7 @@ private void createQueueHierarchy(CapacitySchedulerQueueManager queueManager) { .withQueue("root.user.alice") .withQueue("root.user.bob") .withQueue("root.user.test_dot_user") + .withQueue("root.user.testuser") .withQueue("root.groups.main_dot_grp") .withQueue("root.groups.sec_dot_test_dot_grp") .withQueue("root.secondaryTests.unique") @@ -862,18 +869,12 @@ public List getMappingRules() { } @Test - public void testUserNameSetDefaultAndPlaceWith2Rules() throws IOException { - Log log = LogFactory.getLog("org.apache.hadoop.yarn.server.resourcemanager.placement"); - if (log instanceof Log4JLogger) { - org.apache.log4j.Logger logger = ((Log4JLogger) log).getLogger(); - logger.setLevel(Level.DEBUG); - } - + public void testOriginalUserNameWithDotCanBeUsedInMatchExpression() throws IOException { List rules = new ArrayList<>(); rules.add( new MappingRule( MappingRuleMatchers.createUserMatcher("test.user"), - (MappingRuleActions.createUpdateDefaultAction("root.user")) + (MappingRuleActions.createUpdateDefaultAction("root.user.testuser")) .setFallbackSkip())); rules.add(new MappingRule( MappingRuleMatchers.createUserMatcher("test.user"), @@ -884,34 +885,35 @@ public void testUserNameSetDefaultAndPlaceWith2Rules() throws IOException { ApplicationSubmissionContext app = createApp("app"); assertPlace( "test.user should be placed to root.user", - engine, app, "test.user", "root.user"); + engine, app, "test.user", "root.user.testuser"); } @Test - public void testUserNameSetDefaultAndPlaceWith2RulesUsernameReplacedWithDot() throws IOException { - Log log = LogFactory.getLog("org.apache.hadoop.yarn.server.resourcemanager.placement"); - if (log instanceof Log4JLogger) { - org.apache.log4j.Logger logger = ((Log4JLogger) log).getLogger(); - logger.setLevel(Level.DEBUG); - } - - - ArrayList rules = new ArrayList<>(); + public void testOriginalGroupNameWithDotCanBeUsedInMatchExpression() throws IOException { + List rules = new ArrayList<>(); rules.add( - new MappingRule( - MappingRuleMatchers.createUserMatcher("test_dot_user"), - (MappingRuleActions.createUpdateDefaultAction("root.user.bob")) - .setFallbackSkip())); + new MappingRule( + MappingRuleMatchers.createUserGroupMatcher("sec.test.grp"), + (MappingRuleActions.createUpdateDefaultAction("root.user.testuser")) + .setFallbackSkip())); rules.add(new MappingRule( - MappingRuleMatchers.createUserMatcher("test_dot_user"), - (MappingRuleActions.createPlaceToDefaultAction()) - .setFallbackReject())); + MappingRuleMatchers.createUserMatcher("test.user"), + (MappingRuleActions.createPlaceToDefaultAction()) + .setFallbackReject())); CSMappingPlacementRule engine = setupEngine(true, rules); ApplicationSubmissionContext app = createApp("app"); assertPlace( - "test.user should be placed to root.user.bob", - engine, app, "test.user", "root.user.bob"); + "test.user should be placed to root.user", + engine, app, "test.user", "root.user.testuser"); + } + + private static void setLoggerToDebug() { + Log log = LogFactory.getLog("org.apache.hadoop.yarn.server.resourcemanager.placement"); + if (log instanceof Log4JLogger) { + org.apache.log4j.Logger logger = ((Log4JLogger) log).getLogger(); + logger.setLevel(Level.DEBUG); + } } private CSMappingPlacementRule initPlacementEngine(CapacityScheduler cs) throws IOException { From c8099eed325406bea5dcf0f866442875f2ca1edd Mon Sep 17 00:00:00 2001 From: Szilard Nemeth Date: Mon, 9 Jan 2023 22:37:20 +0100 Subject: [PATCH 3/5] Fix review comments --- .../yarn/server/resourcemanager/placement/VariableContext.java | 3 +-- 1 file changed, 1 insertion(+), 2 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/placement/VariableContext.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java index d47678a6da2f9..46117c73af1e3 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java @@ -156,8 +156,7 @@ public String get(String name) { } public String getOriginal(String name) { - String ret = originalVariables.get(name); - return ret; + return originalVariables.get(name); } /** From 0d6876ed6c328899377e429ca74ea4c1a12b68cc Mon Sep 17 00:00:00 2001 From: Szilard Nemeth Date: Tue, 10 Jan 2023 10:52:38 +0100 Subject: [PATCH 4/5] Remove debug logging --- .../csmappingrule/TestCSMappingPlacementRule.java | 15 --------------- 1 file changed, 15 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/placement/csmappingrule/TestCSMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java index 291129ec7ab6f..e72d09b6f2f79 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java @@ -36,8 +36,6 @@ import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration; import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerQueueManager; import org.apache.hadoop.yarn.util.Records; -import org.apache.log4j.Level; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; @@ -71,11 +69,6 @@ public class TestCSMappingPlacementRule { @Rule public TemporaryFolder folder = new TemporaryFolder(); - @Before - public void setUp() { - setLoggerToDebug(); - } - private Map> userGroups = ImmutableMap.>builder() .put("alice", ImmutableSet.of("p_alice", "unique", "user")) @@ -908,14 +901,6 @@ public void testOriginalGroupNameWithDotCanBeUsedInMatchExpression() throws IOEx engine, app, "test.user", "root.user.testuser"); } - private static void setLoggerToDebug() { - Log log = LogFactory.getLog("org.apache.hadoop.yarn.server.resourcemanager.placement"); - if (log instanceof Log4JLogger) { - org.apache.log4j.Logger logger = ((Log4JLogger) log).getLogger(); - logger.setLevel(Level.DEBUG); - } - } - private CSMappingPlacementRule initPlacementEngine(CapacityScheduler cs) throws IOException { CSMappingPlacementRule engine = new CSMappingPlacementRule(); engine.setFailOnConfigError(true); From 4d541d794f2f9e387cad6c5eb8f7d19c4760a337 Mon Sep 17 00:00:00 2001 From: Szilard Nemeth Date: Tue, 10 Jan 2023 15:20:13 +0100 Subject: [PATCH 5/5] Fix blanks / checkstyle --- .../server/resourcemanager/placement/VariableContext.java | 2 +- .../placement/csmappingrule/MappingRuleMatchers.java | 4 ++-- .../placement/csmappingrule/TestCSMappingPlacementRule.java | 3 --- 3 files changed, 3 insertions(+), 6 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/placement/VariableContext.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java index 46117c73af1e3..e8e419c64eb0d 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/VariableContext.java @@ -154,7 +154,7 @@ public String get(String name) { String ret = variables.get(name); return ret == null ? "" : ret; } - + public String getOriginal(String name) { return originalVariables.get(name); } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java index ae3ca88b5d6e8..0466dcffe974c 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/MappingRuleMatchers.java @@ -87,12 +87,12 @@ public boolean match(VariableContext variables) { } String substituted = variables.replaceVariables(value); - + String originalVariableValue = variables.getOriginal(variable); if (originalVariableValue != null) { return substituted.equals(originalVariableValue); } - + return substituted.equals(variables.get(variable)); } diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java index e72d09b6f2f79..41ce2b56eab20 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/placement/csmappingrule/TestCSMappingPlacementRule.java @@ -18,9 +18,6 @@ package org.apache.hadoop.yarn.server.resourcemanager.placement.csmappingrule; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; -import org.apache.commons.logging.impl.Log4JLogger; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.security.GroupMappingServiceProvider; import org.apache.hadoop.thirdparty.com.google.common.collect.ImmutableMap;