From ccdb66249a208c72a4ef6ef7d1703c3fec29a8ae Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 2 Apr 2024 15:59:04 +0000 Subject: [PATCH] fix(explicitRollback): Add configurable timeout for serverGroup lookup from Clouddriver API (#4686) (#4690) * fix(explicitRollback): Add configurable timeout for serverGroup rollback from Clouddriver API * fix(explicitRollback): Fix tests * fix(explicitRollback): Add configurable timeout for serverGroup lookup from Clouddriver API * fix(explicitRollback): Add configurable timeout for serverGroup lookup from Clouddriver API * fix(explicitRollback): Add configurable timeout for serverGroup lookup from Clouddriver API --------- Co-authored-by: Jason (cherry picked from commit 2b8af022d3ae06a430bc2bcf372072e560eb51c5) Co-authored-by: Christos Arvanitis --- .../rollback/ExplicitRollback.groovy | 11 ++- .../RollbackConfigurationProperties.java | 73 +++++++++++++++++++ .../DetermineRollbackCandidatesTask.java | 6 +- .../rollback/ExplicitRollbackSpec.groovy | 14 +++- ...DetermineRollbackCandidatesTaskSpec.groovy | 10 ++- 5 files changed, 107 insertions(+), 7 deletions(-) create mode 100644 orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/config/RollbackConfigurationProperties.java diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy index 1ae37a1114..c9eeb373ed 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollback.groovy @@ -21,6 +21,7 @@ import com.netflix.spinnaker.kork.core.RetrySupport import com.netflix.spinnaker.kork.exceptions.SpinnakerException import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution import com.netflix.spinnaker.orca.clouddriver.CloudDriverService +import com.netflix.spinnaker.orca.clouddriver.config.RollbackConfigurationProperties import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.ApplySourceServerGroupCapacityStage import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.CaptureSourceServerGroupCapacityStage import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage @@ -34,6 +35,7 @@ import com.netflix.spinnaker.orca.api.pipeline.SyntheticStageOwner import com.netflix.spinnaker.security.AuthenticatedRequest import groovy.util.logging.Slf4j import org.springframework.beans.factory.annotation.Autowired +import org.springframework.beans.factory.annotation.Value import javax.annotation.Nullable import java.util.concurrent.Callable @@ -49,6 +51,10 @@ class ExplicitRollback implements Rollback { Boolean disableOnly Boolean enableAndDisableOnly + @Autowired + @JsonIgnore + RollbackConfigurationProperties rollbackConfigurationProperties; + @JsonIgnore private final ExecutorService executor = java.util.concurrent.Executors.newSingleThreadExecutor() @@ -149,6 +155,7 @@ class ExplicitRollback implements Rollback { @Nullable TargetServerGroup lookupServerGroup(StageExecution parentStage, String serverGroupName) { def fromContext = parentStage.mapTo(ResizeStrategy.Source) + def rollbackTimeout = rollbackConfigurationProperties.explicitRollback.getTimeout() try { // we use an executor+future to timebox how long we can spend in this remote call @@ -163,11 +170,11 @@ class ExplicitRollback implements Rollback { }) executor.submit(authenticatedRequest) - .get(5, TimeUnit.SECONDS) + .get(rollbackTimeout, TimeUnit.SECONDS) .get() // not sure what would cause the Optional to not be present but we would catch and log it } catch(Exception e) { log.error('Could not generate resize stage because there was an error looking up {}', serverGroupName, e) - throw new SpinnakerException("failed to look up ${serverGroupName}", e) + throw new SpinnakerException("Failed Clouddriver look up for ${serverGroupName} in ${rollbackTimeout} sec. Consider increasing rollback.explicitRollback.timeout in orca profile.", e) } } diff --git a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/config/RollbackConfigurationProperties.java b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/config/RollbackConfigurationProperties.java new file mode 100644 index 0000000000..ba4540dc00 --- /dev/null +++ b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/config/RollbackConfigurationProperties.java @@ -0,0 +1,73 @@ +/* + * Copyright 2024 Harness, 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.orca.clouddriver.config; + +import lombok.Data; +import lombok.Getter; +import lombok.NoArgsConstructor; +import lombok.Setter; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.boot.context.properties.DeprecatedConfigurationProperty; +import org.springframework.boot.context.properties.NestedConfigurationProperty; +import org.springframework.context.annotation.Configuration; + +@Data +@Configuration +@ConfigurationProperties(prefix = "rollback") +public class RollbackConfigurationProperties { + + private static final Logger logger = + LoggerFactory.getLogger(RollbackConfigurationProperties.class); + + @Value("${rollback.timeout.enabled:false}") + private boolean dynamicRollbackEnabled; + + @Deprecated + @DeprecatedConfigurationProperty(replacement = "rollback.dynamicRollback.enabled") + public boolean getDynamicRollbackEnabled() { + return dynamicRollbackEnabled; + } + + @NestedConfigurationProperty private ExplicitRollback explicitRollback = new ExplicitRollback(); + + @NestedConfigurationProperty private DynamicRollback dynamicRollback = new DynamicRollback(); + + @Getter + @Setter + @NoArgsConstructor + public static class ExplicitRollback { + private int timeout = 5; + } + + @Getter + @Setter + @NoArgsConstructor + public static class DynamicRollback { + private boolean enabled = false; + } + + public boolean isDynamicRollbackEnabled() { + if (getDynamicRollbackEnabled()) { + logger.warn( + "The rollback.timeout.enabled property is deprecated and will be removed in a future release. Please use rollback.dynamicRollback.enabled instead."); + } + return dynamicRollback.isEnabled() || getDynamicRollbackEnabled(); + } +} diff --git a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTask.java b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTask.java index 565b3ce918..625f7ab8d1 100644 --- a/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTask.java +++ b/orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTask.java @@ -31,6 +31,7 @@ import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution; import com.netflix.spinnaker.orca.clouddriver.CloudDriverService; import com.netflix.spinnaker.orca.clouddriver.FeaturesService; +import com.netflix.spinnaker.orca.clouddriver.config.RollbackConfigurationProperties; import com.netflix.spinnaker.orca.clouddriver.model.Cluster; import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup; import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup.Capacity; @@ -52,7 +53,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Value; import org.springframework.stereotype.Component; /** @@ -92,13 +92,13 @@ public DetermineRollbackCandidatesTask( RetrySupport retrySupport, CloudDriverService cloudDriverService, FeaturesService featuresService, - @Value("${rollback.timeout.enabled:false}") boolean dynamicRollbackTimeoutEnabled) { + RollbackConfigurationProperties rollbackConfigurationProperties) { this.retrySupport = retrySupport; this.cloudDriverService = cloudDriverService; this.previousImageRollbackSupport = new PreviousImageRollbackSupport( objectMapper, cloudDriverService, featuresService, retrySupport); - this.dynamicRollbackTimeoutEnabled = dynamicRollbackTimeoutEnabled; + this.dynamicRollbackTimeoutEnabled = rollbackConfigurationProperties.isDynamicRollbackEnabled(); } @Override diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollbackSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollbackSpec.groovy index 4b82568110..0eb290b341 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollbackSpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/ExplicitRollbackSpec.groovy @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback import com.netflix.spinnaker.kork.exceptions.SpinnakerException import com.netflix.spinnaker.orca.clouddriver.CloudDriverService +import com.netflix.spinnaker.orca.clouddriver.config.RollbackConfigurationProperties import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.ApplySourceServerGroupCapacityStage import com.netflix.spinnaker.orca.clouddriver.pipeline.providers.aws.CaptureSourceServerGroupCapacityStage import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.DisableServerGroupStage @@ -42,6 +43,15 @@ class ExplicitRollbackSpec extends Specification { def captureSourceServerGroupCapacityStage = new CaptureSourceServerGroupCapacityStage() def applySourceServerGroupCapacityStage = new ApplySourceServerGroupCapacityStage() def waitStage = new WaitStage() + + def explicitRollback = new RollbackConfigurationProperties.ExplicitRollback( + timeout: 20 + ) + def rollbackConfigurationProperties = new RollbackConfigurationProperties( + explicitRollback: explicitRollback + + ) + CloudDriverService cloudDriverService = Mock() def stage = stage { @@ -61,7 +71,8 @@ class ExplicitRollbackSpec extends Specification { captureSourceServerGroupCapacityStage: captureSourceServerGroupCapacityStage, applySourceServerGroupCapacityStage: applySourceServerGroupCapacityStage, waitStage: waitStage, - cloudDriverService: cloudDriverService + cloudDriverService: cloudDriverService, + rollbackConfigurationProperties: rollbackConfigurationProperties ) def setup() { @@ -213,4 +224,5 @@ class ExplicitRollbackSpec extends Specification { 1 * cloudDriverService.getTargetServerGroup(_, rollbackServerGroupName, _) >> { throw new Exception(":(") } thrown(SpinnakerException) } + } diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTaskSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTaskSpec.groovy index bc2b089fdb..e5f2143bc0 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTaskSpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTaskSpec.groovy @@ -22,6 +22,7 @@ import com.netflix.spinnaker.moniker.Moniker import com.netflix.spinnaker.orca.clouddriver.CloudDriverService import com.netflix.spinnaker.orca.clouddriver.FeaturesService import com.netflix.spinnaker.orca.clouddriver.ModelUtils +import com.netflix.spinnaker.orca.clouddriver.config.RollbackConfigurationProperties import com.netflix.spinnaker.orca.clouddriver.model.EntityTags import com.netflix.spinnaker.orca.clouddriver.model.ServerGroup import spock.lang.Specification @@ -34,6 +35,12 @@ import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage class DetermineRollbackCandidatesTaskSpec extends Specification { def objectMapper = new ObjectMapper() def featuresService = Mock(FeaturesService) + def dynamicRollback = new RollbackConfigurationProperties.DynamicRollback( + enabled: true + ) + def rollbackConfigurationProperties = new RollbackConfigurationProperties( + dynamicRollback: dynamicRollback + ) CloudDriverService cloudDriverService = Mock() @Subject @@ -42,7 +49,7 @@ class DetermineRollbackCandidatesTaskSpec extends Specification { new RetrySupport(), cloudDriverService, featuresService, - true + rollbackConfigurationProperties ) def stage = stage { @@ -68,6 +75,7 @@ class DetermineRollbackCandidatesTaskSpec extends Specification { def "should use dynamic timeout" () { given: "The user preferred rollbackTimeout is present in the context" + stage.context["rollbackTimeout"] = 2 when: "The dynamicRollback is enabled"