Be down initially

Before ths change containers used to come up in the initial VIP state of up
("in rotation") before having any information about downstream cløusters.
This is problematic if the node determines that it should be
down when learning the state of downstream clusters, especially
when it is determining that it should be down due to failing to complete
pings timely due to initialization load.

This changes to starting in the down state. Crucially, this requires
tracking the current state of clusters in a separate component
such that the state survives reconfiguration and we avoid setting the
state down briefly on each reconfiguration.

In addition to the case where there are downstream clusters deciding
the VIP state oif a container, there are two others:
- There are no such clusters. In this case the VIP state will be up by default.
- Some other component is explicitly deciding the VIP state. This commit makes
  that case clearer.

6 files changed, 170 insertions, 101 deletions

diff --git a/container-core/src/main/java/com/yahoo/container/handler/ClustersStatus.java b/container-core/src/main/java/com/yahoo/container/handler/ClustersStatus.java
new file mode 100644
index 00000000000..19617b46a84
--- /dev/null
+++ b/container-core/src/main/java/com/yahoo/container/handler/ClustersStatus.java
@@ -0,0 +1,75 @@
+package com.yahoo.container.handler;
+
+import com.google.inject.Inject;
+import com.yahoo.component.AbstractComponent;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * A component which tracks the up/down status of any clusters which should influence
+ * the up down status of this container itself, as well as the separate fact that such clusters are present.
+ *
+ * This is a separate component which has <b>no dependencies</b> such that the status tracked in this
+ * will survive reconfiguration events and inform other components even immediately after a reconfiguration
+ * (where the true statue of clusters may not yet be available).
+ *
+ * @author bratseth
+ */
+public class ClustersStatus extends AbstractComponent {
+
+    // NO DEPENDENCIES: Do not add dependencies here
+    @Inject
+    public ClustersStatus() { }
+
+    /** Are there any (in-service influencing) clusters in this container? */
+    private boolean containerHasClusters;
+
+    /** If we have no clusters, what should we answer? */
+    private boolean receiveTrafficByDefault;
+
+    private final Object mutex = new Object();
+
+    /** The status of clusters, when known. Note that clusters may exist for which there is no knowledge yet. */
+    private final Map<Object, Boolean> clusterStatus = new HashMap<>();
+
+    public void setContainerHasClusters(boolean containerHasClusters) {
+        synchronized (mutex) {
+            this.containerHasClusters = containerHasClusters;
+            if ( ! containerHasClusters)
+                clusterStatus.clear(); // forget container clusters which was configured away
+        }
+    }
+
+    public void setReceiveTrafficByDefault(boolean receiveTrafficByDefault) {
+        synchronized (mutex) {
+            this.receiveTrafficByDefault = receiveTrafficByDefault;
+        }
+    }
+
+    public void setUp(Object clusterIdentifier) {
+        synchronized (mutex) {
+            clusterStatus.put(clusterIdentifier, Boolean.TRUE);
+        }
+    }
+
+    public void setDown(Object clusterIdentifier) {
+        synchronized (mutex) {
+            clusterStatus.put(clusterIdentifier, Boolean.FALSE);
+        }
+    }
+
+    /** Returns whether this container should receive traffic based on the state of this */
+    public boolean containerShouldReceiveTraffic() {
+        synchronized (mutex) {
+            if (containerHasClusters) {
+                // Should receive traffic when at least one cluster is up
+                return clusterStatus.values().stream().anyMatch(status -> status==true);
+            }
+            else {
+                return receiveTrafficByDefault;
+            }
+        }
+    }
+
+}
diff --git a/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java b/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java
index d7457140dae..be386b1b84e 100644
--- a/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java
+++ b/container-core/src/main/java/com/yahoo/container/handler/VipStatus.java
@@ -15,70 +15,56 @@ import com.yahoo.container.core.VipStatusConfig;
  */
 public class VipStatus {
 
-    private final Map<Object, Boolean> clusters = new IdentityHashMap<>();
-    private final VipStatusConfig vipStatusConfig;
+    private final ClustersStatus clustersStatus;
+
+    /** If this is non-null, its value decides whether this container is in rotation */
+    private Boolean inRotationOverride;
 
     public VipStatus() {
-        this(null, new VipStatusConfig(new VipStatusConfig.Builder()));
+        this(new QrSearchersConfig(new QrSearchersConfig.Builder()),
+             new VipStatusConfig(new VipStatusConfig.Builder()),
+             new ClustersStatus());
     }
 
     public VipStatus(QrSearchersConfig dispatchers) {
-        this(dispatchers, new VipStatusConfig(new VipStatusConfig.Builder()));
+        this(dispatchers, new VipStatusConfig(new VipStatusConfig.Builder()), new ClustersStatus());
+    }
+
+    public VipStatus(ClustersStatus clustersStatus) {
+        this.clustersStatus = clustersStatus;
     }
 
-    // TODO: Why use QrSearchersConfig here? Remove and inject ComponentRegistry<ClusterSearcher> instead?
     @Inject
-    public VipStatus(QrSearchersConfig dispatchers, VipStatusConfig vipStatusConfig) {
-        // the config is not used for anything, it's just a dummy to create a
-        // dependency link to which dispatchers are used
-        this.vipStatusConfig = vipStatusConfig;
+    public VipStatus(QrSearchersConfig dispatchers, VipStatusConfig vipStatusConfig, ClustersStatus clustersStatus) {
+        this.clustersStatus = clustersStatus;
+        clustersStatus.setReceiveTrafficByDefault(vipStatusConfig.initiallyInRotation());
+        clustersStatus.setContainerHasClusters(! dispatchers.searchcluster().isEmpty());
     }
 
     /**
-     * Set a service or cluster into rotation.
+     * Explicitly set this container in or out of rotation
      *
-     * @param clusterIdentifier
-     *            an object where the object identity will serve to identify the
-     *            cluster or service
+     * @param inRotation true to set this in rotation regardless of any clusters and of the default value,
+     *                   false to set it out, and null to make this decision using the usual cluster-dependent logic
      */
+    public void setInRotation(Boolean inRotation) {
+        this.inRotationOverride = inRotation;
+    }
+
+    /** Note that a cluster (which influences up/down state) is up */
     public void addToRotation(Object clusterIdentifier) {
-        synchronized (clusters) {
-            clusters.put(clusterIdentifier, Boolean.TRUE);
-        }
+        clustersStatus.setUp(clusterIdentifier);
     }
 
-    /**
-     * Set a service or cluster out of rotation.
-     *
-     * @param clusterIdentifier
-     *            an object where the object identity will serve to identify the
-     *            cluster or service
-     */
+    /** Note that a cluster (which influences up/down state) is down */
     public void removeFromRotation(Object clusterIdentifier) {
-        synchronized (clusters) {
-            clusters.put(clusterIdentifier, Boolean.FALSE);
-        }
+        clustersStatus.setDown(clusterIdentifier);
     }
 
-    /**
-     * Tell whether the container is connected to any active services at all.
-     *
-     * @return true if at least one service or cluster is up, or value is taken from config if no services
-     *         are registered (yet)
-     */
+    /** Returns whether this container should receive traffic at this time */
     public boolean isInRotation() {
-        synchronized (clusters) {
-            // if no stored state, use config to decide whether to serve or not
-            if (clusters.size() == 0) {
-                return vipStatusConfig.initiallyInRotation();
-            }
-            for (Boolean inRotation : clusters.values()) {
-                if (inRotation) {
-                    return true;
-                }
-            }
-        }
-        return false;
+        if (inRotationOverride != null) return inRotationOverride;
+        return clustersStatus.containerShouldReceiveTraffic();
     }
 
 }
diff --git a/container-core/src/main/java/com/yahoo/container/handler/VipStatusHandler.java b/container-core/src/main/java/com/yahoo/container/handler/VipStatusHandler.java
index b7977e7832d..60affddeb60 100644
--- a/container-core/src/main/java/com/yahoo/container/handler/VipStatusHandler.java
+++ b/container-core/src/main/java/com/yahoo/container/handler/VipStatusHandler.java
@@ -49,7 +49,7 @@ public final class VipStatusHandler extends ThreadedHttpRequestHandler {
     class StatusResponse extends HttpResponse {
 
         static final String COULD_NOT_FIND_STATUS_FILE = "Could not find status file.\n";
-        static final String NO_SEARCH_BACKENDS = "No search backends available, VIP status disabled.";
+        static final String NO_SEARCH_BACKENDS = "No search backends available, VIP status disabled."; // TODO: Generify
         private static final String TEXT_HTML = "text/html";
         private String contentType = TEXT_HTML;
         private byte[] data = null;
diff --git a/container-core/src/main/resources/configdefinitions/vip-status.def b/container-core/src/main/resources/configdefinitions/vip-status.def
index 1e364419ab8..8e7454e35d7 100644
--- a/container-core/src/main/resources/configdefinitions/vip-status.def
+++ b/container-core/src/main/resources/configdefinitions/vip-status.def
@@ -4,6 +4,7 @@ namespace=container.core
 ## If there is a Vespa search backend connected to this container, and that
 ## backend is out of service, automatically remove this container from VIP
 ## rotation, ignoring any status file.
+## TODO: This is always true and can be removed
 noSearchBackendsImpliesOutOfService bool default=true
 
 ## Whether to return hard-coded reply or serve "status.html" from disk
@@ -13,5 +14,5 @@ accessdisk bool default=false
 ## If the path is relative vespa home is prepended
 statusfile string default="share/qrsdocs/status.html"
 
-## The initial rotation state when no information is known about backend clusters
+## The default rotation state when there are no configured clusters to decide rotation state
 initiallyInRotation bool default=true
diff --git a/container-core/src/test/java/com/yahoo/container/handler/VipStatusHandlerTestCase.java b/container-core/src/test/java/com/yahoo/container/handler/VipStatusHandlerTestCase.java
index ef51a3c0f51..f8de32ee3ff 100644
--- a/container-core/src/test/java/com/yahoo/container/handler/VipStatusHandlerTestCase.java
+++ b/container-core/src/test/java/com/yahoo/container/handler/VipStatusHandlerTestCase.java
@@ -35,16 +35,16 @@ import static org.junit.Assert.fail;
  * care about the incoming URI, that's 100% handled in JDIsc by the binding
  * pattern.
  *
- * @author <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a>
+ * @author Steinar Knutsen
  */
 public class VipStatusHandlerTestCase {
 
-    public static final class MockResponseHandler implements ResponseHandler {
+    public static class MockResponseHandler implements ResponseHandler {
+
         final ReadableContentChannel channel = new ReadableContentChannel();
 
         @Override
-        public ContentChannel handleResponse(
-                final com.yahoo.jdisc.Response response) {
+        public ContentChannel handleResponse(com.yahoo.jdisc.Response response) {
             return channel;
         }
     }
@@ -52,45 +52,44 @@ public class VipStatusHandlerTestCase {
     Metric metric = Mockito.mock(Metric.class);
 
     @Test
-    public final void testHandleRequest() {
-        final VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(false)
+    public void testHandleRequest() {
+        VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(false)
                 .noSearchBackendsImpliesOutOfService(false));
-        final VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config, metric);
-        final MockResponseHandler responseHandler = new MockResponseHandler();
-        final HttpRequest request = createRequest();
-        final BufferedContentChannel requestContent = createChannel();
+        VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config, metric);
+        MockResponseHandler responseHandler = new MockResponseHandler();
+        HttpRequest request = createRequest();
+        BufferedContentChannel requestContent = createChannel();
         handler.handleRequest(request, requestContent, responseHandler);
-        final ByteBuffer b = responseHandler.channel.read();
-        final byte[] asBytes = new byte[b.remaining()];
+        ByteBuffer b = responseHandler.channel.read();
+        byte[] asBytes = new byte[b.remaining()];
         b.get(asBytes);
         assertEquals(VipStatusHandler.OK_MESSAGE, Utf8.toString(asBytes));
     }
 
-    public static final class NotFoundResponseHandler implements
-            ResponseHandler {
+    public static class NotFoundResponseHandler implements ResponseHandler {
+
         final ReadableContentChannel channel = new ReadableContentChannel();
 
         @Override
-        public ContentChannel handleResponse(
-                final com.yahoo.jdisc.Response response) {
-            assertEquals(com.yahoo.jdisc.Response.Status.NOT_FOUND,
-                    response.getStatus());
+        public ContentChannel handleResponse(com.yahoo.jdisc.Response response) {
+            assertEquals(com.yahoo.jdisc.Response.Status.NOT_FOUND, response.getStatus());
             return channel;
         }
+
     }
 
     @Test
-    public final void testFileNotFound() {
-        final VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(true)
+    public void testFileNotFound() {
+        VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(true)
                 .statusfile("/VipStatusHandlerTestCaseFileThatReallyReallyShouldNotExist")
                 .noSearchBackendsImpliesOutOfService(false));
-        final VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config, metric);
-        final NotFoundResponseHandler responseHandler = new NotFoundResponseHandler();
-        final HttpRequest request = createRequest();
-        final BufferedContentChannel requestContent = createChannel();
+        VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config, metric);
+        NotFoundResponseHandler responseHandler = new NotFoundResponseHandler();
+        HttpRequest request = createRequest();
+        BufferedContentChannel requestContent = createChannel();
         handler.handleRequest(request, requestContent, responseHandler);
-        final ByteBuffer b = responseHandler.channel.read();
-        final byte[] asBytes = new byte[b.remaining()];
+        ByteBuffer b = responseHandler.channel.read();
+        byte[] asBytes = new byte[b.remaining()];
         b.get(asBytes);
         assertEquals(
                 VipStatusHandler.StatusResponse.COULD_NOT_FIND_STATUS_FILE,
@@ -98,23 +97,22 @@ public class VipStatusHandlerTestCase {
     }
 
     @Test
-    public final void testFileFound() throws IOException {
-        final File statusFile = File.createTempFile("VipStatusHandlerTestCase",
-                null);
+    public void testFileFound() throws IOException {
+        File statusFile = File.createTempFile("VipStatusHandlerTestCase", null);
         try {
-            final FileWriter writer = new FileWriter(statusFile);
-            final String OK = "OK\n";
+            FileWriter writer = new FileWriter(statusFile);
+            String OK = "OK\n";
             writer.write(OK);
             writer.close();
-            final VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(true)
+            VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(true)
                     .statusfile(statusFile.getAbsolutePath()).noSearchBackendsImpliesOutOfService(false));
-            final VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config, metric);
-            final MockResponseHandler responseHandler = new MockResponseHandler();
-            final HttpRequest request = createRequest();
-            final BufferedContentChannel requestContent = createChannel();
+            VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config, metric);
+            MockResponseHandler responseHandler = new MockResponseHandler();
+            HttpRequest request = createRequest();
+            BufferedContentChannel requestContent = createChannel();
             handler.handleRequest(request, requestContent, responseHandler);
-            final ByteBuffer b = responseHandler.channel.read();
-            final byte[] asBytes = new byte[b.remaining()];
+            ByteBuffer b = responseHandler.channel.read();
+            byte[] asBytes = new byte[b.remaining()];
             b.get(asBytes);
             assertEquals(OK, Utf8.toString(asBytes));
         } finally {
@@ -123,34 +121,34 @@ public class VipStatusHandlerTestCase {
     }
 
     @Test
-    public final void testProgrammaticallyRemovedFromRotation() throws IOException {
+    public void testExplicitlyRotationControl() {
         VipStatus vipStatus = new VipStatus();
-        final VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(false)
+        VipStatusConfig config = new VipStatusConfig(new VipStatusConfig.Builder().accessdisk(false)
                 .noSearchBackendsImpliesOutOfService(true));
-        final VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config,  metric, vipStatus);
+        VipStatusHandler handler = new VipStatusHandler(Executors.newCachedThreadPool(), config,  metric, vipStatus);
 
-        vipStatus.removeFromRotation(this);
+        vipStatus.setInRotation(false);
 
         {
-            final MockResponseHandler responseHandler = new MockResponseHandler();
-            final HttpRequest request = createRequest();
-            final BufferedContentChannel requestContent = createChannel();
+            MockResponseHandler responseHandler = new MockResponseHandler();
+            HttpRequest request = createRequest();
+            BufferedContentChannel requestContent = createChannel();
             handler.handleRequest(request, requestContent, responseHandler);
-            final ByteBuffer b = responseHandler.channel.read();
-            final byte[] asBytes = new byte[b.remaining()];
+            ByteBuffer b = responseHandler.channel.read();
+            byte[] asBytes = new byte[b.remaining()];
             b.get(asBytes);
             assertEquals(VipStatusHandler.StatusResponse.NO_SEARCH_BACKENDS, Utf8.toString(asBytes));
         }
 
-        vipStatus.addToRotation(this);
+        vipStatus.setInRotation(true);
 
         {
-            final MockResponseHandler responseHandler = new MockResponseHandler();
-            final HttpRequest request = createRequest();
-            final BufferedContentChannel requestContent = createChannel();
+            MockResponseHandler responseHandler = new MockResponseHandler();
+            HttpRequest request = createRequest();
+            BufferedContentChannel requestContent = createChannel();
             handler.handleRequest(request, requestContent, responseHandler);
-            final ByteBuffer b = responseHandler.channel.read();
-            final byte[] asBytes = new byte[b.remaining()];
+            ByteBuffer b = responseHandler.channel.read();
+            byte[] asBytes = new byte[b.remaining()];
             b.get(asBytes);
             assertEquals(VipStatusHandler.OK_MESSAGE, Utf8.toString(asBytes));
         }
diff --git a/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java b/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java
index 725f8256ba3..e54f968f41d 100644
--- a/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java
+++ b/container-core/src/test/java/com/yahoo/container/handler/VipStatusTestCase.java
@@ -13,13 +13,22 @@ import org.junit.Test;
 public class VipStatusTestCase {
 
     @Test
-    public final void testSmoke() {
+    public void testVipStatusWorksWithClusters() {
+        ClustersStatus clustersStatus = new ClustersStatus();
+        clustersStatus.setContainerHasClusters(true);
+        VipStatus v = new VipStatus(clustersStatus);
+
         Object cluster1 = new Object();
         Object cluster2 = new Object();
         Object cluster3 = new Object();
-        VipStatus v = new VipStatus();
+
         // initial state
+        assertFalse(v.isInRotation());
+
+        // one cluster becomes up
+        v.addToRotation(cluster1);
         assertTrue(v.isInRotation());
+
         // all clusters down
         v.removeFromRotation(cluster1);
         v.removeFromRotation(cluster2);