Avoid prepare and activate infra app if candidate nodes are active and at target version

2 files changed, 36 insertions, 28 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisioner.java
index 76091c1ceb4..9d87a835960 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisioner.java
@@ -62,39 +62,42 @@ public class InfrastructureProvisioner extends Maintainer {
                     continue;
                 }
 
-                List<Version> wantedVersions = nodeRepository()
-                        .getNodes(nodeType, Node.State.ready, Node.State.reserved, Node.State.active, Node.State.inactive)
-                        .stream()
-                        .map(node -> node.allocation()
-                                .map(allocation -> allocation.membership().cluster().vespaVersion())
-                                .orElse(null))
-                        .collect(Collectors.toList());
-                if (wantedVersions.isEmpty()) {
+                List<Node> candidateNodes = nodeRepository()
+                        .getNodes(nodeType, Node.State.ready, Node.State.reserved, Node.State.active, Node.State.inactive);
+                if (candidateNodes.isEmpty()) {
                     logger.log(LogLevel.DEBUG, "No nodes to provision for " + nodeType + ", removing application");
                     removeApplication(application.getApplicationId());
                     continue;
                 }
 
-                List<HostSpec> hostSpecs = provisioner.prepare(
-                        application.getApplicationId(),
-                        application.getClusterSpecWithVersion(targetVersion.get()),
-                        application.getCapacity(),
-                        1, // groups
-                        logger::log);
+                if (!candidateNodes.stream().allMatch(node ->
+                    node.state() == Node.State.active &&
+                            node.allocation()
+                                    .map(allocation -> allocation.membership().cluster().vespaVersion().equals(targetVersion.get()))
+                                    .orElse(false))) {
+                    List<HostSpec> hostSpecs = provisioner.prepare(
+                            application.getApplicationId(),
+                            application.getClusterSpecWithVersion(targetVersion.get()),
+                            application.getCapacity(),
+                            1, // groups
+                            logger::log);
+
+                    // Sanity-check hostSpecs is the same list as candidateNodes?
 
-                NestedTransaction nestedTransaction = new NestedTransaction();
-                provisioner.activate(nestedTransaction, application.getApplicationId(), hostSpecs);
-                nestedTransaction.commit();
+                    NestedTransaction nestedTransaction = new NestedTransaction();
+                    provisioner.activate(nestedTransaction, application.getApplicationId(), hostSpecs);
+                    nestedTransaction.commit();
+                }
 
                 duperModel.infraApplicationActivated(
                         application.getApplicationId(),
-                        hostSpecs.stream().map(HostSpec::hostname).map(HostName::from).collect(Collectors.toList()));
+                        candidateNodes.stream().map(Node::hostname).map(HostName::from).collect(Collectors.toList()));
 
                 String detail;
-                if (hostSpecs.size() < 10) {
-                    detail = ": " + hostSpecs.stream().map(HostSpec::hostname).collect(Collectors.joining(","));
+                if (candidateNodes.size() < 10) {
+                    detail = ": " + candidateNodes.stream().map(Node::hostname).collect(Collectors.joining(","));
                 } else {
-                    detail = " with " + hostSpecs.size() + " hosts";
+                    detail = " with " + candidateNodes.size() + " hosts";
                 }
                 logger.log(LogLevel.DEBUG, "Infrastructure application " + application.getApplicationId() + " activated" + detail);
             } catch (RuntimeException e) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisionerTest.java
index 85092053bdd..bc83e3525ad 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisionerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureProvisionerTest.java
@@ -123,19 +123,18 @@ public class InfrastructureProvisionerTest {
 
         when(provisioner.prepare(any(), any(), any(), anyInt(), any())).thenReturn(Arrays.asList(
                 new HostSpec(node1.value(), Collections.emptyList()),
-                new HostSpec(node2.value(), Collections.emptyList()),
                 new HostSpec(node3.value(), Collections.emptyList())));
 
         infrastructureProvisioner.maintain();
 
         verify(provisioner).prepare(eq(application.getApplicationId()), any(), any(), anyInt(), any());
         verify(provisioner).activate(any(), eq(application.getApplicationId()), any());
-        verify(duperModelInfraApi).infraApplicationActivated(application.getApplicationId(), Arrays.asList(node1, node2, node3));
+        verify(duperModelInfraApi).infraApplicationActivated(application.getApplicationId(), Arrays.asList(node3, node1));
     }
 
 
     @Test
-    public void activates_the_first_time_after_jvm_start() {
+    public void always_activates_for_dupermodel() {
         when(infrastructureVersions.getTargetVersionFor(eq(nodeType))).thenReturn(Optional.of(target));
 
         addNode(1, Node.State.active, Optional.of(target));
@@ -146,9 +145,15 @@ public class InfrastructureProvisionerTest {
 
         infrastructureProvisioner.maintain();
 
-        verify(provisioner).prepare(eq(application.getApplicationId()), any(), any(), anyInt(), any());
-        verify(provisioner).activate(any(), eq(application.getApplicationId()), any());
-        verify(duperModelInfraApi).infraApplicationActivated(application.getApplicationId(), Arrays.asList(node1));
+        verify(provisioner, never()).prepare(any(), any(), any(), anyInt(), any());
+        verify(provisioner, never()).activate(any(), any(), any());
+        verify(duperModelInfraApi, times(1)).infraApplicationActivated(application.getApplicationId(), Arrays.asList(node1));
+
+        infrastructureProvisioner.maintain();
+
+        verify(provisioner, never()).prepare(any(), any(), any(), anyInt(), any());
+        verify(provisioner, never()).activate(any(), any(), any());
+        verify(duperModelInfraApi, times(2)).infraApplicationActivated(application.getApplicationId(), Arrays.asList(node1));
     }
 
     @Test
@@ -168,7 +173,7 @@ public class InfrastructureProvisionerTest {
 
         verify(provisioner).prepare(eq(application.getApplicationId()), any(), any(), anyInt(), any());
         verify(provisioner).activate(any(), eq(application.getApplicationId()), any());
-        verify(duperModelInfraApi).infraApplicationActivated(application.getApplicationId(), Arrays.asList(node2, node3));
+        verify(duperModelInfraApi).infraApplicationActivated(application.getApplicationId(), Arrays.asList(node3, node2));
     }
 
     @Test