Reboot container on memory resources change

2 files changed, 16 insertions, 22 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 0e651383adc..407814f3b4d 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
@@ -213,7 +213,10 @@ public class NodeAgentImpl implements NodeAgent {
 
             shouldRestartServices(context, existingContainer.get()).ifPresent(restartReason -> {
                 context.log(logger, "Will restart services: " + restartReason);
-                restartServices(context, existingContainer.get());
+                orchestratorSuspendNode(context);
+
+                dockerOperations.restartVespa(context);
+                context.log(logger, "Restarted services");
                 currentRestartGeneration = context.node().wantedRestartGeneration();
             });
         }
@@ -223,7 +226,7 @@ public class NodeAgentImpl implements NodeAgent {
 
     private Optional<String> shouldRestartServices( NodeAgentContext context, Container existingContainer) {
         NodeSpec node = context.node();
-        if (node.wantedRestartGeneration().isEmpty()) return Optional.empty();
+        if (!existingContainer.state.isRunning() || node.state() != NodeState.active) return Optional.empty();
 
         // Restart generation is only optional because it does not exist for unallocated nodes
         if (currentRestartGeneration.get() < node.wantedRestartGeneration().get()) {
@@ -231,25 +234,9 @@ public class NodeAgentImpl implements NodeAgent {
                     + currentRestartGeneration.get() + " -> " + node.wantedRestartGeneration().get());
         }
 
-        // Restart services if wanted memory changes (searchnode and container needs to be restarted to pick up changes)
-        ContainerResources wantedContainerResources = getContainerResources(context);
-        if (!wantedContainerResources.equalsMemory(existingContainer.resources)) {
-            return Optional.of("Container should be running with different memory allocation, wanted: " +
-                                       wantedContainerResources.toStringMemory() + ", actual: " + existingContainer.resources.toStringMemory());
-        }
-
         return Optional.empty();
     }
 
-    private void restartServices(NodeAgentContext context, Container existingContainer) {
-        if (existingContainer.state.isRunning() && context.node().state() == NodeState.active) {
-            context.log(logger, "Restarting services");
-            // Since we are restarting the services we need to suspend the node.
-            orchestratorSuspendNode(context);
-            dockerOperations.restartVespa(context);
-        }
-    }
-
     private void stopServicesIfNeeded(NodeAgentContext context) {
         if (hasStartedServices && context.node().owner().isEmpty())
             stopServices(context);
@@ -305,6 +292,12 @@ public class NodeAgentImpl implements NodeAgent {
                     currentRebootGeneration, context.node().wantedRebootGeneration()));
         }
 
+        ContainerResources wantedContainerResources = getContainerResources(context);
+        if (!wantedContainerResources.equalsMemory(existingContainer.resources)) {
+            return Optional.of("Container should be running with different memory allocation, wanted: " +
+                    wantedContainerResources.toStringMemory() + ", actual: " + existingContainer.resources.toStringMemory());
+        }
+
         if (containerState == STARTING) return Optional.of("Container failed to start");
         return Optional.empty();
     }
@@ -343,7 +336,8 @@ public class NodeAgentImpl implements NodeAgent {
 
         orchestratorSuspendNode(context);
 
-        dockerOperations.updateContainer(context, wantedContainerResources);
+        // Only update CPU resources
+        dockerOperations.updateContainer(context, wantedContainerResources.withMemoryBytes(existingContainer.resources.memoryBytes()));
     }
 
     private ContainerResources getContainerResources(NodeAgentContext context) {
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 bf5e5a67a79..465a430b497 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
@@ -259,7 +259,7 @@ public class NodeAgentImplTest {
     }
 
     @Test
-    public void vespaIsRestartedIfMemoryChanged() {
+    public void containerIsRecreatedIfMemoryChanged() {
         NodeSpec.Builder specBuilder = nodeBuilder
                 .state(NodeState.active)
                 .wantedDockerImage(dockerImage).currentDockerImage(dockerImage)
@@ -280,9 +280,9 @@ public class NodeAgentImplTest {
 
         InOrder inOrder = inOrder(orchestrator, dockerOperations);
         inOrder.verify(orchestrator).resume(any(String.class));
-        inOrder.verify(dockerOperations, never()).removeContainer(any(), any());
+        inOrder.verify(dockerOperations).removeContainer(eq(secondContext), any());
         inOrder.verify(dockerOperations, never()).updateContainer(any(), any());
-        inOrder.verify(dockerOperations).restartVespa(eq(secondContext));
+        inOrder.verify(dockerOperations, never()).restartVespa(any());
 
         nodeAgent.doConverge(secondContext);
         inOrder.verify(orchestrator).resume(any(String.class));