Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error logs with stale success message #357

Merged
merged 2 commits into from
Mar 20, 2023
Merged

Conversation

darkowlzz
Copy link
Contributor

@darkowlzz darkowlzz commented Mar 14, 2023

Depends on fluxcd/pkg#511.

Due to a bug in reconcile ResultFinalizer, the Ready=True condition on
an ImageRepository object from the previous reconciliation wasn't
getting updated even when the reconciliation failed, for example due to
image scan failure.

When the ResultFinalizer receives an object with stale Ready=True with
message "successful scan: found x tags" from the previous successful
reconciliation, Reconciling=True to indicate in-progress reconciliation
and a reconciliation error, ResultFinalizer updates Ready value if it's
True. But the check was performed using conditions.IsReady() which
also takes into consideration the Reconciling and Stalled values in the
status. Due to the presence of Reconciling=True, even though Ready was
True, conditions.IsReady() returned False, and ResultFinalizer skips
updating the Ready value, resulting in a fully reconciled final object
status with Reconciling and Ready both True, which is an incorrect
reconciliation status result.
ResultFinalizer now uses conditions.IsTrue(obj, meta.ReadyCondition)
to precisely check if only Ready=True is present.

Also, remove ExternalEventRecorder from ImageRepositoryReconciler, which
is no longer used and add a missing return on progressive status
patching failure.

In addition, update tests to use condition checker with gomega WithT()
for proper assertion failure and stacktrace.

Refer #356

Due to a bug in reconcile ResultFinalizer, the Ready=True condition on
an ImageRepository object from the previous reconciliation wasn't
getting updated even when the reconciliation failed, for example due to
image scan failure.

When the ResultFinalizer receives an object with stale Ready=True with
message "successful scan: found x tags" from the previous successful
reconciliation, Reconciling=True to indicate in-progress reconciliation
and a reconciliation error, ResultFinalizer updates Ready value if it's
True. But the check was performed using `conditions.IsReady()` which
also takes into consideration the Reconciling and Stalled values in the
status. Due to the presence of Reconciling=True, even though Ready was
True, `conditions.IsReady()` returned False, and ResultFinalizer skips
updating the Ready value, resulting in a fully reconciled final object
status with Reconciling and Ready both True, which is an incorrect
reconciliation status result.
ResultFinalizer now uses `conditions.IsTrue(obj, meta.ReadyCondition)`
to precisely check if only Ready=True is present.

Also, remove ExternalEventRecorder from ImageRepositoryReconciler, which
is no longer used and add a missing return on progressive status
patching failure.

Signed-off-by: Sunny <[email protected]>
This allows using the condition checker as a test helper with proper
test like assertion failure and stacktrace.

Signed-off-by: Sunny <[email protected]>
@hiddeco hiddeco added the bug Something isn't working label Mar 20, 2023
@darkowlzz darkowlzz merged commit eab02e9 into main Mar 20, 2023
@darkowlzz darkowlzz deleted the debug-success-error-356 branch March 20, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants