[DO NOT MERGE]fix: use available+progressing for pool satisfaction and fail stuck N… - #1058
Draft
dakotalongRH wants to merge 1 commit into
Draft
Conversation
…oState accounts The pool controller used unclaimedAccounts to determine if the pool was satisfied, which included NoState zombie CRs that will never provision. This prevented new accounts from being created when the pool was full of stuck accounts. Additionally, accounts stuck on the account-limit requeued every 5 minutes forever without being marked Failed, inflating the unclaimed count permanently. They are now failed after 25 minutes (matching the existing creation timeout). The pool also created all Account CRs in rapid succession via the ownership watch re-reconcile, causing burst CreateAccount API calls that overwhelmed the single-threaded account controller. A 30-second delay between creates prevents this.
dakotalongRH
marked this pull request as draft
July 30, 2026 00:30
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughAccount pool reconciliation now excludes no-state zombie accounts from effective capacity, spaces account creation by 30 seconds, and tests the behavior. Account reconciliation also fails accounts that remain blocked by the global account limit beyond ChangesAccount provisioning reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dakotalongRH 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
![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
🤖 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 `@controllers/account/account_controller.go`:
- Around line 160-185: Update the account-limit handling around
AccountsCanBeCreated so every account blocked by the global limit returns a
retry result, including FedRAMP mode. Keep the stuck-duration timeout and
setAccountFailed flow gated by !config.IsFedramp(), then move the existing retry
logging and return outside that condition to prevent nonCCSAssignAccount from
running.
In `@controllers/accountpool/accountpool_controller.go`:
- Around line 117-121: Update Reconcile so accountCreationDelay is enforced
across all reconciliation paths, including reconciliations triggered by the
Account ownership watch. After creating an Account, ensure every return path
applies the delayed result, or persist and validate a next-creation time at
Reconcile entry before permitting another creation; preserve the required
30-second gap between account creations.
🪄 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: 830754b9-cf7d-42ba-b676-d7185f353877
📒 Files selected for processing (3)
controllers/account/account_controller.gocontrollers/accountpool/accountpool_controller.gocontrollers/accountpool/accountpool_controller_test.go
Comment on lines
+160
to
+185
| // Check account limit before doing any expensive work. | ||
| // If the account has been stuck waiting for the limit to clear for too long, | ||
| // fail it so the pool controller's satisfaction check isn't inflated by | ||
| // accounts that will never provision. | ||
| if !currentAcctInstance.IsPendingDeletion() && !currentAcctInstance.IsBYOC() && currentAcctInstance.IsUnclaimedAndHasNoState() && !currentAcctInstance.HasAwsAccountID() { | ||
| if !totalaccountwatcher.TotalAccountWatcher.AccountsCanBeCreated() { | ||
| if !config.IsFedramp() { | ||
| reqLogger.Info("AWS Account limit reached. This does not always indicate a problem, it's a limit we enforce in the configmap to prevent runaway account creation") | ||
| stuckDuration := time.Since(currentAcctInstance.CreationTimestamp.Time) | ||
| if stuckDuration > createPendTime { | ||
| errMsg := fmt.Sprintf("Account limit reached for longer than %d minutes, failing account", utils.WaitTime) | ||
| reqLogger.Info(errMsg, "stuckFor", stuckDuration.String()) | ||
| _, stateErr := r.setAccountFailed( //nolint:contextcheck // pre-existing function signature | ||
| reqLogger, | ||
| currentAcctInstance, | ||
| awsv1alpha1.AccountCreationFailed, | ||
| "AccountLimitTimeout", | ||
| errMsg, | ||
| AccountFailed, | ||
| ) | ||
| if stateErr != nil { | ||
| reqLogger.Error(stateErr, "failed setting account state", "desiredState", AccountFailed) | ||
| return reconcile.Result{}, stateErr | ||
| } | ||
| return reconcile.Result{}, errors.New(errMsg) | ||
| } | ||
| reqLogger.Info("AWS Account limit reached, will retry", "stuckFor", stuckDuration.String()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the account-limit requeue in FedRAMP mode.
When AccountsCanBeCreated() is false and FedRAMP mode is enabled, this block falls through instead of returning. Reconciliation can then reach Lines 374-377 and call nonCCSAssignAccount, attempting provisioning despite the global limit. Keep the timeout failure gated by !config.IsFedramp(), but move the retry return outside that condition.
Proposed structure
- if !config.IsFedramp() {
- stuckDuration := time.Since(currentAcctInstance.CreationTimestamp.Time)
+ stuckDuration := time.Since(currentAcctInstance.CreationTimestamp.Time)
+ if !config.IsFedramp() {
if stuckDuration > createPendTime {
// existing setAccountFailed path
}
- reqLogger.Info("AWS Account limit reached, will retry", "stuckFor", stuckDuration.String())
- return reconcile.Result{Requeue: true, RequeueAfter: time.Duration(5) * time.Minute}, nil
}
+ reqLogger.Info("AWS Account limit reached, will retry", "stuckFor", stuckDuration.String())
+ return reconcile.Result{Requeue: true, RequeueAfter: 5 * time.Minute}, nil📝 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.
Suggested change
| // Check account limit before doing any expensive work. | |
| // If the account has been stuck waiting for the limit to clear for too long, | |
| // fail it so the pool controller's satisfaction check isn't inflated by | |
| // accounts that will never provision. | |
| if !currentAcctInstance.IsPendingDeletion() && !currentAcctInstance.IsBYOC() && currentAcctInstance.IsUnclaimedAndHasNoState() && !currentAcctInstance.HasAwsAccountID() { | |
| if !totalaccountwatcher.TotalAccountWatcher.AccountsCanBeCreated() { | |
| if !config.IsFedramp() { | |
| reqLogger.Info("AWS Account limit reached. This does not always indicate a problem, it's a limit we enforce in the configmap to prevent runaway account creation") | |
| stuckDuration := time.Since(currentAcctInstance.CreationTimestamp.Time) | |
| if stuckDuration > createPendTime { | |
| errMsg := fmt.Sprintf("Account limit reached for longer than %d minutes, failing account", utils.WaitTime) | |
| reqLogger.Info(errMsg, "stuckFor", stuckDuration.String()) | |
| _, stateErr := r.setAccountFailed( //nolint:contextcheck // pre-existing function signature | |
| reqLogger, | |
| currentAcctInstance, | |
| awsv1alpha1.AccountCreationFailed, | |
| "AccountLimitTimeout", | |
| errMsg, | |
| AccountFailed, | |
| ) | |
| if stateErr != nil { | |
| reqLogger.Error(stateErr, "failed setting account state", "desiredState", AccountFailed) | |
| return reconcile.Result{}, stateErr | |
| } | |
| return reconcile.Result{}, errors.New(errMsg) | |
| } | |
| reqLogger.Info("AWS Account limit reached, will retry", "stuckFor", stuckDuration.String()) | |
| // Check account limit before doing any expensive work. | |
| // If the account has been stuck waiting for the limit to clear for too long, | |
| // fail it so the pool controller's satisfaction check isn't inflated by | |
| // accounts that will never provision. | |
| if !currentAcctInstance.IsPendingDeletion() && !currentAcctInstance.IsBYOC() && currentAcctInstance.IsUnclaimedAndHasNoState() && !currentAcctInstance.HasAwsAccountID() { | |
| if !totalaccountwatcher.TotalAccountWatcher.AccountsCanBeCreated() { | |
| stuckDuration := time.Since(currentAcctInstance.CreationTimestamp.Time) | |
| if !config.IsFedramp() { | |
| if stuckDuration > createPendTime { | |
| errMsg := fmt.Sprintf("Account limit reached for longer than %d minutes, failing account", utils.WaitTime) | |
| reqLogger.Info(errMsg, "stuckFor", stuckDuration.String()) | |
| _, stateErr := r.setAccountFailed( //nolint:contextcheck // pre-existing function signature | |
| reqLogger, | |
| currentAcctInstance, | |
| awsv1alpha1.AccountCreationFailed, | |
| "AccountLimitTimeout", | |
| errMsg, | |
| AccountFailed, | |
| ) | |
| if stateErr != nil { | |
| reqLogger.Error(stateErr, "failed setting account state", "desiredState", AccountFailed) | |
| return reconcile.Result{}, stateErr | |
| } | |
| return reconcile.Result{}, errors.New(errMsg) | |
| } | |
| } | |
| reqLogger.Info("AWS Account limit reached, will retry", "stuckFor", stuckDuration.String()) | |
| return reconcile.Result{Requeue: true, RequeueAfter: 5 * time.Minute}, nil |
🤖 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 `@controllers/account/account_controller.go` around lines 160 - 185, Update the
account-limit handling around AccountsCanBeCreated so every account blocked by
the global limit returns a retry result, including FedRAMP mode. Keep the
stuck-duration timeout and setAccountFailed flow gated by !config.IsFedramp(),
then move the existing retry logging and return outside that condition to
prevent nonCCSAssignAccount from running.
Comment on lines
+117
to
+121
| // Delay before creating the next account to prevent burst-provisioning | ||
| // many AWS accounts at once. Without this, each Account CR creation triggers | ||
| // an immediate re-reconcile (via ownership watch), causing the pool to | ||
| // rapidly create all CRs in quick succession. | ||
| return reconcile.Result{RequeueAfter: accountCreationDelay}, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -a 'accountpool_controller.go|accountpool' . | sed 's#^\./##' | head -50
echo
echo "Outline:"
if [ -f controllers/accountpool/accountpool_controller.go ]; then
ast-grep outline controllers/accountpool/accountpool_controller.go --view compact || true
fi
echo
echo "Relevant source:"
if [ -f controllers/accountpool/accountpool_controller.go ]; then
sed -n '1,220p' controllers/accountpool/accountpool_controller.go | cat -n
fi
echo
echo "Search for accountCreationDelay and RequeueAfter/owner references:"
rg -n "accountCreationDelay|RequeueAfter|owner|Ownership|CreateAccount|Create|Reconcile" controllers/accountpool/controllers/accountpool/accountpool_controller.go controllers/accountpool -S
echo
echo "Diff/stat if available:"
git diff --stat controllers/accountpool/accountpool_controller.go || true
git diff -- controllers/accountpool/accountpool_controller.go | sed -n '1,240p' || trueRepository: openshift/aws-account-operator
Length of output: 14215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "SetupWithManager:"
sed -n '226,290p' controllers/accountpool/accountpool_controller.go | cat -n
echo
echo "Tests around Setup/Reconcile/enqueues:"
sed -n '240,380p' controllers/accountpool/accountpool_controller_test.go | cat -n
echo
echo "Search owner watch / Owns references in accountpool files:"
rg -n "Owns|Build|Watches|Create(\(|\()|RequeueAfter|Requeue|Result\\{" controllers/accountpool -S
echo
echo "Read-only semantic probe of reconcile.Result behavior described in controller-runtime docs/source text if vendored:
github module availability?"
rg -n "func Result RequeueAfter|RequeueAfter|Requeue bool" -S go.mod vendor 2>/dev/null || trueRepository: openshift/aws-account-operator
Length of output: 7376
Enforce creation throttling on every reconcile path
Reconcile creates a new Account and then returns RequeueAfter: accountCreationDelay, but the .Owns(&awsv1alpha1.Account{}) watch enqueues another reconciliation on that create/update. Return only the delayed result after every creation, or persist a next-creation time and check it at the start of Reconcile, so owner-watch events cannot bypass the 30-second gap.
🤖 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 `@controllers/accountpool/accountpool_controller.go` around lines 117 - 121,
Update Reconcile so accountCreationDelay is enforced across all reconciliation
paths, including reconciliations triggered by the Account ownership watch. After
creating an Account, ensure every return path applies the delayed result, or
persist and validate a next-creation time at Reconcile entry before permitting
another creation; preserve the required 30-second gap between account creations.
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…oState accounts
The pool controller used unclaimedAccounts to determine if the pool was satisfied, which included NoState zombie CRs that will never provision. This prevented new accounts from being created when the pool was full of stuck accounts.
Additionally, accounts stuck on the account-limit requeued every 5 minutes forever without being marked Failed, inflating the unclaimed count permanently. They are now failed after 25 minutes (matching the existing creation timeout).
The pool also created all Account CRs in rapid succession via the ownership watch re-reconcile, causing burst CreateAccount API calls that overwhelmed the single-threaded account controller. A 30-second delay between creates prevents this.
What is being added?
Is this a fix for a bug? What's the bug? Is this a new feature? Please describe it. Is this just a small typo fix? That's fine too!
Checklist before requesting review
Steps To Manually Test
Please provide us steps to reproduce the scenarios needed to test this. If an integration test is provided, let us know how to run it. If not, enumerate the steps to validate this does what it's supposed to.
Ref OSD-0000
Summary by CodeRabbit