fix: account for CPU & mem as a binding constraint in slot counting#1044
Open
mblos wants to merge 10 commits into
Open
fix: account for CPU & mem as a binding constraint in slot counting#1044mblos wants to merge 10 commits into
mblos wants to merge 10 commits into
Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
📝 WalkthroughWalkthroughThe capacity controller now probes flavor groups through fresh total and placeable results, computes slots using both memory and CPU, builds split inputs from successful probes, returns stranded host capacity, and writes CRDs with centralized total-capacity calculation. CRD output, deployment overrides, documentation, and tests are updated accordingly. ChangesCapacity reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant reconcileAZ
participant probeGroup
participant probeScheduler
participant SplitCapacity
participant writeCRD
reconcileAZ->>probeGroup: probe flavor group
probeGroup->>probeScheduler: request total and placeable resources
probeScheduler-->>probeGroup: return CPU-aware slot results
reconcileAZ->>SplitCapacity: submit fresh group inputs
SplitCapacity-->>reconcileAZ: return capacity and stranded hosts
reconcileAZ->>writeCRD: persist fresh group capacity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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)
internal/scheduling/reservations/capacity/controller.go (1)
556-574: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment doesn't match the actual (and tested) behavior.
The comment says the returned values come "by the flavor with the highest slot count" (singular), but the loop maximizes memory bytes and CPU cores independently — they can come from two different flavors. This is confirmed by the companion test case "picks flavor with most total memory, not most slots" in
controller_test.go. Worth rewording so a future reader doesn't assumeTotalCapacity[memory]andTotalCapacity[cores]represent a single, jointly-achievable flavor scenario.✏️ Proposed doc fix
-// computeTotalCapacity returns the maximum memory bytes and CPU cores representable -// by the flavor with the highest slot count in the group (empty-datacenter view). +// computeTotalCapacity returns the maximum memory bytes and CPU cores independently +// representable by any single flavor in the group (empty-datacenter view). The two +// maxima may come from different flavors and are not necessarily jointly achievable.🤖 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 `@internal/scheduling/reservations/capacity/controller.go` around lines 556 - 574, Update the doc comment for computeTotalCapacity to state that it independently returns the maximum representable memory bytes and CPU cores across eligible flavors, rather than values from the flavor with the highest slot count. Clarify that the two returned maxima may correspond to different flavors and are not necessarily jointly achievable.
🤖 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 `@internal/scheduling/reservations/capacity/controller.go`:
- Around line 631-635: Update the ExclusivelyFreeSlots calculation in the
capacity computation to use flavorSlots with exclusiveRes, flavorMemBytes, and
the flavor’s vCPU count, so both memory and CPU constrain the result. Reuse the
existing ResourceCores value and preserve the current positive-memory guard; add
coverage for a CPU-bound case alongside TestFlavorSlots or
TestComputeTotalCapacity.
- Around line 208-216: Update probeScheduler and flavorSlots to handle absent
ResourceCores explicitly instead of treating a missing map entry as zero
capacity. Check the ResourceCores map lookup in both probe paths and skip the
host, or otherwise validate its presence before calling flavorSlots, while
preserving the existing memory-capacity handling and CPU limiting when the value
exists.
---
Nitpick comments:
In `@internal/scheduling/reservations/capacity/controller.go`:
- Around line 556-574: Update the doc comment for computeTotalCapacity to state
that it independently returns the maximum representable memory bytes and CPU
cores across eligible flavors, rather than values from the flavor with the
highest slot count. Clarify that the two returned maxima may correspond to
different flavors and are not necessarily jointly achievable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41debee7-9504-47a4-b282-a76a3e9703c9
📒 Files selected for processing (2)
internal/scheduling/reservations/capacity/controller.gointernal/scheduling/reservations/capacity/controller_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/reservations/committed-resource-reservations.md (1)
167-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
To comply with markdownlint rule MD040, please specify a language identifier (like
textorpseudo) for this code block.📝 Proposed fix
-``` +```text confirmed = resources of VMs in both Spec and Status allocations spec_only_unblocked = resources of Spec-only VMs without an active InFlightReservation on this host remaining = max(0, Spec.Resources - confirmed) block = max(remaining, spec_only_unblocked)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/reservations/committed-resource-reservations.mdaround lines 167 - 172,
Specify a language identifier such as text on the fenced code block containing
the resource reservation formulas, without changing its content.</details> <!-- cr-comment:v1:d4884a9de82cb7e92a84fe94 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Nitpick comments:
In@docs/reservations/committed-resource-reservations.md:
- Around line 167-172: Specify a language identifier such as text on the fenced
code block containing the resource reservation formulas, without changing its
content.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `d19c6761-7a81-473e-84ea-d2042ed8314d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3e29c9fa99c7ca4146bf83e5d19f5a8d500782d1 and f19045105fb28ee64a92caf4dad84dcd6b8f81bf. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `Tiltfile` * `api/v1alpha1/flavor_group_capacity_types.go` * `docs/reservations/committed-resource-reservations.md` * `helm/library/cortex/files/crds/cortex.cloud_flavorgroupcapacities.yaml` * `internal/scheduling/reservations/capacity/controller.go` * `internal/scheduling/reservations/capacity/split.go` * `internal/scheduling/reservations/capacity/split_test.go` </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * internal/scheduling/reservations/capacity/controller.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
Test Coverage ReportTest Coverage 📊: 70.3% |
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.
Fix capacity slot counting to use both memory and CPU as binding constraints, not memory alone.