MAPREDUCE-7456. Extend add-opens flag to container launch commands on…

… JDK17 nodes. Contributed by Peter Szucs
apache · Sep 28, 2023 · 2d871fa · 2d871fa
1 parent 0c153fe
commit 2d871fa
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 4 deletions.
diff --git a/...oop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java b/...oop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java
@@ -2232,6 +2232,14 @@ public static boolean isAclEnabled(Configuration conf) {
   public static final String NM_CONTAINER_LOCALIZER_JAVA_OPTS_DEFAULT =
       "-Xmx256m";
 
+  /*
+   * Flag to indicate whether JDK17's required add-exports flags should be added to
+   * container localizers regardless of the user specified JAVA_OPTS.
+   */
+  public static final String NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_KEY =
+      NM_PREFIX + "container-localizer.java.opts.add-exports-as-default";
+  public static final boolean NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_DEFAULT = true;
+
   /** The log level of container localizer process. */
   public static final String NM_CONTAINER_LOCALIZER_LOG_LEVEL=
       NM_PREFIX + "container-localizer.log.level";

diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml
@@ -1484,6 +1484,17 @@
     <value>-Xmx256m</value>
   </property>
 
+  <property>
+    <name>yarn.nodemanager.container-localizer.java.opts.add-exports-as-default</name>
+    <value>true</value>
+    <description>Since on JDK17 it is no longer possible to use the reflection API to
+      access public fields and methods, add-exports flags should be added to container
+      localizers regardless of the user specified JAVA_OPTS. Setting this to true will
+      add the flags to the container localizer on nodes with JDK17 or higher.
+      Defaults to true, but the setting has no effect on nodes using JDK16 and before.
+    </description>
+  </property>
+
   <property>
     <description>
       The log level for container localizer while it is an independent process.

diff --git a/.../org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java b/.../org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
@@ -123,6 +123,11 @@ public class ContainerLaunch implements Callable<Integer> {
   private static final String PID_FILE_NAME_FMT = "%s.pid";
   static final String EXIT_CODE_FILE_SUFFIX = ".exitcode";
 
+  private static final String ADDITIONAL_JDK17_PLUS_OPTIONS =
+      "--add-opens=java.base/java.lang=ALL-UNNAMED " +
+      "--add-exports=java.base/sun.net.dns=ALL-UNNAMED " +
+      "--add-exports=java.base/sun.net.util=ALL-UNNAMED";
+
   protected final Dispatcher dispatcher;
   protected final ContainerExecutor exec;
   protected final Application app;
@@ -171,8 +176,7 @@ public static String expandEnvironment(String var,
       File.pathSeparator);
 
     if (Shell.isJavaVersionAtLeast(17)) {
-      var = var.replace(ApplicationConstants.JVM_ADD_OPENS_VAR,
-              "--add-opens=java.base/java.lang=ALL-UNNAMED");
+      var = var.replace(ApplicationConstants.JVM_ADD_OPENS_VAR, ADDITIONAL_JDK17_PLUS_OPTIONS);
     } else {
       var = var.replace(ApplicationConstants.JVM_ADD_OPENS_VAR, "");
     }

diff --git a/.../apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ContainerLocalizer.java b/.../apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ContainerLocalizer.java
@@ -103,6 +103,14 @@ public class ContainerLocalizer {
       new FsPermission((short) 0755);
   public static final String CSI_VOLIUME_MOUNTS_ROOT = "csivolumes";
 
+  /*
+   * Testing discovered that these Java options are needed for Spark service
+   * running on JDK17 and Isilon clusters.
+   */
+  private static final String ADDITIONAL_JDK17_PLUS_OPTIONS =
+    "--add-exports=java.base/sun.net.dns=ALL-UNNAMED " +
+    "--add-exports=java.base/sun.net.util=ALL-UNNAMED";
+
   private final String user;
   private final String appId;
   private final List<Path> localDirs;
@@ -400,6 +408,13 @@ private LocalizerStatus createStatus() throws InterruptedException {
   public static List<String> getJavaOpts(Configuration conf) {
     String opts = conf.get(YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_KEY,
         YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_DEFAULT);
+    boolean isExtraJDK17OptionsConfigured =
+        conf.getBoolean(YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_KEY,
+        YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_DEFAULT);
+
+    if (Shell.isJavaVersionAtLeast(17) && isExtraJDK17OptionsConfigured) {
+      opts = opts.trim().concat(" " + ADDITIONAL_JDK17_PLUS_OPTIONS);
+    }
     return Arrays.asList(opts.split(" "));
   }
 

diff --git a/.../apache/hadoop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunch.java b/.../apache/hadoop/yarn/server/nodemanager/containermanager/launcher/TestContainerLaunch.java
@@ -580,8 +580,12 @@ public void testEnvExpansion() throws IOException {
 
     String res = ContainerLaunch.expandEnvironment(input, logPath);
 
-    String expectedAddOpens = Shell.isJavaVersionAtLeast(17) ?
-        "--add-opens=java.base/java.lang=ALL-UNNAMED" : "";
+    String additionalJdk17PlusOptions =
+        "--add-opens=java.base/java.lang=ALL-UNNAMED " +
+        "--add-exports=java.base/sun.net.dns=ALL-UNNAMED " +
+        "--add-exports=java.base/sun.net.util=ALL-UNNAMED";
+    String expectedAddOpens = Shell.isJavaVersionAtLeast(17) ? additionalJdk17PlusOptions : "";
+
     if (Shell.WINDOWS) {
       Assert.assertEquals("%HADOOP_HOME%/share/hadoop/common/*;"
           + "%HADOOP_HOME%/share/hadoop/common/lib/*;"

diff --git a/...che/hadoop/yarn/server/nodemanager/containermanager/localizer/TestContainerLocalizer.java b/...che/hadoop/yarn/server/nodemanager/containermanager/localizer/TestContainerLocalizer.java
@@ -702,4 +702,39 @@ public void testUserCacheDirPermission() throws Exception {
         lfs.getFileStatus(destDirPath.getParent().getParent()).getPermission());
   }
 
+  @Test
+  public void testDefaultJavaOptionsWhenExtraJDK17OptionsAreConfigured() throws Exception {
+    ContainerLocalizerWrapper wrapper = new ContainerLocalizerWrapper();
+    ContainerLocalizer localizer = wrapper.setupContainerLocalizerForTest();
+
+    Configuration conf = new Configuration();
+    conf.setBoolean(YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_KEY,
+        true);
+
+    List<String> javaOpts = localizer.getJavaOpts(conf);
+
+    if (Shell.isJavaVersionAtLeast(17)) {
+      Assert.assertTrue(javaOpts.contains("--add-exports=java.base/sun.net.dns=ALL-UNNAMED"));
+      Assert.assertTrue(javaOpts.contains("--add-exports=java.base/sun.net.util=ALL-UNNAMED"));
+    }
+    Assert.assertTrue(javaOpts.contains("-Xmx256m"));
+  }
+
+  @Test
+  public void testDefaultJavaOptionsWhenExtraJDK17OptionsAreNotConfigured() throws Exception {
+    ContainerLocalizerWrapper wrapper = new ContainerLocalizerWrapper();
+    ContainerLocalizer localizer = wrapper.setupContainerLocalizerForTest();
+
+    Configuration conf = new Configuration();
+    conf.setBoolean(YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_KEY,
+        false);
+
+    List<String> javaOpts = localizer.getJavaOpts(conf);
+
+    if (Shell.isJavaVersionAtLeast(17)) {
+      Assert.assertFalse(javaOpts.contains("--add-exports=java.base/sun.net.dns=ALL-UNNAMED"));
+      Assert.assertFalse(javaOpts.contains("--add-exports=java.base/sun.net.util=ALL-UNNAMED"));
+    }
+    Assert.assertTrue(javaOpts.contains("-Xmx256m"));
+  }
 }