[
https://issues.apache.org/jira/browse/HDDS-17623?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17623:
------------------------------
Attachment: TestBugCR1DeadMaintenanceMisReplication.java
> Mis-replication of a container is not repaired while one of its replicas is
> on a datanode that is DEAD in maintenance
> ---------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17623
> URL: https://issues.apache.org/jira/browse/HDDS-17623
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1,
> 2.2.2
> Reporter: Siyao Meng
> Priority: Major
> Attachments:
> CR-1-resolve-rack-of-replica-on-node-outside-topology.patch,
> TestBugCR1DeadMaintenanceMisReplication.java,
> TestBugCR7ReregisteredNodeReplicaRack.java
>
>
> h3. Mechanism
> {{SCMCommonPlacementPolicy.getPlacementGroup}} finds a datanode's rack by
> walking up from the {{DatanodeDetails}} object in the network topology. It
> returns null for a datanode that is no longer in the topology. Two of its
> callers do not handle that:
> * {{replicasToCopyToFixMisreplication}} groups the replicas by rack with
> {{Collectors.groupingBy}}, which throws {{NullPointerException: element
> cannot be mapped to a null key}}.
> * {{replicasToRemoveToFixOverreplication}} adds the racks to a
> {{PriorityQueue}}, which throws a {{NullPointerException}} on null.
> {{validateContainerPlacement}} already handles a null rack: HDDS-8950 drops
> it, and HDDS-13762 counts a node that is DEAD and IN_MAINTENANCE on the rack
> of its network location, because it is expected to come back.
> {{MisReplicationHandler}} and {{ECOverReplicationHandler}} throw,
> {{UnhealthyReplicationProcessor.processAll}} logs the exception at ERROR and
> requeues the container, and no command is sent.
> A replica's datanode is outside the topology in two cases:
> # *DEAD in maintenance.* {{NodeStateManager}} removes a node from the
> topology when it becomes DEAD (since HDDS-16623, before that
> {{DeadNodeHandler}} did), and {{DeadNodeHandler}} keeps the replicas of a
> node that is IN_MAINTENANCE. The replicas hold the same {{DatanodeInfo}}
> object, which now has no parent. This is the normal maintenance workflow: the
> node is put in maintenance and then shut down.
> # *Re-registration with changed details.* When a registered datanode
> registers again with a new address, a new version or new ports,
> {{SCMNodeManager.updateNodeAndTopology}} calls {{NetworkTopology.update}},
> which removes the stored {{DatanodeInfo}} from the tree and clears its
> parent, and stores a new object. Replicas built from earlier reports still
> hold the old object until the full container report sent with the
> registration is processed.
> h3. Trigger
> Case 1: a container that is mis-replicated for another reason (for example
> two replicas on one rack) has a replica on a datanode that enters maintenance
> and is then stopped. {{ozone admin datanode maintenance}} without {{--end}}
> sets no end time, so the node stays DEAD and IN_MAINTENANCE until an operator
> brings it back or ends maintenance. This was run through
> {{NodeDecommissionManager.startMaintenanceNodes}} and the real health check
> (see Reproduction).
> Case 2: a datanode restarts with a changed address, version (rolling upgrade)
> or port set, for example after enabling Ratis DataStream.
> h3. Impact
> * Case 1: every Replication Manager pass queues the container, the
> mis-replication handler throws and no copy is sent, for as long as the node
> stays DEAD in maintenance. The container keeps its reduced rack spread for
> that time, and one ERROR with a stack trace is logged per container and pass.
> The same handler is used for Ratis containers through
> {{RatisMisReplicationHandler}} (argued from the code, only EC was run).
> Over-replication is not affected in this case, because
> {{ECOverReplicationHandler}} only passes replicas on healthy in-service nodes
> to the placement policy.
> * Case 2: until the registration report is processed, a container whose
> replicas are on distinct racks is flagged mis-replicated, and the EC
> mis-replication and over-replication handlers throw for it. No wrong command
> is sent (target selection resolves datanodes through {{NodeManager}}), and
> the report normally arrives within seconds, so the effect is a short delay
> and ERROR logs. It lasts longer only if the registration report is lost
> (argued, not run).
> Releases from 1.4.0 have case 1 by code reading (not run on a release). In
> 1.4.0 to 2.1.x, before HDDS-13762, {{validateContainerPlacement}} also
> dropped the rack of a DEAD maintenance node, so an EC container with a
> replica on such a node could be flagged mis-replicated for that reason alone
> and then hit the exception (argued from the code, not run).
> h3. Reproduction
> Both classes run in {{hadoop-hdds/server-scm}}, pass on unmodified source
> when the bug is present, and have the target path and Maven command in the
> class comment. They use a real SCM ({{HddsTestUtils.getScm}}) with a rack
> mapping, real {{SCMNodeManager}} registration, the real
> {{ContainerReportHandler}}, the EC placement policy SCM builds (rack
> scatter), and the real check handler and EC handlers with SCM's own
> {{ReplicationManager}}. {{ContainerManager}} is a map backed stub that stores
> the replicas the report handler creates.
> * [^TestBugCR1DeadMaintenanceMisReplication.java]: rs-3-2 container with
> index 1 and 2 on one rack, and index 3 and 4 on another. The datanode with
> index 3 is put in maintenance with
> {{NodeDecommissionManager.startMaintenanceNodes}} (no end time) and made DEAD
> by the real health check, which removes it from the topology. The admin
> monitor step to IN_MAINTENANCE and the datanode heartbeat that reports it are
> done with the calls those components make. The container is queued as
> mis-replicated, {{ECMisReplicationHandler}} throws {{NullPointerException:
> element cannot be mapped to a null key}} from
> {{replicasToCopyToFixMisreplication}}, and no command is queued. With the
> patch the handler sends two replicate commands to the free racks; the one for
> index 4 is sent because the DEAD node now counts on the rack it shares with
> index 4.
> * [^TestBugCR7ReregisteredNodeReplicaRack.java]: one datanode per rack, a
> container on 5 racks and one with an extra copy of index 1. One datanode
> registers again with an extra port. Before its registration report is
> processed, the replica's datanode has no parent while {{NodeManager}}'s has
> one, the container on 5 racks is flagged mis-replicated, and
> {{ECMisReplicationHandler}} and {{ECOverReplicationHandler}} throw. After the
> registration report all of this is gone. With the patch the container is not
> flagged.
> h3. Patch
> [^CR-1-resolve-rack-of-replica-on-node-outside-topology.patch], against
> e4ae41fc4047c99af983020b4f33bd297c9d0ac5. It also applies to master at
> 3fc2387040f.
> * {{getPlacementGroup}}: when the given object has no parent, look up the
> rack of the datanode object {{NodeManager}} holds now, as
> {{validateDatanodes}} already does for target selection. This covers case 2
> for all three methods. Nothing changes when the object is in the topology.
> * {{replicasToCopyToFixMisreplication}}: group replicas by the rack path that
> {{validateContainerPlacement}} uses (moved into a helper,
> {{getPlacementGroupPath}}), so a DEAD maintenance node counts on the rack of
> its network location there too. A replica whose rack is still unknown (a DEAD
> node that is not in maintenance, before {{DeadNodeHandler}} removes its
> replicas) is not counted on any rack, as in {{validateContainerPlacement}}.
> It cannot be a copy source anyway. Target selection still does not know the
> rack of a DEAD maintenance node, so it can pick a target on that rack, which
> the check after the copy rejects, and repair may take more than one pass.
> * {{replicasToRemoveToFixOverreplication}} is not changed: its candidates are
> on healthy in-service nodes, which are in the topology, and case 2 is covered
> by the first change.
> Covered by {{testReplicasToCopyWithDeadNodeOutOfTopology}} (the DEAD node
> shares a rack with a healthy replica: when it is IN_MAINTENANCE that replica
> must be copied, when it is IN_SERVICE nothing) and
> {{testReplicaPlacementAfterReregistration}} in the existing
> {{TestSCMContainerPlacementRackScatter}}, which use a real
> {{NetworkTopologyImpl}}. Without the change the first fails with the
> {{NullPointerException}} above and the second with "expected: 0 but was: 1"
> (mis-replication count of a container on 5 racks). Each of the two changes is
> needed for one of the tests; a variant that only skips replicas of unknown
> rack also fails the first, because it copies nothing in the IN_MAINTENANCE
> case. With the change, the placement policy, pipeline placement, Replication
> Manager handler and node manager suites in {{hadoop-hdds/server-scm}} pass
> (702 tests) and checkstyle is clean.
> Found by code review of the SCM EC container replication and placement repair
> paths, as part of the TLA+ verification effort under HDDS-15926, on commit
> e4ae41fc4047c99af983020b4f33bd297c9d0ac5. Checked against HDDS issues and
> apache/ozone pull requests for duplicates before filing. Related but not
> duplicates: HDDS-8950 and HDDS-13762 (the same null rack in
> {{validateContainerPlacement}}), HDDS-13544 (a DEAD maintenance node passed
> as a used node), HDDS-16633 (a datanode missing from the topology after
> re-registering while it is removed as DEAD), HDDS-17363 (the over-replication
> handler does not count a maintenance replica when it picks the copy to
> delete). With the patch a container of case 1 now gets copies, and with some
> rack orders it can then enter the copy then delete loop that HDDS-17363
> describes (argued from the code, not run), so the two should be looked at
> together. The open pull request for HDDS-17381 rewrites the same part of
> {{validateContainerPlacement}}, so one of the two will need a rebase. The
> attached patch is a proposal for review. Generated with Specula (Claude Opus
> 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
