feat(orchestration): Allow sibling stages to continue on FAILED_CONTINUE #3252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajordens
reviewed
Oct 23, 2019
| @@ -142,7 +142,7 @@ class CompleteStageHandler( | |||
|
|
|||
| // When a synthetic stage ends with FAILED_CONTINUE, propagate that status up to the stage's | |||
| // parent so that no more of the parent's synthetic children will run. | |||
| if (stage.status == FAILED_CONTINUE && stage.syntheticStageOwner != null) { | |||
| if (stage.status == FAILED_CONTINUE && stage.syntheticStageOwner != null && stage.allowSiblingStagesToContinueOnFailure) { | |||
There was a problem hiding this comment.
Shouldn't this be !stage.allow...? (based on the comment of not wanting to propagate up to the parent if siblings are allowed to run)
There was a problem hiding this comment.
Yes, thank you! change the name of the var last minute that inverted the meaning and forgot to update this. i guess i should find and update tests for this
There was a problem hiding this comment.
oh, look, the existing tests caught it... fixed up and added a new test
| * @param defaultValue default value to return if key is not present | ||
| * @return value or null if not present | ||
| */ | ||
| Object getCurrentOnly(@Nullable Object key, Object defaultValue) { |
There was a problem hiding this comment.
Not sure how I feel about getCurrentOnly vs overloading get() -- will let someone else weigh in.
There was a problem hiding this comment.
you mean something like
public Object get(@Nullable Object key, Object defaultValue, boolean localOnly)
?
There was a problem hiding this comment.
+1 on overloading get(...)
There was a problem hiding this comment.
Just curious, why?
I think
stage.context.getCurrentOnly("someValue", false);
is more descriptive than:
stage.context.get("someValue", false, true);
There was a problem hiding this comment.
get() because the context is a map. Matter of preference, no strong opinions.
There was a problem hiding this comment.
I hear you. This area has caused some confusion in the past (where something comes from in the context) so I am more inclined to make this very explicit
Normally, if a synthetic child stage fails with `FAILED_CONTINUE` the `CompleteStageHandler` will propagate the `FAILED_CONTINUE` status to the parent. This will prevent all subsequent sibling stages from executing. This change allows for an option (similar to Tasks) to continue execution even when a child stage has a `status=FAILED_CONTINUE` This option can only be set on a stage that is a synthetic child. This is needed for Monitored Deploy where we want to allow the deploy to proceed even if the monitoring fails
marchello2000
force-pushed
the
mark/allowsSiblingStages
branch
from
da0182b
to
10e6b31
Compare
marchello2000
added a commit
to marchello2000/orca
that referenced
this pull request
Oct 25, 2019
This change depends on spinnaker#3252 Sometimes, `DisableClusterStage` can fail. We don't want failures in `DisableClusterStage` prenventing the subsequent `ShrinkCluster` from trying to run and clean up the canary+baseline clusters
|
Going to merge this because there are teams blocked on this. If you have feedback please give it to me and I will incorporate |
mergify bot
pushed a commit
that referenced
this pull request
Oct 25, 2019
This change depends on #3252 Sometimes, `DisableClusterStage` can fail. We don't want failures in `DisableClusterStage` prenventing the subsequent `ShrinkCluster` from trying to run and clean up the canary+baseline clusters
marchello2000
added a commit
to marchello2000/orca
that referenced
this pull request
Oct 27, 2019
This is a follow up to spinnaker#3259 because I rebased my local changes incorrectly. Sometimes, `DisableClusterStage` can fail. We don't want failures in `DisableClusterStage` preventing the subsequent `ShrinkCluster` from trying to run and clean up the canary+baseline clusters This change buids on top of the functionality introduced in spinnaker#3252 to allow sibling stages to run even if a stage fails (correct error code will be propagated to the execution if this happens so it's clear to the user)
This was referenced Oct 27, 2019
mergify bot
pushed a commit
that referenced
this pull request
Nov 1, 2019
This is a follow up to #3259 because I rebased my local changes incorrectly. Sometimes, `DisableClusterStage` can fail. We don't want failures in `DisableClusterStage` preventing the subsequent `ShrinkCluster` from trying to run and clean up the canary+baseline clusters This change buids on top of the functionality introduced in #3252 to allow sibling stages to run even if a stage fails (correct error code will be propagated to the execution if this happens so it's clear to the user)
sergiopena
pushed a commit
to sergiopena/orca
that referenced
this pull request
Dec 13, 2019
This is a follow up to spinnaker#3259 because I rebased my local changes incorrectly. Sometimes, `DisableClusterStage` can fail. We don't want failures in `DisableClusterStage` preventing the subsequent `ShrinkCluster` from trying to run and clean up the canary+baseline clusters This change buids on top of the functionality introduced in spinnaker#3252 to allow sibling stages to run even if a stage fails (correct error code will be propagated to the execution if this happens so it's clear to the user)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Normally, if a synthetic child stage fails with
FAILED_CONTINUEtheCompleteStageHandlerwill propagate the
FAILED_CONTINUEstatus to the parent. This will prevent all subsequentsibling stages from executing.
This change allows for an option (similar to Tasks) to continue execution even when a
child stage has a
status=FAILED_CONTINUEThis option can only be set on a stage that is a synthetic child.
This is needed for Monitored Deploy where we want to allow the deploy to proceed even if the
monitoring fails