[GitHub] [hadoop-ozone] sodonnel commented on a change in pull request #1484: HDDS-4322. Add integration tests for Decommission and resolve issues detected by the tests.

[GitBox <gi...@apache.org>]

sodonnel commented on a change in pull request #1484:
URL: https://github.com/apache/hadoop-ozone/pull/1484#discussion_r502444281



##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DatanodeAdminMonitorImpl.java
##########
@@ -294,6 +294,9 @@ private boolean checkContainersReplicatedOnNode(DatanodeDetails dn)
             "in containerManager", cid, dn);
       }
     }
+    LOG.info("{} has {} sufficientlyReplicated, {} underReplicated and {} " +

Review comment:
       That message would be printed for each decommissioned or maintenance node for each run of the replication monitor. The default now is 30 seconds, but that is probably too short, and we will need to change that default to something longer, maybe a minute or two.
   
   I think it is useful to have this information in the logs from a supportability perspective. It lets us see the progress of decommission, whether a node appears to be stuck, etc, and a few messages per minute only when nodes are decommissioning is not excessively noisy. 

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java
##########
@@ -476,6 +476,9 @@ public Builder setDatanodeDetails(DatanodeDetails details) {
       this.setupTime = details.getSetupTime();
       this.revision = details.getRevision();
       this.buildDate = details.getBuildDate();
+      this.persistedOpState = details.getPersistedOpState();
+      this.persistedOpStateExpiryEpochSec =
+          details.getPersistedOpStateExpiryEpochSec();
       return this;

Review comment:
       These fields had been missed from the DatanodeDetails builder object, so when the DN reported back its "persisted state" the DN was always IN_SERVICE. Adding this change fixed that problem and allowed the state to be returned correctly.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NewNodeHandler.java
##########
@@ -20,27 +20,49 @@
 
 import org.apache.hadoop.hdds.conf.ConfigurationSource;
 import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.node.states.NodeNotFoundException;
 import org.apache.hadoop.hdds.scm.pipeline.PipelineManager;
 import org.apache.hadoop.hdds.server.events.EventHandler;
 import org.apache.hadoop.hdds.server.events.EventPublisher;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Handles New Node event.
  */
 public class NewNodeHandler implements EventHandler<DatanodeDetails> {
 
   private final PipelineManager pipelineManager;
+  private final NodeDecommissionManager decommissionManager;
   private final ConfigurationSource conf;
+  private static final Logger LOG =
+      LoggerFactory.getLogger(NewNodeHandler.class);
 
   public NewNodeHandler(PipelineManager pipelineManager,
+      NodeDecommissionManager decommissionManager,
       ConfigurationSource conf) {
     this.pipelineManager = pipelineManager;
+    this.decommissionManager = decommissionManager;
     this.conf = conf;
   }
 
   @Override
   public void onMessage(DatanodeDetails datanodeDetails,
       EventPublisher publisher) {
     pipelineManager.triggerPipelineCreation();
+    HddsProtos.NodeOperationalState opState
+        = datanodeDetails.getPersistedOpState();
+    if (datanodeDetails.getPersistedOpState()
+        != HddsProtos.NodeOperationalState.IN_SERVICE) {
+      try {
+        decommissionManager.continueAdminForNode(datanodeDetails);
+      } catch (NodeNotFoundException e) {
+        // Should not happen, as the node has just registered to call this event
+        // handler.
+        LOG.warn("NodeNotFound when adding the node to the decommissionManager",
+            e);
+      }
+    }
   }

Review comment:
       If a DN is DECOMMISSIONING, and then SCM is restarted, SCM will forget all about the decommission nodes. Then the nodes will re-register with SCM and report they are DECOMMISSIONING. If the node is DECOMMISSIONING rather than DECOMMISSIONED, we need to get it back into the decommission workflow. This NewNodeHandler is invoked for a new registration of a Node, so this change continues the decom process.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java
##########
@@ -233,6 +233,22 @@ public synchronized void decommissionNodes(List nodes)
     }
   }
 
+  /**
+   * If a SCM is restarted, then upon re-registration the datanode will already
+   * be in DECOMMISSIONING or ENTERING_MAINTENANCE state. In that case, it
+   * needs to be added back into the monitor to track its progress.
+   * @param dn Datanode to add back to tracking.
+   * @throws NodeNotFoundException
+   */
+  public synchronized void continueAdminForNode(DatanodeDetails dn)
+      throws NodeNotFoundException {
+    NodeOperationalState opState = getNodeStatus(dn).getOperationalState();
+    if (opState == NodeOperationalState.DECOMMISSIONING
+        || opState == NodeOperationalState.ENTERING_MAINTENANCE) {
+      monitor.startMonitoring(dn, 0);
+    }
+  }
+

Review comment:
       This facilitates nodes re-joining the workflow - will be called from NewNodeHandler.

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStateManager.java
##########
@@ -239,11 +239,32 @@ private void initializeStateMachines() {
    */
   public void addNode(DatanodeDetails datanodeDetails)
       throws NodeAlreadyExistsException {
-    nodeStateMap.addNode(datanodeDetails, new NodeStatus(
-        NodeOperationalState.IN_SERVICE, nodeHealthSM.getInitialState()));
+    NodeStatus newNodeStatus = newNodeStatus(datanodeDetails);
+    nodeStateMap.addNode(datanodeDetails, newNodeStatus);
     eventPublisher.fireEvent(SCMEvents.NEW_NODE, datanodeDetails);
   }
 
+  /**
+   * When a node registers with SCM, the operational state stored on the
+   * datanode is the source of truth. Therefore, if the datanode reports
+   * anything other than IN_SERVICE on registration, the state in SCM should be
+   * updated to reflect the datanode state.
+   * @param dn DatanodeDetails reported by the datanode
+   */
+  private NodeStatus newNodeStatus(DatanodeDetails dn) {
+    HddsProtos.NodeOperationalState dnOpState = dn.getPersistedOpState();
+    if (dnOpState != NodeOperationalState.IN_SERVICE) {
+      LOG.info("Updating nodeOperationalState on registration as the " +
+              "datanode has a persisted state of {} and expiry of {}",
+          dnOpState, dn.getPersistedOpStateExpiryEpochSec());
+      return new NodeStatus(dnOpState, nodeHealthSM.getInitialState(),
+          dn.getPersistedOpStateExpiryEpochSec());
+    } else {
+      return new NodeStatus(
+          NodeOperationalState.IN_SERVICE, nodeHealthSM.getInitialState());
+    }
+  }
+
   /**

Review comment:
       This code was lifted out of SCMNodeManager as it fits better here. The NodeStatus is now set as part of registration and before any events are triggered (eg PipelineCreation). Before there was a race condition, where the NodeStatus was IN_SERVICE and pipeline creation could be triggered. Then the state was quickly changed to DECOMMISSIONED etc. 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org