Redeploy quickly on manual node changes

6 files changed, 183 insertions, 12 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java
index 2f1459dc54e..967ef023d4c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ApplicationMaintainer.java
@@ -45,8 +45,7 @@ public abstract class ApplicationMaintainer extends Maintainer {
                     Optional<Deployment> deployment = deployer.deployFromLocalActive(application, Duration.ofMinutes(30));
                     if ( ! deployment.isPresent()) continue; // this will be done at another config server
 
-                    // deploy asynchronously to make sure we do all applications even when deployments are slow
-                    deployAsynchronously(deployment.get());
+                    deploy(application, deployment.get());
                 }
                 throttle(applications.size());
             }
@@ -56,7 +55,12 @@ public abstract class ApplicationMaintainer extends Maintainer {
         }
     }
 
-    protected void deployAsynchronously(Deployment deployment) {
+    /**
+     * Redeploy this application using the provided deployer.
+     * The default implementatyion deploys asynchronously to make sure we do all applications timely 
+     * even when deployments are slow.
+     */
+    protected void deploy(ApplicationId applicationId, Deployment deployment) {
         deploymentExecutor.execute(() -> {
             try {
                 deployment.activate();
@@ -67,10 +71,8 @@ public abstract class ApplicationMaintainer extends Maintainer {
         });
     }
 
-    protected void throttle(int applicationCount) {
-        // Sleep for a length of time that will spread deployment evenly over the maintenance period
-        try { Thread.sleep(interval().toMillis() / applicationCount); } catch (InterruptedException e) { return; }
-    }
+    /** Block in this method until the next application should be maintained */
+    protected abstract void throttle(int applicationCount);
 
     private Set<ApplicationId> applicationsNeedingMaintenance() {
         return nodesNeedingMaintenance().stream()
@@ -80,7 +82,7 @@ public abstract class ApplicationMaintainer extends Maintainer {
 
     /** 
      * Returns the nodes whose applications should be maintained by this now. 
-     * This should be some subset of active nodes. 
+     * This should be some subset of the allocated nodes. 
      */
     protected abstract List<Node> nodesNeedingMaintenance();
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainer.java
index f14403d7730..45dadb0e09a 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainer.java
@@ -51,7 +51,8 @@ public class OperatorChangeApplicationMaintainer extends ApplicationMaintainer {
         Instant windowEnd = clock.instant();
         Instant windowStart = previousRun;
         previousRun = windowEnd;
-        return nodeRepository().getNodes(Node.State.active).stream()
+        return nodeRepository().getNodes().stream()
+                               .filter(node -> node.allocation().isPresent())
                                .filter(node -> hasManualStateChangeSince(windowStart, node))
                                .collect(Collectors.toList());
     }
@@ -61,13 +62,17 @@ public class OperatorChangeApplicationMaintainer extends ApplicationMaintainer {
                 .anyMatch(event -> event.agent() == Agent.operator && event.at().isAfter(instant));
     }
 
+    protected void throttle(int applicationCount) { }
+
     /** 
      * Deploy in the maintenance thread to avoid scheduling multiple deployments of the same application if it takes
      * longer to deploy than the (short) maintenance interval of this
      */
     @Override
-    protected void deployAsynchronously(Deployment deployment) {
+    protected void deploy(ApplicationId applicationId, Deployment deployment) {
         deployment.activate();
+        log.info("Redeployed application " + applicationId.toShortString() + 
+                 " as a manual change was made to its nodes");
     }
 
     @Override
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java
index 5f6ed200875..c38d0c42071 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainer.java
@@ -32,6 +32,11 @@ public class PeriodicApplicationMaintainer extends ApplicationMaintainer {
         super(deployer, nodeRepository, interval);
     }
 
+    protected void throttle(int applicationCount) {
+        // Sleep for a length of time that will spread deployment evenly over the maintenance period
+        try { Thread.sleep(interval().toMillis() / applicationCount); } catch (InterruptedException e) { return; }
+    }
+
     @Override
     protected List<Node> nodesNeedingMaintenance() {
         return nodeRepository().getNodes(Node.State.active);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
index cb271f90e6e..49c077f3a05 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
@@ -148,7 +148,7 @@ public class History {
         public Instant at() { return at; }
 
         @Override
-        public String toString() { return "'" + type + "' event at " + at; }
+        public String toString() { return "'" + type + "' event at " + at + " by " + agent; }
 
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java
new file mode 100644
index 00000000000..8899475120d
--- /dev/null
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OperatorChangeApplicationMaintainerTest.java
@@ -0,0 +1,159 @@
+// 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.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.Deployer;
+import com.yahoo.config.provision.Deployment;
+import com.yahoo.config.provision.Environment;
+import com.yahoo.config.provision.HostSpec;
+import com.yahoo.config.provision.InstanceName;
+import com.yahoo.config.provision.NodeFlavors;
+import com.yahoo.config.provision.NodeType;
+import com.yahoo.config.provision.RegionName;
+import com.yahoo.config.provision.TenantName;
+import com.yahoo.config.provision.Zone;
+import com.yahoo.test.ManualClock;
+import com.yahoo.transaction.NestedTransaction;
+import com.yahoo.vespa.curator.Curator;
+import com.yahoo.vespa.curator.mock.MockCurator;
+import com.yahoo.vespa.curator.transaction.CuratorTransaction;
+import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.NodeList;
+import com.yahoo.vespa.hosted.provision.NodeRepository;
+import com.yahoo.vespa.hosted.provision.node.Agent;
+import com.yahoo.vespa.hosted.provision.provisioning.NodeRepositoryProvisioner;
+import com.yahoo.vespa.hosted.provision.testutils.FlavorConfigBuilder;
+import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.time.Duration;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * @author bratseth
+ */
+public class OperatorChangeApplicationMaintainerTest {
+
+    private static final NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("default");
+
+    private NodeRepository nodeRepository;
+    private Fixture fixture;
+
+    @Test
+    public void test_application_maintenance() throws InterruptedException {
+        ManualClock clock = new ManualClock();
+        Curator curator = new MockCurator();
+        Zone zone = new Zone(Environment.prod, RegionName.from("us-east"));
+        this.nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone,
+                                                 new MockNameResolver().mockAnyLookup());
+        this.fixture = new Fixture(zone, nodeRepository, nodeFlavors, curator);
+
+        createReadyNodes(15, nodeRepository, nodeFlavors);
+        createHostNodes(2, nodeRepository, nodeFlavors);
+
+        // Create applications
+        fixture.activate();
+        OperatorChangeApplicationMaintainer maintainer = new OperatorChangeApplicationMaintainer(fixture.deployer, nodeRepository, clock, Duration.ofMinutes(1));
+        
+        clock.advance(Duration.ofMinutes(2));
+        maintainer.maintain();
+        assertEquals("No changes -> no redeployments", 0, fixture.deployer.redeployments);
+
+        nodeRepository.fail(nodeRepository.getNodes(fixture.app1).get(3).hostname(), Agent.system, "Failing to unit test");
+        clock.advance(Duration.ofMinutes(2));
+        maintainer.maintain();
+        assertEquals("System change -> no redeployments", 0, fixture.deployer.redeployments);
+
+        clock.advance(Duration.ofSeconds(1));
+        nodeRepository.fail(nodeRepository.getNodes(fixture.app2).get(4).hostname(), Agent.operator, "Manual node failing");
+        clock.advance(Duration.ofMinutes(2));
+        maintainer.maintain(); // XXX
+        assertEquals("Operator change -> redeployment", 1, fixture.deployer.redeployments);
+
+        clock.advance(Duration.ofMinutes(2));
+        maintainer.maintain();
+        assertEquals("No further operator changes -> no (new) redeployments", 1, fixture.deployer.redeployments);
+    }
+
+    private void createReadyNodes(int count, NodeRepository nodeRepository, NodeFlavors nodeFlavors) {
+        List<Node> nodes = new ArrayList<>(count);
+        for (int i = 0; i < count; i++)
+            nodes.add(nodeRepository.createNode("node" + i, "host" + i, Optional.empty(), nodeFlavors.getFlavorOrThrow("default"), NodeType.tenant));
+        nodes = nodeRepository.addNodes(nodes);
+        nodes = nodeRepository.setDirty(nodes);
+        nodeRepository.setReady(nodes);
+    }
+
+    private void createHostNodes(int count, NodeRepository nodeRepository, NodeFlavors nodeFlavors) {
+        List<Node> nodes = new ArrayList<>(count);
+        for (int i = 0; i < count; i++)
+            nodes.add(nodeRepository.createNode("hostNode" + i, "realHost" + i, Optional.empty(), nodeFlavors.getFlavorOrThrow("default"), NodeType.host));
+        nodes = nodeRepository.addNodes(nodes);
+        nodes = nodeRepository.setDirty(nodes);
+        nodeRepository.setReady(nodes);
+    }
+
+    private class Fixture {
+
+        final NodeRepository nodeRepository;
+        final NodeRepositoryProvisioner provisioner;
+        final Curator curator;
+
+        final ApplicationId app1 = ApplicationId.from(TenantName.from("foo1"), ApplicationName.from("bar"), InstanceName.from("fuz"));
+        final ApplicationId app2 = ApplicationId.from(TenantName.from("foo2"), ApplicationName.from("bar"), InstanceName.from("fuz"));
+        final ClusterSpec clusterApp1 = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("test"), Optional.empty());
+        final ClusterSpec clusterApp2 = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("test"), Optional.empty());
+        final int wantedNodesApp1 = 5;
+        final int wantedNodesApp2 = 7;
+        MockDeployer deployer; // created on activation
+
+        Fixture(Zone zone, NodeRepository nodeRepository, NodeFlavors flavors, Curator curator) {
+            this.nodeRepository = nodeRepository;
+            this.curator = curator;
+            this.provisioner =  new NodeRepositoryProvisioner(nodeRepository, flavors, zone);
+        }
+
+        void activate() {
+            activate(app1, clusterApp1, wantedNodesApp1, provisioner);
+            activate(app2, clusterApp2, wantedNodesApp2, provisioner);
+            assertEquals(wantedNodesApp1, nodeRepository.getNodes(app1, Node.State.active).size());
+            assertEquals(wantedNodesApp2, nodeRepository.getNodes(app2, Node.State.active).size());
+
+            Map<ApplicationId, MockDeployer.ApplicationContext> apps = new HashMap<>();
+            apps.put(app1, new MockDeployer.ApplicationContext(app1, clusterApp1,
+                                                               Capacity.fromNodeCount(wantedNodesApp1, Optional.of("default")), 1));
+            apps.put(app2, new MockDeployer.ApplicationContext(app2, clusterApp2,
+                                                               Capacity.fromNodeCount(wantedNodesApp2, Optional.of("default")), 1));
+            this.deployer = new MockDeployer(provisioner, apps);
+        }
+
+        private void activate(ApplicationId applicationId, ClusterSpec cluster, int nodeCount, NodeRepositoryProvisioner provisioner) {
+            List<HostSpec> hosts = provisioner.prepare(applicationId, cluster, Capacity.fromNodeCount(nodeCount), 1, null);
+            NestedTransaction transaction = new NestedTransaction().add(new CuratorTransaction(curator));
+            provisioner.activate(transaction, applicationId, hosts);
+            transaction.commit();
+        }
+
+        void remove(ApplicationId application) {
+            NestedTransaction transaction = new NestedTransaction().add(new CuratorTransaction(curator));
+            provisioner.remove(transaction, application);
+            transaction.commit();
+        }
+
+        NodeList getNodes(Node.State ... states) {
+            return new NodeList(nodeRepository.getNodes(NodeType.tenant, states));
+        }
+
+    }
+    
+}
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
index 485e12c03c9..c8ca4777ff9 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/PeriodicApplicationMaintainerTest.java
@@ -213,7 +213,7 @@ public class PeriodicApplicationMaintainerTest {
         }
 
         @Override
-        protected void deployAsynchronously(Deployment deployment) {
+        protected void deploy(ApplicationId application, Deployment deployment) {
             deployment.activate();
         }