COO-1404: fix: remove ui plugin finalizers in favor of k8s garbage collection#1084
COO-1404: fix: remove ui plugin finalizers in favor of k8s garbage collection#1084jgbernalp wants to merge 1 commit intorhobs:mainfrom
Conversation
Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
|
@jgbernalp: This pull request references COO-1404 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes enhance UIPlugin cleanup and deletion handling across the codebase. A kubectl patch command in the deployment script is removed and replaced with a comment. The UIPlugin controller's deletion handling is rewritten to include a new private helper method for safely removing finalizers before early reconciliation return. A new exported helper function maps plugin types to console names. The Operator struct gains a restConfig field and new methods that register and execute UI plugin deregistration from the console during shutdown, utilizing direct Kubernetes client interactions. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @jgbernal, I tested this PR against OCP 4.21 using a dedicated uninstall e2e test (PR #1083). The image was built from your commit This PR fixes the UI uninstall. The finalizer removal + OwnerReference approach is correct and works well for the primary uninstall path (OpenShift console UI with "delete operands" selected). There are some remaining gaps for secondary uninstall paths that could be addressed as follow-up work. What works
Remaining gap: CLI / automated uninstallWhen uninstalling via CLI ( The shutdown handler also can't compensate. OLM deletes the operator's RBAC before the SIGTERM handler runs: What's left orphaned depends on whether the namespace is deleted: If only CSV+Sub are deleted (namespace preserved):
If the namespace is also deleted:
Possible follow-upAn OLM pre-delete Job with its own ServiceAccount could cover the CLI path — running before teardown to delete all UIPlugin CRs (triggering the OwnerRef cascade this PR sets up) and deregistering from Console. The e2e test in PR #1083 reproduces the CLI uninstall scenario. Note: the test currently in #1083 is missing some recent improvements (log capture, |
|
@DavidRajnoha I don't think direct deletion of the csv and/or subscription are paths we should cover, this should be delegated to cluster admins. See https://olm.operatorframework.io/docs/tasks/uninstall-operator/ |
This PR: