From e072d33327b8f5d38b74a15e279d492ad379a47c Mon Sep 17 00:00:00 2001 From: bshashikant Date: Tue, 4 Aug 2020 14:10:29 +0530 Subject: [PATCH] HDFS-15497. Make snapshot limit on global as well per snapshot root directory configurable (#2175) --- .../org/apache/hadoop/hdfs/DFSConfigKeys.java | 6 +++ .../namenode/snapshot/SnapshotManager.java | 30 +++++++++-- .../src/main/resources/hdfs-default.xml | 9 ++++ .../snapshot/TestSnapshotManager.java | 53 ++++++++++++++----- 4 files changed, 82 insertions(+), 16 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java index 3865f9b4d7..d1c32f122e 100755 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java @@ -501,6 +501,12 @@ public class DFSConfigKeys extends CommonConfigurationKeys { public static final String DFS_NAMENODE_SNAPSHOT_MAX_LIMIT = "dfs.namenode.snapshot.max.limit"; public static final int DFS_NAMENODE_SNAPSHOT_MAX_LIMIT_DEFAULT = 65536; + public static final String + DFS_NAMENODE_SNAPSHOT_FILESYSTEM_LIMIT = + "dfs.namenode.snapshot.filesystem.limit"; + // default value is same as snapshot quota set for a snapshottable directory + public static final int + DFS_NAMENODE_SNAPSHOT_FILESYSTEM_LIMIT_DEFAULT = 65536; public static final String DFS_NAMENODE_SNAPSHOT_SKIPLIST_SKIP_INTERVAL = "dfs.namenode.snapshot.skiplist.interval"; 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 d56611235a..7569fc64e6 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 @@ -120,12 +120,14 @@ public class SnapshotManager implements SnapshotStatsMXBean { private final boolean snapshotDeletionOrdered; private int snapshotCounter = 0; private final int maxSnapshotLimit; + private final int maxSnapshotFSLimit; /** All snapshottable directories in the namesystem. */ private final Map snapshottables = new ConcurrentHashMap<>(); - public SnapshotManager(final Configuration conf, final FSDirectory fsdir) { + public SnapshotManager(final Configuration conf, final FSDirectory fsdir) + throws SnapshotException { this.fsdir = fsdir; this.captureOpenFiles = conf.getBoolean( DFS_NAMENODE_SNAPSHOT_CAPTURE_OPENFILES, @@ -138,13 +140,20 @@ public SnapshotManager(final Configuration conf, final FSDirectory fsdir) { DFSConfigKeys. DFS_NAMENODE_SNAPSHOT_DIFF_ALLOW_SNAP_ROOT_DESCENDANT_DEFAULT); this.maxSnapshotLimit = conf.getInt( - DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_MAX_LIMIT, - DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_MAX_LIMIT_DEFAULT); + DFSConfigKeys. + DFS_NAMENODE_SNAPSHOT_MAX_LIMIT, + DFSConfigKeys. + DFS_NAMENODE_SNAPSHOT_MAX_LIMIT_DEFAULT); + this.maxSnapshotFSLimit = conf.getInt( + DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_FILESYSTEM_LIMIT, + DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_FILESYSTEM_LIMIT_DEFAULT); LOG.info("Loaded config captureOpenFiles: " + captureOpenFiles + ", skipCaptureAccessTimeOnlyChange: " + skipCaptureAccessTimeOnlyChange + ", snapshotDiffAllowSnapRootDescendant: " + snapshotDiffAllowSnapRootDescendant + + ", maxSnapshotFSLimit: " + + maxSnapshotFSLimit + ", maxSnapshotLimit: " + maxSnapshotLimit); @@ -160,6 +169,13 @@ public SnapshotManager(final Configuration conf, final FSDirectory fsdir) { final int skipInterval = conf.getInt( DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_SKIPLIST_SKIP_INTERVAL, DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_SKIPLIST_SKIP_INTERVAL_DEFAULT); + if (maxSnapshotLimit > maxSnapshotFSLimit) { + final String errMsg = DFSConfigKeys. + DFS_NAMENODE_SNAPSHOT_MAX_LIMIT + + " cannot be greater than " + + DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_FILESYSTEM_LIMIT; + throw new SnapshotException(errMsg); + } DirectoryDiffListFactory.init(skipInterval, maxLevels, LOG); } @@ -405,6 +421,14 @@ public String createSnapshot(final LeaseManager leaseManager, "Failed to create the snapshot. The FileSystem has run out of " + "snapshot IDs and ID rollover is not supported."); } + int n = numSnapshots.get(); + if (n >= maxSnapshotFSLimit) { + // We have reached the maximum snapshot limit + throw new SnapshotException( + "Failed to create snapshot: there are already " + (n + 1) + + " snapshot(s) and the max snapshot limit is " + + maxSnapshotFSLimit); + } srcRoot.addSnapshot(snapshotCounter, snapshotName, leaseManager, this.captureOpenFiles, maxSnapshotLimit, mtime); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml b/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml index 689ecfe2f3..db152e60bd 100755 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml @@ -5101,6 +5101,15 @@ for maximum no of snapshots allowed is 65536. + + dfs.namenode.snapshot.filesystem.limit + 65536 + + Limits the maximum number of snapshots allowed on the entire filesystem. + If the configuration is not set, the default limit + for maximum no of snapshots allowed is 65536. + + dfs.namenode.snapshot.skiplist.max.levels diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotManager.java index 4d6e6f4172..b1e91686b3 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotManager.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestSnapshotManager.java @@ -24,6 +24,7 @@ import static org.mockito.Mockito.spy; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.protocol.SnapshotException; import org.apache.hadoop.hdfs.server.namenode.FSDirectory; import org.apache.hadoop.hdfs.server.namenode.INode; @@ -35,32 +36,54 @@ import org.junit.Assert; import org.junit.Test; +import java.io.IOException; + /** * Testing snapshot manager functionality. */ public class TestSnapshotManager { - private static final int testMaxSnapshotLimit = 7; + private static final int testMaxSnapshotIDLimit = 7; /** - * Test that the global limit on snapshots is honored. + * Test that the global limit on snapshot Ids is honored. */ @Test (timeout=10000) - public void testSnapshotLimits() throws Exception { - // Setup mock objects for SnapshotManager.createSnapshot. - // + public void testSnapshotIDLimits() throws Exception { + testMaxSnapshotLimit(testMaxSnapshotIDLimit, "rollover", + new Configuration(), testMaxSnapshotIDLimit); + } + + /** + * Tests that the global limit on snapshots is honored. + */ + @Test (timeout=10000) + public void testMaxSnapshotLimit() throws Exception { + Configuration conf = new Configuration(); + conf.setInt(DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_FILESYSTEM_LIMIT, + testMaxSnapshotIDLimit); + conf.setInt(DFSConfigKeys. + DFS_NAMENODE_SNAPSHOT_MAX_LIMIT, + testMaxSnapshotIDLimit); + testMaxSnapshotLimit(testMaxSnapshotIDLimit,"max snapshot limit" , + conf, testMaxSnapshotIDLimit * 2); + } + + private void testMaxSnapshotLimit(int maxSnapshotLimit, String errMsg, + Configuration conf, int maxSnapID) + throws IOException { LeaseManager leaseManager = mock(LeaseManager.class); INodeDirectory ids = mock(INodeDirectory.class); FSDirectory fsdir = mock(FSDirectory.class); INodesInPath iip = mock(INodesInPath.class); - SnapshotManager sm = spy(new SnapshotManager(new Configuration(), fsdir)); + SnapshotManager sm = spy(new SnapshotManager(conf, fsdir)); doReturn(ids).when(sm).getSnapshottableRoot(any()); - doReturn(testMaxSnapshotLimit).when(sm).getMaxSnapshotID(); + doReturn(maxSnapID).when(sm).getMaxSnapshotID(); // Create testMaxSnapshotLimit snapshots. These should all succeed. // - for (Integer i = 0; i < testMaxSnapshotLimit; ++i) { + for (Integer i = 0; i < maxSnapshotLimit; ++i) { sm.createSnapshot(leaseManager, iip, "dummy", i.toString(), Time.now()); } @@ -73,7 +96,7 @@ public void testSnapshotLimits() throws Exception { Assert.fail("Expected SnapshotException not thrown"); } catch (SnapshotException se) { Assert.assertTrue( - StringUtils.toLowerCase(se.getMessage()).contains("rollover")); + StringUtils.toLowerCase(se.getMessage()).contains(errMsg)); } // Delete a snapshot to free up a slot. @@ -83,22 +106,26 @@ public void testSnapshotLimits() throws Exception { // Attempt to create a snapshot again. It should still fail due // to snapshot ID rollover. // + try { sm.createSnapshot(leaseManager, iip, "dummy", "shouldFailSnapshot2", Time.now()); - Assert.fail("Expected SnapshotException not thrown"); + // in case the snapshot ID limit is hit, further creation of snapshots + // even post deletions of snapshots won't succeed + if (maxSnapID < maxSnapshotLimit) { + Assert.fail("CreateSnapshot should succeed"); + } } catch (SnapshotException se) { Assert.assertTrue( - StringUtils.toLowerCase(se.getMessage()).contains("rollover")); + StringUtils.toLowerCase(se.getMessage()).contains(errMsg)); } } - /** * Snapshot is identified by INODE CURRENT_STATE_ID. * So maximum allowable snapshotID should be less than CURRENT_STATE_ID */ @Test - public void testValidateSnapshotIDWidth() { + public void testValidateSnapshotIDWidth() throws Exception { FSDirectory fsdir = mock(FSDirectory.class); SnapshotManager snapshotManager = new SnapshotManager(new Configuration(), fsdir);