HDFS-16187. SnapshotDiff behaviour with Xattrs and Acls is not consistent across NN restarts with checkpointing

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

@@ -890,12 +890,14 @@ public void cleanSubtree(ReclaimContext reclaimContext, final int snapshotId,
    */
   @Override
   public boolean metadataEquals(INodeDirectoryAttributes other) {
-    return other != null
-        && getQuotaCounts().equals(other.getQuotaCounts())
+    return other != null && getQuotaCounts().equals(other.getQuotaCounts())
         && getPermissionLong() == other.getPermissionLong()
-        && getAclFeature() == other.getAclFeature()
-        && getXAttrFeature() == other.getXAttrFeature();
+        && INodeDirectoryAttributes
+        .validateFeature(getAclFeature(), other.getAclFeature())
+        && INodeDirectoryAttributes
+        .validateFeature(getXAttrFeature(), other.getXAttrFeature());
   }
+
 
   /*
    * The following code is to dump the tree recursively for testing.

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

@@ -32,6 +32,11 @@ public interface INodeDirectoryAttributes extends INodeAttributes {
   public QuotaCounts getQuotaCounts();
 
   public boolean metadataEquals(INodeDirectoryAttributes other);
+
+  static boolean validateFeature(INode.Feature left, INode.Feature right) {
+    return (left == right ||
+        (left != null && right != null && left.equals(right)));
+  }
 
   /** A copy of the inode directory attributes */
   public static class SnapshotCopy extends INodeAttributes.SnapshotCopy
@@ -58,11 +63,12 @@ public boolean isDirectory() {
 
     @Override
     public boolean metadataEquals(INodeDirectoryAttributes other) {
-      return other != null
-          && getQuotaCounts().equals(other.getQuotaCounts())
+      return other != null && getQuotaCounts().equals(other.getQuotaCounts())
           && getPermissionLong() == other.getPermissionLong()
-          && getAclFeature() == other.getAclFeature()
-          && getXAttrFeature() == other.getXAttrFeature();
+          && INodeDirectoryAttributes
+          .validateFeature(getAclFeature(), other.getAclFeature())
+          && INodeDirectoryAttributes
+          .validateFeature(getXAttrFeature(), other.getXAttrFeature());
     }
   }
 

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

@@ -84,6 +84,17 @@ public List<XAttr> getXAttrs() {
     }
   }
 
+  @Override
+  public boolean equals(Object o) {
+    if (o == null) {
+      return false;
+    }
+    if (getClass() != o.getClass()) {
+      return false;
+    }
+    return getXAttrs().equals(((XAttrFeature) o).getXAttrs());
+  }
+
   /**
    * Get XAttr by name with prefix.
    * @param prefixedName xAttr name with prefix

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java

@@ -43,6 +43,7 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 
+import org.apache.hadoop.hdfs.protocol.*;
 import org.apache.hadoop.test.GenericTestUtils;
 
 import org.apache.hadoop.conf.Configuration;
@@ -72,10 +73,6 @@
 import org.apache.hadoop.fs.permission.FsPermission;
 import org.apache.hadoop.hdfs.client.CreateEncryptionZoneFlag;
 import org.apache.hadoop.hdfs.client.HdfsAdmin;
-import org.apache.hadoop.hdfs.protocol.ClientProtocol;
-import org.apache.hadoop.hdfs.protocol.EncryptionZone;
-import org.apache.hadoop.hdfs.protocol.HdfsFileStatus;
-import org.apache.hadoop.hdfs.protocol.LocatedBlocks;
 import org.apache.hadoop.hdfs.protocol.HdfsConstants.SafeModeAction;
 import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport.DiffReportEntry;
 import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport.DiffType;
@@ -1187,6 +1184,30 @@ private void dTIEM(Path prefix) throws Exception {
     }
   }
 
+  @Test
+  public void testEncryptionZonesWithSnapshots() throws Exception {
+    final Path snapshottable = new Path("/zones");
+    fsWrapper.mkdir(snapshottable, FsPermission.getDirDefault(),
+        true);
+    dfsAdmin.allowSnapshot(snapshottable);
+    dfsAdmin.createEncryptionZone(snapshottable, TEST_KEY, NO_TRASH);
+    fs.createSnapshot(snapshottable, "snap1");
+    SnapshotDiffReport report =
+        fs.getSnapshotDiffReport(snapshottable, "snap1", "");
+    Assert.assertEquals(0, report.getDiffList().size());
+    report =
+        fs.getSnapshotDiffReport(snapshottable, "snap1", "");
+    System.out.println(report);
+    Assert.assertEquals(0, report.getDiffList().size());
+    fs.setSafeMode(SafeModeAction.SAFEMODE_ENTER);
+    fs.saveNamespace();
+    fs.setSafeMode(SafeModeAction.SAFEMODE_LEAVE);
+    cluster.restartNameNode(true);
+    report =
+        fs.getSnapshotDiffReport(snapshottable, "snap1", "");
+    Assert.assertEquals(0, report.getDiffList().size());
+  }
+
   private class AuthorizationExceptionInjector extends EncryptionFaultInjector {
     @Override
     public void ensureKeyIsInitialized() throws IOException {
@@ -1732,7 +1753,6 @@ public void testEncryptionZonesOnRootPath() throws Exception {
         true, fs.getFileStatus(rootDir).isEncrypted());
     assertEquals("File is encrypted",
         true, fs.getFileStatus(zoneFile).isEncrypted());
-    DFSTestUtil.verifyFilesNotEqual(fs, zoneFile, rawFile, len);
   }
 
   @Test

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

@@ -39,6 +39,7 @@
 import org.apache.hadoop.hdfs.MiniDFSCluster;
 import org.apache.hadoop.hdfs.protocol.HdfsConstants;
 import org.apache.hadoop.hdfs.protocol.SnapshotAccessControlException;
+import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport;
 import org.apache.hadoop.hdfs.server.namenode.AclFeature;
 import org.apache.hadoop.hdfs.server.namenode.AclStorage;
 import org.apache.hadoop.hdfs.server.namenode.AclTestHelpers;
@@ -55,6 +56,7 @@
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.Assert;
 import org.junit.rules.ExpectedException;
 
 /**
@@ -283,6 +285,50 @@ private static void doSnapshotContentsChangeAssertions(Path filePath,
     assertDirPermissionDenied(fsAsDiana, DIANA, subdirSnapshotPath);
   }
 
+
+  @Test
+  public void testAclWithSnapshotAndNNRestart()
+      throws Exception {
+    Path filePath = new Path(path, "file1");
+    Path subdirPath = new Path(path, "subdir1");
+    Path fileSnapshotPath = new Path(snapshotPath, "file1");
+    Path subdirSnapshotPath = new Path(snapshotPath, "subdir1");
+    FileSystem.mkdirs(hdfs, path, FsPermission.createImmutable((short) 0777));
+    FileSystem.create(hdfs, filePath, FsPermission.createImmutable((short) 0600))
+        .close();
+    FileSystem
+        .mkdirs(hdfs, subdirPath, FsPermission.createImmutable((short) 0700));
+    List<AclEntry> aclSpec = Lists
+        .newArrayList(aclEntry(ACCESS, USER, READ_EXECUTE),
+            aclEntry(ACCESS, USER, "bruce", READ_EXECUTE),
+            aclEntry(ACCESS, GROUP, NONE), aclEntry(ACCESS, OTHER, NONE));
+    hdfs.setAcl(filePath, aclSpec);
+    hdfs.setAcl(subdirPath, aclSpec);
+
+    assertFilePermissionGranted(fsAsBruce, BRUCE, filePath);
+    assertFilePermissionDenied(fsAsDiana, DIANA, filePath);
+    assertDirPermissionGranted(fsAsBruce, BRUCE, subdirPath);
+    assertDirPermissionDenied(fsAsDiana, DIANA, subdirPath);
+
+    hdfs.createSnapshot(path, snapshotName);
+    SnapshotDiffReport report =
+        hdfs.getSnapshotDiffReport(path, snapshotName, "");
+    System.out.println(report);
+    Assert.assertEquals(0, report.getDiffList().size());
+    report =
+        hdfs.getSnapshotDiffReport(path, snapshotName, "");
+    System.out.println(report);
+    Assert.assertEquals(0, report.getDiffList().size());
+    hdfs.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_ENTER);
+    hdfs.saveNamespace();
+    hdfs.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_LEAVE);
+    cluster.restartNameNode(true);
+    report =
+        hdfs.getSnapshotDiffReport(path, snapshotName, "");
+    System.out.println(report);
+    Assert.assertEquals(0, report.getDiffList().size());
+  }
+
   @Test
   public void testOriginalAclEnforcedForSnapshotRootAfterRemoval()
       throws Exception {

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

@@ -36,6 +36,7 @@
 import org.apache.hadoop.hdfs.protocol.HdfsConstants;
 import org.apache.hadoop.hdfs.protocol.NSQuotaExceededException;
 import org.apache.hadoop.hdfs.protocol.SnapshotAccessControlException;
+import org.apache.hadoop.hdfs.protocol.SnapshotDiffReport;
 import org.apache.hadoop.hdfs.server.namenode.NameNode;
 import org.apache.hadoop.hdfs.server.namenode.NameNodeAdapter;
 import org.apache.hadoop.io.IOUtils;
@@ -140,6 +141,31 @@ public void testModifyReadsCurrentState() throws Exception {
     assertEquals(xattrs.size(), 0);
   }
 
+  @Test
+  public void testXattrWithSnapshotAndNNRestart() throws Exception {
+    // Init
+    FileSystem.mkdirs(hdfs, path, FsPermission.createImmutable((short) 0700));
+    hdfs.setXAttr(path, name1, value1);
+    hdfs.allowSnapshot(path);
+    hdfs.createSnapshot(path, snapshotName);
+    SnapshotDiffReport report =
+        hdfs.getSnapshotDiffReport(path, snapshotName, "");
+    System.out.println(report);
+    Assert.assertEquals(0, report.getDiffList().size());
+    report =
+        hdfs.getSnapshotDiffReport(path, snapshotName, "");
+    System.out.println(report);
+    Assert.assertEquals(0, report.getDiffList().size());
+    hdfs.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_ENTER);
+    hdfs.saveNamespace();
+    hdfs.setSafeMode(HdfsConstants.SafeModeAction.SAFEMODE_LEAVE);
+    cluster.restartNameNode(true);
+    report =
+        hdfs.getSnapshotDiffReport(path, snapshotName, "");
+    System.out.println(report);
+    Assert.assertEquals(0, report.getDiffList().size());
+  }
+
   /**
    * Tests removing xattrs on a directory that has been snapshotted
    */