HDDS-11389. Incorrect number of deleted containers shown in Recon UI.

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/recon/TestReconTasks.java

@@ -185,17 +185,16 @@ public void testMissingContainerDownNode() throws Exception {
   }
 
   /**
-   * This test verifies the count of MISSING and EMPTY_MISSING containers.
+   * This test verifies the count of MISSING containers.
    * Following steps being followed in a single DN cluster.
    *    --- Allocate a container in SCM.
    *    --- Client writes the chunk and put block to only DN successfully.
    *    --- Shuts down the only DN.
    *    --- Since container to key mapping doesn't have any key mapped to
-   *        container, missing container will be marked EMPTY_MISSING.
+   *        container, missing container will be marked MISSING.
    *    --- Add a key mapping entry to key container mapping table for the
    *        container added.
-   *    --- Now container will no longer be marked as EMPTY_MISSING and just
-   *        as MISSING.
+   *    --- Now container will no longer be marked as MISSING.
    *    --- Restart the only DN in cluster.
    *    --- Now container no longer will be marked as MISSING.
    *
@@ -236,14 +235,14 @@ public void testEmptyMissingContainerDownNode() throws Exception {
     // Bring down the Datanode that had the container replica.
     cluster.shutdownHddsDatanode(pipeline.getFirstNode());
 
+    // Verify that the container is marked as MISSING
     LambdaTestUtils.await(25000, 1000, () -> {
-      List<UnhealthyContainers> allEmptyMissingContainers =
+      List<UnhealthyContainers> allMissingContainers =
           reconContainerManager.getContainerSchemaManager()
               .getUnhealthyContainers(
-                  ContainerSchemaDefinition.UnHealthyContainerStates.
-                      EMPTY_MISSING,
+                  ContainerSchemaDefinition.UnHealthyContainerStates.MISSING,
                   0, 1000);
-      return (allEmptyMissingContainers.size() == 1);
+      return (allMissingContainers.size() == 1);
     });
 
 
@@ -255,7 +254,7 @@ public void testEmptyMissingContainerDownNode() throws Exception {
       reconContainerMetadataManager.commitBatchOperation(rdbBatchOperation);
     }
 
-    // Verify again and now container is not empty missing but just missing.
+    // Verify again and now container is still marked as MISSING, not EMPTY_MISSING.
     LambdaTestUtils.await(25000, 1000, () -> {
       List<UnhealthyContainers> allMissingContainers =
           reconContainerManager.getContainerSchemaManager()
@@ -265,36 +264,6 @@ public void testEmptyMissingContainerDownNode() throws Exception {
       return (allMissingContainers.size() == 1);
     });
 
-    LambdaTestUtils.await(25000, 1000, () -> {
-      List<UnhealthyContainers> allEmptyMissingContainers =
-          reconContainerManager.getContainerSchemaManager()
-              .getUnhealthyContainers(
-                  ContainerSchemaDefinition.UnHealthyContainerStates.
-                      EMPTY_MISSING,
-                  0, 1000);
-      return (allEmptyMissingContainers.isEmpty());
-    });
-
-    // Now remove keys from container. This data is used to
-    // identify if container is empty in terms of keys mapped to container.
-    try (RDBBatchOperation rdbBatchOperation = new RDBBatchOperation()) {
-      reconContainerMetadataManager
-          .batchStoreContainerKeyCounts(rdbBatchOperation, containerID, 0L);
-      reconContainerMetadataManager.commitBatchOperation(rdbBatchOperation);
-    }
-
-    // Check existing container state in UNHEALTHY_CONTAINER table
-    // will be updated as EMPTY_MISSING
-    LambdaTestUtils.await(25000, 1000, () -> {
-      List<UnhealthyContainers> allEmptyMissingContainers =
-          reconContainerManager.getContainerSchemaManager()
-              .getUnhealthyContainers(
-                  ContainerSchemaDefinition.UnHealthyContainerStates.
-                      EMPTY_MISSING,
-                  0, 1000);
-      return (allEmptyMissingContainers.size() == 1);
-    });
-
     // Now restart the cluster and verify the container is no longer missing.
     cluster.restartHddsDatanode(pipeline.getFirstNode(), true);
     LambdaTestUtils.await(25000, 1000, () -> {

hadoop-ozone/recon-codegen/src/main/java/org/hadoop/ozone/recon/schema/ContainerSchemaDefinition.java

@@ -31,6 +31,7 @@
 import javax.sql.DataSource;
 import java.sql.Connection;
 import java.sql.SQLException;
+import java.util.Arrays;
 
 /**
  * Class used to create tables that are required for tracking containers.
@@ -69,11 +70,39 @@ public enum UnHealthyContainerStates {
   public void initializeSchema() throws SQLException {
     Connection conn = dataSource.getConnection();
     dslContext = DSL.using(conn);
-    if (!TABLE_EXISTS_CHECK.test(conn, UNHEALTHY_CONTAINERS_TABLE_NAME)) {
+
+    if (TABLE_EXISTS_CHECK.test(conn, UNHEALTHY_CONTAINERS_TABLE_NAME)) {
+      // Drop the existing constraint if it exists
+      String constraintName = UNHEALTHY_CONTAINERS_TABLE_NAME + "ck1";
+      dslContext.alterTable(UNHEALTHY_CONTAINERS_TABLE_NAME)
+          .dropConstraint(constraintName)
+          .execute();
+
+      // Add the updated constraint with all enum states
+      addUpdatedConstraint();
+    } else {
+      // Create the table if it does not exist
       createUnhealthyContainersTable();
     }
   }
 
+  /**
+   * Add the updated constraint to the table.
+   */
+  private void addUpdatedConstraint() {
+    // Get all enum values as a list of strings
+    String[] enumStates = Arrays.stream(UnHealthyContainerStates.values())
+        .map(Enum::name)
+        .toArray(String[]::new);
+
+    // Alter the table to add the updated constraint
+    dslContext.alterTable(UNHEALTHY_CONTAINERS_TABLE_NAME)
+        .add(DSL.constraint(UNHEALTHY_CONTAINERS_TABLE_NAME + "ck1")
+            .check(field(name("container_state"))
+                .in(enumStates)))
+        .execute();
+  }
+
   /**
    * Create the Missing Containers table.
    */

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java

@@ -408,13 +408,18 @@ public Response getUnhealthyContainers(
       summary = containerHealthSchemaManager.getUnhealthyContainersSummary();
       List<UnhealthyContainers> containers = containerHealthSchemaManager
           .getUnhealthyContainers(internalState, offset, limit);
-      List<UnhealthyContainers> emptyMissingFiltered = containers.stream()
-          .filter(
-              container -> !container.getContainerState()
-                  .equals(UnHealthyContainerStates.EMPTY_MISSING.toString()))
-          .collect(
-              Collectors.toList());
-      for (UnhealthyContainers c : emptyMissingFiltered) {
+
+      // Filtering out EMPTY_MISSING and NEGATIVE_SIZE containers from the response.
+      // These container states are not being inserted into the database as they represent
+      // edge cases that are not critical to track as unhealthy containers.
+      List<UnhealthyContainers> filteredContainers = containers.stream()
+          .filter(container -> !container.getContainerState()
+              .equals(UnHealthyContainerStates.EMPTY_MISSING.toString())
+              && !container.getContainerState()
+              .equals(UnHealthyContainerStates.NEGATIVE_SIZE.toString()))
+          .collect(Collectors.toList());
+
+      for (UnhealthyContainers c : filteredContainers) {
         long containerID = c.getContainerId();
         ContainerInfo containerInfo =
             containerManager.getContainer(ContainerID.valueOf(containerID));

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/UnhealthyContainersResponse.java

@@ -50,12 +50,6 @@ public class UnhealthyContainersResponse {
   @JsonProperty("misReplicatedCount")
   private long misReplicatedCount = 0;
 
-  /**
-   * Total count of containers with negative size.
-   */
-  @JsonProperty("negativeSizeCount")
-  private long negativeSizeCount = 0;
-
   /**
    * A collection of unhealthy containers.
    */
@@ -83,9 +77,6 @@ public void setSummaryCount(String state, long count) {
     } else if (state.equals(
         UnHealthyContainerStates.MIS_REPLICATED.toString())) {
       this.misReplicatedCount = count;
-    } else if (state.equals(
-        UnHealthyContainerStates.NEGATIVE_SIZE.toString())) {
-      this.negativeSizeCount = count;
     }
   }
 
@@ -105,10 +96,6 @@ public long getMisReplicatedCount() {
     return misReplicatedCount;
   }
 
-  public long getNegativeSizeCount() {
-    return negativeSizeCount;
-  }
-
   public Collection<UnhealthyContainerMetadata> getContainers() {
     return containers;
   }

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthStatus.java

@@ -160,6 +160,10 @@ public boolean isEmpty() {
     return numKeys == 0;
   }
 
+  public boolean isEmptyMissing() {
+    return numKeys == 0 && numReplicas == 0;
+  }
+
   private ContainerPlacementStatus getPlacementStatus(
       PlacementPolicy policy, int repFactor) {
     List<DatanodeDetails> dns = healthyReplicas.stream()

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java

@@ -29,6 +29,7 @@
 import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.apache.hadoop.hdds.conf.OzoneConfiguration;
 import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
 import org.apache.hadoop.hdds.scm.PlacementPolicy;
@@ -188,7 +189,7 @@ private void logUnhealthyContainerStats(
     // If any EMPTY_MISSING containers, then it is possible that such
     // containers got stuck in the closing state which never got
     // any replicas created on the datanodes. In this case, we log it as
-    // EMPTY, and insert as EMPTY_MISSING in UNHEALTHY_CONTAINERS table.
+    // EMPTY_MISSING in unhealthy container statistics but do not add it to the table.
     unhealthyContainerStateStatsMap.entrySet().forEach(stateEntry -> {
       UnHealthyContainerStates unhealthyContainerState = stateEntry.getKey();
       Map<String, Long> containerStateStatsMap = stateEntry.getValue();
@@ -256,6 +257,11 @@ private void completeProcessingContainer(
    * completeProcessingContainer is called. This will check to see if any
    * additional records need to be added to the database.
    *
+   * If a container is identified as missing, empty-missing, under-replicated,
+   * over-replicated or mis-replicated, the method checks with SCM to determine
+   * if it has been deleted, using {@code containerDeletedInSCM}. If the container is
+   * deleted in SCM, the corresponding record is removed from Recon.
+   *
    * @param currentTime Timestamp to place on all records generated by this run
    * @param unhealthyContainerStateCountMap
    * @return Count of records processed
@@ -273,34 +279,41 @@ private long processExistingDBRecords(long currentTime,
         recordCount++;
         UnhealthyContainersRecord rec = cursor.fetchNext();
         try {
+          // Set the current container if it's not already set
           if (currentContainer == null) {
             currentContainer = setCurrentContainer(rec.getContainerId());
           }
+          // If the container ID has changed, finish processing the previous one
           if (currentContainer.getContainerID() != rec.getContainerId()) {
             completeProcessingContainer(
                 currentContainer, existingRecords, currentTime,
                 unhealthyContainerStateCountMap);
             existingRecords.clear();
             currentContainer = setCurrentContainer(rec.getContainerId());
           }
-          if (ContainerHealthRecords
-              .retainOrUpdateRecord(currentContainer, rec
-              )) {
-            // Check if the missing container is deleted in SCM
-            if (currentContainer.isMissing() &&
-                containerDeletedInSCM(currentContainer.getContainer())) {
-              rec.delete();
-            }
-            existingRecords.add(rec.getContainerState());
-            if (rec.changed()) {
-              rec.update();
-            }
-          } else {
+
+          //  Unhealthy Containers such as MISSING, EMPTY_MISSING, UNDER_REPLICATED,
+          // OVER_REPLICATED, MIS_REPLICATED can have their unhealthy states changed or retained.
+          if (!ContainerHealthRecords.retainOrUpdateRecord(currentContainer, rec)) {
+            rec.delete();
+            LOG.info("DELETED existing unhealthy container record...for Container: {}",
+                currentContainer.getContainerID());
+          }
+
+          // If the container is marked as MISSING and it's deleted in SCM, remove the record
+          if (currentContainer.isMissing() && containerDeletedInSCM(currentContainer.getContainer())) {
             LOG.info("DELETED existing unhealthy container record...for Container: {}",
                 currentContainer.getContainerID());
             rec.delete();
           }
+
+          existingRecords.add(rec.getContainerState());
+          // If the record was changed, update it
+          if (rec.changed()) {
+            rec.update();
+          }
         } catch (ContainerNotFoundException cnf) {
+          // If the container is not found, delete the record and reset currentContainer
           rec.delete();
           currentContainer = null;
         }
@@ -349,6 +362,18 @@ private void processContainer(ContainerInfo container, long currentTime,
     }
   }
 
+  /**
+   * Ensures the container's state in Recon is updated to match its state in SCM.
+   *
+   * If SCM reports the container as DELETED, this method attempts to transition
+   * the container's state in Recon from CLOSED to DELETING, or from DELETING to
+   * DELETED, based on the current state in Recon. It logs each transition attempt
+   * and handles any exceptions that may occur.
+   *
+   * @param containerInfo the container whose state is being checked and potentially updated.
+   * @return {@code true} if the container was found to be DELETED in SCM and the
+   *         state transition was attempted in Recon; {@code false} otherwise.
+   */
   private boolean containerDeletedInSCM(ContainerInfo containerInfo) {
     try {
       ContainerWithPipeline containerWithPipeline =
@@ -358,13 +383,16 @@ private boolean containerDeletedInSCM(ContainerInfo containerInfo) {
         if (containerInfo.getState() == HddsProtos.LifeCycleState.CLOSED) {
           containerManager.updateContainerState(containerInfo.containerID(),
               HddsProtos.LifeCycleEvent.DELETE);
+          LOG.debug("Successfully changed container {} state from CLOSED to DELETING.",
+              containerInfo.containerID());
         }
         if (containerInfo.getState() == HddsProtos.LifeCycleState.DELETING &&
             containerManager.getContainerReplicas(containerInfo.containerID())
                 .size() == 0
         ) {
           containerManager.updateContainerState(containerInfo.containerID(),
               HddsProtos.LifeCycleEvent.CLEANUP);
+          LOG.info("Successfully Deleted container {} from Recon.", containerInfo.containerID());
         }
         return true;
       }
@@ -401,7 +429,6 @@ private void handleNegativeSizedContainers(
     populateContainerStats(containerHealthStatus,
         UnHealthyContainerStates.NEGATIVE_SIZE,
         unhealthyContainerStateStatsMap);
-    containerHealthSchemaManager.insertUnhealthyContainerRecords(records);
   }
 
   /**
@@ -493,13 +520,17 @@ public static List<UnhealthyContainers> generateUnhealthyRecords(
               unhealthyContainerStateStatsMap);
         } else {
 
-          LOG.debug("Empty container {} is missing. Kindly check the " +
-              "consolidated container stats per UNHEALTHY state logged as " +
-              "starting with **Container State Stats:**");
+        // EMPTY_MISSING containers are not inserted into the database.
+        // These containers typically represent those that were never written to
+        // or remain in an incomplete state. Tracking such containers as unhealthy
+        // would not provide valuable insights since they don't pose a risk or issue
+        // to the system. Instead, they are logged for awareness, but not stored in
+        // the UNHEALTHY_CONTAINERS table to avoid unnecessary entries.
+
+          LOG.debug("Empty container {} is missing. It will be logged in the " +
+              "unhealthy container statistics, but no record will be created in the " +
+              "UNHEALTHY_CONTAINERS table.", container.getContainerID());
 
-          records.add(
-              recordForState(container, EMPTY_MISSING,
-                  time));
           populateContainerStats(container,
               EMPTY_MISSING,
               unhealthyContainerStateStatsMap);
@@ -508,6 +539,22 @@ public static List<UnhealthyContainers> generateUnhealthyRecords(
         return records;
       }
 
+      // For Negative sized containers we only log but not insert into DB
+      if (container.getContainer().getUsedBytes() < 0
+          && !recordForStateExists.contains(
+          UnHealthyContainerStates.NEGATIVE_SIZE.toString())) {
+        // NEGATIVE_SIZE containers are also not inserted into the database.
+        // This condition usually arises due to corrupted or invalid metadata, where
+        // the container's size is inaccurately recorded as negative. Since this does not
+        // represent a typical unhealthy scenario and may not have any meaningful
+        // impact on system health, such containers are logged for investigation but
+        // excluded from the UNHEALTHY_CONTAINERS table to maintain data integrity.
+        populateContainerStats(container,
+            UnHealthyContainerStates.NEGATIVE_SIZE,
+            unhealthyContainerStateStatsMap);
+        return records;
+      }
+
       if (container.isUnderReplicated()
           && !recordForStateExists.contains(
               UnHealthyContainerStates.UNDER_REPLICATED.toString())) {
@@ -650,4 +697,15 @@ private static void populateContainerStats(
               (value + container.getContainer().getUsedBytes()));
     }
   }
+
+  /**
+   * Expose the logger for testing purposes.
+   *
+   * @return the logger instance
+   */
+  @VisibleForTesting
+  public Logger getLogger() {
+    return LOG;
+  }
+
 }