fix: prevent pool burst creation, zombie churn, and auto-cleanup - #1059
Conversation
Three interacting bugs caused hive-stage-01 to grow from 1232 to 1701 AWS accounts in one day: 1. Pool counts NoState zombies as satisfied — the satisfaction check `unclaimedAccounts >= poolSize` included stuck accounts that would never become Ready, inflating the count. 2. Burst creation overwhelms the account controller — when the pool was unsatisfied, it created all needed Account CRs in milliseconds. With MaxConcurrentReconciles=1, most timeout waiting for processing. 3. NoState zombies never get failed — accounts stuck on the AWS limit requeue forever without being marked Failed, permanently inflating pool counts and hiding the real shortfall. Fix openshift#1: Change satisfaction check from `unclaimedAccounts >= poolSize` to `available + progressing + pending >= poolSize`. Add IsPendingFirstProcessing() to identify newly created Account CRs (State="", non-failed, pool-owned, never-claimed). Each CR created by the pool immediately counts as pending, naturally throttling creation to exactly poolSize without needing an artificial delay. Fix openshift#2: The pending counter IS the burst protection. When the pool creates an Account CR, the ownership watch triggers re-reconcile. The new CR counts as pending, so effectiveCount increases and the pool only creates more if still below poolSize. Fix openshift#3: Add zombie-failing logic in the account controller — fail NoState accounts stuck on the AWS limit for longer than createPendTime (25 min). Once failed, they drop out of the pending count and the pool can create replacements (if the limit allows). Additionally, add a pool-level account limit check: before creating any Account CR, verify the AWS account count is below the limit. This prevents the pool from creating CRs that would immediately become zombies, eliminating the create-timeout-fail-replace churn cycle. Co-Authored-By: Dakota Long <dlong@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
WalkthroughThe change adds pending-account tracking to account pools, uses pending accounts in pool satisfaction, pauses creation at AWS limits, and cleans up failed pool-owned accounts without AWS IDs. It also records a pending condition for blocked accounts and updates generated CRD, pipeline, and ownership metadata. ChangesAccount capacity and lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AccountPoolReconciler
participant AWSAccountWatcher
participant KubernetesAPI
AccountPoolReconciler->>AWSAccountWatcher: Read current account count and limit
AccountPoolReconciler->>KubernetesAPI: List Account resources
AccountPoolReconciler->>AccountPoolReconciler: Calculate effective count and pending status
AccountPoolReconciler->>KubernetesAPI: Create Account resource if capacity remains
AccountPoolReconciler-->>AccountPoolReconciler: Requeue for five minutes when limit is reached
Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
approved
![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 `@api/v1alpha1/account_types.go`:
- Around line 535-539: Update Account.IsPendingFirstProcessing to also require
that the account is not pending deletion by incorporating IsPendingDeletion into
the predicate, preserving the existing NoState, non-failed, never-claimed, and
pool-owned conditions.
🪄 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: Pro Plus
Run ID: 997cbbd2-5a93-4a64-b13c-e9781dd500f6
⛔ Files ignored due to path filters (4)
boilerplate/_data/last-boilerplate-commitis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/OWNERS_ALIASESis excluded by!boilerplate/**build/Dockerfileis excluded by!build/**build/Dockerfile.olm-registryis excluded by!build/**
📒 Files selected for processing (12)
.tekton/aws-account-operator-agentic-sdlc-check-pull-request.yamlOWNERS_ALIASESapi/v1alpha1/account_types.goapi/v1alpha1/accountpool_types.gocontrollers/account/account_controller.gocontrollers/accountpool/accountpool_controller.gocontrollers/accountpool/accountpool_controller_test.godeploy/crds/aws.managed.openshift.io_accountclaims.yamldeploy/crds/aws.managed.openshift.io_accountpools.yamldeploy/crds/aws.managed.openshift.io_accounts.yamldeploy/crds/aws.managed.openshift.io_awsfederatedaccountaccesses.yamldeploy/crds/aws.managed.openshift.io_awsfederatedroles.yaml
💤 Files with no reviewable changes (1)
- OWNERS_ALIASES
Replace the zombie-failing approach (marking accounts as Failed after 25 min) with an observable Condition (reason: AWSAccountLimitReached). The key insight: accounts blocked by the AWS org limit are valid — they just need capacity. Failing them destroys the recovery path and creates unnecessary CR churn when the limit clears. Setting a Condition instead keeps State empty so the account controller picks them up automatically once AccountsCanBeCreated() returns true. The Condition uses type=Pending with reason=AWSAccountLimitReached, providing the same observability as a Failed state without the permanence. LastProbeTime updates on each reconcile so operators can see how long an account has been waiting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
Accounts with a DeletionTimestamp set should not count as pending first processing — they're being torn down, not waiting for creation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BATMAN-JD
changed the title
Zombie Account CRs are Failed, pool-owned, and have no AWS account ID. They accumulate when the pool controller creates accounts that hit the AWS account limit before provisioning. These CRs are irrecoverable and inflate the Account CR list, adding overhead to every pool reconcile. On deploy, the operator will automatically strip finalizers and delete all ~958 existing zombie CRs in staging, eliminating manual cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
612989a to
534aed9
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (44.44%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #1059 +/- ##
==========================================
+ Coverage 46.60% 46.76% +0.16%
==========================================
Files 46 46
Lines 7124 7163 +39
==========================================
+ Hits 3320 3350 +30
- Misses 3457 3464 +7
- Partials 347 349 +2
🚀 New features to boost your workflow:
|
1. Add AccountsCanBeCreated() to AccountWatcherIface so the pool controller uses the same limit logic as the account controller. This fixes the startup deadlock where limit=0, count=0 evaluated as "limit reached" before the watcher's first poll, and inherits the fail-safe that defaults to false on AWS errors. 2. Switch limit-blocked Condition from write-every-reconcile to write-once. Check if the Pending condition with reason AWSAccountLimitReached already exists before writing, eliminating the status write storm with hundreds of blocked accounts. 3. Add test for the startup/watcher-uninitialized case (limit=0). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@BATMAN-JD: 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. |
| // These were created by the pool controller but never provisioned in AWS | ||
| // (e.g. account limit was reached). They are irrecoverable and inflate | ||
| // the Account CR list, adding overhead to every pool reconcile. | ||
| if !currentAcctInstance.HasAwsAccountID() && currentAcctInstance.IsOwnedByAccountPool() { |
There was a problem hiding this comment.
I'd consider wrapping this in a feature flag to enable at will in staging and not impact our other environments. I'm not totally sure if in all environments if this set of conditionals on an account CR should always lead to use removing finalizers and deleting the account.
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexSmithGH, BATMAN-JD 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 |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
3 participants
Problem
The pool controller had several issues causing staging instability:
Zombie churn cycle: Accounts that hit the AWS account limit were left in NoState forever, requeueing every 5 minutes. The pool counted these as "unclaimed" so it thought the pool was satisfied, but they would never provision — blocking new healthy accounts from being created.
Burst creation: The pool created all needed Account CRs in rapid succession via the ownership watch re-reconcile, causing burst CreateAccount API calls that overwhelmed the account controller.
Pending deletion miscounting: Accounts pending deletion were still counted as "unclaimed", inflating the pool satisfaction check and preventing replacement accounts from being created.
958 zombie Account CRs in staging: Failed, pool-owned Account CRs with no AWS account ID accumulated since 2022. They are irrecoverable but inflate the Account CR list, adding overhead to every pool reconcile.
Changes
Commit: cc94dba — Prevent pool burst creation and zombie churn cycle
IsPendingFirstProcessing()instead of counting all unclaimed accounts, excluding stuck/zombie CRsavailable + progressing + pendinginstead of raw unclaimed countCommit: 01c5c33 — Use Condition instead of failing accounts blocked by AWS limit
AccountPendingCondition (reason:AWSAccountLimitReached) instead of silently requeueingCommit: d059524 — Exclude accounts pending deletion from pending count
Commit: 612989a — Garbage-collect zombie Account CRs on reconcile
Pool-level limit gate (in cc94dba)
accountCount >= limitbefore creating new Account CRsTesting
Follow-up work
ctxthrough helper functions to replacecontext.TODO()— the zombie GC code uses a//nolint:contextcheckforremoveFinalizerwhich doesn't accept context yet