ROSAENG-62527: pass --no-wait to vpc delete when fire-and-forget is set - #174
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cdoan1: This pull request references ROSAENG-62527 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 task 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. |
openshift-ci
Bot
added
the
approved
|
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:
WalkthroughRegional teardown tests conditionally launch asynchronous OIDC, VPC, and IAM deletions through rosactl and skip synchronous cleanup. The IAM path marks cleanup complete, while the deletion helper explicitly constructs its CLI arguments. ChangesRegional teardown cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
There was a problem hiding this comment.
issue: When ROSA_REGIONAL_TEARDOWN_FIRE_AND_FORGET is set, --no-wait is only plumbed into the cluster-vpc delete spec (and the fireAndForgetInfraDelete DeferCleanup helper). The cluster-oidc and cluster-iam delete specs still run synchronously and will block.
Two options to fix:
- Plumb
--no-waitinto the OIDC and IAM delete specs the same way it's done for VPC. - (preferred) When fire-and-forget is set, just call
fireAndForgetInfraDeletefor all stacks and skip the ordered delete specs entirely — that helper already does the right thing (--no-wait+Process.Releasefor every stack). This would be simpler and match the "fire and forget" intent more directly.
f2bb51b to
c8cb882
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-cli/cluster_test.go`:
- Around line 828-833: The fire-and-forget teardown path currently marks cleanup
complete without confirming all async deletes launched. Update
fireAndForgetInfraDelete to return or aggregate cmd.Start errors for every
requested stack, and set cleanupCompleted only when all launches succeed;
otherwise preserve the later VPC/IAM cleanup paths.
🪄 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: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2e39a11b-1e80-4618-886e-d0ede569eaf0
📒 Files selected for processing (1)
test/e2e-cli/cluster_test.go
|
/override ci/prow/lint ci/prow/unit ci/prow/verify |
|
@cdoan1: Overrode contexts on behalf of cdoan1: ci/prow/lint, ci/prow/unit, ci/prow/verify 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. |
|
/test |
|
/test rosa-regionality-compatibility-e2e |
|
@cdoan1: The following test failed, say
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. |
|
e2e failure: ⏺ This failure is also not related to our changes. It's a race condition in the VPC setup step — the test created a VPC stack asynchronously but the subsequent cluster-vpc list ran before CloudFormation had registered the Expected The stack creation returned "Stack is being created asynchronously" but the list command ran immediately and found nothing. This is a pre-existing timing issue in the vpc-list setup spec (line 314), not in the cleanup specs All other test suites passed (Platform API, ZOA, Platform Monitoring). A re-run should get past this transient CloudFormation propagation delay. |
|
I'd suggest a slightly different approach here — opened cdoan1#1 with the change. Instead of having the oidc test launch all three deletes and the vpc/iam tests skip on a shared flag, each cleanup test should independently check |
c8cb882 to
84a8ffb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-cli/cluster_test.go (1)
305-306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRe-enable the disabled cluster list checks.
test/e2e-cli/cluster_test.go:305-306and:462are stillXIt, so the VPC/OIDC list assertions never run; switch them back toItand keep the VPC polling if the create remains async.🤖 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 `@test/e2e-cli/cluster_test.go` around lines 305 - 306, Re-enable both disabled cluster list checks by changing the XIt declarations at test/e2e-cli/cluster_test.go:305-306 and test/e2e-cli/cluster_test.go:462 to It; retain the existing VPC polling in the “should be able to list the cluster-vpc and find that cluster in the list” test because create-vpc remains asynchronous.
🤖 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 `@test/e2e-cli/cluster_test.go`:
- Around line 836-841: Update the fire-and-forget branches in
test/e2e-cli/cluster_test.go at 836-841, 854-859, and 872-877 so each call to
fireAndForgetInfraDelete dispatches exactly one stack: cluster-oidc at 836-841,
cluster-vpc at 854-859, and cluster-iam at 872-877; leave the surrounding
cleanupCompleted and return behavior unchanged.
---
Outside diff comments:
In `@test/e2e-cli/cluster_test.go`:
- Around line 305-306: Re-enable both disabled cluster list checks by changing
the XIt declarations at test/e2e-cli/cluster_test.go:305-306 and
test/e2e-cli/cluster_test.go:462 to It; retain the existing VPC polling in the
“should be able to list the cluster-vpc and find that cluster in the list” test
because create-vpc remains asynchronous.
🪄 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: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40d63811-24e3-4f99-96ec-63054311dfdf
📒 Files selected for processing (1)
test/e2e-cli/cluster_test.go
84a8ffb to
8c1a106
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-cli/cluster_test.go`:
- Around line 836-841: Track asynchronous cleanup dispatch separately for each
required infrastructure stack instead of setting suite-wide cleanupCompleted
after one launch. In test/e2e-cli/cluster_test.go at lines 836-841, 854-859, and
872-877, record successful dispatch for cluster-oidc, cluster-vpc, and
cluster-iam respectively; propagate any process-start failure, and suppress the
DeferCleanup safety net only when every required delete has been successfully
launched.
🪄 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: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 071b394a-9e79-4fd5-85da-8cf71f5b4ff0
📒 Files selected for processing (1)
test/e2e-cli/cluster_test.go
When ROSA_REGIONAL_TEARDOWN_FIRE_AND_FORGET=true (pre-merge e2e), pass --no-wait to rosactl cluster-vpc delete to skip the ~20 min VPC CloudFormation stack deletion wait. Nightly e2e runs still wait for full teardown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8c1a106 to
c3abb75
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-cli/cluster_test.go`:
- Around line 82-83: Update fireAndForgetInfraDelete so its delete command does
not always include --no-wait. Add an explicit mode or separate synchronous
cleanup path for the DeferCleanup caller, ensuring safety-net and nightly
teardown wait for infrastructure deletion while normal fire-and-forget cleanup
remains asynchronous.
🪄 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: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c591c9ac-7aa0-4c84-b725-54b1940cc47a
📒 Files selected for processing (1)
test/e2e-cli/cluster_test.go
typeid
left a comment
typeid
left a comment
There was a problem hiding this comment.
/test rosa-regionality-compatiblity-e2e
/lgtm
/approve
|
@typeid: The specified target(s) for Use 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoan1, typeid 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 |
|
Thank you for this @cdoan1 - it will be incredibly helpful! |
d8bf579
into
openshift-online:main
3 participants
When ROSA_REGIONAL_TEARDOWN_FIRE_AND_FORGET=true (pre-merge e2e), pass --no-wait to rosactl cluster-vpc delete to skip the ~20 min VPC CloudFormation stack deletion wait. Nightly e2e runs still wait for full teardown.
Description
Type of Change
Testing
make test)Checklist
Summary by CodeRabbit