From cba7c00c3fc82991815228122718fd55d9cc5a13 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Tue, 21 Jul 2020 15:48:17 +0530 Subject: [PATCH 1/6] HDFS-15480. Ordered snapshot deletion: record snapshot deletion in XAttr. --- .../server/common/HdfsServerConstants.java | 1 + .../hdfs/server/namenode/FSDirSnapshotOp.java | 34 +++++- .../hdfs/server/namenode/FSDirectory.java | 8 -- .../src/main/resources/hdfs-default.xml | 9 +- .../namenode/TestOrderedSnapshotDeletion.java | 103 +++++++++++++----- 5 files changed, 113 insertions(+), 42 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java index 78d4289a047cb..8bafffa0a6211 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java @@ -366,6 +366,7 @@ enum BlockUCState { "security.hdfs.unreadable.by.superuser"; String XATTR_ERASURECODING_POLICY = "system.hdfs.erasurecoding.policy"; + String SNAPSHOT_XATTR_NAME = "snapshot"; String XATTR_SATISFY_STORAGE_POLICY = "user.hdfs.sps"; 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 c2eb401c5fbb8..df2a5e2500b6e 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 @@ -20,29 +20,37 @@ import org.apache.hadoop.HadoopIllegalArgumentException; import org.apache.hadoop.fs.InvalidPathException; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.XAttr; +import org.apache.hadoop.fs.XAttrSetFlag; import org.apache.hadoop.fs.permission.FsAction; -import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.DFSUtil; +import org.apache.hadoop.hdfs.XAttrHelper; import org.apache.hadoop.hdfs.protocol.FSLimitException; import org.apache.hadoop.hdfs.protocol.HdfsFileStatus; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReportListing; import org.apache.hadoop.hdfs.protocol.SnapshotException; import org.apache.hadoop.hdfs.protocol.SnapshottableDirectoryStatus; +import org.apache.hadoop.hdfs.server.common.HdfsServerConstants; import org.apache.hadoop.hdfs.server.namenode.FSDirectory.DirOp; import org.apache.hadoop.hdfs.server.namenode.snapshot.DirectorySnapshottableFeature; 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.StringUtils; import org.apache.hadoop.util.Time; import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Arrays; +import java.util.EnumSet; class FSDirSnapshotOp { + + static final XAttr.NameSpace XAttrNS = XAttr.NameSpace.SYSTEM; /** Verify if the snapshot name is legal. */ static void verifySnapshotName(FSDirectory fsd, String snapshotName, String path) @@ -263,11 +271,15 @@ static INode.BlocksMapUpdateInfo deleteSnapshot( final int earliest = snapshottable.getDiffs().iterator().next() .getSnapshotId(); if (snapshot.getId() != earliest) { - throw new SnapshotException("Failed to delete snapshot " + snapshotName - + " from directory " + srcRoot.getFullPathName() - + ": " + snapshot + " is not the earliest snapshot id=" + earliest - + " (" + DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED - + " is " + fsd.isSnapshotDeletionOrdered() + ")"); + List existingXAttrs = XAttrStorage.readINodeXAttrs(srcRoot); + XAttr xAttr = buildXAttr(snapshotName); + // Mark the snapshot to be deleted in the xattr of the snapshot root + List newXAttrs = FSDirXAttrOp.setINodeXAttrs(fsd, existingXAttrs, + Arrays.asList(xAttr), + EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE)); + XAttrStorage.updateINodeXAttrs(srcRoot, newXAttrs, + iip.getLatestSnapshotId()); + return null; } } @@ -361,4 +373,14 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip, checkSnapshot(iip.getLastINode(), snapshottableDirs); } } + + static String buildXAttrName(String snapName) { + return StringUtils.toLowerCase(XAttrNS.toString()) + + "." + HdfsServerConstants.SNAPSHOT_XATTR_NAME + "." + snapName; + } + + public static XAttr buildXAttr(String snapName) { + final String name = buildXAttrName(snapName); + return XAttrHelper.buildXAttr(name); + } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java index 6df255e86574e..f201e4764b62e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java @@ -359,14 +359,6 @@ public enum DirOp { DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED_DEFAULT); LOG.info("{} = {}", DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED, snapshotDeletionOrdered); - if (snapshotDeletionOrdered && !xattrsEnabled) { - throw new HadoopIllegalArgumentException("" + - "XAttrs is required by snapshotDeletionOrdered:" - + DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED - + " is true but " - + DFSConfigKeys.DFS_NAMENODE_MAX_XATTR_SIZE_KEY - + " is false."); - } this.accessTimePrecision = conf.getLong( DFS_NAMENODE_ACCESSTIME_PRECISION_KEY, 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 689ecfe2f3342..720c7d625b320 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 @@ -5119,7 +5119,14 @@ for storing directory snapshot diffs. By default, value is set to 10. - + + dfs.namenode.snapshot.deletion.ordered + false + + If set to true, ensures snashots are deleted in order of creation + for a snapshottable root. + + dfs.storage.policy.satisfier.enabled false diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java index c8df780465bab..c114d7b77ab0f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java @@ -19,32 +19,30 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.fs.XAttr; +import org.apache.hadoop.fs.XAttrSetFlag; import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.hdfs.MiniDFSCluster; -import org.apache.hadoop.hdfs.protocol.SnapshotException; -import org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotTestHelper; -import org.apache.hadoop.test.GenericTestUtils; +import org.apache.hadoop.hdfs.XAttrHelper; +import org.apache.hadoop.hdfs.server.common.HdfsServerConstants; import org.junit.After; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.slf4j.event.Level; import java.io.IOException; +import java.util.Arrays; +import java.util.EnumSet; +import java.util.Map; import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED; +import static org.junit.Assert.assertTrue; -/** Test ordered snapshot deletion. */ +/** + * Test ordered snapshot deletion. + */ public class TestOrderedSnapshotDeletion { - static final Logger LOG = LoggerFactory.getLogger(FSDirectory.class); - - { - SnapshotTestHelper.disableLogs(); - GenericTestUtils.setLogLevel(INode.LOG, Level.TRACE); - } - + static final String name = "user.a1"; + static final byte[] value = {0x31, 0x32, 0x33}; private final Path snapshottableDir = new Path("/" + getClass().getSimpleName()); @@ -67,7 +65,7 @@ public void tearDown() throws Exception { } } - @Test (timeout=60000) + @Test(timeout = 60000) public void testConf() throws Exception { DistributedFileSystem hdfs = cluster.getFileSystem(); hdfs.mkdirs(snapshottableDir); @@ -85,21 +83,72 @@ public void testConf() throws Exception { hdfs.mkdirs(sub2); hdfs.createSnapshot(snapshottableDir, "s2"); - assertDeletionDenied(snapshottableDir, "s1", hdfs); - assertDeletionDenied(snapshottableDir, "s2", hdfs); + assertXAttrSet(snapshottableDir, "s1", hdfs, null); + assertXAttrSet(snapshottableDir, "s2", hdfs, null); hdfs.deleteSnapshot(snapshottableDir, "s0"); - assertDeletionDenied(snapshottableDir, "s2", hdfs); + assertXAttrSet(snapshottableDir, "s2", hdfs, null); hdfs.deleteSnapshot(snapshottableDir, "s1"); hdfs.deleteSnapshot(snapshottableDir, "s2"); } - static void assertDeletionDenied(Path snapshottableDir, String snapshot, - DistributedFileSystem hdfs) throws IOException { - try { - hdfs.deleteSnapshot(snapshottableDir, snapshot); - Assert.fail("deleting " +snapshot + " should fail"); - } catch (SnapshotException se) { - LOG.info("Good, it is expected to have " + se); - } + void assertXAttrSet(Path snapshottableDir, String snapshot, + DistributedFileSystem hdfs, XAttr newXattr) + throws IOException { + hdfs.deleteSnapshot(snapshottableDir, snapshot); + // Check xAttr for parent directory + FSNamesystem namesystem = cluster.getNamesystem(); + INode inode = namesystem.getFSDirectory().getINode( + snapshottableDir.toString()); + XAttrFeature f = inode.getXAttrFeature(); + XAttr xAttr = f.getXAttr(FSDirSnapshotOp.buildXAttrName(snapshot)); + assertTrue("Snapshot xAttr should exist", xAttr != null); + assertTrue(xAttr.getName().equals(getXattrName(snapshot))); + assertTrue(xAttr.getNameSpace().equals(XAttr.NameSpace.SYSTEM)); + + // Make sure its not user visible + Map xattrMap = hdfs.getXAttrs(snapshottableDir); + assertTrue(newXattr == null ? xattrMap.isEmpty() : + Arrays.equals(newXattr.getValue(), xattrMap.get(name))); + } + + @Test(timeout = 60000) + public void testSnapshotXattrPersistence() throws Exception { + DistributedFileSystem hdfs = cluster.getFileSystem(); + hdfs.mkdirs(snapshottableDir); + hdfs.allowSnapshot(snapshottableDir); + + final Path sub0 = new Path(snapshottableDir, "sub0"); + hdfs.mkdirs(sub0); + hdfs.createSnapshot(snapshottableDir, "s0"); + + final Path sub1 = new Path(snapshottableDir, "sub1"); + hdfs.mkdirs(sub1); + hdfs.createSnapshot(snapshottableDir, "s1"); + assertXAttrSet(snapshottableDir, "s1", hdfs, null); + cluster.restartNameNodes(); + assertXAttrSet(snapshottableDir, "s1", hdfs, null); + } + + @Test(timeout = 60000) + public void testSnapshotXAttrWithPreExistingXattrs() throws Exception { + DistributedFileSystem hdfs = cluster.getFileSystem(); + hdfs.mkdirs(snapshottableDir); + hdfs.allowSnapshot(snapshottableDir); + hdfs.setXAttr(snapshottableDir, name, value, + EnumSet.of(XAttrSetFlag.CREATE)); + XAttr newXAttr = XAttrHelper.buildXAttr(name, value); + final Path sub0 = new Path(snapshottableDir, "sub0"); + hdfs.mkdirs(sub0); + hdfs.createSnapshot(snapshottableDir, "s0"); + + final Path sub1 = new Path(snapshottableDir, "sub1"); + hdfs.mkdirs(sub1); + hdfs.createSnapshot(snapshottableDir, "s1"); + assertXAttrSet(snapshottableDir, "s1", hdfs, newXAttr); + } + + + private static String getXattrName(String snapshot) { + return HdfsServerConstants.SNAPSHOT_XATTR_NAME + "." + snapshot; } } From f7dfa8dc5ba55eecf59862c867812234112589e1 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Tue, 21 Jul 2020 16:39:40 +0530 Subject: [PATCH 2/6] Addressed review comments. --- .../server/common/HdfsServerConstants.java | 2 +- .../hdfs/server/namenode/FSDirSnapshotOp.java | 25 ++++++++++--------- .../hdfs/server/namenode/FSDirXAttrOp.java | 10 +++++--- .../namenode/TestOrderedSnapshotDeletion.java | 10 +++----- 4 files changed, 23 insertions(+), 24 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java index 8bafffa0a6211..b881fa16d17c8 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java @@ -366,7 +366,7 @@ enum BlockUCState { "security.hdfs.unreadable.by.superuser"; String XATTR_ERASURECODING_POLICY = "system.hdfs.erasurecoding.policy"; - String SNAPSHOT_XATTR_NAME = "snapshot"; + String SNAPSHOT_XATTR_NAME = "system.hdfs.snapshot"; String XATTR_SATISFY_STORAGE_POLICY = "user.hdfs.sps"; 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 df2a5e2500b6e..f101a72f2873d 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 @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hdfs.server.namenode; +import com.google.common.collect.Lists; import org.apache.hadoop.HadoopIllegalArgumentException; import org.apache.hadoop.fs.InvalidPathException; import org.apache.hadoop.fs.Path; @@ -45,12 +46,9 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; -import java.util.Arrays; import java.util.EnumSet; class FSDirSnapshotOp { - - static final XAttr.NameSpace XAttrNS = XAttr.NameSpace.SYSTEM; /** Verify if the snapshot name is legal. */ static void verifySnapshotName(FSDirectory fsd, String snapshotName, String path) @@ -271,14 +269,17 @@ static INode.BlocksMapUpdateInfo deleteSnapshot( final int earliest = snapshottable.getDiffs().iterator().next() .getSnapshotId(); if (snapshot.getId() != earliest) { - List existingXAttrs = XAttrStorage.readINodeXAttrs(srcRoot); - XAttr xAttr = buildXAttr(snapshotName); - // Mark the snapshot to be deleted in the xattr of the snapshot root - List newXAttrs = FSDirXAttrOp.setINodeXAttrs(fsd, existingXAttrs, - Arrays.asList(xAttr), + final XAttr snapshotXAttr = buildXAttr(snapshotName); + final List xattrs = Lists.newArrayListWithCapacity(1); + xattrs.add(snapshotXAttr); + + // The snapshot to be deleted is just marked for deletion in the xAttr. + // Same snaphot delete call can happen multiple times until annd unless + // the very 1st instance of a snapshot delete hides it/remove it from + // snapshot list. XAttrSetFlag.REPLACE needs to be set to here in order + // to address this. + FSDirXAttrOp.unprotectedSetXAttrs(fsd, iip, xattrs, EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE)); - XAttrStorage.updateINodeXAttrs(srcRoot, newXAttrs, - iip.getLatestSnapshotId()); return null; } } @@ -375,8 +376,8 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip, } static String buildXAttrName(String snapName) { - return StringUtils.toLowerCase(XAttrNS.toString()) - + "." + HdfsServerConstants.SNAPSHOT_XATTR_NAME + "." + snapName; + return StringUtils.toLowerCase(HdfsServerConstants.SNAPSHOT_XATTR_NAME + + "." + snapName); } public static XAttr buildXAttr(String snapName) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java index ff82610f545bb..082b0329d79a1 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java @@ -41,10 +41,7 @@ import java.util.List; import java.util.ListIterator; -import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.CRYPTO_XATTR_ENCRYPTION_ZONE; -import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.SECURITY_XATTR_UNREADABLE_BY_SUPERUSER; -import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.XATTR_SATISFY_STORAGE_POLICY; -import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.CRYPTO_XATTR_FILE_ENCRYPTION_INFO; +import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.*; class FSDirXAttrOp { private static final XAttr KEYID_XATTR = @@ -326,6 +323,11 @@ static INode unprotectedSetXAttrs( throw new IOException("Can only set '" + SECURITY_XATTR_UNREADABLE_BY_SUPERUSER + "' on a file."); } + + if (xaName.contains(SNAPSHOT_XATTR_NAME)) { + Preconditions.checkArgument(inode.isDirectory() && + inode.asDirectory().isSnapshottable()); + } } XAttrStorage.updateINodeXAttrs(inode, newXAttrs, iip.getLatestSnapshotId()); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java index c114d7b77ab0f..3e325478ccdd9 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java @@ -24,7 +24,6 @@ import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.hdfs.MiniDFSCluster; import org.apache.hadoop.hdfs.XAttrHelper; -import org.apache.hadoop.hdfs.server.common.HdfsServerConstants; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -35,6 +34,7 @@ import java.util.Map; import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; /** @@ -102,8 +102,9 @@ void assertXAttrSet(Path snapshottableDir, String snapshot, XAttrFeature f = inode.getXAttrFeature(); XAttr xAttr = f.getXAttr(FSDirSnapshotOp.buildXAttrName(snapshot)); assertTrue("Snapshot xAttr should exist", xAttr != null); - assertTrue(xAttr.getName().equals(getXattrName(snapshot))); + assertTrue(xAttr.getName().contains(snapshot)); assertTrue(xAttr.getNameSpace().equals(XAttr.NameSpace.SYSTEM)); + assertNull(xAttr.getValue()); // Make sure its not user visible Map xattrMap = hdfs.getXAttrs(snapshottableDir); @@ -146,9 +147,4 @@ public void testSnapshotXAttrWithPreExistingXattrs() throws Exception { hdfs.createSnapshot(snapshottableDir, "s1"); assertXAttrSet(snapshottableDir, "s1", hdfs, newXAttr); } - - - private static String getXattrName(String snapshot) { - return HdfsServerConstants.SNAPSHOT_XATTR_NAME + "." + snapshot; - } } From 4f2efde88eade660d22c4dee3b6038d63f9870aa Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Tue, 21 Jul 2020 22:35:29 +0530 Subject: [PATCH 3/6] Addressed checkstyle and review comments. --- .../hdfs/server/namenode/FSDirXAttrOp.java | 6 ++- .../namenode/TestOrderedSnapshotDeletion.java | 46 ++++++++++++++----- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java index 082b0329d79a1..dc3d000480f06 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java @@ -41,7 +41,11 @@ import java.util.List; import java.util.ListIterator; -import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.*; +import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.SECURITY_XATTR_UNREADABLE_BY_SUPERUSER; +import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.XATTR_SATISFY_STORAGE_POLICY; +import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.CRYPTO_XATTR_FILE_ENCRYPTION_INFO; +import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.SNAPSHOT_XATTR_NAME; +import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.CRYPTO_XATTR_ENCRYPTION_ZONE; class FSDirXAttrOp { private static final XAttr KEYID_XATTR = diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java index 3e325478ccdd9..30c157b7593fe 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java @@ -24,6 +24,7 @@ import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.hdfs.MiniDFSCluster; import org.apache.hadoop.hdfs.XAttrHelper; +import org.apache.hadoop.hdfs.protocol.HdfsConstants; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -41,8 +42,8 @@ * Test ordered snapshot deletion. */ public class TestOrderedSnapshotDeletion { - static final String name = "user.a1"; - static final byte[] value = {0x31, 0x32, 0x33}; + static final String name1 = "user.a1"; + static final byte[] value1 = {0x31, 0x32, 0x33}; private final Path snapshottableDir = new Path("/" + getClass().getSimpleName()); @@ -83,15 +84,15 @@ public void testConf() throws Exception { hdfs.mkdirs(sub2); hdfs.createSnapshot(snapshottableDir, "s2"); - assertXAttrSet(snapshottableDir, "s1", hdfs, null); - assertXAttrSet(snapshottableDir, "s2", hdfs, null); + assertXAttrSet("s1", hdfs, null); + assertXAttrSet("s2", hdfs, null); hdfs.deleteSnapshot(snapshottableDir, "s0"); - assertXAttrSet(snapshottableDir, "s2", hdfs, null); + assertXAttrSet("s2", hdfs, null); hdfs.deleteSnapshot(snapshottableDir, "s1"); hdfs.deleteSnapshot(snapshottableDir, "s2"); } - void assertXAttrSet(Path snapshottableDir, String snapshot, + void assertXAttrSet(String snapshot, DistributedFileSystem hdfs, XAttr newXattr) throws IOException { hdfs.deleteSnapshot(snapshottableDir, snapshot); @@ -109,7 +110,7 @@ void assertXAttrSet(Path snapshottableDir, String snapshot, // Make sure its not user visible Map xattrMap = hdfs.getXAttrs(snapshottableDir); assertTrue(newXattr == null ? xattrMap.isEmpty() : - Arrays.equals(newXattr.getValue(), xattrMap.get(name))); + Arrays.equals(newXattr.getValue(), xattrMap.get(name1))); } @Test(timeout = 60000) @@ -125,19 +126,40 @@ public void testSnapshotXattrPersistence() throws Exception { final Path sub1 = new Path(snapshottableDir, "sub1"); hdfs.mkdirs(sub1); hdfs.createSnapshot(snapshottableDir, "s1"); - assertXAttrSet(snapshottableDir, "s1", hdfs, null); + assertXAttrSet("s1", hdfs, null); cluster.restartNameNodes(); - assertXAttrSet(snapshottableDir, "s1", hdfs, null); + assertXAttrSet("s1", hdfs, null); } + @Test(timeout = 60000) + public void testSnapshotXattrWithSaveNameSpace() throws Exception { + DistributedFileSystem hdfs = cluster.getFileSystem(); + hdfs.mkdirs(snapshottableDir); + hdfs.allowSnapshot(snapshottableDir); + + final Path sub0 = new Path(snapshottableDir, "sub0"); + hdfs.mkdirs(sub0); + hdfs.createSnapshot(snapshottableDir, "s0"); + + final Path sub1 = new Path(snapshottableDir, "sub1"); + hdfs.mkdirs(sub1); + hdfs.createSnapshot(snapshottableDir, "s1"); + assertXAttrSet("s1", hdfs, null); + hdfs.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_ENTER); + hdfs.saveNamespace(); + hdfs.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_LEAVE); + cluster.restartNameNodes(); + assertXAttrSet("s1", hdfs, null); + } + @Test(timeout = 60000) public void testSnapshotXAttrWithPreExistingXattrs() throws Exception { DistributedFileSystem hdfs = cluster.getFileSystem(); hdfs.mkdirs(snapshottableDir); hdfs.allowSnapshot(snapshottableDir); - hdfs.setXAttr(snapshottableDir, name, value, + hdfs.setXAttr(snapshottableDir, name1, value1, EnumSet.of(XAttrSetFlag.CREATE)); - XAttr newXAttr = XAttrHelper.buildXAttr(name, value); + XAttr newXAttr = XAttrHelper.buildXAttr(name1, value1); final Path sub0 = new Path(snapshottableDir, "sub0"); hdfs.mkdirs(sub0); hdfs.createSnapshot(snapshottableDir, "s0"); @@ -145,6 +167,6 @@ public void testSnapshotXAttrWithPreExistingXattrs() throws Exception { final Path sub1 = new Path(snapshottableDir, "sub1"); hdfs.mkdirs(sub1); hdfs.createSnapshot(snapshottableDir, "s1"); - assertXAttrSet(snapshottableDir, "s1", hdfs, newXAttr); + assertXAttrSet("s1", hdfs, newXAttr); } } From e43dbc026767894b9c60b10d2c13dc84ba42c1ea Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Wed, 22 Jul 2020 12:16:25 +0530 Subject: [PATCH 4/6] Addressed review comments. --- .../hdfs/server/namenode/FSDirSnapshotOp.java | 20 +++---- .../hdfs/server/namenode/FSDirXAttrOp.java | 7 ++- .../src/main/resources/hdfs-default.xml | 8 --- .../namenode/TestOrderedSnapshotDeletion.java | 57 ++++++++++++++++--- 4 files changed, 62 insertions(+), 30 deletions(-) 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 f101a72f2873d..2b772bfe8bad6 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 @@ -269,7 +269,7 @@ static INode.BlocksMapUpdateInfo deleteSnapshot( final int earliest = snapshottable.getDiffs().iterator().next() .getSnapshotId(); if (snapshot.getId() != earliest) { - final XAttr snapshotXAttr = buildXAttr(snapshotName); + final XAttr snapshotXAttr = buildXAttr(); final List xattrs = Lists.newArrayListWithCapacity(1); xattrs.add(snapshotXAttr); @@ -278,8 +278,14 @@ static INode.BlocksMapUpdateInfo deleteSnapshot( // the very 1st instance of a snapshot delete hides it/remove it from // snapshot list. XAttrSetFlag.REPLACE needs to be set to here in order // to address this. - FSDirXAttrOp.unprotectedSetXAttrs(fsd, iip, xattrs, + + // Xattr will set on the snapshot root directory + FSDirXAttrOp.unprotectedSetXAttrs(fsd, + INodesInPath.append(iip, snapshot.getRoot(), + snapshotName.getBytes()), xattrs, EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE)); + fsd.getEditLog().logDeleteSnapshot(snapshotRoot, snapshotName, + logRetryCache, now); return null; } } @@ -375,13 +381,7 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip, } } - static String buildXAttrName(String snapName) { - return StringUtils.toLowerCase(HdfsServerConstants.SNAPSHOT_XATTR_NAME - + "." + snapName); - } - - public static XAttr buildXAttr(String snapName) { - final String name = buildXAttrName(snapName); - return XAttrHelper.buildXAttr(name); + public static XAttr buildXAttr() { ; + return XAttrHelper.buildXAttr(HdfsServerConstants.SNAPSHOT_XATTR_NAME); } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java index dc3d000480f06..2ed8207c60253 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java @@ -328,9 +328,10 @@ static INode unprotectedSetXAttrs( SECURITY_XATTR_UNREADABLE_BY_SUPERUSER + "' on a file."); } - if (xaName.contains(SNAPSHOT_XATTR_NAME)) { - Preconditions.checkArgument(inode.isDirectory() && - inode.asDirectory().isSnapshottable()); + if (xaName.equals(SNAPSHOT_XATTR_NAME) && !(inode.isDirectory() && + inode.getParent().isSnapshottable())) { + throw new IOException("Can only set '" + + SNAPSHOT_XATTR_NAME + "' on a snapshot root."); } } 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 720c7d625b320..b2bf84e24f73f 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 @@ -5119,14 +5119,6 @@ for storing directory snapshot diffs. By default, value is set to 10. - - dfs.namenode.snapshot.deletion.ordered - false - - If set to true, ensures snashots are deleted in order of creation - for a snapshottable root. - - dfs.storage.policy.satisfier.enabled false diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java index 30c157b7593fe..6c67acae8ff93 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java @@ -21,10 +21,13 @@ import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.XAttr; import org.apache.hadoop.fs.XAttrSetFlag; +import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.hdfs.MiniDFSCluster; import org.apache.hadoop.hdfs.XAttrHelper; import org.apache.hadoop.hdfs.protocol.HdfsConstants; +import org.apache.hadoop.hdfs.server.common.HdfsServerConstants; +import org.apache.hadoop.hdfs.server.namenode.snapshot.SnapshotTestHelper; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -34,7 +37,8 @@ import java.util.EnumSet; import java.util.Map; -import static org.apache.hadoop.hdfs.DFSConfigKeys.DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED; +import static org.apache.hadoop.hdfs.DFSConfigKeys. + DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -98,19 +102,25 @@ void assertXAttrSet(String snapshot, hdfs.deleteSnapshot(snapshottableDir, snapshot); // Check xAttr for parent directory FSNamesystem namesystem = cluster.getNamesystem(); - INode inode = namesystem.getFSDirectory().getINode( - snapshottableDir.toString()); + Path snapshotRoot = SnapshotTestHelper.getSnapshotRoot(snapshottableDir, + snapshot); + INode inode = namesystem.getFSDirectory().getINode(snapshotRoot.toString()); XAttrFeature f = inode.getXAttrFeature(); - XAttr xAttr = f.getXAttr(FSDirSnapshotOp.buildXAttrName(snapshot)); + XAttr xAttr = f.getXAttr(HdfsServerConstants.SNAPSHOT_XATTR_NAME); assertTrue("Snapshot xAttr should exist", xAttr != null); - assertTrue(xAttr.getName().contains(snapshot)); + assertTrue(xAttr.getName().equals(HdfsServerConstants.SNAPSHOT_XATTR_NAME. + replace("system.", ""))); assertTrue(xAttr.getNameSpace().equals(XAttr.NameSpace.SYSTEM)); assertNull(xAttr.getValue()); // Make sure its not user visible - Map xattrMap = hdfs.getXAttrs(snapshottableDir); - assertTrue(newXattr == null ? xattrMap.isEmpty() : - Arrays.equals(newXattr.getValue(), xattrMap.get(name1))); + if (cluster.getNameNode().getConf().getBoolean(DFSConfigKeys. + DFS_NAMENODE_XATTRS_ENABLED_KEY, + DFSConfigKeys.DFS_NAMENODE_XATTRS_ENABLED_DEFAULT)) { + Map xattrMap = hdfs.getXAttrs(snapshotRoot); + assertTrue(newXattr == null ? xattrMap.isEmpty() : + Arrays.equals(newXattr.getValue(), xattrMap.get(name1))); + } } @Test(timeout = 60000) @@ -151,7 +161,36 @@ public void testSnapshotXattrWithSaveNameSpace() throws Exception { cluster.restartNameNodes(); assertXAttrSet("s1", hdfs, null); } - + + @Test(timeout = 60000) + public void testSnapshotXattrWithDisablingXattr() throws Exception { + DistributedFileSystem hdfs = cluster.getFileSystem(); + hdfs.mkdirs(snapshottableDir); + hdfs.allowSnapshot(snapshottableDir); + + final Path sub0 = new Path(snapshottableDir, "sub0"); + hdfs.mkdirs(sub0); + hdfs.createSnapshot(snapshottableDir, "s0"); + + final Path sub1 = new Path(snapshottableDir, "sub1"); + hdfs.mkdirs(sub1); + hdfs.createSnapshot(snapshottableDir, "s1"); + assertXAttrSet("s1", hdfs, null); + cluster.getNameNode().getConf().setBoolean( + DFSConfigKeys.DFS_NAMENODE_XATTRS_ENABLED_KEY, false); + cluster.restartNameNodes(); + // ensure xAttr feature is disabled + try { + hdfs.getXAttrs(snapshottableDir); + } catch (Exception e) { + assertTrue(e.getMessage().contains("The XAttr operation has been " + + "rejected. Support for XAttrs has been disabled by " + + "setting dfs.namenode.xattrs.enabled to false")); + } + // try deleting snapshot and verify it still sets the snapshot XAttr + assertXAttrSet("s1", hdfs, null); + } + @Test(timeout = 60000) public void testSnapshotXAttrWithPreExistingXattrs() throws Exception { DistributedFileSystem hdfs = cluster.getFileSystem(); From 918026c2bc484e37770802e8ed2ccb922aa75aad Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Wed, 22 Jul 2020 21:42:40 +0530 Subject: [PATCH 5/6] Addressed checkstyle and findbug issues. --- .../hadoop/hdfs/server/namenode/FSDirSnapshotOp.java | 5 ++--- .../server/namenode/TestOrderedSnapshotDeletion.java | 10 +++++----- 2 files changed, 7 insertions(+), 8 deletions(-) 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 2b772bfe8bad6..b4dd5e525596d 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 @@ -39,7 +39,6 @@ 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.StringUtils; import org.apache.hadoop.util.Time; import java.io.IOException; @@ -282,7 +281,7 @@ static INode.BlocksMapUpdateInfo deleteSnapshot( // Xattr will set on the snapshot root directory FSDirXAttrOp.unprotectedSetXAttrs(fsd, INodesInPath.append(iip, snapshot.getRoot(), - snapshotName.getBytes()), xattrs, + DFSUtil.string2Bytes(snapshotName)), xattrs, EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE)); fsd.getEditLog().logDeleteSnapshot(snapshotRoot, snapshotName, logRetryCache, now); @@ -381,7 +380,7 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip, } } - public static XAttr buildXAttr() { ; + public static XAttr buildXAttr() { return XAttrHelper.buildXAttr(HdfsServerConstants.SNAPSHOT_XATTR_NAME); } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java index 6c67acae8ff93..87504cf09e04b 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java @@ -46,8 +46,8 @@ * Test ordered snapshot deletion. */ public class TestOrderedSnapshotDeletion { - static final String name1 = "user.a1"; - static final byte[] value1 = {0x31, 0x32, 0x33}; + static final String xattrName = "user.a1"; + static final byte[] xattrValue = {0x31, 0x32, 0x33}; private final Path snapshottableDir = new Path("/" + getClass().getSimpleName()); @@ -119,7 +119,7 @@ void assertXAttrSet(String snapshot, DFSConfigKeys.DFS_NAMENODE_XATTRS_ENABLED_DEFAULT)) { Map xattrMap = hdfs.getXAttrs(snapshotRoot); assertTrue(newXattr == null ? xattrMap.isEmpty() : - Arrays.equals(newXattr.getValue(), xattrMap.get(name1))); + Arrays.equals(newXattr.getValue(), xattrMap.get(xattrName))); } } @@ -196,9 +196,9 @@ public void testSnapshotXAttrWithPreExistingXattrs() throws Exception { DistributedFileSystem hdfs = cluster.getFileSystem(); hdfs.mkdirs(snapshottableDir); hdfs.allowSnapshot(snapshottableDir); - hdfs.setXAttr(snapshottableDir, name1, value1, + hdfs.setXAttr(snapshottableDir, xattrName, xattrValue, EnumSet.of(XAttrSetFlag.CREATE)); - XAttr newXAttr = XAttrHelper.buildXAttr(name1, value1); + XAttr newXAttr = XAttrHelper.buildXAttr(xattrName, xattrValue); final Path sub0 = new Path(snapshottableDir, "sub0"); hdfs.mkdirs(sub0); hdfs.createSnapshot(snapshottableDir, "s0"); From 7613161ac163a2ff441a3909b01845a7e4723ae8 Mon Sep 17 00:00:00 2001 From: Shashikant Banerjee Date: Thu, 23 Jul 2020 01:00:18 +0530 Subject: [PATCH 6/6] Fixed a bug while replaying edits and addressed review comments. --- .../server/common/HdfsServerConstants.java | 2 +- .../hdfs/server/namenode/FSDirSnapshotOp.java | 42 ---------------- .../hdfs/server/namenode/FSDirXAttrOp.java | 4 +- .../hdfs/server/namenode/FSDirectory.java | 2 +- .../namenode/snapshot/SnapshotManager.java | 50 ++++++++++++++++--- .../src/main/resources/hdfs-default.xml | 1 + .../namenode/TestOrderedSnapshotDeletion.java | 1 + 7 files changed, 48 insertions(+), 54 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java index b881fa16d17c8..a55985edffcc5 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/HdfsServerConstants.java @@ -366,7 +366,7 @@ enum BlockUCState { "security.hdfs.unreadable.by.superuser"; String XATTR_ERASURECODING_POLICY = "system.hdfs.erasurecoding.policy"; - String SNAPSHOT_XATTR_NAME = "system.hdfs.snapshot"; + String SNAPSHOT_XATTR_NAME = "system.hdfs.snapshot.deleted"; String XATTR_SATISFY_STORAGE_POLICY = "user.hdfs.sps"; 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 b4dd5e525596d..923c6a88b0318 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 @@ -17,22 +17,17 @@ */ package org.apache.hadoop.hdfs.server.namenode; -import com.google.common.collect.Lists; import org.apache.hadoop.HadoopIllegalArgumentException; import org.apache.hadoop.fs.InvalidPathException; import org.apache.hadoop.fs.Path; -import org.apache.hadoop.fs.XAttr; -import org.apache.hadoop.fs.XAttrSetFlag; import org.apache.hadoop.fs.permission.FsAction; import org.apache.hadoop.hdfs.DFSUtil; -import org.apache.hadoop.hdfs.XAttrHelper; import org.apache.hadoop.hdfs.protocol.FSLimitException; import org.apache.hadoop.hdfs.protocol.HdfsFileStatus; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReportListing; import org.apache.hadoop.hdfs.protocol.SnapshotException; import org.apache.hadoop.hdfs.protocol.SnapshottableDirectoryStatus; -import org.apache.hadoop.hdfs.server.common.HdfsServerConstants; import org.apache.hadoop.hdfs.server.namenode.FSDirectory.DirOp; import org.apache.hadoop.hdfs.server.namenode.snapshot.DirectorySnapshottableFeature; import org.apache.hadoop.hdfs.server.namenode.snapshot.Snapshot; @@ -45,7 +40,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; -import java.util.EnumSet; class FSDirSnapshotOp { /** Verify if the snapshot name is legal. */ @@ -257,38 +251,6 @@ static INode.BlocksMapUpdateInfo deleteSnapshot( // time of snapshot deletion final long now = Time.now(); - if (fsd.isSnapshotDeletionOrdered()) { - final INodeDirectory srcRoot = snapshotManager.getSnapshottableRoot(iip); - final DirectorySnapshottableFeature snapshottable - = srcRoot.getDirectorySnapshottableFeature(); - final Snapshot snapshot = snapshottable.getSnapshotByName( - srcRoot, snapshotName); - - // Diffs must be not empty since a snapshot exists in the list - final int earliest = snapshottable.getDiffs().iterator().next() - .getSnapshotId(); - if (snapshot.getId() != earliest) { - final XAttr snapshotXAttr = buildXAttr(); - final List xattrs = Lists.newArrayListWithCapacity(1); - xattrs.add(snapshotXAttr); - - // The snapshot to be deleted is just marked for deletion in the xAttr. - // Same snaphot delete call can happen multiple times until annd unless - // the very 1st instance of a snapshot delete hides it/remove it from - // snapshot list. XAttrSetFlag.REPLACE needs to be set to here in order - // to address this. - - // Xattr will set on the snapshot root directory - FSDirXAttrOp.unprotectedSetXAttrs(fsd, - INodesInPath.append(iip, snapshot.getRoot(), - DFSUtil.string2Bytes(snapshotName)), xattrs, - EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE)); - fsd.getEditLog().logDeleteSnapshot(snapshotRoot, snapshotName, - logRetryCache, now); - return null; - } - } - final INode.BlocksMapUpdateInfo collectedBlocks = deleteSnapshot( fsd, snapshotManager, iip, snapshotName, now); fsd.getEditLog().logDeleteSnapshot(snapshotRoot, snapshotName, @@ -379,8 +341,4 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip, checkSnapshot(iip.getLastINode(), snapshottableDirs); } } - - public static XAttr buildXAttr() { - return XAttrHelper.buildXAttr(HdfsServerConstants.SNAPSHOT_XATTR_NAME); - } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java index 2ed8207c60253..4f215acda1ba4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirXAttrOp.java @@ -47,7 +47,7 @@ import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.SNAPSHOT_XATTR_NAME; import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.CRYPTO_XATTR_ENCRYPTION_ZONE; -class FSDirXAttrOp { +public class FSDirXAttrOp { private static final XAttr KEYID_XATTR = XAttrHelper.buildXAttr(CRYPTO_XATTR_ENCRYPTION_ZONE, null); private static final XAttr UNREADABLE_BY_SUPERUSER_XATTR = @@ -266,7 +266,7 @@ static List filterINodeXAttrs( return newXAttrs; } - static INode unprotectedSetXAttrs( + public static INode unprotectedSetXAttrs( FSDirectory fsd, final INodesInPath iip, final List xAttrs, final EnumSet flag) throws IOException { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java index f201e4764b62e..2c7932c0f8394 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java @@ -618,7 +618,7 @@ boolean isXattrsEnabled() { } int getXattrMaxSize() { return xattrMaxSize; } - boolean isSnapshotDeletionOrdered() { + public boolean isSnapshotDeletionOrdered() { return snapshotDeletionOrdered; } 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 30b98b8e86421..9ace8a97b8641 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 @@ -36,10 +36,14 @@ import javax.management.ObjectName; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.Lists; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.XAttr; +import org.apache.hadoop.fs.XAttrSetFlag; import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.DFSUtil; import org.apache.hadoop.hdfs.DFSUtilClient; +import org.apache.hadoop.hdfs.XAttrHelper; import org.apache.hadoop.hdfs.protocol.HdfsFileStatus; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReportListing; @@ -47,14 +51,9 @@ import org.apache.hadoop.hdfs.protocol.SnapshotInfo; import org.apache.hadoop.hdfs.protocol.SnapshottableDirectoryStatus; import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport.DiffReportEntry; -import org.apache.hadoop.hdfs.server.namenode.FSDirectory; +import org.apache.hadoop.hdfs.server.common.HdfsServerConstants; +import org.apache.hadoop.hdfs.server.namenode.*; import org.apache.hadoop.hdfs.server.namenode.FSDirectory.DirOp; -import org.apache.hadoop.hdfs.server.namenode.FSImageFormat; -import org.apache.hadoop.hdfs.server.namenode.FSNamesystem; -import org.apache.hadoop.hdfs.server.namenode.INode; -import org.apache.hadoop.hdfs.server.namenode.INodeDirectory; -import org.apache.hadoop.hdfs.server.namenode.INodesInPath; -import org.apache.hadoop.hdfs.server.namenode.LeaseManager; import org.apache.hadoop.metrics2.util.MBeans; import com.google.common.base.Preconditions; @@ -352,7 +351,38 @@ public String createSnapshot(final LeaseManager leaseManager, */ public void deleteSnapshot(final INodesInPath iip, final String snapshotName, INode.ReclaimContext reclaimContext, long now) throws IOException { - INodeDirectory srcRoot = getSnapshottableRoot(iip); + final INodeDirectory srcRoot = getSnapshottableRoot(iip); + if (fsdir.isSnapshotDeletionOrdered()) { + final DirectorySnapshottableFeature snapshottable + = srcRoot.getDirectorySnapshottableFeature(); + final Snapshot snapshot = snapshottable.getSnapshotByName( + srcRoot, snapshotName); + + // Diffs must be not empty since a snapshot exists in the list + final int earliest = snapshottable.getDiffs().iterator().next() + .getSnapshotId(); + if (snapshot.getId() != earliest) { + final XAttr snapshotXAttr = buildXAttr(); + final List xattrs = Lists.newArrayListWithCapacity(1); + xattrs.add(snapshotXAttr); + + // The snapshot to be deleted is just marked for deletion in the xAttr. + // Same snaphot delete call can happen multiple times until and unless + // the very 1st instance of a snapshot delete hides it/remove it from + // snapshot list. XAttrSetFlag.REPLACE needs to be set to here in order + // to address this. + + // XAttr will set on the snapshot root directory + // NOTE : This function is directly called while replaying the edit + // logs.While replaying the edit logs we need to mark the snapshot + // deleted in the xattr of the snapshot root. + FSDirXAttrOp.unprotectedSetXAttrs(fsdir, + INodesInPath.append(iip, snapshot.getRoot(), + DFSUtil.string2Bytes(snapshotName)), xattrs, + EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE)); + return; + } + } srcRoot.removeSnapshot(reclaimContext, snapshotName, now); numSnapshots.getAndDecrement(); } @@ -545,6 +575,10 @@ public int getMaxSnapshotID() { return ((1 << SNAPSHOT_ID_BIT_WIDTH) - 1); } + public static XAttr buildXAttr() { + return XAttrHelper.buildXAttr(HdfsServerConstants.SNAPSHOT_XATTR_NAME); + } + private ObjectName mxBeanName; public void registerMXBean() { 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 b2bf84e24f73f..689ecfe2f3342 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 @@ -5119,6 +5119,7 @@ for storing directory snapshot diffs. By default, value is set to 10. + dfs.storage.policy.satisfier.enabled false diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java index 87504cf09e04b..d3097ea507770 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestOrderedSnapshotDeletion.java @@ -137,6 +137,7 @@ public void testSnapshotXattrPersistence() throws Exception { hdfs.mkdirs(sub1); hdfs.createSnapshot(snapshottableDir, "s1"); assertXAttrSet("s1", hdfs, null); + assertXAttrSet("s1", hdfs, null); cluster.restartNameNodes(); assertXAttrSet("s1", hdfs, null); }