Skip to content

Commit

Permalink
[7.8] Fix negative limiting with fewer PARTIAL snapshots than minimum…
Browse files Browse the repository at this point in the history
… required (#58563) (#58570)

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
  • Loading branch information
dakrone committed Jun 25, 2020
1 parent 593e5c0 commit 9de9bf6
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
Expand Up @@ -186,7 +186,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
Expand Down
Expand Up @@ -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);
Expand All @@ -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);
Expand Down

0 comments on commit 9de9bf6

Please sign in to comment.