[grid] stop the health check of a restarted node

SeleniumHQ · Jan 2, 2025 · 4ddb16c · 4ddb16c
1 parent c62597e
commit 4ddb16c
Show file tree

Hide file tree

Showing 5 changed files with 25 additions and 33 deletions.
diff --git a/java/src/org/openqa/selenium/grid/distributor/GridModel.java b/java/src/org/openqa/selenium/grid/distributor/GridModel.java
@@ -114,7 +114,9 @@ public void add(NodeStatus node) {
                   "Re-adding node with id %s and URI %s.",
                   node.getNodeId(), node.getExternalUri()));
 
-          events.fire(new NodeRestartedEvent(node));
+          // Send the previous state to allow cleaning up the old node related resources.
+          // Nodes are initially added in the "down" state, so the new state must be ignored.
+          events.fire(new NodeRestartedEvent(next));
           iterator.remove();
           break;
         }
@@ -226,7 +228,8 @@ public void purgeDeadNodes() {
         if (nodeHealthCount.getOrDefault(id, 0) > UNHEALTHY_THRESHOLD) {
           LOG.info(
               String.format(
-                  "Removing Node %s, unhealthy threshold has been reached", node.getExternalUri()));
+                  "Removing Node %s (uri: %s), unhealthy threshold has been reached",
+                  node.getNodeId(), node.getExternalUri()));
           toRemove.add(node);
           break;
         }
@@ -239,11 +242,17 @@ public void purgeDeadNodes() {
             lastTouched.plus(node.getHeartbeatPeriod().multipliedBy(PURGE_TIMEOUT_MULTIPLIER));
 
         if (node.getAvailability() == UP && lostTime.isBefore(now)) {
-          LOG.info(String.format("Switching Node %s from UP to DOWN", node.getExternalUri()));
+          LOG.info(
+              String.format(
+                  "Switching Node %s (uri: %s) from UP to DOWN",
+                  node.getNodeId(), node.getExternalUri()));
           replacements.put(node, rewrite(node, DOWN));
           nodePurgeTimes.put(id, Instant.now());
         } else if (node.getAvailability() == DOWN && deadTime.isBefore(now)) {
-          LOG.info(String.format("Removing Node %s, DOWN for too long", node.getExternalUri()));
+          LOG.info(
+              String.format(
+                  "Removing Node %s (uri: %s), DOWN for too long",
+                  node.getNodeId(), node.getExternalUri()));
           toRemove.add(node);
         }
       }

diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java
@@ -206,7 +206,8 @@ public LocalDistributor(
 
     bus.addListener(NodeStatusEvent.listener(this::register));
     bus.addListener(NodeStatusEvent.listener(model::refresh));
-    bus.addListener(NodeRestartedEvent.listener(this::handleNodeRestarted));
+    bus.addListener(
+        NodeRestartedEvent.listener(previousNodeStatus -> remove(previousNodeStatus.getNodeId())));
     bus.addListener(NodeRemovedEvent.listener(nodeStatus -> remove(nodeStatus.getNodeId())));
     bus.addListener(
         NodeHeartBeatEvent.listener(
@@ -329,25 +330,6 @@ private void register(NodeStatus status) {
     }
   }
 
-  private void handleNodeRestarted(NodeStatus status) {
-    Require.nonNull("Node", status);
-    Lock writeLock = lock.writeLock();
-    writeLock.lock();
-    try {
-      if (!nodes.containsKey(status.getNodeId())) {
-        return;
-      }
-      if (!getNodeFromURI(status.getExternalUri()).isDraining()) {
-        LOG.info(
-            String.format(
-                "Node %s has restarted. Setting availability to DOWN.", status.getNodeId()));
-        model.setAvailability(status.getNodeId(), DOWN);
-      }
-    } finally {
-      writeLock.unlock();
-    }
-  }
-
   @Override
   public LocalDistributor add(Node node) {
     Require.nonNull("Node", node);
@@ -499,15 +481,13 @@ public void remove(NodeId nodeId) {
     Lock writeLock = lock.writeLock();
     writeLock.lock();
     try {
-      Node node = nodes.get(nodeId);
+      Node node = nodes.remove(nodeId);
+      model.remove(nodeId);
+      allChecks.remove(nodeId);
 
       if (node instanceof RemoteNode) {
         ((RemoteNode) node).close();
       }
-
-      nodes.remove(nodeId);
-      model.remove(nodeId);
-      allChecks.remove(nodeId);
     } finally {
       writeLock.unlock();
     }

diff --git a/java/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java b/java/src/org/openqa/selenium/grid/sessionmap/jdbc/JdbcBackedSessionMap.java
@@ -92,7 +92,8 @@ public JdbcBackedSessionMap(Tracer tracer, Connection jdbcConnection, EventBus b
                     .forEach(this::remove)));
 
     bus.addListener(
-        NodeRestartedEvent.listener(nodeStatus -> this.removeByUri(nodeStatus.getExternalUri())));
+        NodeRestartedEvent.listener(
+            previousNodeStatus -> this.removeByUri(previousNodeStatus.getExternalUri())));
   }
 
   public static SessionMap create(Config config) {

diff --git a/java/src/org/openqa/selenium/grid/sessionmap/local/LocalSessionMap.java b/java/src/org/openqa/selenium/grid/sessionmap/local/LocalSessionMap.java
@@ -67,10 +67,11 @@ public LocalSessionMap(Tracer tracer, EventBus bus) {
 
     bus.addListener(
         NodeRestartedEvent.listener(
-            nodeStatus -> {
+            previousNodeStatus -> {
               List<SessionId> toRemove =
                   knownSessions.entrySet().stream()
-                      .filter((e) -> e.getValue().getUri().equals(nodeStatus.getExternalUri()))
+                      .filter(
+                          (e) -> e.getValue().getUri().equals(previousNodeStatus.getExternalUri()))
                       .map(Map.Entry::getKey)
                       .collect(Collectors.toList());
 

diff --git a/java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java b/java/src/org/openqa/selenium/grid/sessionmap/redis/RedisBackedSessionMap.java
@@ -87,7 +87,8 @@ public RedisBackedSessionMap(Tracer tracer, URI serverUri, EventBus bus) {
                     .forEach(this::remove)));
 
     bus.addListener(
-        NodeRestartedEvent.listener(nodeStatus -> this.removeByUri(nodeStatus.getExternalUri())));
+        NodeRestartedEvent.listener(
+            previousNodeStatus -> this.removeByUri(previousNodeStatus.getExternalUri())));
   }
 
   public static SessionMap create(Config config) {