From b09865c8b531469c1f5ae4d5c71a4dcad341c61d Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Fri, 13 Dec 2024 16:12:25 -0600 Subject: [PATCH 1/5] fix: don't over reconcile on error Signed-off-by: Zach Aller --- rollout/controller.go | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/rollout/controller.go b/rollout/controller.go index 06dd8312bf..6322db5866 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -413,11 +413,29 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error { }() resolveErr := c.refResolver.Resolve(r) + // We should probably lose the error condition here, and just log the error to clean up the logic + //if resolveErr != nil { + // logCtx.Errorf("refResolver.Resolve err %v", resolveErr) + // return resolveErr + //} + 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 and store the errors there from the processNextWorkItem function in controller/controller.go if resolveErr != nil { roCtx.createInvalidRolloutCondition(resolveErr, r) return resolveErr @@ -539,14 +557,11 @@ 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 } - return nil, vErr + return &roCtx, vErr // return the rollout cone text with the validation error so that we can still use the context to update the rollout } return nil, err } From 9da16aed02f235a542dc2ba1a2ec8c42b310ef98 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Fri, 13 Dec 2024 16:14:02 -0600 Subject: [PATCH 2/5] fix: don't send rollout context on field error Signed-off-by: Zach Aller --- rollout/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rollout/controller.go b/rollout/controller.go index 6322db5866..641db0e65e 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -561,7 +561,7 @@ func (c *Controller) newRolloutContext(rollout *v1alpha1.Rollout) (*rolloutConte if err != nil { return nil, err } - return &roCtx, vErr // return the rollout cone text with the validation error so that we can still use the context to update the rollout + return nil, vErr } return nil, err } From f2e026710a9155f04c7affa7e0405f72b29a815e Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Fri, 13 Dec 2024 16:36:56 -0600 Subject: [PATCH 3/5] fix: cleanup comments Signed-off-by: Zach Aller --- rollout/controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rollout/controller.go b/rollout/controller.go index 641db0e65e..61af9b7564 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -413,7 +413,7 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error { }() resolveErr := c.refResolver.Resolve(r) - // We should probably lose the error condition here, and just log the error to clean up the logic + // We should probably lose setting the error condition from the below if resolveErr != nil {}, and just log the error to clean up the logic //if resolveErr != nil { // logCtx.Errorf("refResolver.Resolve err %v", resolveErr) // return resolveErr @@ -435,7 +435,7 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error { 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 and store the errors there from the processNextWorkItem function in controller/controller.go + // 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 From 62e79f51a83cf6150759fd950c7c2d639eb44384 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Mon, 16 Dec 2024 10:31:16 -0600 Subject: [PATCH 4/5] fix: cleanup comments Signed-off-by: Zach Aller --- rollout/controller.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/rollout/controller.go b/rollout/controller.go index 61af9b7564..8dfea9f09f 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -413,12 +413,7 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error { }() resolveErr := c.refResolver.Resolve(r) - // We should probably lose setting the error condition from the below if resolveErr != nil {}, and just log the error to clean up the logic - //if resolveErr != nil { - // logCtx.Errorf("refResolver.Resolve err %v", resolveErr) - // return resolveErr - //} - + // 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") @@ -437,8 +432,11 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error { // 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 resovling 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 From 6df193637570b35014cd0c3cfe7751f8309d5902 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Tue, 17 Dec 2024 09:29:22 -0600 Subject: [PATCH 5/5] fix typo Signed-off-by: Zach Aller --- rollout/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rollout/controller.go b/rollout/controller.go index 8dfea9f09f..7131ee8546 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -434,7 +434,7 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error { if resolveErr != nil { err := roCtx.createInvalidRolloutCondition(resolveErr, r) if err != nil { - return fmt.Errorf("failed to create invalid rollout condition during resovling the rollout: %w", err) + return fmt.Errorf("failed to create invalid rollout condition during resolving the rollout: %w", err) } return fmt.Errorf("failed to resolve rollout: %w", resolveErr) }