YARN-9511. Refactoring TestAuxServices to eliminate flakyness (apache#7598)

Author: p-szucs

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java

@@ -516,7 +516,8 @@ public synchronized void reload(AuxServiceRecords services) throws
     loadServices(services, getConfig(), true);
   }
 
-  private boolean checkManifestPermissions(FileStatus status) throws
+  @VisibleForTesting
+  boolean checkManifestPermissions(FileStatus status) throws
       IOException {
     if ((status.getPermission().toShort() & 0022) != 0) {
       LOG.error("Manifest file and parents must not be writable by group or " +
@@ -528,7 +529,7 @@ private boolean checkManifestPermissions(FileStatus status) throws
     if (parent == null) {
       return true;
     }
-    return checkManifestPermissions(manifestFS.getFileStatus(parent));
+    return checkManifestPermissions(getManifestFS().getFileStatus(parent));
   }
 
   private boolean checkManifestOwnerAndPermissions(FileStatus status) throws
@@ -964,6 +965,10 @@ protected static void setSystemClasses(AuxServiceRecord service, String
     service.getConfiguration().setProperty(SYSTEM_CLASSES, systemClasses);
   }
 
+  protected FileSystem getManifestFS() {
+    return manifestFS;
+  }
+
   /**
    * Class which is used by the {@link Timer} class to periodically execute the
    * manifest reload.

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java

@@ -28,7 +28,9 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.any;
@@ -122,6 +124,8 @@ public class TestAuxServices {
       .getSimpleName());
   private File manifest = new File(rootDir, "manifest.txt");
   private ObjectMapper mapper = new ObjectMapper();
+  private static final FsPermission WRITABLE_BY_OWNER = FsPermission.createImmutable((short) 0755);
+  private static final FsPermission WRITABLE_BY_GROUP = FsPermission.createImmutable((short) 0775);
 
   public static Collection<Boolean> getParams() {
     return Arrays.asList(false, true);
@@ -266,6 +270,24 @@ private void writeManifestFile(AuxServiceRecords services, Configuration
     mapper.writeValue(manifest, services);
   }
 
+  /**
+   * Creates a spy object of AuxServices for test cases which assume that we have proper
+   * file system permissions by default.
+   *
+   * Permission checking iterates through the parents of the manifest file until it
+   * reaches the system root, so without mocking this the success of the initialization
+   * would heavily depend on the environment where the test is running.
+   *
+   * @return a spy object of AuxServices
+   */
+  private AuxServices getSpyAuxServices(AuxiliaryLocalPathHandler auxiliaryLocalPathHandler,
+      Context nmContext, DeletionService deletionService) throws IOException {
+    AuxServices auxServices = spy(new AuxServices(auxiliaryLocalPathHandler,
+        nmContext, deletionService));
+    doReturn(true).when(auxServices).checkManifestPermissions(any(FileStatus.class));
+    return auxServices;
+  }
+
   @SuppressWarnings("resource")
   @ParameterizedTest
   @MethodSource("getParams")
@@ -317,7 +339,7 @@ public void testRemoteAuxServiceClassPath(boolean pUseManifest) throws Exception
               YarnConfiguration.NM_AUX_SERVICE_REMOTE_CLASSPATH, "ServiceC"),
               testJar.getAbsolutePath());
         }
-        aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+        aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
             mockContext2, mockDelService2);
         aux.init(conf);
         fail("The permission of the jar is wrong."
@@ -339,7 +361,7 @@ public void testRemoteAuxServiceClassPath(boolean pUseManifest) throws Exception
             YarnConfiguration.NM_AUX_SERVICE_REMOTE_CLASSPATH, "ServiceC"),
             testJar.getAbsolutePath());
       }
-      aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+      aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
           mockContext2, mockDelService2);
       aux.init(conf);
       aux.start();
@@ -356,7 +378,7 @@ public void testRemoteAuxServiceClassPath(boolean pUseManifest) throws Exception
 
       // initialize the same auxservice again, and make sure that we did not
       // re-download the jar from remote directory.
-      aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+      aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
           mockContext2, mockDelService2);
       aux.init(conf);
       aux.start();
@@ -377,7 +399,7 @@ public void testRemoteAuxServiceClassPath(boolean pUseManifest) throws Exception
       FileTime fileTime = FileTime.fromMillis(time);
       Files.setLastModifiedTime(Paths.get(testJar.getAbsolutePath()),
           fileTime);
-      aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+      aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
           mockContext2, mockDelService2);
       aux.init(conf);
       aux.start();
@@ -419,7 +441,7 @@ public void testCustomizedAuxServiceClassPath(boolean pUseManifest) throws Excep
           "ServiceC"), ServiceC.class, Service.class);
     }
     @SuppressWarnings("resource")
-    AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
     aux.start();
@@ -472,7 +494,7 @@ public void testCustomizedAuxServiceClassPath(boolean pUseManifest) throws Excep
       when(mockDirsHandler.getLocalPathForWrite(anyString())).thenReturn(
           rootAuxServiceDirPath);
       when(mockContext2.getLocalDirsHandler()).thenReturn(mockDirsHandler);
-      aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+      aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
           mockContext2, MOCK_DEL_SERVICE);
       aux.init(conf);
       aux.start();
@@ -654,7 +676,7 @@ private Configuration getABConf(String aName, String bName,
   public void testAuxServices(boolean pUseManifest) throws IOException {
     initTestAuxServices(pUseManifest);
     Configuration conf = getABConf();
-    final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
 
@@ -684,7 +706,7 @@ public void testAuxServices(boolean pUseManifest) throws IOException {
   public void testAuxServicesMeta(boolean pUseManifest) throws IOException {
     initTestAuxServices(pUseManifest);
     Configuration conf = getABConf();
-    final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
 
@@ -718,7 +740,7 @@ public void testAuxUnexpectedStop(boolean pUseManifest) throws IOException {
     initTestAuxServices(pUseManifest);
     // AuxServices no longer expected to stop when services stop
     Configuration conf = getABConf();
-    final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
     aux.start();
@@ -736,7 +758,7 @@ public void testValidAuxServiceName(boolean pUseManifest) throws IOException {
     initTestAuxServices(pUseManifest);
     Configuration conf = getABConf("Asrv1", "Bsrv_2", ServiceA.class,
         ServiceB.class);
-    final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     try {
       aux.init(conf);
@@ -745,7 +767,7 @@ public void testValidAuxServiceName(boolean pUseManifest) throws IOException {
     }
 
     //Test bad auxService Name
-    final AuxServices aux1 = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    final AuxServices aux1 = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     if (useManifest) {
       AuxServiceRecord serviceA =
@@ -776,7 +798,7 @@ public void testAuxServiceRecoverySetup(boolean pUseManifest) throws IOException
     conf.setBoolean(YarnConfiguration.NM_RECOVERY_ENABLED, true);
     conf.set(YarnConfiguration.NM_RECOVERY_DIR, TEST_DIR.toString());
     try {
-      final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+      final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
           MOCK_CONTEXT, MOCK_DEL_SERVICE);
       aux.init(conf);
       assertEquals(2, aux.getServices().size());
@@ -906,62 +928,130 @@ public void testAuxServicesConfChange(boolean pUseManifest) throws IOException {
 
   @ParameterizedTest
   @MethodSource("getParams")
-  public void testAuxServicesManifestPermissions(boolean pUseManifest) throws IOException {
+  public void testAuxServicesInitWithManifestOwnerAndPermissionCheck(boolean pUseManifest)
+      throws IOException {
     initTestAuxServices(pUseManifest);
     assumeTrue(useManifest);
     Configuration conf = getABConf();
-    FileSystem fs = FileSystem.get(conf);
-    fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
-        .createImmutable((short) 0777));
-    AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
-        MOCK_CONTEXT, MOCK_DEL_SERVICE);
+    AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE));
+    doReturn(false).when(aux).checkManifestPermissions(any(FileStatus.class));
     aux.init(conf);
     assertEquals(0, aux.getServices().size());
 
-    fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
-        .createImmutable((short) 0775));
-    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
-        MOCK_CONTEXT, MOCK_DEL_SERVICE);
-    aux.init(conf);
-    assertEquals(0, aux.getServices().size());
-
-    fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
-        .createImmutable((short) 0755));
-    fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
-        .createImmutable((short) 0775));
-    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
-        MOCK_CONTEXT, MOCK_DEL_SERVICE);
-    aux.init(conf);
-    assertEquals(0, aux.getServices().size());
-
-    fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
-        .createImmutable((short) 0755));
-    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
     assertEquals(2, aux.getServices().size());
 
     conf.set(YarnConfiguration.YARN_ADMIN_ACL, "");
-    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
     assertEquals(0, aux.getServices().size());
 
     conf.set(YarnConfiguration.YARN_ADMIN_ACL, UserGroupInformation
         .getCurrentUser().getShortUserName());
-    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
     assertEquals(2, aux.getServices().size());
   }
 
+  @ParameterizedTest
+  @MethodSource("getParams")
+  public void testCheckManifestPermissionsWhenFileIsOnlyWritableByOwner(boolean pUseManifest)
+      throws IOException {
+    initTestAuxServices(pUseManifest);
+    assumeTrue(useManifest);
+    final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE));
+    FileStatus manifestFileStatus = mock(FileStatus.class);
+    Path manifestPath = mock(Path.class);
+
+    when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+    when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+
+    assertTrue(aux.checkManifestPermissions(manifestFileStatus));
+  }
+
+  @ParameterizedTest
+  @MethodSource("getParams")
+  public void testCheckManifestPermissionsWhenFileIsWritableByGroup(boolean pUseManifest)
+      throws IOException {
+    initTestAuxServices(pUseManifest);
+    assumeTrue(useManifest);
+    final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE));
+    FileStatus manifestFileStatus = mock(FileStatus.class);
+    Path manifestPath = mock(Path.class);
+
+    when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_GROUP);
+    when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+
+    assertFalse(aux.checkManifestPermissions(manifestFileStatus));
+  }
+
+  @ParameterizedTest
+  @MethodSource("getParams")
+  public void testCheckManifestPermissionsWhenParentIsWritableByGroup(boolean pUseManifest)
+      throws IOException {
+    initTestAuxServices(pUseManifest);
+    assumeTrue(useManifest);
+    final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE));
+
+    FileStatus manifestFileStatus = mock(FileStatus.class);
+    FileStatus parentFolderStatus = mock(FileStatus.class);
+    when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+    when(parentFolderStatus.getPermission()).thenReturn(WRITABLE_BY_GROUP);
+
+    Path manifestPath = mock(Path.class);
+    Path parentPath = mock(Path.class);
+    when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+    when(manifestPath.getParent()).thenReturn(parentPath);
+
+    FileSystem manifestFs = mock(FileSystem.class);
+    when(manifestFs.getFileStatus(parentPath)).thenReturn(parentFolderStatus);
+    doReturn(manifestFs).when(aux).getManifestFS();
+
+    assertFalse(aux.checkManifestPermissions(manifestFileStatus));
+  }
+
+  @ParameterizedTest
+  @MethodSource("getParams")
+  public void testCheckManifestPermissionsWhenParentAndFileIsWritableByOwner(boolean pUseManifest)
+      throws IOException {
+    initTestAuxServices(pUseManifest);
+    assumeTrue(useManifest);
+    final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE));
+
+    FileStatus manifestFileStatus = mock(FileStatus.class);
+    FileStatus parentFolderStatus = mock(FileStatus.class);
+    when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+    when(parentFolderStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+
+    Path manifestPath = mock(Path.class);
+    Path parentPath = mock(Path.class);
+    when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+    when(parentFolderStatus.getPath()).thenReturn(parentPath);
+    when(manifestPath.getParent()).thenReturn(parentPath);
+
+    FileSystem manifestFs = mock(FileSystem.class);
+    when(manifestFs.getFileStatus(parentPath)).thenReturn(parentFolderStatus);
+    doReturn(manifestFs).when(aux).getManifestFS();
+
+    assertTrue(aux.checkManifestPermissions(manifestFileStatus));
+  }
+
   @ParameterizedTest
   @MethodSource("getParams")
   public void testRemoveManifest(boolean pUseManifest) throws IOException {
     initTestAuxServices(pUseManifest);
     assumeTrue(useManifest);
     Configuration conf = getABConf();
-    final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
     assertEquals(2, aux.getServices().size());
@@ -976,7 +1066,7 @@ public void testManualReload(boolean pUseManifest) throws IOException {
     initTestAuxServices(pUseManifest);
     assumeTrue(useManifest);
     Configuration conf = getABConf();
-    final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+    final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
         MOCK_CONTEXT, MOCK_DEL_SERVICE);
     aux.init(conf);
     try {