Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CSV Finalizer #3148

Merged
merged 1 commit into from
Jan 11, 2024
Merged

CSV Finalizer #3148

merged 1 commit into from
Jan 11, 2024

Conversation

tmshort
Copy link
Contributor

@tmshort tmshort commented Jan 5, 2024

Fix #3143
Add a finalizer for CSVs for cleanup, rather than the existing method.

Description of the change:

Motivation for the change:

Architectural changes:

Testing remarks:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jan 5, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jan 5, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2024
@tmshort
Copy link
Contributor Author

tmshort commented Jan 5, 2024

Some of my own commentary:

  • This just handles items that need to be cleaned up when a CSV is deleted
  • This uses the Reconciler API vs the "Syncer" API, and could be the start of transitioning to the more modern API?
  • Subsequently, if that's the case, then CleanupCsvReconciler could be renamed to ClusterServiceVersionReconciler or similar, to note it's not just for cleanup any more.
  • Looks as though the github linter (‽‽) is flagging some code, so this is not yet ready for prime time.

@tmshort
Copy link
Contributor Author

tmshort commented Jan 5, 2024

  • I have not added a test file for the new reconciler, either.

stevekuznetsov
stevekuznetsov reviewed Jan 5, 2024
View reviewed changes
cmd/olm/manager.go Outdated
@@ -123,6 +123,19 @@ func Manager(ctx context.Context, debug bool) (ctrl.Manager, error) {
return nil, err
}

cleanupCsvReconciler, err := operators.NewCleanupCsvReconciler(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @joelanford and @awgreene on this - while I understand why using the controller-runtime approach may be appealing here, it might be premature to move. It's a duplicate in-memory cache of all of these objects, which is a crying shame, and to make the deletion logic robust we'd need to add listers for mutating and validating admission webhooks, which moves us in the wrong direction. Then again, getting this right in the byzantine framework from v0 may be more work than it's worth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strongly based on (i.e. copied from) adoption_controller.go, which already uses controller-runtime and reconciles CSVs already... BUT is only used when a specific feature flag is enabled. So, my original plan to piggy back on an existing CSV reconciler (there are two already!), was abandoned, and I created a new one. The controller-runtime seems to be easier to use/manage, especially with finalizers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the ease of coding is certainly on the side of the controller-runtime approach - my concern is with duplicating the memory footprint of those informers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood; the reconciller in operatorconditiongenerator_controller.go already reconciles the CSV, so the impact should be minimal. And I could probably move the code into there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either one is missing listers for some of these resources and will have the same downside. @joelanford do we care?

pkg/controller/operators/olm/operator.go
@@ -948,98 +913,6 @@ func (a *Operator) EnsureCSVMetric() error {
return nil
}

func (a *Operator) syncGCObject(obj interface{}) (syncError error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: events on these objects by definition cannot be triggers for cleanup, so losing the cross-queueing should in theory not make any difference

@tmshort
Copy link
Contributor Author

tmshort commented Jan 7, 2024

Thanks @stevekuznetsov for your comments!

@tmshort tmshort force-pushed the finalizer branch 2 times, most recently from 07dec70 to 057af8c Compare January 9, 2024 17:00
@tmshort
Add finalizer for CSV cleanup
a473403 
Remove old CSV cleanup code
Cleanup CRB/CR/MWC/VWC via finalizer.

Signed-off-by: Todd Short <[email protected]>
@tmshort tmshort marked this pull request as ready for review January 10, 2024 18:17
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2024
@stevekuznetsov
Copy link
Contributor

@tmshort can you dupe the issue / create one for the InstallPlan work since this PR will close #3143 but not implement that half?

@stevekuznetsov stevekuznetsov added this pull request to the merge queue Jan 11, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 11, 2024
@tmshort
Add finalizer for CSV cleanup (#3148)
2c3721f 
Remove old CSV cleanup code
Cleanup CRB/CR/MWC/VWC via finalizer.

Signed-off-by: Todd Short <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 11, 2024
@tmshort
Copy link
Contributor Author

tmshort commented Jan 11, 2024 •
edited
Loading

@tmshort can you dupe the issue / create one for the InstallPlan work since this PR will close #3143 but not implement that half?

Yes. #3154

@tmshort tmshort added this pull request to the merge queue Jan 11, 2024
@tmshort tmshort changed the title Finalizer CSV Finalizer Jan 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 11, 2024
@stevekuznetsov stevekuznetsov added this pull request to the merge queue Jan 11, 2024
Merged via the queue into operator-framework:master with commit f94a5ed Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevekuznetsov stevekuznetsov stevekuznetsov left review comments

@dinhxuanvu dinhxuanvu Awaiting requested review from dinhxuanvu

@joelanford joelanford Awaiting requested review from joelanford

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CSV finalizers to OLM
2 participants
@tmshort @stevekuznetsov