SPLAT-2238: Redesign logic to handle 4 pools with 3 max vcenters - #69
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2238 which is a valid jira issue. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vr4manta 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
|
Warning Review limit reached
Next review available in: 40 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: openshift-eng/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds vCenter-cap-aware dynamic filtering to lease pool assignment in the controller: it computes remaining vCenter slots and pools, excludes vCenters below a computed minimum-pools threshold via a fitting-pools calculation, and introduces recovery logic (releasing pool owner references, updating lease conditions, requeueing) when pool assignment stalls due to vCenter cap constraints. Two new test files validate the filtering and stuck scenarios. ChangesDynamic vCenter Filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 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 |
![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
🧹 Nitpick comments (1)
pkg/controller/leases_vcenter_cap_stuck_test.go (1)
1-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd meaningful assertions — the test is mostly logging with no verification.
The test sets up realistic production data and describes the stuck scenario well, but nearly all subtests only call
t.Logfwithout asserting outcomes. The sole assertion (lines 194–196) just checks that vcenter-1 has ≥ 1 pool. The "simulates getting stuck scenario" subtest (lines 234–337) describes the deadlock but doesn't verify that the recovery actually releases pools, updates conditions, or requeues.Consider asserting: which vCenters can/cannot fit the lease, that dynamic filtering produces the expected
minPoolsPerVCenter, and that the stuck condition (cap reached + no fitting pools on in-use vCenters) is correctly detected.♻️ Example: add assertions to the "approaching cap" subtest
t.Logf("Remaining slots: %d, Remaining pools: %d, Min pools per vCenter: %d", remainingSlots, remainingPools, minPoolsPerVCenter) - // Dynamic filtering: must pick a vCenter with >= 2 pools available - // Based on our pool counts: - // - vcenter-120: can provide 4 pools → ALLOWED ✓ - // - vcenter cicluster-1: can provide 2 pools → ALLOWED ✓ - // - vcenter cicluster-2: can provide 2 pools → ALLOWED ✓ - // - vcenter cicluster: can provide 6 pools → ALLOWED ✓ - - // All remaining vCenters can provide >= 2 pools, so none should be excluded - // This is good - the algorithm should work + // Verify minPoolsPerVCenter calculation + if minPoolsPerVCenter != 2 { + t.Errorf("Expected minPoolsPerVCenter=2, got %d", minPoolsPerVCenter) + } + + // Verify all remaining vCenters have >= minPoolsPerVCenter fitting pools + fittingPools, _ := utils.GetFittingPools(lease, pools[2:], nil) + poolsPerVCenter := make(map[string]int) + for _, p := range fittingPools { + if !vcentersInUse[p.Spec.Server] { + poolsPerVCenter[p.Spec.Server]++ + } + } + for server, count := range poolsPerVCenter { + if count < minPoolsPerVCenter { + t.Errorf("vCenter %s has %d pools, expected >= %d", server, count, minPoolsPerVCenter) + } + }🤖 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 `@pkg/controller/leases_vcenter_cap_stuck_test.go` around lines 1 - 352, The test is mostly logging and does not verify the stuck-lease behavior, so add real assertions in TestVCenterCapStuckScenario and its subtests. Use the existing pool/lease setup and helper logic around poolsPerVCenter, assignedPools, and minPoolsPerVCenter to assert which vCenters can or cannot fit the lease, that the computed minimum pools per vCenter is correct, and that the simulated deadlock state is detected rather than only printed. In the "simulates getting stuck scenario" path, assert the expected recovery outcome (release/retry or equivalent condition change) instead of just logging the scenario.
🤖 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 `@pkg/controller/leases.go`:
- Around line 954-955: The stuck-recovery path in leases.go only checks
`len(vcentersInUse) >= lease.Spec.VCenters`, so it misses the case where dynamic
filtering in `GetPoolWithStrategy` excludes all remaining vCenters before the
cap is reached. Update the recovery condition around the existing “cap reached
or approaching cap” check so it also triggers when filtering removed candidates
and no pool was found, using the relevant state from the filtering/selection
logic in `GetPoolWithStrategy` and the lease reconciliation path. This should
prevent falling through to the PARTIAL/break path and requeueing into the same
dead end.
- Around line 959-982: The lease release flow in the controller needs to persist
the OwnerReferences change separately from the status update. In the lease
handling logic where lease.OwnerReferences is filtered and conditions.Set is
used, first call a normal Update on the lease so the metadata change is saved,
then call Status().Update for the Fulfilled/ReasonLeaseNoPool condition, and
make sure any update error is returned rather than only logged.
---
Nitpick comments:
In `@pkg/controller/leases_vcenter_cap_stuck_test.go`:
- Around line 1-352: The test is mostly logging and does not verify the
stuck-lease behavior, so add real assertions in TestVCenterCapStuckScenario and
its subtests. Use the existing pool/lease setup and helper logic around
poolsPerVCenter, assignedPools, and minPoolsPerVCenter to assert which vCenters
can or cannot fit the lease, that the computed minimum pools per vCenter is
correct, and that the simulated deadlock state is detected rather than only
printed. In the "simulates getting stuck scenario" path, assert the expected
recovery outcome (release/retry or equivalent condition change) instead of just
logging the scenario.
🪄 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-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2b1ac0fa-a9de-45da-b3de-49e4c4a20873
📒 Files selected for processing (3)
pkg/controller/dynamic_filtering_test.gopkg/controller/leases.gopkg/controller/leases_vcenter_cap_stuck_test.go
| lease.OwnerReferences = newOwnerRefs | ||
|
|
||
| // Reset to PENDING | ||
| conditions.Set(lease, conditions.FalseConditionWithReason( |
There was a problem hiding this comment.
how many times around will we go around before we reset the condition?
There was a problem hiding this comment.
the idea here is that we move lease back to pending and return. next reconcile loop will attempt again. this way if logic is still broke, we have a chance of the other pools getting resources to allow it to be fulfilled. If this is bad, i can change back.
|
/lgtm |
3 participants
SPLAT-2238
Changes
Summary by CodeRabbit
New Features
Bug Fixes