HDDS-12421. ContainerReportHandler should not make the call to delete replicas

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java

@@ -352,26 +352,32 @@ private boolean updateContainerState(final DatanodeDetails datanode,
        */
       break;
     case DELETING:
+    case DELETED:
       /*
-      HDDS-11136: If a DELETING container has a non-empty CLOSED replica, the container should be moved back to CLOSED
-      state.
+       * HDDS-11136: If a DELETING container has a non-empty CLOSED replica, the container should be moved back to
+       * CLOSED state.
+       *
+       * HDDS-12421: If a DELETED container has a non-empty CLOSED replica, the container should also be moved back to
+       * CLOSED state.
        */
+
+      boolean replicaIsEmpty = replica.hasIsEmpty() && replica.getIsEmpty();
+      // If container is in DELETED state and the reported replica is empty, delete the empty replica.
+      if (container.getState() == HddsProtos.LifeCycleState.DELETED && replicaIsEmpty) {
+        deleteReplica(containerId, datanode, publisher, "DELETED");
+        ignored = true;
+        break;
+      }
+
       boolean replicaStateAllowed = replica.getState() == State.CLOSED;
       boolean replicaNotEmpty = replica.hasIsEmpty() && !replica.getIsEmpty();
       if (replicaStateAllowed && replicaNotEmpty) {
-        logger.info("Moving DELETING container {} to CLOSED state, datanode {} reported replica with state={}, " +
-            "isEmpty={} and keyCount={}.", containerId, datanode.getHostName(), replica.getState(), false,
-            replica.getKeyCount());
-        containerManager.transitionDeletingToClosedState(containerId);
+        logger.info("Moving container {} from {} to CLOSED state, datanode {} reported replica with state={}, " +
+            "isEmpty={}, bcsId={}, keyCount={}, and origin={}",
+            container, container.getState(), datanode.getHostName(), replica.getState(),
+            replica.getIsEmpty(), replica.getBlockCommitSequenceId(), replica.getKeyCount(), replica.getOriginNodeId());
+        containerManager.transitionDeletingOrDeletedToClosedState(containerId);
       }
-
-      break;
-    case DELETED:
-      /*
-       * The container is deleted. delete the replica.
-       */
-      deleteReplica(containerId, datanode, publisher, "DELETED");
-      ignored = true;
       break;
     default:
       break;

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java

@@ -135,14 +135,14 @@ void updateContainerState(ContainerID containerID,
       throws IOException, InvalidStateTransitionException;
 
   /**
-   * Bypasses the container state machine to change a container's state from DELETING to CLOSED. This API was
+   * Bypasses the container state machine to change a container's state from DELETING or DELETED to CLOSED. This API was
    * introduced to fix a bug (HDDS-11136), and should be used with care otherwise.
    *
    * @see <a href="https://issues.apache.org/jira/browse/HDDS-11136">HDDS-11136</a>
    * @param containerID id of the container to transition
    * @throws IOException
    */
-  void transitionDeletingToClosedState(ContainerID containerID) throws IOException;
+  void transitionDeletingOrDeletedToClosedState(ContainerID containerID) throws IOException;
 
   /**
    * Returns the latest list of replicas for given containerId.

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java

@@ -296,12 +296,12 @@ public void updateContainerState(final ContainerID cid,
   }
 
   @Override
-  public void transitionDeletingToClosedState(ContainerID containerID) throws IOException {
+  public void transitionDeletingOrDeletedToClosedState(ContainerID containerID) throws IOException {
     HddsProtos.ContainerID proto = containerID.getProtobuf();
     lock.lock();
     try {
       if (containerExist(containerID)) {
-        containerStateManager.transitionDeletingToClosedState(proto);
+        containerStateManager.transitionDeletingOrDeletedToClosedState(proto);
       } else {
         throwContainerNotFoundException(containerID);
       }

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java

@@ -159,15 +159,15 @@ void updateContainerState(HddsProtos.ContainerID id,
 
 
   /**
-   * Bypasses the container state machine to change a container's state from DELETING to CLOSED. This API was
+   * Bypasses the container state machine to change a container's state from DELETING or DELETED to CLOSED. This API was
    * introduced to fix a bug (HDDS-11136), and should be used with care otherwise.
    *
    * @see <a href="https://issues.apache.org/jira/browse/HDDS-11136">HDDS-11136</a>
    * @param id id of the container to transition
    * @throws IOException
    */
   @Replicate
-  void transitionDeletingToClosedState(HddsProtos.ContainerID id) throws IOException;
+  void transitionDeletingOrDeletedToClosedState(HddsProtos.ContainerID id) throws IOException;
 
   /**
    *

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java

@@ -370,16 +370,16 @@ public void updateContainerState(final HddsProtos.ContainerID containerID,
   }
 
   @Override
-  public void transitionDeletingToClosedState(HddsProtos.ContainerID containerID) throws IOException {
+  public void transitionDeletingOrDeletedToClosedState(HddsProtos.ContainerID containerID) throws IOException {
     final ContainerID id = ContainerID.getFromProtobuf(containerID);
 
     try (AutoCloseableLock ignored = writeLock(id)) {
       if (containers.contains(id)) {
         final ContainerInfo oldInfo = containers.getContainerInfo(id);
         final LifeCycleState oldState = oldInfo.getState();
-        if (oldState != DELETING) {
+        if (oldState != DELETING && oldState != DELETED) {
           throw new InvalidContainerStateException("Cannot transition container " + id + " from " + oldState +
-              " back to CLOSED. The container must be in the DELETING state.");
+              " back to CLOSED. The container must be in the DELETING or DELETED state.");
         }
         ExecutionUtil.create(() -> {
           containers.updateState(id, oldState, CLOSED);

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java

@@ -88,6 +88,7 @@ public boolean handle(ContainerCheckRequest request) {
     //resend deleteCommand if needed
     request.getContainerReplicas().stream()
         .filter(r -> !pendingDelete.contains(r.getDatanodeDetails()))
+        .filter(r -> !r.isEmpty())
         .forEach(rp -> {
           try {
             replicationManager.sendDeleteCommand(

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java

@@ -19,6 +19,7 @@
 
 import static org.apache.hadoop.hdds.scm.exceptions.SCMException.ResultCodes.FAILED_TO_CHANGE_CONTAINER_STATE;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import java.util.Collections;
@@ -457,4 +458,9 @@ private void flushCache(final ContainerInfo... containerInfos) {
     }
   }
 
+  @VisibleForTesting
+  public static Logger getLogger() {
+    return LOG;
+  }
+
 }

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java

@@ -42,6 +42,7 @@
 import org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState;
 import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor;
 import org.apache.hadoop.hdds.scm.container.replication.ContainerReplicaPendingOps;
+import org.apache.hadoop.hdds.scm.container.states.ContainerStateMap;
 import org.apache.hadoop.hdds.scm.ha.SCMHAManager;
 import org.apache.hadoop.hdds.scm.ha.SCMHAManagerStub;
 import org.apache.hadoop.hdds.scm.ha.SequenceIdGenerator;
@@ -53,10 +54,15 @@
 import org.apache.hadoop.hdds.utils.db.DBStoreBuilder;
 import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
 import org.apache.hadoop.ozone.container.common.SCMTestUtils;
+import org.apache.ozone.test.GenericTestUtils;
 import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
+import org.slf4j.event.Level;
 
 /**
  * Tests to verify the functionality of ContainerManager.
@@ -72,6 +78,12 @@ public class TestContainerManagerImpl {
   private NodeManager nodeManager;
   private ContainerReplicaPendingOps pendingOpsMock;
 
+  @BeforeAll
+  static void init() {
+    // Print container state transition logs
+    GenericTestUtils.setLogLevel(ContainerStateMap.getLogger(), Level.TRACE);
+  }
+
   @BeforeEach
   void setUp() throws Exception {
     final OzoneConfiguration conf = SCMTestUtils.getConf(testDir);
@@ -131,9 +143,12 @@ void testUpdateContainerState() throws Exception {
     assertEquals(LifeCycleState.CLOSED, containerManager.getContainer(cid).getState());
   }
 
-  @Test
-  void testTransitionDeletingToClosedState() throws IOException, InvalidStateTransitionException {
-    // allocate OPEN Ratis and Ec containers, and do a series of state changes to transition them to DELETING
+  @ParameterizedTest
+  @EnumSource(value = HddsProtos.LifeCycleState.class,
+      names = {"DELETING", "DELETED"})
+  void testTransitionDeletingOrDeletedToClosedState(HddsProtos.LifeCycleState desiredState)
+      throws IOException, InvalidStateTransitionException {
+    // Allocate OPEN Ratis and Ec containers, and do a series of state changes to transition them to DELETING / DELETED
     final ContainerInfo container = containerManager.allocateContainer(
         RatisReplicationConfig.getInstance(
             ReplicationFactor.THREE), "admin");
@@ -162,9 +177,17 @@ void testTransitionDeletingToClosedState() throws IOException, InvalidStateTrans
     assertEquals(LifeCycleState.DELETING, containerManager.getContainer(cid).getState());
     assertEquals(LifeCycleState.DELETING, containerManager.getContainer(ecCid).getState());
 
-    // DELETING -> CLOSED
-    containerManager.transitionDeletingToClosedState(cid);
-    containerManager.transitionDeletingToClosedState(ecCid);
+    if (desiredState == LifeCycleState.DELETED) {
+      // DELETING -> DELETED
+      containerManager.updateContainerState(cid, HddsProtos.LifeCycleEvent.CLEANUP);
+      containerManager.updateContainerState(ecCid, HddsProtos.LifeCycleEvent.CLEANUP);
+      assertEquals(LifeCycleState.DELETED, containerManager.getContainer(cid).getState());
+      assertEquals(LifeCycleState.DELETED, containerManager.getContainer(ecCid).getState());
+    }
+
+    // DELETING / DELETED -> CLOSED
+    containerManager.transitionDeletingOrDeletedToClosedState(cid);
+    containerManager.transitionDeletingOrDeletedToClosedState(ecCid);
     // the containers should be back in CLOSED state now
     assertEquals(LifeCycleState.CLOSED, containerManager.getContainer(cid).getState());
     assertEquals(LifeCycleState.CLOSED, containerManager.getContainer(ecCid).getState());
@@ -178,13 +201,13 @@ void testTransitionDeletingToClosedStateAllowsOnlyDeletingContainers() throws IO
             ReplicationFactor.THREE), "admin");
     final ContainerID cid = container.containerID();
     assertEquals(LifeCycleState.OPEN, containerManager.getContainer(cid).getState());
-    assertThrows(IOException.class, () -> containerManager.transitionDeletingToClosedState(cid));
+    assertThrows(IOException.class, () -> containerManager.transitionDeletingOrDeletedToClosedState(cid));
 
     // test for EC container
     final ContainerInfo ecContainer = containerManager.allocateContainer(new ECReplicationConfig(3, 2), "admin");
     final ContainerID ecCid = ecContainer.containerID();
     assertEquals(LifeCycleState.OPEN, containerManager.getContainer(ecCid).getState());
-    assertThrows(IOException.class, () -> containerManager.transitionDeletingToClosedState(ecCid));
+    assertThrows(IOException.class, () -> containerManager.transitionDeletingOrDeletedToClosedState(ecCid));
   }
 
   @Test

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java

@@ -151,9 +151,10 @@ void setup() throws IOException, InvalidStateTransitionException {
         any(ContainerID.class), any(ContainerReplica.class));
 
     doAnswer(invocation -> {
-      containerStateManager.transitionDeletingToClosedState(((ContainerID) invocation.getArgument(0)).getProtobuf());
+      containerStateManager.transitionDeletingOrDeletedToClosedState(
+          ((ContainerID) invocation.getArgument(0)).getProtobuf());
       return null;
-    }).when(containerManager).transitionDeletingToClosedState(any(ContainerID.class));
+    }).when(containerManager).transitionDeletingOrDeletedToClosedState(any(ContainerID.class));
   }
 
   @AfterEach

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManager.java

@@ -53,6 +53,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
 
 /**
  * Testing ContainerStatemanager.
@@ -147,11 +149,14 @@ public void checkReplicationStateMissingReplica()
     assertEquals(3, c1.getReplicationConfig().getRequiredNodes());
   }
 
-  @Test
-  public void testTransitionDeletingToClosedState() throws IOException {
+  @ParameterizedTest
+  @EnumSource(value = HddsProtos.LifeCycleState.class,
+      names = {"DELETING", "DELETED"})
+  public void testTransitionDeletingOrDeletedToClosedState(HddsProtos.LifeCycleState lifeCycleState)
+      throws IOException {
     HddsProtos.ContainerInfoProto.Builder builder = HddsProtos.ContainerInfoProto.newBuilder();
     builder.setContainerID(1)
-        .setState(HddsProtos.LifeCycleState.DELETING)
+        .setState(lifeCycleState)
         .setUsedBytes(0)
         .setNumberOfKeys(0)
         .setOwner("root")
@@ -161,7 +166,7 @@ public void testTransitionDeletingToClosedState() throws IOException {
     HddsProtos.ContainerInfoProto container = builder.build();
     HddsProtos.ContainerID cid = HddsProtos.ContainerID.newBuilder().setId(container.getContainerID()).build();
     containerStateManager.addContainer(container);
-    containerStateManager.transitionDeletingToClosedState(cid);
+    containerStateManager.transitionDeletingOrDeletedToClosedState(cid);
     assertEquals(HddsProtos.LifeCycleState.CLOSED, containerStateManager.getContainer(ContainerID.getFromProtobuf(cid))
         .getState());
   }
@@ -181,7 +186,7 @@ public void testTransitionDeletingToClosedStateAllowsOnlyDeletingContainer() thr
     HddsProtos.ContainerID cid = HddsProtos.ContainerID.newBuilder().setId(container.getContainerID()).build();
     containerStateManager.addContainer(container);
     try {
-      containerStateManager.transitionDeletingToClosedState(cid);
+      containerStateManager.transitionDeletingOrDeletedToClosedState(cid);
       fail("Was expecting an Exception, but did not catch any.");
     } catch (IOException e) {
       assertInstanceOf(InvalidContainerStateException.class, e.getCause().getCause());

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandling.java

@@ -53,7 +53,8 @@
 import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
 import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup;
 import org.apache.ozone.test.GenericTestUtils;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
 
 /**
  * Tests for container report handling.
@@ -64,13 +65,18 @@ public class TestContainerReportHandling {
   private static final String KEY = "key1";
 
   /**
-   * Tests that a DELETING container moves to the CLOSED state if a non-empty CLOSED replica is reported. To do this,
-   * the test first creates a key and closes its corresponding container. Then it moves that container to the
-   * DELETING state using ContainerManager. Then it restarts a Datanode hosting that container, making it send a full
-   * container report. Then the test waits for the container to move from DELETING to CLOSED.
+   * Tests that a DELETING (or DELETED) container moves to the CLOSED state if a non-empty CLOSED replica is reported.
+   * To do this, the test first creates a key and closes its corresponding container. Then it moves that container to
+   * DELETING (or DELETED) state using ContainerManager. Then it restarts a Datanode hosting that container,
+   * making it send a full container report.
+   * Finally, the test waits for the container to move from DELETING (or DELETED) to CLOSED.
    */
-  @Test
-  void testDeletingContainerTransitionsToClosedWhenNonEmptyReplicaIsReported() throws Exception {
+  @ParameterizedTest
+  @EnumSource(value = HddsProtos.LifeCycleState.class,
+      names = {"DELETING", "DELETED"})
+  void testDeletingOrDeletedContainerTransitionsToClosedWhenNonEmptyReplicaIsReported(
+      HddsProtos.LifeCycleState desiredState)
+      throws Exception {
     OzoneConfiguration conf = new OzoneConfiguration();
     conf.setTimeDuration(OZONE_SCM_STALENODE_INTERVAL, 3, TimeUnit.SECONDS);
     conf.setTimeDuration(OZONE_SCM_DEADNODE_INTERVAL, 6, TimeUnit.SECONDS);
@@ -94,6 +100,12 @@ void testDeletingContainerTransitionsToClosedWhenNonEmptyReplicaIsReported() thr
         containerManager.updateContainerState(containerID, HddsProtos.LifeCycleEvent.DELETE);
         assertEquals(HddsProtos.LifeCycleState.DELETING, containerManager.getContainer(containerID).getState());
 
+        // move the container to DELETED in the second test case
+        if (desiredState == HddsProtos.LifeCycleState.DELETED) {
+          containerManager.updateContainerState(containerID, HddsProtos.LifeCycleEvent.CLEANUP);
+          assertEquals(HddsProtos.LifeCycleState.DELETED, containerManager.getContainer(containerID).getState());
+        }
+
         // restart a DN and wait for the container to get CLOSED.
         HddsDatanodeService dn = cluster.getHddsDatanode(keyLocation.getPipeline().getFirstNode());
         cluster.restartHddsDatanode(dn.getDatanodeDetails(), false);

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandlingWithHA.java

@@ -54,7 +54,8 @@
 import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
 import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup;
 import org.apache.ozone.test.GenericTestUtils;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
 
 /**
  * Tests for container report handling with SCM High Availability.
@@ -65,13 +66,18 @@ public class TestContainerReportHandlingWithHA {
   private static final String KEY = "key1";
 
   /**
-   * Tests that a DELETING container moves to the CLOSED state if a non-empty CLOSED replica is reported. To do this,
-   * the test first creates a key and closes its corresponding container. Then it moves that container to the
-   * DELETING state using ContainerManager. Then it restarts a Datanode hosting that container, making it send a full
-   * container report. Then the test waits for the container to move from DELETING to CLOSED in all SCMs.
+   * Tests that a DELETING (or DELETED) container moves to the CLOSED state if a non-empty CLOSED replica is reported.
+   * To do this, the test first creates a key and closes its corresponding container. Then it moves that container to
+   * DELETING (or DELETED) state using ContainerManager. Then it restarts a Datanode hosting that container,
+   * making it send a full container report.
+   * Finally, the test waits for the container to move from DELETING (or DELETED) to CLOSED in all SCMs.
    */
-  @Test
-  void testDeletingContainerTransitionsToClosedWhenNonEmptyReplicaIsReportedWithScmHA() throws Exception {
+  @ParameterizedTest
+  @EnumSource(value = HddsProtos.LifeCycleState.class,
+      names = {"DELETING", "DELETED"})
+  void testDeletingOrDeletedContainerTransitionsToClosedWhenNonEmptyReplicaIsReportedWithScmHA(
+      HddsProtos.LifeCycleState desiredState)
+      throws Exception {
     OzoneConfiguration conf = new OzoneConfiguration();
     conf.setTimeDuration(OZONE_SCM_STALENODE_INTERVAL, 3, TimeUnit.SECONDS);
     conf.setTimeDuration(OZONE_SCM_DEADNODE_INTERVAL, 6, TimeUnit.SECONDS);
@@ -96,6 +102,12 @@ void testDeletingContainerTransitionsToClosedWhenNonEmptyReplicaIsReportedWithSc
         containerManager.updateContainerState(containerID, HddsProtos.LifeCycleEvent.DELETE);
         assertEquals(HddsProtos.LifeCycleState.DELETING, containerManager.getContainer(containerID).getState());
 
+        // move the container to DELETED in the second test case
+        if (desiredState == HddsProtos.LifeCycleState.DELETED) {
+          containerManager.updateContainerState(containerID, HddsProtos.LifeCycleEvent.CLEANUP);
+          assertEquals(HddsProtos.LifeCycleState.DELETED, containerManager.getContainer(containerID).getState());
+        }
+
         // restart a DN and wait for the container to get CLOSED in all SCMs
         HddsDatanodeService dn = cluster.getHddsDatanode(keyLocation.getPipeline().getFirstNode());
         cluster.restartHddsDatanode(dn.getDatanodeDetails(), false);