Code review fixes

6 files changed, 42 insertions, 30 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 c958be13a5d..b6955195dcf 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
@@ -23,6 +23,10 @@ import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 /**
+ * This maintainer makes sure that infrastructure nodes are allocated with correct wanted
+ * version. Source for the wanted version comes from the target version set using
+ * /nodes/v2/upgrade/ endpoint.
+ *
  * @author freva
  */
 public class InfrastructureProvisioner extends Maintainer {
@@ -47,7 +51,7 @@ public class InfrastructureProvisioner extends Maintainer {
     protected void maintain() {
         for (HostedVespaApplication application: HOSTED_VESPA_APPLICATIONS) {
             try (Mutex lock = nodeRepository().lock(application.getApplicationId())) {
-                Optional<Version> version = getVersionToProvision(application.getCapacity().type());
+                Optional<Version> version = getTargetVersion(application.getCapacity().type());
                 if (! version.isPresent()) continue;
 
                 List<HostSpec> hostSpecs = provisioner.prepare(
@@ -69,31 +73,32 @@ public class InfrastructureProvisioner extends Maintainer {
      * the version returned by {@link InfrastructureVersions#getTargetVersionFor} unless a provisioning is:
      * <ul>
      *   <li>not possible: no nodes of given type in legal state in node-repo</li>
-     *   <li>redudant: all nodes that can be provisioned already have the right wanted Vespa version</li>
+     *   <li>redundant: all nodes that can be provisioned already have the right wanted Vespa version</li>
      * </ul>
      */
-    Optional<Version> getVersionToProvision(NodeType nodeType) {
-        Optional<Version> wantedWantedVersion = infrastructureVersions.getTargetVersionFor(nodeType);
-        if (!wantedWantedVersion.isPresent()) {
+    Optional<Version> getTargetVersion(NodeType nodeType) {
+        Optional<Version> targetVersion = infrastructureVersions.getTargetVersionFor(nodeType);
+        if (!targetVersion.isPresent()) {
             logger.log(LogLevel.DEBUG, "Skipping provision of " + nodeType + ": No target version set");
             return Optional.empty();
         }
 
-        List<Optional<Version>> currentWantedVersions = nodeRepository().getNodes(nodeType,
+        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()))
+                        .map(allocation -> allocation.membership().cluster().vespaVersion())
+                        .orElse(null))
                 .collect(Collectors.toList());
-        if (currentWantedVersions.isEmpty()) {
+        if (wantedVersions.isEmpty()) {
             logger.log(LogLevel.DEBUG, "Skipping provision of " + nodeType + ": No nodes to provision");
             return Optional.empty();
         }
 
-        if (currentWantedVersions.stream().allMatch(wantedWantedVersion::equals)) {
+        if (wantedVersions.stream().allMatch(targetVersion.get()::equals)) {
             logger.log(LogLevel.DEBUG, "Skipping provision of " + nodeType +
-                    ": Already provisioned to wanted version " + wantedWantedVersion);
+                    ": Already provisioned to target version " + targetVersion);
             return Optional.empty();
         }
-        return wantedWantedVersion;
+        return targetVersion;
     }
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureVersions.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureVersions.java
index b90ec492e1a..52d7a63dc5e 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureVersions.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/InfrastructureVersions.java
@@ -13,7 +13,7 @@ import java.util.logging.Logger;
 /**
  * Multithread safe class to see and set target versions for infrastructure node types.
  * {@link InfrastructureProvisioner} maintainer will then allocate the nodes of given node type
- * with a wantedVersionVersion equal to the given target version.
+ * with a wanted version equal to the given target version.
  *
  * @author freva
  */
@@ -27,29 +27,29 @@ public class InfrastructureVersions {
         this.db = db;
     }
 
-    public void setTargetVersion(NodeType nodeType, Version version, boolean force) {
+    public void setTargetVersion(NodeType nodeType, Version newTargetVersion, boolean force) {
         if (nodeType != NodeType.config && nodeType != NodeType.confighost && nodeType != NodeType.proxyhost) {
             throw new IllegalArgumentException("Cannot set version for type " + nodeType);
         }
 
         try (Lock lock = db.lockInfrastructureVersions()) {
             Map<NodeType, Version> infrastructureVersions = db.readInfrastructureVersions();
-            Optional<Version> currentWantedVersion = Optional.ofNullable(infrastructureVersions.get(nodeType));
+            Optional<Version> currentTargetVersion = Optional.ofNullable(infrastructureVersions.get(nodeType));
 
             // Trying to set the version to the current version, skip
-            if (currentWantedVersion.equals(Optional.of(version))) return;
+            if (currentTargetVersion.equals(Optional.of(newTargetVersion))) return;
 
             // If we don't force the set, we must set the new version to higher than the already set version
-            if (!force) {
-                if (currentWantedVersion.map(curVersion -> curVersion.compareTo(version) > 0).orElse(false))
+            if (!force && currentTargetVersion.isPresent()) {
+                if (currentTargetVersion.get().isAfter(newTargetVersion))
                     throw new IllegalArgumentException(String.format("Cannot downgrade version without setting 'force'. " +
-                            "Current wanted version: %s, attempted to set wanted version: %s",
-                            currentWantedVersion.get().toFullString(), version.toFullString()));
+                            "Current target version: %s, attempted to set target version: %s",
+                            currentTargetVersion.get().toFullString(), newTargetVersion.toFullString()));
             }
 
-            infrastructureVersions.put(nodeType, version);
+            infrastructureVersions.put(nodeType, newTargetVersion);
             db.writeInfrastructureVersions(infrastructureVersions);
-            logger.info("Set target version for " + nodeType + " to " + version.toFullString());
+            logger.info("Set target version for " + nodeType + " to " + newTargetVersion.toFullString());
         }
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java
index 12d4e935231..20a0139a178 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/UpgradeResponse.java
@@ -12,7 +12,11 @@ import java.io.OutputStream;
 import java.util.Comparator;
 import java.util.Map;
 
-/** A response containing infrastructure versions */
+/**
+ * A response containing infrastructure versions
+ *
+ * @author freva
+ */
 public class UpgradeResponse extends HttpResponse {
 
     private final InfrastructureVersions infrastructureVersions;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockProvisioner.java
index 44ee9390e65..af0ca7b6b75 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockProvisioner.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.provision.testutils;
 
 import com.google.inject.Inject;
@@ -15,6 +15,9 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 
+/**
+ * @author freva
+ */
 public class MockProvisioner implements Provisioner {
 
     @Inject
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 56c7114064b..586498619c6 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
@@ -46,7 +46,7 @@ public class InfrastructureProvisionerTest {
         addNode(2, Node.State.dirty, Optional.empty());
         addNode(3, Node.State.active, Optional.of(oldVersion));
 
-        assertEquals(Optional.of(target), infrastructureProvisioner.getVersionToProvision(NodeType.config));
+        assertEquals(Optional.of(target), infrastructureProvisioner.getTargetVersion(NodeType.config));
     }
 
     @Test
@@ -59,7 +59,7 @@ public class InfrastructureProvisionerTest {
         addNode(2, Node.State.ready, Optional.empty());
         addNode(3, Node.State.active, Optional.of(target));
 
-        assertEquals(Optional.of(target), infrastructureProvisioner.getVersionToProvision(NodeType.config));
+        assertEquals(Optional.of(target), infrastructureProvisioner.getTargetVersion(NodeType.config));
     }
 
     @Test
@@ -74,7 +74,7 @@ public class InfrastructureProvisionerTest {
         addNode(4, Node.State.inactive, Optional.of(target));
         addNode(5, Node.State.dirty, Optional.empty());
 
-        assertEquals(Optional.empty(), infrastructureProvisioner.getVersionToProvision(NodeType.config));
+        assertEquals(Optional.empty(), infrastructureProvisioner.getTargetVersion(NodeType.config));
     }
 
     @Test
@@ -82,18 +82,18 @@ public class InfrastructureProvisionerTest {
         when(infrastructureVersions.getTargetVersionFor(eq(NodeType.config))).thenReturn(Optional.of(Version.fromString("6.123.456")));
 
         // No nodes in node repo
-        assertEquals(Optional.empty(), infrastructureProvisioner.getVersionToProvision(NodeType.config));
+        assertEquals(Optional.empty(), infrastructureProvisioner.getTargetVersion(NodeType.config));
 
         // Add nodes in non-provisionable states
         addNode(1, Node.State.dirty, Optional.empty());
         addNode(2, Node.State.failed, Optional.empty());
-        assertEquals(Optional.empty(), infrastructureProvisioner.getVersionToProvision(NodeType.config));
+        assertEquals(Optional.empty(), infrastructureProvisioner.getTargetVersion(NodeType.config));
     }
 
     @Test
     public void returns_empty_if_target_version_not_set() {
         when(infrastructureVersions.getTargetVersionFor(eq(NodeType.config))).thenReturn(Optional.empty());
-        assertEquals(Optional.empty(), infrastructureProvisioner.getVersionToProvision(NodeType.config));
+        assertEquals(Optional.empty(), infrastructureProvisioner.getTargetVersion(NodeType.config));
     }
 
     private Node addNode(int id, Node.State state, Optional<Version> wantedVespaVersion) {
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 5e58d0d89e7..581b82e5fd5 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
@@ -520,7 +520,7 @@ public class RestApiTest {
                         Request.Method.PATCH),
                 400,
                 "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Cannot downgrade version without setting 'force'. " +
-                        "Current wanted version: 6.123.456, attempted to set wanted version: 6.123.1\"}");
+                        "Current target version: 6.123.456, attempted to set target version: 6.123.1\"}");
 
         // Downgrade with force is OK
         assertResponse(new Request("http://localhost:8080/nodes/v2/upgrade/confighost",