Fix error logs with stale success message #357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
darkowlzz
force-pushed
the
debug-success-error-356
branch
from
fd22b49
to
b89a023
Compare
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 <darkowlzz@protonmail.com>
This allows using the condition checker as a test helper with proper test like assertion failure and stacktrace. Signed-off-by: Sunny <darkowlzz@protonmail.com>
darkowlzz
force-pushed
the
debug-success-error-356
branch
from
b89a023
to
c612504
Compare
hiddeco
approved these changes
Mar 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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()whichalso 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 skipsupdating 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