diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisContainerReplicaCount.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisContainerReplicaCount.java index 4b09259dac72..e04740ab30f8 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisContainerReplicaCount.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisContainerReplicaCount.java @@ -22,6 +22,8 @@ import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto; import org.apache.hadoop.hdds.scm.container.ContainerInfo; import org.apache.hadoop.hdds.scm.container.ContainerReplica; +import org.apache.hadoop.hdds.scm.container.replication.ContainerHealthResult.OverReplicatedHealthResult; +import org.apache.hadoop.hdds.scm.container.replication.ContainerHealthResult.UnderReplicatedHealthResult; import java.util.ArrayList; import java.util.Comparator; @@ -34,6 +36,7 @@ import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState.DECOMMISSIONING; import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState.ENTERING_MAINTENANCE; import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState.IN_MAINTENANCE; +import static org.apache.hadoop.hdds.scm.container.replication.ReplicationManager.compareState; /** * Immutable object that is created with a set of ContainerReplica objects and @@ -50,6 +53,8 @@ public class RatisContainerReplicaCount implements ContainerReplicaCount { private int matchingReplicaCount; private int decommissionCount; private int maintenanceCount; + private int unhealthyDecommissionCount; + private int unhealthyMaintenanceCount; private int inFlightAdd; private int inFlightDel; private final int repFactor; @@ -63,12 +68,6 @@ public RatisContainerReplicaCount(ContainerInfo container, int inFlightAdd, int inFlightDelete, int replicationFactor, int minHealthyForMaintenance) { - this.unhealthyReplicaCount = 0; - this.healthyReplicaCount = 0; - this.misMatchedReplicaCount = 0; - this.matchingReplicaCount = 0; - this.decommissionCount = 0; - this.maintenanceCount = 0; this.inFlightAdd = inFlightAdd; this.inFlightDel = inFlightDelete; this.repFactor = replicationFactor; @@ -83,29 +82,7 @@ public RatisContainerReplicaCount(ContainerInfo container, = Math.min(this.repFactor, minHealthyForMaintenance); this.container = container; - for (ContainerReplica cr : this.replicas) { - HddsProtos.NodeOperationalState state = - cr.getDatanodeDetails().getPersistedOpState(); - if (state == DECOMMISSIONED || state == DECOMMISSIONING) { - decommissionCount++; - } else if (state == IN_MAINTENANCE || state == ENTERING_MAINTENANCE) { - maintenanceCount++; - } else if (cr.getState() == ContainerReplicaProto.State.UNHEALTHY) { - unhealthyReplicaCount++; - } else if (!ReplicationManager.compareState(container.getState(), - cr.getState())) { - /* - Replica's state is not UNHEALTHY, but it doesn't match the - container's state. For example, a CLOSING replica of a CLOSED container. - */ - healthyReplicaCount++; - misMatchedReplicaCount++; - } else { - // replica's state exactly matches container's state - healthyReplicaCount++; - matchingReplicaCount++; - } - } + countReplicas(); } public RatisContainerReplicaCount(ContainerInfo containerInfo, @@ -126,15 +103,6 @@ public RatisContainerReplicaCount(ContainerInfo containerInfo, = Math.min(this.repFactor, minHealthyForMaintenance); this.considerUnhealthy = considerUnhealthy; - this.unhealthyReplicaCount = 0; - this.healthyReplicaCount = 0; - this.misMatchedReplicaCount = 0; - this.matchingReplicaCount = 0; - this.decommissionCount = 0; - this.maintenanceCount = 0; - this.inFlightAdd = 0; - this.inFlightDel = 0; - // collect DNs that have UNHEALTHY replicas Set unhealthyReplicaDNs = new HashSet<>(); for (ContainerReplica r : replicas) { @@ -162,27 +130,37 @@ public RatisContainerReplicaCount(ContainerInfo containerInfo, } } - for (ContainerReplica cr : this.replicas) { + countReplicas(); + } + + private void countReplicas() { + for (ContainerReplica cr : replicas) { HddsProtos.NodeOperationalState state = cr.getDatanodeDetails().getPersistedOpState(); + boolean unhealthy = + cr.getState() == ContainerReplicaProto.State.UNHEALTHY; + if (state == DECOMMISSIONED || state == DECOMMISSIONING) { - decommissionCount++; + if (unhealthy) { + unhealthyDecommissionCount++; + } else { + decommissionCount++; + } } else if (state == IN_MAINTENANCE || state == ENTERING_MAINTENANCE) { - maintenanceCount++; - } else if (cr.getState() == ContainerReplicaProto.State.UNHEALTHY) { + if (unhealthy) { + unhealthyMaintenanceCount++; + } else { + maintenanceCount++; + } + } else if (unhealthy) { unhealthyReplicaCount++; - } else if (!ReplicationManager.compareState(container.getState(), - cr.getState())) { - /* - Replica's state is not UNHEALTHY, but it doesn't match the - container's state. For example, a CLOSING replica of a CLOSED container. - */ - healthyReplicaCount++; - misMatchedReplicaCount++; } else { - // replica's state exactly matches container's state healthyReplicaCount++; - matchingReplicaCount++; + if (compareState(container.getState(), cr.getState())) { + matchingReplicaCount++; + } else { + misMatchedReplicaCount++; + } } } } @@ -200,11 +178,13 @@ public RatisContainerReplicaCount(ContainerInfo containerInfo, * Total healthy replicas = 3 = 1 matching + 2 mismatched replicas */ public int getHealthyReplicaCount() { - return healthyReplicaCount + healthyReplicaCountAdapter(); + return healthyReplicaCount + healthyReplicaCountAdapter() + + decommissionCount + maintenanceCount; } public int getUnhealthyReplicaCount() { - return unhealthyReplicaCount + getUnhealthyReplicaCountAdapter(); + return unhealthyReplicaCount + getUnhealthyReplicaCountAdapter() + + unhealthyDecommissionCount + unhealthyMaintenanceCount; } protected int getUnhealthyReplicaCountAdapter() { @@ -220,11 +200,11 @@ public int getMatchingReplicaCount() { } private int getAvailableReplicas() { + int available = healthyReplicaCount + healthyReplicaCountAdapter(); if (considerUnhealthy) { - return getHealthyReplicaCount() + getUnhealthyReplicaCount(); - } else { - return getHealthyReplicaCount(); + available += unhealthyReplicaCount + getUnhealthyReplicaCountAdapter(); } + return available; } /** @@ -241,12 +221,16 @@ protected int healthyReplicaCountAdapter() { @Override public int getDecommissionCount() { - return decommissionCount; + return considerUnhealthy + ? decommissionCount + unhealthyDecommissionCount + : decommissionCount; } @Override public int getMaintenanceCount() { - return maintenanceCount; + return considerUnhealthy + ? maintenanceCount + unhealthyMaintenanceCount + : maintenanceCount; } public int getReplicationFactor() { @@ -265,16 +249,20 @@ public List getReplicas() { @Override public String toString() { - return "Container State: " + container.getState() + + String result = "Container State: " + container.getState() + " Replica Count: " + replicas.size() + - " Healthy Count: " + healthyReplicaCount + - " Unhealthy Count: " + unhealthyReplicaCount + - " Decommission Count: " + decommissionCount + - " Maintenance Count: " + maintenanceCount + - " inFlightAdd Count: " + inFlightAdd + - " inFightDel Count: " + inFlightDel + + " Healthy (I/D/M): " + healthyReplicaCount + + "/" + decommissionCount + "/" + maintenanceCount + + " Unhealthy (I/D/M): " + unhealthyReplicaCount + + "/" + unhealthyDecommissionCount + "/" + unhealthyMaintenanceCount + + " inFlightAdd: " + inFlightAdd + + " inFightDel: " + inFlightDel + " ReplicationFactor: " + repFactor + - " minMaintenance Count: " + minHealthyForMaintenance; + " minMaintenance: " + minHealthyForMaintenance; + if (considerUnhealthy) { + result += " +considerUnhealthy"; + } + return result; } /** @@ -378,7 +366,7 @@ private int missingReplicas() { return delta; } else if (delta > 0) { // May be under-replicated, depending on maintenance. - delta = Math.max(0, delta - maintenanceCount); + delta = Math.max(0, delta - getMaintenanceCount()); int neededHealthy = Math.max(0, minHealthyForMaintenance - getAvailableReplicas()); delta = Math.max(neededHealthy, delta); @@ -520,16 +508,13 @@ private int redundancyDelta(boolean includePendingDelete, /** * Checks whether insufficient replication is because of some replicas * being on datanodes that were decommissioned. - * @param includePendingAdd if pending adds should be considered + * * @return true if there is insufficient replication and it's because of * decommissioning. */ - public boolean inSufficientDueToDecommission(boolean includePendingAdd) { - if (isSufficientlyReplicated(includePendingAdd)) { - return false; - } - int delta = redundancyDelta(true, includePendingAdd); - return decommissionCount >= delta; + private boolean inSufficientDueToDecommission() { + int delta = redundancyDelta(true, false); + return 0 < delta && delta <= getDecommissionCount(); } /** @@ -542,9 +527,10 @@ public boolean inSufficientDueToDecommission(boolean includePendingAdd) { * @return Count of remaining redundant replicas. */ public int getRemainingRedundancy() { - return Math.max(0, - getAvailableReplicas() + decommissionCount + maintenanceCount - - inFlightDel - 1); + int availableReplicas = getAvailableReplicas() + + getDecommissionCount() + getMaintenanceCount(); + + return Math.max(0, availableReplicas - inFlightDel - 1); } /** @@ -557,4 +543,28 @@ public int getRemainingRedundancy() { public boolean isUnrecoverable() { return getReplicas().isEmpty(); } + + public UnderReplicatedHealthResult toUnderHealthResult() { + UnderReplicatedHealthResult result = new UnderReplicatedHealthResult( + getContainer(), + getRemainingRedundancy(), + inSufficientDueToDecommission(), + isSufficientlyReplicated(true), + isUnrecoverable()); + result.setHasHealthyReplicas(getHealthyReplicaCount() > 0); + return result; + } + + public OverReplicatedHealthResult toOverHealthResult() { + OverReplicatedHealthResult result = new OverReplicatedHealthResult( + getContainer(), + getExcessRedundancy(false), + !isOverReplicated(true)); + result.setHasMismatchedReplicas(getMisMatchedReplicaCount() > 0); + // FIXME not used in RatisReplicationCheckHandler: OK? + result.setIsSafelyOverReplicated(isSafelyOverReplicated()); + return result; + + } + } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisReplicationCheckHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisReplicationCheckHandler.java index 468b72b4da63..5ee2fb3d86c4 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisReplicationCheckHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisReplicationCheckHandler.java @@ -187,14 +187,7 @@ public ContainerHealthResult checkHealth(ContainerCheckRequest request) { boolean sufficientlyReplicated = replicaCount.isSufficientlyReplicated(false); if (!sufficientlyReplicated) { - ContainerHealthResult.UnderReplicatedHealthResult result = - new ContainerHealthResult.UnderReplicatedHealthResult( - container, replicaCount.getRemainingRedundancy(), - replicaCount.inSufficientDueToDecommission(false), - replicaCount.isSufficientlyReplicated(true), - replicaCount.isUnrecoverable()); - result.setHasHealthyReplicas(replicaCount.getHealthyReplicaCount() > 0); - return result; + return replicaCount.toUnderHealthResult(); } /* @@ -209,14 +202,7 @@ of UNHEALTHY replicas (such as 3 CLOSED and 1 UNHEALTHY replicas of a minReplicasForMaintenance, true); boolean isOverReplicated = consideringUnhealthy.isOverReplicated(false); if (isOverReplicated) { - boolean repOkWithPending = !consideringUnhealthy.isOverReplicated(true); - ContainerHealthResult.OverReplicatedHealthResult result = - new ContainerHealthResult.OverReplicatedHealthResult( - container, consideringUnhealthy.getExcessRedundancy(false), - repOkWithPending); - result.setHasMismatchedReplicas( - consideringUnhealthy.getMisMatchedReplicaCount() > 0); - return result; + return consideringUnhealthy.toOverHealthResult(); } int requiredNodes = container.getReplicationConfig().getRequiredNodes(); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisUnhealthyReplicationCheckHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisUnhealthyReplicationCheckHandler.java index 26668c5dc9fa..8bc84cd8d136 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisUnhealthyReplicationCheckHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/RatisUnhealthyReplicationCheckHandler.java @@ -65,10 +65,7 @@ public boolean handle(ContainerCheckRequest request) { } // Now, consider UNHEALTHY replicas when calculating replication status - RatisContainerReplicaCount replicaCount = - new RatisContainerReplicaCount(container, - request.getContainerReplicas(), request.getPendingOps(), - request.getMaintenanceRedundancy(), true); + RatisContainerReplicaCount replicaCount = getReplicaCount(request); if (replicaCount.getUnhealthyReplicaCount() == 0) { LOG.debug("No UNHEALTHY replicas are present for container {} with " + "replicas [{}].", container, request.getContainerReplicas()); @@ -80,7 +77,7 @@ public boolean handle(ContainerCheckRequest request) { container.containerID()); } - ContainerHealthResult health = checkReplication(request); + ContainerHealthResult health = checkReplication(replicaCount); if (health.getHealthState() == ContainerHealthResult.HealthState.UNDER_REPLICATED) { ContainerHealthResult.UnderReplicatedHealthResult underHealth @@ -155,39 +152,29 @@ private boolean verifyPerfectReplication(ContainerCheckRequest request) { * Checks if the container is over or under replicated. */ @VisibleForTesting - protected ContainerHealthResult checkReplication( + ContainerHealthResult checkReplication(ContainerCheckRequest request) { + return checkReplication(getReplicaCount(request)); + } + + private static RatisContainerReplicaCount getReplicaCount( ContainerCheckRequest request) { - RatisContainerReplicaCount replicaCount = - new RatisContainerReplicaCount(request.getContainerInfo(), - request.getContainerReplicas(), request.getPendingOps(), - request.getMaintenanceRedundancy(), true); + return new RatisContainerReplicaCount(request.getContainerInfo(), + request.getContainerReplicas(), request.getPendingOps(), + request.getMaintenanceRedundancy(), true); + } + + private ContainerHealthResult checkReplication( + RatisContainerReplicaCount replicaCount) { boolean sufficientlyReplicated = replicaCount.isSufficientlyReplicated(false); if (!sufficientlyReplicated) { - ContainerHealthResult.UnderReplicatedHealthResult result = - new ContainerHealthResult.UnderReplicatedHealthResult( - replicaCount.getContainer(), - replicaCount.getRemainingRedundancy(), - replicaCount.inSufficientDueToDecommission(false), - replicaCount.isSufficientlyReplicated(true), - replicaCount.isUnrecoverable()); - result.setHasHealthyReplicas(replicaCount.getHealthyReplicaCount() > 0); - return result; + return replicaCount.toUnderHealthResult(); } boolean isOverReplicated = replicaCount.isOverReplicated(false); if (isOverReplicated) { - boolean repOkWithPending = !replicaCount.isOverReplicated(true); - ContainerHealthResult.OverReplicatedHealthResult result = - new ContainerHealthResult.OverReplicatedHealthResult( - replicaCount.getContainer(), - replicaCount.getExcessRedundancy(false), - repOkWithPending); - result.setHasMismatchedReplicas( - replicaCount.getMisMatchedReplicaCount() > 0); - result.setIsSafelyOverReplicated(replicaCount.isSafelyOverReplicated()); - return result; + return replicaCount.toOverHealthResult(); } return new ContainerHealthResult.UnHealthyResult( diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestRatisContainerReplicaCount.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestRatisContainerReplicaCount.java index ca8485243ea5..cee80925d04d 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestRatisContainerReplicaCount.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestRatisContainerReplicaCount.java @@ -515,10 +515,10 @@ void testUnhealthyReplicaOnDecommissionedNodeWithPendingDelete() { assertEquals(1, rcnt.getMisMatchedReplicaCount()); // CLOSED + CLOSED = 2 assertEquals(2, rcnt.getMatchingReplicaCount()); - // UNHEALTHY should be 0 because it is counted as decommissioned - assertEquals(0, rcnt.getUnhealthyReplicaCount()); - // 1 because the UNHEALTHY replica is on a decommissioned node - assertEquals(1, rcnt.getDecommissionCount()); + // UNHEALTHY decommissioned is counted, too + assertEquals(1, rcnt.getUnhealthyReplicaCount()); + // due to considerUnhealthy=false + assertEquals(0, rcnt.getDecommissionCount()); // Now, test by considering UNHEALTHY replicas rcnt = new RatisContainerReplicaCount(container, replicas, @@ -533,8 +533,9 @@ void testUnhealthyReplicaOnDecommissionedNodeWithPendingDelete() { assertEquals(1, rcnt.getMisMatchedReplicaCount()); // CLOSED + CLOSED = 2 assertEquals(2, rcnt.getMatchingReplicaCount()); - // UNHEALTHY should be 0 because it is counted as decommissioned - assertEquals(0, rcnt.getUnhealthyReplicaCount()); + // UNHEALTHY decommissioned is counted as unhealthy + assertEquals(1, rcnt.getUnhealthyReplicaCount()); + // due to considerUnhealthy=true assertEquals(1, rcnt.getDecommissionCount()); } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestRatisReplicationCheckHandler.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestRatisReplicationCheckHandler.java index 0107f4d19b71..e0605469a365 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestRatisReplicationCheckHandler.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestRatisReplicationCheckHandler.java @@ -22,6 +22,7 @@ import org.apache.hadoop.hdds.client.ReplicationConfig; import org.apache.hadoop.hdds.protocol.DatanodeDetails; import org.apache.hadoop.hdds.protocol.MockDatanodeDetails; +import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State; import org.apache.hadoop.hdds.scm.PlacementPolicy; import org.apache.hadoop.hdds.scm.container.ContainerInfo; @@ -216,6 +217,39 @@ public void testUnderReplicatedDueToDecommission() { ReplicationManagerReport.HealthState.UNDER_REPLICATED)); } + @Test + public void testUnderReplicatedDueToAllDecommissioning() { + Pair state = + Pair.of(DECOMMISSIONING, 0); + + ContainerInfo container = createContainerInfo(repConfig); + Set replicas = createReplicas(container.containerID(), + state, state, state); + + ContainerCheckRequest checkRequest = requestBuilder + .setContainerReplicas(replicas) + .setContainerInfo(container) + .build(); + + ContainerHealthResult healthResult = healthCheck.checkHealth(checkRequest); + Assert.assertEquals(HealthState.UNDER_REPLICATED, + healthResult.getHealthState()); + Assert.assertEquals(UnderReplicatedHealthResult.class, + healthResult.getClass()); + UnderReplicatedHealthResult result = (UnderReplicatedHealthResult) + healthResult; + + Assert.assertEquals(2, result.getRemainingRedundancy()); + Assert.assertFalse(result.isReplicatedOkAfterPending()); + Assert.assertTrue(result.underReplicatedDueToDecommission()); + + Assert.assertTrue(healthCheck.handle(checkRequest)); + Assert.assertEquals(1, repQueue.underReplicatedQueueSize()); + Assert.assertEquals(0, repQueue.overReplicatedQueueSize()); + Assert.assertEquals(1, report.getStat( + ReplicationManagerReport.HealthState.UNDER_REPLICATED)); + } + @Test public void testUnderReplicatedDueToDecommissionFixedWithPending() { ContainerInfo container = createContainerInfo(repConfig); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/node/TestDecommissionAndMaintenance.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/node/TestDecommissionAndMaintenance.java index fb23046ac79e..073e0e3bbd4e 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/node/TestDecommissionAndMaintenance.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/node/TestDecommissionAndMaintenance.java @@ -140,7 +140,6 @@ public static void init() { replicationConf.setInterval(Duration.ofSeconds(1)); replicationConf.setUnderReplicatedInterval(Duration.ofSeconds(1)); replicationConf.setOverReplicatedInterval(Duration.ofSeconds(1)); - replicationConf.setEnableLegacy(true); // disable as part of HDDS-8459 conf.setFromObject(replicationConf); MiniOzoneCluster.Builder builder = MiniOzoneCluster.newBuilder(conf)