Merge pull request #9696 from vespa-engine/revert-9414-olaa/seeded-coredump-id

Revert "Merge pull request #9414"

2 files changed, 35 insertions, 36 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java
index 60f4d532784..95964ec8e7f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java
@@ -3,8 +3,6 @@ package com.yahoo.vespa.hosted.node.admin.maintenance.coredump;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableMap;
-import com.yahoo.component.Version;
-import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
 import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder;
 import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
@@ -48,6 +46,7 @@ public class CoredumpHandler {
     private final CoredumpReporter coredumpReporter;
     private final Path crashPatchInContainer;
     private final Path doneCoredumpsPath;
+    private final Supplier<String> coredumpIdSupplier;
 
     /**
      * @param crashPathInContainer path inside the container where core dump are dumped
@@ -55,17 +54,21 @@ public class CoredumpHandler {
      */
     public CoredumpHandler(Terminal terminal, CoreCollector coreCollector, CoredumpReporter coredumpReporter,
                            Path crashPathInContainer, Path doneCoredumpsPath) {
+        this(terminal, coreCollector, coredumpReporter, crashPathInContainer, doneCoredumpsPath, () -> UUID.randomUUID().toString());
+    }
+
+    CoredumpHandler(Terminal terminal, CoreCollector coreCollector, CoredumpReporter coredumpReporter,
+                           Path crashPathInContainer, Path doneCoredumpsPath, Supplier<String> coredumpIdSupplier) {
         this.terminal = terminal;
         this.coreCollector = coreCollector;
         this.coredumpReporter = coredumpReporter;
         this.crashPatchInContainer = crashPathInContainer;
         this.doneCoredumpsPath = doneCoredumpsPath;
+        this.coredumpIdSupplier = coredumpIdSupplier;
     }
 
 
     public void converge(NodeAgentContext context, Supplier<Map<String, Object>> nodeAttributesSupplier) {
-
-        UUID coredumpId = generateCoredumpId(context);
         Path containerCrashPathOnHost = context.pathOnHostFromPathInNode(crashPatchInContainer);
         Path containerProcessingPathOnHost = containerCrashPathOnHost.resolve(PROCESSING_DIRECTORY_NAME);
 
@@ -76,16 +79,16 @@ public class CoredumpHandler {
                 .deleteRecursively();
 
         // Check if we have already started to process a core dump or we can enqueue a new core one
-        getCoredumpToProcess(coredumpId, containerCrashPathOnHost, containerProcessingPathOnHost)
+        getCoredumpToProcess(containerCrashPathOnHost, containerProcessingPathOnHost)
                 .ifPresent(path -> processAndReportSingleCoredump(context, path, nodeAttributesSupplier));
     }
 
     /** @return path to directory inside processing directory that contains a core dump file to process */
-    Optional<Path> getCoredumpToProcess(UUID coredumpId, Path containerCrashPathOnHost, Path containerProcessingPathOnHost) {
+    Optional<Path> getCoredumpToProcess(Path containerCrashPathOnHost, Path containerProcessingPathOnHost) {
         return FileFinder.directories(containerProcessingPathOnHost).stream()
                 .map(FileFinder.FileAttributes::path)
                 .findAny()
-                .or(() -> enqueueCoredump(coredumpId, containerCrashPathOnHost, containerProcessingPathOnHost));
+                .or(() -> enqueueCoredump(containerCrashPathOnHost, containerProcessingPathOnHost));
     }
 
     /**
@@ -94,7 +97,7 @@ public class CoredumpHandler {
      *
      * @return path to directory inside processing directory which contains the enqueued core dump file
      */
-    Optional<Path> enqueueCoredump(UUID coredumpID, Path containerCrashPathOnHost, Path containerProcessingPathOnHost) {
+    Optional<Path> enqueueCoredump(Path containerCrashPathOnHost, Path containerProcessingPathOnHost) {
         return FileFinder.files(containerCrashPathOnHost)
                 .match(nameStartsWith(".").negate())
                 .maxDepth(1)
@@ -104,7 +107,7 @@ public class CoredumpHandler {
                 .map(coredumpPath -> {
                     UnixPath coredumpInProcessingDirectory = new UnixPath(
                             containerProcessingPathOnHost
-                                    .resolve(coredumpID.toString())
+                                    .resolve(coredumpIdSupplier.get())
                                     .resolve(COREDUMP_FILENAME_PREFIX + coredumpPath.getFileName()));
                     coredumpInProcessingDirectory.createParents();
                     return uncheck(() -> Files.move(coredumpPath, coredumpInProcessingDirectory.toPath())).getParent();
@@ -171,12 +174,4 @@ public class CoredumpHandler {
                 .orElseThrow(() -> new IllegalStateException(
                         "No coredump file found in processing directory " + coredumpProccessingDirectory));
     }
-
-    private UUID generateCoredumpId(NodeAgentContext context) {
-        NodeSpec nodeSpec = context.node();
-        String hostname = nodeSpec.getParentHostname().orElse(nodeSpec.getHostname());
-        String version = nodeSpec.getVespaVersion().orElse(Version.emptyVersion).toFullString();
-        byte[] seed = (hostname + version).getBytes();
-        return UUID.nameUUIDFromBytes(seed);
-    }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
index fa5ed169fbc..6d3a4dbb553 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java
@@ -24,7 +24,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
-import java.util.UUID;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
 import static com.yahoo.yolean.Exceptions.uncheck;
@@ -53,8 +53,9 @@ public class CoredumpHandlerTest {
     private final CoreCollector coreCollector = mock(CoreCollector.class);
     private final CoredumpReporter coredumpReporter = mock(CoredumpReporter.class);
     @SuppressWarnings("unchecked")
+    private final Supplier<String> coredumpIdSupplier = mock(Supplier.class);
     private final CoredumpHandler coredumpHandler = new CoredumpHandler(terminal, coreCollector, coredumpReporter,
-            crashPathInContainer, doneCoredumpsPath);
+            crashPathInContainer, doneCoredumpsPath, coredumpIdSupplier);
 
 
     @Test
@@ -66,9 +67,7 @@ public class CoredumpHandlerTest {
         Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve(".bash.core.431")), FileTime.from(Instant.now()));
 
         assertFolderContents(crashPathOnHost, ".bash.core.431");
-        UUID coredumpId = UUID.randomUUID();
-
-        Optional<Path> enqueuedPath = coredumpHandler.enqueueCoredump(coredumpId, crashPathOnHost, processingDir);
+        Optional<Path> enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir);
         assertEquals(Optional.empty(), enqueuedPath);
 
         // bash.core.431 finished writing... and 2 more have since been written
@@ -76,27 +75,30 @@ public class CoredumpHandlerTest {
         Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-proton.core.119")), FileTime.from(Instant.now().minus(Duration.ofMinutes(10))));
         Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-slobrok.core.673")), FileTime.from(Instant.now().minus(Duration.ofMinutes(5))));
 
-        enqueuedPath = coredumpHandler.enqueueCoredump(coredumpId, crashPathOnHost, processingDir);
-        assertEquals(Optional.of(processingDir.resolve(coredumpId.toString())), enqueuedPath);
+        when(coredumpIdSupplier.get()).thenReturn("id-123").thenReturn("id-321");
+        enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir);
+        assertEquals(Optional.of(processingDir.resolve("id-123")), enqueuedPath);
         assertFolderContents(crashPathOnHost, "bash.core.431", "vespa-slobrok.core.673");
-        assertFolderContents(processingDir, coredumpId.toString());
-        assertFolderContents(processingDir.resolve(coredumpId.toString()), "dump_vespa-proton.core.119");
+        assertFolderContents(processingDir, "id-123");
+        assertFolderContents(processingDir.resolve("id-123"), "dump_vespa-proton.core.119");
+        verify(coredumpIdSupplier, times(1)).get();
 
         // Enqueue another
-        enqueuedPath = coredumpHandler.enqueueCoredump(coredumpId, crashPathOnHost, processingDir);
-        assertEquals(Optional.of(processingDir.resolve(coredumpId.toString())), enqueuedPath);
+        enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir);
+        assertEquals(Optional.of(processingDir.resolve("id-321")), enqueuedPath);
         assertFolderContents(crashPathOnHost, "bash.core.431");
-        assertFolderContents(processingDir, coredumpId.toString());
-        assertFolderContents(processingDir.resolve(coredumpId.toString()), "dump_vespa-proton.core.119", "dump_vespa-slobrok.core.673");
+        assertFolderContents(processingDir, "id-123", "id-321");
+        assertFolderContents(processingDir.resolve("id-321"), "dump_vespa-slobrok.core.673");
+        verify(coredumpIdSupplier, times(2)).get();
     }
 
     @Test
     public void coredump_to_process_test() throws IOException {
         final Path crashPathOnHost = fileSystem.getPath("/home/docker/container-1/some/crash/path");
         final Path processingDir = fileSystem.getPath("/home/docker/container-1/some/other/processing");
-        UUID coredumpId = UUID.randomUUID();
+
         // Initially there are no core dumps
-        Optional<Path> enqueuedPath = coredumpHandler.enqueueCoredump(coredumpId, crashPathOnHost, processingDir);
+        Optional<Path> enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir);
         assertEquals(Optional.empty(), enqueuedPath);
 
         // 3 core dumps occur
@@ -105,12 +107,14 @@ public class CoredumpHandlerTest {
         Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-proton.core.119")), FileTime.from(Instant.now().minus(Duration.ofMinutes(10))));
         Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-slobrok.core.673")), FileTime.from(Instant.now().minus(Duration.ofMinutes(5))));
 
-        enqueuedPath = coredumpHandler.getCoredumpToProcess(coredumpId, crashPathOnHost, processingDir);
-        assertEquals(Optional.of(processingDir.resolve(coredumpId.toString())), enqueuedPath);
+        when(coredumpIdSupplier.get()).thenReturn("id-123");
+        enqueuedPath = coredumpHandler.getCoredumpToProcess(crashPathOnHost, processingDir);
+        assertEquals(Optional.of(processingDir.resolve("id-123")), enqueuedPath);
 
         // Running this again wont enqueue new core dumps as we are still processing the one enqueued previously
-        enqueuedPath = coredumpHandler.getCoredumpToProcess(coredumpId, crashPathOnHost, processingDir);
-        assertEquals(Optional.of(processingDir.resolve(coredumpId.toString())), enqueuedPath);
+        enqueuedPath = coredumpHandler.getCoredumpToProcess(crashPathOnHost, processingDir);
+        assertEquals(Optional.of(processingDir.resolve("id-123")), enqueuedPath);
+        verify(coredumpIdSupplier, times(1)).get();
     }
 
     @Test