Bug 1779390: force reconcile if any error conditions are set or !Provisioned #152
Conversation
in the event that a CredentialsRequest object has been recently synced, but has an error condition set, ensure that the CredentialsRequest is re-reconciled to attemp detecting/handling the error condition (so that it can be cleared). also check the CredentialsRequest.Status.Provisioned flag so that we always re-reconcile if Provisioned != True.
openshift-ci-robot
added
the
size/M
openshift-ci-robot
added
the
approved
|
/test e2e-aws |
joelddiaz
changed the title
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
@joelddiaz: This pull request references Bugzilla bug 1779390, which is invalid:
Comment In response to this:
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. |
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/valid-bug
|
@joelddiaz: This pull request references Bugzilla bug 1779390, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
|
|
||
| if !isStale && hasRecentlySynced && crSecretExists { | ||
| if !isStale && hasRecentlySynced && crSecretExists && !hasActiveFailureConditions && cr.Status.Provisioned { |
There was a problem hiding this comment.
Is it technically feasible to not set lastSyncGen and lastSyncTime, if we did not fully reconcile the cred request without error?
I feel like the intent of these fields is to indicate the last successfully synced generation and they should not be set if we hit errors.
There was a problem hiding this comment.
From slack, we don't, normally, and we're not sure how this is possible that we got into this state.
When I read this, I thought "but we want to resync if there is an error, even if we haven't been recently synced". Looking at the code, I see that is the case, where we skip resyncing if everything looks good, but an old sync, or an error condition, or a provision failure are all sufficient on their own to trigger a resync. Just thought I'd translate that into English to make sure we're all on the same page and head off anyone else making my same mistaken first impression ;). |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgoodwin, joelddiaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@joelddiaz: All pull requests linked via external trackers have merged. Bugzilla bug 1779390 has been moved to the MODIFIED state. In response to this:
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. |
in the event that a CredentialsRequest object has been recently synced, but has an error condition set, ensure that the CredentialsRequest is re-reconciled to attemp detecting/handling the error condition (so that it can be cleared).
also check the CredentialsRequest.Status.Provisioned flag so that we always re-reconcile if Provisioned != True.