feat: perform selected strategy for isbsvc fields to support PPND #317
Conversation
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
dpadhiar
changed the title
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
| @@ -278,32 +302,60 @@ func (r *ISBServiceRolloutReconciler) processExistingISBService(ctx context.Cont | |||
| r.processISBServiceStatus(ctx, existingISBServiceDef, isbServiceRollout) | |||
|
|
|||
| // if I need to update or am in the middle of an update of the ISBService, then I need to make sure all the Pipelines are pausing | |||
There was a problem hiding this comment.
can you update the comment to remove "need to update or"?
|
nice job |
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
Signed-off-by: Dillen Padhiar <[email protected]>
|
Spoke to @dpadhiar and when he gets time he'll explore why the previous test for updating isbsvc with PPND is sometimes failing before we merge this. (Merging this is not critically important to be done by Q1) |
|
Actually, I went through the artifacts I had when I saw the failure locally - not sure if numaplane is really doing anything wrong - we ultimately need to get the fixes from numaflow and try with that. I'm still a little nervous about merging this in since we weren't seeing this failure before. |
| // if not, should we set one? | ||
| if !inProgressStrategySet { | ||
| if userPreferredStrategy == config.PPNDStrategyID { | ||
| if upgradeStrategyType == apiv1.UpgradeStrategyPPND { |
There was a problem hiding this comment.
nit: I know you copied this from my redundant code in PipelineRolloutController, so my bad, but I believe the check for userPreferredStrategy here and below may be unnecessary as it's happening within ResourceNeedsUpdating(). We probably don't need to get the value for userPreferredStrategy above at all.
| existingStatefulSetDef: createDefaultISBStatefulSet("2.10.11", true), | ||
| existingPipeline: createDefaultPipelineOfPhase(numaflowv1.PipelinePhasePaused, map[string]string{}), | ||
| expectedRolloutPhase: apiv1.PhaseDeployed, | ||
| existingInProgressStrategy: nil, |
There was a problem hiding this comment.
nit: I'm thinking in this case the incoming InProgressStrategy would probably be PPND
| verifyInProgressStrategy(pipelineRolloutName, apiv1.UpgradeStrategyNoOp) | ||
| verifyPipelineStatusConsistently(Namespace, pipelineName, func(retrievedPipelineSpec numaflowv1.PipelineSpec, retrievedPipelineStatus numaflowv1.PipelineStatus) bool { | ||
| return retrievedPipelineStatus.Phase != numaflowv1.PipelinePhasePaused | ||
| }) |
There was a problem hiding this comment.
oh, this looks like a Consistently is being executed within a Consistently. Instead of calling verifyPipelineStatusConsistently(), can we just check the Pipeline phase directly?
| verifyISBSvcRolloutReady(isbServiceRolloutName) | ||
|
|
||
| verifyISBSvcReady(Namespace, isbServiceRolloutName, 3) | ||
|
|
||
| verifyInProgressStrategyISBService(Namespace, isbServiceRolloutName, apiv1.UpgradeStrategyNoOp) | ||
| verifyInProgressStrategy(pipelineRolloutName, apiv1.UpgradeStrategyNoOp) | ||
| verifyPipelineRunning(Namespace, pipelineName, 3) | ||
|
|
There was a problem hiding this comment.
some of these are being checked in the Consistently block above I think, and we don't need them ?
Fixes #248
Modifications
Integrates the USDE logic to derive an upgrade strategy (that we currently have for PipelineRollouts) into the ISBServiceRollout reconciliation process.
Verification
Ran
make testto verify unit tests are passing.Ran
make test-e2elocally and on CI to see that updating with and without excluded fields works properly.