CNTRLPLANE-3943: feat(destroy): add --force flag to strip finalizers on grace period expiry - #9134
CNTRLPLANE-3943: feat(destroy): add --force flag to strip finalizers on grace period expiry#9134bryan-cox wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@bryan-cox: This pull request references CNTRLPLANE-3943 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. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox 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 |
openshift-ci
Bot
added
approved
📝 WalkthroughWalkthroughThe cluster destroy command adds a Sequence Diagram(s)sequenceDiagram
participant Operator
participant DestroyCluster
participant KubernetesAPI
participant HostedClusterResources
Operator->>DestroyCluster: Run destroy --force
DestroyCluster->>KubernetesAPI: Wait for remaining finalizers
KubernetesAPI-->>DestroyCluster: Wait fails
DestroyCluster->>HostedClusterResources: Strip child-resource finalizers
HostedClusterResources->>KubernetesAPI: Patch resources
DestroyCluster->>KubernetesAPI: Delete control-plane namespace
KubernetesAPI-->>DestroyCluster: Cleanup result
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/cluster/core/destroy_test.go (1)
314-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
ForceDestroycontrol-flow and list-error paths.Only the no-match tolerance case is exercised. Consider adding: (1) a genuine
Listfailure asserting the error is returned, and (2) aDestroyCluster/returnOrForceLogcase verifying that platform-cleanup and destroy-finalizer failures are logged-and-continued withForceDestroy: truebut returned when false — that conditional is the core of this PR and is currently untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/cluster/core/destroy_test.go` around lines 314 - 331, Expand TestStripFinalizersFromList with a genuine List failure case that asserts the error is returned, while preserving the existing NoKindMatchError success case. Add coverage around DestroyCluster and returnOrForceLog to verify platform-cleanup and destroy-finalizer failures are logged and execution continues with ForceDestroy true, but are returned when ForceDestroy is false.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/cluster/core/destroy.go`:
- Around line 295-318: Make forceRemoveAllFinalizers platform-agnostic by
stripping finalizers from the provider-specific infrastructure machine and
cluster resources for the selected platform, not only capzv1.AzureMachineList
and AzureClusterList. Reuse the existing platform/provider selection mechanism
so AWS, OpenStack, KubeVirt, PowerVS, and GCP force-destroy paths unblock their
control-plane namespaces while preserving the current bottom-up processing
order.
- Around line 344-359: Update the control-plane namespace cleanup in the destroy
flow to clear namespace finalizers through the namespace finalize subresource,
including spec.finalizers, rather than relying only on stripFinalizers. Preserve
error aggregation, and log “Deleted control plane namespace” only when c.Delete
succeeds; treat IsNotFound as a no-op without logging success.
---
Nitpick comments:
In `@cmd/cluster/core/destroy_test.go`:
- Around line 314-331: Expand TestStripFinalizersFromList with a genuine List
failure case that asserts the error is returned, while preserving the existing
NoKindMatchError success case. Add coverage around DestroyCluster and
returnOrForceLog to verify platform-cleanup and destroy-finalizer failures are
logged and execution continues with ForceDestroy true, but are returned when
ForceDestroy is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 27098d98-1b5b-46c7-833d-f93a75de49da
📒 Files selected for processing (3)
cmd/cluster/cluster.gocmd/cluster/core/destroy.gocmd/cluster/core/destroy_test.go
bbf06af to
3030955
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9134 +/- ##
==========================================
+ Coverage 44.51% 44.57% +0.06%
==========================================
Files 774 774
Lines 96997 97120 +123
==========================================
+ Hits 43179 43295 +116
+ Misses 50830 50825 -5
- Partials 2988 3000 +12
... and 6 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…xpiry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3030955 to
a47428c
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@bryan-cox: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What this PR does / why we need it:
Adds a
--forceflag tohypershift destroy clusterthat, when the grace period expires, force-removes all finalizers from child resources and continues with best-effort infrastructure cleanup. This fixes the Azure CI deprovisioner which fails to clean up stuck HostedClusters when the 7-layer finalizer chain gets blocked by Azure API failures.Without this fix, orphaned HCs accumulate on the CI cluster (37 were found stuck with
deletionTimestampset but finalizers never removed), consuming cluster resources until manual intervention.Key behaviors:
--forceis set and the grace period expires, finalizers are stripped bottom-up: AzureMachines → AzureClusters → CAPI Machines → CAPI Clusters → HostedControlPlane → Deployments → NodePools → HostedCluster (preserving the destroy finalizer) → Namespacemeta.IsNoMatchErrorremoveFinalizer()can run after platform cleanupWhich issue(s) this PR fixes:
Fixes CNTRLPLANE-3943
Special notes for your reviewer:
returnOrForceLoghelper reduces the repeatedif !o.ForceDestroy { return err }pattern acrossDestroyClusterforceRemoveAllFinalizerscurrently hardcodes Azure CAPZ types — this is intentional since the feature targets the Azure CI deprovisioner. Other platform types can be added later if needed.Checklist:
Summary by CodeRabbit
--forceoption to cluster destruction.