Do not update version unless container is running

4 files changed, 39 insertions, 19 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
index b160b4ac364..8e67cac5b2f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
@@ -199,25 +199,14 @@ public class NodeAgentImpl implements NodeAgent {
         logger.info("Resume command successfully executed, new containerState is " + containerState);
     }
 
-    private void updateNodeRepoAndMarkNodeAsReady(ContainerNodeSpec nodeSpec) {
-        publishStateToNodeRepoIfChanged(
-                // Clear current Docker image and vespa version, as nothing is running on this node
-                new NodeAttributes()
-                        .withRestartGeneration(nodeSpec.wantedRestartGeneration.orElse(null))
-                        .withRebootGeneration(nodeSpec.wantedRebootGeneration.orElse(0L))
-                        .withDockerImage(new DockerImage(""))
-                        .withVespaVersion(""));
-        nodeRepository.markNodeAvailableForNewAllocation(nodeSpec.hostname);
-    }
-
     private void updateNodeRepoWithCurrentAttributes(final ContainerNodeSpec nodeSpec) {
         final NodeAttributes nodeAttributes = new NodeAttributes()
                 .withRestartGeneration(nodeSpec.wantedRestartGeneration.orElse(null))
                 // update reboot gen with wanted gen if set, we ignore reboot for Docker nodes but
                 // want the two to be equal in node repo
                 .withRebootGeneration(nodeSpec.wantedRebootGeneration.orElse(0L))
-                .withDockerImage(nodeSpec.wantedDockerImage.orElse(new DockerImage("")))
-                .withVespaVersion(nodeSpec.wantedVespaVersion.orElse(""));
+                .withDockerImage(nodeSpec.wantedDockerImage.filter(node -> containerState != ABSENT).orElse(new DockerImage("")))
+                .withVespaVersion(nodeSpec.wantedVespaVersion.filter(node -> containerState != ABSENT).orElse(""));
 
         publishStateToNodeRepoIfChanged(nodeAttributes);
     }
@@ -467,7 +456,8 @@ public class NodeAgentImpl implements NodeAgent {
                 removeContainerIfNeededUpdateContainerState(nodeSpec, container);
                 logger.info("State is " + nodeSpec.nodeState + ", will delete application storage and mark node as ready");
                 storageMaintainer.ifPresent(maintainer -> maintainer.archiveNodeData(containerName));
-                updateNodeRepoAndMarkNodeAsReady(nodeSpec);
+                updateNodeRepoWithCurrentAttributes(nodeSpec);
+                nodeRepository.markNodeAvailableForNewAllocation(hostname);
                 break;
             default:
                 throw new RuntimeException("UNKNOWN STATE " + nodeSpec.nodeState.name());
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/MultiDockerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/MultiDockerTest.java
index ba3fe71637d..c06febe3f17 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/MultiDockerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/MultiDockerTest.java
@@ -54,10 +54,10 @@ public class MultiDockerTest {
                                                              "DeleteContainerStorage with ContainerName { name=host2 }");
 
             callOrderVerifier.assertInOrder(
-                    "updateNodeAttributes with HostName: host1.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image1, vespaVersion=''}",
-                    "updateNodeAttributes with HostName: host2.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image2, vespaVersion=''}",
+                    "updateNodeAttributes with HostName: host1.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image1, vespaVersion='1.2.3'}",
+                    "updateNodeAttributes with HostName: host2.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image2, vespaVersion='1.2.3'}",
                     "markNodeAvailableForNewAllocation with HostName: host2.test.yahoo.com",
-                    "updateNodeAttributes with HostName: host3.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image1, vespaVersion=''}");
+                    "updateNodeAttributes with HostName: host3.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image1, vespaVersion='1.2.3'}");
         }
     }
 
@@ -65,6 +65,7 @@ public class MultiDockerTest {
         ContainerNodeSpec containerNodeSpec = new ContainerNodeSpec.Builder()
                 .hostname(hostName)
                 .wantedDockerImage(dockerImage)
+                .wantedVespaVersion("1.2.3")
                 .nodeState(Node.State.active)
                 .nodeType("tenant")
                 .nodeFlavor("docker")
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RestartTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RestartTest.java
index d7fd753fabe..a0024fae3dc 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RestartTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RestartTest.java
@@ -32,7 +32,7 @@ public class RestartTest {
             CallOrderVerifier callOrderVerifier = dockerTester.getCallOrderVerifier();
             // Check that the container is started and NodeRepo has received the PATCH update
             callOrderVerifier.assertInOrder("createContainerCommand with DockerImage { imageId=image:1.2.3 }, HostName: host1.test.yahoo.com, ContainerName { name=host1 }",
-                                            "updateNodeAttributes with HostName: host1.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image:1.2.3, vespaVersion=''}");
+                                            "updateNodeAttributes with HostName: host1.test.yahoo.com, NodeAttributes{restartGeneration=1, rebootGeneration=0, dockerImage=image:1.2.3, vespaVersion='1.2.3'}");
 
             wantedRestartGeneration = 2;
             currentRestartGeneration = 1;
@@ -46,8 +46,9 @@ public class RestartTest {
     private ContainerNodeSpec createContainerNodeSpec(long wantedRestartGeneration, long currentRestartGeneration) {
         return new ContainerNodeSpec.Builder()
                 .hostname("host1.test.yahoo.com")
-                .wantedDockerImage(new DockerImage("image:1.2.3"))
                 .nodeState(Node.State.active)
+                .wantedDockerImage(new DockerImage("image:1.2.3"))
+                .wantedVespaVersion("1.2.3")
                 .nodeType("tenant")
                 .nodeFlavor("docker")
                 .wantedRestartGeneration(wantedRestartGeneration)
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
index ef49b6878b6..83a00241919 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
@@ -316,6 +316,34 @@ public class NodeAgentImplTest {
                         .withVespaVersion(vespaVersion));
     }
 
+    @Test
+    public void reservedNodeDoesNotUpdateNodeRepoWithVersion() throws Exception {
+        final long restartGeneration = 1;
+        final long rebootGeneration = 0;
+
+        final ContainerNodeSpec nodeSpec = nodeSpecBuilder
+                .wantedDockerImage(dockerImage)
+                .nodeState(Node.State.reserved)
+                .wantedVespaVersion(vespaVersion)
+                .wantedRestartGeneration(restartGeneration)
+                .currentRestartGeneration(restartGeneration)
+                .wantedRebootGeneration(rebootGeneration)
+                .build();
+
+        NodeAgentImpl nodeAgent = makeNodeAgent(null, false);
+
+        when(nodeRepository.getContainerNodeSpec(hostName)).thenReturn(Optional.of(nodeSpec));
+
+        nodeAgent.converge();
+
+        verify(nodeRepository).updateNodeAttributes(
+                hostName, new NodeAttributes()
+                        .withRestartGeneration(restartGeneration)
+                        .withRebootGeneration(rebootGeneration)
+                        .withDockerImage(new DockerImage(""))
+                        .withVespaVersion(""));
+    }
+
     private void nodeRunningContainerIsTakenDownAndCleanedAndRecycled(Node.State nodeState, Optional<Long> wantedRestartGeneration) {
         wantedRestartGeneration.ifPresent(restartGeneration -> nodeSpecBuilder
                 .wantedRestartGeneration(restartGeneration)