From e106997b74e401212a41687315f53af666f2f176 Mon Sep 17 00:00:00 2001
From: Lee Hinman <lee@writequit.org>
Date: Thu, 25 Jun 2020 11:01:02 -0600
Subject: [PATCH] Fix negative limiting with fewer PARTIAL snapshots than
 minimum required

In SLM retention, when a minimum number of snapshots is required for retention, we prefer to remove
the oldest snapshots first. To perform this, we limit one of the streams, in a rare case this can
cause:

```
[mynode] error during snapshot retention task
java.lang.IllegalArgumentException: -5
	at java.util.stream.ReferencePipeline.limit(ReferencePipeline.java:469) ~[?:?]
	at org.elasticsearch.xpack.core.slm.SnapshotRetentionConfiguration.lambda$getSnapshotDeletionPredicate$6(SnapshotRetentionConfiguration.java:195) ~[?:?]
	at org.elasticsearch.xpack.slm.SnapshotRetentionTask.snapshotEligibleForDeletion(SnapshotRetentionTask.java:245) ~[?:?]
	at org.elasticsearch.xpack.slm.SnapshotRetentionTask$1.lambda$onResponse$0(SnapshotRetentionTask.java:163) ~[?:?]
	at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:176) ~[?:?]
	at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1624) ~[?:?]
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) ~[?:?]
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[?:?]
	at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913) ~[?:?]
```

When certain criteria are met. This commit fixes the negative limiting with `Math.max(0, ...)` and
adds a unit test for the behavior.

Resolves #58515
---
 .../core/slm/SnapshotRetentionConfiguration.java    |  2 +-
 .../slm/SnapshotRetentionConfigurationTests.java    | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfiguration.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfiguration.java
index cb21bc1c02d5d..a6265cecb44a1 100644
--- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfiguration.java
+++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotRetentionConfiguration.java
@@ -184,7 +184,7 @@ public Predicate<SnapshotInfo> getSnapshotDeletionPredicate(final List<SnapshotI
                 final TimeValue snapshotAge = new TimeValue(nowSupplier.getAsLong() - si.startTime());
 
                 if (this.minimumSnapshotCount != null) {
-                    final long eligibleForExpiration = successfulSnapshotCount - minimumSnapshotCount;
+                    final long eligibleForExpiration = Math.max(0, successfulSnapshotCount - minimumSnapshotCount);
 
                     // Only the oldest N snapshots are actually eligible, since if we went below this we
                     // would fall below the configured minimum number of snapshots to keep
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java
index fa79dd7d4eee6..c65e76108024f 100644
--- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java
+++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/slm/SnapshotRetentionConfigurationTests.java
@@ -227,7 +227,6 @@ private void assertUnsuccessfulNotCountedTowardsMinimum(boolean failure) {
         assertThat(conf.getSnapshotDeletionPredicate(infos).test(oldInfo), equalTo(true));
     }
 
-
     public void testMostRecentSuccessfulTimestampIsUsed() {
         boolean failureBeforePartial = randomBoolean();
         SnapshotRetentionConfiguration conf = new SnapshotRetentionConfiguration(() -> 1, null, 2, 2);
@@ -243,6 +242,18 @@ public void testMostRecentSuccessfulTimestampIsUsed() {
         assertThat(conf.getSnapshotDeletionPredicate(infos).test(s4), equalTo(false));
     }
 
+    public void testFewerSuccessesThanMinWithPartial() {
+        SnapshotRetentionConfiguration conf = new SnapshotRetentionConfiguration(() -> 1, TimeValue.timeValueSeconds(5), 10, 20);
+        SnapshotInfo s1 = makeInfo(1);
+        SnapshotInfo sP = makePartialInfo(2);
+        SnapshotInfo s2 = makeInfo(3);
+
+        List<SnapshotInfo> infos = Arrays.asList(s1 , sP, s2);
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(s1), equalTo(false));
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(sP), equalTo(false));
+        assertThat(conf.getSnapshotDeletionPredicate(infos).test(s2), equalTo(false));
+    }
+
     private SnapshotInfo makeInfo(long startTime) {
         final Map<String, Object> meta = new HashMap<>();
         meta.put(SnapshotLifecyclePolicy.POLICY_ID_METADATA_FIELD, REPO);