Merge pull request #15388 from vespa-engine/bratseth/delete-nodes-on-application-delete

Delete non-active nodes on application delete

5 files changed, 28 insertions, 10 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 663d1d19995..05bdfd25b76 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
@@ -503,12 +503,6 @@ public class NodeRepository extends AbstractComponent {
         }
     }
 
-    /** Deactivate nodes owned by application guarded by given lock */
-    public void deactivate(ApplicationTransaction transaction) {
-        deactivate(db.readNodes(transaction.application(), State.reserved, State.active), transaction);
-        applications.remove(transaction);
-    }
-
     /**
      * Deactivates these nodes in a transaction and returns the nodes in the new state which will hold if the
      * transaction commits.
@@ -517,6 +511,19 @@ public class NodeRepository extends AbstractComponent {
         return db.writeTo(State.inactive, nodes, Agent.application, Optional.empty(), transaction.nested());
     }
 
+    /** Removes this application: Active nodes are deactivated while all non-active nodes are set dirty. */
+    public void remove(ApplicationTransaction transaction) {
+        NodeList applicationNodes = list(transaction.application());
+        NodeList activeNodes = applicationNodes.state(State.active);
+        deactivate(activeNodes.asList(), transaction);
+        db.writeTo(State.dirty,
+                   applicationNodes.except(activeNodes.asSet()).asList(),
+                   Agent.system,
+                   Optional.of("Application is removed"),
+                   transaction.nested());
+        applications.remove(transaction);
+    }
+
     /** Move nodes to the dirty state */
     public List<Node> setDirty(List<Node> nodes, Agent agent, String reason) {
         return performOn(NodeListFilter.from(nodes), (node, lock) -> setDirty(node, agent, reason));
@@ -532,6 +539,7 @@ public class NodeRepository extends AbstractComponent {
         return db.writeTo(State.dirty, node, agent, Optional.of(reason));
     }
 
+
     public List<Node> dirtyRecursively(String hostname, Agent agent, String reason) {
         Node nodeToDirty = getNode(hostname).orElseThrow(() ->
                 new IllegalArgumentException("Could not deallocate " + hostname + ": Node not found"));
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
index edf151ff2d8..ede6f4ef250 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
@@ -29,6 +29,7 @@ import com.yahoo.vespa.hosted.provision.autoscale.AllocatableClusterResources;
 import com.yahoo.vespa.hosted.provision.autoscale.AllocationOptimizer;
 import com.yahoo.vespa.hosted.provision.autoscale.Limits;
 import com.yahoo.vespa.hosted.provision.autoscale.ResourceTarget;
+import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.Allocation;
 import com.yahoo.vespa.hosted.provision.node.filter.ApplicationFilter;
 import com.yahoo.vespa.hosted.provision.node.filter.NodeHostFilter;
@@ -132,7 +133,7 @@ public class NodeRepositoryProvisioner implements Provisioner {
 
     @Override
     public void remove(ApplicationTransaction transaction) {
-        nodeRepository.deactivate(transaction);
+        nodeRepository.remove(transaction);
         loadBalancerProvisioner.ifPresent(lbProvisioner -> lbProvisioner.deactivate(transaction));
     }
 
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 2fe39780cf5..cbac5a39e09 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
@@ -103,9 +103,13 @@ public class ProvisioningTest {
         assertEquals(5, 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(state1.allHosts), tester.toHostNames(tester.nodeRepository().getNodes(application1, Node.State.inactive)));
+        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(0, tester.getNodes(application1, Node.State.active).size());
+        assertTrue(tester.nodeRepository().applications().get(application1).isEmpty());
 
         // other application is unaffected
         assertEquals(state1App2.hostNames(), tester.toHostNames(tester.nodeRepository().getNodes(application2, Node.State.active)));
@@ -121,6 +125,7 @@ public class ProvisioningTest {
         tester.activate(application2, state2App2.allHosts);
 
         // deploy first app again
+        tester.nodeRepository().setReady(tester.nodeRepository().getNodes(Node.State.dirty), Agent.system, "recycled");
         SystemState state7 = prepare(application1, 2, 2, 3, 3, defaultResources, tester);
         state7.assertEquals(state1);
         tester.activate(application1, state7.allHosts);
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 d39ea3786f1..f012f0a428f 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
@@ -242,8 +242,8 @@ public class ProvisioningTester {
     public void deactivate(ApplicationId applicationId) {
         try (var lock = nodeRepository.lock(applicationId)) {
             NestedTransaction deactivateTransaction = new NestedTransaction();
-            nodeRepository.deactivate(new ApplicationTransaction(new ProvisionLock(applicationId, lock),
-                                                                 deactivateTransaction));
+            nodeRepository.remove(new ApplicationTransaction(new ProvisionLock(applicationId, lock),
+                                                             deactivateTransaction));
             deactivateTransaction.commit();
         }
     }
diff --git a/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java b/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java
index de1040852f5..4a24cdcc7bf 100644
--- a/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java
+++ b/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java
@@ -10,6 +10,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
 import java.util.Random;
+import java.util.Set;
 import java.util.function.BiFunction;
 import java.util.function.Function;
 import java.util.function.Predicate;
@@ -72,6 +73,9 @@ public abstract class AbstractFilteringList<Type, ListType extends AbstractFilte
     /** Returns the items in this as an immutable list. */
     public final List<Type> asList() { return items; }
 
+    /** Returns the items in this as a set. */
+    public final Set<Type> asSet() { return new HashSet<>(items); }
+
     /** Returns the items in this as an immutable list after mapping with the given function. */
     public final <OtherType> List<OtherType> mapToList(Function<Type, OtherType> mapper) {
         return items.stream().map(mapper).collect(toUnmodifiableList());