feat: VM usage and fair capacity split for overlapping flavor groups#983
Open
mblos wants to merge 6 commits into
Open
feat: VM usage and fair capacity split for overlapping flavor groups#983mblos wants to merge 6 commits into
mblos wants to merge 6 commits into
Conversation
mblos
commented
Jun 25, 2026
mblos
commented
Collaborator
- FlavorGroupCapacity CRD tracks running resources/instances, free capacity and exclusively free capacity
- capacity calculation respects cases when flavor groups can use same resources: capacity is now split fairly across flavor groups sharing the same hypervisors via a deterministic round-robin algorithm, so the sum of reported capacities never exceeds real installed capacity
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughFlavorGroupCapacity now stores running counts and resource maps plus free and exclusively free capacity. The controller computes VM usage, splits AZ capacity, and patches the CRD. Commitments capacity, metrics, report-capacity monitoring, and alerts consume the updated fields and metric names. ChangesCapacity status and reporting pipeline
Sequence Diagram(s)sequenceDiagram
participant Controller
participant VMSource
participant Scheduler
participant SplitCapacity
participant KubernetesAPI
Controller->>VMSource: computeVMUsage()
Controller->>Scheduler: probeScheduler()
Scheduler-->>Controller: candidates and remaining capacity
Controller->>SplitCapacity: SplitCapacity(groups, hosts)
SplitCapacity-->>Controller: freeResources, exclusiveResources, unassigned
Controller->>KubernetesAPI: patch FlavorGroupCapacity status
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/scheduling/reservations/capacity/metrics.go (1)
54-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
cortex_committed_resource_committed_gibis exporting slot counts, not GiB.
Status.CommittedCapacityis written as smallest-flavor slots, but this collector publishes that raw integer under a_gibmetric/help string. For any group whose smallest flavor is not 1 GiB, the exposed value is numerically wrong. Either rename the metric to slots or export a real quantity-based GiB value.Also applies to: 105-111
🤖 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/metrics.go` around lines 54 - 57, The `committedCapacity` collector in `metrics.go` is publishing raw `Status.CommittedCapacity` slot counts under a `_gib` metric name/help string, so update `collectCommittedCapacityMetrics` and the metric definition to match the actual unit. Either rename `cortex_committed_resource_committed_gib` and its help text to slots, or convert the value to real GiB before calling `committedCapacity.WithLabelValues(...).Set(...)`, keeping the symbol names `committedCapacity` and `collectCommittedCapacityMetrics` aligned with the chosen unit.
🧹 Nitpick comments (3)
internal/scheduling/reservations/commitments/api/report_capacity_test.go (2)
177-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a core-constrained capacity case.
The new calculator consumes raw memory and core capacity, but these fixtures only vary exclusive free memory. Add one case where memory allows slots but exclusive free cores are zero/low, so a memory-only implementation cannot pass unnoticed.
Also applies to: 497-509
🤖 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/commitments/api/report_capacity_test.go` around lines 177 - 210, The CRD capacity fixtures in the report capacity test only exercise memory-based slot calculation, so add a new case to crdValueCase in this test that varies core availability as well as memory. Use the existing test helpers and report/capacity symbols in the same table-driven block to define a scenario where memory would permit capacity but exclusive free cores are zero or too low, and set the expected capacity/usage accordingly so the calculator must honor both memory and core constraints.
49-53: 📐 Maintainability & Code Quality | 🔵 TrivialReplace
interface{}withanyin changed test code.Per coding guidelines, use
anyinstead ofinterface{}in modern Go code. The verification scan exposed multiple remaining occurrences ininternal/scheduling/reservations/commitments/api/report_capacity_test.gothat should be updated:
- Line 52:
bodyfield in the test struct- Line 394: Arguments to
v1alpha1.BoxFeatureList- Lines 437–444:
featuresslice and nested map literals- Lines 469–474: Second
featuresslice definition and nested map literals♻️ Suggested cleanup
- body interface{} + body any- raw, err := v1alpha1.BoxFeatureList([]map[string]interface{}{}) + raw, err := v1alpha1.BoxFeatureList([]map[string]any{})- features := []map[string]interface{}{ - "flavors": []map[string]interface{}{ - "largestFlavor": map[string]interface{}{"name": "test_c8_m32", "vcpus": 8, "memoryMB": 32752, "diskGB": 50}, - "smallestFlavor": map[string]interface{}{"name": "test_c8_m32", "vcpus": 8, "memoryMB": 32752, "diskGB": 50}, + features := []map[string]any{ + "flavors": []map[string]any{ + "largestFlavor": map[string]any{"name": "test_c8_m32", "vcpus": 8, "memoryMB": 32752, "diskGB": 50}, + "smallestFlavor": map[string]any{"name": "test_c8_m32", "vcpus": 8, "memoryMB": 32752, "diskGB": 50},- features := []map[string]interface{}{ - "flavors": []map[string]interface{}{{"name": "test-flavor", "vcpus": 8, "memoryMB": flavorMemMiB, "diskGB": 50}}, - "largestFlavor": map[string]interface{}{"name": "test-flavor", "vcpus": 8, "memoryMB": flavorMemMiB, "diskGB": 50}, - "smallestFlavor": map[string]interface{}{"name": "test-flavor", "vcpus": 8, "memoryMB": flavorMemMiB, "diskGB": 50}, + features := []map[string]any{ + "flavors": []map[string]any{{"name": "test-flavor", "vcpus": 8, "memoryMB": flavorMemMiB, "diskGB": 50}}, + "largestFlavor": map[string]any{"name": "test-flavor", "vcpus": 8, "memoryMB": flavorMemMiB, "diskGB": 50}, + "smallestFlavor": map[string]any{"name": "test-flavor", "vcpus": 8, "memoryMB": flavorMemMiB, "diskGB": 50},🤖 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/commitments/api/report_capacity_test.go` around lines 49 - 53, Update the remaining Go test code in report_capacity_test.go to use any instead of interface{} for all changed type declarations. Replace the test struct’s body field in the table-driven test, the arguments passed to v1alpha1.BoxFeatureList, and the features slices plus nested map literals in the related test cases. Keep the changes scoped to the affected test helpers and fixtures so the signatures and literal types are consistent with modern Go style.Source: Coding guidelines
internal/scheduling/reservations/capacity/controller_test.go (1)
114-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one reconcile test with a non-
nilVMSource.Every helper-created controller in this file hardcodes
nil, so the newcomputeVMUsage/RunningResourcespath only gets zero-value coverage. A tiny fakeVMSourcereturning one known VM would lock down the new per-group/AZ aggregation logic.🤖 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_test.go` around lines 114 - 118, Add a reconcile test that constructs the Controller with a non-nil VMSource instead of relying only on newController, because newController hardcodes nil and only covers zero-value behavior. Create a tiny fake VMSource in controller_test.go that returns one known VM, then exercise Controller.Reconcile (or the relevant helper path) so computeVMUsage and RunningResources are covered for per-group/AZ aggregation. Use the existing Controller and computeVMUsage symbols to locate the right test setup and assert the expected grouped usage result.
🤖 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 145-148: The VM usage read path in the capacity controller is
swallowing errors and returning an empty result, which causes the reconcile flow
to overwrite running usage with zeros. Update the error handling in the
controller method that calls c.vmSource.ListVMsOnHypervisors so the failure is
propagated or used to skip the status write, and preserve the previous
RunningInstances and RunningResources values when the VM data is stale. Ensure
the downstream cycle logic does not treat a VMSource error as a valid zero-usage
snapshot.
- Around line 547-553: The host counting in the capacity calculation is too
permissive: in the capacity loop that builds `candidateHosts` and returns
`capacity`, `hosts`, and `candidateHosts`, a host with only a sub-flavor
remainder is still being counted even though it cannot fit a full flavor. Update
the logic in this capacity calculation path so both the slot contribution and
the `candidateHosts` append are gated on `capBytes / flavorBytes > 0` rather
than `capBytes > 0`, ensuring only truly placeable hosts are included in the
returned host list and count.
In `@internal/scheduling/reservations/capacity/split.go`:
- Around line 126-146: The freeResources calculation in split.go is overcounting
usable capacity because it floors memory and CPU separately per host in the
capacity split loop. Update the logic in the group/host iteration to compute a
single per-host slot count from the minimum across all required flavor
resources, then add that shared slot count multiplied by each resource amount so
FreeCapacity stays consistent and CPU- or memory-limited hosts do not overreport
capacity.
In `@internal/scheduling/reservations/commitments/capacity.go`:
- Around line 94-110: The capacity calculation in capacity.go is using only
memory to derive exclusive free slots, which can overstate both
instancesCapacity and fixed-ratio RAM capacity. Update the logic in the
capacity-computation block (the exclusive-free-slot calculation and the later
ramCapacity path, including the same pattern in the other referenced section) to
bound free instance slots by both memory and core availability, using the
limiting resource via the smallest flavor’s slot count (for example,
min(memorySlots, coreSlots)). Keep the result flowing through instancesCapacity
and the HasFixedRamCoreRatio branch so both values reflect the true bottleneck.
---
Outside diff comments:
In `@internal/scheduling/reservations/capacity/metrics.go`:
- Around line 54-57: The `committedCapacity` collector in `metrics.go` is
publishing raw `Status.CommittedCapacity` slot counts under a `_gib` metric
name/help string, so update `collectCommittedCapacityMetrics` and the metric
definition to match the actual unit. Either rename
`cortex_committed_resource_committed_gib` and its help text to slots, or convert
the value to real GiB before calling
`committedCapacity.WithLabelValues(...).Set(...)`, keeping the symbol names
`committedCapacity` and `collectCommittedCapacityMetrics` aligned with the
chosen unit.
---
Nitpick comments:
In `@internal/scheduling/reservations/capacity/controller_test.go`:
- Around line 114-118: Add a reconcile test that constructs the Controller with
a non-nil VMSource instead of relying only on newController, because
newController hardcodes nil and only covers zero-value behavior. Create a tiny
fake VMSource in controller_test.go that returns one known VM, then exercise
Controller.Reconcile (or the relevant helper path) so computeVMUsage and
RunningResources are covered for per-group/AZ aggregation. Use the existing
Controller and computeVMUsage symbols to locate the right test setup and assert
the expected grouped usage result.
In `@internal/scheduling/reservations/commitments/api/report_capacity_test.go`:
- Around line 177-210: The CRD capacity fixtures in the report capacity test
only exercise memory-based slot calculation, so add a new case to crdValueCase
in this test that varies core availability as well as memory. Use the existing
test helpers and report/capacity symbols in the same table-driven block to
define a scenario where memory would permit capacity but exclusive free cores
are zero or too low, and set the expected capacity/usage accordingly so the
calculator must honor both memory and core constraints.
- Around line 49-53: Update the remaining Go test code in
report_capacity_test.go to use any instead of interface{} for all changed type
declarations. Replace the test struct’s body field in the table-driven test, the
arguments passed to v1alpha1.BoxFeatureList, and the features slices plus nested
map literals in the related test cases. Keep the changes scoped to the affected
test helpers and fixtures so the signatures and literal types are consistent
with modern Go style.
🪄 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: 04cec33f-841a-4c95-8590-aff0c81a7b80
📒 Files selected for processing (11)
api/v1alpha1/flavor_group_capacity_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/manager/main.gohelm/library/cortex/files/crds/cortex.cloud_flavorgroupcapacities.yamlinternal/scheduling/reservations/capacity/controller.gointernal/scheduling/reservations/capacity/controller_test.gointernal/scheduling/reservations/capacity/metrics.gointernal/scheduling/reservations/capacity/split.gointernal/scheduling/reservations/capacity/split_test.gointernal/scheduling/reservations/commitments/api/report_capacity_test.gointernal/scheduling/reservations/commitments/capacity.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.9% |
umswmayj
reviewed
Jun 25, 2026
| // On error returns an empty map with fresh=false — callers must not overwrite running fields. | ||
| func (c *Controller) computeVMUsage( | ||
| ctx context.Context, | ||
| logger interface{ Error(error, string, ...any) }, |
Collaborator
There was a problem hiding this comment.
why not logger from ctx?
| var smallestCandidates []string | ||
| for _, flavor := range flavors { | ||
| cur := existingByName[flavor.Name] | ||
| cur.FlavorName = flavor.Name |
| // hvRemainingResources returns remaining schedulable resources after subtracting | ||
| // current allocations and (for memory) active reservation blocks. | ||
| // Returns nil if the hypervisor has no capacity data. | ||
| func hvRemainingResources(hv hv1.Hypervisor, blockedMemBytes int64) map[string]int64 { |
Collaborator
There was a problem hiding this comment.
same code in probeScheduler can we share?
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
2 participants
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.