HDDS-13083. Handle cases where block deletion generates tree file before scanner

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java

@@ -44,7 +44,6 @@
 import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException;
 import org.apache.hadoop.hdds.utils.SimpleStriped;
 import org.apache.hadoop.ozone.container.common.impl.ContainerData;
-import org.apache.hadoop.ozone.container.common.interfaces.Container;
 import org.apache.hadoop.ozone.container.common.statemachine.DatanodeConfiguration;
 import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
 import org.apache.ratis.thirdparty.com.google.protobuf.ByteString;
@@ -91,15 +90,8 @@ public ContainerProtos.ContainerChecksumInfo writeContainerDataTree(ContainerDat
     Lock writeLock = getLock(containerID);
     writeLock.lock();
     try {
-      ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = null;
-      try {
-        // If the file is not present, we will create the data for the first time. This happens under a write lock.
-        checksumInfoBuilder = readBuilder(data).orElse(ContainerProtos.ContainerChecksumInfo.newBuilder());
-      } catch (IOException ex) {
-        LOG.error("Failed to read container checksum tree file for container {}. Creating a new instance.",
-            containerID, ex);
-        checksumInfoBuilder = ContainerProtos.ContainerChecksumInfo.newBuilder();
-      }
+      // If the file is not present, we will create the data for the first time. This happens under a write lock.
+      ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = readOrCreate(data).toBuilder();
 
       ContainerProtos.ContainerMerkleTree treeProto = captureLatencyNs(metrics.getCreateMerkleTreeLatencyNS(),
           tree::toProto);
@@ -126,16 +118,8 @@ public void markBlocksAsDeleted(KeyValueContainerData data, Collection<Long> del
     Lock writeLock = getLock(containerID);
     writeLock.lock();
     try {
-      ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = null;
-      try {
-        // If the file is not present, we will create the data for the first time. This happens under a write lock.
-        checksumInfoBuilder = readBuilder(data)
-            .orElse(ContainerProtos.ContainerChecksumInfo.newBuilder());
-      } catch (IOException ex) {
-        LOG.error("Failed to read container checksum tree file for container {}. Overwriting it with a new instance.",
-            data.getContainerID(), ex);
-        checksumInfoBuilder = ContainerProtos.ContainerChecksumInfo.newBuilder();
-      }
+      // If the file is not present, we will create the data for the first time. This happens under a write lock.
+      ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = readOrCreate(data).toBuilder();
 
       // Although the persisted block list should already be sorted, we will sort it here to make sure.
       // This will automatically fix any bugs in the persisted order that may show up.
@@ -304,13 +288,6 @@ public static long getDataChecksum(ContainerProtos.ContainerChecksumInfo checksu
     return checksumInfo.getContainerMerkleTree().getDataChecksum();
   }
 
-  /**
-   * Returns whether the container checksum tree file for the specified container exists without deserializing it.
-   */
-  public static boolean hasContainerChecksumFile(ContainerData data) {
-    return getContainerChecksumFile(data).exists();
-  }
-
   /**
    * Returns the container checksum tree file for the specified container without deserializing it.
    */
@@ -329,21 +306,34 @@ private Lock getLock(long containerID) {
   }
 
   /**
+   * Reads the checksum info of the specified container. If the tree file with the information does not exist, an empty
+   * instance is returned.
    * Callers are not required to hold a lock while calling this since writes are done to a tmp file and atomically
    * swapped into place.
    */
-  public Optional<ContainerProtos.ContainerChecksumInfo> read(ContainerData data) throws IOException {
+  public ContainerProtos.ContainerChecksumInfo read(ContainerData data) throws IOException {
     try {
-      return captureLatencyNs(metrics.getReadContainerMerkleTreeLatencyNS(), () -> readChecksumInfo(data));
+      return captureLatencyNs(metrics.getReadContainerMerkleTreeLatencyNS(), () ->
+          readChecksumInfo(data).orElse(ContainerProtos.ContainerChecksumInfo.newBuilder().build()));
     } catch (IOException ex) {
       metrics.incrementMerkleTreeReadFailures();
-      throw new IOException(ex);
+      throw ex;
     }
   }
 
-  private Optional<ContainerProtos.ContainerChecksumInfo.Builder> readBuilder(ContainerData data) throws IOException {
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = read(data);
-    return checksumInfo.map(ContainerProtos.ContainerChecksumInfo::toBuilder);
+  /**
+   * Reads the checksum info of the specified container. If the tree file with the information does not exist, or there
+   * is an exception trying to read the file, an empty instance is returned.
+   */
+  private ContainerProtos.ContainerChecksumInfo readOrCreate(ContainerData data) {
+    try {
+      // If the file is not present, we will create the data for the first time. This happens under a write lock.
+      return read(data);
+    } catch (IOException ex) {
+      LOG.error("Failed to read container checksum tree file for container {}. Overwriting it with a new instance.",
+          data.getContainerID(), ex);
+      return ContainerProtos.ContainerChecksumInfo.newBuilder().build();
+    }
   }
 
   /**
@@ -395,6 +385,7 @@ public ByteString getContainerChecksumInfo(KeyValueContainerData data) throws IO
    * Callers are not required to hold a lock while calling this since writes are done to a tmp file and atomically
    * swapped into place.
    */
+  // TODO HDDS-12824 Once data checksum is stored in RocksDB this method can be removed.
   public static Optional<ContainerProtos.ContainerChecksumInfo> readChecksumInfo(ContainerData data)
       throws IOException {
     long containerID = data.getContainerID();
@@ -417,10 +408,4 @@ public static Optional<ContainerProtos.ContainerChecksumInfo> readChecksumInfo(C
   public ContainerMerkleTreeMetrics getMetrics() {
     return this.metrics;
   }
-
-  public static boolean checksumFileExist(Container<?> container) {
-    File checksumFile = getContainerChecksumFile(container.getContainerData());
-    return checksumFile.exists();
-  }
-
 }

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java

@@ -657,6 +657,10 @@ public long getDataChecksum() {
     return dataChecksum;
   }
 
+  public boolean needsDataChecksum() {
+    return !isEmpty && dataChecksum == 0;
+  }
+
   /**
    * Returns a ProtoBuf Message from ContainerData.
    *

hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java

@@ -81,7 +81,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.NavigableMap;
-import java.util.Optional;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.concurrent.locks.Lock;
@@ -1383,15 +1382,15 @@ public void updateContainerChecksum(Container container, ContainerMerkleTreeWrit
    * Write the merkle tree for this container using the existing checksum metadata only. The data is not read or
    * validated by this method, so it is expected to run quickly.
    * <p>
-   * If a checksum file already exists on the disk, this method will do nothing. The existing file would have either
+   * If a data checksum for the container already exists, this method does nothing. The existing value would have either
    * been made from the metadata or data itself so there is no need to recreate it from the metadata. This method
    * does not send an ICR with the updated checksum info.
    * <p>
    *
    * @param container The container which will have a tree generated.
    */
   private void updateContainerChecksumFromMetadataIfNeeded(Container container) {
-    if (ContainerChecksumTreeManager.checksumFileExist(container)) {
+    if (!container.getContainerData().needsDataChecksum()) {
       return;
     }
 
@@ -1435,24 +1434,24 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont
     // checksum to prevent divergence from what SCM sees in the ICR vs what datanode peers will see when pulling the
     // merkle tree.
     long originalDataChecksum = containerData.getDataChecksum();
+    boolean hadDataChecksum = containerData.needsDataChecksum();
     ContainerProtos.ContainerChecksumInfo updateChecksumInfo = checksumManager.writeContainerDataTree(containerData,
         treeWriter);
     long updatedDataChecksum = updateChecksumInfo.getContainerMerkleTree().getDataChecksum();
 
     if (updatedDataChecksum != originalDataChecksum) {
       containerData.setDataChecksum(updatedDataChecksum);
-      String message =
-          "Container data checksum updated from " + checksumToString(originalDataChecksum) + " to " +
-              checksumToString(updatedDataChecksum);
       if (sendICR) {
         sendICR(container);
       }
-      if (ContainerChecksumTreeManager.hasContainerChecksumFile(containerData)) {
+
+      String message = "Container data checksum updated from " + checksumToString(originalDataChecksum) + " to " +
+              checksumToString(updatedDataChecksum);
+      if (hadDataChecksum) {
         LOG.warn(message);
         ContainerLogger.logChecksumUpdated(containerData, originalDataChecksum);
       } else {
-        // If this is the first time the scanner has run with the feature to generate a checksum file, don't
-        // log a warning for the checksum update.
+        // If this is the first time the checksum is being generated, don't log a warning about updating the checksum.
         LOG.debug(message);
       }
     }
@@ -1572,12 +1571,9 @@ public void reconcileContainer(DNContainerOperationClient dnClient, Container<?>
     long containerID = containerData.getContainerID();
 
     // Obtain the original checksum info before reconciling with any peers.
-    Optional<ContainerProtos.ContainerChecksumInfo> optionalChecksumInfo = checksumManager.read(containerData);
-    ContainerProtos.ContainerChecksumInfo originalChecksumInfo;
-    if (optionalChecksumInfo.isPresent()) {
-      originalChecksumInfo = optionalChecksumInfo.get();
-    } else {
-      // Try creating the checksum info from RocksDB metadata if it is not present.
+    ContainerProtos.ContainerChecksumInfo originalChecksumInfo = checksumManager.read(containerData);
+    if (!originalChecksumInfo.hasContainerMerkleTree()) {
+      // Try creating the merkle tree from RocksDB metadata if it is not present.
       originalChecksumInfo = updateAndGetContainerChecksumFromMetadata(kvContainer);
     }
     // This holds our current most up-to-date checksum info that we are using for the container.

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java

@@ -330,12 +330,12 @@ private static void assertEqualsChunkMerkleTree(List<ContainerProtos.ChunkMerkle
   }
 
   /**
-   * This function checks whether the container checksum file exists.
+   * This function checks whether the container checksum file exists for a container in a given datanode.
    */
   public static boolean containerChecksumFileExists(HddsDatanodeService hddsDatanode, long containerID) {
     OzoneContainer ozoneContainer = hddsDatanode.getDatanodeStateMachine().getContainer();
     Container<?> container = ozoneContainer.getController().getContainer(containerID);
-    return ContainerChecksumTreeManager.checksumFileExist(container);
+    return getContainerChecksumFile(container.getContainerData()).exists();
   }
 
   public static void writeContainerDataTreeProto(ContainerData data, ContainerProtos.ContainerMerkleTree tree)

hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java

@@ -39,7 +39,6 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import java.util.Optional;
 import java.util.stream.Stream;
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.hadoop.hdds.conf.ConfigurationSource;
@@ -336,8 +335,8 @@ public void testContainerWithNoDiff() throws Exception {
     ContainerProtos.ContainerChecksumInfo peerChecksumInfo = ContainerProtos.ContainerChecksumInfo.newBuilder()
             .setContainerID(container.getContainerID())
             .setContainerMerkleTree(peerMerkleTree.toProto()).build();
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    ContainerDiffReport diff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    ContainerDiffReport diff = checksumManager.diff(checksumInfo, peerChecksumInfo);
     assertTrue(checksumManager.getMetrics().getMerkleTreeDiffLatencyNS().lastStat().total() > 0);
     assertFalse(diff.needsRepair());
     assertEquals(checksumManager.getMetrics().getNoRepairContainerDiffs(), 1);
@@ -360,8 +359,8 @@ public void testContainerDiffWithMismatches(int numMissingBlock, int numMissingC
     ContainerProtos.ContainerChecksumInfo peerChecksumInfo = ContainerProtos.ContainerChecksumInfo.newBuilder()
         .setContainerID(container.getContainerID())
         .setContainerMerkleTree(peerMerkleTree.toProto()).build();
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    ContainerDiffReport diff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    ContainerDiffReport diff = checksumManager.diff(checksumInfo, peerChecksumInfo);
     assertTrue(metrics.getMerkleTreeDiffLatencyNS().lastStat().total() > 0);
     assertContainerDiffMatch(expectedDiff, diff);
     assertEquals(checksumManager.getMetrics().getRepairContainerDiffs(), 1);
@@ -384,8 +383,8 @@ public void testPeerWithMismatchesHasNoDiff(int numMissingBlock, int numMissingC
     ContainerProtos.ContainerChecksumInfo peerChecksumInfo = ContainerProtos.ContainerChecksumInfo.newBuilder()
         .setContainerID(container.getContainerID())
         .setContainerMerkleTree(peerMerkleTree).build();
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    ContainerDiffReport diff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    ContainerDiffReport diff = checksumManager.diff(checksumInfo, peerChecksumInfo);
     assertFalse(diff.needsRepair());
     assertEquals(checksumManager.getMetrics().getNoRepairContainerDiffs(), 1);
   }
@@ -395,8 +394,8 @@ public void testFailureContainerMerkleTreeMetric() throws IOException {
     ContainerProtos.ContainerChecksumInfo peerChecksum = ContainerProtos.ContainerChecksumInfo.newBuilder().build();
     ContainerMerkleTreeWriter ourMerkleTree = buildTestTree(config);
     checksumManager.writeContainerDataTree(container, ourMerkleTree);
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    assertThrows(StorageContainerException.class, () -> checksumManager.diff(checksumInfo.get(), peerChecksum));
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    assertThrows(StorageContainerException.class, () -> checksumManager.diff(checksumInfo, peerChecksum));
     assertEquals(checksumManager.getMetrics().getMerkleTreeDiffFailure(), 1);
   }
 
@@ -416,8 +415,8 @@ void testDeletedBlocksInPeerAndBoth() throws Exception {
         .addAllDeletedBlocks(deletedBlockList).build();
 
     writeContainerDataTreeProto(container, ourMerkleTree);
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo);
 
     // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted
     // in peer merkle tree.
@@ -428,7 +427,7 @@ void testDeletedBlocksInPeerAndBoth() throws Exception {
     // Delete blocks in our merkle tree as well.
     checksumManager.markBlocksAsDeleted(container, deletedBlockList);
     checksumInfo = checksumManager.read(container);
-    containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo);
 
     // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted
     // in both merkle tree.
@@ -454,8 +453,8 @@ void testDeletedBlocksInOurContainerOnly() throws Exception {
     writeContainerDataTreeProto(container, ourMerkleTree);
     checksumManager.markBlocksAsDeleted(container, deletedBlockList);
 
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo);
 
     // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted
     // in our merkle tree.
@@ -481,8 +480,8 @@ void testCorruptionInOurMerkleTreeAndDeletedBlocksInPeer() throws Exception {
 
     writeContainerDataTreeProto(container, ourMerkleTree);
 
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo);
 
     // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted
     // in peer merkle tree.
@@ -506,8 +505,8 @@ void testContainerDiffWithBlockDeletionInPeer() throws Exception {
     writeContainerDataTreeProto(container, ourMerkleTree);
 
     ContainerProtos.ContainerChecksumInfo peerChecksumInfo = peerChecksumInfoBuilder.build();
-    Optional<ContainerProtos.ContainerChecksumInfo> checksumInfo = checksumManager.read(container);
-    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container);
+    ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo);
     // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted
     // in peer merkle tree.
     assertFalse(containerDiff.getMissingBlocks().isEmpty());
@@ -520,7 +519,7 @@ void testContainerDiffWithBlockDeletionInPeer() throws Exception {
     // Clear deleted blocks to add them in missing blocks.
     peerChecksumInfo = peerChecksumInfoBuilder.clearDeletedBlocks().build();
     checksumInfo = checksumManager.read(container);
-    containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo);
+    containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo);
 
     assertFalse(containerDiff.getMissingBlocks().isEmpty());
     // Missing block does not contain the deleted blocks 6L to 10L