YARN-11692. Fixed review comments

Change-Id: Ib7673c9ba18fd7a4f7edde63972fc1ff66bca7de

Author: p-szucs

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java

@@ -2733,7 +2733,7 @@ public static boolean isAclEnabled(Configuration conf) {
   public static final String NM_LINUX_CONTAINER_CGROUPS_MOUNT_PATH =
     NM_PREFIX + "linux-container-executor.cgroups.mount-path";
 
-  /** Where the linux container executor should mount cgroups v2 if not found */
+  /** Where the linux container executor should mount cgroups v2 if not found. */
   public static final String NM_LINUX_CONTAINER_CGROUPS_V2_MOUNT_PATH =
       NM_PREFIX + "linux-container-executor.cgroups.v2.mount-path";
 

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml

@@ -2090,10 +2090,10 @@
   <property>
     <description>This property sets the mount path for CGroups v2.
       This parameter is optional, and needed to be set only in mixed mode,
-      when CGroups v2 is mounted in a different path than Cgroups v1.
-      For example, in hybrid mode, CGroups v1 controllers can be mounted in
-      /sys/fs/cgroup, while v2 can be mounted in /sys/fs/cgroup/unified folder.
-      
+      when CGroups v2 is mounted alongside with Cgroups v1.
+      For example, in hybrid mode, CGroups v1 controllers can be mounted under /sys/fs/cgroup/
+      (for example /sys/fs/cgroup/cpu,cpuacct), while v2 can be mounted in /sys/fs/cgroup/unified folder.
+
       If this value is not set, the value of
       yarn.nodemanager.linux-container-executor.cgroups.mount-path
       will be used for CGroups v2 as well.

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/CGroupsMountConfig.java

@@ -25,9 +25,9 @@
 public class CGroupsMountConfig {
   private final boolean enableMount;
   private final String mountPath;
-  
+
   // CGroups v2 mount path is only relevant in mixed CGroups v1/v2 mode,
-  // where v2 can be mounted in a different folder than v1.
+  // where v2 is mounted alongside with v1.
   private final String v2MountPath;
 
   public CGroupsMountConfig(Configuration conf) {

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/ResourceHandlerModule.java

@@ -63,8 +63,8 @@ public class ResourceHandlerModule {
    * as resource metrics functionality. We need to ensure that the same
    * instance is used for both.
    */
-  private static volatile CGroupsHandler cGroupsV1Handler;
-  private static volatile CGroupsHandler cGroupsV2Handler;
+  private static volatile CGroupsHandler cGroupV1Handler;
+  private static volatile CGroupsHandler cGroupV2Handler;
   private static volatile TrafficControlBandwidthHandlerImpl
       trafficControlBandwidthHandler;
   private static volatile NetworkPacketTaggingHandlerImpl
@@ -76,41 +76,42 @@ public class ResourceHandlerModule {
   private static volatile CpuResourceHandler
       cGroupsCpuResourceHandler;
 
-  private static void initializeCGroupsHandlers(Configuration conf) throws ResourceHandlerException {
-    initializeCGroupsV1Handler(conf);
+  private static void initializeCGroupHandlers(Configuration conf)
+      throws ResourceHandlerException {
+    initializeCGroupV1Handler(conf);
     if (cgroupsV2Enabled) {
-      initializeCGroupsV2Handler(conf);
+      initializeCGroupV2Handler(conf);
     }
   }
 
-  private static void initializeCGroupsV1Handler(Configuration conf)
+  private static void initializeCGroupV1Handler(Configuration conf)
       throws ResourceHandlerException {
-    if (cGroupsV1Handler == null) {
+    if (cGroupV1Handler == null) {
       synchronized (CGroupsHandler.class) {
-        if (cGroupsV1Handler == null) {
-          cGroupsV1Handler = new CGroupsHandlerImpl(
+        if (cGroupV1Handler == null) {
+          cGroupV1Handler = new CGroupsHandlerImpl(
               conf, PrivilegedOperationExecutor.getInstance(conf));
-          LOG.debug("Value of CGroupsV1Handler is: {}", cGroupsV1Handler);
+          LOG.debug("Value of CGroupsV1Handler is: {}", cGroupV1Handler);
         }
       }
     }
   }
 
-  private static void initializeCGroupsV2Handler(Configuration conf)
+  private static void initializeCGroupV2Handler(Configuration conf)
       throws ResourceHandlerException {
-    if (cGroupsV2Handler == null) {
+    if (cGroupV2Handler == null) {
       synchronized (CGroupsHandler.class) {
-        if (cGroupsV2Handler == null) {
-          cGroupsV2Handler = new CGroupsV2HandlerImpl(
+        if (cGroupV2Handler == null) {
+          cGroupV2Handler = new CGroupsV2HandlerImpl(
               conf, PrivilegedOperationExecutor.getInstance(conf));
-          LOG.debug("Value of CGroupsV2Handler is: {}", cGroupsV2Handler);
+          LOG.debug("Value of CGroupsV2Handler is: {}", cGroupV2Handler);
         }
       }
     }
   }
 
   private static boolean isMountedInCGroupsV2(CGroupsHandler.CGroupController controller) {
-    return (cGroupsV2Handler != null && cGroupsV2Handler.getControllerPath(controller) != null);
+    return (cGroupV2Handler != null && cGroupV2Handler.getControllerPath(controller) != null);
   }
 
   /**
@@ -120,7 +121,7 @@ private static boolean isMountedInCGroupsV2(CGroupsHandler.CGroupController cont
    */
 
   public static CGroupsHandler getCGroupsHandler() {
-    return cgroupsV2Enabled ? cGroupsV2Handler : cGroupsV1Handler;
+    return cGroupV1Handler;
   }
 
   /**
@@ -173,11 +174,11 @@ private static CpuResourceHandler initCGroupsCpuResourceHandler(
           if (cGroupsCpuResourceHandler == null) {
             LOG.debug("Creating new cgroups cpu handler");
 
-            initializeCGroupsHandlers(conf);
+            initializeCGroupHandlers(conf);
             if (isMountedInCGroupsV2(CGroupsHandler.CGroupController.CPU)) {
-              cGroupsCpuResourceHandler = new CGroupsV2CpuResourceHandlerImpl(cGroupsV2Handler);
+              cGroupsCpuResourceHandler = new CGroupsV2CpuResourceHandlerImpl(cGroupV2Handler);
             } else {
-              cGroupsCpuResourceHandler = new CGroupsCpuResourceHandlerImpl(cGroupsV1Handler);
+              cGroupsCpuResourceHandler = new CGroupsCpuResourceHandlerImpl(cGroupV1Handler);
             }
             return cGroupsCpuResourceHandler;
           }
@@ -197,10 +198,10 @@ private static CpuResourceHandler initCGroupsCpuResourceHandler(
           if (trafficControlBandwidthHandler == null) {
             LOG.info("Creating new traffic control bandwidth handler.");
 
-            initializeCGroupsHandlers(conf);
+            initializeCGroupHandlers(conf);
             trafficControlBandwidthHandler = new
                 TrafficControlBandwidthHandlerImpl(PrivilegedOperationExecutor
-                .getInstance(conf), cGroupsV1Handler,
+                .getInstance(conf), cGroupV1Handler,
                 new TrafficController(conf, PrivilegedOperationExecutor
                     .getInstance(conf)));
           }
@@ -234,10 +235,10 @@ public static ResourceHandler getNetworkTaggingHandler(Configuration conf)
         if (networkPacketTaggingHandlerImpl == null) {
           LOG.info("Creating new network-tagging-handler.");
 
-          initializeCGroupsHandlers(conf);
+          initializeCGroupHandlers(conf);
           networkPacketTaggingHandlerImpl =
               new NetworkPacketTaggingHandlerImpl(
-                  PrivilegedOperationExecutor.getInstance(conf), cGroupsV1Handler);
+                  PrivilegedOperationExecutor.getInstance(conf), cGroupV1Handler);
         }
       }
     }
@@ -266,9 +267,9 @@ private static CGroupsBlkioResourceHandlerImpl getCgroupsBlkioResourceHandler(
         if (cGroupsBlkioResourceHandler == null) {
           LOG.debug("Creating new cgroups blkio handler");
 
-          initializeCGroupsHandlers(conf);
+          initializeCGroupHandlers(conf);
           cGroupsBlkioResourceHandler =
-              new CGroupsBlkioResourceHandlerImpl(cGroupsV1Handler);
+              new CGroupsBlkioResourceHandlerImpl(cGroupV1Handler);
         }
       }
     }
@@ -291,11 +292,11 @@ public static MemoryResourceHandler initMemoryResourceHandler(
       synchronized (MemoryResourceHandler.class) {
         if (cGroupsMemoryResourceHandler == null) {
 
-          initializeCGroupsHandlers(conf);
+          initializeCGroupHandlers(conf);
           if (isMountedInCGroupsV2(CGroupsHandler.CGroupController.MEMORY)) {
-            cGroupsMemoryResourceHandler = new CGroupsV2MemoryResourceHandlerImpl(cGroupsV2Handler);
+            cGroupsMemoryResourceHandler = new CGroupsV2MemoryResourceHandlerImpl(cGroupV2Handler);
           } else {
-            cGroupsMemoryResourceHandler = new CGroupsMemoryResourceHandlerImpl(cGroupsV1Handler);
+            cGroupsMemoryResourceHandler = new CGroupsMemoryResourceHandlerImpl(cGroupV1Handler);
           }
         }
       }
@@ -358,10 +359,10 @@ private static void addHandlersFromConfiguredResourcePlugins(
     }
 
     for (ResourcePlugin plugin : pluginMap.values()) {
-      initializeCGroupsHandlers(conf);
+      initializeCGroupHandlers(conf);
       addHandlerIfNotNull(handlerList,
           plugin.createResourceHandler(nmContext,
-              cGroupsV1Handler,
+              cGroupV1Handler,
               PrivilegedOperationExecutor.getInstance(conf)));
     }
   }
@@ -392,22 +393,6 @@ static void nullifyResourceHandlerChain() throws ResourceHandlerException {
     resourceHandlerChain = null;
   }
 
-  @VisibleForTesting
-  static void resetCGroupsHandlers() {
-    cGroupsV1Handler = null;
-    cGroupsV2Handler = null;
-  }
-
-  @VisibleForTesting
-  static void resetCpuResourceHandler() {
-    cGroupsCpuResourceHandler = null;
-  }
-
-  @VisibleForTesting
-  static void resetMemoryResourceHandler() {
-    cGroupsMemoryResourceHandler = null;
-  }
-
   /**
    * If a cgroup mount directory is specified, it returns cgroup directories
    * with valid names.

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/linux/resources/TestResourceHandlerModule.java

@@ -49,9 +49,6 @@ public void setup() throws Exception {
     networkEnabledConf.setBoolean(YarnConfiguration.NM_NETWORK_RESOURCE_ENABLED,
         true);
     ResourceHandlerModule.nullifyResourceHandlerChain();
-    ResourceHandlerModule.resetCGroupsHandlers();
-    ResourceHandlerModule.resetCpuResourceHandler();
-    ResourceHandlerModule.resetMemoryResourceHandler();
   }
 
   @Test
@@ -114,37 +111,4 @@ public void testDiskResourceHandler() throws Exception {
       Assert.fail("Null returned");
     }
   }
-
-  @Test
-  public void testCgroupsHandlerClassForCgroupV1() throws ResourceHandlerException {
-    Configuration conf = new YarnConfiguration();
-    conf.setBoolean(YarnConfiguration.NM_MEMORY_RESOURCE_ENABLED, true);
-    conf.setBoolean(YarnConfiguration.NM_LINUX_CONTAINER_CGROUPS_V2_ENABLED, false);
-
-    initResourceHandlerChain(conf);
-
-    Assert.assertTrue(ResourceHandlerModule.getCGroupsHandler()
-        instanceof CGroupsHandlerImpl);
-  }
-
-  @Test
-  public void testCgroupsHandlerClassForCgroupV2() throws ResourceHandlerException {
-    Configuration conf = new YarnConfiguration();
-    conf.setBoolean(YarnConfiguration.NM_MEMORY_RESOURCE_ENABLED, true);
-    conf.setBoolean(YarnConfiguration.NM_LINUX_CONTAINER_CGROUPS_V2_ENABLED, true);
-
-    initResourceHandlerChain(conf);
-
-    Assert.assertTrue(ResourceHandlerModule.getCGroupsHandler()
-        instanceof CGroupsV2HandlerImpl);
-  }
-
-  private void initResourceHandlerChain(Configuration conf) throws ResourceHandlerException {
-    ResourceHandlerChain resourceHandlerChain =
-        ResourceHandlerModule.getConfiguredResourceHandlerChain(conf,
-            mock(Context.class));
-    if (resourceHandlerChain == null) {
-      Assert.fail("Could not initialize resource handler chain");
-    }
-  }
 }