Only update fields that have changed. Simplify NodeAgentImptTest.

3 files changed, 32 insertions, 82 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java
index 9a94231437d..91ff159ac41 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/configserver/noderepository/NodeAttributes.java
@@ -130,6 +130,6 @@ public class NodeAttributes {
                         wantToDeprovision.map(depr -> "wantToDeprovision=" + depr))
                 .filter(Optional::isPresent)
                 .map(Optional::get)
-                .collect(Collectors.joining(", ", "NodeAttributes{", "}"));
+                .collect(Collectors.joining(", ", "{", "}"));
     }
 }
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 ef137c55ffb..d85344dffd2 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
@@ -33,6 +33,7 @@ import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
@@ -236,26 +237,33 @@ public class NodeAgentImpl implements NodeAgent {
     }
 
     private void updateNodeRepoWithCurrentAttributes(final NodeSpec node) {
-        final NodeAttributes currentNodeAttributes = new NodeAttributes()
-                .withRestartGeneration(node.getCurrentRestartGeneration())
-                .withRebootGeneration(node.getCurrentRebootGeneration())
-                .withDockerImage(node.getCurrentDockerImage().orElse(new DockerImage("")));
-
-        final NodeAttributes wantedNodeAttributes = new NodeAttributes()
-                .withRestartGeneration(node.getWantedRestartGeneration())
-                // 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(node.getWantedRebootGeneration())
-                .withDockerImage(node.getWantedDockerImage().filter(n -> containerState == UNKNOWN).orElse(new DockerImage("")));
-
-        publishStateToNodeRepoIfChanged(currentNodeAttributes, wantedNodeAttributes);
+        final NodeAttributes currentNodeAttributes = new NodeAttributes();
+        final NodeAttributes newNodeAttributes = new NodeAttributes();
+
+        if (!Objects.equals(node.getCurrentRestartGeneration(), node.getWantedRestartGeneration())) {
+            currentNodeAttributes.withRestartGeneration(node.getCurrentRestartGeneration());
+            newNodeAttributes.withRestartGeneration(node.getWantedRestartGeneration());
+        }
+
+        if (!Objects.equals(node.getCurrentRebootGeneration(), node.getWantedRebootGeneration())) {
+            currentNodeAttributes.withRebootGeneration(node.getCurrentRebootGeneration());
+            newNodeAttributes.withRebootGeneration(node.getWantedRebootGeneration());
+        }
+
+        Optional<DockerImage> actualDockerImage = node.getWantedDockerImage().filter(n -> containerState == UNKNOWN);
+        if (!Objects.equals(node.getCurrentDockerImage(), actualDockerImage)) {
+            currentNodeAttributes.withDockerImage(node.getCurrentDockerImage().orElse(new DockerImage("")));
+            newNodeAttributes.withDockerImage(actualDockerImage.orElse(new DockerImage("")));
+        }
+
+        publishStateToNodeRepoIfChanged(currentNodeAttributes, newNodeAttributes);
     }
 
-    private void publishStateToNodeRepoIfChanged(NodeAttributes currentAttributes, NodeAttributes wantedAttributes) {
-        if (!currentAttributes.equals(wantedAttributes)) {
+    private void publishStateToNodeRepoIfChanged(NodeAttributes currentAttributes, NodeAttributes newAttributes) {
+        if (!currentAttributes.equals(newAttributes)) {
             context.log(logger, "Publishing new set of attributes to node repo: %s -> %s",
-                    currentAttributes, wantedAttributes);
-            nodeRepository.updateNodeAttributes(context.hostname().value(), wantedAttributes);
+                    currentAttributes, newAttributes);
+            nodeRepository.updateNodeAttributes(context.hostname().value(), newAttributes);
         }
     }
 
@@ -285,8 +293,8 @@ public class NodeAgentImpl implements NodeAgent {
     private Optional<String> shouldRestartServices(NodeSpec node) {
         if (!node.getWantedRestartGeneration().isPresent()) return Optional.empty();
 
-        if (!node.getCurrentRestartGeneration().isPresent() ||
-                node.getCurrentRestartGeneration().get() < node.getWantedRestartGeneration().get()) {
+        // Restart generation is only optional because it does not exist for unallocated nodes
+        if (node.getCurrentRestartGeneration().get() < node.getWantedRestartGeneration().get()) {
             return Optional.of("Restart requested - wanted restart generation has been bumped: "
                     + node.getCurrentRestartGeneration().get() + " -> " + node.getWantedRestartGeneration().get());
         }
@@ -466,7 +474,6 @@ public class NodeAgentImpl implements NodeAgent {
                 new IllegalStateException(String.format("Node '%s' missing from node repository", context.hostname())));
         expectNodeNotInNodeRepo = false;
 
-
         Optional<Container> container = getContainer();
         if (!node.equals(lastNode)) {
             // Every time the node spec changes, we should clear the metrics for this container as the dimensions
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 4e90c8f5a13..648d1f96094 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
@@ -87,17 +87,12 @@ public class NodeAgentImplTest {
 
     @Test
     public void upToDateContainerIsUntouched() {
-        final long restartGeneration = 1;
-        final long rebootGeneration = 0;
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .currentDockerImage(dockerImage)
                 .state(Node.State.active)
                 .wantedVespaVersion(vespaVersion)
                 .vespaVersion(vespaVersion)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
-                .wantedRebootGeneration(rebootGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
@@ -120,17 +115,12 @@ public class NodeAgentImplTest {
 
     @Test
     public void verifyRemoveOldFilesIfDiskFull() {
-        final long restartGeneration = 1;
-        final long rebootGeneration = 0;
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .currentDockerImage(dockerImage)
                 .state(Node.State.active)
                 .wantedVespaVersion(vespaVersion)
                 .vespaVersion(vespaVersion)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
-                .wantedRebootGeneration(rebootGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
@@ -181,14 +171,12 @@ public class NodeAgentImplTest {
     @Test
     public void absentContainerCausesStart() {
         final Optional<Long> restartGeneration = Optional.of(1L);
-        final long rebootGeneration = 0;
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .state(Node.State.active)
                 .wantedVespaVersion(vespaVersion)
                 .wantedRestartGeneration(restartGeneration.get())
                 .currentRestartGeneration(restartGeneration.get())
-                .wantedRebootGeneration(rebootGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(null, false);
@@ -210,26 +198,19 @@ public class NodeAgentImplTest {
         inOrder.verify(aclMaintainer, times(1)).converge();
         inOrder.verify(dockerOperations, times(1)).resumeNode(eq(context));
         inOrder.verify(nodeRepository).updateNodeAttributes(
-                hostName, new NodeAttributes()
-                        .withRestartGeneration(restartGeneration)
-                        .withRebootGeneration(rebootGeneration)
-                        .withDockerImage(dockerImage));
+                hostName, new NodeAttributes().withDockerImage(dockerImage));
         inOrder.verify(orchestrator).resume(hostName);
     }
 
     @Test
     public void containerIsNotStoppedIfNewImageMustBePulled() {
         final DockerImage newDockerImage = new DockerImage("new-image");
-        final long wantedRestartGeneration = 2;
-        final long currentRestartGeneration = 1;
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(newDockerImage)
                 .currentDockerImage(dockerImage)
                 .state(Node.State.active)
                 .wantedVespaVersion(vespaVersion)
                 .vespaVersion(vespaVersion)
-                .wantedRestartGeneration(wantedRestartGeneration)
-                .currentRestartGeneration(currentRestartGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
@@ -250,16 +231,12 @@ public class NodeAgentImplTest {
 
     @Test
     public void containerIsRestartedIfFlavorChanged() {
-        final long wantedRestartGeneration = 1;
-        final long currentRestartGeneration = 1;
         NodeSpec.Builder specBuilder = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .currentDockerImage(dockerImage)
                 .state(Node.State.active)
                 .wantedVespaVersion(vespaVersion)
-                .vespaVersion(vespaVersion)
-                .wantedRestartGeneration(wantedRestartGeneration)
-                .currentRestartGeneration(currentRestartGeneration);
+                .vespaVersion(vespaVersion);
 
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
         NodeSpec firstSpec = specBuilder.build();
@@ -316,15 +293,12 @@ public class NodeAgentImplTest {
 
     @Test
     public void failedNodeRunningContainerShouldStillBeRunning() {
-        final long restartGeneration = 1;
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .currentDockerImage(dockerImage)
                 .state(Node.State.failed)
                 .wantedVespaVersion(vespaVersion)
                 .vespaVersion(vespaVersion)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
@@ -340,13 +314,8 @@ public class NodeAgentImplTest {
 
     @Test
     public void readyNodeLeadsToNoAction() {
-        final long restartGeneration = 1;
-        final long rebootGeneration = 0;
         final NodeSpec node = nodeBuilder
                 .state(Node.State.ready)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
-                .wantedRebootGeneration(rebootGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(null,false);
@@ -368,18 +337,12 @@ public class NodeAgentImplTest {
 
     @Test
     public void inactiveNodeRunningContainerShouldStillBeRunning() {
-        final long restartGeneration = 1;
-        final long rebootGeneration = 0;
-
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .currentDockerImage(dockerImage)
                 .state(Node.State.inactive)
                 .wantedVespaVersion(vespaVersion)
                 .vespaVersion(vespaVersion)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
-                .wantedRebootGeneration(rebootGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
@@ -397,16 +360,10 @@ public class NodeAgentImplTest {
 
     @Test
     public void reservedNodeDoesNotUpdateNodeRepoWithVersion() {
-        final long restartGeneration = 1;
-        final long rebootGeneration = 0;
-
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .state(Node.State.reserved)
                 .wantedVespaVersion(vespaVersion)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
-                .wantedRebootGeneration(rebootGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(null, false);
@@ -450,10 +407,7 @@ public class NodeAgentImplTest {
         verify(orchestrator, never()).suspend(any(String.class));
         // current Docker image and vespa version should be cleared
         verify(nodeRepository, times(1)).updateNodeAttributes(
-                any(String.class), eq(new NodeAttributes()
-                        .withRestartGeneration(wantedRestartGeneration)
-                        .withRebootGeneration(0L)
-                        .withDockerImage(new DockerImage(""))));
+                eq(hostName), eq(new NodeAttributes().withDockerImage(new DockerImage(""))));
     }
 
     @Test
@@ -503,14 +457,11 @@ public class NodeAgentImplTest {
 
     @Test
     public void resumeProgramRunsUntilSuccess() {
-        final long restartGeneration = 1;
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .currentDockerImage(dockerImage)
                 .state(Node.State.active)
                 .vespaVersion(vespaVersion)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
@@ -569,15 +520,10 @@ public class NodeAgentImplTest {
 
     @Test
     public void start_container_subtask_failure_leads_to_container_restart() {
-        final long restartGeneration = 1;
-        final long rebootGeneration = 0;
         final NodeSpec node = nodeBuilder
                 .wantedDockerImage(dockerImage)
                 .state(Node.State.active)
                 .wantedVespaVersion(vespaVersion)
-                .wantedRestartGeneration(restartGeneration)
-                .currentRestartGeneration(restartGeneration)
-                .wantedRebootGeneration(rebootGeneration)
                 .build();
 
         NodeAgentImpl nodeAgent = spy(makeNodeAgent(null, false));
@@ -694,7 +640,6 @@ public class NodeAgentImplTest {
 
     @Test
     public void testRunningConfigServer() {
-        final long rebootGeneration = 0;
         final NodeSpec node = nodeBuilder
                 .nodeType(NodeType.config)
                 .wantedDockerImage(dockerImage)
@@ -720,9 +665,7 @@ public class NodeAgentImplTest {
         inOrder.verify(aclMaintainer, times(1)).converge();
         inOrder.verify(dockerOperations, times(1)).resumeNode(eq(context));
         inOrder.verify(nodeRepository).updateNodeAttributes(
-                hostName, new NodeAttributes()
-                        .withRebootGeneration(rebootGeneration)
-                        .withDockerImage(dockerImage));
+                hostName, new NodeAttributes().withDockerImage(dockerImage));
         inOrder.verify(orchestrator).resume(hostName);
     }