errose28 commented on code in PR #11692:
URL: https://github.com/apache/ozone/pull/11692#discussion_r4350279586
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeManager.java:
##########
@@ -408,6 +408,16 @@ void processNodeReport(DatanodeDetails datanodeDetails,
*/
void processVersionReport(DatanodeDetails datanodeDetails,
DatanodeVersionProto versionReport);
+ /**
+ * Returns true if the datanode should be blocked from joining the cluster
due to a version
+ * incompatibility with this SCM. See {@code
SCMNodeManager.shouldFenceDatanode} for the full
+ * set of conditions. Returns false by default for implementations that do
not enforce versioning.
Review Comment:
```suggestion
* set of conditions. Returns false by default for test implementations
that do not enforce versioning.
```
Another option is to explicitly update each test implementation with `return
false`, although I am fine with this approach too. Just want it to be clear
that prod instances are not expected to use the default implementation.
##########
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:
I think the intent of the copilot comment is valid, although I don't like
the way it rewrote the method. Here we are defining the server side protocol
which is:
- Stale or Dead nodes must re-register
- Registration is the only time a version is required to be attached to the
heartbeat
So this method should first check if the node is healthy. If not, it should
call `shouldFenceDatanode`. `shouldFenceDatanode` already returns `true` if the
version is absent so this method does not need that check.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -104,6 +105,18 @@ 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)) {
Review Comment:
Do we need to process the command queue report for these nodes?
Also are we intentionally processing the version report for the node before
this check? I guess that will update SCM's metadata of the node's versions
which may help with debugging when it is fenced. However this method also calls
`sendFinalizeToDatanodeIfNeeded` which calls `shouldFenceDatanode`, so the
fencing check logs twice.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeHeartbeatDispatcher.java:
##########
@@ -104,6 +105,18 @@ 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);
+ nodeManager.addDatanodeCommand(datanodeDetails.getID(), new
ReregisterCommand());
+ return nodeManager.getCommandQueue(datanodeDetails.getID());
Review Comment:
The first block which tells unknown nodes to re-register doesn't return so
that the commands sent in the heartbeat are picked up by a debug log. Maybe
this new case should be contained in an `else if` block and follow a similar
pattern.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMDatanodeHeartbeatDispatcher.java:
##########
@@ -141,9 +144,10 @@ public <PAYLOAD, EVENT extends Event<PAYLOAD>> void
fireEvent(
}
@Test
- public void testVersionReportProcessedOnlyWhenPresent() throws IOException {
+ public void testVersionReportProcessedOnlyWhenPresent() throws IOException,
NodeNotFoundException {
NodeManager mockNodeManager = mock(NodeManager.class);
when(mockNodeManager.isNodeRegistered(any())).thenReturn(true);
+
when(mockNodeManager.getNodeStatus(any())).thenReturn(NodeStatus.inServiceStale());
Review Comment:
Why was stale chosen here?
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMDatanodeHeartbeatDispatcher.java:
##########
@@ -166,6 +170,74 @@ public void testVersionReportProcessedOnlyWhenPresent()
throws IOException {
verify(mockNodeManager, times(1)).processVersionReport(any(), any());
}
+ @Test
+ public void testStaleNodeWithIncompatibleVersionGetsReregisterCommand()
throws Exception {
Review Comment:
Looks like these stale and dead tests can be combined and parameterized by
health state. We can also test that re-register is skipped for these health
states when the version is allowed.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMDatanodeHeartbeatDispatcher.java:
##########
@@ -166,6 +170,74 @@ 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);
+
+ SCMDatanodeHeartbeatDispatcher dispatcher =
+ new SCMDatanodeHeartbeatDispatcher(mockNodeManager,
mock(EventPublisher.class));
+
+ DatanodeVersionProto version = DatanodeVersionProto.newBuilder()
+ .setApparentVersion(1).setSoftwareVersion(1).build();
+ SCMHeartbeatRequestProto heartbeat = SCMHeartbeatRequestProto.newBuilder()
+ .setDatanodeDetails(randomDatanodeDetails().getProtoBufMessage())
+ .setDatanodeVersion(version)
+ .build();
+
+ dispatcher.dispatch(heartbeat);
+
+ verify(mockNodeManager,
times(1)).addDatanodeCommand(any(DatanodeID.class),
any(ReregisterCommand.class));
+ verify(mockNodeManager, never()).processHeartbeat(any(), any());
+ }
+
+ @Test
+ public void testDeadNodeWithIncompatibleVersionGetsReregisterCommand()
throws Exception {
+ NodeManager mockNodeManager = mock(NodeManager.class);
+ when(mockNodeManager.isNodeRegistered(any())).thenReturn(true);
+
when(mockNodeManager.getNodeStatus(any())).thenReturn(NodeStatus.inServiceDead());
+ when(mockNodeManager.shouldFenceDatanode(any(), any())).thenReturn(true);
+
+ SCMDatanodeHeartbeatDispatcher dispatcher =
+ new SCMDatanodeHeartbeatDispatcher(mockNodeManager,
mock(EventPublisher.class));
+
+ DatanodeVersionProto version = DatanodeVersionProto.newBuilder()
+ .setApparentVersion(1).setSoftwareVersion(1).build();
+ SCMHeartbeatRequestProto heartbeat = SCMHeartbeatRequestProto.newBuilder()
+ .setDatanodeDetails(randomDatanodeDetails().getProtoBufMessage())
+ .setDatanodeVersion(version)
+ .build();
+
+ dispatcher.dispatch(heartbeat);
+
+ verify(mockNodeManager,
times(1)).addDatanodeCommand(any(DatanodeID.class),
any(ReregisterCommand.class));
+ verify(mockNodeManager, never()).processHeartbeat(any(), any());
+ }
+
+ @Test
+ public void testUnhealthyNodeWithoutVersionReportIsNotFenced() throws
Exception {
+ // A stale or dead node that omits datanodeVersion (e.g. an older binary)
must not be
Review Comment:
I don't think this test is valid. We don't have to handle older binaries
because ZDU support doesn't start until we release this. Actually the stale or
dead nodes should not be allowed back if they do not report a version. The
comment I left on `SCMDatanodeHeartbeatDispatcher` about delegating this to
`shouldFenceDatanode` should fix this.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMDatanodeHeartbeatDispatcher.java:
##########
Review Comment:
We should probably add a test that healthy nodes are let through the
dispatcher gate. Their software version is only expected to change if the node
restarts, which would cause it to re-register on its own.
--
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]
