HDDS-10293. IllegalArgumentException: containerSize Negative

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ContainerSizeCountTask.java

@@ -41,6 +41,7 @@
 import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.DELETED;
 import static org.hadoop.ozone.recon.schema.tables.ContainerCountBySizeTable.CONTAINER_COUNT_BY_SIZE;
 
 
@@ -122,6 +123,11 @@ private void process(ContainerInfo container,
       Map<ContainerSizeCountKey, Long> map) {
     final ContainerID id = container.containerID();
     final long currentSize = container.getUsedBytes();
+    if (currentSize < 0) {
+      LOG.error("Negative container size: {} for container: {}", currentSize,
+          id);
+      return;
+    }
     final Long previousSize = processedContainers.put(id, currentSize);
     if (previousSize != null) {
       decrementContainerSizeCount(previousSize, map);
@@ -132,36 +138,40 @@ private void process(ContainerInfo container,
   /**
    * The process() function is responsible for updating the counts of
    * containers being tracked in a containerSizeCountMap based on the
-   * ContainerInfo objects in the list containers.It then iterates through
+   * ContainerInfo objects in the list containers. It then iterates through
    * the list of containers and does the following for each container:
    *
-   * 1) If the container is not present in processedContainers,
-   * it is a new container, so it is added to the processedContainers map
-   * and the count for its size in the containerSizeCountMap is incremented
-   * by 1 using the handlePutKeyEvent() function.
-   * 2) If the container is present in processedContainers but its size has
-   * been updated to the new size then the count for the old size in the
-   * containerSizeCountMap is decremented by 1 using the
-   * handleDeleteKeyEvent() function. The count for the new size is then
-   * incremented by 1 using the handlePutKeyEvent() function.
-   * 3) If the container is not present in containers list, it means the
-   * container has been deleted.
-   * The remaining containers inside the deletedContainers map are the ones
-   * that are not in the cluster and need to be deleted. Finally, the counts in
-   * the containerSizeCountMap are written to the database using the
-   * writeCountsToDB() function.
+   * 1) If the container's state is not "deleted," it will be processed:
+   *    - If the container is not present in processedContainers, it is a new
+   *      container. Therefore, it is added to the processedContainers map, and
+   *      the count for its size in the containerSizeCountMap is incremented by
+   *      1 using the handlePutKeyEvent() function.
+   *    - If the container is present in processedContainers but its size has
+   *      been updated to a new size, the count for the old size in the
+   *      containerSizeCountMap is decremented by 1 using the
+   *      handleDeleteKeyEvent() function. Subsequently, the count for the new
+   *      size is incremented by 1 using the handlePutKeyEvent() function.
+   *
+   * 2) If the container's state is "deleted," it is skipped, as deleted
+   *    containers are not processed.
+   *
+   * After processing, the remaining containers inside the deletedContainers map
+   * are those that are not in the cluster and need to be deleted from the total
+   * size counts. Finally, the counts in the containerSizeCountMap are written
+   * to the database using the writeCountsToDB() function.
    */
   public void process(List<ContainerInfo> containers) {
     lock.writeLock().lock();
     try {
       final Map<ContainerSizeCountKey, Long> containerSizeCountMap
           = new HashMap<>();
-      final Map<ContainerID, Long> deletedContainers
-          = new HashMap<>(processedContainers);
-
+      final Map<ContainerID, Long> deletedContainers =
+          new HashMap<>(processedContainers);
       // Loop to handle container create and size-update operations
       for (ContainerInfo container : containers) {
-        // The containers present in the cache hence it is not yet deleted
+        if (container.getState().equals(DELETED)) {
+          continue; // Skip deleted containers
+        }
         deletedContainers.remove(container.containerID());
         // For New Container being created
         try {
@@ -261,6 +271,9 @@ private void handleContainerDeleteOperations(
       Map<ContainerSizeCountKey, Long> containerSizeCountMap) {
     for (Map.Entry<ContainerID, Long> containerId :
         deletedContainers.entrySet()) {
+      // processedContainers will only keep a track of all containers that have
+      // been processed except DELETED containers.
+      processedContainers.remove(containerId.getKey());
       long containerSize = deletedContainers.get(containerId.getKey());
       decrementContainerSizeCount(containerSize, containerSizeCountMap);
     }

hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestEndpoints.java

@@ -866,10 +866,12 @@ public void testGetContainerCounts() throws Exception {
     ContainerInfo omContainerInfo1 = mock(ContainerInfo.class);
     given(omContainerInfo1.containerID()).willReturn(new ContainerID(1));
     given(omContainerInfo1.getUsedBytes()).willReturn(1500000000L); // 1.5GB
+    given(omContainerInfo1.getState()).willReturn(LifeCycleState.OPEN);
 
     ContainerInfo omContainerInfo2 = mock(ContainerInfo.class);
     given(omContainerInfo2.containerID()).willReturn(new ContainerID(2));
     given(omContainerInfo2.getUsedBytes()).willReturn(2500000000L); // 2.5GB
+    given(omContainerInfo2.getState()).willReturn(LifeCycleState.OPEN);
 
     // Create a list of container info objects
     List<ContainerInfo> containers = new ArrayList<>();

hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestContainerSizeCountTask.java

@@ -18,6 +18,11 @@
 
 package org.apache.hadoop.ozone.recon.tasks;
 
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.OPEN;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.DELETED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.QUASI_CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
 import static org.hadoop.ozone.recon.schema.tables.ContainerCountBySizeTable.CONTAINER_COUNT_BY_SIZE;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.mockito.BDDMockito.given;
@@ -84,18 +89,21 @@ public void setUp() {
   @Test
   public void testProcess() {
     // mock a container with invalid used bytes
-    final ContainerInfo omContainerInfo0 = mock(ContainerInfo.class);
+    ContainerInfo omContainerInfo0 = mock(ContainerInfo.class);
     given(omContainerInfo0.containerID()).willReturn(new ContainerID(0));
     given(omContainerInfo0.getUsedBytes()).willReturn(-1L);
+    given(omContainerInfo0.getState()).willReturn(OPEN);
 
     // Write 2 keys
     ContainerInfo omContainerInfo1 = mock(ContainerInfo.class);
     given(omContainerInfo1.containerID()).willReturn(new ContainerID(1));
     given(omContainerInfo1.getUsedBytes()).willReturn(1500000000L); // 1.5GB
+    given(omContainerInfo1.getState()).willReturn(CLOSED);
 
     ContainerInfo omContainerInfo2 = mock(ContainerInfo.class);
     given(omContainerInfo2.containerID()).willReturn(new ContainerID(2));
     given(omContainerInfo2.getUsedBytes()).willReturn(2500000000L); // 2.5GB
+    given(omContainerInfo2.getState()).willReturn(CLOSING);
 
     // mock getContainers method to return a list of containers
     List<ContainerInfo> containers = new ArrayList<>();
@@ -124,10 +132,11 @@ public void testProcess() {
         containerCountBySizeDao.findById(recordToFind.value1()).getCount()
             .longValue());
 
-    // Add a new key
+    // Add a new container
     ContainerInfo omContainerInfo3 = mock(ContainerInfo.class);
     given(omContainerInfo3.containerID()).willReturn(new ContainerID(3));
     given(omContainerInfo3.getUsedBytes()).willReturn(1000000000L); // 1GB
+    given(omContainerInfo3.getState()).willReturn(QUASI_CLOSED);
     containers.add(omContainerInfo3);
 
     // Update existing key.
@@ -164,4 +173,59 @@ public void testProcess() {
         .getCount()
         .longValue());
   }
+
+  @Test
+  public void testProcessDeletedAndNegativeSizedContainers() {
+    // Create a list of containers, including one that is deleted
+    ContainerInfo omContainerInfo1 = mock(ContainerInfo.class);
+    given(omContainerInfo1.containerID()).willReturn(new ContainerID(1));
+    given(omContainerInfo1.getUsedBytes()).willReturn(1500000000L); // 1.5GB
+    given(omContainerInfo1.getState()).willReturn(OPEN);
+
+    ContainerInfo omContainerInfo2 = mock(ContainerInfo.class);
+    given(omContainerInfo2.containerID()).willReturn(new ContainerID(2));
+    given(omContainerInfo2.getUsedBytes()).willReturn(2500000000L); // 2.5GB
+    given(omContainerInfo2.getState()).willReturn(CLOSED);
+
+    ContainerInfo omContainerInfoDeleted = mock(ContainerInfo.class);
+    given(omContainerInfoDeleted.containerID()).willReturn(new ContainerID(3));
+    given(omContainerInfoDeleted.getUsedBytes()).willReturn(1000000000L);
+    given(omContainerInfoDeleted.getState()).willReturn(DELETED); // 1GB
+
+    // Create a mock container with negative size
+    final ContainerInfo negativeSizeContainer = mock(ContainerInfo.class);
+    given(negativeSizeContainer.containerID()).willReturn(new ContainerID(0));
+    given(negativeSizeContainer.getUsedBytes()).willReturn(-1L);
+    given(negativeSizeContainer.getState()).willReturn(OPEN);
+
+    // Create a mock container with negative size and DELETE state
+    final ContainerInfo negativeSizeDeletedContainer =
+        mock(ContainerInfo.class);
+    given(negativeSizeDeletedContainer.containerID()).willReturn(
+        new ContainerID(0));
+    given(negativeSizeDeletedContainer.getUsedBytes()).willReturn(-1L);
+    given(negativeSizeDeletedContainer.getState()).willReturn(DELETED);
+
+    // Create a container with valid size
+    final ContainerInfo validSizeContainer = mock(ContainerInfo.class);
+    given(validSizeContainer.containerID()).willReturn(new ContainerID(1));
+    given(validSizeContainer.getUsedBytes()).willReturn(1000000000L); // 1GB
+    given(validSizeContainer.getState()).willReturn(CLOSED);
+
+    // Mock getContainers method to return a list of containers including
+    // both valid and invalid ones
+    List<ContainerInfo> containers = new ArrayList<>();
+    containers.add(omContainerInfo1);
+    containers.add(omContainerInfo2);
+    containers.add(omContainerInfoDeleted);
+    containers.add(negativeSizeContainer);
+    containers.add(negativeSizeDeletedContainer);
+    containers.add(validSizeContainer);
+
+    task.process(containers);
+
+    // Verify that only the valid containers are counted
+    assertEquals(2, containerCountBySizeDao.count());
+  }
+
 }