From 4e9d96a153ca8939c37ec3385329a7d7dd7e6579 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Mon, 27 Jul 2020 17:58:10 +0530 Subject: [PATCH 1/6] HDFS-15483. Ordered snapshot deletion: Disallow rename between two snapshottable directories. --- .../hdfs/server/namenode/FSDirRenameOp.java | 4 +- .../hdfs/server/namenode/FSDirSnapshotOp.java | 20 ++++ .../namenode/snapshot/SnapshotManager.java | 37 ++++--- ...TestRenameWithOrderedSnapshotDeletion.java | 96 +++++++++++++++++++ 4 files changed, 145 insertions(+), 12 deletions(-) create mode 100644 hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java 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 a0c1e3a8814e3..ecc5483c4dcb3 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 @@ -33,6 +33,7 @@ import org.apache.hadoop.hdfs.server.namenode.FSDirectory.DirOp; import org.apache.hadoop.hdfs.server.namenode.INode.BlocksMapUpdateInfo; import org.apache.hadoop.hdfs.server.namenode.snapshot.Snapshot; +import org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotManager; import org.apache.hadoop.hdfs.util.ReadOnlyList; import org.apache.hadoop.util.ChunkedArrayList; import org.apache.hadoop.util.Time; @@ -193,7 +194,7 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd, } validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs); - + FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP); fsd.ezManager.checkMoveValidity(srcIIP, dstIIP); // Ensure dst has quota to accommodate rename verifyFsLimitsForRename(fsd, srcIIP, dstIIP); @@ -407,6 +408,7 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd, validateNestSnapshot(fsd, src, dstParent.asDirectory(), srcSnapshottableDirs); + FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP); // Ensure dst has quota to accommodate rename verifyFsLimitsForRename(fsd, srcIIP, dstIIP); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java index f264dc34063f1..9d3d4aa2e753d 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java @@ -358,4 +358,24 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip, checkSnapshot(iip.getLastINode(), snapshottableDirs); } } + + static void checkUnderSameSnapshottableRoot(FSDirectory fsd, + INodesInPath srcIIP, INodesInPath dstIIP) throws IOException { + // Ensure rename out of a snapshottable root is not permitted if ordered + // snapshot deletion feature is enabled + if (fsd.isSnapshotDeletionOrdered()) { + SnapshotManager snapshotManager = fsd.getFSNamesystem(). + getSnapshotManager(); + String errMsg = "Source " + srcIIP.getPath() + + " and dest " + dstIIP.getPath() + " are not under " + + "the same snapshot root."; + INodeDirectory src = snapshotManager. + getSnapshottableAncestorDir(srcIIP); + INodeDirectory dst = snapshotManager.getSnapshottableAncestorDir(dstIIP); + if (!(dstIIP.isDescendant(snapshotManager. + getSnapshottableAncestorDir(srcIIP)))) { + throw new SnapshotException(errMsg); + } + } + } } 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 7569fc64e6666..a7661bc726d7e 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 @@ -363,23 +363,38 @@ void assertFirstSnapshot(INodeDirectory dir, * @param iip INodesInPath for the directory to get snapshot root. * @return the snapshot root INodeDirectory */ + public INodeDirectory checkAndGetSnapshottableAncestorDir( + final INodesInPath iip) throws IOException { + final INodeDirectory dir = getSnapshottableAncestorDir(iip); + if (dir == null) { + throw new SnapshotException("Directory is neither snapshottable nor" + + " under a snap root!"); + } + return dir; + } + public INodeDirectory getSnapshottableAncestorDir(final INodesInPath iip) throws IOException { final String path = iip.getPath(); - final INodeDirectory dir = INodeDirectory.valueOf(iip.getLastINode(), path); - if (dir.isSnapshottable()) { - return dir; + final INode inode = iip.getLastINode(); + final INodeDirectory dir; + if (inode instanceof INodeDirectory) { + dir = INodeDirectory.valueOf(inode, path); + if (dir.isSnapshottable()) { + return dir; + } } else { - for (INodeDirectory snapRoot : this.snapshottables.values()) { - if (dir.isAncestorDirectory(snapRoot)) { - return snapRoot; - } + dir = INodeDirectory.valueOf(iip.getINode(-2), iip.getParentPath()); + } + for (INodeDirectory snapRoot : this.snapshottables.values()) { + if (dir.isAncestorDirectory(snapRoot)) { + return snapRoot; } - throw new SnapshotException("Directory is neither snapshottable nor" + - " under a snap root!"); } + return null; } + public boolean isDescendantOfSnapshotRoot(INodeDirectory dir) { if (dir.isSnapshottable()) { return true; @@ -641,7 +656,7 @@ public SnapshotDiffReport diff(final INodesInPath iip, // All the check for path has been included in the valueOf method. INodeDirectory snapshotRootDir; if (this.snapshotDiffAllowSnapRootDescendant) { - snapshotRootDir = getSnapshottableAncestorDir(iip); + snapshotRootDir = checkAndGetSnapshottableAncestorDir(iip); } else { snapshotRootDir = getSnapshottableRoot(iip); } @@ -674,7 +689,7 @@ public SnapshotDiffReportListing diff(final INodesInPath iip, // All the check for path has been included in the valueOf method. INodeDirectory snapshotRootDir; if (this.snapshotDiffAllowSnapRootDescendant) { - snapshotRootDir = getSnapshottableAncestorDir(iip); + snapshotRootDir = checkAndGetSnapshottableAncestorDir(iip); } else { snapshotRootDir = getSnapshottableRoot(iip); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java new file mode 100644 index 0000000000000..1c1259e8c13eb --- /dev/null +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hdfs.DistributedFileSystem; +import org.apache.hadoop.hdfs.MiniDFSCluster; +import org.apache.hadoop.hdfs.DFSTestUtil; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + + +import java.io.IOException; + +import static org.apache.hadoop.hdfs.DFSConfigKeys. + DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED; + +/** + * Test Rename with ordered snapshot deletion. + */ +public class TestRenameWithOrderedSnapshotDeletion { + private final Path snapshottableDir + = new Path("/" + getClass().getSimpleName()); + private DistributedFileSystem hdfs; + private MiniDFSCluster cluster; + + @Before + public void setUp() throws Exception { + final Configuration conf = new Configuration(); + conf.setBoolean(DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED, true); + + cluster = new MiniDFSCluster.Builder(conf).numDataNodes(0).build(); + cluster.waitActive(); + hdfs = cluster.getFileSystem(); + } + + @After + public void tearDown() throws Exception { + if (cluster != null) { + cluster.shutdown(); + cluster = null; + } + } + + @Test(timeout = 60000) + public void testRename() throws Exception { + final Path dir1 = new Path("/dir1"); + final Path dir2 = new Path("/dir2"); + final Path sub0 = new Path(snapshottableDir, "sub0"); + hdfs.mkdirs(sub0); + final Path file1 = new Path(dir1, "file1"); + final Path file2 = new Path(sub0, "file2"); + hdfs.mkdirs(snapshottableDir); + hdfs.mkdirs(dir1); + hdfs.mkdirs(dir2); + hdfs.mkdirs(sub0); + DFSTestUtil.createFile(hdfs, file1, 0, (short) 1, 0); + DFSTestUtil.createFile(hdfs, file2, 0, (short) 1, 0); + hdfs.allowSnapshot(snapshottableDir); + // rename from non snapshottable dir to snapshottable dir should fail + validateRename(file1, sub0); + hdfs.createSnapshot(snapshottableDir, "s0"); + validateRename(file1, sub0); + // rename across non snapshottable dirs should work + hdfs.rename(file1, dir2); + // rename beyond snapshottable root should fail + validateRename(file2, dir1); + // rename within snapshottable root should work + hdfs.rename(file2, snapshottableDir); + } + + private void validateRename(Path src, Path dest) { + try { + hdfs.rename(src, dest); + } catch (IOException ioe) { + ioe.getMessage().contains("are not under the same snapshot root."); + } + } + } From 6d5c151b684d2ed78d21fb0ce9a21cff62882d3b Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Wed, 29 Jul 2020 12:58:04 +0530 Subject: [PATCH 2/6] Addressed review comments. --- .../hdfs/server/namenode/FSDirRenameOp.java | 1 - .../hdfs/server/namenode/FSDirSnapshotOp.java | 4 +--- .../TestRenameWithOrderedSnapshotDeletion.java | 16 ++++++++++++++-- 3 files changed, 15 insertions(+), 6 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 ecc5483c4dcb3..d3080912931ad 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 @@ -33,7 +33,6 @@ import org.apache.hadoop.hdfs.server.namenode.FSDirectory.DirOp; import org.apache.hadoop.hdfs.server.namenode.INode.BlocksMapUpdateInfo; import org.apache.hadoop.hdfs.server.namenode.snapshot.Snapshot; -import org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotManager; import org.apache.hadoop.hdfs.util.ReadOnlyList; import org.apache.hadoop.util.ChunkedArrayList; import org.apache.hadoop.util.Time; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java index 9d3d4aa2e753d..043e85bf1e742 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java @@ -371,9 +371,7 @@ static void checkUnderSameSnapshottableRoot(FSDirectory fsd, "the same snapshot root."; INodeDirectory src = snapshotManager. getSnapshottableAncestorDir(srcIIP); - INodeDirectory dst = snapshotManager.getSnapshottableAncestorDir(dstIIP); - if (!(dstIIP.isDescendant(snapshotManager. - getSnapshottableAncestorDir(srcIIP)))) { + if (!(dstIIP.isDescendant(src))) { throw new SnapshotException(errMsg); } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java index 1c1259e8c13eb..ecbe9efc2662c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java @@ -23,6 +23,7 @@ import org.apache.hadoop.hdfs.MiniDFSCluster; import org.apache.hadoop.hdfs.DFSTestUtil; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -64,6 +65,7 @@ public void testRename() throws Exception { final Path dir1 = new Path("/dir1"); final Path dir2 = new Path("/dir2"); final Path sub0 = new Path(snapshottableDir, "sub0"); + final Path sub1 = new Path(snapshottableDir, "sub1"); hdfs.mkdirs(sub0); final Path file1 = new Path(dir1, "file1"); final Path file2 = new Path(sub0, "file2"); @@ -84,13 +86,23 @@ public void testRename() throws Exception { validateRename(file2, dir1); // rename within snapshottable root should work hdfs.rename(file2, snapshottableDir); + + // rename dirs outside snapshottable root should work + hdfs.rename(dir2, dir1); + // rename dir into snapshottable root should fail + validateRename(dir2, snapshottableDir); + // rename dir outside snapshottable root should fail + validateRename(sub0, dir2); + // rename dir within snapshottable root should work + hdfs.rename(sub0, sub1); } private void validateRename(Path src, Path dest) { try { hdfs.rename(src, dest); } catch (IOException ioe) { - ioe.getMessage().contains("are not under the same snapshot root."); + Assert.assertTrue(ioe.getMessage().contains("are not under the" + + " same snapshot root.")); } } - } +} From 70f6603fe0cc4a6081622dee40ca50fa4d7f0a48 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Wed, 12 Aug 2020 11:34:08 +0530 Subject: [PATCH 3/6] Rebased to latest trunk and added snapshotTrashRoot config check. --- .../hdfs/server/namenode/FSDirSnapshotOp.java | 7 ++++--- .../hdfs/server/namenode/FSNamesystem.java | 16 ++++++++++++---- .../TestRenameWithOrderedSnapshotDeletion.java | 8 ++++---- 3 files changed, 20 insertions(+), 11 deletions(-) rename hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/{ => snapshot}/TestRenameWithOrderedSnapshotDeletion.java (91%) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java index 043e85bf1e742..8dc6be26354c0 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java @@ -363,9 +363,10 @@ static void checkUnderSameSnapshottableRoot(FSDirectory fsd, INodesInPath srcIIP, INodesInPath dstIIP) throws IOException { // Ensure rename out of a snapshottable root is not permitted if ordered // snapshot deletion feature is enabled - if (fsd.isSnapshotDeletionOrdered()) { - SnapshotManager snapshotManager = fsd.getFSNamesystem(). - getSnapshotManager(); + SnapshotManager snapshotManager = fsd.getFSNamesystem(). + getSnapshotManager(); + if (snapshotManager.isSnapshotDeletionOrdered() && fsd.getFSNamesystem() + .isSnapshotTrashRootEnabled()) { String errMsg = "Source " + srcIIP.getPath() + " and dest " + dstIIP.getPath() + " are not under " + "the same snapshot root."; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index dd590c6d80b4f..9f588de1d6a59 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -384,9 +384,10 @@ public class FSNamesystem implements Namesystem, FSNamesystemMBean, .getLogger(FSNamesystem.class.getName()); // The following are private configurations - static final String DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED = + public static final String DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED = "dfs.namenode.snapshot.trashroot.enabled"; - static final boolean DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED_DEFAULT = false; + public static final boolean DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED_DEFAULT + = false; private final MetricsRegistry registry = new MetricsRegistry("FSNamesystem"); @Metric final MutableRatesWithAggregation detailedLockHoldTimeMetrics = @@ -468,6 +469,7 @@ private void logAuditEvent(boolean succeeded, private final UserGroupInformation fsOwner; private final String supergroup; private final boolean standbyShouldCheckpoint; + private final boolean isSnapshotTrashRootEnabled; private final int snapshotDiffReportLimit; private final int blockDeletionIncrement; @@ -882,6 +884,9 @@ static FSNamesystem loadFromDisk(Configuration conf) throws IOException { // Get the checksum type from config String checksumTypeStr = conf.get(DFS_CHECKSUM_TYPE_KEY, DFS_CHECKSUM_TYPE_DEFAULT); + this.isSnapshotTrashRootEnabled = conf.getBoolean( + DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED, + DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED_DEFAULT); DataChecksum.Type checksumType; try { checksumType = DataChecksum.Type.valueOf(checksumTypeStr); @@ -909,8 +914,7 @@ static FSNamesystem loadFromDisk(Configuration conf) throws IOException { CommonConfigurationKeysPublic.HADOOP_SECURITY_KEY_PROVIDER_PATH, ""), blockManager.getStoragePolicySuite().getDefaultPolicy().getId(), - conf.getBoolean(DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED, - DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED_DEFAULT)); + isSnapshotTrashRootEnabled); this.maxFsObjects = conf.getLong(DFS_NAMENODE_MAX_OBJECTS_KEY, DFS_NAMENODE_MAX_OBJECTS_DEFAULT); @@ -1054,6 +1058,10 @@ public int getMaxListOpenFilesResponses() { return maxListOpenFilesResponses; } + boolean isSnapshotTrashRootEnabled() { + return isSnapshotTrashRootEnabled; + } + void lockRetryCache() { if (retryCache != null) { retryCache.lock(); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithOrderedSnapshotDeletion.java similarity index 91% rename from hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java rename to hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithOrderedSnapshotDeletion.java index ecbe9efc2662c..9475bd60c8cf2 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestRenameWithOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithOrderedSnapshotDeletion.java @@ -15,7 +15,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.hadoop.hdfs.server.namenode; +package org.apache.hadoop.hdfs.server.namenode.snapshot; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; @@ -30,9 +30,8 @@ import java.io.IOException; -import static org.apache.hadoop.hdfs.DFSConfigKeys. - DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED; - +import static org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotManager.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED; +import static org.apache.hadoop.hdfs.server.namenode.FSNamesystem.DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED; /** * Test Rename with ordered snapshot deletion. */ @@ -46,6 +45,7 @@ public class TestRenameWithOrderedSnapshotDeletion { public void setUp() throws Exception { final Configuration conf = new Configuration(); conf.setBoolean(DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED, true); + conf.setBoolean(DFS_NAMENODE_SNAPSHOT_TRASHROOT_ENABLED, true); cluster = new MiniDFSCluster.Builder(conf).numDataNodes(0).build(); cluster.waitActive(); From 279e772f2736a6beac1ef034bfaa4e6dbb329855 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Fri, 14 Aug 2020 11:17:32 +0530 Subject: [PATCH 4/6] Addressed review comments. --- .../hdfs/server/namenode/FSDirRenameOp.java | 28 +++++++++++++++++-- .../hdfs/server/namenode/FSDirSnapshotOp.java | 19 ------------- .../namenode/snapshot/SnapshotManager.java | 6 ++-- ...TestRenameWithOrderedSnapshotDeletion.java | 4 ++- 4 files changed, 32 insertions(+), 25 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 d3080912931ad..7396519e90af2 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 @@ -33,6 +33,7 @@ import org.apache.hadoop.hdfs.server.namenode.FSDirectory.DirOp; import org.apache.hadoop.hdfs.server.namenode.INode.BlocksMapUpdateInfo; import org.apache.hadoop.hdfs.server.namenode.snapshot.Snapshot; +import org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotManager; import org.apache.hadoop.hdfs.util.ReadOnlyList; import org.apache.hadoop.util.ChunkedArrayList; import org.apache.hadoop.util.Time; @@ -193,7 +194,7 @@ static INodesInPath unprotectedRenameTo(FSDirectory fsd, } validateNestSnapshot(fsd, src, dstParent.asDirectory(), snapshottableDirs); - FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP); + checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP); fsd.ezManager.checkMoveValidity(srcIIP, dstIIP); // Ensure dst has quota to accommodate rename verifyFsLimitsForRename(fsd, srcIIP, dstIIP); @@ -407,7 +408,7 @@ static RenameResult unprotectedRenameTo(FSDirectory fsd, validateNestSnapshot(fsd, src, dstParent.asDirectory(), srcSnapshottableDirs); - FSDirSnapshotOp.checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP); + checkUnderSameSnapshottableRoot(fsd, srcIIP, dstIIP); // Ensure dst has quota to accommodate rename verifyFsLimitsForRename(fsd, srcIIP, dstIIP); @@ -822,6 +823,29 @@ void updateQuotasInSourceTree(BlockStoragePolicySuite bsps) { } } + private static void checkUnderSameSnapshottableRoot( + FSDirectory fsd, INodesInPath srcIIP, INodesInPath dstIIP) + throws IOException { + // Ensure rename out of a snapshottable root is not permitted if ordered + // snapshot deletion feature is enabled + SnapshotManager snapshotManager = fsd.getFSNamesystem(). + getSnapshotManager(); + if (snapshotManager.isSnapshotDeletionOrdered() && fsd.getFSNamesystem() + .isSnapshotTrashRootEnabled()) { + INodeDirectory srcRoot = snapshotManager. + getSnapshottableAncestorDir(srcIIP); + INodeDirectory dstRoot = snapshotManager. + getSnapshottableAncestorDir(dstIIP); + // Ensure snapshoottable root for both src and dest are same. + if (srcRoot != dstRoot) { + String errMsg = "Source " + srcIIP.getPath() + + " and dest " + dstIIP.getPath() + " are not under " + + "the same snapshot root."; + throw new SnapshotException(errMsg); + } + } + } + private static RenameResult createRenameResult(FSDirectory fsd, INodesInPath dst, boolean filesDeleted, BlocksMapUpdateInfo collectedBlocks) throws IOException { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java index 8dc6be26354c0..f264dc34063f1 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirSnapshotOp.java @@ -358,23 +358,4 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip, checkSnapshot(iip.getLastINode(), snapshottableDirs); } } - - static void checkUnderSameSnapshottableRoot(FSDirectory fsd, - INodesInPath srcIIP, INodesInPath dstIIP) throws IOException { - // Ensure rename out of a snapshottable root is not permitted if ordered - // snapshot deletion feature is enabled - SnapshotManager snapshotManager = fsd.getFSNamesystem(). - getSnapshotManager(); - if (snapshotManager.isSnapshotDeletionOrdered() && fsd.getFSNamesystem() - .isSnapshotTrashRootEnabled()) { - String errMsg = "Source " + srcIIP.getPath() + - " and dest " + dstIIP.getPath() + " are not under " + - "the same snapshot root."; - INodeDirectory src = snapshotManager. - getSnapshottableAncestorDir(srcIIP); - if (!(dstIIP.isDescendant(src))) { - throw new SnapshotException(errMsg); - } - } - } } 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 a7661bc726d7e..b786a75581bec 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 @@ -380,12 +380,12 @@ public INodeDirectory getSnapshottableAncestorDir(final INodesInPath iip) final INodeDirectory dir; if (inode instanceof INodeDirectory) { dir = INodeDirectory.valueOf(inode, path); - if (dir.isSnapshottable()) { - return dir; - } } else { dir = INodeDirectory.valueOf(iip.getINode(-2), iip.getParentPath()); } + if (dir.isSnapshottable()) { + return dir; + } for (INodeDirectory snapRoot : this.snapshottables.values()) { if (dir.isAncestorDirectory(snapRoot)) { return snapRoot; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithOrderedSnapshotDeletion.java index 9475bd60c8cf2..052610baec873 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestRenameWithOrderedSnapshotDeletion.java @@ -67,6 +67,7 @@ public void testRename() throws Exception { final Path sub0 = new Path(snapshottableDir, "sub0"); final Path sub1 = new Path(snapshottableDir, "sub1"); hdfs.mkdirs(sub0); + hdfs.mkdirs(dir2); final Path file1 = new Path(dir1, "file1"); final Path file2 = new Path(sub0, "file2"); hdfs.mkdirs(snapshottableDir); @@ -90,7 +91,7 @@ public void testRename() throws Exception { // rename dirs outside snapshottable root should work hdfs.rename(dir2, dir1); // rename dir into snapshottable root should fail - validateRename(dir2, snapshottableDir); + validateRename(dir1, snapshottableDir); // rename dir outside snapshottable root should fail validateRename(sub0, dir2); // rename dir within snapshottable root should work @@ -100,6 +101,7 @@ public void testRename() throws Exception { private void validateRename(Path src, Path dest) { try { hdfs.rename(src, dest); + Assert.fail("Expected exception not thrown."); } catch (IOException ioe) { Assert.assertTrue(ioe.getMessage().contains("are not under the" + " same snapshot root.")); From edee22e378985607bcc813b388f1c80f18b996a9 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Fri, 14 Aug 2020 11:20:23 +0530 Subject: [PATCH 5/6] Removede empty line added in SnapshotManager.Java. --- .../hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 b786a75581bec..3e11889182b6d 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 @@ -393,8 +393,7 @@ public INodeDirectory getSnapshottableAncestorDir(final INodesInPath iip) } return null; } - - + public boolean isDescendantOfSnapshotRoot(INodeDirectory dir) { if (dir.isSnapshottable()) { return true; From 8444f5deac11b5880bf5d73c5e9f5f72051e48f0 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Fri, 14 Aug 2020 20:57:47 +0530 Subject: [PATCH 6/6] Addressed whitespace issues. --- .../hadoop/hdfs/server/namenode/snapshot/SnapshotManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3e11889182b6d..83cf32af34fad 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 @@ -393,7 +393,7 @@ public INodeDirectory getSnapshottableAncestorDir(final INodesInPath iip) } return null; } - + public boolean isDescendantOfSnapshotRoot(INodeDirectory dir) { if (dir.isSnapshottable()) { return true;