From 2f0e25f8afd0dea6cd57d3d4ba4fe62d51162d5b Mon Sep 17 00:00:00 2001 From: Adam Jordens Date: Thu, 20 Jun 2019 11:16:59 -0700 Subject: [PATCH] feat(perf): Favor a single pipeline config lookup (#3001) Fetching a single pipeline config _should_ be more efficient than fetching all pipelines (for an application) and filtering in `orca`. This is somewhat nuanced as the "all pipelines for application" is likely being served out of the in-memory cache. We have seen what looks like serialization-related load and this PR aims to offer some relief. --- .../pipeline/EnabledPipelineValidator.java | 20 +++++++ .../EnabledPipelineValidatorSpec.groovy | 60 +++++++++++++------ 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidator.java b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidator.java index 0a542eec44..9b8dfc7a28 100644 --- a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidator.java +++ b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidator.java @@ -48,6 +48,26 @@ public void checkRunnable(Execution pipeline) { throw new UnsupportedOperationException( "Front50 not enabled, no way to validate pipeline. Fix this by setting front50.enabled: true"); } + + if (!isStrategy(pipeline)) { + // attempt an optimized lookup via pipeline history vs fetching all pipelines for the + // application and filtering + List> pipelineConfigHistory = + front50Service.getPipelineHistory(pipeline.getPipelineConfigId(), 1); + + if (!pipelineConfigHistory.isEmpty()) { + Map pipelineConfig = pipelineConfigHistory.get(0); + if ((boolean) pipelineConfig.getOrDefault("disabled", false)) { + throw new PipelineIsDisabled( + pipelineConfig.get("id").toString(), + pipelineConfig.get("application").toString(), + pipelineConfig.get("name").toString()); + } + + return; + } + } + List> pipelines = isStrategy(pipeline) ? front50Service.getStrategies(pipeline.getApplication()) diff --git a/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidatorSpec.groovy b/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidatorSpec.groovy index de715f9976..38f015c739 100644 --- a/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidatorSpec.groovy +++ b/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/pipeline/EnabledPipelineValidatorSpec.groovy @@ -26,18 +26,19 @@ import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline class EnabledPipelineValidatorSpec extends Specification { - def front50Service = Stub(Front50Service) + def front50Service = Mock(Front50Service) + @Subject def validator = new EnabledPipelineValidator(Optional.of(front50Service)) def "allows one-off pipeline to run"() { - given: - front50Service.getPipelines(execution.application, false) >> [] - when: validator.checkRunnable(execution) then: + 1 * front50Service.getPipelineHistory(execution.pipelineConfigId, 1) >> [] + 1 * front50Service.getPipelines(execution.application, false) >> [] + notThrown(PipelineValidationFailed) where: @@ -48,15 +49,28 @@ class EnabledPipelineValidatorSpec extends Specification { } def "allows enabled pipeline to run"() { - given: - front50Service.getPipelines(execution.application, false) >> [ - [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: false] + when: + validator.checkRunnable(execution) + + then: + 1 * front50Service.getPipelineHistory(execution.pipelineConfigId, 1) >> [ + ] + 1 * front50Service.getPipelines(execution.application, false) >> [ + [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: false] ] + 0 * _ + + notThrown(PipelineValidationFailed) when: validator.checkRunnable(execution) then: + 1 * front50Service.getPipelineHistory(execution.pipelineConfigId, 1) >> [ + [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: false] + ] + 0 * _ + notThrown(PipelineValidationFailed) where: @@ -67,15 +81,28 @@ class EnabledPipelineValidatorSpec extends Specification { } def "prevents disabled pipeline from running"() { - given: - front50Service.getPipelines(execution.application, false) >> [ - [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: true] + when: + validator.checkRunnable(execution) + + then: + 1 * front50Service.getPipelineHistory(execution.pipelineConfigId, 1) >> [ ] + 1 * front50Service.getPipelines(execution.application, false) >> [ + [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: true] + ] + 0 * _ + + thrown(EnabledPipelineValidator.PipelineIsDisabled) when: validator.checkRunnable(execution) then: + 1 * front50Service.getPipelineHistory(execution.pipelineConfigId, 1) >> [ + [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: true] + ] + 0 * _ + thrown(EnabledPipelineValidator.PipelineIsDisabled) where: @@ -88,7 +115,7 @@ class EnabledPipelineValidatorSpec extends Specification { def "allows enabled strategy to run"() { given: front50Service.getStrategies(execution.application) >> [ - [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: false] + [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: false] ] when: @@ -109,7 +136,7 @@ class EnabledPipelineValidatorSpec extends Specification { def "prevents disabled strategy from running"() { given: front50Service.getStrategies(execution.application) >> [ - [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: true] + [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: true] ] when: @@ -128,15 +155,14 @@ class EnabledPipelineValidatorSpec extends Specification { } def "doesn't choke on non-boolean strategy value"() { - given: - front50Service.getPipelines(execution.application, false) >> [ - [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: false] - ] - when: validator.checkRunnable(execution) then: + 1 * front50Service.getPipelineHistory(execution.pipelineConfigId, 1) >> [ + [id: execution.pipelineConfigId, application: execution.application, name: "whatever", disabled: false] + ] + notThrown(PipelineValidationFailed) where: