Improve controller error handling and logging. #42
Conversation
dgoodwin
force-pushed
the
errorhandling
branch
from
d4590ee
to
77d522b
Compare
| @@ -135,18 +144,19 @@ func (r *ApplicationSetReconciler) createOrUpdateInCluster(ctx context.Context, | |||
| }) | |||
|
|
|||
| if err != nil { | |||
| appLog.WithError(err).Errorf("failed to %s Application", action) | |||
| appLog.WithError(err).WithField("action", action).Error("failed to create/update Application") | |||
| continue | |||
There was a problem hiding this comment.
Maybe add a flag to indicate if there were errors, and return the flag at the end of the function
| @@ -167,12 +177,13 @@ func (r *ApplicationSetReconciler) deleteInCluster(ctx context.Context, applicat | |||
| err := r.Client.Delete(ctx, &app) | |||
| if err != nil { | |||
| appLog.WithError(err).Error("failed to delete Application") | |||
| continue | |||
| return err | |||
There was a problem hiding this comment.
Instead of stoping the loop, we can try to do the best effort, and continue to delete other applications. We can use a boolean flag to indicate if there was an error
There was a problem hiding this comment.
Done.
It seems to make sense here. We should be careful with the concept of continuing after an error though as there could be some implications, if we applied the same concept up a level and decided to proceed with cleanup of obsolete apps after a failure creating a new one, something unintended could happen.
|
Pls fix the conflict |
Controller was ignoring errors creating/updating/deleting Applications when it should be returning that error so the object is requeued with backoff. Fixed some inaccurate or messy logging.
dgoodwin
force-pushed
the
errorhandling
branch
from
77d522b
to
62febc8
Compare
| @@ -57,6 +57,7 @@ func TestExtractApplications(t *testing.T) { | |||
| template argoprojiov1alpha1.ApplicationSetTemplate | |||
| generateParamsError error | |||
| rendererError error | |||
| expectErr bool | |||
There was a problem hiding this comment.
Per these changes we now expect these tests to do something differently, as we don't want to swallow an error, we want to return it and let the queue retry with backoff.
|
PR updated, conflict resolved, tests passing. |
Controller was ignoring errors creating/updating/deleting Applications
when it should be returning that error so the object is requeued with
backoff.
Fixed some inaccurate or messy logging.