Merge pull request #27040 from vespa-engine/hakonhall/fix-provisioningtester

Config servers are never inactive

4 files changed, 12 insertions, 53 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java
index 81ba2870a92..aa7aac34389 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveExpirer.java
@@ -1,7 +1,6 @@
 // Copyright Yahoo. 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.NodeType;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
@@ -11,7 +10,6 @@ import com.yahoo.vespa.hosted.provision.node.Status;
 
 import java.time.Duration;
 import java.util.List;
-import java.util.Map;
 
 /**
  * Maintenance job which moves inactive nodes to dirty or parked after timeout.
@@ -32,15 +30,12 @@ import java.util.Map;
 public class InactiveExpirer extends Expirer {
 
     private final NodeRepository nodeRepository;
-    private final Duration defaultTimeout;
-    private final Map<NodeType, Duration> inactiveTimeouts;
+    private final Duration timeout;
 
-    InactiveExpirer(NodeRepository nodeRepository, Duration defaultTimeout, Map<NodeType, Duration> inactiveTimeouts,
-                    Metric metric) {
-        super(Node.State.inactive, History.Event.Type.deactivated, nodeRepository, defaultTimeout, metric);
+    InactiveExpirer(NodeRepository nodeRepository, Duration timeout, Metric metric) {
+        super(Node.State.inactive, History.Event.Type.deactivated, nodeRepository, timeout, metric);
         this.nodeRepository = nodeRepository;
-        this.defaultTimeout = defaultTimeout;
-        this.inactiveTimeouts = Map.copyOf(inactiveTimeouts);
+        this.timeout = timeout;
     }
 
     @Override
@@ -57,7 +52,7 @@ public class InactiveExpirer extends Expirer {
     }
 
     private Duration timeout(Node node) {
-        return inactiveTimeouts.getOrDefault(node.type(), defaultTimeout);
+        return timeout;
     }
 
 }
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 9436fcc150e..722c9acfdc0 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
@@ -6,7 +6,6 @@ import com.yahoo.component.annotation.Inject;
 import com.yahoo.concurrent.maintenance.Maintainer;
 import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.InfraDeployer;
-import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.flags.FlagSource;
@@ -17,7 +16,6 @@ import com.yahoo.vespa.service.monitor.ServiceMonitor;
 
 import java.time.Duration;
 import java.util.List;
-import java.util.Map;
 import java.util.concurrent.CopyOnWriteArrayList;
 
 /**
@@ -49,9 +47,7 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         maintainers.add(new ExpeditedChangeApplicationMaintainer(deployer, metric, nodeRepository, defaults.expeditedChangeRedeployInterval));
         maintainers.add(new ReservationExpirer(nodeRepository, defaults.reservationExpiry, metric));
         maintainers.add(new RetiredExpirer(nodeRepository, deployer, metric, defaults.retiredInterval, defaults.retiredExpiry));
-        maintainers.add(new InactiveExpirer(nodeRepository, defaults.inactiveExpiry, Map.of(NodeType.config, defaults.inactiveConfigServerExpiry,
-                                                                                            NodeType.controller, defaults.inactiveControllerExpiry),
-                                            metric));
+        maintainers.add(new InactiveExpirer(nodeRepository, defaults.inactiveExpiry, metric));
         maintainers.add(new FailedExpirer(nodeRepository, zone, defaults.failedExpirerInterval, metric));
         maintainers.add(new DirtyExpirer(nodeRepository, defaults.dirtyExpiry, metric));
         maintainers.add(new ProvisionedExpirer(nodeRepository, defaults.provisionedExpiry, metric));
@@ -100,8 +96,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
         
         private final Duration reservationExpiry;
         private final Duration inactiveExpiry;
-        private final Duration inactiveConfigServerExpiry;
-        private final Duration inactiveControllerExpiry;
         private final Duration retiredExpiry;
         private final Duration failedExpirerInterval;
         private final Duration dirtyExpiry;
@@ -154,8 +148,6 @@ public class NodeRepositoryMaintenance extends AbstractComponent {
             spareCapacityMaintenanceInterval = Duration.ofMinutes(30);
             switchRebalancerInterval = Duration.ofHours(1);
             throttlePolicy = NodeFailer.ThrottlePolicy.hosted;
-            inactiveConfigServerExpiry = Duration.ofMinutes(5);
-            inactiveControllerExpiry = Duration.ofMinutes(5);
             hostRetirerInterval = Duration.ofMinutes(30);
 
             if (zone.environment().isProduction() && ! zone.system().isCd()) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
index d7e2dbb6f58..563b76327e6 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
@@ -1,7 +1,6 @@
 // Copyright Yahoo. 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.component.Vtag;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.Capacity;
@@ -19,7 +18,6 @@ import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.History;
-import com.yahoo.vespa.hosted.provision.node.filter.NodeListFilter;
 import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
 import com.yahoo.vespa.hosted.provision.testutils.MockDeployer;
 import com.yahoo.vespa.orchestrator.OrchestrationException;
@@ -30,12 +28,8 @@ import java.time.Duration;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
-import java.util.function.Supplier;
-import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doThrow;
@@ -69,7 +63,7 @@ public class InactiveAndFailedExpirerTest {
 
         // Inactive times out
         tester.advanceTime(Duration.ofMinutes(14));
-        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), Map.of(), new TestMetric()).run();
+        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), new TestMetric()).run();
         assertEquals(0, tester.nodeRepository().nodes().list(Node.State.inactive).size());
         NodeList dirty = tester.nodeRepository().nodes().list(Node.State.dirty);
         assertEquals(2, dirty.size());
@@ -132,7 +126,7 @@ public class InactiveAndFailedExpirerTest {
 
         // Inactive times out and one node is moved to parked
         tester.advanceTime(Duration.ofMinutes(11)); // Trigger InactiveExpirer
-        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), Map.of(), new TestMetric()).run();
+        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), new TestMetric()).run();
         assertEquals(1, tester.nodeRepository().nodes().list(Node.State.parked).size());
     }
 
@@ -154,7 +148,7 @@ public class InactiveAndFailedExpirerTest {
         assertEquals(1, inactiveNodes.size());
 
         // See that nodes are moved to dirty immediately.
-        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), Map.of(), new TestMetric()).run();
+        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), new TestMetric()).run();
         assertEquals(0, tester.nodeRepository().nodes().list(Node.State.inactive).size());
         NodeList dirty = tester.nodeRepository().nodes().list(Node.State.dirty);
         assertEquals(1, dirty.size());
@@ -178,7 +172,7 @@ public class InactiveAndFailedExpirerTest {
         // Nodes marked for deprovisioning are moved to dirty and then parked when readied by host-admin
         tester.patchNodes(inactiveNodes, (node) -> node.withWantToRetire(true, true, Agent.system, tester.clock().instant()));
         tester.advanceTime(Duration.ofMinutes(11));
-        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), Map.of(), new TestMetric()).run();
+        new InactiveExpirer(tester.nodeRepository(), Duration.ofMinutes(10), new TestMetric()).run();
 
         NodeList expired = tester.nodeRepository().nodes().list(Node.State.dirty);
         assertEquals(2, expired.size());
@@ -186,27 +180,4 @@ public class InactiveAndFailedExpirerTest {
         assertEquals(2, tester.nodeRepository().nodes().list(Node.State.parked).size());
     }
 
-    @Test
-    public void inactive_config_server_expires_according_to_custom_timeout() {
-        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))).build();
-        InactiveExpirer expirer = new InactiveExpirer(tester.nodeRepository(), Duration.ofHours(1),
-                                                      Map.of(NodeType.config, Duration.ofMinutes(5)),
-                                                      new TestMetric());
-        NodeList nodes = tester.makeConfigServers(3, "default", Vtag.currentVersion);
-        Supplier<Node> firstNode = () -> tester.nodeRepository().nodes().node(nodes.first().get().hostname()).get();
-        ApplicationId application = firstNode.get().allocation().get().owner();
-
-        // Retired config server is moved to inactive
-        tester.nodeRepository().nodes().retire(NodeListFilter.from(firstNode.get()), Agent.system, tester.clock().instant());
-        tester.prepareAndActivateInfraApplication(application, NodeType.config);
-        assertSame(Node.State.inactive, firstNode.get().state());
-        expirer.maintain();
-        assertSame(Node.State.inactive, firstNode.get().state());
-
-        // Config server expires
-        tester.clock().advance(Duration.ofMinutes(5).plus(Duration.ofSeconds(1)));
-        expirer.maintain();
-        assertSame(Node.State.dirty, firstNode.get().state());
-    }
-
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
index 991c6c69c35..76dcc6cf8a8 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
@@ -239,7 +239,8 @@ public class ProvisioningTester {
     }
 
     public List<HostSpec> prepareAndActivateInfraApplication(ApplicationId application, NodeType nodeType, Version version) {
-        ClusterSpec cluster = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from(nodeType.toString()))
+        ClusterSpec cluster = ClusterSpec.request(nodeType.isConfigServerLike() ? ClusterSpec.Type.admin : ClusterSpec.Type.container,
+                                                  ClusterSpec.Id.from(nodeType == NodeType.config ? "zone-config-servers" : nodeType.toString()))
                                          .vespaVersion(version)
                                          .stateful(nodeType == NodeType.config || nodeType == NodeType.controller)
                                          .build();