fix(cleanup): remove 2nd check on thresholdDays in old pipeline clean…

…up (#2948)

* fix(pipeline cleanup): remove 2nd check on thresholdDays

Fixes spinnaker/spinnaker#4467

OldPipelineCleanupPollingNotificationAgent is checking the age of the
pipeline execution twice before removing it:

1. the `Observable<Execution>` returned from
`executionRepository.retrievePipelinesForApplication(app)` is filtered
to only executions that have a completed status and where startTime (or
buildTime, if startTime is not set) is before `now - thresholdDays days`

2. After converting the `Observable<Execution>` to a `List`, sorting it,
and retaining `minimumPipelineExecutions` elements, each execution is
only deleted if `days between startTime and now > thresholdDays`.

The first check is true for executions as soon as they are a moment
older than `executionDays` days, but the second requires executions to be
older than `executionDays + 1` days (due to using `>`).

Since the second check seems redundant, I've simplify removed it.

My assumption here is that it is a bug that a configuration setting like
`thresholdDays: N` actually only removes executions older than `N+1`
days.

* refactor(pipeline cleanup): failing test to demonstrate #4467

Refactor the existing test to demonstrate spinnaker/spinnaker#4467

The existing test logic sets up 7 Executions that are all one day apart
(D1, D2, D3, D4, D5, D6, D7), and then sets the clock to be at D10. This
masks the bug where only executions older than `thresholdDays + 1` days
are deleted, since every execution is older than `thresholdDays + 1`
after removing the most recent `minimumPipelineExecutions` from the
candidates to be removed.

orca-core/src/main/java/com/netflix/spinnaker/orca/notifications/scheduling/OldPipelineCleanupPollingNotificationAgent.java

@@ -28,7 +28,11 @@
 import java.time.Clock;
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
 import java.util.stream.Collectors;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -179,15 +183,9 @@ private void cleanup(List<PipelineExecutionDetails> executions) {
         .subList(0, (executions.size() - minimumPipelineExecutions))
         .forEach(
             p -> {
-              long startTime = p.startTime == null ? p.buildTime : p.startTime;
-              long days =
-                  ChronoUnit.DAYS.between(
-                      Instant.ofEpochMilli(startTime), Instant.ofEpochMilli(clock.millis()));
-              if (days > thresholdDays) {
-                log.info("Deleting pipeline execution " + p.id + ": " + p.toString());
-                registry.counter(deletedId.withTag("application", p.application)).increment();
-                executionRepository.delete(PIPELINE, p.id);
-              }
+              log.info("Deleting pipeline execution " + p.id + ": " + p.toString());
+              executionRepository.delete(PIPELINE, p.id);
+              registry.counter(deletedId.withTag("application", p.application)).increment();
             });
   }
 

orca-core/src/test/groovy/com/netflix/spinnaker/orca/notifications/scheduling/OldPipelineCleanupPollingNotificationAgentSpec.groovy

@@ -25,7 +25,7 @@ import spock.lang.Specification
 
 import java.time.Clock
 import java.time.Duration
-import java.util.concurrent.atomic.AtomicInteger
+import java.time.Instant
 
 import static com.netflix.spinnaker.orca.pipeline.model.Execution.ExecutionType.PIPELINE
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
@@ -98,12 +98,25 @@ class OldPipelineCleanupPollingNotificationAgentSpec extends Specification {
 
   void 'tick should cleanup pipeline with executions older than threshold, but no less than minimum execution limit'() {
     given:
-    def startTime = new AtomicInteger(0)
-    def pipelines = buildPipelines(startTime, 7, "P1")
+    def startTimes = [
+            Instant.parse("2019-06-01T00:00:00Z"),
+            Instant.parse("2019-06-01T01:00:00Z"),
+            Instant.parse("2019-06-01T02:00:00Z"),
+            Instant.parse("2019-06-01T03:00:00Z"),
+            Instant.parse("2019-06-01T04:00:00Z"),
+            Instant.parse("2019-06-01T05:00:00Z"),
+            Instant.parse("2019-06-01T06:00:00Z"),
+    ]
+
+    def pipelines = buildPipelines(startTimes, "P1")
+    def thresholdDays = 5
+    def retain = 3
 
     and:
     def clock = Mock(Clock) {
-      millis() >> { Duration.ofDays(10).toMillis() }
+      // `thresholdDays` days and one minute after pipeline4 above, so that the first five pipelines
+      // are past the threshold
+      millis() >> { Instant.parse("2019-06-06T04:01:00Z").toEpochMilli() }
     }
     def executionRepository = Mock(ExecutionRepository) {
       1 * retrieveAllApplicationNames(PIPELINE) >> ["orca"]
@@ -115,27 +128,30 @@ class OldPipelineCleanupPollingNotificationAgentSpec extends Specification {
       clock,
       new NoopRegistry(),
       5000,
-      5,
-      3
+      thresholdDays,
+      retain
     )
 
     when:
     agent.tick()
 
     then:
+    // with pipeline executions at D1, D2, D3, D4, D5, D6, D7, and clock at D10, we
+    // expect D1-5 to be too old, but for the most recent 3 to be retained
     1 * executionRepository.delete(PIPELINE, '1')
     1 * executionRepository.delete(PIPELINE, '2')
   }
 
   private
-  static Collection<Execution> buildPipelines(AtomicInteger stageStartTime, int count, String configId) {
-    (1..count).collect {
-      def time = stageStartTime.incrementAndGet()
+  static Collection<Execution> buildPipelines(List<Instant> startTimes, String configId) {
+    (1..startTimes.size()).collect {
+      def n = it
+      def time = startTimes.get(n - 1).toEpochMilli()
       pipeline {
-        id = time as String
+        id = n
         application = "orca"
         pipelineConfigId = configId
-        startTime = Duration.ofDays(time).toMillis()
+        startTime = time
         buildTime = time
         status = ExecutionStatus.SUCCEEDED
         stage {