fix ansible reconcile when cr was not found #2032
Conversation
openshift-ci-robot
added
the
size/M
|
/test e2e-aws-ansible |
1 similar comment
|
/test e2e-aws-ansible |
camilamacedo86
force-pushed
the
fix-ansible-reconcile
branch
2 times, most recently
from
c451e83
to
92e59c8
Compare
camilamacedo86
force-pushed
the
fix-ansible-reconcile
branch
from
92e59c8
to
bb3ad27
Compare
|
/test e2e-aws-subcommand |
|
/test e2e-aws-ansible |
|
/test e2e-aws-ansible |
There was a problem hiding this comment.
IIUC, the changes to markDone and markError would actually result in extra reconciliations because we would end up returning the not found error instead of nil, when returning from the Reconcile() method. So I think those changes should be reverted.
I don't think the other refactoring and logging changes are necessary, since they just add extra logs to the operator output that are probably more debug/trace type of logs, but I'll defer to @fabianvf to see if he thinks we should add more verbosity.
| if apierrors.IsNotFound(err) { | ||
| logger.Info("Resource not found, assuming it was deleted", err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This change would result in returning an error from this function when the CR is not found, which would result in returning an error from the main Reconcile function (see here).
By returning the not found error, we would trigger another Reconcile unnecessarily. By returning nil, the Reconcile function will also return nil, which will result in no further reconciliations occurring.
Ditto for the changes in markError, except that we never actually check the error returned from markError. In the markError cases, we always return the original error no matter what happens with markError.
| return reconcile.Result{}, nil | ||
| } | ||
| // Error reading the object - requeue the request. | ||
| logger.Error(err, "Failed to get Resource.") |
There was a problem hiding this comment.
I don't think it is necessary to log this error, since controller-runtime will also log it when we return from Reconcile().
Ditto for the logged error in line 209.
|
@camilamacedo86: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Hi @joelanford, Thank you for your explanations. It makes sense. I am closing this one. |
Description of the change:
When the CR is deleted we should stop the reconcile which shows that is NOT and it shows not happening. See that the markDone and markError will return nil then the reconcile will not have the info which is required to be stoped.
Motivation for the change:
#2031