HDDS-7252. Polled source Datanodes are wrongly not re-considered for balancing in Container Balancer

[Tejaskriya]

What changes were proposed in this pull request?

The FindSourceGreedy class maintains a priority queue of source datanodes. This class is used to get the next source DN for balancing. According to current design, a datanode can be involved in multiple source->target pairings in one iteration of container balancer. When FindSourceGreedy#getNextCandidateSourceDataNode() is called during an iteration, it polls a DN from the queue. This DN is re queued when FindSourceGreedy#increaseSizeLeaving is called. Problem is that if ContainerBalancer#moveContainer returns false, then increaseSizeLeaving will not be called and the DN doesn't get added back. If the failure of move operation is due to some errors related to the container selected in that run, then we should be adding back the DN to the queue. That way in the next run, the source DN could still be considered.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7252

How was this patch tested?

Added unit test in TestContainerBalancerTask

[siddhantsangwan]

@Tejaskriya We were discussing how to test this change earlier. I came up with a bit of a hack to test it.

  @Test
  public void testSourceDatanodeAddedBack()
      throws NodeNotFoundException, IOException, IllegalContainerBalancerStateException,
      InvalidContainerBalancerConfigurationException, TimeoutException, InterruptedException {

    when(moveManager.move(any(ContainerID.class),
        any(DatanodeDetails.class),
        any(DatanodeDetails.class)))
        .thenReturn(CompletableFuture.completedFuture(MoveManager.MoveResult.REPLICATION_FAIL_NOT_EXIST_IN_SOURCE))
        .thenReturn(CompletableFuture.completedFuture(MoveManager.MoveResult.COMPLETED));
    balancerConfiguration.setThreshold(10);
    balancerConfiguration.setIterations(1);
    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);
    GenericTestUtils.waitFor(() -> ContainerBalancerTask.IterationResult.ITERATION_COMPLETED ==
        containerBalancerTask.getIterationResult(), 10, 50);

    assertEquals(2, containerBalancerTask.getCountDatanodesInvolvedPerIteration());
    assertTrue(containerBalancerTask.getMetrics().getNumContainerMovesCompletedInLatestIteration() >= 1);
    assertThat(containerBalancerTask.getMetrics().getNumContainerMovesFailed()).isEqualTo(1);
    stopBalancer();
  }

It uses the includeNodes configuration to ensure only one under and one over utilized node is involved in balancing. This way we're able to test whether the same source was or wasn't added back. What do you think?

[Tejaskriya]

@siddhantsangwan Thank you for finding the hack to test it! It works, I have added it in the unit tests.

@siddhantsangwan @symious Could you please review this updated PR? I have incorporated the suggested changes.

[Tejaskriya]

@siddhantsangwan Thank you for the review! I have made the changes suggested by you and addressed the remaining comments with appropriate code changes. Could you please take another look at the PR now?

[siddhantsangwan]

Holding off on merging while we're still discussing a point above.

[Tejaskriya]

@siddhantsangwan @sumitagrawl following our discussion today, I have reverted the latest commit. Now the container is excluded for all DNs as ContainerNotFoundException will come up for every DN that the container is present in. For replica related intermittent failures, the container is not excluded. I have also added some comments to mention the reason we are adding a source DN back and excluding containers. The merge conflicts are resolved. Could you please give it a final review?

[siddhantsangwan]

@Tejaskriya please check the failing tests.