fix(jenkins): Don't kick off multiples of the same build

igor-core/src/main/java/com/netflix/spinnaker/igor/PendingOperationsCache.java

@@ -0,0 +1,149 @@
+/*
+ * Copyright 2020 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.netflix.spinnaker.igor;
+
+import com.netflix.spinnaker.kork.annotations.VisibleForTesting;
+import com.netflix.spinnaker.kork.jedis.RedisClientDelegate;
+import java.util.*;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+import redis.clients.jedis.commands.JedisCommands;
+
+/**
+ * Shared cache of pending operations (e.g. builds being kicked off)
+ *
+ * <p>The idea here is that when an operation takes a long time (e.g. talking to jenkins to kick off
+ * a build) this prolonged time can cause orca to try to resubmit the request multiple times kicking
+ * off multiple builds.
+ *
+ * <p>We solve this, but setting a flag that a given request is being processed so if an identical
+ * request comes in the controller can detect that. When the request is completed and it's status is
+ * queried (by orca) the flag is cleared
+ */
+@Service
+public class PendingOperationsCache {
+
+  private static final String ID = "pending_operation";
+
+  /**
+   * This is the TTL in for the key in redis, we want this to be larger than the possible time it
+   * takes an async operation
+   */
+  private static final int TTL_SECONDS = 600;
+
+  private final RedisClientDelegate redisClientDelegate;
+  private final IgorConfigurationProperties igorConfigurationProperties;
+
+  @Autowired
+  public PendingOperationsCache(
+      RedisClientDelegate redisClientDelegate,
+      IgorConfigurationProperties igorConfigurationProperties) {
+    this.redisClientDelegate = redisClientDelegate;
+    this.igorConfigurationProperties = igorConfigurationProperties;
+  }
+
+  public void setOperationStatus(String operationKey, OperationStatus status, String value) {
+    String key = makeKey(operationKey);
+
+    redisClientDelegate.withCommandsClient(
+        c -> {
+          setStatusValue(c, key, status, value);
+        });
+  }
+
+  /**
+   * Returns the current status of a given operation key and sets the status if the key doesn't
+   * exist
+   *
+   * @param operationKey key for the operation
+   * @param status status to set to
+   * @param value value to set
+   * @return
+   */
+  public OperationState getAndSetOperationStatus(
+      String operationKey, OperationStatus status, String value) {
+    String key = makeKey(operationKey);
+
+    return redisClientDelegate.withCommandsClient(
+        c -> {
+          OperationState currentState = new OperationState();
+
+          if (c.exists(key)) {
+            currentState.load(c.get(key));
+          }
+
+          if (currentState.status == OperationStatus.UNKNOWN) {
+            setStatusValue(c, key, status, value);
+          }
+
+          return currentState;
+        });
+  }
+
+  public void clear(String operationKey) {
+    String key = makeKey(operationKey);
+
+    redisClientDelegate.withCommandsClient(
+        c -> {
+          c.del(key);
+        });
+  }
+
+  private void setStatusValue(
+      JedisCommands jedis, String key, OperationStatus status, String value) {
+    jedis.set(key, status.toString() + ":" + value);
+    jedis.expire(key, TTL_SECONDS);
+  }
+
+  protected String makeKey(String key) {
+    return igorConfigurationProperties.getSpinnaker().getJedis().getPrefix() + ":" + ID + ":" + key;
+  }
+
+  public enum OperationStatus {
+    PENDING,
+    COMPLETED,
+    UNKNOWN
+  }
+
+  public static class OperationState {
+    private OperationStatus status;
+    private String value;
+
+    public OperationState() {
+      status = OperationStatus.UNKNOWN;
+    }
+
+    public OperationState(OperationStatus status) {
+      this.status = status;
+    }
+
+    @VisibleForTesting
+    public void load(String redisValue) {
+      int splitPoint = redisValue.indexOf(":");
+
+      status = OperationStatus.valueOf(redisValue.substring(0, splitPoint));
+      value = redisValue.substring(splitPoint + 1);
+    }
+
+    public OperationStatus getStatus() {
+      return status;
+    }
+
+    public String getValue() {
+      return value;
+    }
+  }
+}

igor-web/src/main/groovy/com/netflix/spinnaker/igor/build/BuildController.groovy

@@ -17,6 +17,8 @@
 
 package com.netflix.spinnaker.igor.build
 
+import com.google.common.base.Strings
+import com.netflix.spinnaker.igor.PendingOperationsCache
 import com.netflix.spinnaker.igor.artifacts.ArtifactExtractor
 import com.netflix.spinnaker.igor.build.model.GenericBuild
 import com.netflix.spinnaker.igor.exceptions.BuildJobError
@@ -30,8 +32,10 @@ import com.netflix.spinnaker.igor.service.BuildServices
 import com.netflix.spinnaker.kork.artifacts.model.Artifact
 import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
+import com.netflix.spinnaker.security.AuthenticatedRequest
 import groovy.transform.InheritConstructors
 import groovy.util.logging.Slf4j
+import org.springframework.http.ResponseEntity
 import org.springframework.security.access.prepost.PreAuthorize
 import org.springframework.web.bind.annotation.PathVariable
 import org.springframework.web.bind.annotation.RequestMapping
@@ -55,12 +59,15 @@ class BuildController {
   private BuildArtifactFilter buildArtifactFilter
   private ArtifactDecorator artifactDecorator
   private ArtifactExtractor artifactExtractor
+  private PendingOperationsCache pendingOperationsCache
 
   BuildController(BuildServices buildServices,
+                  PendingOperationsCache pendingOperationsCache,
                   Optional<BuildArtifactFilter> buildArtifactFilter,
                   Optional<ArtifactDecorator> artifactDecorator,
                   Optional<ArtifactExtractor> artifactExtractor) {
     this.buildServices = buildServices
+    this.pendingOperationsCache = pendingOperationsCache
     this.buildArtifactFilter = buildArtifactFilter.orElse(null)
     this.artifactDecorator = artifactDecorator.orElse(null)
     this.artifactExtractor = artifactExtractor.orElse(null)
@@ -165,11 +172,25 @@ class BuildController {
 
   @RequestMapping(value = '/masters/{name}/jobs/**', method = RequestMethod.PUT)
   @PreAuthorize("hasPermission(#master, 'BUILD_SERVICE', 'WRITE')")
-    String build(
+  ResponseEntity<String> build(
     @PathVariable("name") String master,
     @RequestParam Map<String, String> requestParams, HttpServletRequest request) {
     def job = ((String) request.getAttribute(
       HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE)).split('/').drop(4).join('/')
+
+    String pendingKey = computePendingBuildKey(master, job, requestParams)
+    String buildNumber = null
+
+    PendingOperationsCache.OperationState pendingStatus = pendingOperationsCache.getAndSetOperationStatus(pendingKey, PendingOperationsCache.OperationStatus.PENDING, "")
+    if (pendingStatus.status == PendingOperationsCache.OperationStatus.PENDING) {
+      return ResponseEntity.accepted().build()
+    }
+    if (pendingStatus.status == PendingOperationsCache.OperationStatus.COMPLETED && !Strings.isNullOrEmpty(pendingStatus.value)) {
+      pendingOperationsCache.clear(pendingKey)
+      return ResponseEntity.of(Optional.of(pendingStatus.value))
+    }
+
+    try {
       def buildService = getBuildService(master)
       if (buildService instanceof JenkinsService) {
         def response
@@ -205,10 +226,16 @@ class BuildController {
         }
         def queuedLocation = locationHeader.value
 
-            queuedLocation.split('/')[-1]
+        buildNumber = queuedLocation.split('/')[-1]
       } else {
-            return buildService.triggerBuildWithParameters(job, requestParams)
+        buildNumber = buildService.triggerBuildWithParameters(job, requestParams)
+      }
     }
+    finally {
+      pendingOperationsCache.setOperationStatus(pendingKey, PendingOperationsCache.OperationStatus.COMPLETED, buildNumber)
+    }
+
+    return ResponseEntity.of(Optional.of(buildNumber))
   }
 
   static void validateJobParameters(JobConfig jobConfig, Map<String, String> requestParams) {
@@ -224,6 +251,16 @@ class BuildController {
     }
   }
 
+  static String computePendingBuildKey(String master, String job, Map<String, String> requestParams) {
+    String key = master + ":" + job + ":" + AuthenticatedRequest.getSpinnakerExecutionId().orElse("NO_EXECUTION_ID")
+
+    requestParams.each { parameterDefinition ->
+      key = key + ":" + parameterDefinition.key + "=" + parameterDefinition.value
+    }
+
+    return key;
+  }
+
   @RequestMapping(value = '/builds/properties/{buildNumber}/{fileName}/{master:.+}/**')
   @PreAuthorize("hasPermission(#master, 'BUILD_SERVICE', 'READ')")
   Map<String, Object> getProperties(

igor-web/src/test/groovy/com/netflix/spinnaker/igor/build/BuildControllerSpec.groovy

@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.igor.build
 
+import com.netflix.spinnaker.igor.PendingOperationsCache
 import com.netflix.spinnaker.igor.build.model.GenericBuild
 import com.netflix.spinnaker.igor.config.JenkinsConfig
 import com.netflix.spinnaker.igor.jenkins.client.model.Build
@@ -28,7 +29,6 @@ import com.netflix.spinnaker.igor.model.BuildServiceProvider
 import com.netflix.spinnaker.igor.service.BuildOperations
 import com.netflix.spinnaker.igor.service.BuildServices
 import com.netflix.spinnaker.igor.travis.service.TravisService
-import com.netflix.spinnaker.kork.core.RetrySupport
 import com.netflix.spinnaker.kork.web.exceptions.GenericExceptionHandlers
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException
 import com.squareup.okhttp.mockwebserver.MockWebServer
@@ -60,10 +60,7 @@ class BuildControllerSpec extends Specification {
   JenkinsService jenkinsService
   BuildOperations service
   TravisService travisService
-    Map<String, BuildOperations> serviceList
-    def retrySupport = Spy(RetrySupport) {
-        _ * sleep(_) >> { /* do nothing */ }
-    }
+  PendingOperationsCache pendingOperationService
 
   @Shared
   MockWebServer server
@@ -76,7 +73,7 @@ class BuildControllerSpec extends Specification {
   final BUILD_ID = 654321
   final QUEUED_JOB_NUMBER = 123456
   final JOB_NAME = "job/name/can/have/slashes"
-    final JOB_NAME_LEGACY = "job"
+  final PENDING_JOB_NAME = "pendingjob"
   final FILE_NAME = "test.yml"
 
   GenericBuild genericBuild
@@ -103,9 +100,13 @@ class BuildControllerSpec extends Specification {
 
     cache = Mock(BuildCache)
     server = new MockWebServer()
+    pendingOperationService = Mock(PendingOperationsCache)
+    pendingOperationService.getAndSetOperationStatus(_, _, _) >> {
+        return new PendingOperationsCache.OperationState()
+    }
 
     mockMvc = MockMvcBuilders
-            .standaloneSetup(new BuildController(buildServices, Optional.empty(), Optional.empty(), Optional.empty()))
+      .standaloneSetup(new BuildController(buildServices, pendingOperationService, Optional.empty(), Optional.empty(), Optional.empty()))
       .setControllerAdvice(new GenericExceptionHandlers())
       .build()
   }
@@ -242,6 +243,8 @@ class BuildControllerSpec extends Specification {
     then:
     response.contentAsString == BUILD_NUMBER.toString()
 
+    1 * pendingOperationService.getAndSetOperationStatus(_, _, _) >> { new PendingOperationsCache.OperationState() }
+    1 * pendingOperationService.setOperationStatus(_, PendingOperationsCache.OperationStatus.COMPLETED, BUILD_NUMBER.toString())
   }
 
   void 'trigger a build with parameters to a job with parameters'() {
@@ -348,4 +351,48 @@ class BuildControllerSpec extends Specification {
     thrown(InvalidJobParameterException)
   }
 
+
+  void "doesn't trigger a build when previous request is still in progress"() {
+    given:
+    pendingOperationService = Stub(PendingOperationsCache)
+    pendingOperationService.getAndSetOperationStatus("${JENKINS_SERVICE}:${PENDING_JOB_NAME}:NO_EXECUTION_ID:foo=bat", _, _) >> {
+      return new PendingOperationsCache.OperationState(PendingOperationsCache.OperationStatus.PENDING)
+    }
+
+    mockMvc = MockMvcBuilders
+      .standaloneSetup(new BuildController(buildServices, pendingOperationService, Optional.empty(), Optional.empty(), Optional.empty()))
+      .setControllerAdvice(new GenericExceptionHandlers())
+      .build()
+
+    when:
+    MockHttpServletResponse response = mockMvc.perform(put("/masters/${JENKINS_SERVICE}/jobs/${PENDING_JOB_NAME}")
+      .contentType(MediaType.APPLICATION_JSON).param("foo", "bat")).andReturn().response
+
+    then:
+    response.status == HttpStatus.ACCEPTED.value()
+  }
+
+  void "resets the cache once the build status has been retrieved"() {
+    given:
+    pendingOperationService = Mock(PendingOperationsCache)
+    pendingOperationService.getAndSetOperationStatus("${JENKINS_SERVICE}:${JOB_NAME}:NO_EXECUTION_ID:foo=bat", _, _) >> {
+      PendingOperationsCache.OperationState state = new PendingOperationsCache.OperationState()
+      state.load(PendingOperationsCache.OperationStatus.COMPLETED.toString() + ":" + BUILD_NUMBER)
+      return state
+    }
+
+    mockMvc = MockMvcBuilders
+      .standaloneSetup(new BuildController(buildServices, pendingOperationService, Optional.empty(), Optional.empty(), Optional.empty()))
+      .setControllerAdvice(new GenericExceptionHandlers())
+      .build()
+
+    when:
+    MockHttpServletResponse response = mockMvc.perform(put("/masters/${JENKINS_SERVICE}/jobs/${JOB_NAME}")
+      .contentType(MediaType.APPLICATION_JSON).param("foo", "bat")).andReturn().response
+
+    then:
+    response.status == HttpStatus.OK.value()
+    response.contentAsString == BUILD_NUMBER.toString()
+    1 * pendingOperationService.clear("${JENKINS_SERVICE}:${JOB_NAME}:NO_EXECUTION_ID:foo=bat")
+  }
 }