From 9c5d75d84a998bc0515caa64085bf8c23f2a41b7 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 17 Dec 2024 10:03:01 -0600 Subject: [PATCH] fix: don't over reconcile on error (#4005) * fix: don't over reconcile on error Signed-off-by: Zach Aller * fix: don't send rollout context on field error Signed-off-by: Zach Aller * fix: cleanup comments Signed-off-by: Zach Aller * fix: cleanup comments Signed-off-by: Zach Aller * fix typo Signed-off-by: Zach Aller --------- Signed-off-by: Zach Aller --- rollout/controller.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/rollout/controller.go b/rollout/controller.go index a6ad762702..7e94d7f789 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -406,14 +406,30 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error { }() resolveErr := c.refResolver.Resolve(r) + // We could maybe lose setting the error condition from the below if resolveErr != nil {}, and just log the error to clean up the logic roCtx, err := c.newRolloutContext(r) + if roCtx == nil { + logCtx.Error("newRolloutContext returned nil") + return err + } if err != nil { + if _, ok := err.(*field.Error); ok { + // We want to frequently requeue rollouts with InvalidSpec errors, because the error + // condition might be timing related (e.g. the Rollout was applied before the Service). + c.enqueueRolloutAfter(roCtx.rollout, 20*time.Second) + return nil // do not requeue from error because we already re-queued above + } logCtx.Errorf("newRolloutContext err %v", err) return err } + // We should probably delete this if block and just log the error to clean up the logic, a bigger change would be to add a new + // field to the status maybe (reconcileErrMsg) and store the errors there from the processNextWorkItem function in controller/controller.go if resolveErr != nil { - roCtx.createInvalidRolloutCondition(resolveErr, r) - return resolveErr + err := roCtx.createInvalidRolloutCondition(resolveErr, r) + if err != nil { + return fmt.Errorf("failed to create invalid rollout condition during resolving the rollout: %w", err) + } + return fmt.Errorf("failed to resolve rollout: %w", resolveErr) } // In order to work with HPA, the rollout.Spec.Replica field cannot be nil. As a result, the controller will update @@ -532,9 +548,6 @@ func (c *Controller) newRolloutContext(rollout *v1alpha1.Rollout) (*rolloutConte err = roCtx.getRolloutValidationErrors() if err != nil { if vErr, ok := err.(*field.Error); ok { - // We want to frequently requeue rollouts with InvalidSpec errors, because the error - // condition might be timing related (e.g. the Rollout was applied before the Service). - c.enqueueRolloutAfter(roCtx.rollout, 20*time.Second) err := roCtx.createInvalidRolloutCondition(vErr, roCtx.rollout) if err != nil { return nil, err