Merge pull request #5390 from vespa-engine/mpolden/always-enforce-node-count-requirement

Ensure prod node count requirement is respected

9 files changed, 57 insertions, 45 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java
index 3f392674b20..19489cf230f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/CapacityPolicies.java
@@ -3,10 +3,10 @@ package com.yahoo.vespa.hosted.provision.provisioning;
 
 import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterSpec;
-import com.yahoo.config.provision.SystemName;
-import com.yahoo.config.provision.Zone;
 import com.yahoo.config.provision.Flavor;
 import com.yahoo.config.provision.NodeFlavors;
+import com.yahoo.config.provision.SystemName;
+import com.yahoo.config.provision.Zone;
 
 import java.util.Optional;
 
@@ -26,14 +26,14 @@ public class CapacityPolicies {
     }
 
     public int decideSize(Capacity requestedCapacity) {
-        int requestedNodes = requestedCapacity.nodeCount();
+        int requestedNodes = ensureRedundancy(requestedCapacity.nodeCount());
         if (requestedCapacity.isRequired()) return requestedNodes;
 
         switch(zone.environment()) {
             case dev : case test : return 1;
             case perf : return Math.min(requestedCapacity.nodeCount(), 3);
             case staging: return requestedNodes <= 1 ? requestedNodes : Math.max(2, requestedNodes / 10);
-            case prod : return ensureRedundancy(requestedCapacity.nodeCount());
+            case prod : return requestedNodes;
             default : throw new IllegalArgumentException("Unsupported environment " + zone.environment());
         }
     }
@@ -55,13 +55,13 @@ public class CapacityPolicies {
     }
 
     /**
-     * Throw if the node count is 1
+     * Throw if the node count is 1 and we're in a production zone
      *
      * @return the argument node count
      * @throws IllegalArgumentException if only one node is requested
      */
     private int ensureRedundancy(int nodeCount) {
-        if (nodeCount == 1 && zone.system() != SystemName.cd) {
+        if (nodeCount == 1 && zone.environment().isProduction() && zone.system() != SystemName.cd) {
             throw new IllegalArgumentException("Deployments to prod require at least 2 nodes per cluster for redundancy");
         }
         return nodeCount;
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 f45c10f97f1..5c9d05eb8b8 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
@@ -27,7 +27,6 @@ import java.util.Collection;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
-import java.util.function.BiConsumer;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -48,7 +47,6 @@ public class NodeRepositoryProvisioner implements Provisioner {
     private final Zone zone;
     private final Preparer preparer;
     private final Activator activator;
-    private final BiConsumer<List<Node>, String> debugRecorder;
 
     int getSpareCapacityProd() {
         return SPARE_CAPACITY_PROD;
@@ -56,10 +54,10 @@ public class NodeRepositoryProvisioner implements Provisioner {
 
     @Inject
     public NodeRepositoryProvisioner(NodeRepository nodeRepository, NodeFlavors flavors, Zone zone) {
-        this(nodeRepository, flavors, zone, Clock.systemUTC(), (x, y) -> {});
+        this(nodeRepository, flavors, zone, Clock.systemUTC());
     }
 
-    public NodeRepositoryProvisioner(NodeRepository nodeRepository, NodeFlavors flavors, Zone zone, Clock clock, BiConsumer<List<Node>, String> debugRecorder) {
+    public NodeRepositoryProvisioner(NodeRepository nodeRepository, NodeFlavors flavors, Zone zone, Clock clock) {
         this.nodeRepository = nodeRepository;
         this.capacityPolicies = new CapacityPolicies(zone, flavors);
         this.zone = zone;
@@ -67,7 +65,6 @@ public class NodeRepositoryProvisioner implements Provisioner {
                 ? SPARE_CAPACITY_PROD
                 : SPARE_CAPACITY_NONPROD);
         this.activator = new Activator(nodeRepository, clock);
-        this.debugRecorder = debugRecorder;
     }
 
     /**
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
index 730803708e7..b808921a713 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodePatcher.java
@@ -4,7 +4,6 @@ package com.yahoo.vespa.hosted.provision.restapi.v2;
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.NodeFlavors;
-import com.yahoo.config.provision.NodeType;
 import com.yahoo.io.IOUtils;
 import com.yahoo.slime.Inspector;
 import com.yahoo.slime.Type;
@@ -108,8 +107,6 @@ public class NodePatcher {
 
     private Node applyField(String name, Inspector value) {
         switch (name) {
-            case "convergedStateVersion" :
-                return node; // TODO: Ignored, can be removed when callers no longer include this field
             case "currentRebootGeneration" :
                 return node.withCurrentRebootGeneration(asLong(value), nodeRepository.clock().instant());
             case "currentRestartGeneration" :
@@ -123,8 +120,6 @@ public class NodePatcher {
                 return node.with(node.status().withVespaVersion(versionFromImage));
             case "currentVespaVersion" :
                 return node.with(node.status().withVespaVersion(Version.fromString(asString(value))));
-            case "currentHostedVersion" :
-                return node; // TODO: Ignored, can be removed when callers no longer include this field
             case "failCount" :
                 return node.with(node.status().setFailCount(asLong(value).intValue()));
             case "flavor" :
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java
index 7c064c5faf1..194857c8219 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirerTest.java
@@ -218,8 +218,7 @@ public class FailedExpirerTest {
             this.nodeRepository = new NodeRepository(nodeFlavors, curator, clock, zone,
                                                      new MockNameResolver().mockAnyLookup(),
                                                      new DockerImage("docker-image"));
-            this.provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, Zone.defaultZone(), clock,
-                                                             (x, y) -> {});
+            this.provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, Zone.defaultZone(), clock);
             this.expirer = new FailedExpirer(nodeRepository, zone, clock, Duration.ofMinutes(30),
                                              new JobControl(nodeRepository.database()));
         }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java
index 3766bbba602..6f0449d8eb0 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ReservationExpirerTest.java
@@ -43,7 +43,7 @@ public class ReservationExpirerTest {
         NodeRepository nodeRepository = new NodeRepository(flavors, curator, clock, Zone.defaultZone(),
                                                            new MockNameResolver().mockAnyLookup(),
                                                            new DockerImage("docker-registry.domain.tld:8080/dist/vespa"));
-        NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, flavors, Zone.defaultZone(), clock, (x,y) -> {});
+        NodeRepositoryProvisioner provisioner = new NodeRepositoryProvisioner(nodeRepository, flavors, Zone.defaultZone(), clock);
 
         List<Node> nodes = new ArrayList<>(2);
         nodes.add(nodeRepository.createNode(UUID.randomUUID().toString(), UUID.randomUUID().toString(), Optional.empty(), flavors.getFlavorOrThrow("default"), NodeType.tenant));
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
index 9587fedb7b2..cb2a752215f 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
@@ -94,7 +94,7 @@ public class MultigroupProvisioningTest {
 
     @Test
     public void test_one_node_and_group_to_two() {
-        ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.prod, RegionName.from("us-east")));
+        ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.perf, RegionName.from("us-east")));
 
         ApplicationId application1 = tester.makeApplicationId();
 
@@ -106,7 +106,7 @@ public class MultigroupProvisioningTest {
 
     @Test
     public void test_one_node_and_group_to_two_with_flavor_migration() {
-        ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.prod, RegionName.from("us-east")));
+        ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.perf, RegionName.from("us-east")));
 
         ApplicationId application1 = tester.makeApplicationId();
 
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 26e0e714201..801bee314ab 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
@@ -41,6 +41,7 @@ import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.UUID;
+import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
@@ -728,6 +729,16 @@ public class ProvisioningTest {
         assertEquals(4, tester.getNodes(application, Node.State.active).size());
     }
 
+    @Test
+    public void required_capacity_respects_prod_redundancy_requirement() {
+        ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.prod, RegionName.from("us-east")));
+        ApplicationId application = tester.makeApplicationId();
+        try {
+            prepare(application, 1, 0, 1, 0, true, "default", Version.fromString("6.42"), tester);
+            fail("Expected exception");
+        } catch (IllegalArgumentException ignored) {}
+    }
+
     private void assertCorrectFlavorPreferences(boolean largeIsStock) {
         FlavorConfigBuilder b = new FlavorConfigBuilder();
         b.addFlavor("large", 4., 8., 100, Flavor.Type.BARE_METAL).cost(10).stock(largeIsStock);
@@ -762,21 +773,31 @@ public class ProvisioningTest {
         tester.assertNumberOfNodesWithFlavor(contentNodes, "large-variant-variant", 3);
     }
 
-    private SystemState prepare(ApplicationId application, int container0Size, int container1Size, int content0Size, int content1Size, String flavor, ProvisioningTester tester) {
-        return prepare(application, container0Size, container1Size, content0Size, content1Size, flavor, Version.fromString("6.42"), tester);
+    private SystemState prepare(ApplicationId application, int container0Size, int container1Size, int content0Size,
+                                int content1Size, String flavor, ProvisioningTester tester) {
+        return prepare(application, container0Size, container1Size, content0Size, content1Size, flavor,
+                       Version.fromString("6.42"), tester);
+    }
+
+    private SystemState prepare(ApplicationId application, int container0Size, int container1Size, int content0Size,
+                                int content1Size, String flavor, Version wantedVersion, ProvisioningTester tester) {
+        return prepare(application, container0Size, container1Size, content0Size, content1Size, false, flavor,
+                       wantedVersion, tester);
     }
 
-    private SystemState prepare(ApplicationId application, int container0Size, int container1Size, int content0Size, int content1Size, String flavor, Version wantedVersion, ProvisioningTester tester) {
+    private SystemState prepare(ApplicationId application, int container0Size, int container1Size, int content0Size,
+                                int content1Size, boolean required, String flavor, Version wantedVersion,
+                                ProvisioningTester tester) {
         // "deploy prepare" with a two container clusters and a storage cluster having of two groups
         ClusterSpec containerCluster0 = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("container0"), wantedVersion);
         ClusterSpec containerCluster1 = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("container1"), wantedVersion);
         ClusterSpec contentCluster0 = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("content0"), wantedVersion);
         ClusterSpec contentCluster1 = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("content1"), wantedVersion);
 
-        Set<HostSpec> container0 = prepare(application, containerCluster0, container0Size, 1, flavor, tester);
-        Set<HostSpec> container1 = prepare(application, containerCluster1, container1Size, 1, flavor, tester);
-        Set<HostSpec> content0 = prepare(application, contentCluster0, content0Size, 1, flavor, tester);
-        Set<HostSpec> content1 = prepare(application, contentCluster1, content1Size, 1, flavor, tester);
+        Set<HostSpec> container0 = prepare(application, containerCluster0, container0Size, 1, required, flavor, tester);
+        Set<HostSpec> container1 = prepare(application, containerCluster1, container1Size, 1, required, flavor, tester);
+        Set<HostSpec> content0 = prepare(application, contentCluster0, content0Size, 1, required, flavor, tester);
+        Set<HostSpec> content1 = prepare(application, contentCluster1, content1Size, 1, required, flavor, tester);
 
         Set<HostSpec> allHosts = new HashSet<>();
         allHosts.addAll(container0);
@@ -784,10 +805,11 @@ public class ProvisioningTest {
         allHosts.addAll(content0);
         allHosts.addAll(content1);
 
-        int expectedContainer0Size = tester.capacityPolicies().decideSize(Capacity.fromNodeCount(container0Size));
-        int expectedContainer1Size = tester.capacityPolicies().decideSize(Capacity.fromNodeCount(container1Size));
-        int expectedContent0Size = tester.capacityPolicies().decideSize(Capacity.fromNodeCount(content0Size));
-        int expectedContent1Size = tester.capacityPolicies().decideSize(Capacity.fromNodeCount(content1Size));
+        Function<Integer, Capacity> capacity = required ? Capacity::fromRequiredNodeCount : Capacity::fromNodeCount;
+        int expectedContainer0Size = tester.capacityPolicies().decideSize(capacity.apply(container0Size));
+        int expectedContainer1Size = tester.capacityPolicies().decideSize(capacity.apply(container1Size));
+        int expectedContent0Size = tester.capacityPolicies().decideSize(capacity.apply(content0Size));
+        int expectedContent1Size = tester.capacityPolicies().decideSize(capacity.apply(content1Size));
 
         assertEquals("Hosts in each group cluster is disjunct and the total number of unretired nodes is correct",
                      expectedContainer0Size + expectedContainer1Size + expectedContent0Size + expectedContent1Size,
@@ -806,9 +828,10 @@ public class ProvisioningTest {
         return new SystemState(allHosts, container0, container1, content0, content1);
     }
 
-    private Set<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, int nodeCount, int groups, String flavor, ProvisioningTester tester) {
+    private Set<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, int nodeCount, int groups,
+                                  boolean required, String flavor, ProvisioningTester tester) {
         if (nodeCount == 0) return Collections.emptySet(); // this is a shady practice
-        return new HashSet<>(tester.prepare(application, cluster, nodeCount, groups, flavor));
+        return new HashSet<>(tester.prepare(application, cluster, nodeCount, groups, required, flavor));
     }
 
     private static class SystemState {
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 976069758ea..0f9d06a69cc 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
@@ -64,7 +64,6 @@ public class ProvisioningTester {
     private final NodeRepositoryProvisioner provisioner;
     private final CapacityPolicies capacityPolicies;
     private final ProvisionLogger provisionLogger;
-    private final List<AllocationSnapshot> allocationSnapshots = new ArrayList<>();
 
     private int nextHost = 0;
     private int nextIP = 0;
@@ -86,8 +85,7 @@ public class ProvisioningTester {
                                                      new DockerImage("docker-registry.domain.tld:8080/dist/vespa"));
             this.orchestrator = mock(Orchestrator.class);
             doThrow(new RuntimeException()).when(orchestrator).acquirePermissionToRemove(any());
-            this.provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone, clock,
-                    (x,y) -> allocationSnapshots.add(new AllocationSnapshot(new NodeList(x), "Provision tester", y)));
+            this.provisioner = new NodeRepositoryProvisioner(nodeRepository, nodeFlavors, zone, clock);
             this.capacityPolicies = new CapacityPolicies(zone, nodeFlavors);
             this.provisionLogger = new NullProvisionLogger();
         }
@@ -130,7 +128,14 @@ public class ProvisioningTester {
     public void patchNode(Node node) { nodeRepository.write(node); }
 
     public List<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, int nodeCount, int groups, String flavor) {
-        return prepare(application, cluster, Capacity.fromNodeCount(nodeCount, Optional.ofNullable(flavor)), groups);
+        return prepare(application, cluster, nodeCount, groups, false, flavor);
+    }
+
+    public List<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, int nodeCount, int groups, boolean required, String flavor) {
+        Capacity capacity = required
+                ? Capacity.fromRequiredNodeCount(nodeCount)
+                : Capacity.fromNodeCount(nodeCount, Optional.ofNullable(flavor));
+        return prepare(application, cluster, capacity, groups);
     }
 
     public List<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, Capacity capacity, int groups) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
index cbdd4dec3dc..dbf08597d6b 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
@@ -196,12 +196,6 @@ public class RestApiTest {
                                    Utf8.toBytes("{\"currentVespaVersion\": \"5.104.142\"}"), Request.Method.PATCH),
                        "{\"message\":\"Updated host4.yahoo.com\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
-                        Utf8.toBytes("{\"currentHostedVersion\": \"2.1.2408\"}"), Request.Method.PATCH),
-                "{\"message\":\"Updated host4.yahoo.com\"}");
-        assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
-                        Utf8.toBytes("{\"convergedStateVersion\": \"5.104.142-2.1.2408\"}"), Request.Method.PATCH),
-                "{\"message\":\"Updated host4.yahoo.com\"}");
-        assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
                                    Utf8.toBytes("{\"hardwareFailureDescription\": \"memory_mcelog\"}"), Request.Method.PATCH),
                        "{\"message\":\"Updated host4.yahoo.com\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
@@ -417,7 +411,6 @@ public class RestApiTest {
                                            "\"currentRebootGeneration\": 3," +
                                            "\"flavor\": \"medium-disk\"," +
                                            "\"currentVespaVersion\": \"5.104.142\"," +
-                                           "\"currentHostedVersion\": \"2.1.2408\"," +
                                            "\"hardwareFailureDescription\": \"memory_mcelog\"," +
                                            "\"failCount\": 0," +
                                            "\"parentHostname\": \"parent.yahoo.com\"" +