Merge pull request #14348 from vespa-engine/revert-14336-bratseth/allocation-improvements

Revert "Bratseth/allocation improvements" MERGEOK

10 files changed, 32 insertions, 112 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
index b08dc6bbaf2..e146583ae04 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
@@ -204,11 +204,6 @@ public final class Node {
         return with(requireAllocation("Cannot unretire").unretire());
     }
 
-    /** Returns a copy of this with removable set to the given value */
-    public Node removable(boolean removable) {
-        return with(requireAllocation("Cannot set removable").removable(removable));
-    }
-
     /** Returns a copy of this with the restart generation set to generation */
     public Node withRestart(Generation generation) {
         Allocation allocation = requireAllocation("Cannot set restart generation");
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 4ec7ddd04c4..983ba5165e3 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
@@ -511,7 +511,7 @@ public class NodeRepository extends AbstractComponent {
     public void setRemovable(ApplicationId application, List<Node> nodes) {
         try (Mutex lock = lock(application)) {
             List<Node> removableNodes =
-                nodes.stream().map(node -> node.with(node.allocation().get().removable(true)))
+                nodes.stream().map(node -> node.with(node.allocation().get().removable()))
                               .collect(Collectors.toList());
             write(removableNodes, lock);
         }
@@ -641,7 +641,7 @@ public class NodeRepository extends AbstractComponent {
     }
 
     private Node move(Node node, State toState, Agent agent, Optional<String> reason) {
-        if (toState == Node.State.active && node.allocation().isEmpty())
+        if (toState == Node.State.active && ! node.allocation().isPresent())
             illegal("Could not set " + node + " active. It has no allocation.");
 
         try (Mutex lock = lock(node)) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Allocation.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Allocation.java
index b476a2bdefc..30ef84c6927 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Allocation.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Allocation.java
@@ -85,9 +85,9 @@ public class Allocation {
         return new Allocation(owner, clusterMembership, requestedResources, generation, removable, networkPorts);
     }
 
-    /** Returns a copy of this allocation where removable is set to the given value */
-    public Allocation removable(boolean removable) {
-        return new Allocation(owner, clusterMembership, requestedResources, restartGeneration, removable, networkPorts);
+    /** Returns a copy of this allocation where removable is set to true */
+    public Allocation removable() {
+        return new Allocation(owner, clusterMembership, requestedResources, restartGeneration, true, networkPorts);
     }
 
     public Allocation with(ClusterMembership newMembership) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
index 5a3584b6ff4..37842115949 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
@@ -241,7 +241,7 @@ public class NodeSerializer {
     }
 
     private Optional<Allocation> allocationFromSlime(NodeResources assignedResources, Inspector object) {
-        if ( ! object.valid()) return Optional.empty();
+        if ( ! object.valid()) return Optional.empty(); // TODO: Remove this line (and to the simplifications that follows) after November 2019
         return Optional.of(new Allocation(applicationIdFromSlime(object),
                                           clusterMembershipFromSlime(object),
                                           NodeResourcesSerializer.optionalResourcesFromSlime(object.field(requestedResourcesKey))
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
index a37da10f5f0..1aa0f69dd9b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
@@ -104,23 +104,27 @@ class NodeAllocation {
             Node offered = node.node;
 
             if (offered.allocation().isPresent()) {
-                Allocation allocation = offered.allocation().get();
-                ClusterMembership membership = allocation.membership();
-                if ( ! allocation.owner().equals(application)) continue; // wrong application
+                ClusterMembership membership = offered.allocation().get().membership();
+                if ( ! offered.allocation().get().owner().equals(application)) continue; // wrong application
                 if ( ! membership.cluster().satisfies(cluster)) continue; // wrong cluster id/type
                 if ((! node.isSurplusNode || saturated()) && ! membership.cluster().group().equals(cluster.group())) continue; // wrong group and we can't or have no reason to change it
-                if ( offered.state() == Node.State.active && allocation.isRemovable()) continue; // don't accept; causes removal
+                if ( offered.allocation().get().isRemovable()) continue; // don't accept; causes removal
                 if ( indexes.contains(membership.index())) continue; // duplicate index (just to be sure)
 
-                boolean resizeable = false;
-                boolean acceptToRetire = false;
                 if (requestedNodes.considerRetiring()) {
-                    resizeable = node.isResizable;
-                    acceptToRetire = acceptToRetire(node);
+                    boolean wantToRetireNode = false;
+                    if ( ! nodeResourceLimits.isWithinRealLimits(offered, cluster)) wantToRetireNode = true;
+                    if (violatesParentHostPolicy(this.nodes, offered)) wantToRetireNode = true;
+                    if ( ! hasCompatibleFlavor(node)) wantToRetireNode = true;
+                    if (offered.status().wantToRetire()) wantToRetireNode = true;
+                    if (requestedNodes.isExclusive() && ! hostsOnly(application.tenant(), application.application(), offered.parentHostname()))
+                        wantToRetireNode = true;
+                    if ((! saturated() && hasCompatibleFlavor(node)) || acceptToRetire(node))
+                        accepted.add(acceptNode(node, wantToRetireNode, node.isResizable));
+                }
+                else {
+                    accepted.add(acceptNode(node, false, false));
                 }
-
-                if ((! saturated() && hasCompatibleFlavor(node) && requestedNodes.acceptable(offered)) || acceptToRetire)
-                    accepted.add(acceptNode(node, shouldRetire(node), resizeable));
             }
             else if (! saturated() && hasCompatibleFlavor(node)) {
                 if ( ! nodeResourceLimits.isWithinRealLimits(offered, cluster)) {
@@ -135,7 +139,7 @@ class NodeAllocation {
                     ++rejectedDueToExclusivity;
                     continue;
                 }
-                if ( requestedNodes.isExclusive() && ! hostsOnly(application, offered.parentHostname())) {
+                if ( requestedNodes.isExclusive() && ! hostsOnly(application.tenant(), application.application(), offered.parentHostname())) {
                     ++rejectedDueToExclusivity;
                     continue;
                 }
@@ -153,15 +157,6 @@ class NodeAllocation {
         return accepted;
     }
 
-    private boolean shouldRetire(PrioritizableNode node) {
-        if ( ! requestedNodes.considerRetiring()) return false;
-        if ( ! nodeResourceLimits.isWithinRealLimits(node.node, cluster)) return true;
-        if (violatesParentHostPolicy(this.nodes, node.node)) return true;
-        if ( ! hasCompatibleFlavor(node)) return true;
-        if (node.node.status().wantToRetire()) return true;
-        if (requestedNodes.isExclusive() && ! hostsOnly(application, node.node.parentHostname())) return true;
-        return false;
-    }
 
     private boolean violatesParentHostPolicy(Collection<PrioritizableNode> accepted, Node offered) {
         return checkForClashingParentHost() && offeredNodeHasParentHostnameAlreadyAccepted(accepted, offered);
@@ -198,13 +193,13 @@ class NodeAllocation {
         return true;
     }
 
-    /** Returns true if this host only hosts the given application (in any instance) */
-    private boolean hostsOnly(ApplicationId application, Optional<String> parentHostname) {
+    /** Returns true if this host only hosts the given applicaton (in any instance) */
+    private boolean hostsOnly(TenantName tenant, ApplicationName application, Optional<String> parentHostname) {
         if (parentHostname.isEmpty()) return true; // yes, as host is exclusive
 
         for (Node nodeOnHost : allNodes.childrenOf(parentHostname.get())) {
             if (nodeOnHost.allocation().isEmpty()) continue;
-            if ( ! allocatedTo(application.tenant(), application.application(), nodeOnHost)) return false;
+            if ( ! allocatedTo(tenant, application, nodeOnHost)) return false;
         }
         return true;
     }
@@ -261,8 +256,8 @@ class NodeAllocation {
             if (resizeable && ! ( node.allocation().isPresent() && node.allocation().get().membership().retired()))
                 node = resize(node);
 
-            if (node.state() != Node.State.active) // reactivated node - wipe state that deactivated it
-                node = node.unretire().removable(false);
+            if (node.state() != Node.State.active) // reactivated node - make sure its not retired
+                node = node.unretire();
         } else {
             ++wasRetiredJustNow;
             node = node.retire(nodeRepository.clock().instant());
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeSpec.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeSpec.java
index f50c988edfd..9971aae1714 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeSpec.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeSpec.java
@@ -60,9 +60,6 @@ public interface NodeSpec {
     /** Returns whether the given node must be resized to match this spec */
     boolean needsResize(Node node);
 
-    /** Returns true if there exist some circumstance where we may accept to have this node allocated */
-    boolean acceptable(Node node);
-
     /**
      * Returns true if a node with given current resources and current spare host resources can be resized
      * in-place to resources in this spec.
@@ -160,9 +157,6 @@ public interface NodeSpec {
         }
 
         @Override
-        public boolean acceptable(Node node) { return true; }
-
-        @Override
         public String toString() { return "request for " + count + " nodes with " + requestedNodeResources; }
 
     }
@@ -217,12 +211,6 @@ public interface NodeSpec {
         public boolean needsResize(Node node) { return false; }
 
         @Override
-        public boolean acceptable(Node node) {
-            // Since we consume all offered nodes we should not accept previously deactivated nodes
-            return node.state() != Node.State.inactive;
-        }
-
-        @Override
         public String toString() { return "request for all nodes of type '" + type + "'"; }
 
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java
index cb39e8fecce..1137ae5ce2c 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java
@@ -105,7 +105,7 @@ class AutoscalingTester {
         try (Mutex lock = nodeRepository().lock(application)){
             for (Node node : nodeRepository().getNodes(application, Node.State.active)) {
                 if (node.allocation().get().membership().retired())
-                    nodeRepository().write(node.with(node.allocation().get().removable(true)), lock);
+                    nodeRepository().write(node.with(node.allocation().get().removable()), lock);
             }
         }
         deploy(application, cluster, resources);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
index dbbad0b8982..5e4bfc2a7bc 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
@@ -179,7 +179,7 @@ public class NodeSerializerTest {
                      (copy.history().event(History.Event.Type.retired).get()).agent());
         assertTrue(copy.allocation().get().membership().retired());
 
-        Node removable = copy.with(node.allocation().get().removable(true));
+        Node removable = copy.with(node.allocation().get().removable());
         Node removableCopy = nodeSerializer.fromJson(Node.State.provisioned, nodeSerializer.toJson(removable));
         assertTrue(removableCopy.allocation().get().isRemovable());
     }
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 e566172b524..cddc1fcb253 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
@@ -20,7 +20,6 @@ import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
-import com.yahoo.vespa.hosted.provision.NodeRepository;
 import org.junit.Test;
 
 import java.util.HashSet;
@@ -381,63 +380,6 @@ public class DockerProvisioningTest {
         }
     }
 
-    @Test
-    public void test_startup_redeployment_with_inactive_nodes() {
-        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)))
-                                                                    .build();
-        tester.makeReadyHosts(5, r).deployZoneApp();
-
-        ApplicationId app1 = tester.makeApplicationId("app1");
-        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.container, 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)));
-
-        assertEquals(2, tester.getNodes(app1, Node.State.active).size());
-        assertEquals(3, tester.getNodes(app1, Node.State.inactive).size());
-
-        // Startup deployment: Not failable
-        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, r), false, false));
-        // ... causes no change
-        assertEquals(2, tester.getNodes(app1, Node.State.active).size());
-        assertEquals(3, tester.getNodes(app1, Node.State.inactive).size());
-    }
-
-    @Test
-    public void inactive_container_nodes_are_reused() {
-        assertInactiveReuse(ClusterSpec.Type.container);
-    }
-
-    @Test
-    public void inactive_content_nodes_are_reused() {
-        assertInactiveReuse(ClusterSpec.Type.content);
-    }
-
-    private void assertInactiveReuse(ClusterSpec.Type clusterType) {
-        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)))
-                                                                    .build();
-        tester.makeReadyHosts(4, r).deployZoneApp();
-
-        ApplicationId app1 = tester.makeApplicationId("app1");
-        ClusterSpec cluster1 = ClusterSpec.request(clusterType, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
-
-        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(4, 1, r)));
-        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, r)));
-
-        // Deactivate any retired nodes - usually done by the RetiredExpirer
-        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());
-    }
-
-
     private Set<String> hostsOf(NodeList nodes) {
         return nodes.asList().stream().map(Node::parentHostname).map(Optional::get).collect(Collectors.toSet());
     }
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 ff2f0ffca96..5c730912c49 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
@@ -888,7 +888,7 @@ public class ProvisioningTest {
         // Application allocates two content nodes initially, with cluster type content
         ClusterSpec cluster = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("music")).vespaVersion("1.2.3").build();
         var initialNodes = tester.activate(application, tester.prepare(application, cluster,
-                                                                       Capacity.from(new ClusterResources(2, 1, defaultResources))));
+                                                                       Capacity.from(new ClusterResources(2, 1, defaultResources), false, false)));
 
         // Application is redeployed with cluster type combined
         cluster = ClusterSpec.request(ClusterSpec.Type.combined, ClusterSpec.Id.from("music"))
@@ -896,7 +896,7 @@ public class ProvisioningTest {
                              .combinedId(Optional.of(ClusterSpec.Id.from("qrs")))
                              .build();
         var newNodes = tester.activate(application, tester.prepare(application, cluster,
-                                                                   Capacity.from(new ClusterResources(2, 1, defaultResources))));
+                                                                   Capacity.from(new ClusterResources(2, 1, defaultResources), false, false)));
 
         assertEquals("Node allocation remains the same", initialNodes, newNodes);
         assertEquals("Cluster type is updated",
@@ -906,7 +906,7 @@ public class ProvisioningTest {
         // Application is redeployed with cluster type content again
         cluster = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("music")).vespaVersion("1.2.3").build();
         newNodes = tester.activate(application, tester.prepare(application, cluster,
-                                                               Capacity.from(new ClusterResources(2, 1, defaultResources))));
+                                                               Capacity.from(new ClusterResources(2, 1, defaultResources), false, false)));
         assertEquals("Node allocation remains the same", initialNodes, newNodes);
         assertEquals("Cluster type is updated",
                      Set.of(ClusterSpec.Type.content),