Merge pull request #3998 from vespa-engine/freva/node-failer

Count Docker nodes when throttling in node failure

1 files changed, 53 insertions, 64 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
index 1c81d97ddea..266d91e7e3e 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.provision.maintenance;
 
 import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.Deployment;
-import com.yahoo.config.provision.Flavor;
 import com.yahoo.config.provision.HostLivenessTracker;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.transaction.Mutex;
@@ -24,12 +23,12 @@ import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
-import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 
 /**
  * Maintains information in the node repo about when this node last responded to ping
@@ -76,21 +75,15 @@ public class NodeFailer extends Maintainer {
     @Override
     protected void maintain() {
         // Ready nodes
-        updateNodeLivenessEventsForReadyNodes();
-        for (Node node : readyNodesWhichAreDead()) {
-            // Docker hosts and nodes do not run Vespa services
-            if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER || node.type() == NodeType.host) continue;
-            if ( ! throttle(node)) nodeRepository().fail(node.hostname(),
-                                                         Agent.system, "Not receiving config requests from node");
-        }
-
-        for (Node node : readyNodesWithHardwareFailure())
-            if ( ! throttle(node)) nodeRepository().fail(node.hostname(),
-                                                         Agent.system, "Node has hardware failure");
+        try (Mutex lock = nodeRepository().lockUnallocated()) {
+            updateNodeLivenessEventsForReadyNodes();
 
-        for (Node node: readyNodesWithHardwareDivergence())
-            if ( ! throttle(node)) nodeRepository().fail(node.hostname(),
-                                                         Agent.system, "Node hardware diverges from spec");
+            getReadyNodesByFailureReason().forEach((node, reason) -> {
+                if (!throttle(node)) {
+                    nodeRepository().fail(node.hostname(), Agent.system, reason);
+                }
+            });
+        }
 
         // Active nodes
         for (Node node : determineActiveNodeDownStatus()) {
@@ -103,59 +96,55 @@ public class NodeFailer extends Maintainer {
     private void updateNodeLivenessEventsForReadyNodes() {
         // Update node last request events through ZooKeeper to collect request to all config servers.
         // We do this here ("lazily") to avoid writing to zk for each config request.
-        try (Mutex lock = nodeRepository().lockUnallocated()) {
-            for (Node node : nodeRepository().getNodes(Node.State.ready)) {
-                Optional<Instant> lastLocalRequest = hostLivenessTracker.lastRequestFrom(node.hostname());
-                if ( ! lastLocalRequest.isPresent()) continue;
-
-                Optional<History.Event> recordedRequest = node.history().event(History.Event.Type.requested);
-                if ( ! recordedRequest.isPresent() || recordedRequest.get().at().isBefore(lastLocalRequest.get())) {
-                    History updatedHistory = node.history().with(new History.Event(History.Event.Type.requested,
-                                                                                   Agent.system,
-                                                                                   lastLocalRequest.get()));
-                    nodeRepository().write(node.with(updatedHistory));
-                }
+        for (Node node : nodeRepository().getNodes(Node.State.ready)) {
+            Optional<Instant> lastLocalRequest = hostLivenessTracker.lastRequestFrom(node.hostname());
+            if ( ! lastLocalRequest.isPresent()) continue;
+
+            Optional<History.Event> recordedRequest = node.history().event(History.Event.Type.requested);
+            if ( ! recordedRequest.isPresent() || recordedRequest.get().at().isBefore(lastLocalRequest.get())) {
+                History updatedHistory = node.history().with(new History.Event(History.Event.Type.requested,
+                        Agent.system,
+                        lastLocalRequest.get()));
+                nodeRepository().write(node.with(updatedHistory));
             }
         }
     }
 
-    private List<Node> readyNodesWhichAreDead() {
-        // Allow requests some time to be registered in case all config servers have been down
-        if (constructionTime.isAfter(clock.instant().minus(nodeRequestInterval).minus(nodeRequestInterval) ))
-            return Collections.emptyList();
-
-        // Nodes are taken as dead if they have not made a config request since this instant.
-        // Add 10 minutes to the down time limit to allow nodes to make a request that infrequently.
-        Instant oldestAcceptableRequestTime = clock.instant().minus(downTimeLimit).minus(nodeRequestInterval);
-
-        return nodeRepository().getNodes(Node.State.ready).stream()
-                .filter(node -> wasMadeReadyBefore(oldestAcceptableRequestTime, node))
-                .filter(node -> ! hasRecordedRequestAfter(oldestAcceptableRequestTime, node))
-                .collect(Collectors.toList());
+    private Map<Node, String> getReadyNodesByFailureReason() {
+        Instant oldestAcceptableRequestTime =
+                // Allow requests some time to be registered in case all config servers have been down
+                constructionTime.isAfter(clock.instant().minus(nodeRequestInterval.multipliedBy(2))) ?
+                        Instant.EPOCH :
+
+                        // Nodes are taken as dead if they have not made a config request since this instant.
+                        // Add 10 minutes to the down time limit to allow nodes to make a request that infrequently.
+                        clock.instant().minus(downTimeLimit).minus(nodeRequestInterval);
+
+        Map<Node, String> nodesByFailureReason = new HashMap<>();
+        for (Node node : nodeRepository().getNodes(Node.State.ready)) {
+            if (! hasNodeRequestedConfigAfter(node, oldestAcceptableRequestTime)) {
+                nodesByFailureReason.put(node, "Not receiving config requests from node");
+            } else if (node.status().hardwareFailureDescription().isPresent()) {
+                nodesByFailureReason.put(node, "Node has hardware failure");
+            } else if (node.status().hardwareDivergence().isPresent()) {
+                nodesByFailureReason.put(node, "Node has hardware divergence");
+            }
+        }
+        return nodesByFailureReason;
     }
 
-    private List<Node> readyNodesWithHardwareDivergence() {
-        return nodeRepository().getNodes(Node.State.ready).stream()
-                .filter(node -> node.status().hardwareDivergence().isPresent())
-                .collect(Collectors.toList());
+    private boolean hasNodeRequestedConfigAfter(Node node, Instant instant) {
+        return !wasMadeReadyBefore(node, instant) || hasRecordedRequestAfter(node, instant);
     }
 
-    private boolean wasMadeReadyBefore(Instant instant, Node node) {
+    private boolean wasMadeReadyBefore(Node node, Instant instant) {
         Optional<History.Event> readiedEvent = node.history().event(History.Event.Type.readied);
-        if ( ! readiedEvent.isPresent()) return false;
-        return readiedEvent.get().at().isBefore(instant);
+        return readiedEvent.map(event -> event.at().isBefore(instant)).orElse(false);
     }
 
-    private boolean hasRecordedRequestAfter(Instant instant, Node node) {
+    private boolean hasRecordedRequestAfter(Node node, Instant instant) {
         Optional<History.Event> lastRequest = node.history().event(History.Event.Type.requested);
-        if ( ! lastRequest.isPresent()) return false;
-        return lastRequest.get().at().isAfter(instant);
-    }
-
-    private List<Node> readyNodesWithHardwareFailure() {
-        return nodeRepository().getNodes(Node.State.ready).stream()
-                .filter(node -> node.status().hardwareFailureDescription().isPresent())
-                .collect(Collectors.toList());
+        return lastRequest.map(event -> event.at().isAfter(instant)).orElse(false);
     }
 
     private boolean applicationSuspended(Node node) {
@@ -272,18 +261,18 @@ public class NodeFailer extends Maintainer {
     private boolean throttle(Node node) {
         if (throttlePolicy == ThrottlePolicy.disabled) return false;
         Instant startOfThrottleWindow = clock.instant().minus(throttlePolicy.throttleWindow);
-        List<Node> nodes = nodeRepository().getNodes().stream()
-                // Do not consider Docker containers when throttling
-                .filter(n -> n.flavor().getType() != Flavor.Type.DOCKER_CONTAINER)
-                .collect(Collectors.toList());
+        List<Node> nodes = nodeRepository().getNodes();
         long recentlyFailedNodes = nodes.stream()
                 .map(n -> n.history().event(History.Event.Type.failed))
                 .filter(Optional::isPresent)
                 .map(Optional::get)
                 .filter(failedEvent -> failedEvent.at().isAfter(startOfThrottleWindow))
                 .count();
-        boolean throttle = recentlyFailedNodes >= Math.max(nodes.size() * throttlePolicy.fractionAllowedToFail,
-                                                           throttlePolicy.minimumAllowedToFail);
+        int allowedFailedNodes = (int) Math.max(nodes.size() * throttlePolicy.fractionAllowedToFail,
+                throttlePolicy.minimumAllowedToFail);
+
+        boolean throttle = allowedFailedNodes < recentlyFailedNodes ||
+                (allowedFailedNodes == recentlyFailedNodes && node.type() != NodeType.host);
         if (throttle) {
             log.info(String.format("Want to fail node %s, but throttling is in effect: %s", node.hostname(),
                                    throttlePolicy.toHumanReadableString()));