Handle coredumps when in dirty and when (re)starting container

5 files changed, 32 insertions, 19 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
index 3410963d48a..d16b09834c4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java
@@ -215,9 +215,11 @@ public class StorageMaintainer {
 
     /**
      * Checks if container has any new coredumps, reports and archives them if so
+     *
+     * @param force Set to true to bypass throttling
      */
-    public void handleCoreDumpsForContainer(ContainerName containerName, ContainerNodeSpec nodeSpec, Environment environment) {
-        if (! getMaintenanceThrottlerFor(containerName).shouldHandleCoredumpsNow()) return;
+    public void handleCoreDumpsForContainer(ContainerName containerName, ContainerNodeSpec nodeSpec, boolean force) {
+        if (! getMaintenanceThrottlerFor(containerName).shouldHandleCoredumpsNow() && !force) return;
 
         MaintainerExecutor maintainerExecutor = new MaintainerExecutor();
         addHandleCoredumpsCommand(maintainerExecutor, containerName, nodeSpec);
@@ -289,10 +291,13 @@ public class StorageMaintainer {
     }
 
     /**
-     * Archives container data, runs when container enters state "dirty"
+     * Prepares the container-storage for the next container by deleting/archiving all the data of the current container.
+     * Removes old files, reports coredumps and archives container data, runs when container enters state "dirty"
      */
-    public void archiveNodeData(ContainerName containerName) {
+    public void cleanupNodeStorage(ContainerName containerName, ContainerNodeSpec nodeSpec) {
         MaintainerExecutor maintainerExecutor = new MaintainerExecutor();
+        addRemoveOldFilesCommand(maintainerExecutor, containerName);
+        addHandleCoredumpsCommand(maintainerExecutor, containerName, nodeSpec);
         addArchiveNodeData(maintainerExecutor, containerName);
 
         maintainerExecutor.execute();
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 e1b99e5ce69..2e81ef19f5e 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
@@ -454,7 +454,7 @@ public class NodeAgentImpl implements NodeAgent {
             case active:
                 storageMaintainer.ifPresent(maintainer -> {
                     maintainer.removeOldFilesFromNode(containerName);
-                    maintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
+                    maintainer.handleCoreDumpsForContainer(containerName, nodeSpec, false);
                 });
                 scheduleDownLoadIfNeeded(nodeSpec);
                 if (isDownloadingImage()) {
@@ -463,6 +463,7 @@ public class NodeAgentImpl implements NodeAgent {
                 }
                 container = removeContainerIfNeededUpdateContainerState(nodeSpec, container);
                 if (! container.isPresent()) {
+                    storageMaintainer.ifPresent(maintainer -> maintainer.handleCoreDumpsForContainer(containerName, nodeSpec, false));
                     startContainer(nodeSpec);
                 }
 
@@ -490,10 +491,9 @@ public class NodeAgentImpl implements NodeAgent {
                 nodeRepository.markAsDirty(hostname);
                 break;
             case dirty:
-                storageMaintainer.ifPresent(maintainer -> maintainer.removeOldFilesFromNode(containerName));
                 removeContainerIfNeededUpdateContainerState(nodeSpec, container);
                 logger.info("State is " + nodeSpec.nodeState + ", will delete application storage and mark node as ready");
-                storageMaintainer.ifPresent(maintainer -> maintainer.archiveNodeData(containerName));
+                storageMaintainer.ifPresent(maintainer -> maintainer.cleanupNodeStorage(containerName, nodeSpec));
                 updateNodeRepoWithCurrentAttributes(nodeSpec);
                 nodeRepository.markNodeAvailableForNewAllocation(hostname);
                 break;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java
index a7ce706063a..ffb4105888b 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java
@@ -30,7 +30,7 @@ public class StorageMaintainerMock extends StorageMaintainer {
     }
 
     @Override
-    public void handleCoreDumpsForContainer(ContainerName containerName, ContainerNodeSpec nodeSpec, Environment environment) {
+    public void handleCoreDumpsForContainer(ContainerName containerName, ContainerNodeSpec nodeSpec, boolean force) {
     }
 
     @Override
@@ -42,7 +42,7 @@ public class StorageMaintainerMock extends StorageMaintainer {
     }
 
     @Override
-    public void archiveNodeData(ContainerName containerName) {
+    public void cleanupNodeStorage(ContainerName containerName, ContainerNodeSpec nodeSpec) {
         callOrderVerifier.add("DeleteContainerStorage with " + containerName);
     }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java
index 95c240b171a..69969336efc 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainerTest.java
@@ -77,7 +77,7 @@ public class StorageMaintainerTest {
         verifyProcessExecuterCalled(1);
 
         // Coredump handler has its own throttler
-        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
+        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, false);
         verifyProcessExecuterCalled(2);
 
 
@@ -85,21 +85,30 @@ public class StorageMaintainerTest {
         storageMaintainer.removeOldFilesFromNode(containerName);
         verifyProcessExecuterCalled(3);
 
-        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
+        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, false);
         verifyProcessExecuterCalled(4);
 
-        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
+        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, false);
         verifyProcessExecuterCalled(4);
 
-
-        // archiveNodeData is unthrottled and it should reset previous times
-        storageMaintainer.archiveNodeData(containerName);
+        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, true);
         verifyProcessExecuterCalled(5);
-        storageMaintainer.archiveNodeData(containerName);
+
+        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, true);
+        verifyProcessExecuterCalled(6);
+
+        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, false);
         verifyProcessExecuterCalled(6);
 
-        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, environment);
+
+        // cleanupNodeStorage is unthrottled and it should reset previous times
+        storageMaintainer.cleanupNodeStorage(containerName, nodeSpec);
         verifyProcessExecuterCalled(7);
+        storageMaintainer.cleanupNodeStorage(containerName, nodeSpec);
+        verifyProcessExecuterCalled(8);
+
+        storageMaintainer.handleCoreDumpsForContainer(containerName, nodeSpec, false);
+        verifyProcessExecuterCalled(9);
     }
 
     @Test
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 50e58bdaa6e..3ddb28eed77 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
@@ -367,9 +367,8 @@ public class NodeAgentImplTest {
         nodeAgent.converge();
 
         final InOrder inOrder = inOrder(storageMaintainer, dockerOperations, nodeRepository);
-        inOrder.verify(storageMaintainer, times(1)).removeOldFilesFromNode(eq(containerName));
         inOrder.verify(dockerOperations, times(1)).removeContainer(any());
-        inOrder.verify(storageMaintainer, times(1)).archiveNodeData(eq(containerName));
+        inOrder.verify(storageMaintainer, times(1)).cleanupNodeStorage(eq(containerName), eq(nodeSpec));
         inOrder.verify(nodeRepository, times(1)).markNodeAvailableForNewAllocation(eq(hostName));
 
         verify(dockerOperations, never()).startContainer(eq(containerName), any());