Add VPC cleanup safety net for Classic STS conformance - #82824
Conversation
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe pull request adds a ROSA VPC cleanup CI step. The step finds cluster-tagged orphaned VPCs, removes dependent AWS resources, and deletes each VPC. The STS conformance workflow runs the cleanup as a best-effort post action. ChangesROSA VPC cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
@dustman9000: |
![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
`@ci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.sh`:
- Around line 3-4: Update the shell options at the start of the cleanup command
script to use set -euo pipefail, preserving nounset and pipefail while enabling
errexit. If any cleanup command is intentionally allowed to fail, handle its
status explicitly rather than disabling errexit globally.
- Around line 24-26: Update the cleanup flow before the AWS VPC lookup and
mutation represented by vpc_ids so it first confirms the cluster’s ROSA or OCM
deletion state. Treat a failed or unreadable describe result as an error, not as
an empty/deleted state; proceed only for an explicit terminal deletion state.
Otherwise skip VPC cleanup and report the state-read or non-terminal failure.
- Around line 24-26: Update the cleanup flow for delete-vpc-endpoints, load
balancer deletion, and delete-vpc to stop suppressing failures and retain
affected resource IDs. Track endpoint deletion failures, poll until dependent
resources are gone using a bounded deadline instead of relying on the fixed
sleep, and retry delete-vpc during that period. When the deadline expires,
report unresolved resource IDs and return a nonzero status; otherwise preserve
successful cleanup behavior.
🪄 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: 9670296d-3f02-4df1-a72c-9e583858023a
📒 Files selected for processing (5)
ci-operator/step-registry/rosa/aws/sts/conformance/rosa-aws-sts-conformance-workflow.yamlci-operator/step-registry/rosa/cluster/cleanup-vpc/OWNERSci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.shci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-ref.metadata.jsonci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-ref.yaml
| set -o nounset | ||
| set -o pipefail |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use set -euo pipefail as the default.
If a cleanup command must continue after an error, handle that command status explicitly. Do not remove -e globally.
As per coding guidelines, step registry command scripts must default to set -euo pipefail without -x.
🤖 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
`@ci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.sh`
around lines 3 - 4, Update the shell options at the start of the cleanup command
script to use set -euo pipefail, preserving nounset and pipefail while enabling
errexit. If any cleanup command is intentionally allowed to fail, handle its
status explicitly rather than disabling errexit globally.
Source: Coding guidelines
| vpc_ids=$(aws ec2 describe-vpcs \ | ||
| --filters "Name=tag:api.openshift.com/name,Values=${CLUSTER_NAME}" \ | ||
| --query 'Vpcs[].VpcId' --output text 2>/dev/null || true) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f '^rosa-aws-sts-deprovision' ci-operator/step-registry
rg -n -C 4 'rosa delete|delete cluster|cluster-name|SHARED_DIR|deletion' ci-operator/step-registry/rosaRepository: openshift/release
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target cleanup script ---'
cat -n ci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.sh
printf '%s\n' '--- related cleanup metadata and references ---'
fd -t f 'rosa-cluster-cleanup-vpc|rosa-aws-sts-deprovision|rosa-teardown' ci-operator/step-registry/rosa \
-x sh -c 'echo "--- $1"; cat -n "$1"' sh
printf '%s\n' '--- references to target cleanup step ---'
rg -n -C 5 'rosa-cluster-cleanup-vpc|cleanup-vpc' ci-operator/step-registry/rosaRepository: openshift/release
Length of output: 33726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cluster deprovision chain and components ---'
fd -t f 'rosa-cluster-deprovision|cluster-deprovision' ci-operator/step-registry/rosa \
-x sh -c 'echo "--- $1"; cat -n "$1"' sh
printf '%s\n' '--- ROSA/OCM cluster deletion and status operations ---'
rg -n -C 5 --glob '*.sh' --glob '*.yaml' --glob '*.json' \
'rosa (delete|describe|get)|ocm .*clusters|cluster.*(delete|state|status)|DELETE|destroy' \
ci-operator/step-registry/rosa/cluster ci-operator/step-registry/rosa/aws ci-operator/step-registry/rosa/e2eRepository: openshift/release
Length of output: 50373
Gate VPC cleanup on a confirmed cluster deletion state.
The best-effort deprovision step can fail, and its failed rosa describe call is converted to an empty state that it treats as deletion. Before any AWS mutation, require an explicit terminal ROSA or OCM deletion state. If the state cannot be read or is not terminal, skip cleanup and report the failure.
🤖 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
`@ci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.sh`
around lines 24 - 26, Update the cleanup flow before the AWS VPC lookup and
mutation represented by vpc_ids so it first confirms the cluster’s ROSA or OCM
deletion state. Treat a failed or unreadable describe result as an error, not as
an empty/deleted state; proceed only for an explicit terminal deletion state.
Otherwise skip VPC cleanup and report the state-read or non-terminal failure.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
script='ci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.sh'
rg -n -C 3 'delete-vpc-endpoints|delete-load-balancer|sleep 30|delete-vpc|2>/dev/null \|\| true' "${script}"Repository: openshift/release
Length of output: 6535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
script='ci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.sh'
printf '%s\n' '--- script ---'
cat -n "${script}"
printf '%s\n' '--- related files ---'
git ls-files 'ci-operator/step-registry/rosa/cluster/cleanup-vpc/*' 'ci-operator/step-registry/rosa/cluster/*' | sed -n '1,120p'
printf '%s\n' '--- callers and step metadata ---'
rg -n -C 3 'rosa-cluster-cleanup-vpc|cleanup-vpc|VPC cleanup complete|orphaned VPCs' ci-operator | sed -n '1,240p'Repository: openshift/release
Length of output: 27895
🌐 Web query:
AWS CLI delete-vpc-endpoints Unsuccessful response endpoint deletion asynchronous delete-load-balancer wait delete-vpc dependencies documentation
💡 Result:
When using the AWS CLI to delete VPC endpoints or load balancers, it is important to understand that these operations involve asynchronous backend processes and specific resource dependencies [1][2][3]. AWS CLI delete-vpc-endpoints The delete-vpc-endpoints command deletes specified VPC endpoints [1]. If a batch deletion is performed, the command returns a list of any unsuccessful items [1]. If the command succeeds, this returned list is empty [1]. Common causes for unsuccessful deletion include: - Requester-Managed Endpoints: These are created and managed by other AWS services (e.g., RDS Proxy, ElastiCache Serverless) [4][3]. You cannot delete them directly; you must identify and delete the underlying AWS resource that created them, which will then trigger the automatic deletion of the endpoint [4][3]. - Dependencies: For Gateway Load Balancer endpoints, you must ensure all associated routes are deleted before the endpoint can be deleted [1]. - NotFound Errors: If you receive an InvalidVpcEndpoint.NotFound error, the endpoint may have already been cleaned up by the service that created it, often as a result of deleting the parent resource [3]. AWS CLI delete-load-balancer Deleting a load balancer is an operation that may not be instantaneous [2]. While the command initiates the deletion, it is best practice to use a waiter to confirm the state of the resource if you have subsequent dependent actions [5]. The AWS CLI provides a specific waiter command for this purpose: aws elbv2 wait load-balancers-deleted --load-balancer-arns This command polls the load balancer status every 15 seconds until it confirms the load balancer is deleted (or reaches a maximum number of attempts) [2][6]. Using this waiter is essential in scripts where you intend to delete related resources (like target groups or security groups) immediately after a load balancer, as it prevents errors caused by attempting to delete resources that are still in use by the load balancer [5]. General Guidance When cleaning up VPC resources, you must remove them in the correct dependency order [7]. If you encounter a DependencyViolation error, it indicates that other resources (like network interfaces or active services) are still associated with the component you are trying to delete [8]. Always prioritize deleting the parent application or service first, allowing the backend to clean up managed network interfaces and endpoints automatically [4][3].
Citations:
- 1: https://docs.aws.amazon.com/cli/latest/reference/ec2/delete-vpc-endpoints.html
- 2: https://awscli.amazonaws.com/v2/documentation/api/2.8.7/reference/elbv2/wait/load-balancers-deleted.html
- 3: https://repost.aws/questions/QU7yedQF1TTfum4gikIkInKA/can-not-delete-vpc-endpoint-does-not-exist
- 4: https://repost.aws/knowledge-center/vpc-delete-requester-managed-endpoint
- 5: https://docs.aws.amazon.com/cli/v1/userguide/bash_elastic-load-balancing-v2_code_examples.html
- 6: https://awscli.amazonaws.com/v2/documentation/api/2.0.33/reference/elbv2/wait/load-balancers-deleted.html
- 7: https://docs.aws.amazon.com/vpc/latest/userguide/delete-vpc.html
- 8: https://repost.aws/knowledge-center/troubleshoot-dependency-error-delete-vpc
Fail cleanup when dependent resources remain.
delete-vpc-endpoints, load balancer deletion, and delete-vpc suppress errors and discard responses. The fixed 30-second sleep does not prove that dependent resources were deleted. Track failed endpoint deletions, poll dependencies with a bounded deadline, retry delete-vpc, and return nonzero with unresolved resource IDs when the deadline expires.
🤖 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
`@ci-operator/step-registry/rosa/cluster/cleanup-vpc/rosa-cluster-cleanup-vpc-commands.sh`
around lines 24 - 26, Update the cleanup flow for delete-vpc-endpoints, load
balancer deletion, and delete-vpc to stop suppressing failures and retain
affected resource IDs. Track endpoint deletion failures, poll until dependent
resources are gone using a bounded deadline instead of relying on the fixed
sleep, and retry delete-vpc during that period. When the deadline expires,
report unresolved resource IDs and return a nonzero status; otherwise preserve
successful cleanup behavior.
Source: MCP tools
8938c5b to
81dcbf7
Compare
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-rosa-sts-ovn |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@dustman9000, |
Classic STS conformance jobs (4.19-4.22) are failing at 17% pass rate due to NAT Gateway quota exhaustion in us-east-2. Root cause: when rosa delete cluster fails (cluster in error state), the deprovision step exits without cleaning up ROSA-managed VPC resources, orphaning NAT Gateways that accumulate across runs. Fix: - Mark rosa-aws-sts-deprovision as best_effort so post steps always run even when cluster deletion fails - Add rosa-cluster-cleanup-vpc step that finds orphaned VPCs by api.openshift.com/name tag and force-deletes all resources (NAT GWs, EIPs, ENIs, subnets, route tables, IGWs, security groups) This aligns the STS conformance workflow with the HCP conformance workflow which already has best_effort and cleanup safety nets.
81dcbf7 to
0559a80
Compare
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse periodic-ci-openshift-release-main-nightly-4.22-e2e-rosa-sts-ovn |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse abort |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse ack |
|
@dustman9000: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000, jfrazierRedHat 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 |
e545c41
into
openshift:main
![openshift-prow[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Classic STS conformance jobs (4.19-4.22) are failing at 17% pass rate due to NAT Gateway quota exhaustion in us-east-2. Root cause: when rosa delete cluster fails (cluster in error state), the deprovision step exits without cleaning up ROSA-managed VPC resources, orphaning NAT Gateways that accumulate across runs. Fix: - Mark rosa-aws-sts-deprovision as best_effort so post steps always run even when cluster deletion fails - Add rosa-cluster-cleanup-vpc step that finds orphaned VPCs by api.openshift.com/name tag and force-deletes all resources (NAT GWs, EIPs, ENIs, subnets, route tables, IGWs, security groups) This aligns the STS conformance workflow with the HCP conformance workflow which already has best_effort and cleanup safety nets.
Classic STS conformance jobs (4.19-4.22) are failing at 17% pass rate due to NAT Gateway quota exhaustion in us-east-2. Root cause: when rosa delete cluster fails (cluster in error state), the deprovision step exits without cleaning up ROSA-managed VPC resources, orphaning NAT Gateways that accumulate across runs. Fix: - Mark rosa-aws-sts-deprovision as best_effort so post steps always run even when cluster deletion fails - Add rosa-cluster-cleanup-vpc step that finds orphaned VPCs by api.openshift.com/name tag and force-deletes all resources (NAT GWs, EIPs, ENIs, subnets, route tables, IGWs, security groups) This aligns the STS conformance workflow with the HCP conformance workflow which already has best_effort and cleanup safety nets.
Classic STS conformance jobs (4.19-4.22) are failing at 17% pass rate due to NAT Gateway quota exhaustion in us-east-2. Root cause: when rosa delete cluster fails (cluster in error state), the deprovision step exits without cleaning up ROSA-managed VPC resources, orphaning NAT Gateways that accumulate across runs. Fix: - Mark rosa-aws-sts-deprovision as best_effort so post steps always run even when cluster deletion fails - Add rosa-cluster-cleanup-vpc step that finds orphaned VPCs by api.openshift.com/name tag and force-deletes all resources (NAT GWs, EIPs, ENIs, subnets, route tables, IGWs, security groups) This aligns the STS conformance workflow with the HCP conformance workflow which already has best_effort and cleanup safety nets.
2 participants
Summary
Classic STS conformance jobs (4.19-4.22) are failing at 17% pass rate due to NAT Gateway quota exhaustion in us-east-2. Root cause: when
rosa delete clusterfails (cluster in error state), the deprovision step exits without cleaning up ROSA-managed VPC resources, orphaning NAT Gateways that accumulate across ~35 runs/week.Changes:
rosa-aws-sts-deprovisionchain asbest_effort: trueso post steps always run even when cluster deletion failsrosa-cluster-cleanup-vpcstep that finds orphaned VPCs byapi.openshift.com/nametag and force-deletes all resources (NAT GWs, EIPs, ENIs, subnets, route tables, IGWs, security groups)This aligns the STS conformance workflow with the HCP conformance workflow, which already has
best_effortand cleanup safety nets (rosa-e2e-cleanup-vpc+aws-deprovision-stacks).Test plan
Summary by CodeRabbit
REGIONoverride.