Skip to content

Commit

Permalink
🐛 Controller reconcileHandler shouldn't stop a worker on error
Browse files Browse the repository at this point in the history
During a code walkthrough noticed that oddly the reconcileHandler was
returning false and stopping the worker entirely when an error is
returned from the reconciler. This change removes the return boolean
from reconcileHandler altogether and a worker is now only stopped when
the queue is shutdown.

Signed-off-by: Vince Prignano <vincepri@vmware.com>
  • Loading branch information
vincepri committed Oct 1, 2020
1 parent 7f050c2 commit b4a0212
Showing 1 changed file with 7 additions and 8 deletions.
15 changes: 7 additions & 8 deletions pkg/internal/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,11 @@ func (c *Controller) processNextWorkItem(ctx context.Context) bool {
ctrlmetrics.ActiveWorkers.WithLabelValues(c.Name).Add(1)
defer ctrlmetrics.ActiveWorkers.WithLabelValues(c.Name).Add(-1)

return c.reconcileHandler(ctx, obj)
c.reconcileHandler(ctx, obj)
return true
}

func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) bool {
func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) {
// Update metrics after processing each item
reconcileStartTS := time.Now()
defer func() {
Expand All @@ -251,7 +252,7 @@ func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) bool
c.Queue.Forget(obj)
c.Log.Error(nil, "Queue item was not a Request", "type", fmt.Sprintf("%T", obj), "value", obj)
// Return true, don't take a break
return true
return
}

log := c.Log.WithValues("name", req.Name, "namespace", req.Namespace)
Expand All @@ -263,7 +264,7 @@ func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) bool
c.Queue.AddRateLimited(req)
ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc()
ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "error").Inc()
return false
return
} else if result.RequeueAfter > 0 {
// The result.RequeueAfter request will be lost, if it is returned
// along with a non-nil error. But this is intended as
Expand All @@ -272,20 +273,18 @@ func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) bool
c.Queue.Forget(obj)
c.Queue.AddAfter(req, result.RequeueAfter)
ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "requeue_after").Inc()
return true
return
} else if result.Requeue {
c.Queue.AddRateLimited(req)
ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "requeue").Inc()
return true
return
}

// Finally, if no error occurs we Forget this item so it does not
// get queued again until another change happens.
c.Queue.Forget(obj)

ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "success").Inc()
// Return true, don't take a break
return true
}

// InjectFunc implement SetFields.Injector
Expand Down

0 comments on commit b4a0212

Please sign in to comment.