🌱 CER: centralize status updates into big-R Reconcile method #2200
Conversation
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
joelanford
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2200 +/- ##
==========================================
+ Coverage 72.05% 72.09% +0.03%
==========================================
Files 85 85
Lines 8332 8372 +40
==========================================
+ Hits 6004 6036 +32
- Misses 1926 1932 +6
- Partials 402 404 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if err := c.removeFinalizer(ctx, rev, clusterExtensionRevisionTeardownFinalizer); err != nil { | ||
| meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{ | ||
| Type: "Available", | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "ReconcileFailure", | ||
| Message: err.Error(), | ||
| ObservedGeneration: rev.Generation, | ||
| }) | ||
| return ctrl.Result{}, fmt.Errorf("error removing teardown finalizer: %v", err) | ||
| } | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
It seems semantically wrong to mark a Revision as "Available" == False for any issue with finalizers.
In general I would not report any internal error as a Status Condition.
If we want to show individual errors to users we should publish events on the resource.
There was a problem hiding this comment.
Totally agree that this is likely the wrong type for this. I would expect to come back and clean this up prior to GA once there is a formal design on status handling.
In the meantime, I do think it is an important UX concern to tell the user something in a place they are likely to see it.
If they've deleted a CER (or an agent has on their behalf), but the finalizer isn't being removed for some reason, it would be a bad to UX for the CER to just... have no update. It would look like we just aren't reconciling it.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort 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-ci
bot
added
the
approved
|
/lgtm |
openshift-merge-bot
bot
merged commit f512e1e
into
operator-framework:main
Re-opening here now that poc-boxcutter PR is merged
Original PR: thetechnick#5
Description
A few small fixups and comment additions to help remember follow-ups:
Reconcileandreconcilemore consistent between CE and CER reconcilersReconcilereconcileNOTE: There is still one major difference: CER updates finalizers as separate patches and continues reconcile logic. CE updates finalizers and requires re-reconcile to continue beyond. The CER logic may cause issues where the next reconcile sees a stale resource version (from finalizer patch) because the status update event isn't seen by our informer in time. This ultimately leads to reconciling a stale CER that manifests as a conflict error in our logs that is often a red herring for users trying to troubleshoot issues in controllers.
Another noticeable difference is that CE uses controller-runtime Finalizers helpers, and CER handles things manually.
Changing this broke unit tests, so I decided not to leave that change out to keep the PR scope small. I'll make a separate PR that focuses just on finalizers.
Reviewer Checklist