diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java index aeb96cbf3196..6e2134e253fb 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java @@ -245,7 +245,7 @@ private boolean updateContainerState(final DatanodeDetails datanode, final ContainerID containerId = container.containerID(); boolean ignored = false; - + boolean replicaIsEmpty = replica.hasIsEmpty() && replica.getIsEmpty(); switch (container.getState()) { case OPEN: /* @@ -351,27 +351,29 @@ private boolean updateContainerState(final DatanodeDetails datanode, * The container is already in closed state. do nothing. */ break; + case DELETED: + // If container is in DELETED state and the reported replica is empty, delete the empty replica. + // We should also do this for DELETING containers and currently DeletingContainerHandler does that + if (replicaIsEmpty) { + deleteReplica(containerId, datanode, publisher, "DELETED", false); + break; + } case DELETING: /* - 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 DELETING or DELETED container has a non-empty replica, the container should also be moved + * back to CLOSED state. */ - 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); + boolean replicaStateAllowed = (replica.getState() != State.INVALID && replica.getState() != State.DELETED); + if (!replicaIsEmpty && replicaStateAllowed) { + 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; @@ -453,9 +455,9 @@ protected ContainerManager getContainerManager() { } protected void deleteReplica(ContainerID containerID, DatanodeDetails dn, - EventPublisher publisher, String reason) { + EventPublisher publisher, String reason, boolean force) { SCMCommand command = new DeleteContainerCommand( - containerID.getId(), true); + containerID.getId(), force); try { command.setTerm(scmContext.getTermOfLeader()); } catch (NotLeaderException nle) { diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java index 50fc751d936d..b1cbd129b702 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java +++ b/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 HDDS-11136 * @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. diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java index c9c5bcf592fa..113f620ebcd5 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java @@ -290,12 +290,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); } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java index 6b0aa0a561bd..484f84576e32 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReportHandler.java @@ -234,7 +234,7 @@ private void processSingleReplica(final DatanodeDetails datanodeDetails, UNKNOWN_CONTAINER_ACTION_DELETE)) { final ContainerID containerId = ContainerID .valueOf(replicaProto.getContainerID()); - deleteReplica(containerId, datanodeDetails, publisher, "unknown"); + deleteReplica(containerId, datanodeDetails, publisher, "unknown", true); } return; } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java index dbac6a899d26..0ec9c2593ec5 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java @@ -159,7 +159,7 @@ 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 HDDS-11136 @@ -167,7 +167,7 @@ void updateContainerState(HddsProtos.ContainerID id, * @throws IOException */ @Replicate - void transitionDeletingToClosedState(HddsProtos.ContainerID id) throws IOException; + void transitionDeletingOrDeletedToClosedState(HddsProtos.ContainerID id) throws IOException; /** * diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java index cd7496535b45..693eed710bb0 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java +++ b/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); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java index 31fcd6e9c8a6..09a613e9ba6b 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/DeletingContainerHandler.java @@ -23,6 +23,7 @@ import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.hdds.scm.container.ContainerID; import org.apache.hadoop.hdds.scm.container.ContainerInfo; +import org.apache.hadoop.hdds.scm.container.ContainerReplica; import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest; import org.apache.hadoop.hdds.scm.container.replication.ContainerReplicaOp; import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager; @@ -88,6 +89,7 @@ public boolean handle(ContainerCheckRequest request) { //resend deleteCommand if needed request.getContainerReplicas().stream() .filter(r -> !pendingDelete.contains(r.getDatanodeDetails())) + .filter(ContainerReplica::isEmpty) .forEach(rp -> { try { replicationManager.sendDeleteCommand( diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java index 71a6f7824a89..561104b810d7 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/states/ContainerStateMap.java +++ b/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; @@ -93,6 +94,11 @@ public ContainerStateMap() { this.replicaMap = new ConcurrentHashMap<>(); } + @VisibleForTesting + public static Logger getLogger() { + return LOG; + } + /** * Adds a ContainerInfo Entry in the ContainerStateMap. * diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java index bea8ec488fca..adf9c9531646 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerManagerImpl.java +++ b/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,29 +177,39 @@ 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()); } @Test - void testTransitionDeletingToClosedStateAllowsOnlyDeletingContainers() throws IOException { + void testTransitionContainerToClosedStateAllowOnlyDeletingOrDeletedContainers() throws IOException { + // Negative test for Ratis/EC container OPEN -> CLOSED transition + // test for RATIS container final ContainerInfo container = containerManager.allocateContainer( RatisReplicationConfig.getInstance( 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 diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java index f1a9091a991c..9f382dd70ec1 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java @@ -24,6 +24,7 @@ import static org.apache.hadoop.hdds.scm.HddsTestUtils.getReplicas; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -36,6 +37,7 @@ import java.time.Clock; import java.time.ZoneId; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -54,6 +56,7 @@ import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReportsProto; import org.apache.hadoop.hdds.scm.HddsTestUtils; import org.apache.hadoop.hdds.scm.container.replication.ContainerReplicaPendingOps; +import org.apache.hadoop.hdds.scm.events.SCMEvents; import org.apache.hadoop.hdds.scm.ha.SCMHAManager; import org.apache.hadoop.hdds.scm.ha.SCMHAManagerStub; import org.apache.hadoop.hdds.scm.metadata.SCMDBDefinition; @@ -76,7 +79,10 @@ 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.Arguments; import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; /** * Test the behaviour of the ContainerReportHandler. @@ -151,9 +157,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 @@ -164,6 +171,52 @@ public void tearDown() throws Exception { } } + static Stream containerAndReplicaStates() { + // Replication types to test + List replicationTypes = Arrays.asList( + HddsProtos.ReplicationType.RATIS, + HddsProtos.ReplicationType.EC + ); + + // Container states to test + List containerStates = Arrays.asList( + HddsProtos.LifeCycleState.DELETING, + HddsProtos.LifeCycleState.DELETED + ); + + // Replica states to test + List replicaStates = Arrays.asList( + ContainerReplicaProto.State.QUASI_CLOSED, + ContainerReplicaProto.State.CLOSED, + ContainerReplicaProto.State.CLOSING, + ContainerReplicaProto.State.OPEN, + ContainerReplicaProto.State.UNHEALTHY + ); + + List invalidReplicaStates = Arrays.asList( + ContainerReplicaProto.State.INVALID, + ContainerReplicaProto.State.DELETED + ); + + // Generate all combinations, container state * replica state + List combinations = new ArrayList<>(); + for (HddsProtos.ReplicationType replicationType : replicationTypes) { + for (HddsProtos.LifeCycleState containerState : containerStates) { + for (ContainerReplicaProto.State replicaState : replicaStates) { + if (replicationType == HddsProtos.ReplicationType.EC && + replicaState.equals(ContainerReplicaProto.State.QUASI_CLOSED)) { + continue; + } + for (ContainerReplicaProto.State invalidState : invalidReplicaStates) { + combinations.add(Arguments.of(replicationType, containerState, replicaState, invalidState)); + } + } + } + } + + return combinations.stream(); + } + private void testReplicaIndexUpdate(ContainerInfo container, DatanodeDetails dn, int replicaIndex, Map expectedReplicaMap) { @@ -446,104 +499,126 @@ public void testClosingToClosedForECContainer() } /** - * Tests that a DELETING RATIS container transitions to CLOSED if a non-empty CLOSED replica is reported. It does not - * transition if a non-empty CLOSING replica is reported. + * Helper method to get a container with specified replication type and state for testing. + * @param replicationType HddsProtos.ReplicationType + * @param containerState HddsProtos.LifeCycleState + * @return ContainerInfo */ - @Test - public void ratisContainerShouldTransitionFromDeletingToClosedWhenNonEmptyClosedReplica() throws IOException { - ContainerInfo container = getContainer(LifeCycleState.DELETING); + private ContainerInfo getContainerHelper( + HddsProtos.ReplicationType replicationType, + final HddsProtos.LifeCycleState containerState) { + switch (replicationType) { + case RATIS: + return getContainer(containerState); + case EC: + return getECContainer(containerState, PipelineID.randomId(), new ECReplicationConfig(6, 3)); + default: + fail("Unsupported replication type: " + replicationType); + } + // make the compiler happy + return null; + } + + /** + * Tests that a DELETING or DELETED RATIS/EC container transitions to CLOSED if a non-empty replica in OPEN, CLOSING, + * CLOSED, QUASI_CLOSED or UNHEALTHY state is reported. + * It should not transition if the replica is in INVALID or DELETED states. + */ + @ParameterizedTest + @MethodSource("containerAndReplicaStates") + public void containerShouldTransitionFromDeletingOrDeletedToClosedWhenNonEmptyReplica( + HddsProtos.ReplicationType replicationType, + LifeCycleState containerState, + ContainerReplicaProto.State replicaState, + ContainerReplicaProto.State invalidReplicaState) + throws IOException { + + ContainerInfo container = getContainerHelper(replicationType, containerState); containerStateManager.addContainer(container.getProtobuf()); - // set up a non-empty CLOSED replica - DatanodeDetails dnWithClosedReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(0); + // set up a non-empty replica in replicaState + DatanodeDetails dnWithValidReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(0); ContainerReplicaProto.Builder builder = ContainerReplicaProto.newBuilder(); - ContainerReplicaProto closedReplica = builder.setContainerID(container.getContainerID()) + ContainerReplicaProto validReplica = builder + .setContainerID(container.getContainerID()) .setIsEmpty(false) - .setState(ContainerReplicaProto.State.CLOSED) - .setKeyCount(0) - .setBlockCommitSequenceId(123) - .setOriginNodeId(dnWithClosedReplica.getUuidString()).build(); + .setState(replicaState) + .setKeyCount(0L) + .setBlockCommitSequenceId(123L) + .setReplicaIndex(1) + .setOriginNodeId(dnWithValidReplica.getUuidString()) + .build(); - // set up a non-empty CLOSING replica - DatanodeDetails dnWithClosingReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(1); - ContainerReplicaProto closingReplica = builder.setState(ContainerReplicaProto.State.CLOSING) - .setOriginNodeId(dnWithClosingReplica.getUuidString()).build(); + // set up a non-empty replica in invalidReplicaState + DatanodeDetails dnWithInvalidReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(1); + ContainerReplicaProto invalidReplica = builder + .setIsEmpty(false) + .setState(invalidReplicaState) + .setReplicaIndex(2) + .setOriginNodeId(dnWithInvalidReplica.getUuidString()) + .build(); - // should not transition on processing the CLOSING replica's report + // should not transition on processing the invalid replica's report ContainerReportHandler containerReportHandler = new ContainerReportHandler(nodeManager, containerManager); - ContainerReportsProto closingContainerReport = getContainerReports(closingReplica); + ContainerReportsProto invalidContainerReport = getContainerReports(invalidReplica); containerReportHandler - .onMessage(new ContainerReportFromDatanode(dnWithClosingReplica, closingContainerReport), publisher); - - assertEquals(LifeCycleState.DELETING, containerStateManager.getContainer(container.containerID()).getState()); + .onMessage(new ContainerReportFromDatanode(dnWithInvalidReplica, invalidContainerReport), publisher); + assertEquals(containerState, containerStateManager.getContainer(container.containerID()).getState()); + /** + * containerState, DELETED + * replicaState, QUASI_CLOSED + * invalidReplicaState, INVALID + * replicationType EC + */ - // should transition on processing the CLOSED replica's report - ContainerReportsProto closedContainerReport = getContainerReports(closedReplica); + // should transition on processing the valid replica's report + ContainerReportsProto closedContainerReport = getContainerReports(validReplica); containerReportHandler - .onMessage(new ContainerReportFromDatanode(dnWithClosedReplica, closedContainerReport), publisher); + .onMessage(new ContainerReportFromDatanode(dnWithValidReplica, closedContainerReport), publisher); assertEquals(LifeCycleState.CLOSED, containerStateManager.getContainer(container.containerID()).getState()); - } - - @Test - public void ratisContainerShouldNotTransitionFromDeletingToClosedWhenEmptyClosedReplica() throws IOException { - ContainerInfo container = getContainer(LifeCycleState.DELETING); - containerStateManager.addContainer(container.getProtobuf()); - - // set up an empty CLOSED replica - DatanodeDetails dnWithClosedReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(0); - ContainerReplicaProto.Builder builder = ContainerReplicaProto.newBuilder(); - ContainerReplicaProto closedReplica = builder.setContainerID(container.getContainerID()) - .setIsEmpty(true) - .setState(ContainerReplicaProto.State.CLOSED) - .setKeyCount(0) - .setBlockCommitSequenceId(123) - .setOriginNodeId(dnWithClosedReplica.getUuidString()).build(); - ContainerReportHandler containerReportHandler = new ContainerReportHandler(nodeManager, containerManager); - ContainerReportsProto closedContainerReport = getContainerReports(closedReplica); - containerReportHandler - .onMessage(new ContainerReportFromDatanode(dnWithClosedReplica, closedContainerReport), publisher); - assertEquals(LifeCycleState.DELETING, containerStateManager.getContainer(container.containerID()).getState()); + // verify that no delete command is issued for non-empty replica, regardless of container state + verify(publisher, times(0)) + .fireEvent(eq(SCMEvents.DATANODE_COMMAND), any(CommandForDatanode.class)); } - /** - * Tests that a DELETING EC container transitions to CLOSED if a non-empty CLOSED replica is reported. It does not - * transition if a non-empty CLOSING (or any other state) replica is reported. - */ - @Test - public void ecContainerShouldTransitionFromDeletingToClosedWhenNonEmptyClosedReplica() throws IOException { - ContainerInfo container = getECContainer(LifeCycleState.DELETING, PipelineID.randomId(), - new ECReplicationConfig(6, 3)); + @ParameterizedTest + @MethodSource("containerAndReplicaStates") + public void containerShouldNotTransitionFromDeletingOrDeletedToClosedWhenEmptyReplica( + HddsProtos.ReplicationType replicationType, + LifeCycleState containerState, + ContainerReplicaProto.State replicaState) throws IOException { + + ContainerInfo container = getContainerHelper(replicationType, containerState); containerStateManager.addContainer(container.getProtobuf()); - // set up a non-empty CLOSED replica - DatanodeDetails dnWithClosedReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(0); + // set up an empty replica + DatanodeDetails dnWithEmptyReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(0); ContainerReplicaProto.Builder builder = ContainerReplicaProto.newBuilder(); - ContainerReplicaProto closedReplica = builder.setContainerID(container.getContainerID()) - .setIsEmpty(false) - .setState(ContainerReplicaProto.State.CLOSED) - .setKeyCount(0) - .setBlockCommitSequenceId(0) + ContainerReplicaProto emptyReplica = builder + .setContainerID(container.getContainerID()) + .setIsEmpty(true) + .setState(replicaState) + .setKeyCount(0L) + .setBlockCommitSequenceId(123L) .setReplicaIndex(1) - .setOriginNodeId(dnWithClosedReplica.getUuidString()).build(); - - // set up a non-empty CLOSING replica - DatanodeDetails dnWithClosingReplica = nodeManager.getNodes(NodeStatus.inServiceHealthy()).get(1); - ContainerReplicaProto closingReplica = builder.setState(ContainerReplicaProto.State.CLOSING).setReplicaIndex(2) - .setOriginNodeId(dnWithClosingReplica.getUuidString()).build(); + .setOriginNodeId(dnWithEmptyReplica.getUuidString()) + .build(); - // should not transition on processing the CLOSING replica's report ContainerReportHandler containerReportHandler = new ContainerReportHandler(nodeManager, containerManager); - ContainerReportsProto closingContainerReport = getContainerReports(closingReplica); - containerReportHandler - .onMessage(new ContainerReportFromDatanode(dnWithClosingReplica, closingContainerReport), publisher); - assertEquals(LifeCycleState.DELETING, containerStateManager.getContainer(container.containerID()).getState()); - - // should transition on processing the CLOSED replica's report - ContainerReportsProto closedContainerReport = getContainerReports(closedReplica); + ContainerReportsProto emptyContainerReport = getContainerReports(emptyReplica); containerReportHandler - .onMessage(new ContainerReportFromDatanode(dnWithClosedReplica, closedContainerReport), publisher); - assertEquals(LifeCycleState.CLOSED, containerStateManager.getContainer(container.containerID()).getState()); + .onMessage(new ContainerReportFromDatanode(dnWithEmptyReplica, emptyContainerReport), publisher); + assertEquals(containerState, containerStateManager.getContainer(container.containerID()).getState()); + + // verify number of datanode command fired (e.g. whether delete command is issued for a replica) + int countDatanodeCommandFired = 0; + if (containerState == LifeCycleState.DELETED) { + // when the container is in DELETED state, the empty replica would be removed + countDatanodeCommandFired++; + } + verify(publisher, times(countDatanodeCommandFired)) + .fireEvent(eq(SCMEvents.DATANODE_COMMAND), any(CommandForDatanode.class)); } /** @@ -1096,12 +1171,10 @@ public void closedECContainerKeyAndBytesUsedUpdatedToMinimumOfAllReplicas() .getNumberOfKeys()); } - @Test - public void testStaleReplicaOfDeletedContainer() throws NodeNotFoundException, - IOException, TimeoutException { - - final ContainerReportHandler reportHandler = new ContainerReportHandler( - nodeManager, containerManager); + @ParameterizedTest + @ValueSource(booleans = {false, true}) + public void testStaleReplicaOfDeletedContainer(boolean isEmpty) throws NodeNotFoundException, IOException { + final ContainerReportHandler reportHandler = new ContainerReportHandler(nodeManager, containerManager); final Iterator nodeIterator = nodeManager.getNodes( NodeStatus.inServiceHealthy()).iterator(); @@ -1115,18 +1188,21 @@ public void testStaleReplicaOfDeletedContainer() throws NodeNotFoundException, nodeManager.setContainers(datanodeOne, containerIDSet); containerStateManager.addContainer(containerOne.getProtobuf()); - // Expects the replica will be deleted. final ContainerReportsProto containerReport = getContainerReportsProto( containerOne.containerID(), ContainerReplicaProto.State.CLOSED, - datanodeOne.getUuidString(), 0); + datanodeOne.getUuidString(), 0, isEmpty); final ContainerReportFromDatanode containerReportFromDatanode = new ContainerReportFromDatanode(datanodeOne, containerReport); reportHandler.onMessage(containerReportFromDatanode, publisher); - verify(publisher, times(1)).fireEvent(any(), any(CommandForDatanode.class)); - - assertEquals(0, containerManager.getContainerReplicas( - containerOne.containerID()).size()); + if (isEmpty) { + // Expect the replica to be deleted when it is empty + verify(publisher, times(1)).fireEvent(any(), any(CommandForDatanode.class)); + } else { + // Expect the replica to stay when it is NOT empty + verify(publisher, times(0)).fireEvent(any(), any(CommandForDatanode.class)); + } + assertEquals(1, containerManager.getContainerReplicas(containerOne.containerID()).size()); } private ContainerReportFromDatanode getContainerReportFromDatanode( @@ -1164,7 +1240,14 @@ protected static ContainerReportsProto getContainerReportsProto( final ContainerID containerId, final ContainerReplicaProto.State state, final String originNodeId, int replicaIndex) { return getContainerReportsProto(containerId, state, originNodeId, - 2000000000L, 100000000L, 10000L, replicaIndex); + 2000000000L, 100000000L, 10000L, replicaIndex, false); + } + + protected static ContainerReportsProto getContainerReportsProto( + final ContainerID containerId, final ContainerReplicaProto.State state, + final String originNodeId, int replicaIndex, boolean isEmpty) { + return getContainerReportsProto(containerId, state, originNodeId, + 2000000000L, 100000000L, 10000L, replicaIndex, isEmpty); } protected static ContainerReportsProto getContainerReportsProto( @@ -1174,10 +1257,20 @@ protected static ContainerReportsProto getContainerReportsProto( 2000000000L, 100000000L, bcsId, replicaIndex); } + protected static ContainerReportsProto getContainerReportsProto( final ContainerID containerId, final ContainerReplicaProto.State state, final String originNodeId, final long usedBytes, final long keyCount, final long bcsId, final int replicaIndex) { + return getContainerReportsProto(containerId, state, originNodeId, usedBytes, + keyCount, bcsId, replicaIndex, false); + } + + @SuppressWarnings("checkstyle:ParameterNumber") + protected static ContainerReportsProto getContainerReportsProto( + final ContainerID containerId, final ContainerReplicaProto.State state, + final String originNodeId, final long usedBytes, final long keyCount, + final long bcsId, final int replicaIndex, final boolean isEmpty) { final ContainerReportsProto.Builder crBuilder = ContainerReportsProto.newBuilder(); final ContainerReplicaProto replicaProto = @@ -1196,6 +1289,7 @@ protected static ContainerReportsProto getContainerReportsProto( .setBlockCommitSequenceId(bcsId) .setDeleteTransactionId(0) .setReplicaIndex(replicaIndex) + .setIsEmpty(isEmpty) .build(); return crBuilder.addReports(replicaProto).build(); } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManager.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManager.java index e9af42469784..e48c8f1316dd 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerStateManager.java +++ b/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,16 +166,22 @@ 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()); } - @Test - public void testTransitionDeletingToClosedStateAllowsOnlyDeletingContainer() throws IOException { + @ParameterizedTest + @EnumSource(value = HddsProtos.LifeCycleState.class, + names = {"CLOSING", "QUASI_CLOSED", "CLOSED", "RECOVERING"}) + public void testTransitionContainerToClosedStateAllowOnlyDeletingOrDeletedContainer( + HddsProtos.LifeCycleState initialState) throws IOException { + // Negative test for non-OPEN Ratis container -> CLOSED transitions. OPEN -> CLOSED is tested in: + // TestContainerManagerImpl#testTransitionContainerToClosedStateAllowOnlyDeletingOrDeletedContainers + HddsProtos.ContainerInfoProto.Builder builder = HddsProtos.ContainerInfoProto.newBuilder(); builder.setContainerID(1) - .setState(HddsProtos.LifeCycleState.QUASI_CLOSED) + .setState(initialState) .setUsedBytes(0) .setNumberOfKeys(0) .setOwner("root") @@ -181,7 +192,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()); diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationTestUtil.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationTestUtil.java index 0d66dbd4e415..d3ee327bc431 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationTestUtil.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationTestUtil.java @@ -89,6 +89,16 @@ public static Set createReplicas(ContainerID containerID, return createReplicas(containerID, CLOSED, indexes); } + public static Set createEmptyReplicas(ContainerID containerID, + ContainerReplicaProto.State replicaState, int... indexes) { + Set replicas = new HashSet<>(); + for (int i : indexes) { + replicas.add(createEmptyContainerReplica( + containerID, i, IN_SERVICE, replicaState)); + } + return replicas; + } + public static Set createReplicas(ContainerID containerID, ContainerReplicaProto.State replicaState, int... indexes) { Set replicas = new HashSet<>(); @@ -124,6 +134,15 @@ public static Set createReplicasWithSameOrigin( } return replicas; } + public static ContainerReplica createEmptyContainerReplica(ContainerID containerID, + int replicaIndex, HddsProtos.NodeOperationalState opState, + ContainerReplicaProto.State replicaState) { + DatanodeDetails datanodeDetails + = MockDatanodeDetails.randomDatanodeDetails(); + return createContainerReplica(containerID, replicaIndex, opState, + replicaState, 0L, 0L, + datanodeDetails, datanodeDetails.getUuid()); + } public static ContainerReplica createContainerReplica(ContainerID containerID, int replicaIndex, HddsProtos.NodeOperationalState opState, diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java index 1b18e6eaa4ff..10aadc687ca8 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestDeletingContainerHandler.java @@ -205,7 +205,7 @@ public void testResendDeleteCommand() throws NotLeaderException { ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo( ratisReplicationConfig, 1, DELETING); Set containerReplicas = ReplicationTestUtil - .createReplicas(containerInfo.containerID(), + .createEmptyReplicas(containerInfo.containerID(), ContainerReplicaProto.State.CLOSED, 0, 0, 0); List pendingOps = new ArrayList<>(); containerReplicas.stream().limit(2).forEach(replica -> pendingOps.add( @@ -217,7 +217,7 @@ public void testResendDeleteCommand() throws NotLeaderException { containerInfo = ReplicationTestUtil.createContainerInfo( ecReplicationConfig, 1, DELETING); containerReplicas = ReplicationTestUtil - .createReplicas(containerInfo.containerID(), + .createEmptyReplicas(containerInfo.containerID(), ContainerReplicaProto.State.CLOSED, 1, 2, 3, 4, 5); pendingOps.clear(); containerReplicas.stream().limit(3).forEach(replica -> pendingOps.add( @@ -229,6 +229,25 @@ public void testResendDeleteCommand() throws NotLeaderException { } + @Test + public void testDeleteCommandIsNotSentForNonEmptyReplica() throws NotLeaderException { + // Ratis container + ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo( + ratisReplicationConfig, 1, DELETING); + Set containerReplicas = ReplicationTestUtil + .createReplicas(containerInfo.containerID(), + ContainerReplicaProto.State.CLOSED, 0, 0, 0); + verifyDeleteCommandCount(containerInfo, containerReplicas, Collections.emptyList(), 0); + + // EC container + containerInfo = ReplicationTestUtil.createContainerInfo( + ecReplicationConfig, 1, DELETING); + containerReplicas = ReplicationTestUtil + .createReplicas(containerInfo.containerID(), + ContainerReplicaProto.State.CLOSED, 1, 2, 3, 4, 5); + verifyDeleteCommandCount(containerInfo, containerReplicas, Collections.emptyList(), 0); + } + private void verifyDeleteCommandCount(ContainerInfo containerInfo, Set containerReplicas, List pendingOps, diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandling.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandling.java index b639d98e9228..6613b246d82a 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandling.java +++ b/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 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); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandlingWithHA.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandlingWithHA.java index 2d3c9a2bf491..55039a38cc36 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/TestContainerReportHandlingWithHA.java +++ b/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 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);