Consider bipolarity conditions in Ready condition summarization #907
Conversation
darkowlzz
added
area/git
|
could we instead compare the |
This introduces the consideration of bipolarity conditions in the status condition summary for Ready condition. The summarize.HelperOptions can now be configured with a list of bipolarity conditions which are used in SummarizeAndPatch() to set the Ready condition to failing bipolarity condition with the highest priority. Bipolarity condition is not a typical status property. It is a mix of positive and negative polarities. It's "normal-true" and "abnormal-false". Failing bipolarity conditions are prioritized over other conditions to show the actual reason of failure on the Ready status. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Use the bipolarity condition options in OCIRepository and GitRepository reconcilers. Signed-off-by: Sunny <darkowlzz@protonmail.com>
Signed-off-by: Sunny <darkowlzz@protonmail.com>
darkowlzz
force-pushed
the
summarize-with-bipolarity
branch
from
d3f2575 to
90b7cec
Compare
My concern was about deleting staled verified conditions. After discussing it further this now make sense to me. |
There was a problem hiding this comment.
Given this solves the issue I am happy to get this in as-is. 👍
For possible inclusion in the runtime package we need to have another chat though. As already discussed in private I have concerns about introducing another name for the type of condition. I would like to understand the problem as a whole better to either see if the complexity of most of our controllers have outgrown working with a framework like this, and in my latest changes to the helm-controller I have sensed that it might be better to design them specifically for the object you are working with. Easing working with the conditional polarity of certain types based on e.g. the current specification. For the simpler API objects (from e.g. notification-controller), this might not be the best option however.
Thanks 🙇
This change can be applied without #876 but is meant to fix Ready status condition discrepancies when bipolarity conditions, like SourceVerified, is False. It fixes the same issue in GitRepository reconciler for commit verification.
Bipolarity condition is not a typical status property. It is a mix of positive and negative polarities. It's "normal-true" and "abnormal-false". With this change, failing bipolarity conditions are prioritized over other conditions to show the actual reason of failure on the Ready status.
An example of bipolar condition is SourceVerified. When it's verified/normal, its value is True. When it's unverified/fails/abnormal, its value is False.
Problem
Without special consideration to the bipolarity conditions, when a reconciliation fails, it also leaves the other negative conditions on the status which are usually removed on successful reconciliation. Negative conditions being of the highest priority are set as the value of Ready condition. Example of the status in this scenario:
In the above scenario, the Ready condition shows the reason for not being Ready as
new digest ... for .... This value of Ready doesn't indicate the actual reason for the failure, which is the source verification failure. Because we couldn't verify the source, new artifact wasn't created.Solution
In order to fix this, failing bipolarity conditions need to be prioritized over any other reconciling conditions.
The changes in internal/reconcile/summarize/summary.go fix this issue by evaluating the value of bipolar conditions in the status. With this, the above scenario becomes:
The Ready condition now shows the actual reason for the failure
failed to verify the signature using provider....Now, since bipolarity gets the highest priority when it's False, even in presence of other negative conditions, this could results in scenarios like the following:
In this, Ready shows that the failure is due to verification failure, but FetchFailed condition shows that the actual reason for the failure is an authentication failure. Since bipolar False conditions are of the highest priority, their presence need to be handled better. The above situation is due to a stale SourceVerified=False condition from the previous reconciliation.
For the record, the same is seen in GitRepository with commit verification enabled:
A way to solve this is to remove any stale failing bipolarity condition at the beginning of the reconciliation and let it be recalculated if it's still failing. But leave any stale True bipolarity conditions as they don't have high priority.
Result of this fix is:
The same examples apply to GitRepository reconciler. Both OCIRepository and GitRepository reconcilers have also been updated to handle these concerns.