OCPBUGS-98601: Add --force-cleanup-on-timeout flag to destroy command#9013
OCPBUGS-98601: Add --force-cleanup-on-timeout flag to destroy command#9013hypershift-jira-solve-ci[bot] wants to merge 3 commits into
Conversation
When the grace period expires during `hypershift destroy cluster`, the CLI currently aborts with an error, leaving stale HostedClusters with unremoved finalizers. On CI management clusters this creates a vicious cycle: accumulated stale control-plane pods consume resources, causing API timeouts that prevent the deprovisioner from cleaning them up. Add a --force-cleanup-on-timeout flag that, when enabled, force-removes finalizers from the HostedCluster and its child resources (HCP, CAPI Cluster, MachineDeployments, MachineSets, Machines) in the control plane namespace after the grace period expires, then continues with platform-specific cleanup instead of aborting. The flag defaults to false to preserve existing interactive behavior. Signed-off-by: OpenShift CI Bot <ci-bot@redhat.com>
…eness Address review findings for the --force-cleanup-on-timeout feature: - Preserve the CLI's destroyFinalizer on the HostedCluster during force cleanup to prevent premature garbage collection before destroyPlatformSpecifics completes - Add NodePool cleanup to forceCleanupFinalizers — NodePools live in the management namespace (not the CP namespace) and were previously missed - Add retry.RetryOnConflict with optimistic locking to removeFinalizersFromObject, matching the existing removeFinalizer pattern - Return aggregated errors from forceCleanupFinalizers so the caller can detect and log partial failures - Only trigger force cleanup on context.DeadlineExceeded, not on user cancellation (Ctrl+C) - Use table-driven pattern for CP namespace resource cleanup to reduce duplication - Improve logging: prefer GVK kind over Go %T pointer types, log preserved finalizers - Strengthen --force-cleanup-on-timeout flag description with orphaned cloud resource warning - Add tests for: preserved destroyFinalizer, NodePool filtering by cluster name, preserve parameter, no-op when only preserved finalizers remain Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98601, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe destroy command adds Sequence Diagram(s)sequenceDiagram
participant ClusterCLI
participant destroyCluster
participant KubernetesAPI
ClusterCLI->>destroyCluster: enable force cleanup
destroyCluster->>KubernetesAPI: wait for HostedCluster deletion
KubernetesAPI-->>destroyCluster: deadline exceeded
destroyCluster->>KubernetesAPI: list related resources
destroyCluster->>KubernetesAPI: remove selected finalizers
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/cli
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot] The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-98601, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_test.go`:
- Line 96: Rename each added t.Run description in the destroy tests, including
the subtests near the referenced cases, to follow the required “When ..., it
should ...” format. Preserve each test’s existing behavior and intent while
making every description begin with “When” and state the expected outcome with
“it should”.
- Line 95: Add unit tests around DestroyCluster covering the timeout branch,
including flag-disabled behavior, deadline versus cancellation handling,
continuation after platform cleanup, and propagation of cleanup errors. Extend
the existing TestForceCleanupFinalizers test area or related DestroyCluster
tests, using focused cases that verify each outcome.
In `@cmd/cluster/core/destroy.go`:
- Around line 156-160: Update the timeout branch in the destroy flow around
forceCleanupFinalizers so a non-nil cleanupErr is retained instead of only
logged. Continue platform cleanup, then return the retained error before the
success logging/return path, while preserving normal success behavior when
forced cleanup succeeds.
🪄 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: 8cd33f27-93c8-4960-82ed-e14050b76b98
📒 Files selected for processing (3)
cmd/cluster/cluster.gocmd/cluster/core/destroy.gocmd/cluster/core/destroy_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9013 +/- ##
==========================================
+ Coverage 44.11% 44.13% +0.01%
==========================================
Files 772 772
Lines 96226 96319 +93
==========================================
+ Hits 42448 42507 +59
- Misses 50832 50858 +26
- Partials 2946 2954 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| } | ||
| } else { | ||
| return err | ||
| } |
There was a problem hiding this comment.
question: If force cleanup partially fails (say, the HC's own finalizer wasn't removed because the API server was flaky), destroyPlatformSpecifics still runs and then removeFinalizer tries to remove destroyFinalizer. The HC could end up with stale non-destroy finalizers and no controller to process them. Should partial failure at least be reflected in the exit code?
There was a problem hiding this comment.
Done. forceCleanupErr is now retained and returned after platform-specific cleanup completes, so partial failure is reflected in the exit code.
AI-assisted response via Claude Code
| g.Expect(err).To(HaveOccurred()) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: Test names should follow the project's Gherkin convention ("When... it should...") and the TestRemoveFinalizersFromObject subtests share identical structure — good candidate for a table-driven test. Examples:
"should remove finalizers from child resources..."→"When child resources have finalizers, it should remove them and preserve destroyFinalizer on HostedCluster""should be a no-op when object has no finalizers"→"When object has no finalizers, it should be a no-op"
There was a problem hiding this comment.
Done. Renamed all 8 test descriptions to use the "When... it should..." convention.
AI-assisted response via Claude Code
| g.Expect(c.Get(ctx, client.ObjectKeyFromObject(obj), &updated)).To(Succeed()) | ||
| g.Expect(updated.Finalizers).To(ConsistOf(destroyFinalizer)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
suggestion: The integration point in DestroyCluster (the ForceCleanupOnTimeout && errors.Is(err, context.DeadlineExceeded) branch) has no test coverage. Three scenarios worth adding:
ForceCleanupOnTimeout=true+DeadlineExceeded→ force cleanup runsForceCleanupOnTimeout=true+context.Canceled(Ctrl+C) → error returned, no force cleanupForceCleanupOnTimeout=false+DeadlineExceeded→ error returned, no force cleanup
The existing TestDestroyCluster uses FAKE_CLIENT env var which could be extended for this.
There was a problem hiding this comment.
Done. Added TestDestroyClusterForceCleanup covering all 3 requested scenarios. Extracted an internal destroyCluster function accepting client.Client to enable testing with a pre-populated fake client.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
Address review feedback: - Propagate forceCleanupFinalizers errors to the caller instead of silently logging success. The error is retained and returned after platform-specific cleanup completes. - Extract destroyCluster internal function accepting a client.Client parameter to enable integration testing without FAKE_CLIENT. - Rename all test descriptions to "When... it should..." Gherkin convention. - Add TestDestroyClusterForceCleanup with three scenarios: ForceCleanupOnTimeout+DeadlineExceeded, context.Canceled bypass, and flag-disabled behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-98601, which is valid. 3 validation(s) were run on this bug
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. |
|
@hypershift-jira-solve-ci[bot]: 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:
When the grace period expires during
hypershift destroy cluster, the CLI currently aborts with an error, leaving stale HostedClusters with unremoved finalizers. On CI management clusters this creates a vicious cycle: accumulated stale control-plane pods consume resources, causing API timeouts that prevent the deprovisioner from cleaning them up.This PR adds a
--force-cleanup-on-timeoutflag that, when enabled, force-removes finalizers from the HostedCluster and its child resources (HCP, CAPI Cluster, MachineDeployments, MachineSets, Machines, NodePools) in the control plane namespace after the grace period expires, then continues with platform-specific cleanup instead of aborting.Key behaviors:
falseto preserve existing interactive behaviordestroyFinalizeron the HostedCluster during force cleanup to prevent premature garbage collection beforedestroyPlatformSpecificscompletesretry.RetryOnConflictwith optimistic locking, matching the existingremoveFinalizerpatterncontext.DeadlineExceeded, not on user cancellation (Ctrl+C)Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-98601
Special notes for your reviewer:
The second commit hardens the initial implementation based on review findings: preserved destroyFinalizer logic, NodePool cleanup, retry-on-conflict, improved logging (GVK kind over Go
%T), and comprehensive tests.Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit
New Features
--force-cleanup-on-timeoutoption for cluster deletion.Tests