Skip to content

COO-1404: fix: remove ui plugin finalizers in favor of k8s garbage collection#1084

Open
jgbernalp wants to merge 1 commit intorhobs:mainfrom
jgbernalp:remove-ui-plugins-finalizers
Open

COO-1404: fix: remove ui plugin finalizers in favor of k8s garbage collection#1084
jgbernalp wants to merge 1 commit intorhobs:mainfrom
jgbernalp:remove-ui-plugins-finalizers

Conversation

@jgbernalp
Copy link
Copy Markdown
Member

This PR:

  • Removes the finalizers added to ui plugins to use regular k8s garbage collection as the ConsolePlugin resources have owner references to the UIPlugins. This avoids dangling resources when the operator is uninstalled and does not have time to cleanup the finalizers.
  • Removes finalizers form existing resources, for example during upgrades
  • Executes a best effort console plugins removal when the manager is being terminated

Signed-off-by: Gabriel Bernal <gbernal@redhat.com>
@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented May 6, 2026

@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.

Details

In response to this:

This PR:

  • Removes the finalizers added to ui plugins to use regular k8s garbage collection as the ConsolePlugin resources have owner references to the UIPlugins. This avoids dangling resources when the operator is uninstalled and does not have time to cleanup the finalizers.
  • Removes finalizers form existing resources, for example during upgrades
  • Executes a best effort console plugins removal when the manager is being terminated

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.

@openshift-ci openshift-ci Bot requested review from simonpasquier and zhuje May 6, 2026 16:26
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 6, 2026

[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

Details 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 label May 6, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 6, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 521c9d8a-88a1-4e4e-9de0-9ad6141903c5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The 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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly describes the main change: removing UI plugin finalizers in favor of Kubernetes garbage collection, which aligns with the file modifications across multiple files.
Description check ✅ Passed The PR description accurately relates to the changeset, detailing the removal of finalizers, cleanup of existing resources, and best-effort console plugin removal on manager termination.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@DavidRajnoha
Copy link
Copy Markdown
Collaborator

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 a55377c and confirmed by matching the shutdown-cleanup logger in cluster event logs.

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

  • Removing the finalizer eliminates stuck-terminating UIPlugins during normal deletion.
  • The OwnerReference chain is set up correctly — deleting a UIPlugin CR cascade-deletes all children (Deployments, Services, RBAC).
  • Uninstall via OpenShift Console UI (with "delete all operands" checked) works fully: the UI issues DELETE on the UIPlugin CRs, which triggers the cascade, and the operator handles Console deregistration while still alive.

Remaining gap: CLI / automated uninstall

When uninstalling via CLI (oc delete csv, oc delete sub) or any path that doesn't explicitly delete UIPlugin CRs first, the cleanup doesn't happen because nobody issues the initial DELETE on the UIPlugin CR — the OwnerReference cascade never starts.

The shutdown handler also can't compensate. OLM deletes the operator's RBAC before the SIGTERM handler runs:

2026-05-07T13:59:22Z  ERROR  shutdown-cleanup  failed to list UIPlugins during shutdown cleanup
  {"error": "uiplugins.observability.openshift.io is forbidden: User
  \"system:serviceaccount:openshift-cluster-observability-operator:observability-operator-sa\"
  cannot list resource \"uiplugins\" in API group \"observability.openshift.io\" at the cluster scope"}

What's left orphaned depends on whether the namespace is deleted:

If only CSV+Sub are deleted (namespace preserved):

  • All namespaced resources remain: Deployments, Services, ServiceAccounts, running pods
  • All cluster-scoped resources remain: UIPlugin CR, ConsolePlugin, ClusterRoles, ClusterRoleBindings
  • Console spec.plugins retains stale entry — UI tabs still visible

If the namespace is also deleted:

  • Namespaced resources are cleaned up (gone with the namespace)
  • Cluster-scoped resources still orphaned: UIPlugin CR, ConsolePlugin, ClusterRoles, ClusterRoleBindings (6 resources in total for a monitoring+health-analyzer setup)
  • Console spec.plugins gets cleaned up (observed in testing — likely reconciled by the Console operator)

Possible follow-up

An 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, --postpone-restoration flag support in run-e2e.sh) that haven't been pushed yet.

@jgbernalp
Copy link
Copy Markdown
Member Author

@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/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants