feat: VM usage and fair capacity split for overlapping flavor groups - #983
Merged
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
June 25, 2026 11:04
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFlavorGroupCapacity status now tracks running counts/resources and free/exclusive capacity. The controller computes VM usage, splits AZ capacity, and patches the CRD. Commitments reporting, metrics, tests, 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)
✨ 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: 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
umswmayj
reviewed
Jun 25, 2026
umswmayj
reviewed
Jun 26, 2026
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 `@internal/scheduling/reservations/capacity/split_test.go`:
- Around line 127-190: Add cores assertions to the resource-map tests so they
validate the full allocator contract, not just memory. Update the tests around
computeFreeResources, computeUnassigned, and SplitCapacity to also check
ResourceCores for each group/result, using the existing helpers like
computeFreeResources, computeUnassigned, SplitCapacity, and the ResourceCores
key to locate the assertions. This ensures CPU bookkeeping regressions are
caught alongside memory checks.
- Around line 108-122: The test in initGroupStates validation only checks
returned states, so a missing input group can slip through unnoticed. Tighten
the assertions in the t.Run loop by first verifying that
initGroupStates(tc.groups, tc.hosts) produced a state for every tc.groups entry,
using st.input.Name to match against tc.wantRemaining, and fail if any expected
group is absent before comparing st.remaining contents.
🪄 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: 544b032c-b0ef-4c04-b286-27c4316a8dfc
📒 Files selected for processing (2)
internal/scheduling/reservations/capacity/split.gointernal/scheduling/reservations/capacity/split_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/scheduling/reservations/capacity/split.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.9% |
mblos
enabled auto-merge (squash)
June 29, 2026 08:19
umswmayj
approved these changes
Jun 29, 2026
2 tasks
mblos
added a commit
that referenced
this pull request
Jun 29, 2026
## Changelog ## 2026-06-29 — [#990](#990) ### cortex v0.2.0 (sha-124ec226) Breaking changes: - FlavorGroupCapacity CRD status: removed `TotalInstances` field and replaced with `RunningInstances`, `RunningResources`, `FreeCapacity`, `ExclusivelyFreeCapacity`, `ExclusivelyFreeSlots`, `CommittedCapacityBytes`, and `SmallestFlavorName` ([#983](#983)) - Committed resource capacity metric renamed from `cortex_committed_resource_reported_capacity_gib` to `cortex_committed_resource_reported_capacity` with added `unit_size` label ([#983](#983)) Non-breaking changes: - VM usage and fair capacity split for overlapping flavor groups — capacity is now split fairly across flavor groups sharing the same hypervisors via a deterministic round-robin algorithm ([#983](#983)) - Allow KVM-enabled deployments to opt out of critical-severity alerting via `kvm.criticalAlerts` value ([#988](#988)) - Update External dependencies ([#977](#977), [#982](#982)) - Update `github.com/sapcc` ([#960](#960), [#976](#976)) - Update `github.com/sapcc/go-bits` ([#984](#984)) - Bump `github.com/opencontainers/runc` from 1.3.3 to 1.3.6 ([#973](#973)) - Update kube-prometheus-stack Docker tag to v87 ([#965](#965)) - Update postgres Dockerfile base image ([#978](#978), [#979](#979)) - Update actions/checkout action to v7 ([#961](#961)) ### cortex-nova v0.0.77 Includes updated chart cortex v0.2.0. - Add `kvm.criticalAlerts` config key to opt out of critical-severity alerting for KVM regions ([#988](#988)) ### cortex-crds v0.0.77 Includes updated chart cortex v0.2.0. ### cortex-cinder v0.0.77 Includes updated chart cortex v0.2.0. ### cortex-pods v0.0.77 Includes updated chart cortex v0.2.0. ### cortex-ironcore v0.0.77 Includes updated chart cortex v0.2.0. ### cortex-manila v0.0.77 Includes updated chart cortex v0.2.0. ### General - Update hypervisor CRD for local development ([#980](#980)) - Document InFlightReservation, domain hints, and CRS metrics ([#963](#963)) - Fix stale file paths, grace period behavior, and dead cross-reference in docs ([#962](#962)) ## Prerequisites - [ ] Merge chart-bump PR: #991 (must be merged before this PR) - [ ] Merge changelog PR: #992 (merge after this PR)
mblos
pushed a commit
that referenced
this pull request
Jul 7, 2026
The FlavorGroupCapacity section in docs/apis.md listed TotalInstances as a status field, but PR #983 removed that field and added several new ones (RunningInstances, RunningResources, FreeCapacity, ExclusivelyFreeCapacity, ExclusivelyFreeSlots, CommittedCapacityBytes, SmallestFlavorName). Updated the paragraph to reflect the actual CRD status structure and added a sentence explaining the fair capacity split across overlapping flavor groups. Assisted-by: Claude Opus 4.7 Co-authored-by: cortex-ai-agents[bot] <279748396+cortex-ai-agents[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Jul 8, 2026
PhilippMatthes
pushed a commit
that referenced
this pull request
Jul 21, 2026
The FlavorGroupCapacity section in docs/apis.md listed TotalInstances as a status field, but PR #983 removed that field and added several new ones (RunningInstances, RunningResources, FreeCapacity, ExclusivelyFreeCapacity, ExclusivelyFreeSlots, CommittedCapacityBytes, SmallestFlavorName). Updated the paragraph to reflect the actual CRD status structure and added a sentence explaining the fair capacity split across overlapping flavor groups. Assisted-by: Claude Opus 4.7 Co-authored-by: cortex-ai-agents[bot] <279748396+cortex-ai-agents[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Philipp Matthes <p.matthes@sap.com>
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.