fix: don't over reconcile on error (#4005)

* fix: don't over reconcile on error Signed-off-by: Zach Aller <[email protected]> * fix: don't send rollout context on field error Signed-off-by: Zach Aller <[email protected]> * fix: cleanup comments Signed-off-by: Zach Aller <[email protected]> * fix: cleanup comments Signed-off-by: Zach Aller <[email protected]> * fix typo Signed-off-by: Zach Aller <[email protected]> --------- Signed-off-by: Zach Aller <[email protected]>
argoproj · Dec 17, 2024 · 0eec0d8 · 0eec0d8
1 parent 707bd9f
commit 0eec0d8
Showing 1 changed file with 18 additions and 5 deletions.
diff --git a/rollout/controller.go b/rollout/controller.go
@@ -413,14 +413,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
@@ -539,9 +555,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