Merge pull request #24598 from vespa-engine/hmusum/use-last-modified-time-for-deleting-unused-file-references

Remove unused file references based on last modified time

3 files changed, 28 insertions, 27 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index cf79005b1ab..cd61b44be1b 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -597,14 +597,15 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
     public List<String> deleteUnusedFileDistributionReferences(File fileReferencesPath,
                                                                Duration keepFileReferencesDuration,
                                                                int numberToAlwaysKeep) {
-        log.log(Level.FINE, () -> "Keep unused file references for " + keepFileReferencesDuration);
         if (!fileReferencesPath.isDirectory()) throw new RuntimeException(fileReferencesPath + " is not a directory");
 
         Set<String> fileReferencesInUse = getFileReferencesInUse();
         log.log(Level.FINE, () -> "File references in use : " + fileReferencesInUse);
+        Instant instant = clock.instant().minus(keepFileReferencesDuration);
+        log.log(Level.FINE, () -> "Remove unused file references last modified before " + instant +
+                " (but keep " + numberToAlwaysKeep + " of those)");
 
-        List<String> candidates = sortedUnusedFileReferences(fileReferencesPath, fileReferencesInUse, keepFileReferencesDuration);
-        // Do not delete the newest ones
+        List<String> candidates = sortedUnusedFileReferences(fileReferencesPath, fileReferencesInUse, instant);
         List<String> fileReferencesToDelete = candidates.subList(0, Math.max(0, candidates.size() - numberToAlwaysKeep));
         if (fileReferencesToDelete.size() > 0) {
             log.log(Level.FINE, () -> "Will delete file references not in use: " + fileReferencesToDelete);
@@ -628,15 +629,14 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         return fileReferencesInUse;
     }
 
-    private List<String> sortedUnusedFileReferences(File fileReferencesPath, Set<String> fileReferencesInUse, Duration keepFileReferences) {
+    private List<String> sortedUnusedFileReferences(File fileReferencesPath, Set<String> fileReferencesInUse, Instant instant) {
         Set<String> fileReferencesOnDisk = getFileReferencesOnDisk(fileReferencesPath);
         log.log(Level.FINE, () -> "File references on disk (in " + fileReferencesPath + "): " + fileReferencesOnDisk);
-        Instant instant = clock.instant().minus(keepFileReferences);
         return fileReferencesOnDisk
                 .stream()
                 .filter(fileReference -> ! fileReferencesInUse.contains(fileReference))
-                .filter(fileReference -> isLastFileAccessBefore(new File(fileReferencesPath, fileReference), instant))
-                .sorted(Comparator.comparing(a -> lastAccessed(new File(fileReferencesPath, a))))
+                .filter(fileReference -> isLastModifiedBefore(new File(fileReferencesPath, fileReference), instant))
+                .sorted(Comparator.comparing(a -> lastModified(new File(fileReferencesPath, a))))
                 .collect(Collectors.toList());
     }
 
@@ -690,15 +690,15 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
                 .collect(Collectors.toList());
     }
 
-    private boolean isLastFileAccessBefore(File fileReference, Instant instant) {
-        return lastAccessed(fileReference).isBefore(instant);
+    private boolean isLastModifiedBefore(File fileReference, Instant instant) {
+        return lastModified(fileReference).isBefore(instant);
     }
 
-    private Instant lastAccessed(File fileReference) {
+    private Instant lastModified(File fileReference) {
         BasicFileAttributes fileAttributes;
         try {
             fileAttributes = readAttributes(fileReference.toPath(), BasicFileAttributes.class);
-            return fileAttributes.lastAccessTime().toInstant();
+            return fileAttributes.lastModifiedTime().toInstant();
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/FileDistributionMaintainer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/FileDistributionMaintainer.java
index f6aee416c9c..0588a126b68 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/FileDistributionMaintainer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/FileDistributionMaintainer.java
@@ -20,7 +20,7 @@ import java.time.Duration;
  */
 public class FileDistributionMaintainer extends ConfigServerMaintainer {
 
-    private static final int numberToAlwaysKeep = 20;
+    private static final int numberToAlwaysKeep = 10; // TODO: Reduce to 0 / remove
 
     private final ApplicationRepository applicationRepository;
     private final File fileReferencesDir;
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
index b185bb5915d..e8d969119f9 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
@@ -61,7 +61,6 @@ import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
-import java.nio.file.attribute.FileTime;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
@@ -266,13 +265,13 @@ public class ApplicationRepositoryTest {
         Duration keepFileReferencesDuration = Duration.ofSeconds(4);
 
         // Add file reference that is not in use and should be deleted (older than 'keepFileReferencesDuration')
-        File filereferenceDirOldest = createFilereferenceOnDisk(new File(fileReferencesDir, "foo"));
+        File filereferenceDirOldest = createFileReferenceOnDisk(new File(fileReferencesDir, "bar"));
         clock.advance(Duration.ofSeconds(1));
 
         // Add file references that are not in use and could be deleted
         IntStream.range(0, 3).forEach(i -> {
             try {
-                createFilereferenceOnDisk(new File(fileReferencesDir, "bar" + i));
+                createFileReferenceOnDisk(new File(fileReferencesDir, "baz" + i));
             } catch (IOException e) {
                 fail(e.getMessage());
             }
@@ -281,7 +280,7 @@ public class ApplicationRepositoryTest {
         clock.advance(keepFileReferencesDuration);
 
         // Add file reference that is not in use, but should not be deleted (newer than 'keepFileReferencesDuration')
-        File filereferenceDirNewest = createFilereferenceOnDisk(new File(fileReferencesDir, "baz"));
+        File filereferenceDirNewest = createFileReferenceOnDisk(new File(fileReferencesDir, "foo"));
 
         applicationRepository = new ApplicationRepository.Builder()
                 .withTenantRepository(tenantRepository)
@@ -294,23 +293,25 @@ public class ApplicationRepositoryTest {
         PrepareParams prepareParams = new PrepareParams.Builder().applicationId(applicationId()).ignoreValidationErrors(true).build();
         deployApp(new File("src/test/apps/app"), prepareParams);
 
-        List<String> toBeDeleted = applicationRepository.deleteUnusedFileDistributionReferences(fileReferencesDir,
+        List<String> deleted = applicationRepository.deleteUnusedFileDistributionReferences(fileReferencesDir,
                                                                                                 keepFileReferencesDuration,
                                                                                                 2);
-        Collections.sort(toBeDeleted);
-        assertEquals(List.of("bar0", "foo"), toBeDeleted);
-        // bar0 and foo are the only ones that will be deleted (keeps 2 newest no matter how old they are)
+        Collections.sort(deleted);
+        List<String> expected = new ArrayList<>(List.of("bar", "baz0"));
+        Collections.sort(expected);
+        assertEquals(expected, deleted);
+        // bar and baz0 will be deleted, 2 of the old ones (baz1 and baz2) will be kept and foo is not old enough to be considered
         assertFalse(filereferenceDirOldest.exists());
-        assertFalse(new File(fileReferencesDir, "bar0").exists());
+        assertFalse(new File(fileReferencesDir, "baz0").exists());
         assertTrue(filereferenceDirNewest.exists());
     }
 
-    private File createFilereferenceOnDisk(File filereferenceDir) throws IOException {
-        assertTrue(filereferenceDir.mkdir());
-        File file = new File(filereferenceDir, "bar");
-        IOUtils.writeFile(file, Utf8.toBytes("test"));
-        Files.setAttribute(filereferenceDir.toPath(), "lastAccessTime", FileTime.from(clock.instant()));
-        return filereferenceDir;
+    private File createFileReferenceOnDisk(File filereference) throws IOException {
+        File fileReferenceDir = filereference.getParentFile();
+        fileReferenceDir.mkdir();
+        IOUtils.writeFile(filereference, Utf8.toBytes("test"));
+        filereference.setLastModified(clock.instant().toEpochMilli());
+        return filereference;
     }
 
     @Test