diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java index c72cf5f03467..efe246a11b57 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerSelectionCriteria.java @@ -55,7 +55,11 @@ public class ContainerBalancerSelectionCriteria { private Map containerToSourceMap; private Set excludeContainers; private Set includeContainers; - private Set excludeContainersDueToFailure; + + // excludeContainersDueToFailure: Containers excluded for current iteration only. + // excludeContainersNotFound: Container exclusions persist across iterations. + private final Set excludeContainersDueToFailure; + private final Set excludeContainersNotFound; private FindSourceStrategy findSourceStrategy; private Map> setMap; @@ -66,12 +70,25 @@ public ContainerBalancerSelectionCriteria( ContainerManager containerManager, FindSourceStrategy findSourceStrategy, Map containerToSourceMap) { + this(balancerConfiguration, nodeManager, replicationManager, containerManager, + findSourceStrategy, containerToSourceMap, new HashSet<>()); + } + + public ContainerBalancerSelectionCriteria( + ContainerBalancerConfiguration balancerConfiguration, + NodeManager nodeManager, + ReplicationManager replicationManager, + ContainerManager containerManager, + FindSourceStrategy findSourceStrategy, + Map containerToSourceMap, + Set excludeContainersNotFound) { this.balancerConfiguration = balancerConfiguration; this.nodeManager = nodeManager; this.replicationManager = replicationManager; this.containerManager = containerManager; this.containerToSourceMap = containerToSourceMap; - excludeContainersDueToFailure = new HashSet<>(); + this.excludeContainersDueToFailure = new HashSet<>(); + this.excludeContainersNotFound = excludeContainersNotFound; excludeContainers = balancerConfiguration.getExcludeContainers(); includeContainers = balancerConfiguration.getIncludeContainers(); this.findSourceStrategy = findSourceStrategy; @@ -175,11 +192,13 @@ public boolean shouldBeExcluded(ContainerID containerID, } catch (ContainerNotFoundException e) { LOG.warn("Could not find Container {} to check if it should be a " + "candidate container. Excluding it.", containerID); + excludeContainersNotFound.add(containerID); return true; } if (excludeContainers.contains(containerID) || excludeContainersDueToFailure.contains(containerID) || + excludeContainersNotFound.contains(containerID) || containerToSourceMap.containsKey(containerID) || !findSourceStrategy.canSizeLeaveSource(node, container.getUsedBytes()) || breaksMaxSizeToMoveLimit(container.containerID(), @@ -193,6 +212,7 @@ public boolean shouldBeExcluded(ContainerID containerID, } catch (ContainerNotFoundException e) { LOG.warn("Container {} does not exist in ContainerManager. Skipping " + "this container.", container.getContainerID(), e); + excludeContainersNotFound.add(containerID); return true; } @@ -369,6 +389,14 @@ Set getExcludeDueToFailContainers() { return excludeContainersDueToFailure; } + public void addToExcludeNotFoundContainers(ContainerID container) { + this.excludeContainersNotFound.add(container); + } + + Set getExcludeNotFoundContainers() { + return excludeContainersNotFound; + } + private NavigableSet getCandidateContainers(DatanodeDetails node) { NavigableSet newSet = new TreeSet<>(orderContainersByUsedBytes().reversed()); @@ -380,9 +408,9 @@ private NavigableSet getCandidateContainers(DatanodeDetails node) { if (excludeContainers != null) { idSet.removeAll(excludeContainers); } - if (excludeContainersDueToFailure != null) { - idSet.removeAll(excludeContainersDueToFailure); - } + idSet.removeAll(excludeContainersDueToFailure); + idSet.removeAll(excludeContainersNotFound); + idSet.removeAll(containerToSourceMap.keySet()); newSet.addAll(idSet); return newSet; diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java index 38554253bab3..a7f428d35c89 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java @@ -108,6 +108,7 @@ public class ContainerBalancerTask implements Runnable { private Set selectedTargets; private Set selectedSources; + private final Set excludeContainersNotFound = new HashSet<>(); private FindTargetStrategy findTargetStrategy; private FindSourceStrategy findSourceStrategy; private Map> @@ -635,7 +636,7 @@ private boolean initializeIteration() { selectionCriteria = new ContainerBalancerSelectionCriteria(config, nodeManager, replicationManager, containerManager, findSourceStrategy, - containerToSourceMap); + containerToSourceMap, excludeContainersNotFound); return true; } @@ -740,8 +741,8 @@ private boolean processMoveSelection(DatanodeDetails source, "starting a container move", containerID, e); // add source back to queue as a different container can be selected in next run. findSourceStrategy.addBackSourceDataNode(source); - // exclude the container which caused failure of move to avoid error in next run. - selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID()); + // exclude the permanently missing container across balancer iterations. + selectionCriteria.addToExcludeNotFoundContainers(moveSelection.getContainerID()); return false; } LOG.info("ContainerBalancer is trying to move container {} with size " + @@ -1032,8 +1033,8 @@ private boolean moveContainer(DatanodeDetails source, containerID, e); // add source back to queue as a different container can be selected in next run. findSourceStrategy.addBackSourceDataNode(source); - // exclude the container which caused failure of move to avoid error in next run. - selectionCriteria.addToExcludeDueToFailContainers(moveSelection.getContainerID()); + // exclude the permanently missing container across balancer iterations. + selectionCriteria.addToExcludeNotFoundContainers(moveSelection.getContainerID()); metrics.incrementNumContainerMovesFailedInLatestIteration(1); return false; } catch (NodeNotFoundException e) { diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java index bb407c9b9a80..605f28868d95 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java @@ -21,6 +21,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.anySet; import static org.mockito.Mockito.any; @@ -28,6 +29,7 @@ import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -63,6 +65,7 @@ import org.apache.hadoop.hdds.scm.container.ContainerID; import org.apache.hadoop.hdds.scm.container.ContainerInfo; import org.apache.hadoop.hdds.scm.container.ContainerManager; +import org.apache.hadoop.hdds.scm.container.ContainerNotFoundException; import org.apache.hadoop.hdds.scm.container.ContainerReplica; import org.apache.hadoop.hdds.scm.container.MockNodeManager; import org.apache.hadoop.hdds.scm.container.placement.algorithms.ContainerPlacementPolicyFactory; @@ -489,7 +492,104 @@ public void testConcurrentMoveCallbacksAccumulateMovedBytesAtomically() throws E moveCompletionExecutor.shutdownNow(); } } - + + @Test + public void testExcludeContainersNotFoundPersistsAcrossIterations() throws Exception { + DatanodeUsageInfo sourceNode = nodesInCluster.get(nodesInCluster.size() - 1); + ContainerManager containerManager = scm.getContainerManager(); + final ContainerID[] missingContainerId = new ContainerID[1]; + + // Container exists during candidate selection; after it is first selected for + // move, SCM can no longer find it (simulates container deleted mid-balance). + when(moveManager.move(any(ContainerID.class), any(DatanodeDetails.class), + any(DatanodeDetails.class))) + .thenAnswer(invocation -> { + ContainerID containerId = invocation.getArgument(0); + if (missingContainerId[0] == null) { + missingContainerId[0] = containerId; + doThrow(ContainerNotFoundException.newInstanceForTesting()) + .when(containerManager).getContainer(containerId); + doThrow(ContainerNotFoundException.newInstanceForTesting()) + .when(containerManager).getContainerReplicas(containerId); + cidToInfoMap.remove(containerId); + } + return CompletableFuture.completedFuture(MoveManager.MoveResult.COMPLETED); + }); + + balancerConfiguration.setThreshold(10); + balancerConfiguration.setIterations(2); + balancerConfiguration.setBalancingInterval(0); + balancerConfiguration.setMaxSizeEnteringTarget(10 * STORAGE_UNIT); + balancerConfiguration.setMaxSizeToMovePerIteration(100 * STORAGE_UNIT); + balancerConfiguration.setMaxDatanodesPercentageToInvolvePerIteration(100); + String includeNodes = nodesInCluster.get(0).getDatanodeDetails().getHostName() + "," + + sourceNode.getDatanodeDetails().getHostName(); + balancerConfiguration.setIncludeNodes(includeNodes); + + startBalancer(balancerConfiguration); + + assertEquals(2, containerBalancerTask.getMetrics().getNumIterations(), + "Balancer must complete both iterations"); + + ContainerID notFoundId = missingContainerId[0]; + assertNotNull(notFoundId, "Expected at least one container move"); + assertTrue(containerBalancerTask.getSelectionCriteria() + .getExcludeNotFoundContainers().contains(notFoundId), + "Missing container should be in the cross-iteration exclude set"); + + ArgumentCaptor containerCaptor = ArgumentCaptor.forClass(ContainerID.class); + verify(moveManager, atLeast(1)).move(containerCaptor.capture(), + any(DatanodeDetails.class), any(DatanodeDetails.class)); + + long moveAttempts = containerCaptor.getAllValues().stream() + .filter(id -> id.equals(notFoundId)) + .count(); + assertEquals(1, moveAttempts, + "Permanently missing container should not be retried in later iterations"); + } + + @Test + public void testTransientFailureNotPersistedAcrossIterations() throws Exception { + when(moveManager.move(any(ContainerID.class), any(DatanodeDetails.class), + any(DatanodeDetails.class))) + .thenReturn(CompletableFuture.completedFuture( + MoveManager.MoveResult.REPLICATION_NOT_HEALTHY_AFTER_MOVE)) + .thenReturn(CompletableFuture.completedFuture(MoveManager.MoveResult.COMPLETED)); + + balancerConfiguration.setThreshold(10); + balancerConfiguration.setIterations(2); + balancerConfiguration.setBalancingInterval(0); + balancerConfiguration.setMaxSizeEnteringTarget(10 * STORAGE_UNIT); + balancerConfiguration.setMaxSizeToMovePerIteration(100 * STORAGE_UNIT); + balancerConfiguration.setMaxDatanodesPercentageToInvolvePerIteration(100); + String includeNodes = nodesInCluster.get(0).getDatanodeDetails().getHostName() + "," + + nodesInCluster.get(nodesInCluster.size() - 1).getDatanodeDetails().getHostName(); + balancerConfiguration.setIncludeNodes(includeNodes); + + startBalancer(balancerConfiguration); + + assertEquals(2, containerBalancerTask.getMetrics().getNumIterations(), + "Balancer must complete both iterations"); + + ArgumentCaptor containerCaptor = ArgumentCaptor.forClass(ContainerID.class); + verify(moveManager, atLeast(1)).move(containerCaptor.capture(), + any(DatanodeDetails.class), any(DatanodeDetails.class)); + ContainerID failedContainerId = containerCaptor.getAllValues().get(0); + + // Transient failure must not land in the cross-iteration not-found set. + assertFalse(containerBalancerTask.getSelectionCriteria() + .getExcludeNotFoundContainers().contains(failedContainerId), + "Transient failure must not be treated as permanently missing"); + + // Iteration 2 should retry the same container because excludeDueToFailure + // is iteration-scoped and cleared between iterations. + long moveAttempts = containerCaptor.getAllValues().stream() + .filter(id -> id.equals(failedContainerId)) + .count(); + assertEquals(2, moveAttempts, + "Transient failure should be retried in the next iteration"); + } + /** * Generates a range of equally spaced utilization(that is, used / capacity) * values from 0 to 1.