HADOOP-16380 S3Guard to determine empty directory status for all non-root directories

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

@@ -220,21 +220,21 @@ directory contains many thousands of files.
 
 Consider a directory `"/d"` with the contents:
 
-	a
-	part-0000001
-	part-0000002
-	...
-	part-9999999
+    a
+    part-0000001
+    part-0000002
+    ...
+    part-9999999
 
 
 If the number of files is such that HDFS returns a partial listing in each
 response, then, if a listing `listStatus("/d")` takes place concurrently with the operation
 `rename("/d/a","/d/z"))`, the result may be one of:
 
-	[a, part-0000001, ... , part-9999999]
-	[part-0000001, ... , part-9999999, z]
-	[a, part-0000001, ... , part-9999999, z]
-	[part-0000001, ... , part-9999999]
+    [a, part-0000001, ... , part-9999999]
+    [part-0000001, ... , part-9999999, z]
+    [a, part-0000001, ... , part-9999999, z]
+    [part-0000001, ... , part-9999999]
 
 While this situation is likely to be a rare occurrence, it MAY happen. In HDFS
 these inconsistent views are only likely when listing a directory with many children.
@@ -964,7 +964,7 @@ A path referring to a file is removed, return value: `True`
 Deleting an empty root does not change the filesystem state
 and may return true or false.
 
-    if isDir(FS, p) and isRoot(p) and children(FS, p) == {} :
+    if isRoot(p) and children(FS, p) == {} :
         FS ' = FS
         result = (undetermined)
 
@@ -973,6 +973,9 @@ There is no consistent return code from an attempt to delete the root directory.
 Implementations SHOULD return true; this avoids code which checks for a false
 return value from overreacting.
 
+*Object Stores*: see [Object Stores: root directory deletion](#object-stores-rm-root).
+
+
 ##### Empty (non-root) directory `recursive == False`
 
 Deleting an empty directory that is not root will remove the path from the FS and
@@ -986,7 +989,7 @@ return true.
 ##### Recursive delete of non-empty root directory
 
 Deleting a root path with children and `recursive==True`
- can do one of two things.
+can generally have three outcomes:
 
 1. The POSIX model assumes that if the user has
 the correct permissions to delete everything,
@@ -1004,6 +1007,8 @@ filesystem is desired.
             FS' = FS
             result = False
 
+1. Object Stores: see [Object Stores: root directory deletion](#object-stores-rm-root).
+
 HDFS has the notion of *Protected Directories*, which are declared in
 the option `fs.protected.directories`. Any attempt to delete such a directory
 or a parent thereof raises an `AccessControlException`. Accordingly, any
@@ -1019,6 +1024,23 @@ Any filesystem client which interacts with a remote filesystem which lacks
 such a security model, MAY reject calls to `delete("/", true)` on the basis
 that it makes it too easy to lose data.
 
+
+### <a name="object-stores-rm-root"></a> Object Stores: root directory deletion
+
+Some of the object store based filesystem implementations always return
+false when deleting the root, leaving the state of the store unchanged.
+
+    if isRoot(p) :
+        FS ' = FS
+        result = False
+
+This is irrespective of the recursive flag status or the state of the directory.
+
+This is a simplification which avoids the inevitably non-atomic scan and delete
+of the contents of the store. It also avoids any confusion about whether
+the operation actually deletes that specific store/container itself, and
+adverse consequences of the simpler permissions models of stores.
+
 ##### Recursive delete of non-root directory
 
 Deleting a non-root path with children `recursive==true`

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractRootDirectoryTest.java

@@ -189,7 +189,7 @@ public void testListEmptyRootDirectory() throws IOException {
     Path root = new Path("/");
     FileStatus[] statuses = fs.listStatus(root);
     for (FileStatus status : statuses) {
-      ContractTestUtils.assertDeleted(fs, status.getPath(), true);
+      ContractTestUtils.assertDeleted(fs, status.getPath(), false,true, false);
     }
     FileStatus[] rootListStatus = fs.listStatus(root);
     assertEquals("listStatus on empty root-directory returned found: "

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/ContractTestUtils.java

@@ -674,7 +674,8 @@ public static void touch(FileSystem fs,
   /**
    * Delete a file/dir and assert that delete() returned true
    * <i>and</i> that the path no longer exists. This variant rejects
-   * all operations on root directories.
+   * all operations on root directories and requires the target path
+   * to exist before the deletion operation.
    * @param fs filesystem
    * @param file path to delete
    * @param recursive flag to enable recursive delete
@@ -688,20 +689,41 @@ public static void assertDeleted(FileSystem fs,
 
   /**
    * Delete a file/dir and assert that delete() returned true
-   * <i>and</i> that the path no longer exists. This variant rejects
-   * all operations on root directories
+   * <i>and</i> that the path no longer exists.
+   * This variant requires the target path
+   * to exist before the deletion operation.
+   * @param fs filesystem
+   * @param file path to delete
+   * @param recursive flag to enable recursive delete
+   * @param allowRootOperations can the root dir be deleted?
+   * @throws IOException IO problems
+   */
+  public static void assertDeleted(FileSystem fs,
+      Path file,
+      boolean recursive,
+      boolean allowRootOperations) throws IOException {
+    assertDeleted(fs, file, true, recursive, allowRootOperations);
+  }
+
+  /**
+   * Delete a file/dir and assert that delete() returned true
+   * <i>and</i> that the path no longer exists.
    * @param fs filesystem
    * @param file path to delete
+   * @param requirePathToExist check for the path existing first?
    * @param recursive flag to enable recursive delete
    * @param allowRootOperations can the root dir be deleted?
    * @throws IOException IO problems
    */
   public static void assertDeleted(FileSystem fs,
       Path file,
+      boolean requirePathToExist,
       boolean recursive,
       boolean allowRootOperations) throws IOException {
     rejectRootOperation(file, allowRootOperations);
-    assertPathExists(fs, "about to be deleted file", file);
+    if (requirePathToExist) {
+      assertPathExists(fs, "about to be deleted file", file);
+    }
     boolean deleted = fs.delete(file, recursive);
     String dir = ls(fs, file.getParent());
     assertTrue("Delete failed on " + file + ": " + dir, deleted);

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

@@ -2307,30 +2307,16 @@ private boolean innerDelete(S3AFileStatus status, boolean recursive)
    * Implements the specific logic to reject root directory deletion.
    * The caller must return the result of this call, rather than
    * attempt to continue with the delete operation: deleting root
-   * directories is never allowed. This method simply implements
-   * the policy of when to return an exit code versus raise an exception.
+   * directories is never allowed.
    * @param status filesystem status
    * @param recursive recursive flag from command
    * @return a return code for the operation
-   * @throws PathIOException if the operation was explicitly rejected.
    */
   private boolean rejectRootDirectoryDelete(S3AFileStatus status,
-      boolean recursive) throws IOException {
-    LOG.info("s3a delete the {} root directory. Path: {}. Recursive: {}",
-        bucket, status.getPath(), recursive);
-    boolean emptyRoot = status.isEmptyDirectory() == Tristate.TRUE;
-    if (emptyRoot) {
-      return true;
-    }
-    if (recursive) {
-      LOG.error("Cannot delete root path: {}", status.getPath());
-      return false;
-    } else {
-      // reject
-      String msg = "Cannot delete root path: " + status.getPath();
-      LOG.error(msg);
-      throw new PathIOException(bucket, msg);
-    }
+      boolean recursive) {
+    LOG.error("S3A: Cannot delete the {} root directory. Path: {}. Recursive: "
+            + "{}", bucket, status.getPath(), recursive);
+    return false;
   }
 
   /**
@@ -2623,7 +2609,8 @@ S3AFileStatus innerGetFileStatus(final Path f,
     // Check MetadataStore, if any.
     PathMetadata pm = null;
     if (hasMetadataStore()) {
-      pm = S3Guard.getWithTtl(metadataStore, path, ttlTimeProvider);
+      pm = S3Guard.getWithTtl(metadataStore, path, ttlTimeProvider,
+          needEmptyDirectoryFlag);
     }
     Set<Path> tombstones = Collections.emptySet();
     if (pm != null) {

hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/s3guard/S3Guard.java

@@ -712,12 +712,15 @@ static void patchLastUpdated(
    * @param ms metastore
    * @param path path to look up.
    * @param timeProvider nullable time provider
+   * @param needEmptyDirectoryFlag if true, implementation will
+   * return known state of directory emptiness.
    * @return the metadata or null if there as no entry.
    * @throws IOException failure.
    */
   public static PathMetadata getWithTtl(MetadataStore ms, Path path,
-      @Nullable ITtlTimeProvider timeProvider) throws IOException {
-    final PathMetadata pathMetadata = ms.get(path);
+      @Nullable ITtlTimeProvider timeProvider,
+      final boolean needEmptyDirectoryFlag) throws IOException {
+    final PathMetadata pathMetadata = ms.get(path, needEmptyDirectoryFlag);
     // if timeProvider is null let's return with what the ms has
     if (timeProvider == null) {
       LOG.debug("timeProvider is null, returning pathMetadata as is");

hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/contract/s3a/ITestS3AContractRootDir.java

@@ -26,6 +26,7 @@
 import org.apache.hadoop.fs.contract.AbstractFSContract;
 import org.apache.hadoop.fs.s3a.S3AFileSystem;
 
+import org.junit.Ignore;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -62,16 +63,18 @@ public S3AFileSystem getFileSystem() {
     return (S3AFileSystem) super.getFileSystem();
   }
 
+  @Override
+  @Ignore("S3 always return false when non-recursively remove root dir")
+  public void testRmNonEmptyRootDirNonRecursive() throws Throwable {
+  }
+
   /**
    * This is overridden to allow for eventual consistency on listings,
    * but only if the store does not have S3Guard protecting it.
    */
   @Override
   public void testListEmptyRootDirectory() throws IOException {
     int maxAttempts = 10;
-    if (getFileSystem().hasMetadataStore()) {
-      maxAttempts = 1;
-    }
     describe("Listing root directory; for consistency allowing "
         + maxAttempts + " attempts");
     for (int attempt = 1; attempt <= maxAttempts; ++attempt) {