Merge pull request #21407 from vespa-engine/bratseth/reuse-indexesv7.550.61

Reuse node indexes

8 files changed, 77 insertions, 14 deletions

diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java b/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java
index d550c5287cc..03c35f789b1 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/Zone.java
@@ -68,6 +68,9 @@ public class Zone {
         return region;
     }
 
+    /** Returns the string "environment.region" */
+    public String systemLocalValue() { return environment + "." + region; }
+
     /** Do not use */
     public static Zone defaultZone() {
         return new Zone(Cloud.defaultCloud(), SystemName.defaultSystem(), Environment.defaultEnvironment(), RegionName.defaultName());
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java b/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java
index 34e69e735fa..1572286207d 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/FlagSource.java
@@ -9,6 +9,8 @@ import java.util.Optional;
  * @author hakonhall
  */
 public interface FlagSource {
-    /** Get raw flag for the given vector (specifying hostname, application id, etc). */
+
+    /** Returns the raw flag for the given vector (specifying hostname, application id, etc). */
     Optional<RawFlag> fetch(FlagId id, FetchVector vector);
+
 }
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
index 271c75fe446..0ef2e51fe1f 100644
--- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
+++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java
@@ -383,6 +383,13 @@ public class Flags {
             "Takes effect at redeployment",
             ZONE_ID, APPLICATION_ID);
 
+    public static final UnboundBooleanFlag REUSE_NODE_INDEXES = defineFeatureFlag(
+            "reuse-node-indexes", false,
+            List.of("bratseth"), "2022-02-25", "2022-04-25",
+            "Whether we should reuse earlier indexes when allocating new nodes",
+            "Takes effect immediately",
+            ZONE_ID);
+
     /** WARNING: public for testing: All flags should be defined in {@link Flags}. */
     public static UnboundBooleanFlag defineFeatureFlag(String flagId, boolean defaultValue, List<String> owners,
                                                        String createdAt, String expiresAt, String description,
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
index e5e5ce27cf1..cb82585fe01 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
@@ -280,8 +280,8 @@ public class Nodes {
     }
 
     public List<Node> deallocateRecursively(String hostname, Agent agent, String reason) {
-        Node nodeToDirty = node(hostname).orElseThrow(() ->
-                                                                 new IllegalArgumentException("Could not deallocate " + hostname + ": Node not found"));
+        Node nodeToDirty = node(hostname).orElseThrow(() -> new IllegalArgumentException("Could not deallocate " +
+                                                                                         hostname + ": Node not found"));
 
         List<Node> nodesToDirty =
                 (nodeToDirty.type().isHost() ?
@@ -289,7 +289,6 @@ public class Nodes {
                  Stream.of(nodeToDirty))
                         .filter(node -> node.state() != Node.State.dirty)
                         .collect(Collectors.toList());
-
         List<String> hostnamesNotAllowedToDirty = nodesToDirty.stream()
                                                               .filter(node -> node.state() != Node.State.provisioned)
                                                               .filter(node -> node.state() != Node.State.failed)
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 7d15a2b30b1..4dc199dd7b2 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
@@ -17,7 +17,9 @@ import com.yahoo.config.provision.ProvisionLogger;
 import com.yahoo.config.provision.Provisioner;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.transaction.Mutex;
+import com.yahoo.vespa.flags.FetchVector;
 import com.yahoo.vespa.flags.FlagSource;
+import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
@@ -62,7 +64,8 @@ public class NodeRepositoryProvisioner implements Provisioner {
     @Inject
     public NodeRepositoryProvisioner(NodeRepository nodeRepository,
                                      Zone zone,
-                                     ProvisionServiceProvider provisionServiceProvider, FlagSource flagSource) {
+                                     ProvisionServiceProvider provisionServiceProvider,
+                                     FlagSource flagSource) {
         this.nodeRepository = nodeRepository;
         this.allocationOptimizer = new AllocationOptimizer(nodeRepository);
         this.capacityPolicies = new CapacityPolicies(nodeRepository);
@@ -113,7 +116,10 @@ public class NodeRepositoryProvisioner implements Provisioner {
                                                                                  : requested.minResources().nodeResources();
             nodeSpec = NodeSpec.from(requested.type());
         }
-        return asSortedHosts(preparer.prepare(application, cluster, nodeSpec, groups), resources);
+        var reuseIndexes = Flags.REUSE_NODE_INDEXES.bindTo(nodeRepository.flagSource())
+                                                   .with(FetchVector.Dimension.ZONE_ID, zone.systemLocalValue())
+                                                   .value();
+        return asSortedHosts(preparer.prepare(application, cluster, nodeSpec, groups, reuseIndexes), resources);
     }
 
     @Override
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java
index 838d6751d09..a6f16d3d65e 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java
@@ -34,9 +34,10 @@ class Preparer {
     }
 
     /** Prepare all required resources for the given application and cluster */
-    public List<Node> prepare(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, int wantedGroups) {
+    public List<Node> prepare(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, int wantedGroups,
+                              boolean reuseIndexes) {
         try {
-            var nodes = prepareNodes(application, cluster, requestedNodes, wantedGroups);
+            var nodes = prepareNodes(application, cluster, requestedNodes, wantedGroups, reuseIndexes);
             prepareLoadBalancer(application, cluster, requestedNodes);
             return nodes;
         }
@@ -55,13 +56,14 @@ class Preparer {
      // Note: This operation may make persisted changes to the set of reserved and inactive nodes,
      // but it may not change the set of active nodes, as the active nodes must stay in sync with the
      // active config model which is changed on activate
-    private List<Node> prepareNodes(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, int wantedGroups) {
+    private List<Node> prepareNodes(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes,
+                                    int wantedGroups, boolean reuseIndexes) {
         NodesAndHosts<LockedNodeList> allNodesAndHosts = groupPreparer.createNodesAndHostUnlocked();
         NodeList appNodes = allNodesAndHosts.nodes().owner(application);
         List<Node> surplusNodes = findNodesInRemovableGroups(appNodes, cluster, wantedGroups);
 
         List<Integer> usedIndices = appNodes.cluster(cluster.id()).mapToList(node -> node.allocation().get().membership().index());
-        NodeIndices indices = new NodeIndices(usedIndices, ! cluster.type().isContent());
+        NodeIndices indices = new NodeIndices(usedIndices, reuseIndexes || ! cluster.type().isContent());
         List<Node> acceptedNodes = new ArrayList<>();
 
         for (int groupIndex = 0; groupIndex < wantedGroups; groupIndex++) {
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 d0eb55469b6..ba9da20b615 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
@@ -3,6 +3,7 @@ package com.yahoo.vespa.hosted.provision.provisioning;
 
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.ApplicationTransaction;
 import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterMembership;
 import com.yahoo.config.provision.ClusterResources;
@@ -16,11 +17,14 @@ import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.NodeAllocationException;
 import com.yahoo.config.provision.ParentHostUnavailableException;
+import com.yahoo.config.provision.ProvisionLock;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
 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.NodeMutex;
 import com.yahoo.vespa.hosted.provision.maintenance.ReservationExpirer;
 import com.yahoo.vespa.hosted.provision.maintenance.TestMetric;
 import com.yahoo.vespa.hosted.provision.node.Agent;
@@ -32,6 +36,7 @@ import com.yahoo.vespa.service.duper.InfraApplication;
 import org.junit.Test;
 
 import java.time.Duration;
+import java.time.Instant;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -47,6 +52,7 @@ import java.util.stream.Collectors;
 import static java.time.temporal.ChronoUnit.MILLIS;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -145,6 +151,39 @@ public class ProvisioningTest {
     }
 
     @Test
+    public void application_deployment_reuses_node_indexes() {
+        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))).build();
+
+        ApplicationId app1 = ProvisioningTester.applicationId("app1");
+
+        tester.makeReadyHosts(21, defaultResources).activateTenantHosts();
+
+        // deploy
+        SystemState state1 = prepare(app1, 2, 2, 3, 3, defaultResources, tester);
+        tester.activate(app1, state1.allHosts);
+        Set<Integer> state1Indexes = state1.allHosts.stream().map(hostSpec -> hostSpec.membership().get().index()).collect(Collectors.toSet());
+
+        // deallocate 2 nodes with index 0
+        NodeMutex node1 = tester.nodeRepository().nodes().lockAndGet(tester.removeOne(state1.container0).hostname()).get();
+        NodeMutex node2 = tester.nodeRepository().nodes().lockAndGet(tester.removeOne(state1.content0).hostname()).get();
+        tester.nodeRepository().nodes().write(node1.node().withoutAllocation(), node1);
+        tester.nodeRepository().nodes().write(node2.node().withoutAllocation(), node1);
+
+        // redeploy to get new nodes
+        SystemState state2 = prepare(app1, 2, 2, 3, 3, defaultResources, tester);
+        Set<Integer> state2Indexes = state2.allHosts.stream().map(hostSpec -> hostSpec.membership().get().index()).collect(Collectors.toSet());
+        assertEquals("Indexes are reused", state1Indexes, state2Indexes);
+
+        // if nodes are e.g failed indexes are not reused as they are still allocated
+        tester.nodeRepository().nodes().fail(tester.removeOne(state2.container0).hostname(), Agent.system, "test");
+        tester.nodeRepository().nodes().fail(tester.removeOne(state2.content0).hostname(), Agent.system, "test");
+        SystemState state3 = prepare(app1, 2, 2, 3, 3, defaultResources, tester);
+        Set<Integer> state3Indexes = state3.allHosts.stream().map(hostSpec -> hostSpec.membership().get().index()).collect(Collectors.toSet());
+        assertNotEquals("Indexes are not reused", state2Indexes, state3Indexes);
+
+    }
+
+    @Test
     public void nodeVersionIsReturnedIfSet() {
         ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.dev, RegionName.from("us-east"))).build();
 
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 b781f397f70..7b988f83c01 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
@@ -30,6 +30,7 @@ import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.curator.transaction.CuratorTransaction;
 import com.yahoo.vespa.flags.FlagSource;
+import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
@@ -66,9 +67,6 @@ import java.util.stream.Collectors;
 import static com.yahoo.config.provision.NodeResources.StorageType.local;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.doThrow;
-import static org.mockito.Mockito.mock;
 
 /**
  * A test utility for provisioning tests.
@@ -151,6 +149,7 @@ public class ProvisioningTester {
     public CapacityPolicies capacityPolicies() { return capacityPolicies; }
     public NodeList getNodes(ApplicationId id, Node.State ... inState) { return nodeRepository.nodes().list(inState).owner(id); }
     public InMemoryFlagSource flagSource() { return (InMemoryFlagSource) nodeRepository.flagSource(); }
+    public Node node(String hostname) { return nodeRepository.nodes().node(hostname).get(); }
 
     public int decideSize(Capacity capacity, ApplicationId application) {
         return capacityPolicies.applyOn(capacity, application).minResources().nodes();
@@ -688,6 +687,12 @@ public class ProvisioningTester {
             return this;
         }
 
+        private FlagSource defaultFlagSource() {
+            var flagSource = new InMemoryFlagSource();
+            flagSource.withBooleanFlag(Flags.REUSE_NODE_INDEXES.id(), true);
+            return flagSource;
+        }
+
         public ProvisioningTester build() {
             return new ProvisioningTester(Optional.ofNullable(curator).orElseGet(MockCurator::new),
                                           new NodeFlavors(Optional.ofNullable(flavorsConfig).orElseGet(ProvisioningTester::createConfig)),
@@ -698,7 +703,7 @@ public class ProvisioningTester {
                                           Optional.ofNullable(orchestrator).orElseGet(OrchestratorMock::new),
                                           hostProvisioner,
                                           new LoadBalancerServiceMock(),
-                                          Optional.ofNullable(flagSource).orElseGet(InMemoryFlagSource::new),
+                                          Optional.ofNullable(flagSource).orElse(defaultFlagSource()),
                                           spareCount);
         }