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

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

@@ -18,6 +18,7 @@
 package org.apache.hadoop.hdfs.server.namenode;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
 import org.apache.hadoop.classification.InterfaceAudience;
@@ -84,6 +85,22 @@ public class XAttrFeature implements INode.Feature {
     }
   }
 
+  @Override
+  public boolean equals(Object o) {
+    if (o == null) {
+      return false;
+    }
+    if (getClass() != o.getClass()) {
+      return false;
+    }
+    return getXAttrs().equals(((XAttrFeature) o).getXAttrs());
+  }
+
+  @Override
+  public int hashCode() {
+    return Arrays.hashCode(getXAttrs().toArray());
+  }
+
   /**
    * Get XAttr by name with prefix.
    * @param prefixedName xAttr name with prefix

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

@@ -24,11 +24,14 @@ import java.text.SimpleDateFormat;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.Date;
+import java.util.Objects;
+
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hdfs.DFSUtil;
 import org.apache.hadoop.hdfs.protocol.HdfsConstants;
 import org.apache.hadoop.hdfs.server.namenode.AclFeature;
+import org.apache.hadoop.hdfs.server.namenode.INodeDirectoryAttributes;
 import org.apache.hadoop.hdfs.server.namenode.ContentSummaryComputationContext;
 import org.apache.hadoop.hdfs.server.namenode.DirectoryWithQuotaFeature;
 import org.apache.hadoop.hdfs.server.namenode.FSImageFormat;
@@ -196,6 +199,18 @@ public class Snapshot implements Comparable<byte[]> {
       return computeDirectoryContentSummary(summary, snapshotId);
     }
 
+    @Override
+    public boolean metadataEquals(INodeDirectoryAttributes other) {
+      return other != null && getQuotaCounts().equals(other.getQuotaCounts())
+          && getPermissionLong() == other.getPermissionLong()
+          // Acl feature maintains a reference counted map, thereby
+          // every snapshot copy should point to the same Acl object unless
+          // there is no change in acl values.
+          // Reference equals is hence intentional here.
+          && getAclFeature() == other.getAclFeature()
+          && Objects.equals(getXAttrFeature(), other.getXAttrFeature());
+    }
+
     @Override
     public String getFullPathName() {
       return getSnapshotPath(getParent().getFullPathName(), getLocalName());

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

@@ -43,6 +43,11 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 
+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.SnapshotDiffReport;
 import org.apache.hadoop.test.GenericTestUtils;
 
 import org.apache.hadoop.conf.Configuration;
@@ -72,10 +77,6 @@ import org.apache.hadoop.fs.permission.FsAction;
 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 +1188,30 @@ public class TestEncryptionZones {
     }
   }
 
+  @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 +1757,6 @@ public class TestEncryptionZones {
         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/TestXAttrWithSnapshot.java

@@ -36,6 +36,7 @@ import org.apache.hadoop.hdfs.MiniDFSCluster;
 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 class TestXAttrWithSnapshot {
     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
    */