HDDS-15657. excludeContainersDueToFailure resets every iteration, repeatedly failing containers keep being retried

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<ContainerID, DatanodeDetails> containerToSourceMap;
   private Set<ContainerID> excludeContainers;
   private Set<ContainerID> includeContainers;
-  private Set<ContainerID> excludeContainersDueToFailure;
+
+  // excludeContainersDueToFailure: Containers excluded for current iteration only.
+  // excludeContainersNotFound: Container exclusions persist across iterations.
+  private final Set<ContainerID> excludeContainersDueToFailure;
+  private final Set<ContainerID> excludeContainersNotFound;
   private FindSourceStrategy findSourceStrategy;
   private Map<DatanodeDetails, NavigableSet<ContainerID>> setMap;
 
@@ -66,12 +70,25 @@ public ContainerBalancerSelectionCriteria(
       ContainerManager containerManager,
       FindSourceStrategy findSourceStrategy,
       Map<ContainerID, DatanodeDetails> containerToSourceMap) {
+    this(balancerConfiguration, nodeManager, replicationManager, containerManager,
+        findSourceStrategy, containerToSourceMap, new HashSet<>());
+  }
+
+  public ContainerBalancerSelectionCriteria(
+      ContainerBalancerConfiguration balancerConfiguration,
+      NodeManager nodeManager,
+      ReplicationManager replicationManager,
+      ContainerManager containerManager,
+      FindSourceStrategy findSourceStrategy,
+      Map<ContainerID, DatanodeDetails> containerToSourceMap,
+      Set<ContainerID> 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<ContainerID> getExcludeDueToFailContainers() {
     return excludeContainersDueToFailure;
   }
 
+  public void addToExcludeNotFoundContainers(ContainerID container) {
+    this.excludeContainersNotFound.add(container);
+  }
+
+  Set<ContainerID> getExcludeNotFoundContainers() {
+    return excludeContainersNotFound;
+  }
+
   private NavigableSet<ContainerID> getCandidateContainers(DatanodeDetails node) {
     NavigableSet<ContainerID> newSet =
         new TreeSet<>(orderContainersByUsedBytes().reversed());
@@ -380,9 +408,9 @@ private NavigableSet<ContainerID> 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;

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<DatanodeDetails> selectedTargets;
   private Set<DatanodeDetails> selectedSources;
+  private final Set<ContainerID> excludeContainersNotFound = new HashSet<>();
   private FindTargetStrategy findTargetStrategy;
   private FindSourceStrategy findSourceStrategy;
   private Map<ContainerMoveSelection, CompletableFuture<MoveManager.MoveResult>>
@@ -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) {

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

@@ -21,13 +21,15 @@
 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;
 import static org.mockito.Mockito.anyString;
 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<ContainerID> 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<ContainerID> 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.