Skip to content

HDDS-12428. Avoid force closing OPEN/CLOSING replica of a CLOSED Container #7985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nandakumar131 merged 4 commits into from
Mar 6, 2025
Merged

HDDS-12428. Avoid force closing OPEN/CLOSING replica of a CLOSED Container #7985

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d70d19
HDDS-12428. Avoid force closing OPEN/CLOSING replica of a CLOSED Cont…
swamirishi Feb 28, 2025
aa8a8c2
Unit test added to verify the behaviour
nandakumar131 Feb 28, 2025
721d912
HDDS-12428. Quasi close on Ratis containers and leave out EC containers
swamirishi Mar 6, 2025
c1393a9
Update hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/sc…
swamirishi Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 30 additions & 36 deletions ...va/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.hadoop.hdds.scm.container.replication.health;

import java.util.Set;
import java.util.function.Predicate;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
import org.apache.hadoop.hdds.scm.container.ContainerInfo;
Expand Down Expand Up @@ -57,35 +58,28 @@ public MismatchedReplicasHandler(
*/
@Override
public boolean handle(ContainerCheckRequest request) {
ContainerInfo containerInfo = request.getContainerInfo();
Set<ContainerReplica> replicas = request.getContainerReplicas();
if (request.isReadOnly()) {
return false;
}

final ContainerInfo containerInfo = request.getContainerInfo();
final Set<ContainerReplica> replicas = request.getContainerReplicas();

if (containerInfo.getState() != HddsProtos.LifeCycleState.CLOSED &&
containerInfo.getState() != HddsProtos.LifeCycleState.QUASI_CLOSED) {
// Handler is only relevant for CLOSED or QUASI-CLOSED containers.
return false;
}
LOG.debug("Checking container {} in MismatchedReplicasHandler",
containerInfo);

if (request.isReadOnly()) {
return false;
}
// close replica if needed
for (ContainerReplica replica : replicas) {
if (shouldBeClosed(containerInfo, replica)) {
LOG.debug("Sending close command for mismatched replica {} of " +
"container {}.", replica, containerInfo);
LOG.debug("Checking container {} in MismatchedReplicasHandler", containerInfo);

if (containerInfo.getState() == HddsProtos.LifeCycleState.CLOSED) {
replicationManager.sendCloseContainerReplicaCommand(
containerInfo, replica.getDatanodeDetails(), true);
} else if (containerInfo.getState() ==
HddsProtos.LifeCycleState.QUASI_CLOSED) {
replicationManager.sendCloseContainerReplicaCommand(
containerInfo, replica.getDatanodeDetails(), false);
}
}
}
final Predicate<ContainerReplica> shouldSendClose = (r) -> shouldSendClose(containerInfo, r);

replicas.stream().filter(shouldSendClose).forEach(r -> {
LOG.debug("Sending close command for mismatched replica {} of container {}.", r, containerInfo);
Comment thread
sumitagrawl marked this conversation as resolved.
Outdated
replicationManager.sendCloseContainerReplicaCommand(
containerInfo, r.getDatanodeDetails(), shouldForceClose(containerInfo, r));

@siddhantsangwan siddhantsangwan Feb 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to be clear, we're also sending a force close if the replica is open/closing and the bcsid matches. I think this is different from the previous commit's behaviour.

@nandakumar131 nandakumar131 Feb 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if the BCSID matches we are sending force close command to that Container replica.

This is not different from the previous behaviour, we were sending force close for open/closing replica even without checking the sequence Id. With this change we will only send force close if the sequence Id matches.

We believe that the BCSID of a CLOSED container is the highest (correct).

@siddhantsangwan siddhantsangwan Feb 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I meant it's different from the behaviour that @swamirishi had pushed in his commit. Your latest one looks fine to me, but @swamirishi should take a look.

@swamirishi swamirishi Feb 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nandakumar131 Lets not send a force close directly from OPEN/CLOSING. Let us move it to QUASI_CLOSED and let the QUASI_CLOSED container handler do it. Let us not have duplicate coding logic floating around.

@errose28 errose28 Mar 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should look at this in context with the datanode flow. A non-force close to an open or closing replica will still try to use the pipeline if it is available. Only if that fails on the DN will it quasi-close the container. Then we can invoke the force flow from SCM. Since we are checking the BCSID to decide whether to force close or not, we don't necessarily need the pipeline though.

I think both approaches give the correct result. However I think it makes sense to at least try a non-force close in case the pipeline is still there and let the DN tell us if force is required by moving the replica to quasi-closed. Otherwise it feels like we skipped a step.

@swamirishi swamirishi Mar 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i agree with you @errose28

@nandakumar131 nandakumar131 Mar 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same handler is used for EC Containers as well and for EC Containers the Datanode expects force flag to be set for Replicas in OPEN/CLOSING state.

We should add logic based on Container type.

If everyone feels strongly about this, I can change the behaviour for Ratis Containers.

});

/*
This handler is unique because it always returns false. This allows
Expand All @@ -95,23 +89,23 @@ public boolean handle(ContainerCheckRequest request) {
}

/**
* If a CLOSED or QUASI-CLOSED container has an OPEN or CLOSING replica,
* there is a state mismatch. QUASI_CLOSED replica of a CLOSED container
* should be closed if their sequence IDs match.
* @param replica replica to check for mismatch and if it should be closed
* @return true if the replica should be closed, else false
* Returns true if the replica state doesn't match the container state and the replica can be
* QUASI_CLOSED/CLOSED.
*
* This method only works for QUASI_CLOSED/CLOSED Containers.
*/
private boolean shouldBeClosed(ContainerInfo container,
ContainerReplica replica) {
if (replica.getState() == ContainerReplicaProto.State.OPEN ||
replica.getState() == ContainerReplicaProto.State.CLOSING) {
return true;
}
private boolean shouldSendClose(final ContainerInfo container, final ContainerReplica replica) {
return replica.getState() == ContainerReplicaProto.State.OPEN ||
replica.getState() == ContainerReplicaProto.State.CLOSING ||
(replica.getState() == ContainerReplicaProto.State.QUASI_CLOSED &&
shouldForceClose(container, replica));
}

// a quasi closed replica of a closed container should be closed if their
// sequence IDs match
/**
* Retruns true if the Container is CLOSED but the Replica is not, and the Sequence Id matches.
*/
private boolean shouldForceClose(final ContainerInfo container, final ContainerReplica replica) {
return container.getState() == HddsProtos.LifeCycleState.CLOSED &&
replica.getState() == ContainerReplicaProto.State.QUASI_CLOSED &&
container.getSequenceId() == replica.getSequenceId();
}
}
48 changes: 48 additions & 0 deletions ...rg/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,4 +326,52 @@ public void testQuasiClosedReplicaOfClosedContainer() {
verify(replicationManager, times(0)).sendCloseContainerReplicaCommand(containerInfo,
differentSeqID.getDatanodeDetails(), true);
}

@Test
public void testCloseCommandSentForMismatchedRatisReplicasWithIncorrectBCSID() {
ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
ratisReplicationConfig, 1, CLOSED, 1000);
ContainerReplica mismatch1 = ReplicationTestUtil.createContainerReplica(
containerInfo.containerID(), 0,
HddsProtos.NodeOperationalState.IN_SERVICE,
ContainerReplicaProto.State.OPEN, 99);
ContainerReplica mismatch2 = ReplicationTestUtil.createContainerReplica(
containerInfo.containerID(), 0,
HddsProtos.NodeOperationalState.IN_SERVICE,
ContainerReplicaProto.State.CLOSING, 999);
ContainerReplica mismatch3 = ReplicationTestUtil.createContainerReplica(
containerInfo.containerID(), 0,
HddsProtos.NodeOperationalState.IN_SERVICE,
ContainerReplicaProto.State.QUASI_CLOSED, 1000);
Set<ContainerReplica> containerReplicas = new HashSet<>();
containerReplicas.add(mismatch1);
containerReplicas.add(mismatch2);
containerReplicas.add(mismatch3);
ContainerCheckRequest request = new ContainerCheckRequest.Builder()
.setPendingOps(Collections.emptyList())
.setReport(new ReplicationManagerReport())
.setContainerInfo(containerInfo)
.setContainerReplicas(containerReplicas)
.build();
ContainerCheckRequest readRequest = new ContainerCheckRequest.Builder()
.setPendingOps(Collections.emptyList())
.setReport(new ReplicationManagerReport())
.setContainerInfo(containerInfo)
.setContainerReplicas(containerReplicas)
.setReadOnly(true)
.build();

// this handler always returns false so other handlers can fix issues
// such as under replication
assertFalse(handler.handle(request));
assertFalse(handler.handle(readRequest));

verify(replicationManager, times(1)).sendCloseContainerReplicaCommand(
containerInfo, mismatch1.getDatanodeDetails(), false);
verify(replicationManager, times(1)).sendCloseContainerReplicaCommand(
containerInfo, mismatch2.getDatanodeDetails(), false);
// close command should not be sent for unhealthy replica
verify(replicationManager, times(1)).sendCloseContainerReplicaCommand(
containerInfo, mismatch3.getDatanodeDetails(), true);
}
}