fix: don't wait for pipeline to finish updating because we can't distinguish failure from progressing #313
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
…gives a false positive failure Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
juliev0
commented
Oct 2, 2024
| @@ -110,15 +110,16 @@ func verifyPipelinePaused(namespace string, pipelineRolloutName string, pipeline | |||
| }, testTimeout).Should(Equal(metav1.ConditionTrue)) | |||
|
|
|||
| document("Verify that Pipeline is paused and fully drained") | |||
| verifyPipelineStatusEventually(Namespace, pipelineName, | |||
| verifyPipelineStatusEventually(namespace, pipelineName, | |||
There was a problem hiding this comment.
we weren't using the parameter passed in to the function
juliev0
commented
Oct 2, 2024
| @@ -251,7 +251,7 @@ var _ = Describe("Functional e2e", Serial, func() { | |||
|
|
|||
| verifyPipelineRolloutDeployed(pipelineRolloutName) | |||
| verifyPipelineRolloutHealthy(pipelineRolloutName) | |||
| verifyInProgressStrategy(Namespace, pipelineRolloutName, apiv1.UpgradeStrategyNoOp) | |||
| verifyInProgressStrategy(pipelineRolloutName, apiv1.UpgradeStrategyNoOp) | |||
There was a problem hiding this comment.
remove unused parameter from this function
juliev0
commented
Oct 2, 2024
| } | ||
| return false, metav1.Condition{} | ||
| } | ||
|
|
juliev0
commented
Oct 2, 2024
| @@ -669,15 +669,10 @@ func createPipelineRollout(isbsvcSpec numaflowv1.PipelineSpec) *apiv1.PipelineRo | |||
| } | |||
| } | |||
|
|
|||
| func createPipelineOfSpec(spec numaflowv1.PipelineSpec, phase numaflowv1.PipelinePhase, fullyReconciled bool) *numaflowv1.Pipeline { | |||
| func createPipelineOfSpec(spec numaflowv1.PipelineSpec, phase numaflowv1.PipelinePhase) *numaflowv1.Pipeline { | |||
There was a problem hiding this comment.
not needed in the test anymore
juliev0
commented
Oct 2, 2024
|
|
||
| return unhealthyOrProgressing, nil | ||
|
|
||
| } |
juliev0
commented
Oct 2, 2024
| func(retrievedPipelineSpec numaflowv1.PipelineSpec, retrievedPipelineStatus numaflowv1.PipelineStatus) bool { | ||
| return retrievedPipelineStatus.Phase == numaflowv1.PipelinePhasePaused && retrievedPipelineStatus.DrainedOnPause | ||
|
|
||
| }) | ||
| verifyPodsRunning(namespace, 0, getVertexLabelSelector(pipelineName)) | ||
| // this happens too fast to verify it: | ||
| //verifyPodsRunning(namespace, 0, getVertexLabelSelector(pipelineName)) |
There was a problem hiding this comment.
this can happen too fast now to verify 0 Pods now but note that Numaflow has in fact indicated phase=Paused and drainedOnPause=true in the previous step, which it would only do if there were 0 Pods
Signed-off-by: Julie Vogelman <[email protected]>
xdevxy
approved these changes
Oct 2, 2024
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.
Modifications
This makes it so that we no longer require to be paused simply because pipeline is still reconciling.
The original process of updating was:
desiredPhase=Pausedpipeline.ObservedGenerationand pipeline Conditions, which indicate whether daemon, vertices, side inputs are fully reconcileddesiredPhase=RunningI have to remove step 4 because it's impossible to know based on Pipeline Conditions whether Pipeline is in fact still "progressing" or failed. This becomes clear when we actually deploy a bad pipeline.
Why I think this is okay to remove: After the Pipeline has been paused, it goes down to 0 vertices. After we apply the spec, Numaflow Controller updates the Vertex specs. Then we set it to
desiredPhase=Running, and it scales those Vertices back up. My original motivation to wait for it to fully reconcile was out of paranoia that we would somehow be "Running" an older Vertex but now this seems unnecessary.Verification
Ran e2e test about 6 times with success every time