Track config requests and fail ready hosts which don't make them

3 files changed, 151 insertions, 17 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 9a116478882..e216a5c9354 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,6 +3,7 @@ package com.yahoo.vespa.hosted.provision.maintenance;
 
 import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.Deployment;
+import com.yahoo.config.provision.HostLivenessTracker;
 import com.yahoo.transaction.Mutex;
 import com.yahoo.vespa.applicationmodel.ApplicationInstance;
 import com.yahoo.vespa.applicationmodel.ServiceCluster;
@@ -20,6 +21,7 @@ import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.logging.Level;
@@ -36,17 +38,24 @@ public class NodeFailer extends Maintainer {
 
     private static final Logger log = Logger.getLogger(NodeFailer.class.getName());
 
-    private final Deployer deployer;
+    /** Provides information about the status of ready hosts */
+    private final HostLivenessTracker hostLivenessTracker;
+    
+    /** Provides (more accurate) information about the status of active hosts */
     private final ServiceMonitor serviceMonitor;
+
+    private final Deployer deployer;
     private final Duration downTimeLimit;
     private final Clock clock;
     private final Orchestrator orchestrator;
 
-    public NodeFailer(Deployer deployer, ServiceMonitor serviceMonitor, NodeRepository nodeRepository,
+    public NodeFailer(Deployer deployer, HostLivenessTracker hostLivenessTracker, 
+                      ServiceMonitor serviceMonitor, NodeRepository nodeRepository,
                       Duration downTimeLimit, Clock clock, Orchestrator orchestrator) {
         // check ping status every five minutes, but at least twice as often as the down time limit
         super(nodeRepository, min(downTimeLimit.dividedBy(2), Duration.ofMinutes(5)));
         this.deployer = deployer;
+        this.hostLivenessTracker = hostLivenessTracker;
         this.serviceMonitor = serviceMonitor;
         this.downTimeLimit = downTimeLimit;
         this.clock = clock;
@@ -59,27 +68,57 @@ public class NodeFailer extends Maintainer {
 
     @Override
     protected void maintain() {
+        // Ready nodes
+        for (Node node : readyNodesWithHardwareFailure())
+            nodeRepository().fail(node.hostname());
+        for (Node node : readyNodesWhichAreDead())
+            nodeRepository().fail(node.hostname());
+
+        // Active nodes
         for (Node node : determineActiveNodeDownStatus()) {
             Instant graceTimeEnd = node.history().event(History.Event.Type.down).get().at().plus(downTimeLimit);
             if (graceTimeEnd.isBefore(clock.instant()) && ! applicationSuspended(node))
                 failActive(node);
         }
         
-        for (Node node : readyNodesWithHardwareFailure()) {
-            nodeRepository().fail(node.hostname());
-        }
     }
 
     private List<Node> readyNodesWithHardwareFailure() {
         return nodeRepository().getNodes(Node.Type.tenant, Node.State.ready).stream()
-                .filter(n -> n.status().hardwareFailure())
+                .filter(node -> node.status().hardwareFailure())
                 .collect(Collectors.toList());
     }
     
+    private List<Node> readyNodesWhichAreDead() {
+        // 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(Duration.ofMinutes(10));
+        
+        if ( ! hostLivenessTracker.remembersRequestsSince().isBefore(oldestAcceptableRequestTime))
+            return Collections.emptyList(); // we haven't tracked long enough to tell if nodes are dead
+
+        return nodeRepository().getNodes(Node.Type.tenant, Node.State.ready).stream()
+                .filter(node -> wasMadeReadyBefore(oldestAcceptableRequestTime, node))
+                .filter(node -> ! hasRecordedResponseAfter(oldestAcceptableRequestTime, node))
+                .collect(Collectors.toList());
+    }
+
+    private boolean wasMadeReadyBefore(Instant instant, Node node) {
+        Optional<History.Event> readiedEvent = node.history().event(History.Event.Type.readied);
+        if ( ! readiedEvent.isPresent()) return false;
+        return readiedEvent.get().at().isBefore(instant);
+    }
+    
+    private boolean hasRecordedResponseAfter(Instant instant, Node node) {
+        Optional<Instant> lastResponse = hostLivenessTracker.lastRequestFrom(node.hostname());
+        if ( ! lastResponse.isPresent()) return false;
+        return lastResponse.get().isAfter(instant);
+    }
+    
     private boolean applicationSuspended(Node node) {
         try {
             return orchestrator.getApplicationInstanceStatus(node.allocation().get().owner())
-                    == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN;
+                   == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN;
         } catch (ApplicationIdNotFoundException e) {
             //Treat it as not suspended and allow to fail the node anyway
             return false;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
index c23a0fa5f56..2bc826adc30 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRepositoryMaintenance.java
@@ -5,6 +5,7 @@ import com.google.inject.Inject;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.Environment;
+import com.yahoo.config.provision.HostLivenessTracker;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.orchestrator.Orchestrator;
@@ -30,13 +31,17 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
     private final DirtyExpirer dirtyExpirer;
 
     @Inject
-    public NodeRepositoryMaintenance(NodeRepository nodeRepository, Deployer deployer, ServiceMonitor serviceMonitor, Zone zone, Orchestrator orchestrator) {
-        this(nodeRepository, deployer, serviceMonitor, zone, Clock.systemUTC(), orchestrator);
+    public NodeRepositoryMaintenance(NodeRepository nodeRepository, Deployer deployer, 
+                                     HostLivenessTracker hostLivenessTracker, ServiceMonitor serviceMonitor, 
+                                     Zone zone, Orchestrator orchestrator) {
+        this(nodeRepository, deployer, hostLivenessTracker, serviceMonitor, zone, Clock.systemUTC(), orchestrator);
     }
 
-    public NodeRepositoryMaintenance(NodeRepository nodeRepository, Deployer deployer, ServiceMonitor serviceMonitor, Zone zone, Clock clock, Orchestrator orchestrator) {
+    public NodeRepositoryMaintenance(NodeRepository nodeRepository, Deployer deployer, 
+                                     HostLivenessTracker hostLivenessTracker, ServiceMonitor serviceMonitor, 
+                                     Zone zone, Clock clock, Orchestrator orchestrator) {
         DefaultTimes defaults = new DefaultTimes(zone.environment());
-        nodeFailer = new NodeFailer(deployer, serviceMonitor, nodeRepository, fromEnv("fail_grace").orElse(defaults.failGrace), clock, orchestrator);
+        nodeFailer = new NodeFailer(deployer, hostLivenessTracker, serviceMonitor, nodeRepository, fromEnv("fail_grace").orElse(defaults.failGrace), clock, orchestrator);
         applicationMaintainer = new ApplicationMaintainer(deployer, nodeRepository, fromEnv("redeploy_frequency").orElse(defaults.redeployFrequency));
         reservationExpirer = new ReservationExpirer(nodeRepository, clock, fromEnv("reservation_expiry").orElse(defaults.reservationExpiry));
         retiredExpirer = new RetiredExpirer(nodeRepository, deployer, clock, fromEnv("retired_expiry").orElse(defaults.retiredExpiry));
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
index d12eb6ffe0b..e86316d11f0 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
@@ -1,11 +1,13 @@
 // Copyright 2016 Yahoo Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.maintenance;
 
+import com.google.inject.Inject;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
+import com.yahoo.config.provision.HostLivenessTracker;
 import com.yahoo.config.provision.HostSpec;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
@@ -47,8 +49,11 @@ import com.yahoo.vespa.service.monitor.ServiceMonitorStatus;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.time.Clock;
 import java.time.Duration;
+import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -76,6 +81,7 @@ public class NodeFailerTest {
     // Components with state
     private ManualClock clock;
     private Curator curator;
+    private TestHostLivenessTracker hostLivenessTracker;
     private ServiceMonitorStub serviceMonitor;
     private MockDeployer deployer;
     private NodeRepository nodeRepository;
@@ -107,13 +113,12 @@ public class NodeFailerTest {
         apps.put(APP_1, new MockDeployer.ApplicationContext(APP_1, clusterApp1, wantedNodesApp1, Optional.of("default"), 1));
         apps.put(APP_2, new MockDeployer.ApplicationContext(APP_2, clusterApp2, wantedNodesApp2, Optional.of("default"), 1));
         deployer = new MockDeployer(provisioner, apps);
-        // ... and a service monitor
+        // ... and the other services
+        hostLivenessTracker = new TestHostLivenessTracker(clock);
         serviceMonitor = new ServiceMonitorStub(apps, nodeRepository);
-
         orchestrator = new OrchestratorMock();
 
-        failer = new NodeFailer(deployer, serviceMonitor, nodeRepository,
-                DOWNTIME_LIMIT_ONE_HOUR, clock, orchestrator);
+        failer = new NodeFailer(deployer, hostLivenessTracker, serviceMonitor, nodeRepository, DOWNTIME_LIMIT_ONE_HOUR, clock, orchestrator);
     }
 
     @Test
@@ -139,6 +144,8 @@ public class NodeFailerTest {
         for (int minutes = 0; minutes < 24 * 60; minutes +=5 ) {
             failer.run();
             clock.advance(Duration.ofMinutes(5));
+            allNodesMakeAConfigRequestExcept();
+
             assertEquals( 0, deployer.redeployments);
             assertEquals(12, nodeRepository.getNodes(Node.Type.tenant, Node.State.active).size());
             assertEquals( 0, nodeRepository.getNodes(Node.Type.tenant, Node.State.failed).size());
@@ -164,6 +171,7 @@ public class NodeFailerTest {
         for (int minutes = 0; minutes < 45; minutes +=5 ) {
             failer.run();
             clock.advance(Duration.ofMinutes(5));
+            allNodesMakeAConfigRequestExcept();
             assertEquals( 0, deployer.redeployments);
             assertEquals(12, nodeRepository.getNodes(Node.Type.tenant, Node.State.active).size());
             assertEquals( 2, nodeRepository.getNodes(Node.Type.tenant, Node.State.failed).size());
@@ -173,6 +181,7 @@ public class NodeFailerTest {
         for (int minutes = 0; minutes < 30; minutes +=5 ) {
             failer.run();
             clock.advance(Duration.ofMinutes(5));
+            allNodesMakeAConfigRequestExcept();
         }
 
         // downHost2 should now be failed and replaced, but not downHost1
@@ -186,8 +195,10 @@ public class NodeFailerTest {
         serviceMonitor.setHostDown(downHost1);
         failer.run();
         clock.advance(Duration.ofMinutes(5));
+        allNodesMakeAConfigRequestExcept();
         // the system goes down and do not have updated information when coming back
         clock.advance(Duration.ofMinutes(120));
+        hostLivenessTracker.setConstructedNow();
         serviceMonitor.setStatusIsKnown(false);
         failer.run();
         // due to this, nothing is failed
@@ -197,6 +208,7 @@ public class NodeFailerTest {
         assertEquals( 1, nodeRepository.getNodes(Node.Type.tenant, Node.State.ready).size());
         // when status becomes known, and the host is still down, it is failed
         clock.advance(Duration.ofMinutes(5));
+        allNodesMakeAConfigRequestExcept();
         serviceMonitor.setStatusIsKnown(true);
         failer.run();
         assertEquals( 2, deployer.redeployments);
@@ -211,6 +223,7 @@ public class NodeFailerTest {
         for (int minutes = 0; minutes < 75; minutes +=5 ) {
             failer.run();
             clock.advance(Duration.ofMinutes(5));
+            allNodesMakeAConfigRequestExcept();
             assertEquals( 2, deployer.redeployments);
             assertEquals(12, nodeRepository.getNodes(Node.Type.tenant, Node.State.active).size());
             assertEquals( 4, nodeRepository.getNodes(Node.Type.tenant, Node.State.failed).size());
@@ -230,7 +243,51 @@ public class NodeFailerTest {
                    >
                    lastNode.allocation().get().membership().index());
     }
+    
+    @Test
+    public void testFailingReadyNodes() {
+        // For a day all nodes work so nothing happens
+        for (int minutes = 0; minutes < 24 * 60; minutes +=5 ) {
+            clock.advance(Duration.ofMinutes(5));
+            allNodesMakeAConfigRequestExcept();
+            failer.run();
+            assertEquals( 4, nodeRepository.getNodes(Node.Type.tenant, Node.State.ready).size());
+        }
+        
+        List<Node> ready = nodeRepository.getNodes(Node.Type.tenant, Node.State.ready);
 
+        // Two ready nodes die
+        clock.advance(Duration.ofMinutes(180));
+        allNodesMakeAConfigRequestExcept(ready.get(0), ready.get(2));
+        failer.run();
+        assertEquals( 2, nodeRepository.getNodes(Node.Type.tenant, Node.State.ready).size());
+        assertEquals( 2, nodeRepository.getNodes(Node.Type.tenant, Node.State.failed).size());
+
+        // Another ready node die but we restart so we don't have enough information
+        clock.advance(Duration.ofMinutes(180));
+        hostLivenessTracker.setConstructedNow();
+        allNodesMakeAConfigRequestExcept(ready.get(0), ready.get(2), ready.get(3));
+        failer.run();
+        assertEquals( 2, nodeRepository.getNodes(Node.Type.tenant, Node.State.ready).size());
+        assertEquals( 2, nodeRepository.getNodes(Node.Type.tenant, Node.State.failed).size());
+        
+        // Now we get enough information
+        clock.advance(Duration.ofMinutes(180));
+        allNodesMakeAConfigRequestExcept(ready.get(0), ready.get(2), ready.get(3));
+        failer.run();
+        assertEquals( 1, nodeRepository.getNodes(Node.Type.tenant, Node.State.ready).size());
+        assertEquals(ready.get(1), nodeRepository.getNodes(Node.Type.tenant, Node.State.ready).get(0));
+        assertEquals( 3, nodeRepository.getNodes(Node.Type.tenant, Node.State.failed).size());
+    }
+    
+    private void allNodesMakeAConfigRequestExcept(Node ... deadNodeArray) {
+        Set<Node> deadNodes = new HashSet<>(Arrays.asList(deadNodeArray));
+        for (Node node : nodeRepository.getNodes(Node.Type.tenant)) {
+            if ( ! deadNodes.contains(node))
+                hostLivenessTracker.receivedRequestFrom(node.hostname());
+        }
+    }
+    
     private void createReadyNodes(int count, NodeRepository nodeRepository, NodeFlavors nodeFlavors) {
         createReadyNodes(count, 0, nodeRepository, nodeFlavors);
     }
@@ -269,6 +326,38 @@ public class NodeFailerTest {
         return highestIndex;
     }
 
+    /** This is a fully functional implementation */
+    private static class TestHostLivenessTracker implements HostLivenessTracker {
+
+        private final Clock clock;
+        private Instant constructionTime;
+        private final Map<String, Instant> lastRequestFromHost = new HashMap<>();
+
+        public TestHostLivenessTracker(Clock clock) {
+            this.clock = clock;
+            this.constructionTime = clock.instant();
+        }
+        
+        public void setConstructedNow() {
+            constructionTime = clock.instant();
+            lastRequestFromHost.clear();
+        }
+
+        @Override
+        public Instant remembersRequestsSince() { return constructionTime; }
+
+        @Override
+        public void receivedRequestFrom(String hostname) {
+            lastRequestFromHost.put(hostname, clock.instant());
+        }
+
+        @Override
+        public Optional<Instant> lastRequestFrom(String hostname) {
+            return Optional.ofNullable(lastRequestFromHost.get(hostname));
+        }
+
+    }
+
     private static class ServiceMonitorStub implements ServiceMonitor {
 
         private final Map<ApplicationId, MockDeployer.ApplicationContext> apps;
@@ -343,8 +432,8 @@ public class NodeFailerTest {
 
         @Override
         public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) throws ApplicationIdNotFoundException {
-            return suspendedApplications.contains(appId) ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN :
-                    ApplicationInstanceStatus.NO_REMARKS;
+            return suspendedApplications.contains(appId) 
+                                ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : ApplicationInstanceStatus.NO_REMARKS;
         }
 
         @Override
@@ -367,4 +456,5 @@ public class NodeFailerTest {
             throw new RuntimeException("Not implemented");
         }
     }
+
 }