OCPBUGS-98384: fix bastion cleanup KeyPair leak by capturing infraID/region eagerly - #8982
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98384, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
openshift-ci-robot
added
the
jira/valid-bug
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/testing
|
[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
the
approved
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-98384, 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. |
|
/test e2e-aws |
![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/util/dump/journals.go`:
- Around line 166-175: Bound the bastion cleanup context instead of passing
context.Background() directly: create a child context with a fixed timeout
inside the t.Cleanup callback, defer its cancellation, and pass it to
destroyBastion.Run. Preserve independence from the parent test context while
ensuring cleanup cannot retry indefinitely.
🪄 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: 2a700cb1-f11c-4537-aab8-40bf7d50bf98
📒 Files selected for processing (1)
test/e2e/util/dump/journals.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8982 +/- ##
==========================================
+ Coverage 43.67% 43.79% +0.11%
==========================================
Files 771 772 +1
Lines 95840 96037 +197
==========================================
+ Hits 41862 42061 +199
+ Misses 51067 51061 -6
- Partials 2911 2915 +4 see 20 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:
|
Test Resultse2e-aws
e2e-aks
|
|
/retest |
…Pair leak The refactor in PR openshift#8309 changed bastion cleanup from defer to t.Cleanup(), which runs after the HostedCluster is already deleted. Since DestroyBastionOpts used Name/Namespace to look up the HC for infraID/region, the lookup fails silently and the bastion EC2 instance, security group, and KeyPair are leaked on every test run. Capture infraID and region at bastion creation time and pass them directly to DestroyBastionOpts. Also use context.Background() in the cleanup closure to avoid context cancellation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/retest |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe sole failure is Root CauseThe After the test function completes, the framework's The non-Karpenter This failure is completely unrelated to PR #8982. The PR changes only the Additionally, the KeyPairLimitExceeded errors visible in the logs ( Recommendations
Evidence
|
|
/retest I cleaned up all the leaked keypairs so hopefully the karpenter test can pass now. |
|
/verify by e2e Job: pull-ci-openshift-hypershift-main-e2e-aws/2077722929948266496 Only TestKarpenter — keypair created and cleaned up: TestKarpenterUpgradeControlPlane — keypair created and cleaned up: Both keypair IDs match between create and destroy — no leak. Baseline comparison (build 2077075393017286656, before keypair cleanup): destroy-bastion.log files were 0 bytes because the CI account had 4,996 orphaned |
| infraID := hc.Spec.InfraID | ||
| region := hc.Spec.Platform.AWS.Region | ||
| t.Cleanup(func() { | ||
| destroyCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) |
There was a problem hiding this comment.
I need to look through the code again to see how context management has evolved, but is there no secondary context being managed that could be used here instead of background + local arbitrary timeout?
There was a problem hiding this comment.
No, there's no secondary context managed in the e2e framework that's available here. The ctx parameter to setupBastion is the test context, which may already be canceled when t.Cleanup fires — specifically, the cleanup teardown path at hypershift_framework.go:334 passes context.Background() explicitly. The testContext (process-wide, only canceled on SIGINT/SIGTERM) would work in principle, but it's not accessible from setupBastion and threading it through the call chain would change the signatures of DumpJournals, newClusterDumper, and teardownHostedCluster.
context.Background() with a bounded timeout is the established pattern for cleanup operations in this codebase — the integration tests use the same approach (e.g. test/integration/framework/run.go:45,83), and test/e2e/util/aws.go:121,395 both use context.WithTimeout(context.Background(), 2*time.Minute) in cleanup paths. The 5-minute timeout here accounts for the bastion destroy needing to sequentially delete the EC2 instance, security group, and keypair.
AI-assisted response via Claude Code
There was a problem hiding this comment.
Prow will issue a SIGTERM which drives cancellation, and then a SIGKILL after the grace period, so I don't see an opportunity to drive a secondary cancellation within the cleanup... that said, it raises the question why use a timeout at all? Could let the cleanup block forever until it either finishes or Prow kills the whole process, no arbitrary timeout required?
https://docs.ci.openshift.org/architecture/timeouts/#handling-sigterm-in-a-test-process
There was a problem hiding this comment.
Good point — the testContext signal handler at e2e_test.go:220-224 does cancel on SIGTERM, so Prow's kill chain would propagate. But testContext isn't accessible from setupBastion without threading it through DumpJournals → newClusterDumper → teardownHostedCluster.
A bare context.Background() without timeout would work if this cleanup were the only thing running, but t.Cleanup functions fire LIFO — if the bastion AWS call hangs (network partition, throttled API), it blocks all subsequent cleanups from running until Prow SIGKILLs the process. The 5-minute bound ensures other cleanup functions (HC destroy, namespace deletion) still get a chance to execute within Prow's grace period.
That said, 5 minutes is generous — the destroy typically completes in under 30 seconds (delete instance, delete SG, delete keypair). Happy to adjust the timeout if you think a shorter bound makes more sense.
AI-assisted response via Claude Code
There was a problem hiding this comment.
How many functions are in the cleanup stack? What is a "fair" timeout to allocate to each? How would you compute that statically or at runtime? If this timeout is exceeded and it falls through to the next, you still leaked. Is it better to try each one until pass or overall timeout, or give arbitrary inconsistent timeouts to everything in the stack? etc. etc.
Not sure what is the right answer. If our best current approach is limited to arbitrary timeout assignments, I guess I would err on the side of shorter to improve the chance of other items in the stack executing and ensuring we're logging leaks. Maybe take a look at the current grace timeouts in the prow config to see what budget we're working with.
Post-hoc detection / backstopping is probably our best bet in any case as we'll never be able to account for the orchestrator SIGTERM'ing us in any case, so as long as the things we're creating are being appropriately tagged and/or isolated in accounts, risk of cleanup issues like this can be reduced
There was a problem hiding this comment.
You're right that there's no principled way to statically budget timeouts across the cleanup stack — that's a real framework gap worth tracking separately.
For this PR though, the primary fix is the eager capture of infraID/region (lines 165-166), which directly prevents the KeyPair leak. The timeout is a secondary defensive measure, consistent with existing patterns in the codebase:
aws.go:121—PutRolePolicycleanup:context.WithTimeout(context.Background(), 2*time.Minute)aws.go:395—CreateCapacityReservationcleanup:context.WithTimeout(context.Background(), 2*time.Minute)
The main framework teardown (hypershift_framework.go:334) uses bare context.Background() with no timeout at all, so there's no existing budget system to integrate with.
Happy to shorten this to 2 minutes to match the existing cleanup timeout pattern and improve the odds for subsequent cleanup items, but I think the broader cleanup orchestration work (managed cleanup context with budget allocation) is a separate enhancement. The immediate priority is stopping the keypair leaks on every Karpenter test run.
Want me to drop to 2 minutes, or is 5 minutes acceptable as-is?
AI-assisted response via Claude Code
There was a problem hiding this comment.
Your call, this isn't blocking feedback
|
/cc @sdminonne |
|
/lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/hold cancel Will take what's here for now to stop the keypair bleeding. |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/pipeline required |
|
Scheduling tests matching the |
|
/verified by e2e Job: pull-ci-openshift-hypershift-main-e2e-aws/2077722929948266496 Only TestKarpenter and TestKarpenterUpgradeControlPlane exercise the bastion path (PublicOnly=false). All other tests use public IPs and skip bastion entirely. TestKarpenter — keypair created and cleaned up: create: {"msg":"Created key pair","id":"key-09fa036eb67b366e0","name":"karpenter-dbqlr-bastion"} create: {"msg":"Created key pair","id":"key-01c6a91c733938ad8","name":"karpenter-upgrade-control-plane-sssqw-bastion"} Baseline comparison (build 2077075393017286656, before keypair cleanup): destroy-bastion.log files were 0 bytes because the CI account had 4,996 orphaned *-bastion keypairs (limit 5,000), causing KeyPairLimitExceeded during bastion creation. Since creation failed before t.Cleanup was registered, cleanup never ran. After purging the orphaned keypairs, this run shows the fix working end-to-end. |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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. |
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-aws-4-22 |
enxebre
added
the
acknowledge-critical-fixes-only
|
@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. |
|
@bryan-cox: Jira Issue Verification Checks: Jira Issue OCPBUGS-98384 Jira Issue OCPBUGS-98384 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
6 participants
What this PR does / why we need it:
PR #8309 (merged May 11, 2026) refactored
test/e2e/util/dump/journals.goto reduce cyclomatic complexity, changing bastion cleanup fromdefertot.Cleanup(). This changed the execution order so that bastion destroy now runs after the HostedCluster is already deleted from Kubernetes.Since
DestroyBastionOptswas constructed withName/Namespace, the destroy code tried to look up the already-deleted HC to resolveinfraIDandregion, failed with a "not found" error, and the error was silently swallowed byt.Logf. This left EC2 bastion instances, security groups, and KeyPairs orphaned in AWS on every e2e test run. The AWS KeyPair limit (5000) was hit on June 17, 2026.Fix: Capture
hc.Spec.InfraIDandhc.Spec.Platform.AWS.Regionat bastion creation time and pass them directly toDestroyBastionOpts, bypassing the HC lookup path entirely. Also usecontext.Background()in the cleanup closure to avoid the parent context being cancelled before cleanup runs.Which issue(s) this PR fixes:
Fixes OCPBUGS-98384
Special notes for your reviewer:
DestroyBastionOpts.Run()(cmd/bastion/aws/destroy.go:87-112) supports two ways to resolve infraID/region: by looking up the HC viaName/Namespace, or by acceptingInfraID/Regiondirectly. The old test code used the lookup path, which fails when the HC is already deleted. This fix switches to passingInfraID/Regiondirectly so no Kubernetes call is needed during cleanup.Reported by Dan Mace.
Checklist:
Summary by CodeRabbit