Merge pull request #15107 from vespa-engine/freva/exclusive-retire

Force migration on exclusivity violation in dynamically provisioned zones

2 files changed, 40 insertions, 33 deletions

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 1e98160955c..68e11c4c995 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
@@ -127,11 +127,7 @@ class NodeAllocation {
                     ++rejectedDueToClashingParentHost;
                     continue;
                 }
-                if ( ! exclusiveTo(application, candidate.parentHostname())) {
-                    ++rejectedDueToExclusivity;
-                    continue;
-                }
-                if ( requestedNodes.isExclusive() && ! hostsOnly(application, candidate.parentHostname())) {
+                if ( violatesExclusivity(candidate)) {
                     ++rejectedDueToExclusivity;
                     continue;
                 }
@@ -158,7 +154,7 @@ class NodeAllocation {
         if (violatesParentHostPolicy(candidate)) return true;
         if ( ! hasCompatibleFlavor(candidate)) return true;
         if (candidate.wantToRetire()) return true;
-        if (requestedNodes.isExclusive() && ! hostsOnly(application, candidate.parentHostname())) return true;
+        if (violatesExclusivity(candidate)) return true;
         return false;
     }
 
@@ -182,35 +178,23 @@ class NodeAllocation {
         return false;
     }
 
-    /**
-     * If a parent host is given, and it hosts another application which requires exclusive access
-     * to the physical host, then we cannot host this application on it.
-     */
-    private boolean exclusiveTo(ApplicationId applicationId, Optional<String> parentHostname) {
-        if (parentHostname.isEmpty()) return true;
-        for (Node nodeOnHost : allNodes.childrenOf(parentHostname.get())) {
-            if (nodeOnHost.allocation().isEmpty()) continue;
-            if ( nodeOnHost.allocation().get().membership().cluster().isExclusive() &&
-                 ! allocatedTo(applicationId, nodeOnHost))
-                return false;
-        }
-        return true;
-    }
+    private boolean violatesExclusivity(NodeCandidate candidate) {
+        if (candidate.parentHostname().isEmpty()) return false;
 
-    /** Returns true if this host only hosts the given application (in any instance) */
-    private boolean hostsOnly(ApplicationId application, Optional<String> parentHostname) {
-        if (parentHostname.isEmpty()) return true; // yes, as host is exclusive
+        // In dynamic provisioned zones a node requiring exclusivity must be on a host that has exclusiveTo equal to its owner
+        if (nodeRepository.zone().getCloud().dynamicProvisioning())
+            return requestedNodes.isExclusive() &&
+                    ! candidate.parent.flatMap(Node::exclusiveTo).map(application::equals).orElse(false);
 
-        for (Node nodeOnHost : allNodes.childrenOf(parentHostname.get())) {
+        // In non-dynamic provisioned zones we require that if either of the nodes on the host requires exclusivity,
+        // then all the nodes on the host must have the same owner
+        for (Node nodeOnHost : allNodes.childrenOf(candidate.parentHostname().get())) {
             if (nodeOnHost.allocation().isEmpty()) continue;
-            if ( ! allocatedTo(application, nodeOnHost)) return false;
+            if (requestedNodes.isExclusive() || nodeOnHost.allocation().get().membership().cluster().isExclusive()) {
+                if ( ! nodeOnHost.allocation().get().owner().equals(application)) return true;
+            }
         }
-        return true;
-    }
-
-    private boolean allocatedTo(ApplicationId applicationId, Node node) {
-        if (node.allocation().isEmpty()) return false;
-        return node.allocation().get().owner().equals(applicationId);
+        return false;
     }
 
     /**
@@ -390,7 +374,7 @@ class NodeAllocation {
     /** Prefer to unretire nodes we don't want to retire, and otherwise those with lower index */
     private List<NodeCandidate> byUnretiringPriority(Collection<NodeCandidate> candidates) {
         return candidates.stream()
-                         .sorted(Comparator.comparing((NodeCandidate n) -> n.wantToRetire())
+                         .sorted(Comparator.comparing(NodeCandidate::wantToRetire)
                                            .thenComparing(n -> n.allocation().get().membership().index()))
                          .collect(Collectors.toList());
     }
@@ -407,7 +391,7 @@ class NodeAllocation {
             reasons.add("insufficient real resources on hosts");
 
         if (reasons.isEmpty()) return "";
-        return ": Not enough nodes available due to " + reasons.stream().collect(Collectors.joining(", "));
+        return ": Not enough nodes available due to " + String.join(", ", reasons);
     }
 
     static class FlavorCount {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java
index b871404aa9d..40d0f52dc37 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java
@@ -96,6 +96,7 @@ public class DynamicDockerProvisionTest {
 
         // Deploy new exclusive application
         ApplicationId application3 = ProvisioningTester.makeApplicationId();
+        mockHostProvisioner(hostProvisioner, "large", 3, application3);
         prepareAndActivate(application3, clusterSpec("mycluster", true), 4, 1, resources);
         verify(hostProvisioner).provisionHosts(List.of(104, 105, 106, 107), resources, application3,
                 Version.emptyVersion, HostSharing.exclusive);
@@ -159,6 +160,28 @@ public class DynamicDockerProvisionTest {
     }
 
     @Test
+    public void retires_on_exclusivity_violation() {
+        ApplicationId application1 = ProvisioningTester.makeApplicationId();
+        NodeResources resources = new NodeResources(1, 4, 10, 1);
+
+        mockHostProvisioner(hostProvisioner, "large", 3, null); // Provision shared hosts
+        prepareAndActivate(application1, clusterSpec("mycluster"), 4, 1, resources);
+        Set<Node> initialNodes = tester.nodeRepository().list(application1).stream().collect(Collectors.toSet());
+        assertEquals(4, initialNodes.size());
+
+        // Redeploy same application with exclusive=true
+        mockHostProvisioner(hostProvisioner, "large", 3, application1);
+        prepareAndActivate(application1, clusterSpec("mycluster", true), 4, 1, resources);
+        assertEquals(8, tester.nodeRepository().list(application1).size());
+        assertEquals(initialNodes, tester.nodeRepository().list(application1).retired().stream().collect(Collectors.toSet()));
+
+        // Redeploy without exclusive again is no-op
+        prepareAndActivate(application1, clusterSpec("mycluster"), 4, 1, resources);
+        assertEquals(8, tester.nodeRepository().list(application1).size());
+        assertEquals(initialNodes, tester.nodeRepository().list(application1).retired().stream().collect(Collectors.toSet()));
+    }
+
+    @Test
     public void node_indices_are_unique_even_when_a_node_is_left_in_reserved_state() {
         NodeResources resources = new NodeResources(10, 10, 10, 10);
         ApplicationId app = ProvisioningTester.makeApplicationId();