YARN-11578. Cache fs supports chmod in LogAggregationFileController. (#6120) (#6142)

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/logaggregation/filecontroller/LogAggregationFileController.java

@@ -33,7 +33,9 @@ import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.hadoop.classification.InterfaceAudience.Public;
@@ -108,6 +110,35 @@ public abstract class LogAggregationFileController {
 
   protected boolean fsSupportsChmod = true;
 
+  private static class FsLogPathKey {
+    private Class<? extends FileSystem> fsType;
+    private Path logPath;
+
+    FsLogPathKey(Class<? extends FileSystem> fsType, Path logPath) {
+      this.fsType = fsType;
+      this.logPath = logPath;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) {
+        return true;
+      }
+      if (o == null || getClass() != o.getClass()) {
+        return false;
+      }
+      FsLogPathKey that = (FsLogPathKey) o;
+      return Objects.equals(fsType, that.fsType) && Objects.equals(logPath, that.logPath);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(fsType, logPath);
+    }
+  }
+  private static final ConcurrentHashMap<FsLogPathKey, Boolean> FS_CHMOD_CACHE
+      = new ConcurrentHashMap<>();
+
   public LogAggregationFileController() {}
 
   /**
@@ -375,26 +406,34 @@ public abstract class LogAggregationFileController {
             + remoteRootLogDir + "]", e);
       }
     } else {
-      //Check if FS has capability to set/modify permissions
-      Path permissionCheckFile = new Path(qualified, String.format("%s.permission_check",
-          RandomStringUtils.randomAlphanumeric(8)));
+      final FsLogPathKey key = new FsLogPathKey(remoteFS.getClass(), qualified);
+      FileSystem finalRemoteFS = remoteFS;
+      fsSupportsChmod = FS_CHMOD_CACHE.computeIfAbsent(key,
+          k -> checkFsSupportsChmod(finalRemoteFS, remoteRootLogDir, qualified));
+    }
+  }
+
+  private boolean checkFsSupportsChmod(FileSystem remoteFS, Path logDir, Path qualified) {
+    //Check if FS has capability to set/modify permissions
+    Path permissionCheckFile = new Path(qualified, String.format("%s.permission_check",
+        RandomStringUtils.randomAlphanumeric(8)));
+    try {
+      remoteFS.createNewFile(permissionCheckFile);
+      remoteFS.setPermission(permissionCheckFile, new FsPermission(TLDIR_PERMISSIONS));
+      return true;
+    } catch (UnsupportedOperationException use) {
+      LOG.info("Unable to set permissions for configured filesystem since"
+          + " it does not support this {}", remoteFS.getScheme());
+    } catch (IOException e) {
+      LOG.warn("Failed to check if FileSystem supports permissions on "
+          + "remoteLogDir [{}]", logDir, e);
+    } finally {
       try {
-        remoteFS.createNewFile(permissionCheckFile);
-        remoteFS.setPermission(permissionCheckFile, new FsPermission(TLDIR_PERMISSIONS));
-      } catch (UnsupportedOperationException use) {
-        LOG.info("Unable to set permissions for configured filesystem since"
-            + " it does not support this", remoteFS.getScheme());
-        fsSupportsChmod = false;
-      } catch (IOException e) {
-        LOG.warn("Failed to check if FileSystem suppports permissions on "
-            + "remoteLogDir [" + remoteRootLogDir + "]", e);
-      } finally {
-        try {
-          remoteFS.delete(permissionCheckFile, false);
-        } catch (IOException ignored) {
-        }
+        remoteFS.delete(permissionCheckFile, false);
+      } catch (IOException ignored) {
       }
     }
+    return false;
   }
 
   /**

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/test/java/org/apache/hadoop/yarn/logaggregation/filecontroller/TestLogAggregationFileController.java

@@ -18,6 +18,14 @@
 
 package org.apache.hadoop.yarn.logaggregation.filecontroller;
 
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+
+import org.mockito.ArgumentMatcher;
+import org.mockito.Mockito;
+
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
@@ -27,11 +35,7 @@ import org.apache.hadoop.security.UserGroupInformation;
 import org.apache.hadoop.yarn.conf.YarnConfiguration;
 import org.junit.Assert;
 import org.junit.Test;
-import org.mockito.ArgumentMatcher;
-import org.mockito.Mockito;
 
-import java.io.FileNotFoundException;
-import java.net.URI;
 
 import static org.apache.hadoop.yarn.logaggregation.filecontroller.LogAggregationFileController.TLDIR_PERMISSIONS;
 import static org.mockito.ArgumentMatchers.any;
@@ -40,6 +44,7 @@ import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
 /**
@@ -115,30 +120,76 @@ public class TestLogAggregationFileController {
 
   @Test
   public void testRemoteDirCreationWithCustomUser() throws Exception {
+    LogAggregationFileController controller = mock(
+        LogAggregationFileController.class, Mockito.CALLS_REAL_METHODS);
     FileSystem fs = mock(FileSystem.class);
-    doReturn(new URI("")).when(fs).getUri();
-    doReturn(new FileStatus(128, false, 0, 64, System.currentTimeMillis(),
-        System.currentTimeMillis(), new FsPermission(TLDIR_PERMISSIONS),
-        "not_yarn_user", "yarn_group", new Path("/tmp/logs"))).when(fs)
-        .getFileStatus(any(Path.class));
+    setupCustomUserMocks(controller, fs, "/tmp/logs");
 
-    Configuration conf = new Configuration();
+    controller.initialize(new Configuration(), "TFile");
+    controller.fsSupportsChmod = false;
+
+    controller.verifyAndCreateRemoteLogDir();
+    assertPermissionFileWasUsedOneTime(fs);
+    Assert.assertTrue(controller.fsSupportsChmod);
+
+    doThrow(UnsupportedOperationException.class).when(fs).setPermission(any(), any());
+    controller.verifyAndCreateRemoteLogDir();
+    assertPermissionFileWasUsedOneTime(fs); // still once -> cached
+    Assert.assertTrue(controller.fsSupportsChmod);
+
+    controller.fsSupportsChmod = false;
+    controller.verifyAndCreateRemoteLogDir();
+    assertPermissionFileWasUsedOneTime(fs); // still once -> cached
+    Assert.assertTrue(controller.fsSupportsChmod);
+  }
+
+  @Test
+  public void testRemoteDirCreationWithCustomUserFsChmodNotSupported() throws Exception {
     LogAggregationFileController controller = mock(
         LogAggregationFileController.class, Mockito.CALLS_REAL_METHODS);
+    FileSystem fs = mock(FileSystem.class);
+    setupCustomUserMocks(controller, fs, "/tmp/logs2");
+    doThrow(UnsupportedOperationException.class).when(fs).setPermission(any(), any());
+
+    Configuration conf = new Configuration();
+    conf.set(YarnConfiguration.NM_REMOTE_APP_LOG_DIR, "/tmp/logs2");
+    controller.initialize(conf, "TFile");
+    controller.verifyAndCreateRemoteLogDir();
+    assertPermissionFileWasUsedOneTime(fs);
+    Assert.assertFalse(controller.fsSupportsChmod);
+
+    controller.verifyAndCreateRemoteLogDir();
+    assertPermissionFileWasUsedOneTime(fs); // still once -> cached
+    Assert.assertFalse(controller.fsSupportsChmod);
+
     controller.fsSupportsChmod = true;
+    controller.verifyAndCreateRemoteLogDir();
+    assertPermissionFileWasUsedOneTime(fs); // still once -> cached
+    Assert.assertFalse(controller.fsSupportsChmod);
+  }
+
+  private static void setupCustomUserMocks(LogAggregationFileController controller,
+                                           FileSystem fs, String path)
+      throws URISyntaxException, IOException {
+    doReturn(new URI("")).when(fs).getUri();
+    doReturn(new FileStatus(128, false, 0, 64, System.currentTimeMillis(),
+        System.currentTimeMillis(), new FsPermission(TLDIR_PERMISSIONS),
+        "not_yarn_user", "yarn_group", new Path(path))).when(fs)
+        .getFileStatus(any(Path.class));
     doReturn(fs).when(controller).getFileSystem(any(Configuration.class));
 
     UserGroupInformation ugi = UserGroupInformation.createUserForTesting(
         "yarn_user", new String[]{"yarn_group", "other_group"});
     UserGroupInformation.setLoginUser(ugi);
+  }
 
-    controller.initialize(conf, "TFile");
-    controller.verifyAndCreateRemoteLogDir();
-
-    verify(fs).createNewFile(argThat(new PathContainsString(".permission_check")));
-    verify(fs).setPermission(argThat(new PathContainsString(".permission_check")),
-        eq(new FsPermission(TLDIR_PERMISSIONS)));
-    verify(fs).delete(argThat(new PathContainsString(".permission_check")), eq(false));
-    Assert.assertTrue(controller.fsSupportsChmod);
+  private static void assertPermissionFileWasUsedOneTime(FileSystem fs) throws IOException {
+    verify(fs, times(1))
+        .createNewFile(argThat(new PathContainsString(".permission_check")));
+    verify(fs, times(1))
+        .setPermission(argThat(new PathContainsString(".permission_check")),
+            eq(new FsPermission(TLDIR_PERMISSIONS)));
+    verify(fs, times(1))
+        .delete(argThat(new PathContainsString(".permission_check")), eq(false));
   }
 }