HDDS-10715. Remove Decommision nodes on replication

[symious]

What changes were proposed in this pull request?

For containers with insufficient replicas, new target will be chose for the replication.

For decommissioned nodes, although it will be excluded at last, but it will waste the "retry count", by default it's 3, which will cause containers can not be replicated when a cluster has many nodes in decommission state.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing tests.

[sodonnel]

My first thought on this, is that if the placement policy already excludes decommissioning nodes via the isValidateNode method at the end, could we simply add all decommissioning nodes (and we probably need to include manintenance nodes too) to the exclude list at the start in the placement policy. That way it would fix it for all callers in one place.

Eg we already have this as the entry point in SCMCommonPlacementPolicy and it has access to NodeManager I think, so we could add to the exclude list right at the start:

  @Override
  public final List<DatanodeDetails> chooseDatanodes(
          List<DatanodeDetails> usedNodes,
          List<DatanodeDetails> excludedNodes,
          List<DatanodeDetails> favoredNodes,
          int nodesRequired, long metadataSizeRequired, long dataSizeRequired)
          throws SCMException {
/*
  This method calls the chooseDatanodeInternal after fixing
  the excludeList to get the DatanodeDetails from the node manager.
  When the object of the Class DataNodeDetails is built from protobuf
  only UUID of the datanode is added which is used for the hashcode.
  Thus not passing any information about the topology. While excluding
  datanodes the object is built from protobuf @Link {ExcludeList.java}.
  NetworkTopology removes all nodes from the list which does not fall under
  the scope while selecting a random node. Default scope value is
  "/default-rack/" which won't match the required scope. Thus passing the proper
  object of DatanodeDetails(with Topology Information) while trying to get the
  random node from NetworkTopology should fix this. Check HDDS-7015
 */
    return chooseDatanodesInternal(validateDatanodes(usedNodes),
            validateDatanodes(excludedNodes), favoredNodes, nodesRequired,
            metadataSizeRequired, dataSizeRequired);
  }

[symious]

@sodonnel IMHO, expanding the excludedNodes list within the implementation of the chooseDatanodes method may indeed deviate from the original intent of the interface. Modifying these parameters in the implementation could confuse users of the interface, as they generally do not expect the parameters they pass to be altered.

[sodonnel]

But it is wrong for the placement policy to return a decommissioning node. It does indeed filter out any decommission nodes it finds and retries. So it is "removing them" but in a sub-optimal way.

The caller should not need to know all illegal nodes it needs to pass into the placement policy. What if there is another illegal node type in the future? Then we have to modified all the callers, rather than a single place.

You are probably correct that it is not good to modify the passed parameter list, but we can copy it into a new list that is used inside the placement policy and add to that copy.

[sodonnel]

I think there are other places in the code where the placement policies are called too. Eg pipeline creation, which could run into the same sort of problems I think. If we fix it inside the placement policy then it covers all existing scenarios.

[symious]

I think there are other places in the code where the placement policies are called too. Eg pipeline creation, which could run into the same sort of problems I think. If we fix it inside the placement policy then it covers all existing scenarios.

@sodonnel Agreed. Updated the PR, PTAL.

[sodonnel]

We probably need to included the two maintenance states too, which means 4 iterations across all nodes in the cluster to find them. As this is a somewhat hot path, especially for EC pipeline allocation I wonder if this will hurt performance on larger clusters.

Partly this is due to the NodeManager interface - there is no method to find all nodes not in_service in a single iteration, which is really what we want.

At the end of the selection, we have to check if the nodes are still good, have enough space etc.

I wonder if we could do something smarter with the retry count to make it try more times on a larger cluster?

[symious]

@sodonnel PTAL.

[sodonnel]

This new method does not address the performance concern I had. It is basically calling the original getNodes() method for each of the 4 out of service states. Each of those calls has to iterate all nodes in the cluster and return a set of the ones which are out of service.

The nodes picked by the policy have to be checked again before they are returned. While I originally suggested this solution, I am not sure it is a good one. It may be better to look at the retry count, and allow more retries if the failure reason is that the node is not in service. Or have a larger retry count if the cluster is large etc. At least then, the common case, which is no nodes out of service, does not pay a performance penalty on every call.

[symious]

@sodonnel IMHO, the current changes should be a long-term solution, the change of maxRetries is currently being used in our cluster, but not a good way when the cluster is getting bigger.

For the getNodes part, we should improve the performance for the general usages.

[sumitagrawl]

@symious Thanks for working over this, have minor comment

[sumitagrawl]

unavailableNodes is not used, just its count. so also can get unavailableCount = usedNodesCount + excludedNodesCount
currently, unionOfCollection seems un-necessary for this case