do not always error out while checking for existence of resources #151
Conversation
|
I'm a bit divided b/n where the handling should happen, like, err returned from the load function can't always be same as returned by api server. To be moved inside the function it needs to copied at a couple of places, pls review. |
| @@ -130,6 +131,10 @@ func (r *ClientProfileReconciler) Reconcile(ctx context.Context, req ctrl.Reques | |||
|
|
|||
| func (r *ClientProfileReconcile) reconcile() error { | |||
| if err := r.loadAndValidate(); err != nil { | |||
| if k8serrors.IsNotFound(err) && r.clientProfile.UID == "" { | |||
There was a problem hiding this comment.
@leelavg wouldn't it make more sense to investigate the error object to see the target of the not found error?
| @@ -226,6 +226,10 @@ func (r *DriverReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr | |||
| func (r *driverReconcile) reconcile() error { | |||
| // Load the driver desired state based on driver resource, operator config resource and default values. | |||
| if err := r.LoadAndValidateDesiredState(); err != nil { | |||
| if k8serrors.IsNotFound(err) && r.driver.UID == "" { | |||
controllers also get reconciles for deletion events and it may happen that resource at api server be deleted first before it being purged from cache and during that time if we error out the reconciliation will never be successful in standard conditions. Signed-off-by: Leela Venkaiah G <[email protected]>
| status, ok := err.(k8serrors.APIStatus) | ||
| return (ok || errors.As(err, &status)) && |
There was a problem hiding this comment.
@leelavg Why the two different approaches? can we always use errors.As?
There was a problem hiding this comment.
there is a possibility that APIStatus could be wrapped and so the interface checks fails but unwrapping it can be successful which provides the Status() function satisfying this interface and query the error details. ref: https://github.com/kubernetes/apimachinery/blob/master/pkg/api/errors/errors.go#L809-L814
another thing to note here is, API could just return httpStatusNotFound without setting details of the resource in which case this function fails to work as intended, based on the review I could return true for both if status not found and resource is intended object, if status is not found but resource isn't set for us to verify.
There was a problem hiding this comment.
We don't need to complicate this so much, we know exactly the function that produced the error and the potential error space coming from it, Also we are looking for a subset of a specific error code that comes from a trusted source (the k8s client). With all of this in mind, you don't need to write code that is so defensive
There was a problem hiding this comment.
initial ask from #151 (comment) is the error object to see the target of the not found error? and I see it's not always possible to get the target name which was not found.
controllers also get reconciles for deletion events and it may happen that resource at api server be deleted first before it being purged from cache and during that time if we error out the reconciliation will never be successful in standard conditions.
fixes: #78