HDFS-15480. Ordered snapshot deletion: record snapshot deletion in XAttr

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 = "system.hdfs.snapshot";
 
   String XATTR_SATISFY_STORAGE_POLICY = "user.hdfs.sps";
 

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

@@ -17,30 +17,36 @@
  */
 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.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.EnumSet;
 
 class FSDirSnapshotOp {
   /** Verify if the snapshot name is legal. */
@@ -263,11 +269,24 @@ 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() + ")");
+        final XAttr snapshotXAttr = buildXAttr();
+        final List<XAttr> 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(),
+                snapshotName.getBytes()), xattrs,
+            EnumSet.of(XAttrSetFlag.CREATE, XAttrSetFlag.REPLACE));
+        fsd.getEditLog().logDeleteSnapshot(snapshotRoot, snapshotName,
+            logRetryCache, now);
+        return null;
       }
     }
 
@@ -361,4 +380,8 @@ static void checkSnapshot(FSDirectory fsd, INodesInPath iip,
       checkSnapshot(iip.getLastINode(), snapshottableDirs);
     }
   }
+
+  public static XAttr buildXAttr() { ;
+    return XAttrHelper.buildXAttr(HdfsServerConstants.SNAPSHOT_XATTR_NAME);
+  }
 }

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

@@ -41,10 +41,11 @@
 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.SNAPSHOT_XATTR_NAME;
+import static org.apache.hadoop.hdfs.server.common.HdfsServerConstants.CRYPTO_XATTR_ENCRYPTION_ZONE;
 
 class FSDirXAttrOp {
   private static final XAttr KEYID_XATTR =
@@ -326,6 +327,12 @@ static INode unprotectedSetXAttrs(
         throw new IOException("Can only set '" +
             SECURITY_XATTR_UNREADABLE_BY_SUPERUSER + "' on a file.");
       }
+
+      if (xaName.equals(SNAPSHOT_XATTR_NAME) && !(inode.isDirectory() &&
+          inode.getParent().isSnapshottable())) {
+        throw new IOException("Can only set '" +
+            SNAPSHOT_XATTR_NAME + "' on a snapshot root.");
+      }
     }
 
     XAttrStorage.updateINodeXAttrs(inode, newXAttrs, iip.getLatestSnapshotId());

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,

hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml

@@ -5119,7 +5119,6 @@
     for storing directory snapshot diffs. By default, value is set to 10.
   </description>
 </property>
-
 <property>
   <name>dfs.storage.policy.satisfier.enabled</name>
   <value>false</value>

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

@@ -19,32 +19,35 @@
 
 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.DFSConfigKeys;
 import org.apache.hadoop.hdfs.DistributedFileSystem;
 import org.apache.hadoop.hdfs.MiniDFSCluster;
-import org.apache.hadoop.hdfs.protocol.SnapshotException;
+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.apache.hadoop.test.GenericTestUtils;
 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.apache.hadoop.hdfs.DFSConfigKeys.
+    DFS_NAMENODE_SNAPSHOT_DELETION_ORDERED;
+import static org.junit.Assert.assertNull;
+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 name1 = "user.a1";
+  static final byte[] value1 = {0x31, 0x32, 0x33};
   private final Path snapshottableDir
       = new Path("/" + getClass().getSimpleName());
 
@@ -67,7 +70,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 +88,124 @@ public void testConf() throws Exception {
     hdfs.mkdirs(sub2);
     hdfs.createSnapshot(snapshottableDir, "s2");
 
-    assertDeletionDenied(snapshottableDir, "s1", hdfs);
-    assertDeletionDenied(snapshottableDir, "s2", hdfs);
+    assertXAttrSet("s1", hdfs, null);
+    assertXAttrSet("s2", hdfs, null);
     hdfs.deleteSnapshot(snapshottableDir, "s0");
-    assertDeletionDenied(snapshottableDir, "s2", hdfs);
+    assertXAttrSet("s2", hdfs, null);
     hdfs.deleteSnapshot(snapshottableDir, "s1");
     hdfs.deleteSnapshot(snapshottableDir, "s2");
   }
 
-  static void assertDeletionDenied(Path snapshottableDir, String snapshot,
-      DistributedFileSystem hdfs) throws IOException {
+  void assertXAttrSet(String snapshot,
+                      DistributedFileSystem hdfs, XAttr newXattr)
+      throws IOException {
+    hdfs.deleteSnapshot(snapshottableDir, snapshot);
+    // Check xAttr for parent directory
+    FSNamesystem namesystem = cluster.getNamesystem();
+    Path snapshotRoot = SnapshotTestHelper.getSnapshotRoot(snapshottableDir,
+        snapshot);
+    INode inode = namesystem.getFSDirectory().getINode(snapshotRoot.toString());
+    XAttrFeature f = inode.getXAttrFeature();
+    XAttr xAttr = f.getXAttr(HdfsServerConstants.SNAPSHOT_XATTR_NAME);
+    assertTrue("Snapshot xAttr should exist", xAttr != null);
+    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
+    if (cluster.getNameNode().getConf().getBoolean(DFSConfigKeys.
+            DFS_NAMENODE_XATTRS_ENABLED_KEY,
+        DFSConfigKeys.DFS_NAMENODE_XATTRS_ENABLED_DEFAULT)) {
+      Map<String, byte[]> xattrMap = hdfs.getXAttrs(snapshotRoot);
+      assertTrue(newXattr == null ? xattrMap.isEmpty() :
+          Arrays.equals(newXattr.getValue(), xattrMap.get(name1)));
+    }
+  }
+
+  @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("s1", hdfs, null);
+    cluster.restartNameNodes();
+    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 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.deleteSnapshot(snapshottableDir, snapshot);
-      Assert.fail("deleting " +snapshot + " should fail");
-    } catch (SnapshotException se) {
-      LOG.info("Good, it is expected to have " + se);
+      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();
+    hdfs.mkdirs(snapshottableDir);
+    hdfs.allowSnapshot(snapshottableDir);
+    hdfs.setXAttr(snapshottableDir, name1, value1,
+        EnumSet.of(XAttrSetFlag.CREATE));
+    XAttr newXAttr = XAttrHelper.buildXAttr(name1, value1);
+    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, newXAttr);
   }
 }