Copilot commented on code in PR #11692:
URL: https://github.com/apache/ozone/pull/11692#discussion_r4347246811
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -182,6 +196,25 @@ public List<SCMCommand<?>>
dispatch(SCMHeartbeatRequestProto heartbeat) {
return commands;
}
+ /**
+ * Returns true if the node is not healthy (stale or dead) and its reported
version is
+ * incompatible with this SCM. Healthy nodes are always allowed through —
their version was
+ * already validated at registration. If the node status cannot be
determined, returns false
+ * so the heartbeat is processed normally.
+ */
+ private boolean isUnhealthyWithIncompatibleVersion(DatanodeDetails
datanodeDetails,
+ SCMHeartbeatRequestProto heartbeat) {
+ if (!heartbeat.hasDatanodeVersion()) {
+ return false;
+ }
+ try {
+ return !nodeManager.getNodeStatus(datanodeDetails).isHealthy()
+ && nodeManager.shouldFenceDatanode(datanodeDetails,
heartbeat.getDatanodeVersion());
+ } catch (NodeNotFoundException e) {
Review Comment:
Swallowing `NodeNotFoundException` without any logging can make operational
troubleshooting difficult (eg, unexpected heartbeats from unknown nodes).
Consider logging at least at `DEBUG` (or `WARN` if unexpected) with the
datanode details, then allowing normal processing as you do today.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -182,6 +196,25 @@ public List<SCMCommand<?>>
dispatch(SCMHeartbeatRequestProto heartbeat) {
return commands;
}
+ /**
+ * Returns true if the node is not healthy (stale or dead) and its reported
version is
+ * incompatible with this SCM. Healthy nodes are always allowed through —
their version was
+ * already validated at registration. If the node status cannot be
determined, returns false
+ * so the heartbeat is processed normally.
+ */
+ private boolean isUnhealthyWithIncompatibleVersion(DatanodeDetails
datanodeDetails,
+ SCMHeartbeatRequestProto heartbeat) {
+ if (!heartbeat.hasDatanodeVersion()) {
+ return false;
+ }
+ try {
+ return !nodeManager.getNodeStatus(datanodeDetails).isHealthy()
+ && nodeManager.shouldFenceDatanode(datanodeDetails,
heartbeat.getDatanodeVersion());
+ } catch (NodeNotFoundException e) {
Review Comment:
This gate is intended to prevent STALE/DEAD nodes from updating their
heartbeat timestamp unless their version is validated, but
`!heartbeat.hasDatanodeVersion()` currently returns `false` (allow-through). If
unhealthy nodes don’t always include a version report on heartbeat (which the
existing tests imply), an incompatible node can still ‘resurrect’ without
re-registering. Consider changing the logic so that if the node is unhealthy
and the heartbeat lacks `datanodeVersion`, SCM requests re-registration (or
uses the last known version if available) instead of processing the heartbeat
normally.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMDatanodeHeartbeatDispatcher.java:
##########
@@ -166,6 +170,52 @@ public void testVersionReportProcessedOnlyWhenPresent()
throws IOException {
verify(mockNodeManager, times(1)).processVersionReport(any(), any());
}
+ @Test
+ public void testStaleNodeWithIncompatibleVersionGetsReregisterCommand()
throws Exception {
+ NodeManager mockNodeManager = mock(NodeManager.class);
+ when(mockNodeManager.isNodeRegistered(any())).thenReturn(true);
+
when(mockNodeManager.getNodeStatus(any())).thenReturn(NodeStatus.inServiceStale());
+ when(mockNodeManager.shouldFenceDatanode(any(), any())).thenReturn(true);
Review Comment:
Given the dispatcher’s new behavior depends on
`heartbeat.hasDatanodeVersion()`, add a unit test for a STALE/DEAD node
heartbeat *without* `datanodeVersion` to lock in the intended behavior
(especially if you update the production logic to require re-registration when
version is missing).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -104,6 +105,19 @@ public List<SCMCommand<?>>
dispatch(SCMHeartbeatRequestProto heartbeat) {
if (heartbeat.hasCommandQueueReport()) {
commandQueueReport = heartbeat.getCommandQueueReport();
}
+
+ // Stale and dead nodes must pass a version compatibility check before
their heartbeat
+ // timestamp is updated. Without this gate a node that went unhealthy
during a rolling
+ // upgrade could resurrect at an incompatible software version. If the
check fails, ask
+ // the node to re-register so the full registration fence runs with
current version info.
+ if (isUnhealthyWithIncompatibleVersion(datanodeDetails, heartbeat)) {
+ LOG.info("Node {} is not healthy and reported an incompatible version.
Asking to re-register.",
+ datanodeDetails);
+ DatanodeID dnID = datanodeDetails.getID();
+ nodeManager.addDatanodeCommand(datanodeDetails.getID(), new
ReregisterCommand());
+ return nodeManager.getCommandQueue(dnID);
Review Comment:
`dnID` is computed but not used for `addDatanodeCommand`, which repeats
`datanodeDetails.getID()`. Use `dnID` consistently to avoid duplication and
keep the intent clear.
--
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]
