[peterxcli]

What changes were proposed in this pull request?

SCM picks datanodes for pipelines and new replicas from its network topology. Two pieces of code change the topology for a datanode that is already registered:

• The health check (NodeStateManager#checkNodesHealth) removes a datanode from the topology when it marks the datanode DEAD.
• When a datanode registers again with a new IP address, hostname, ports or version, SCMNodeManager#register replaces the datanode's object in the topology. Then it replaces the datanode's DatanodeInfo.

These two run on different threads, and no lock keeps them apart. So they can interleave like this:

1. The health check marks the datanode DEAD. It is about to remove the datanode from the topology.
2. At that moment, the datanode registers again with new ports. The registration puts a new object for the datanode in the topology and marks the datanode HEALTHY.
3. The health check now removes the datanode from the topology. The topology removes a node by its name, so it removes the new object that the registration just added.

Now the datanode is HEALTHY, but it is not in the topology, so placement never picks it. It stays out until it becomes DEAD and recovers, or until SCM restarts. NetworkTopology#contains still returns true for it, because the datanode's parent pointer still points to the rack.

The other order breaks too. Say the registration has put the new object in the topology, but has not stored the new DatanodeInfo yet. If the health check marks the datanode DEAD at that moment, it removes the new object. Then the registration stores the HEALTHY DatanodeInfo, and the datanode ends up the same way.

The window is small: the datanode has to register again at the same moment SCM marks it DEAD.

With this change, the health check and the registration do not lock each other out, because the race is rare. Instead, each one checks for the other:

• The health check changes the health of a node only if the node has not changed since the check read it. NodeStateMap#updateNodeHealthState checks that the map still holds the same DatanodeInfo object, with the same health. A registration that changes the datanode puts a new DatanodeInfo in the map, so a decision made on the old one is skipped. The next health check decides again.
• NodeStateManager#updateNode now does the topology update, instead of SCMNodeManager#register. It puts the new DatanodeInfo itself in the topology, before it replaces the old one in the node state.
• The registration checks the topology after it has stored the new DatanodeInfo. If the topology does not hold the new DatanodeInfo, or holds the old one again, it updates the topology again.
• The health check looks at the node state after it has removed a DEAD node from the topology, or added back a node that recovered. If the node state now holds a new DatanodeInfo, the health check puts that one in the topology.

Whichever side checks last sees the change of the other side. So the topology always ends up with the new DatanodeInfo.

An earlier version of this PR made updateNode and checkNodesHealth synchronized. That held a lock during every health check, only to guard against a rare race. This version checks for the race instead.

The bug was found while testing HDDS-16627 (#11349), the deterministic SCM simulation.

What is the link to the Apache JIRA

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

How was this patch tested?

New test TestSCMNodeManager#testRegisterWhileMarkedDeadKeepsNodeInTopology. It runs the health check and a registration with new ports on two threads, in two cases:

1. The health check starts first. The test stops it just before it removes the DEAD datanode from the topology. The registration then runs to the end, and after that the health check goes on.
2. The registration starts first. The test stops it just after it has put the datanode with its new ports in the topology. The health check then runs to the end, and after that the registration goes on.

At the end, the datanode must be HEALTHY and in the topology, with its new ports. Without the fix, it ends up HEALTHY but not in the topology. The first case also fails without the check in the health check, and the second case fails without the check in the registration.

New test TestNodeStateMap#testUpdateHealthSkipsChangedNode checks that the health is not changed when the node has changed since the caller read it.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[smengcl]

+1