HDDS-6940. EC: Skip the EC container for balancer

[siddhantsangwan]

What changes were proposed in this pull request?

Excluding EC containers from balancing by removing them from the set of Candidate containers selected in ContainerBalancerSelectionCriteria.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing UTs.

[myskov]

Wouldn't it be cleaner to move the try-catch block from lambda to the isECContainer() method?

[adoroszlai]

I agree with @myskov. What's more, we could merge these conditions that require a container lookup. Something like this:

    // single removeIf
    containerIDSet.removeIf(this::shouldBeExcluded);
    ...
  }

  boolean shouldBeExcluded(ContainerID id) {
    ContainerInfo container;
    try {
      container = containerManager.getContainer(id);
    } catch (ContainerNotFoundException e) {
      LOG.warn(...);
      return true;
    }

    return isClosed(container) || ... || isECContainer(container);
  }

[siddhantsangwan]

Great suggestions. Added.

[JacksonYao287]

@siddhantsangwan thanks for this patch. can you explain why we should skip EC container for now, and when can we take EC container into account?

[umamaheswararao]

@JacksonYao287 We have fully done the actual recovery work. We also relaxed placement checks in replication flows currently. Unless we have ready analysis balancer can work with current version EC RM, we can switch off EC container from balancer just to avoid, not to go in wrong way. ( similar to how we were returning from RM for EC containers until before we started offline recovery work)

I see you have another patch where you are working to make Balancer work with EC. If you tested and works with balancer, we can remove the flag and enable it.

[JacksonYao287]

@umamaheswararao there are two steps when balancing container:
1 containerBalancer will select a candidate container from a source over-utilized Dn, and find an under-utilized Dn as a target
2 call legacyRm#move to take this move action.

the only difference for EC container in step 1 is the placement policy, this is what is done in HDDS-6533, and i think it has nothing to do with RM. we can merge is first.

for EC container, we can just skip move in legacy in RM until we complete the RM related work.

@siddhantsangwan

if container balancer will select an EC container but the move will fail since we skip it now. so theoretically，there might be a scenario that an EC container might always be selected for move ,but move always fail and the utilization of the datanode will never change, which will lead to Infinite loop。 please notice this. we can fix this after RM related work is completed

[siddhantsangwan]

Thanks for the reviews.

if container balancer will select an EC container but the move will fail since we skip it now. so theoretically，there might be a scenario that an EC container might always be selected for move ,but move always fail and the utilization of the datanode will never change, which will lead to Infinite loop。 please notice this. we can fix this after RM related work is completed

To avoid this scenario, we can merge this PR to exclude EC containers from balancing for now. We can then proceed with #3455 and finally include EC containers (by undoing the code in this PR) once steps 1 and 2 are complete. What do you all think?

[JacksonYao287]

@siddhantsangwan i am ok with your proposal. lets skip EC container at both containerBalancer side and RM side until we complete refactoring RM

[umamaheswararao]

Thank you @siddhantsangwan and @JacksonYao287 for the discussion and coming to a conclusion.

[adoroszlai]

Thanks @siddhantsangwan for updating the patch, LGTM.

[siddhantsangwan]

@myskov @JacksonYao287 Can you also review the latest changes please?

[umamaheswararao]

I have just merged this as we have received approval. I have overlooked the above question from @siddhantsangwan sorry. Feel free to file a followup if you have any points to cover.