peterxcli opened a new pull request, #11688: URL: https://github.com/apache/ozone/pull/11688
## What changes were proposed in this pull request? When a registered datanode registers again with a new IP address, hostname, ports or version, `SCMNodeManager#register` replaces its object in the network topology with `NetworkTopology#update`, and then replaces its `DatanodeInfo` with `NodeStateManager#updateNode`. Since HDDS-16623, `NodeStateManager#checkNodesHealth` removes a datanode from the topology when it marks the datanode DEAD. Nothing keeps these two apart, so they can interleave: 1. The health check marks the datanode DEAD and is about to remove it from the topology. 2. The datanode registers again with new ports. The registration puts the new object in the topology and stores a HEALTHY `DatanodeInfo`. 3. The health check removes the datanode from the topology. `InnerNodeImpl#remove` removes whichever child has the datanode's name, so it removes the object the registration just added. The datanode is then HEALTHY but missing from the topology, so placement never chooses it. It only comes back if it goes DEAD and recovers, or when SCM restarts. `NetworkTopology#contains` still returns true for it, because its parent pointer still points at the rack. The same happens if the registration has updated the topology, but not yet stored the new `DatanodeInfo`, when the health check marks the datanode DEAD and removes it. The window is small: the datanode has to register again at the moment SCM declares it DEAD. With this change: - `NodeStateManager#updateNode` updates the topology itself, before it replaces the `DatanodeInfo`. `SCMNodeManager#register` no longer calls `NetworkTopology#update` for this. - `NodeStateManager#updateNode` and `NodeStateManager#checkNodesHealth` are `synchronized`. A registration that changes the datanode waits for a running health check to finish, and a health check waits for a running update. `checkNodesHealth` was `synchronized` until HDDS-14671 removed `forceNodesToHealthyReadOnly`, the method it was synchronized with. 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 `TestSCMNodeManager#testRegisterWhileMarkedDeadKeepsNodeInTopology`, which runs the health check and a registration with new ports on two threads. One case stops the health check just before it removes the DEAD datanode from the topology. The other stops the registration just after it has put the datanode with its new ports into the topology. Each case then starts the other operation, checks that it waits, and checks that the datanode ends up HEALTHY and in the topology with its new ports. Without the fix, the other operation does not wait, and the datanode ends up HEALTHY with no leaf in the topology. - `TestSCMNodeManager`, `TestNodeStateManager`, `TestNodeStateMap`, `TestDeadNodeHandler`, `TestContainerPlacement`, `TestIncrementalContainerReportHandler`, `TestNodeReportHandler` and `TestSCMNodeMetrics` pass. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
