ACM-11453 Fix flaky subscription constraints not satisfiable condition #258
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
JustinKuli
force-pushed
the
11453-help-sub-find-csv
branch
11 times, most recently
from
b242866 to
ce197ba
Compare
JustinKuli
force-pushed
the
11453-help-sub-find-csv
branch
from
ce197ba to
aebb208
Compare
|
Weird flakes? 😭
Attempt 1
/home/runner/work/config-policy-controller/config-policy-controller/test/e2e/case23_invalid_field_test.go:76 |
|
@mprahl this now has the change you requested in Slack, to only update a smaller part of the Subscription status. |
mprahl
reviewed
Jun 5, 2024
| @@ -216,14 +219,14 @@ func (r *OperatorPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque | |||
| errs = append(errs, err) | |||
| } | |||
|
|
|||
| if err := r.Status().Update(ctx, policy); err != nil { | |||
There was a problem hiding this comment.
Could we avoid always updating the status if nothing changed? This could increase API server hits a lot. Maybe a reflect.DeepEqual comparing before and after would suffice.
There was a problem hiding this comment.
I can add the DeepEqual, I'm surprised controller-runtime (or whatever) wouldn't do that for us...
For extra context, the cause of this was the intervention timestamp not being in a condition. I felt like we were dealing with confusing cases where the Subscription status wasn't always updated correctly by OLM, I wanted to be more paranoid about making sure our status was accurate.
mprahl
reviewed
Jun 5, 2024
mprahl
reviewed
Jun 5, 2024
| return mergedSub, nil, changed, nil | ||
| } | ||
|
|
||
| csvIdx := 0 |
There was a problem hiding this comment.
Wouldn't this always be 0 because of if len(relatedCSVs) != 1 {?
There was a problem hiding this comment.
The RelatedObjsOfKind gives back a map 😅 I thought it would be a good idea because then it's easy to update the condition in-place... I mostly stand by that, but it does make this usage look odd.
Usually index 0 is the CatalogSource.
mprahl
reviewed
Jun 5, 2024
|
|
||
| updateStatus(policy, updatedCond("Subscription"), updatedObj(mergedSub)) | ||
|
|
||
| return mergedSub, nil, true, nil |
There was a problem hiding this comment.
Should policy.Status.SubscriptionInterventionTime be set to empty after the update is successful and before returning?
There was a problem hiding this comment.
Originally I had this just apply the intervention once, then wait another 30 seconds again before trying again. What I found in testing was that about 5% of the time, OLM would then add a different condition, remove our update, and I think confuse itself... I still don't know what exactly was happening there, but the new implementation will now immediately re-apply the intervention in that case, as long as 10 seconds haven't passed. And it requires not clearing the timestamp until those 10 seconds are up.
There was a problem hiding this comment.
The weirdest thing looking back at it is that after it got into that weird situation of confusing itself, the controller would re-apply the intervention 30 seconds later, but OLM seemed to do the same weird thing consistently this time (not a 5% chance like I'd expect). It wasn't just a bad sync of timing, because I tried a different grace period and it would still happen.
JustinKuli
force-pushed
the
11453-help-sub-find-csv
branch
from
aebb208 to
d0db499
Compare
mprahl
previously approved these changes
Jun 5, 2024
After the changes suggested in the review, the failure was a repeat of one of the previous "flakes":
/home/runner/work/config-policy-controller/config-policy-controller/test/e2e/case38_install_operator_test.go:3232 |
|
First 2 runs passed completely. The third run had this during hosted mode tests, which is odd:
/home/runner/work/config-policy-controller/config-policy-controller/test/e2e/case38_install_operator_test.go:2663 |
JustinKuli
force-pushed
the
11453-help-sub-find-csv
branch
from
2e51945 to
5e795c6
Compare
mprahl
reviewed
Jun 6, 2024
| @@ -216,14 +221,16 @@ func (r *OperatorPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque | |||
| errs = append(errs, err) | |||
| } | |||
|
|
|||
| if !reflect.DeepEqual(policy.Status, originalStatus) { | |||
There was a problem hiding this comment.
Optional optimization:
Suggested change
| if !reflect.DeepEqual(policy.Status, originalStatus) { | |
| if conditionChanged || !reflect.DeepEqual(policy.Status, originalStatus) { |
mprahl
previously approved these changes
Jun 6, 2024
|
@JustinKuli looks like you need to rebase. I'll approve it again afterwards. |
Previously, the selector used to find OLM resources like InstallPlans and CSVs was not exactly what OLM would use in situations where the name and namespace of the operator was very long. It should now match the behavior of OLM. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
Some places were not using the logger provided in the Reconcile context, which has some potentially helpful information. Logs were added to record API calls like create/update/delete, and some debug logs were added for potentially complex methods. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
Previously, the controller needed to parse and sort the message associated with the ConstraintsNotSatisfiable condition on the Subscription, because the order of certain parts ot the message were inconsistent. The intent was to not update the OperatorPolicy condition when the actual situation on the cluster was unchanged. In some situations, particularly when the subscription is deleted and quickly recreated, the condition on the Subscription does not just have parts in an inconsistent order: it can alternate between two different clauses. To prevent constant updates to the OperatorPolicy status, it now just reports a generic "conditions not satisfiable" condition, and directs the user to the Subscription for more information. This also reduces noise in the compliance events be removing the lists of operator versions. Refs: - https://issues.redhat.com/browse/ACM-11453 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
In some situations when actions are happening quickly, OLM can lose the connection between the Subscription and the ClusterServiceVersion, which leads to a "constraints not satisfiable" condition on the Subscription. It can be hard to reproduce the exact situation we've seen rarely in our tests, but manually deleting and immediately recreating the subscription causes a similar situation. In this change, in those situations, the controller will intervene after 30 seconds by updating the Subscription status directly. The implementation allows for a 10 second window for the intervention, during which the controller may update the status multiple times, to address a case where OLM immediately overwrote the update. If the window is missed, the controller may schedule another time. This is intended to give time to OLM to potentially resolve the situation on its own. Refs: - https://issues.redhat.com/browse/ACM-11453 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
force-pushed
the
11453-help-sub-find-csv
branch
from
5e795c6 to
88c37db
Compare
mprahl
approved these changes
Jun 6, 2024
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl 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 |
openshift-merge-bot
bot
merged commit 4b0fd44
into
open-cluster-management-io:main
9 checks passed
JustinKuli
added a commit
to JustinKuli/governance-policy-addon-controller-1
that referenced
this pull request
Jun 7, 2024
Syncs changes from open-cluster-management-io/config-policy-controller#258 Refs: - https://issues.redhat.com/browse/ACM-11453 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
openshift-merge-bot bot
pushed a commit
to open-cluster-management-io/governance-policy-addon-controller
that referenced
this pull request
Jun 7, 2024
Syncs changes from open-cluster-management-io/config-policy-controller#258 Refs: - https://issues.redhat.com/browse/ACM-11453 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
magic-mirror-bot bot
pushed a commit
to stolostron/governance-policy-addon-controller
that referenced
this pull request
Jun 7, 2024
Syncs changes from open-cluster-management-io/config-policy-controller#258 Refs: - https://issues.redhat.com/browse/ACM-11453 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com> (cherry picked from commit d89fa6dad5f88886ef1b602980fd27ecb6a71170)
magic-mirror-bot bot
pushed a commit
to stolostron/governance-policy-addon-controller
that referenced
this pull request
Jun 7, 2024
Syncs changes from open-cluster-management-io/config-policy-controller#258 Refs: - https://issues.redhat.com/browse/ACM-11453 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com> (cherry picked from commit d89fa6dad5f88886ef1b602980fd27ecb6a71170)
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.
See each commit for details