HDDS-2607 DeadNodeHandler should not remove replica for a dead maintenance node #343
Conversation
sodonnel
force-pushed
the
HDDS-2607-dead-handling
branch
from
6109a94 to
ae23eff
Compare
|
This change partly tested by modifying the single test in TestDeadNodeHandler(), however due to HDDS-2508 this test is Ignored (passes locally, failing under CI). Therefore I ran the test manually a few times and it always passed and the test should be re-enabled when the mentioned issue is resolved. |
…Also ensure container replicas are not removed when an IN_MAINTENANCE node goes dead
…known issue tracked in HDDS-2508
sodonnel
force-pushed
the
HDDS-2607-dead-handling
branch
from
e9320c3 to
89d8427
Compare
| import org.junit.Before; | ||
| import org.junit.Ignore; | ||
| import org.junit.Test; | ||
| import org.junit.*; |
There was a problem hiding this comment.
Generally, explicit imports are preferred over *
There was a problem hiding this comment.
Ah, caught out by Intellij again! I have fixed this, and also, hopefully fixed my Intellij settings to stop it doing this.
|
|
||
| nodeOpStateSM.addTransition( | ||
| NodeOperationalState.IN_SERVICE, NodeOperationalState.DECOMMISSIONING, | ||
| NodeOperationStateEvent.START_DECOMMISSION); |
There was a problem hiding this comment.
Now enum NodeOperationStateEvent is unused. Can it be removed?
There was a problem hiding this comment.
Correct, this can be removed. I have made this change.
| removeContainerReplicas(datanodeDetails); | ||
| // Remove the container replicas associated with the dead node unless it | ||
| // is IN_MAINTENANCE | ||
| if (!nodeManager.getNodeStatus(datanodeDetails).isInMaintenance()) { |
There was a problem hiding this comment.
Is it possible to get this message for a node ENTERING_MAINTENANCE? If so, shouldn't it be ignored here as well?
There was a problem hiding this comment.
If a node goes dead while "entering maintenance" then it should be handled as a dead node. The reason, is that we have not yet determine if all the containers on the host are sufficiently replicated. If it goes dead before that check has completed and the node has moved into IN_MAINTENANCE, then it must be handled as a dead node as normal.
This is the same for a node which is DECOMMISSIONING and not yet reached DECOMMISSIONED.
Unless the node has reached its end state (IN_MAINTENANCE or DECOMMISSIONED), if it goes dead the workflow is aborted and it is handled just like any other dead node.
| assertEquals("NON_HEALTHY_TO_HEALTHY_NODE", | ||
| eventPublisher.getLastEvent().getName()); |
There was a problem hiding this comment.
I think it would be safer to compare the event, not its name:
assertEquals(SCMEvents.NON_HEALTHY_TO_HEALTHY_NODE,
eventPublisher.getLastEvent());
There was a problem hiding this comment.
True. I was pretty much copying existing code when I added this test. I have made this change here and also applied it to the other occurrences in the class to keep in consistent.
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thank @sodonnel for updating the patch.
3 participants
What changes were proposed in this pull request?
Normally, when a node goes dead, the DeadNodeHandler removes all the containers and replica associated with the node from the ContainerManager.
If a node is IN_MAINTENANCE and goes dead, then we do not want to remove its replica. They should remain present in the system to prevent the container being marked as under-replicated.
We also need to consider the case where the node is dead, and then maintenance expires automatically. In that case, the replica associated with the node must be removed and the affected containers will become under-replicated.
This PR resolves this issue, by ensuring stale, dead and healthy node events are fired as normal by the NodeStateManager when the node health state changes. This is works as before, prior to any decommission related changes.
To ensure the the container replicas are not removed for a node IN_MAINTENANCE, the DeadNodeHandler has been modified to check the node state and skip removing the replicas if the node is IN_MAINTENANE. For all other states, the dead node handling is unchanged.
Then, for any changes in the node operational state, the same health events are fired when the operational state is changed. This means that a DECOMMISSIONED or IN_MAINTENANCE node that is heartbeating and healthy will fire the HEALTHY node event when it is moved to IN_SERVICE, which triggers pipeline creation.
Similarly, if a node was IN_MAINTENANCE and is DEAD, when it transitions to IN_SERVICE, the dead node event will be trigger, causing the container replicas to be removed. Some of these events will have no effect, but the volume of changes to the node operational state will be minimal, as it will only be changed by the decommission monitor or someone triggering or cancelling decommission and maintenance.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-2607
How was this patch tested?
Additional and modified unit tests