Skip inactive for stateless nodes

7 files changed, 45 insertions, 33 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index 92e070aa7b0..25aec6a1d04 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -518,8 +518,8 @@ public class NodeRepository extends AbstractComponent {
         var stateless = NodeList.copyOf(nodes).stateless();
         var stateful  = NodeList.copyOf(nodes).stateful();
         List<Node> written = new ArrayList<>();
-        // written.addAll(deallocate(stateless.asList(), Agent.application, "Deactivated by application", transaction.nested()));
-        written.addAll(db.writeTo(State.inactive,  nodes, Agent.application, Optional.empty(), transaction.nested()));
+        written.addAll(deallocate(stateless.asList(), Agent.application, "Deactivated by application", transaction.nested()));
+        written.addAll(db.writeTo(State.inactive, stateful.asList(), Agent.application, Optional.empty(), transaction.nested()));
         return written;
 
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java
index 368f5da0e5b..60ca625d07e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirerTest.java
@@ -60,12 +60,6 @@ public class LoadBalancerExpirerTest {
 
         // Expirer defers removal while nodes are still allocated to application
         expirer.maintain();
-        assertEquals(3, tester.loadBalancerService().instances().size());
-        assertEquals(2, tester.loadBalancerService().instances().get(lb1).reals().size());
-        dirtyNodesOf(app1, cluster1);
-
-        // Expirer prunes reals before expiration time of load balancer itself
-        expirer.maintain();
         assertEquals(Set.of(), tester.loadBalancerService().instances().get(lb1).reals());
         assertEquals(Set.of(), loadBalancers.get().get(lb1).instance().reals());
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java
index 416ba7c6c14..9adba744101 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MetricsReporterTest.java
@@ -225,7 +225,7 @@ public class MetricsReporterTest {
         ApplicationId application = ApplicationId.from("t1", "a1", "default");
         Map<String, String> dimensions = Map.of("applicationId", application.toFullString());
         NodeResources resources = new NodeResources(2, 8, 100, 1);
-        List<Node> activeNodes = tester.deploy(application, Capacity.from(new ClusterResources(4, 1, resources)));
+        List<Node> activeNodes = tester.deploy(application, ProvisioningTester.contentClusterSpec(), Capacity.from(new ClusterResources(4, 1, resources)));
         metricsReporter.maintain();
         assertEquals(0D, getMetric("nodes.nonActiveFraction", metric, dimensions));
         assertEquals(4, getMetric("nodes.active", metric, dimensions));
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 cd188bc017f..ae1e36d73dd 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
@@ -96,10 +96,10 @@ public class PeriodicApplicationMaintainerTest {
         // Cause maintenance deployment which will update the applications with the re-activated nodes
         clock.advance(Duration.ofMinutes(35)); // Otherwise redeploys are inhibited
         fixture.runApplicationMaintainer();
-        assertEquals("Superflous content nodes are retired",
+        assertEquals("Superfluous content nodes are retired",
                      reactivatedInApp2, fixture.getNodes(Node.State.active).retired().size());
-        assertEquals("Superflous container nodes are deactivated (this makes little point for container nodes)",
-                     reactivatedInApp1, fixture.getNodes(Node.State.inactive).size());
+        assertEquals("Superfluous container nodes are deallocated",
+                     reactivatedInApp1, fixture.getNodes(Node.State.dirty).size());
     }
 
     @Test(timeout = 60_000)
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java
index 0246ec524e3..ef0e6d4ddc4 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision.provisioning;
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ApplicationName;
+import com.yahoo.config.provision.ApplicationTransaction;
 import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterResources;
 import com.yahoo.config.provision.ClusterSpec;
@@ -14,11 +15,14 @@ import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.OutOfCapacityException;
+import com.yahoo.config.provision.ProvisionLock;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.Zone;
+import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
+import com.yahoo.vespa.hosted.provision.node.Agent;
 import org.junit.Test;
 
 import java.util.HashSet;
@@ -356,11 +360,15 @@ public class DockerProvisioningTest {
         tester.makeReadyHosts(5, r).activateTenantHosts();
 
         ApplicationId app1 = ProvisioningTester.applicationId("app1");
-        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.container, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
 
         tester.activate(app1, cluster1, Capacity.from(new ClusterResources(5, 1, r)));
         tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, r)));
 
+        var tx = new ApplicationTransaction(new ProvisionLock(app1, tester.nodeRepository().lock(app1)), new NestedTransaction());
+        tester.nodeRepository().deactivate(tester.nodeRepository().list(app1, Node.State.active).retired().asList(), tx);
+        tx.nested().commit();
+
         assertEquals(2, tester.getNodes(app1, Node.State.active).size());
         assertEquals(3, tester.getNodes(app1, Node.State.inactive).size());
 
@@ -372,16 +380,16 @@ public class DockerProvisioningTest {
     }
 
     @Test
-    public void inactive_container_nodes_are_reused() {
-        assertInactiveReuse(ClusterSpec.Type.container);
+    public void inactive_container_nodes_are_not_reused() {
+        assertInactiveReuse(ClusterSpec.Type.container, false);
     }
 
     @Test
     public void inactive_content_nodes_are_reused() {
-        assertInactiveReuse(ClusterSpec.Type.content);
+        assertInactiveReuse(ClusterSpec.Type.content, true);
     }
 
-    private void assertInactiveReuse(ClusterSpec.Type clusterType) {
+    private void assertInactiveReuse(ClusterSpec.Type clusterType, boolean expectedReuse) {
         NodeResources r = new NodeResources(20, 40, 100, 4);
         ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
                                                                     .flavors(List.of(new Flavor(r)))
@@ -398,9 +406,18 @@ public class DockerProvisioningTest {
         tester.nodeRepository().setRemovable(app1, tester.getNodes(app1).retired().asList());
         tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, r)));
 
-        assertEquals(2, tester.getNodes(app1, Node.State.inactive).size());
-        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(4, 1, r)));
-        assertEquals(0, tester.getNodes(app1, Node.State.inactive).size());
+        if (expectedReuse) {
+            assertEquals(2, tester.getNodes(app1, Node.State.inactive).size());
+            tester.activate(app1, cluster1, Capacity.from(new ClusterResources(4, 1, r)));
+            assertEquals(0, tester.getNodes(app1, Node.State.inactive).size());
+        }
+        else {
+            assertEquals(0, tester.getNodes(app1, Node.State.inactive).size());
+            assertEquals(2, tester.nodeRepository().getNodes(Node.State.dirty).size());
+            tester.nodeRepository().setReady(tester.nodeRepository().getNodes(Node.State.dirty), Agent.system, "test");
+            tester.activate(app1, cluster1, Capacity.from(new ClusterResources(4, 1, r)));
+        }
+
     }
 
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java
index acf7bda3cbf..bf2a6ba3627 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeTypeProvisioningTest.java
@@ -153,7 +153,7 @@ public class NodeTypeProvisioningTest {
             assertEquals(10, nodes.size());
 
             // Verify that the node is now inactive
-            assertEquals(Node.State.inactive, tester.nodeRepository().getNode(nodeToRetire.hostname())
+            assertEquals(Node.State.dirty, tester.nodeRepository().getNode(nodeToRetire.hostname())
                     .orElseThrow(RuntimeException::new).state());
         }
     }
@@ -232,7 +232,7 @@ public class NodeTypeProvisioningTest {
             assertEquals(10, nodes.size());
 
             // Verify the node we previously set to retire has finished retiring
-            assertEquals(Node.State.inactive, tester.nodeRepository().getNode(currentyRetiringHostname)
+            assertEquals(Node.State.dirty, tester.nodeRepository().getNode(currentyRetiringHostname)
                     .orElseThrow(RuntimeException::new).state());
 
             // Verify that a node is currently retiring
@@ -263,9 +263,9 @@ public class NodeTypeProvisioningTest {
                 .count();
         assertEquals(11 - numNodesToRetire, numRetiredActiveProxyNodes);
 
-        // All the nodes that were marked with wantToRetire earlier are now inactive
+        // All the nodes that were marked with wantToRetire earlier are now dirty
         assertEquals(nodesToRetire.stream().map(Node::hostname).collect(Collectors.toSet()),
-                tester.nodeRepository().getNodes(Node.State.inactive).stream().map(Node::hostname).collect(Collectors.toSet()));
+                tester.nodeRepository().getNodes(Node.State.dirty).stream().map(Node::hostname).collect(Collectors.toSet()));
     }
 
     private List<HostSpec> deployProxies(ApplicationId application, ProvisioningTester tester) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
index 611c3839f56..7778077110a 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
@@ -32,6 +32,7 @@ import com.yahoo.vespa.service.duper.InfraApplication;
 import org.junit.Test;
 
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
@@ -100,14 +101,15 @@ public class ProvisioningTest {
         SystemState state6 = prepare(application1, 0, 2, 0, 3, defaultResources, tester);
         tester.activate(application1, state6.allHosts);
         assertEquals(5, tester.getNodes(application1, Node.State.active).size());
-        assertEquals(5, tester.getNodes(application1, Node.State.inactive).size());
+        assertEquals(3, tester.getNodes(application1, Node.State.inactive).size());
 
         // delete app
         NodeList previouslyActive = tester.getNodes(application1, Node.State.active);
         NodeList previouslyInactive = tester.getNodes(application1, Node.State.inactive);
         tester.remove(application1);
-        assertEquals(tester.toHostNames(previouslyActive.asList()), tester.toHostNames(tester.nodeRepository().getNodes(application1, Node.State.inactive)));
-        assertEquals(tester.toHostNames(previouslyInactive.asList()), tester.toHostNames(tester.nodeRepository().getNodes(Node.State.dirty)));
+        assertEquals(tester.toHostNames(previouslyActive.not().container().asList()),
+                     tester.toHostNames(tester.nodeRepository().getNodes(application1, Node.State.inactive)));
+        assertTrue(tester.nodeRepository().getNodes(Node.State.dirty).containsAll(previouslyActive.container().asList()));
         assertEquals(0, tester.getNodes(application1, Node.State.active).size());
         assertTrue(tester.nodeRepository().applications().get(application1).isEmpty());
 
@@ -203,7 +205,7 @@ public class ProvisioningTest {
 
         ApplicationId application1 = ProvisioningTester.applicationId();
 
-        tester.makeReadyHosts(24, defaultResources);
+        tester.makeReadyHosts(30, defaultResources);
         tester.activateTenantHosts();
 
         // deploy
@@ -219,8 +221,8 @@ public class ProvisioningTest {
         // decrease again
         SystemState state3 = prepare(application1, 2, 2, 3, 3, defaultResources, tester);
         tester.activate(application1, state3.allHosts);
-        assertEquals("Superfluous container nodes are deactivated",
-                     3-2 + 4-2, tester.getNodes(application1, Node.State.inactive).size());
+        assertEquals("Superfluous container nodes are dirtyed",
+                     3-2 + 4-2, tester.nodeRepository().getNodes(Node.State.dirty).size());
         assertEquals("Superfluous content nodes are retired",
                      4-3 + 5-3, tester.getNodes(application1, Node.State.active).retired().size());
 
@@ -229,7 +231,6 @@ public class ProvisioningTest {
         assertEquals("Inactive nodes are reused", 0, tester.getNodes(application1, Node.State.inactive).size());
         assertEquals("Earlier retired nodes are not unretired before activate",
                      4-3 + 5-3, tester.getNodes(application1, Node.State.active).retired().size());
-        state4.assertExtends(state2);
         assertEquals("New and inactive nodes are reserved", 4 + 3, tester.getNodes(application1, Node.State.reserved).size());
         // Remove a retired host from one of the content clusters (which one is random depending on host names)
         HostSpec removed = state4.removeHost(tester.getNodes(application1, Node.State.active).retired().asList().get(0).hostname());
@@ -243,8 +244,8 @@ public class ProvisioningTest {
         // decrease again
         SystemState state5 = prepare(application1, 2, 2, 3, 3, defaultResources, tester);
         tester.activate(application1, state5.allHosts);
-        assertEquals("Superfluous container nodes are also deactivated",
-                     4-2 + 5-2 + 1, tester.getNodes(application1, Node.State.inactive).size()); //
+        assertEquals("Superfluous container nodes are also dirtyed",
+                     4-2 + 5-2 + 1 + 4-2, tester.nodeRepository().getNodes(Node.State.dirty).size());
         assertEquals("Superfluous content nodes are retired",
                      5-3 + 6-3 - 1, tester.getNodes(application1, Node.State.active).retired().size());