HDFS-14925. Rename operation should check nest snapshot

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java

@@ -143,8 +143,8 @@ private static INodesInPath dstForRenameTo(
    * Change a path name
    *
    * @param fsd FSDirectory
-   * @param src source path
-   * @param dst destination path
+   * @param srcIIP source path
+   * @param dstIIP destination path
    * @return true INodesInPath if rename succeeds; null otherwise
    * @deprecated See {@link #renameToInt(FSDirectory, String, String,
    * boolean, Options.Rename...)}
@@ -155,8 +155,9 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
       throws IOException {
     assert fsd.hasWriteLock();
     final INode srcInode = srcIIP.getLastINode();
+    List<INodeDirectory> snapshottableDirs = new ArrayList<>();
     try {
-      validateRenameSource(fsd, srcIIP);
+      validateRenameSource(fsd, srcIIP, snapshottableDirs);
     } catch (SnapshotException e) {
       throw e;
     } catch (IOException ignored) {
@@ -190,6 +191,8 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd,
       return null;
     }
 
+    validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs);
+
     fsd.ezManager.checkMoveValidity(srcIIP, dstIIP);
     // Ensure dst has quota to accommodate rename
     verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
@@ -342,8 +345,8 @@ static void renameForEditLog(
    * for details related to rename semantics and exceptions.
    *
    * @param fsd             FSDirectory
-   * @param src             source path
-   * @param dst             destination path
+   * @param srcIIP          source path
+   * @param dstIIP          destination path
    * @param timestamp       modification time
    * @param collectedBlocks blocks to be removed
    * @param options         Rename options
@@ -361,7 +364,8 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
     final String dst = dstIIP.getPath();
     final String error;
     final INode srcInode = srcIIP.getLastINode();
-    validateRenameSource(fsd, srcIIP);
+    List<INodeDirectory> srcSnapshottableDirs = new ArrayList<>();
+    validateRenameSource(fsd, srcIIP, srcSnapshottableDirs);
 
     // validate the destination
     if (dst.equals(src)) {
@@ -380,10 +384,10 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
     BlockStoragePolicySuite bsps = fsd.getBlockStoragePolicySuite();
     fsd.ezManager.checkMoveValidity(srcIIP, dstIIP);
     final INode dstInode = dstIIP.getLastINode();
-    List<INodeDirectory> snapshottableDirs = new ArrayList<>();
+    List<INodeDirectory> dstSnapshottableDirs = new ArrayList<>();
     if (dstInode != null) { // Destination exists
       validateOverwrite(src, dst, overwrite, srcInode, dstInode);
-      FSDirSnapshotOp.checkSnapshot(fsd, dstIIP, snapshottableDirs);
+      FSDirSnapshotOp.checkSnapshot(fsd, dstIIP, dstSnapshottableDirs);
     }
 
     INode dstParent = dstIIP.getINode(-2);
@@ -400,6 +404,9 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
       throw new ParentNotDirectoryException(error);
     }
 
+    validateNestSnapshot(fsd, src,
+            dstParent.asDirectory(), srcSnapshottableDirs);
+
     // Ensure dst has quota to accommodate rename
     verifyFsLimitsForRename(fsd, srcIIP, dstIIP);
     verifyQuotaForRename(fsd, srcIIP, dstIIP);
@@ -439,10 +446,10 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd,
           }
         }
 
-        if (snapshottableDirs.size() > 0) {
+        if (dstSnapshottableDirs.size() > 0) {
           // There are snapshottable directories (without snapshots) to be
           // deleted. Need to update the SnapshotManager.
-          fsd.getFSNamesystem().removeSnapshottableDirs(snapshottableDirs);
+          fsd.getFSNamesystem().removeSnapshottableDirs(dstSnapshottableDirs);
         }
 
         tx.updateQuotasInSourceTree(bsps);
@@ -556,7 +563,8 @@ private static void validateOverwrite(
   }
 
   private static void validateRenameSource(FSDirectory fsd,
-      INodesInPath srcIIP) throws IOException {
+      INodesInPath srcIIP, List<INodeDirectory> snapshottableDirs)
+      throws IOException {
     String error;
     final INode srcInode = srcIIP.getLastINode();
     // validate source
@@ -574,7 +582,32 @@ private static void validateRenameSource(FSDirectory fsd,
     }
     // srcInode and its subtree cannot contain snapshottable directories with
     // snapshots
-    FSDirSnapshotOp.checkSnapshot(fsd, srcIIP, null);
+    FSDirSnapshotOp.checkSnapshot(fsd, srcIIP, snapshottableDirs);
+  }
+
+  private static void validateNestSnapshot(FSDirectory fsd, String srcPath,
+      INodeDirectory dstParent, List<INodeDirectory> snapshottableDirs)
+      throws SnapshotException {
+
+    if (fsd.getFSNamesystem().getSnapshotManager().isAllowNestedSnapshots()) {
+      return;
+    }
+
+    /*
+     * snapshottableDirs is a list of snapshottable directory (child of rename
+     * src) which do not have snapshots yet. If this list is not empty, that
+     * means rename src has snapshottable descendant directories.
+     */
+    if (snapshottableDirs != null && snapshottableDirs.size() > 0) {
+      if (fsd.getFSNamesystem().getSnapshotManager()
+              .isDescendantOfSnapshotRoot(dstParent)) {
+        String dstPath = dstParent.getFullPathName();
+        throw new SnapshotException("Unable to rename because " + srcPath
+                + " has snapshottable descendant directories and " + dstPath
+                + " is a descent of a snapshottable directory, and HDFS does"
+                + " not support nested snapshottable directory.");
+      }
+    }
   }
 
   private static class RenameOperation {

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java

@@ -155,6 +155,10 @@ void setAllowNestedSnapshots(boolean allowNestedSnapshots) {
     this.allowNestedSnapshots = allowNestedSnapshots;
   }
 
+  public boolean isAllowNestedSnapshots() {
+    return allowNestedSnapshots;
+  }
+
   private void checkNestedSnapshottable(INodeDirectory dir, String path)
       throws SnapshotException {
     if (allowNestedSnapshots) {
@@ -288,6 +292,19 @@ public INodeDirectory getSnapshottableAncestorDir(final INodesInPath iip)
     }
   }
 
+  public boolean isDescendantOfSnapshotRoot(INodeDirectory dir) {
+    if (dir.isSnapshottable()) {
+      return true;
+    } else {
+      for (INodeDirectory p = dir; p != null; p = p.getParent()) {
+        if (this.snapshottables.containsValue(p)) {
+          return true;
+        }
+      }
+      return false;
+    }
+  }
+
   /**
    * Create a snapshot of the given path.
    * It is assumed that the caller will perform synchronization.

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java

@@ -1115,7 +1115,45 @@ public void testRenameAndUpdateSnapshottableDirs() throws Exception {
     assertEquals(bar, dirs[0].getFullPath());
     assertEquals(fooId, dirs[0].getDirStatus().getFileId());
   }
-
+
+  /**
+   * Test rename where src has snapshottable descendant directories and
+   * dst is a descent of a snapshottable directory. Such case will cause
+   * nested snapshot which HDFS currently not fully supported.
+   */
+  @Test
+  public void testRenameWithNestedSnapshottableDirs() throws Exception {
+    final Path sdir1 = new Path("/dir1");
+    final Path sdir2 = new Path("/dir2");
+    final Path foo = new Path(sdir1, "foo");
+    final Path bar = new Path(sdir2, "bar");
+
+    hdfs.mkdirs(foo);
+    hdfs.mkdirs(bar);
+
+    hdfs.allowSnapshot(foo);
+    hdfs.allowSnapshot(sdir2);
+
+    try {
+      hdfs.rename(foo, bar, Rename.OVERWRITE);
+      fail("Except exception since " + "Unable to rename because "
+          + foo.toString() + " has snapshottable descendant directories and "
+          + sdir2.toString() + " is a descent of a snapshottable directory, "
+          + "and HDFS does not support nested snapshottable directory.");
+    } catch (IOException e) {
+      GenericTestUtils.assertExceptionContains("Unable to rename because "
+            + foo.toString() + " has snapshottable descendant directories and "
+            + sdir2.toString() + " is a descent of a snapshottable directory, "
+            + "and HDFS does not support nested snapshottable directory.", e);
+    }
+
+    hdfs.disallowSnapshot(foo);
+    hdfs.rename(foo, bar, Rename.OVERWRITE);
+    SnapshottableDirectoryStatus[] dirs = fsn.getSnapshottableDirListing();
+    assertEquals(1, dirs.length);
+    assertEquals(sdir2, dirs[0].getFullPath());
+  }
+
   /**
    * After rename, delete the snapshot in src
    */