Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cfn): Add retries when force cache refreshing #3200

Merged
merged 2 commits into from
Oct 1, 2019

Conversation

xavileon
Copy link
Contributor

@xavileon xavileon commented Oct 1, 2019

For cache refreshing the CloudFormation cache can fail for
some reason (e.g. read timeout), but should generally succeed.
This patch makes this task more reliable by retrying if there was
a failure for up to 5 minutes.

For cache refreshing can fail for some reason (e.g. read
timeout), but should generally succeed. This patch makes
this task more reliable by retrying if there was a failure
for 5 minutes.

Signed-off-by: Xavi León <xavi.leon@adevinta.com>
import javax.annotation.Nonnull;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

@Component
public class CloudFormationForceCacheRefreshTask extends AbstractCloudProviderAwareTask
implements Task {
implements OverridableTimeoutRetryableTask {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just making sure that you intended this to be OverridableTimeoutRetryableTask vs. RetryableTask

OverridableTimeoutRetryableTask allow the timeout to be overridden by the user in the stage config, is that what you want?

(https://github.com/spinnaker/orca/blob/master/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandler.kt#L238)

Copy link
Contributor

@marchello2000 marchello2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One clarification, otherwise LGTM

@xavileon
Copy link
Contributor Author

xavileon commented Oct 1, 2019

Ah good point! Haven't thought about that. I don't think it needs to be configurable but I see no harm on it in case the default I chose doesn't apply. Is there any hidden reason to not making it configurable? I think I'm fine either way.

@marchello2000
Copy link
Contributor

no real reason/harm in having it configurable, @xavileon - just that it adds another point to think about when you're debugging a deploy that took way too long..? I will merge as is, and you can change it later if you want - unless you tell me otherwise - need to pull master and wait for travis anyway

@marchello2000 marchello2000 merged commit 3bad6ca into spinnaker:master Oct 1, 2019
@xavileon
Copy link
Contributor Author

xavileon commented Oct 2, 2019

@spinnakerbot cherry-pick 1.16

@spinnakerbot
Copy link
Contributor

Cherry pick successful: #3204

spinnakerbot pushed a commit that referenced this pull request Oct 2, 2019
For cache refreshing can fail for some reason (e.g. read
timeout), but should generally succeed. This patch makes
this task more reliable by retrying if there was a failure
for 5 minutes.

Signed-off-by: Xavi León <xavi.leon@adevinta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants