Shut down ClusterController from last configurer, thus before ZK

4 files changed, 52 insertions, 20 deletions

diff --git a/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java b/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java
index 1eca2d18493..5f606c68c1e 100644
--- a/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java
+++ b/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java
@@ -17,6 +17,8 @@ import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Logger;
 
 /**
@@ -30,6 +32,8 @@ public class ClusterController extends AbstractComponent
     private final JDiscMetricWrapper metricWrapper;
     private final Map<String, FleetController> controllers = new TreeMap<>();
     private final Map<String, StatusHandler.ContainerStatusPageServer> status = new TreeMap<>();
+    private final AtomicInteger referents = new AtomicInteger();
+    private final AtomicBoolean shutdown = new AtomicBoolean();
 
     /**
      * Dependency injection constructor for controller. A {@link VespaZooKeeperServer} argument is required
@@ -43,6 +47,7 @@ public class ClusterController extends AbstractComponent
     }
 
     public void setOptions(FleetControllerOptions options, Metric metricImpl) throws Exception {
+        referents.incrementAndGet();
         metricWrapper.updateMetricImplementation(metricImpl);
         verifyThatZooKeeperWorks(options);
         synchronized (controllers) {
@@ -60,16 +65,32 @@ public class ClusterController extends AbstractComponent
 
     @Override
     public void deconstruct() {
-        synchronized (controllers) {
-            for (FleetController controller : controllers.values()) {
-                try{
-                    shutdownController(controller);
-                } catch (Exception e) {
-                    log.warning("Failed to shut down fleet controller: " + e.getMessage());
+        shutdown();
+    }
+
+    /**
+     * Since we hack around injecting a running ZK here by providing one through the configurer instead,
+     * we must also let the last configurer shut down this controller, to ensure this is shut down
+     * before the ZK server it had injected from the configurers.
+     */
+    void countdown() {
+        if (referents.decrementAndGet() == 0)
+            shutdown();
+    }
+
+    void shutdown() {
+        if (shutdown.compareAndSet(false, true)) {
+            synchronized (controllers) {
+                for (FleetController controller : controllers.values()) {
+                    try {
+                        shutdownController(controller);
+                    }
+                    catch (Exception e) {
+                        log.warning("Failed to shut down fleet controller: " + e.getMessage());
+                    }
                 }
             }
         }
-        super.deconstruct();
     }
 
     @Override
@@ -79,6 +100,8 @@ public class ClusterController extends AbstractComponent
         }
     }
 
+    FleetController getController(String name) { return controllers.get(name); }
+
     @Override
     public StatusHandler.ContainerStatusPageServer get(String cluster) {
         return status.get(cluster);
diff --git a/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurer.java b/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurer.java
index 75e838bae3e..a0e290de172 100644
--- a/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurer.java
+++ b/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurer.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.clustercontroller.apps.clustercontroller;
 
 import com.google.inject.Inject;
+import com.yahoo.component.AbstractComponent;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vdslib.distribution.Distribution;
 import com.yahoo.vdslib.state.NodeType;
@@ -19,9 +20,10 @@ import java.util.Map;
  * When the cluster controller is reconfigured, a new instance of this is created, which will propagate configured
  * options to receivers such as the fleet controller.
  */
-public class ClusterControllerClusterConfigurer {
+public class ClusterControllerClusterConfigurer extends AbstractComponent {
 
     private final FleetControllerOptions options;
+    private final ClusterController controller;
 
     /**
      * The {@link VespaZooKeeperServer} argument is required by the injected {@link ClusterController},
@@ -37,9 +39,13 @@ public class ClusterControllerClusterConfigurer {
                                               Metric metricImpl,
                                               VespaZooKeeperServer started) throws Exception {
         this.options = configure(distributionConfig, fleetcontrollerConfig, slobroksConfig, zookeepersConfig);
-        if (controller != null) {
-            controller.setOptions(options, metricImpl);
-        }
+        this.controller = controller;
+        if (controller != null) controller.setOptions(options, metricImpl);
+    }
+
+    @Override
+    public void deconstruct() {
+        if (controller != null) controller.countdown();
     }
 
     FleetControllerOptions getOptions() { return options; }
diff --git a/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurerTest.java b/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurerTest.java
index f6e64dc7186..dda18fc3396 100644
--- a/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurerTest.java
+++ b/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerClusterConfigurerTest.java
@@ -11,6 +11,7 @@ import org.junit.Test;
 import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -50,7 +51,7 @@ public class ClusterControllerClusterConfigurerTest {
             @Override
             public Context createContext(Map<String, ?> stringMap) { return null; }
         };
-            // Used in standalone modus to get config without a cluster controller instance
+            // Used in standalone mode to get config without a cluster controller instance
         ClusterControllerClusterConfigurer configurer = new ClusterControllerClusterConfigurer(
                 null,
                 new StorDistributionConfig(distributionConfig),
@@ -60,7 +61,7 @@ public class ClusterControllerClusterConfigurerTest {
                 metric,
                 null
         );
-        assertTrue(configurer.getOptions() != null);
+        assertNotNull(configurer.getOptions());
         assertEquals(0.123, configurer.getOptions().minNodeRatioPerGroup, 0.01);
         assertTrue(configurer.getOptions().clusterFeedBlockEnabled);
         assertEquals(0.5, configurer.getOptions().clusterFeedBlockLimit.get("foo"), 0.01);
diff --git a/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerTest.java b/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerTest.java
index 5c9a7eb9c0c..2c33f781737 100644
--- a/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerTest.java
+++ b/clustercontroller-apps/src/test/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterControllerTest.java
@@ -11,8 +11,11 @@ import org.junit.Test;
 import java.util.Map;
 import java.util.Set;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Doesn't really test cluster controller, but runs some lines of code.
@@ -41,17 +44,16 @@ public class ClusterControllerTest {
 
     @Test
     public void testSimple() throws Exception {
-        // Cluster controller object keeps state and should never be remade, so should
-        // inject nothing
         ClusterController cc = new ClusterController();
         cc.setOptions(options, metric);
         cc.setOptions(options, metric);
-        cc.getFleetControllers();
-        cc.getAll();
-
+        assertEquals(1, cc.getFleetControllers().size());
         assertNotNull(cc.get("storage"));
         assertNull(cc.get("music"));
-        cc.deconstruct();
+        cc.countdown();
+        assertTrue(cc.getController("storage").isRunning());
+        cc.countdown();
+        assertFalse(cc.getController("storage").isRunning());
     }
 
     @Test
@@ -62,7 +64,7 @@ public class ClusterControllerTest {
             }
         };
         cc.setOptions(options, metric);
-        cc.deconstruct();
+        cc.countdown();
     }
 
 }