CNTRLPLANE-3793: enhance HyperShift teardown to prevent AWS resource leaks - #81788
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request references CNTRLPLANE-3793 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 story 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. |
|
@jparrill: GitHub didn't allow me to request PR reviews from the following users: openshift/hypershift-team. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughHyperShift pruning now attempts graceful destruction before forced Kubernetes and AWS cleanup. The script records pruner failures and combines them with deprovisioning failures to determine its final exit status. ChangesHyperShift pruning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Deprovisioner
participant KubernetesAPI
participant AWS
participant HyperShiftCLI
Deprovisioner->>KubernetesAPI: Enumerate stale HostedClusters
Deprovisioner->>HyperShiftCLI: Attempt graceful cluster destroy
Deprovisioner->>KubernetesAPI: Delete resources and strip finalizers
Deprovisioner->>AWS: Terminate infraID-tagged EC2 instances
Deprovisioner->>HyperShiftCLI: Destroy AWS infrastructure and IAM
Deprovisioner->>Deprovisioner: Combine cleanup and deprovisioning status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-merge-bot
Bot
added
the
rehearsals-ack
![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)
core-services/ipi-deprovision/aws.sh (1)
103-126: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftSequential processing could make the pruner run very long with multiple stale clusters.
Each iteration of the graceful-destroy loop can take up to 30 minutes, and each force-cleanup iteration can take up to ~60+ minutes (2m wait + 30m infra destroy + 30m iam destroy). Both loops (lines 103-121, 123-126) process clusters sequentially rather than using the
queue()helper already established in this file (lines 16-24) for concurrency. With several stale/stuck HostedClusters accumulating, this could make the periodic pruning job run for many hours, risking job-level timeouts.Consider parallelizing cluster processing similar to the
queue/file-flag pattern used bydeprovision()(e.g., signal per-cluster success/failure via temp files under a scratch dir, then aggregate).🤖 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 `@core-services/ipi-deprovision/aws.sh` around lines 103 - 126, Parallelize both the graceful-destroy loop and the subsequent hypershift_force_cleanup loop using the existing queue() helper and scratch-file success/failure signaling pattern from deprovision(). Preserve per-cluster namespace/name handling, aggregate failed clusters after queued destruction completes, and continue updating had_failure for force-cleanup failures without processing clusters sequentially.
🤖 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 `@core-services/ipi-deprovision/aws.sh`:
- Line 257: Quote the $FAILED expansion in the xargs command within the
deprovisioning flow to satisfy SC2086 and prevent unintended glob expansion,
while preserving xargs’ existing whitespace-based splitting behavior.
- Around line 81-85: Update hypershift_force_cleanup so failures from the
timeout-wrapped hypershift destroy infra and destroy iam commands are preserved
instead of being suppressed by || true. Capture and propagate a nonzero status
from either cleanup command while still attempting both operations, so callers
such as hypershift_pruner can increment had_failure and surface the final
failure status.
---
Nitpick comments:
In `@core-services/ipi-deprovision/aws.sh`:
- Around line 103-126: Parallelize both the graceful-destroy loop and the
subsequent hypershift_force_cleanup loop using the existing queue() helper and
scratch-file success/failure signaling pattern from deprovision(). Preserve
per-cluster namespace/name handling, aggregate failed clusters after queued
destruction completes, and continue updating had_failure for force-cleanup
failures without processing clusters sequentially.
🪄 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: 0310a592-ea41-45d1-ac2c-cf74496b8c50
📒 Files selected for processing (1)
core-services/ipi-deprovision/aws.sh
|
/approve |
openshift-ci
Bot
added
the
approved
eb2c1ae to
f620bf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core-services/ipi-deprovision/aws.sh (1)
71-79: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider waiting for EC2 termination before running infra destroy.
aws ec2 terminate-instancesis asynchronous; proceeding straight tohypershift destroy infra aws(line 84) without confirming instances are gone can cause spurious failures if VPC/subnet/security-group teardown requires instances to be fully terminated.♻️ Proposed fix
if [[ -n "${instance_ids}" ]]; then echo " Terminating EC2 instances: ${instance_ids}" aws ec2 terminate-instances --region "${region}" --instance-ids ${instance_ids} || true + aws ec2 wait instance-terminated --region "${region}" --instance-ids ${instance_ids} || true fi[reliability]
🤖 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 `@core-services/ipi-deprovision/aws.sh` around lines 71 - 79, Update the orphaned EC2 cleanup flow around the terminate-instances call to wait until every instance in instance_ids reaches the terminated state before proceeding to hypershift destroy infra aws. Reuse the existing region and instance ID values, and ensure the destroy command is not reached while termination is still pending.
🤖 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 `@core-services/ipi-deprovision/aws.sh`:
- Around line 39-44: Wrap every remaining oc invocation in strip_finalizers and
hypershift_force_cleanup with the existing timeout mechanism, including oc get,
oc patch, oc delete, and oc wait calls. Preserve the current arguments, cleanup
flow, and tolerated-failure behavior while ensuring stalled management-cluster
API calls cannot block indefinitely.
- Around line 52-57: Remove the guessed defaults for infra_id and region in the
deprovisioning flow. Make the HostedCluster lookups fail explicitly when either
value is missing or the oc command fails, before the AWS tag filtering and
hypershift destroy operations use them; preserve the existing values from the
HostedCluster for successful lookups.
---
Nitpick comments:
In `@core-services/ipi-deprovision/aws.sh`:
- Around line 71-79: Update the orphaned EC2 cleanup flow around the
terminate-instances call to wait until every instance in instance_ids reaches
the terminated state before proceeding to hypershift destroy infra aws. Reuse
the existing region and instance ID values, and ensure the destroy command is
not reached while termination is still pending.
🪄 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: 35b9e01a-e900-4ee1-936d-0e4e04c8fb9c
📒 Files selected for processing (1)
core-services/ipi-deprovision/aws.sh
| function strip_finalizers() { | ||
| local resource="${1}" ns="${2}" | ||
| oc get "${resource}" -n "${ns}" --no-headers -o custom-columns=NAME:.metadata.name 2>/dev/null | while read -r obj; do | ||
| [[ -n "${obj}" ]] && oc patch "${resource}" "${obj}" -n "${ns}" -p '{"metadata":{"finalizers":null}}' --type=merge 2>/dev/null || true | ||
| done | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='core-services/ipi-deprovision/aws.sh'
echo '--- map structure ---'
ast-grep outline "$file" --view expanded
echo '--- relevant line ranges ---'
sed -n '1,170p' "$file" | cat -n
echo '--- search for timeout usage ---'
rg -n 'timeout|oc get hostedcluster|oc delete hostedcluster|oc patch|oc wait|strip_finalizers|hypershift_force_cleanup' "$file"Repository: openshift/release
Length of output: 10703
Add timeouts around the remaining oc calls in core-services/ipi-deprovision/aws.sh:39-69. strip_finalizers and the oc get/oc delete/oc wait calls in hypershift_force_cleanup are still unbounded, while the surrounding hypershift destroy calls already use timeout. If the management-cluster API stalls, cleanup can hang indefinitely here.
🤖 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 `@core-services/ipi-deprovision/aws.sh` around lines 39 - 44, Wrap every
remaining oc invocation in strip_finalizers and hypershift_force_cleanup with
the existing timeout mechanism, including oc get, oc patch, oc delete, and oc
wait calls. Preserve the current arguments, cleanup flow, and tolerated-failure
behavior while ensuring stalled management-cluster API calls cannot block
indefinitely.
| local infra_id region | ||
| infra_id="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.infraID}' 2>/dev/null || echo "")" | ||
| region="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.platform.aws.region}' 2>/dev/null || echo "")" | ||
| : "${infra_id:=${hc_name}}" | ||
| : "${region:=us-east-1}" | ||
| echo " infraID=${infra_id} region=${region} hcp_ns=${hcp_ns}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guessed infra_id/region fallback can silently no-op the cleanup instead of failing.
If the oc get hostedcluster lookups fail or return empty, infra_id falls back to hc_name and region falls back to us-east-1. Since infra_id/region drive both the EC2 tag filter (line 74) and the direct hypershift destroy infra/iam calls (84-85), a wrong guess makes those AWS calls simply find nothing and return success — leaking exactly the resources this PR is meant to catch, without force_rc ever reflecting the failure.
🐛 Proposed fix
local infra_id region
infra_id="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.infraID}' 2>/dev/null || echo "")"
region="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.platform.aws.region}' 2>/dev/null || echo "")"
- : "${infra_id:=${hc_name}}"
- : "${region:=us-east-1}"
+ if [[ -z "${infra_id}" || -z "${region}" ]]; then
+ echo " ERROR: could not determine infraID/region for ${hc_ns}/${hc_name}; skipping direct AWS cleanup to avoid targeting wrong resources" >&2
+ return 1
+ fi
echo " infraID=${infra_id} region=${region} hcp_ns=${hcp_ns}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local infra_id region | |
| infra_id="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.infraID}' 2>/dev/null || echo "")" | |
| region="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.platform.aws.region}' 2>/dev/null || echo "")" | |
| : "${infra_id:=${hc_name}}" | |
| : "${region:=us-east-1}" | |
| echo " infraID=${infra_id} region=${region} hcp_ns=${hcp_ns}" | |
| local infra_id region | |
| infra_id="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.infraID}' 2>/dev/null || echo "")" | |
| region="$(oc get hostedcluster -n "${hc_ns}" "${hc_name}" -o jsonpath='{.spec.platform.aws.region}' 2>/dev/null || echo "")" | |
| if [[ -z "${infra_id}" || -z "${region}" ]]; then | |
| echo " ERROR: could not determine infraID/region for ${hc_ns}/${hc_name}; skipping direct AWS cleanup to avoid targeting wrong resources" >&2 | |
| return 1 | |
| fi | |
| echo " infraID=${infra_id} region=${region} hcp_ns=${hcp_ns}" |
🤖 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 `@core-services/ipi-deprovision/aws.sh` around lines 52 - 57, Remove the
guessed defaults for infra_id and region in the deprovisioning flow. Make the
HostedCluster lookups fail explicitly when either value is missing or the oc
command fails, before the AWS tag filtering and hypershift destroy operations
use them; preserve the existing values from the HostedCluster for successful
lookups.
|
This one makes me a little hesitant, lots of hard to test logic in the CI scripts which has a large blast radius if it goes wrong, and is leaking knowledge of state that hypershift owns the tracking of (e.g. should this be incorporated into the CLI somehow?) This class of tooling might be safer with openshift/hypershift#8909? |
|
cc @csrwng |
|
/auto-cc |
There was a problem hiding this comment.
If you strip finalizers, then some cleanup will not happen (like deleting the control plane namespace), I think we need to be a little smarter with this fast deletion. One option would be to simply remove the finalizer from any awsmachine resource once they have a deletion timestamp (and only after they have a deletion timestamp).
As a next step, I would remove the finalizer from the hcp resource only after it has a deletion timestamp.
For this last part, we need to ensure that any left over volumes, load balancers, s3 buckets are properly cleaned up.
There was a problem hiding this comment.
Good call. Changed strip_finalizers to only act on resources that already have a deletionTimestamp — it now filters with jq before patching:
oc get "${resource}" -n "${ns}" -o json | \
jq -r '.items[] | select(.metadata.deletionTimestamp != null) | .metadata.name' | \
while read -r obj; do ...Additionally, the force cleanup now explicitly deletes HCP namespace resources (--wait=false) before stripping finalizers, so they always have a deletionTimestamp set. This preserves normal cleanup ordering — finalizers are only removed to unblock stuck deletions, not to bypass cleanup.
f620bf0 to
d3426b3
Compare
There was a problem hiding this comment.
I think that just as with the rest of the script, using json output would be more appropriate. Maybe something like (I also changed instance_ids to be a shell array to be safer (and shellcheck compliant) with word splitting.
| local -a instance_ids | |
| readarray -t instance_ids < <(aws ec2 describe-instances --region "${region}" \ | |
| --filters "Name=tag:kubernetes.io/cluster/${infra_id},Values=owned" "Name=instance-state-name,Values=running,pending,stopping,stopped" \ | |
| --query 'Reservations[].Instances[].InstanceId' --output json 2>/dev/null | jq -r '.[]' 2>/dev/null || true) | |
| if [[ ${#instance_ids[@]} -gt 0 && -n "${instance_ids[0]}" ]]; then | |
| echo " Terminating EC2 instances: ${instance_ids[*]}" | |
| aws ec2 terminate-instances --region "${region}" --instance-ids "${instance_ids[@]}" || true |
There was a problem hiding this comment.
Done — switched to --output json with jq and a shell array (readarray) for safe word splitting. Thanks for the suggestion.
262ffd3 to
a82772c
Compare
|
Reworked per your feedback — force_cleanup now uses tiered finalizer removal based on the HC deletion age. The pruner cron runs every 15min, so each invocation evaluates how long the HC has been deleting and escalates one tier:
Finalizers are only removed from resources that already have a deletionTimestamp (unchanged from previous push). AWS infra/IAM cleanup runs on every invocation regardless of tier. |
|
@jparrill: |
a82772c to
0ca96df
Compare
…er removal Restructure the HyperShift pruner in the ipi-deprovision script to prevent AWS resource leaks when graceful destroy times out. When hypershift destroy cluster fails, fall back to hypershift_force_cleanup which uses tiered finalizer removal based on how long the HC has been in deletion. The pruner cron runs every 15min, so each invocation evaluates the deletion age and runs only the highest applicable tier, avoiding wasted time on lower-level cleanup: - Tier 1 (>=1hr): strip awsmachine finalizers + terminate EC2 instances - Tier 2 (>=2hr): strip HCP + CAPI resource finalizers - Tier 3 (>=3hr): delete CP namespace if not already deleting - Tier 4 (>=4hr): strip HC and NodePool finalizers (last resort) Finalizers are only removed from resources that already have a deletionTimestamp set (not blindly). AWS infra/IAM cleanup runs on every invocation regardless of tier. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…region Three bugs found during live testing of the tiered teardown: 1. strip_finalizers namespace-wide blast radius: At Tier 4, strip_finalizers stripped finalizers from ALL HCs and NodePools in the namespace, not just the target cluster. This caused other HCs to be garbage-collected before their own force-cleanup could run. Fix: add optional 3rd name parameter to strip_finalizers, and scope T4 to only the target HC and its NodePools (filtered by spec.clusterName). 2. infraID/region loss: When an HC disappears between graceful destroy and force-cleanup, the script fell back to HC name as infraID and us-east-1 as region, causing infra/IAM cleanup to target the wrong resources. Fix: pre-capture infraID and region before graceful destroy, pack them into the failed_clusters array, and pass them through to hypershift_force_cleanup. 3. hypershift_force_cleanup now accepts optional $3/$4 for pre-captured infraID/region, only querying the HC if they are not provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
30533e1 to
cfc1861
Compare
|
/label acknowledge-critical-fixes-only |
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files ( Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, deepsm007, jparrill 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 |
…leaks (openshift#81788) * fix(ipi-deprovision): enhance HyperShift teardown with tiered finalizer removal Restructure the HyperShift pruner in the ipi-deprovision script to prevent AWS resource leaks when graceful destroy times out. When hypershift destroy cluster fails, fall back to hypershift_force_cleanup which uses tiered finalizer removal based on how long the HC has been in deletion. The pruner cron runs every 15min, so each invocation evaluates the deletion age and runs only the highest applicable tier, avoiding wasted time on lower-level cleanup: - Tier 1 (>=1hr): strip awsmachine finalizers + terminate EC2 instances - Tier 2 (>=2hr): strip HCP + CAPI resource finalizers - Tier 3 (>=3hr): delete CP namespace if not already deleting - Tier 4 (>=4hr): strip HC and NodePool finalizers (last resort) Finalizers are only removed from resources that already have a deletionTimestamp set (not blindly). AWS infra/IAM cleanup runs on every invocation regardless of tier. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * fix(ipi-deprovision): scope finalizer stripping and preserve infraID/region Three bugs found during live testing of the tiered teardown: 1. strip_finalizers namespace-wide blast radius: At Tier 4, strip_finalizers stripped finalizers from ALL HCs and NodePools in the namespace, not just the target cluster. This caused other HCs to be garbage-collected before their own force-cleanup could run. Fix: add optional 3rd name parameter to strip_finalizers, and scope T4 to only the target HC and its NodePools (filtered by spec.clusterName). 2. infraID/region loss: When an HC disappears between graceful destroy and force-cleanup, the script fell back to HC name as infraID and us-east-1 as region, causing infra/IAM cleanup to target the wrong resources. Fix: pre-capture infraID and region before graceful destroy, pack them into the failed_clusters array, and pass them through to hypershift_force_cleanup. 3. hypershift_force_cleanup now accepts optional $3/$4 for pre-captured infraID/region, only querying the HC if they are not provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Cesar Wong <cewong@redhat.com>
…leaks (openshift#81788) * fix(ipi-deprovision): enhance HyperShift teardown with tiered finalizer removal Restructure the HyperShift pruner in the ipi-deprovision script to prevent AWS resource leaks when graceful destroy times out. When hypershift destroy cluster fails, fall back to hypershift_force_cleanup which uses tiered finalizer removal based on how long the HC has been in deletion. The pruner cron runs every 15min, so each invocation evaluates the deletion age and runs only the highest applicable tier, avoiding wasted time on lower-level cleanup: - Tier 1 (>=1hr): strip awsmachine finalizers + terminate EC2 instances - Tier 2 (>=2hr): strip HCP + CAPI resource finalizers - Tier 3 (>=3hr): delete CP namespace if not already deleting - Tier 4 (>=4hr): strip HC and NodePool finalizers (last resort) Finalizers are only removed from resources that already have a deletionTimestamp set (not blindly). AWS infra/IAM cleanup runs on every invocation regardless of tier. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * fix(ipi-deprovision): scope finalizer stripping and preserve infraID/region Three bugs found during live testing of the tiered teardown: 1. strip_finalizers namespace-wide blast radius: At Tier 4, strip_finalizers stripped finalizers from ALL HCs and NodePools in the namespace, not just the target cluster. This caused other HCs to be garbage-collected before their own force-cleanup could run. Fix: add optional 3rd name parameter to strip_finalizers, and scope T4 to only the target HC and its NodePools (filtered by spec.clusterName). 2. infraID/region loss: When an HC disappears between graceful destroy and force-cleanup, the script fell back to HC name as infraID and us-east-1 as region, causing infra/IAM cleanup to target the wrong resources. Fix: pre-capture infraID and region before graceful destroy, pack them into the failed_clusters array, and pass them through to hypershift_force_cleanup. 3. hypershift_force_cleanup now accepts optional $3/$4 for pre-captured infraID/region, only querying the HC if they are not provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Cesar Wong <cewong@redhat.com>
![openshift-prow[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…leaks (openshift#81788) * fix(ipi-deprovision): enhance HyperShift teardown with tiered finalizer removal Restructure the HyperShift pruner in the ipi-deprovision script to prevent AWS resource leaks when graceful destroy times out. When hypershift destroy cluster fails, fall back to hypershift_force_cleanup which uses tiered finalizer removal based on how long the HC has been in deletion. The pruner cron runs every 15min, so each invocation evaluates the deletion age and runs only the highest applicable tier, avoiding wasted time on lower-level cleanup: - Tier 1 (>=1hr): strip awsmachine finalizers + terminate EC2 instances - Tier 2 (>=2hr): strip HCP + CAPI resource finalizers - Tier 3 (>=3hr): delete CP namespace if not already deleting - Tier 4 (>=4hr): strip HC and NodePool finalizers (last resort) Finalizers are only removed from resources that already have a deletionTimestamp set (not blindly). AWS infra/IAM cleanup runs on every invocation regardless of tier. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * fix(ipi-deprovision): scope finalizer stripping and preserve infraID/region Three bugs found during live testing of the tiered teardown: 1. strip_finalizers namespace-wide blast radius: At Tier 4, strip_finalizers stripped finalizers from ALL HCs and NodePools in the namespace, not just the target cluster. This caused other HCs to be garbage-collected before their own force-cleanup could run. Fix: add optional 3rd name parameter to strip_finalizers, and scope T4 to only the target HC and its NodePools (filtered by spec.clusterName). 2. infraID/region loss: When an HC disappears between graceful destroy and force-cleanup, the script fell back to HC name as infraID and us-east-1 as region, causing infra/IAM cleanup to target the wrong resources. Fix: pre-capture infraID and region before graceful destroy, pack them into the failed_clusters array, and pass them through to hypershift_force_cleanup. 3. hypershift_force_cleanup now accepts optional $3/$4 for pre-captured infraID/region, only querying the HC if they are not provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Cesar Wong <cewong@redhat.com>
…leaks (openshift#81788) * fix(ipi-deprovision): enhance HyperShift teardown with tiered finalizer removal Restructure the HyperShift pruner in the ipi-deprovision script to prevent AWS resource leaks when graceful destroy times out. When hypershift destroy cluster fails, fall back to hypershift_force_cleanup which uses tiered finalizer removal based on how long the HC has been in deletion. The pruner cron runs every 15min, so each invocation evaluates the deletion age and runs only the highest applicable tier, avoiding wasted time on lower-level cleanup: - Tier 1 (>=1hr): strip awsmachine finalizers + terminate EC2 instances - Tier 2 (>=2hr): strip HCP + CAPI resource finalizers - Tier 3 (>=3hr): delete CP namespace if not already deleting - Tier 4 (>=4hr): strip HC and NodePool finalizers (last resort) Finalizers are only removed from resources that already have a deletionTimestamp set (not blindly). AWS infra/IAM cleanup runs on every invocation regardless of tier. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * fix(ipi-deprovision): scope finalizer stripping and preserve infraID/region Three bugs found during live testing of the tiered teardown: 1. strip_finalizers namespace-wide blast radius: At Tier 4, strip_finalizers stripped finalizers from ALL HCs and NodePools in the namespace, not just the target cluster. This caused other HCs to be garbage-collected before their own force-cleanup could run. Fix: add optional 3rd name parameter to strip_finalizers, and scope T4 to only the target HC and its NodePools (filtered by spec.clusterName). 2. infraID/region loss: When an HC disappears between graceful destroy and force-cleanup, the script fell back to HC name as infraID and us-east-1 as region, causing infra/IAM cleanup to target the wrong resources. Fix: pre-capture infraID and region before graceful destroy, pack them into the failed_clusters array, and pass them through to hypershift_force_cleanup. 3. hypershift_force_cleanup now accepts optional $3/$4 for pre-captured infraID/region, only querying the HC if they are not provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Cesar Wong <cewong@redhat.com>
6 participants
Summary
core-services/ipi-deprovision/aws.shto prevent AWS resource leaks whenhypershift destroy cluster awshangs on stuck finalizerstimeout 30m+--cluster-grace-period 15mto the graceful destroy path (currently has neither)exit $had_failurewith return-code tracking so the VPC expiration cleanup always runsFixes
Problem
When
hypershift destroy cluster awshangs (stuck finalizers, degraded controller), the script exits early and the VPC expiration-date cleanup section never runs — leaving orphaned VPCs, EC2 instances, NAT gateways, IAM roles, and OIDC providers in AWS.Changes
Three new functions added:
strip_finalizers()— clears all finalizers from resources of a given type in a namespacehypershift_force_cleanup()— for each stuck HC: extracts metadata, strips finalizers from 9 resource types (machine, awsmachine, machineset, machinedeployment, cluster, awsendpointservice, hostedcontrolplane, nodepool, hostedcluster), terminates orphaned EC2 instances, runshypershift destroy infra/iam awsdirectly via AWS APIhypershift_pruner()— replaces the inline block: graceful destroy with timeout, falls back to force-cleanup on failure, returns rc instead of callingexitThe OIDC deadlock concern from the original code is resolved because force-cleanup strips finalizers before deletion (no controller reconciliation needed) and infra/IAM cleanup uses the AWS API directly.
Finalizer list verified against a live management cluster and the HyperShift source code.
Test plan
bash -n aws.sh— syntax validation passesjparrill-dev) with HCjparrill-hosted(us-west-1)/cc @openshift/hypershift-team
🤖 Generated with Claude Code
Summary by CodeRabbit
Updates the OpenShift CI AWS deprovisioning teardown for HyperShift in
core-services/ipi-deprovision/aws.shto prevent AWS resource leaks whenhypershift destroy cluster awsstalls on stuck finalizers or degraded HostedCluster controllers.In practice, the script now:
hypershift destroy cluster awscapped at 30 minutes and--cluster-grace-period 15m.hypershift_force_cleanupescalates based on HostedCluster deletion age (minutes sincemetadata.deletionTimestampis set), running only the highest applicable tier per invocation:awsmachineandMachinefinalizers; terminate matching EC2 instances taggedkubernetes.io/cluster/${infraID}=owned.hostedcontrolplane,cluster,machinedeployment, etc.).nodepool+ theHostedCluster.oc patch ... -p '{"metadata":{"finalizers":null}}'.hypershift destroy infra awsandhypershift destroy iam aws(each 30m timeout) using the derivedinfraIDand AWS region (andHYPERSHIFT_BASE_DOMAINdefault).HostedClusterobjects either cluster-wide (HYPERSHIFT_PRUNER_ALL_NAMESPACES) or within theclustersnamespace.hypershift_pruner_rc) and sets the script’sfinal_rcat the end, after the VPC deprovisioning work and other cleanup steps, instead of exiting early.The change includes syntax validation and local testing; CI rehearsal remains pending.