Fix finalizer processing #3180
Merged
Fix finalizer processing #3180
+117
−120
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
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
This is an improvement, but it's not perfect... there's still a race condition. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
tmshort
force-pushed
the
finalizer-fix
branch
from
941887a to
127d7ed
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
tmshort
force-pushed
the
finalizer-fix
branch
from
127d7ed to
aaed167
Compare
There are two reconcilers for the CSV, one is controller-runtime based, the other is based on queueinformer. A finalizer was added to the CSV and it's handled by the controller-runtime reconciler. However, the queueinformer-based reconciler may take a while to do its processing such that the CSV may be deleted and the finalizer run via the controller-runtime reconciler. (This still takes <1s.) This causes problems when CSV being synced is deleted. The queueinformer attempts to sync RBAC to the stale CSV. The proper (BIG/risky) fix is to consolidate the two reconcilers. A less risky fix is to move the finalizer to the queueinformer reconciler and query the lister cache to see if the CSV has been deleted, and not do the RBAC updates if that's the case. Signed-off-by: Todd Short <[email protected]>
tmshort
force-pushed
the
finalizer-fix
branch
from
aaed167 to
a237f2f
Compare
kevinrizza
reviewed
Feb 27, 2024
| @@ -1093,6 +1095,9 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) { | |||
| "phase": clusterServiceVersion.Status.Phase, | |||
| }) | |||
|
|
|||
| logger.Debug("start deleting CSV") | |||
There was a problem hiding this comment.
They are useful in tracking what's happening; this one is debug, so it doesn't clutter the log normally.
There was a problem hiding this comment.
Are you asking me to get rid of the extra logs?
| return nil, false | ||
| } | ||
|
|
||
| log.Info("started finalizer") |
There was a problem hiding this comment.
This one is Info, however.
| @@ -100,10 +95,6 @@ func (r *OperatorConditionGeneratorReconciler) Reconcile(ctx context.Context, re | |||
| return ctrl.Result{}, client.IgnoreNotFound(err) | |||
| } | |||
|
|
|||
| if err, ok := r.processFinalizer(ctx, in); !ok { | |||
There was a problem hiding this comment.
Do you know why we were doing this here?
There was a problem hiding this comment.
At the time, it was the easier location to put the finalizer, since it was the controller-runtime reconciler. But that reconciler didn't manage the RBAC, which caused this race condition. Moving it to the queueinformer reconciler is the proper place for it. It's easier than combining the reconcilers.
|
ping @kevinrizza ? |
|
/approve |
kevinrizza
approved these changes
Feb 28, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Merged
via the queue into
operator-framework:master
with commit Feb 28, 2024
fc3c183
15 checks passed
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.
Description of the change:
Handle CSV deletion during sync.
Motivation for the change:
There are two reconcilers for the CSV, one is controller-runtime based,
the other is based on queueinformer. A finalizer was added to the CSV
and it's handled by the controller-runtime reconciler.
However, the queueinformer-based reconciler may take a while to do its
processing such that the CSV may be deleted and the finalizer run via
the controller-runtime reconciler. (This still takes <1s.)
This causes problems when CSV being synced is deleted. The queueinformer
attempts to sync RBAC to the stale CSV. The proper (BIG/risky) fix is to
consolidate the two reconcilers. A less risky fix is to query the lister
cache to see if the CSV has been deleted (or has a DeletionTimestamp),
and not do the RBAC updates if that's the case.
Architectural changes:
None.
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue