SLM: max/after and max/failed fix (#68352)

Combining both maximum and expire after retention configuration did not compose well in that, as long as there are any snapshots to delete due to the maximum criteria, the expire after criteria was not evaluated, fixed to consider both. When evaluating the maximum criteria, a number of snapshots to delete was calculated based on successful snapshots. However, when evaluating which snapshot is eligible for deletion, the entire list was used, resulting in deleting too few snapshots.
elastic · Feb 2, 2021 · baf15b5 · baf15b5
1 parent 1916fe4
commit baf15b5
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 5 deletions.
diff --git a/...n/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfiguration.java b/...n/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfiguration.java
@@ -28,6 +28,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.LongSupplier;
 import java.util.function.Predicate;
@@ -146,21 +147,26 @@ public Predicate<SnapshotInfo> getSnapshotDeletionPredicate(final List<SnapshotI
             // count) snapshots to be eligible for deletion
             if (this.maximumSnapshotCount != null) {
                 if (successfulSnapshotCount > this.maximumSnapshotCount) {
-                    final long snapsToDelete = successfulSnapshotCount - this.maximumSnapshotCount;
+                    final long successfulSnapsToDelete = successfulSnapshotCount - this.maximumSnapshotCount;
+                    final Optional<SnapshotInfo> firstNonEligible = sortedSnapshots.stream()
+                        .filter(snap -> SnapshotState.SUCCESS.equals(snap.state()))
+                        .skip(successfulSnapsToDelete)
+                        .findFirst();
+                    assert firstNonEligible.isPresent();
+                    final int snapsToDelete = sortedSnapshots.indexOf(firstNonEligible.get());
                     final boolean eligible = sortedSnapshots.stream()
                         .limit(snapsToDelete)
                         .anyMatch(s -> s.equals(si));
 
                     if (eligible) {
                         logger.trace("[{}]: ELIGIBLE as it is one of the {} oldest snapshots with " +
                                 "{} non-failed snapshots ({} total), over the limit of {} maximum snapshots",
-                            snapName, snapsToDelete, successfulSnapshotCount, totalSnapshotCount, this.maximumSnapshotCount);
+                            snapName, successfulSnapsToDelete, successfulSnapshotCount, totalSnapshotCount, this.maximumSnapshotCount);
                         return true;
                     } else {
-                        logger.trace("[{}]: INELIGIBLE as it is not one of the {} oldest snapshots with " +
+                        logger.trace("[{}]: SKIPPING as it is not one of the {} oldest snapshots with " +
                                 "{} non-failed snapshots ({} total), over the limit of {} maximum snapshots",
-                            snapName, snapsToDelete, successfulSnapshotCount, totalSnapshotCount, this.maximumSnapshotCount);
-                        return false;
+                            snapName, successfulSnapsToDelete, successfulSnapshotCount, totalSnapshotCount, this.maximumSnapshotCount);
                     }
                 }
             }

diff --git a/...e/src/test/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfigurationTests.java b/...e/src/test/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfigurationTests.java
@@ -101,6 +101,34 @@ public void testMaximum() {
         assertThat(conf.getSnapshotDeletionPredicate(infos).test(s9), equalTo(false));
     }
 
+    public void testMaximumWithExpireAfter() {
+        SnapshotRetentionConfiguration conf = new SnapshotRetentionConfiguration(
+            () -> TimeValue.timeValueDays(1).millis() + 2,
+            TimeValue.timeValueDays(1), null, 2);
+        SnapshotInfo old1 = makeInfo(0);
+        SnapshotInfo old2 = makeInfo(1);
+        SnapshotInfo new1 = makeInfo(2);
+
+        List<SnapshotInfo> infos = Arrays.asList(old1, old2 , new1);
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(old1), equalTo(true));
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(old2), equalTo(true));
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(new1), equalTo(false));
+    }
+
+    public void testMaximumWithFailedOrPartial() {
+        SnapshotRetentionConfiguration conf = new SnapshotRetentionConfiguration(() -> 1, null, null, 1);
+        SnapshotInfo s1 = makeInfo(1);
+        SnapshotInfo s2 = makeFailureOrPartial(2, randomBoolean());
+        SnapshotInfo s3 = makeInfo(3);
+        SnapshotInfo s4 = makeInfo(4);
+
+        List<SnapshotInfo> infos = Arrays.asList(s1 , s2, s3, s4);
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(s1), equalTo(true));
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(s2), equalTo(true));
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(s3), equalTo(true));
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(s4), equalTo(false));
+    }
+
     public void testFailuresDeletedIfExpired() {
         assertUnsuccessfulDeletedIfExpired(true);
     }