Merge pull request #5158 from vespa-engine/freva/check-if-update-is-needed-against-current-node-spec

Check if node-repo update is needed against current NodeSpec

2 files changed, 17 insertions, 43 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 7b0ba1cae58..fcbe4e15213 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
@@ -105,8 +105,6 @@ public class NodeAgentImpl implements NodeAgent {
 
     private ContainerState containerState = UNKNOWN;
 
-    // The attributes of the last successful node repo attribute update for this node. Used to avoid redundant calls.
-    private NodeAttributes lastAttributesSet = null;
     private ContainerNodeSpec lastNodeSpec = null;
     private CpuUsageReporter lastCpuMetric = new CpuUsageReporter();
 
@@ -230,7 +228,13 @@ public class NodeAgentImpl implements NodeAgent {
     }
 
     private void updateNodeRepoWithCurrentAttributes(final ContainerNodeSpec nodeSpec) {
-        final NodeAttributes nodeAttributes = new NodeAttributes()
+        final NodeAttributes currentNodeAttributes = new NodeAttributes()
+                .withRestartGeneration(nodeSpec.currentRestartGeneration.orElse(null))
+                .withRebootGeneration(nodeSpec.currentRebootGeneration.orElse(0L))
+                .withDockerImage(nodeSpec.currentDockerImage.orElse(new DockerImage("")))
+                .withVespaVersion(nodeSpec.vespaVersion.orElse(""));
+
+        final NodeAttributes wantedNodeAttributes = 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
@@ -238,18 +242,16 @@ public class NodeAgentImpl implements NodeAgent {
                 .withDockerImage(nodeSpec.wantedDockerImage.filter(node -> containerState != ABSENT).orElse(new DockerImage("")))
                 .withVespaVersion(nodeSpec.wantedVespaVersion.filter(node -> containerState != ABSENT).orElse(""));
 
-        publishStateToNodeRepoIfChanged(nodeAttributes);
+        publishStateToNodeRepoIfChanged(currentNodeAttributes, wantedNodeAttributes);
     }
 
-    private void publishStateToNodeRepoIfChanged(NodeAttributes currentAttributes) {
-        // TODO: We should only update if the new current values do not match the node repo's current values
-        if (!currentAttributes.equals(lastAttributesSet)) {
+    private void publishStateToNodeRepoIfChanged(NodeAttributes currentAttributes, NodeAttributes wantedAttributes) {
+        if (!currentAttributes.equals(wantedAttributes)) {
             logger.info("Publishing new set of attributes to node repo: "
-                    + lastAttributesSet + " -> " + currentAttributes);
+                    + currentAttributes + " -> " + wantedAttributes);
             addDebugMessage("Publishing new set of attributes to node repo: {" +
-                    lastAttributesSet + "} -> {" + currentAttributes + "}");
-            nodeRepository.updateNodeAttributes(hostname, currentAttributes);
-            lastAttributesSet = currentAttributes;
+                    currentAttributes + "} -> {" + wantedAttributes + "}");
+            nodeRepository.updateNodeAttributes(hostname, wantedAttributes);
         }
     }
 
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 693a919ff12..0b9564fad8c 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
@@ -124,14 +124,6 @@ public class NodeAgentImplTest {
         final InOrder inOrder = inOrder(dockerOperations, orchestrator, nodeRepository);
         // TODO: Verify this isn't run unless 1st time
         inOrder.verify(dockerOperations, times(1)).resumeNode(eq(containerName));
-        // TODO: This should not happen when nothing is changed. Now it happens 1st time through.
-        inOrder.verify(nodeRepository).updateNodeAttributes(
-                hostName,
-                new NodeAttributes()
-                        .withRestartGeneration(restartGeneration)
-                        .withRebootGeneration(rebootGeneration)
-                        .withDockerImage(dockerImage)
-                        .withVespaVersion(vespaVersion));
         inOrder.verify(orchestrator).resume(hostName);
     }
 
@@ -319,12 +311,7 @@ public class NodeAgentImplTest {
 
         verify(dockerOperations, never()).removeContainer(any(), any());
         verify(orchestrator, never()).resume(any(String.class));
-        verify(nodeRepository).updateNodeAttributes(
-                hostName, new NodeAttributes()
-                        .withRestartGeneration(restartGeneration)
-                        .withRebootGeneration(rebootGeneration)
-                        .withDockerImage(dockerImage)
-                        .withVespaVersion(vespaVersion));
+        verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
     }
 
     @Test
@@ -352,12 +339,7 @@ public class NodeAgentImplTest {
         verify(dockerOperations, never()).createContainer(eq(containerName), eq(nodeSpec));
         verify(dockerOperations, never()).startContainer(eq(containerName), eq(nodeSpec));
         verify(orchestrator, never()).resume(any(String.class));
-        verify(nodeRepository).updateNodeAttributes(
-                hostName, new NodeAttributes()
-                        .withRestartGeneration(restartGeneration)
-                        .withRebootGeneration(rebootGeneration)
-                        .withDockerImage(new DockerImage(""))
-                        .withVespaVersion(""));
+        verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
     }
 
     @Test
@@ -386,12 +368,7 @@ public class NodeAgentImplTest {
         inOrder.verify(dockerOperations, never()).removeContainer(any(), any());
 
         verify(orchestrator, never()).resume(any(String.class));
-        verify(nodeRepository).updateNodeAttributes(
-                hostName, new NodeAttributes()
-                        .withRestartGeneration(restartGeneration)
-                        .withRebootGeneration(rebootGeneration)
-                        .withDockerImage(dockerImage)
-                        .withVespaVersion(vespaVersion));
+        verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
     }
 
     @Test
@@ -414,12 +391,7 @@ public class NodeAgentImplTest {
 
         nodeAgent.converge();
 
-        verify(nodeRepository).updateNodeAttributes(
-                hostName, new NodeAttributes()
-                        .withRestartGeneration(restartGeneration)
-                        .withRebootGeneration(rebootGeneration)
-                        .withDockerImage(new DockerImage(""))
-                        .withVespaVersion(""));
+        verify(nodeRepository, never()).updateNodeAttributes(eq(hostName), any());
     }
 
     private void nodeRunningContainerIsTakenDownAndCleanedAndRecycled(Node.State nodeState, Optional<Long> wantedRestartGeneration) {