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.

Sign up for GitHub

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

Jump to bottom

fix(Orca): Feature: option to add a delay before polling starts in Webhook stage #3450 #2974

Merged
merged 5 commits into from
Jun 11, 2019
Merged

fix(Orca): Feature: option to add a delay before polling starts in Webhook stage #3450 #2974

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d42bfd3
Merge pull request #1 from spinnaker/master
neelimamanubolu May 22, 2019
4d5e37b
Merge pull request #2 from spinnaker/master
neelimamanubolu Jun 11, 2019
9405642
fix(Orca): Feature: option to add a delay before polling starts in We…
Jun 11, 2019
23a861a
Merge branch 'master' of github.com:neelimamanubolu/orca
Jun 11, 2019
7cfcc06
Merge branch 'master' into master
asher Jun 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...webhook/src/main/groovy/com/netflix/spinnaker/orca/webhook/tasks/CreateWebhookTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class CreateWebhookTask implements RetryableTask {
return TaskResult.builder(ExecutionStatus.TERMINAL).context(outputs).build()
}

if (statusCode.is5xxServerError() || statusCode.value() == 429) {
if (statusCode.is5xxServerError() || statusCode.value() == 429 || statusCode.value() == 404) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the issue is talking about the monitoring version of the webhook task:
https://github.com/spinnaker/orca/blob/master/orca-webhook/src/main/groovy/com/netflix/spinnaker/orca/webhook/tasks/MonitorWebhookTask.groovy#L92

I am a little concerned about blindly retrying on 404 - I think in more cases than not 404 is an irrecoverable error rather than retryable.

Adding this to the monitor side is a little more palatable but I'd rather we don't add it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Mark Vulfson ,
Kindly please go through @eric Zimanyi comments for the issue " Adding two parameters in the UI to create delay before polling starts in Webhook stage spinnaker/spinnaker#4470 ".
Original issue is "Feature: option to add a delay before polling starts in Webhook stage spinnaker/spinnaker#3450".
We proposed solution that we will go in the form of issue #4470, Eric suggested to go with two parts.. 1. add 404 status and 2.add delay.
We done 404 status as the first attempt and trying to add delay in the next step.

Copy link
Contributor

Choose a reason for hiding this comment

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

@neelimamanubolu I see what @ezimanyi is saying. But note that as I mentioned earlier, the changes you made do not implement his suggestion. His idea is to retry on 404 in the MonitorWebhookTask not WebhookTask. I would be more OK with retrying on 404 on the MonitorTask (though I still think, and @ezimanyi notes in his comment as well, this is a breaking change.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In MonitorWebhookTask, changes are not reflecting, @ezimanyi gave suggestion to add like 5xx and 429, but not exactly in the same class. So we created in WebhookTask , tested and fially made PR.

String errorMessage = "error submitting webhook for pipeline ${stage.execution.id} to ${stageData.url}, will retry."
log.warn(errorMessage, e)

Expand Down