From de6b8b0c0b1933aab2af3e8adc50a2091d428238 Mon Sep 17 00:00:00 2001 From: Zhao Junwang Date: Sat, 2 Nov 2019 07:37:53 +0800 Subject: [PATCH] HDFS-14925. Rename operation should check nest snapshot (#1670) If the src directory or any of its descendant is snapshottable and the dst directory or any of its ancestors is snapshottable, we consider this as nested snapshot, which should be denied. Reviewed-by: Shashikant Banerjee --- .../hdfs/server/namenode/FSDirRenameOp.java | 57 +++++++++++++++---- .../namenode/snapshot/SnapshotManager.java | 17 ++++++ .../snapshot/TestRenameWithSnapshots.java | 40 ++++++++++++- 3 files changed, 101 insertions(+), 13 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java index 6ca6568ea2..af156e9a50 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java +++ b/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 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 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 snapshottableDirs = new ArrayList<>(); + List 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 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 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 { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java index 1786346695..b908ee93af 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java +++ b/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. diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java index b8898eb3e8..f5b5345bf8 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithSnapshots.java +++ b/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 */