fix: use 1gb unit for committed resources instead of smallest flavor of flavor group#822
Merged
Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR normalizes commitment memory sizing from flavor-dependent to fixed GiB across commitments/quota/reporting, adds a TotalCapacity map to FlavorGroupCapacity (type, deepcopy, CRD, controller population), and extends tools/visualize-committed-resources to query multiple Kubernetes contexts with aggregated results. ChangesCommitment Memory Unit Normalization to Fixed GiB
TotalCapacity Field Infrastructure
Multi-Context Visualization Tool Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/flavor_group_capacity_types.go (1)
55-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc comment on
CommittedCapacity— please verify against the 1 GiB unit change.The comment currently says "expressed in multiples of the smallest flavor's memory", but the PR title states the unit is being changed to a fixed 1 GiB slot. The controller (
sumCommittedCapacity) still divides bysmallestFlavorBytesderived from the actual smallest flavor, so if that arithmetic has been updated elsewhere in the pipeline (outside the four files under review), this comment will mislead API consumers.✏️ Proposed doc update (if the unit has changed to 1 GiB)
- // CommittedCapacity is the sum of AcceptedAmount across active CommittedResource CRDs, - // expressed in multiples of the smallest flavor's memory. + // CommittedCapacity is the sum of AcceptedAmount across active CommittedResource CRDs, + // expressed in multiples of 1 GiB.🤖 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 `@api/v1alpha1/flavor_group_capacity_types.go` around lines 55 - 58, The doc comment on the CommittedCapacity field is stale: it claims the value is "expressed in multiples of the smallest flavor's memory" while the PR indicates the unit may have been changed to fixed 1 GiB slots; inspect the controller function sumCommittedCapacity (and the smallestFlavorBytes calculation it uses) to confirm which unit is actually used, then update the CommittedCapacity comment to match the implementation (if unit is fixed 1 GiB, say "expressed as number of 1 GiB slots"; if it remains relative to flavors, keep "multiples of the smallest flavor's memory") and ensure any related arithmetic in sumCommittedCapacity and references to smallestFlavorBytes are consistent with that documented unit.
🧹 Nitpick comments (5)
internal/scheduling/reservations/commitments/state_test.go (2)
16-32: ⚡ Quick winRemove unused
testFlavorGrouphelper.After the
FromCommitmentsignature change, no test in this file referencestestFlavorGroupanymore. Thecomputeimport (line 10) is also only used here, so it will become unused as well.♻️ Proposed cleanup
import ( "testing" "github.com/cobaltcore-dev/cortex/api/v1alpha1" - "github.com/cobaltcore-dev/cortex/internal/knowledge/extractor/plugins/compute" hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// Test helper: creates a minimal flavor group for testing -func testFlavorGroup() compute.FlavorGroupFeature { - return compute.FlavorGroupFeature{ - Name: "test-group", - Flavors: []compute.FlavorInGroup{ - {Name: "large", VCPUs: 16, MemoryMB: 32768, DiskGB: 100}, - {Name: "medium", VCPUs: 8, MemoryMB: 16384, DiskGB: 50}, - {Name: "small", VCPUs: 4, MemoryMB: 8192, DiskGB: 25}, - }, - SmallestFlavor: compute.FlavorInGroup{ - Name: "small", VCPUs: 4, MemoryMB: 8192, DiskGB: 25, - }, - LargestFlavor: compute.FlavorInGroup{ - Name: "large", VCPUs: 16, MemoryMB: 32768, DiskGB: 100, - }, - } -}As per coding guidelines: "Test files should be short and contain only necessary test cases".
🤖 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/state_test.go` around lines 16 - 32, Remove the now-unused test helper testFlavorGroup and the associated compute import: delete the testFlavorGroup function declaration and remove compute from the imports so there are no unused symbols after the FromCommitment signature change; ensure no other tests in state_test.go reference testFlavorGroup before deleting.
39-62: 💤 Low valueLGTM!
Comment and expected value (
5 * (1<<30)= 5 GiB) correctly reflect the new fixed GiB-per-unit semantics, and theAmount: 5field comment on line 39 (// 5 multiples of smallest flavor) is now stale though — it should read something like// 5 GiB.📝 Stale inline comment
- Amount: 5, // 5 multiples of smallest flavor + Amount: 5, // 5 GiB🤖 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/state_test.go` around lines 39 - 62, Update the stale inline comment for the test commitment's Amount field to reflect the new GiB-per-unit semantics: change the comment on the Amount assignment (in the test that calls FromCommitment and asserts state.TotalMemoryBytes) from "// 5 multiples of smallest flavor" to something like "// 5 GiB" so it correctly documents that Amount: 5 means 5 GiB used in the memory calculation and aligns with the expectedMemory assertion.tools/visualize-committed-resources/main.go (2)
478-478: 💤 Low valueHelp text omits the
usageview.The available views listed in
--viewshelp (all, summary, commitments, reservations, allocations) doesn't mentionusage, even thoughviewUsageis inallViewsand is consumed byprintCommitments/printReservations.📝 Update help text
- viewsFlag := flag.String("views", "all", "Views: all, summary, commitments, reservations, allocations") + viewsFlag := flag.String("views", "all", "Views: all, summary, commitments, reservations, allocations, usage")🤖 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 `@tools/visualize-committed-resources/main.go` at line 478, The help string for the flag defined in viewsFlag (flag.String("views", "all", ...)) omits the "usage" view; update the flag.String help text to list "usage" alongside the other options (e.g., all, summary, commitments, reservations, allocations, usage) so it matches allViews and the behavior used by printCommitments and printReservations which consume viewUsage.
495-510: 💤 Low valueEdge case: an all-empty
--contextsvalue silently produces zero clients.
strings.FieldsFuncwith the comma predicate drops empty fields, so--contexts=,,,(or--contexts=,) yields an emptycontextNamesslice, and the loop below never produces a client. The subsequentfetchSnapshotthen silently returns empty results instead of failing or falling back to the current context.Consider falling back to the default
[]string{""}when parsing yields zero entries:♻️ Proposed fallback
contextNames := []string{""} if *contextsFlag != "" { contextNames = strings.FieldsFunc(*contextsFlag, func(r rune) bool { return r == ',' }) for i := range contextNames { contextNames[i] = strings.TrimSpace(contextNames[i]) } + if len(contextNames) == 0 { + contextNames = []string{""} + } }🤖 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 `@tools/visualize-committed-resources/main.go` around lines 495 - 510, Parsing --contexts with strings.FieldsFunc can yield an empty contextNames slice for inputs like ",,,"; update the logic around contextsFlag/contextNames so that after splitting and trimming you check if len(contextNames) == 0 and, in that case, fall back to the default single empty context (set contextNames = []string{""}) before iterating to call getClientForContext and create contextClient entries; this ensures fetchSnapshot uses the current context instead of silently producing zero clients.internal/scheduling/reservations/commitments/syncer.go (1)
144-164: ⚡ Quick winStale wording in unit-mismatch warning.
The expected unit is now a fixed constant (
GiB) rather than something derived from the flavor group, so the message text and thesmallestFlavorMemoryMBstructured field are no longer relevant to explaining the mismatch.📝 Suggested message cleanup
- log.V(0).Info("WARNING: skipping commitment due to unit mismatch - Limes unit differs from Cortex flavor group, waiting for Limes to update", + log.V(0).Info("WARNING: skipping commitment due to unit mismatch - Limes unit differs from expected GiB, waiting for Limes to update", "commitmentUUID", commitment.UUID, "flavorGroup", flavorGroupName, "limesUnit", commitment.Unit, - "expectedUnit", expectedUnit, - "smallestFlavorMemoryMB", flavorGroup.SmallestFlavor.MemoryMB) + "expectedUnit", expectedUnit)If you drop
smallestFlavorMemoryMB,flavorGroupitself becomes unused — switch the lookup at line 133 to an existence-only check (_, exists := flavorGroups[flavorGroupName]).🤖 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/syncer.go` around lines 144 - 164, The unit-mismatch warning message still references the now-irrelevant flavorGroup and smallestFlavorMemoryMB; update the log call where commitment.Unit != "" && commitment.Unit != expectedUnit to remove the "smallestFlavorMemoryMB" structured field and drop any use of flavorGroup in that log entry (keep commitment.UUID, flavorGroupName, limesUnit, expectedUnit). Also change the earlier lookup that currently assigns a full flavorGroup (used only to test existence) to an existence-only check (use _, exists := flavorGroups[flavorGroupName]) and adjust any later code to use the boolean exists where needed; leave the skip logic (s.monitor.RecordCommitmentSkipped(SkipReasonUnitMismatch) and result.skippedUUIDs) unchanged.
🤖 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/commitments/api/info.go`:
- Around line 53-55: The response body currently echoes internal error text;
change the http.Error call in
internal/scheduling/reservations/commitments/api/info.go so it returns a generic
message (e.g., "Service temporarily unavailable") instead of concatenating
err.Error(), keep statusCode = http.StatusServiceUnavailable, and ensure the
detailed error remains in logs (the existing logger.Info("service info not
available yet", "error", err.Error()) can stay or be promoted to logger.Error if
desired); update the http.Error invocation that references statusCode and
err.Error() to use the generic message only.
In `@internal/scheduling/reservations/commitments/capacity.go`:
- Around line 66-67: The code currently computes ramPerSlotGiB by doing integer
division of groupData.SmallestFlavor.MemoryMB / 1024 which floors GiB and
underreports RAM; change this to use ceiling division so any fractional GiB
rounds up (e.g. (MemoryMB + 1023) / 1024 or use math.Ceil on a float conversion)
and keep the result as an integer type, and apply the same ceiling logic to the
other occurrence doing the same calculation (the block referencing ramPerSlotGiB
and groupData.SmallestFlavor.MemoryMB around lines 107-118).
In `@internal/scheduling/reservations/commitments/usage.go`:
- Around line 298-301: The conversion from MiB to GiB in
internal/scheduling/reservations/commitments/usage.go currently truncates
partial GiB because usageMultiple is computed as row.FlavorRAM / 1024; change
this to use ceiling division so partial GiB rounds up (e.g., compute
(row.FlavorRAM + 1024 - 1) / 1024 or equivalent) when setting usageMultiple for
row.FlavorRAM to ensure 1536 MiB becomes 2 GiB and 512 MiB becomes 1 GiB.
---
Outside diff comments:
In `@api/v1alpha1/flavor_group_capacity_types.go`:
- Around line 55-58: The doc comment on the CommittedCapacity field is stale: it
claims the value is "expressed in multiples of the smallest flavor's memory"
while the PR indicates the unit may have been changed to fixed 1 GiB slots;
inspect the controller function sumCommittedCapacity (and the
smallestFlavorBytes calculation it uses) to confirm which unit is actually used,
then update the CommittedCapacity comment to match the implementation (if unit
is fixed 1 GiB, say "expressed as number of 1 GiB slots"; if it remains relative
to flavors, keep "multiples of the smallest flavor's memory") and ensure any
related arithmetic in sumCommittedCapacity and references to smallestFlavorBytes
are consistent with that documented unit.
---
Nitpick comments:
In `@internal/scheduling/reservations/commitments/state_test.go`:
- Around line 16-32: Remove the now-unused test helper testFlavorGroup and the
associated compute import: delete the testFlavorGroup function declaration and
remove compute from the imports so there are no unused symbols after the
FromCommitment signature change; ensure no other tests in state_test.go
reference testFlavorGroup before deleting.
- Around line 39-62: Update the stale inline comment for the test commitment's
Amount field to reflect the new GiB-per-unit semantics: change the comment on
the Amount assignment (in the test that calls FromCommitment and asserts
state.TotalMemoryBytes) from "// 5 multiples of smallest flavor" to something
like "// 5 GiB" so it correctly documents that Amount: 5 means 5 GiB used in the
memory calculation and aligns with the expectedMemory assertion.
In `@internal/scheduling/reservations/commitments/syncer.go`:
- Around line 144-164: The unit-mismatch warning message still references the
now-irrelevant flavorGroup and smallestFlavorMemoryMB; update the log call where
commitment.Unit != "" && commitment.Unit != expectedUnit to remove the
"smallestFlavorMemoryMB" structured field and drop any use of flavorGroup in
that log entry (keep commitment.UUID, flavorGroupName, limesUnit, expectedUnit).
Also change the earlier lookup that currently assigns a full flavorGroup (used
only to test existence) to an existence-only check (use _, exists :=
flavorGroups[flavorGroupName]) and adjust any later code to use the boolean
exists where needed; leave the skip logic
(s.monitor.RecordCommitmentSkipped(SkipReasonUnitMismatch) and
result.skippedUUIDs) unchanged.
In `@tools/visualize-committed-resources/main.go`:
- Line 478: The help string for the flag defined in viewsFlag
(flag.String("views", "all", ...)) omits the "usage" view; update the
flag.String help text to list "usage" alongside the other options (e.g., all,
summary, commitments, reservations, allocations, usage) so it matches allViews
and the behavior used by printCommitments and printReservations which consume
viewUsage.
- Around line 495-510: Parsing --contexts with strings.FieldsFunc can yield an
empty contextNames slice for inputs like ",,,"; update the logic around
contextsFlag/contextNames so that after splitting and trimming you check if
len(contextNames) == 0 and, in that case, fall back to the default single empty
context (set contextNames = []string{""}) before iterating to call
getClientForContext and create contextClient entries; this ensures fetchSnapshot
uses the current context instead of silently producing zero clients.
🪄 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: a343131b-8f4b-488f-bef3-1b779b9a9a97
📒 Files selected for processing (18)
api/v1alpha1/flavor_group_capacity_types.goapi/v1alpha1/zz_generated.deepcopy.gohelm/library/cortex/files/crds/cortex.cloud_flavorgroupcapacities.yamlinternal/knowledge/extractor/plugins/compute/flavor_groups.gointernal/scheduling/reservations/capacity/controller.gointernal/scheduling/reservations/commitments/api/change_commitments.gointernal/scheduling/reservations/commitments/api/info.gointernal/scheduling/reservations/commitments/api/info_test.gointernal/scheduling/reservations/commitments/api/report_capacity_test.gointernal/scheduling/reservations/commitments/api/report_usage_test.gointernal/scheduling/reservations/commitments/api/usage_test.gointernal/scheduling/reservations/commitments/capacity.gointernal/scheduling/reservations/commitments/state.gointernal/scheduling/reservations/commitments/state_test.gointernal/scheduling/reservations/commitments/syncer.gointernal/scheduling/reservations/commitments/syncer_test.gointernal/scheduling/reservations/commitments/usage.gotools/visualize-committed-resources/main.go
Comment on lines
+53
to
+55
| logger.Info("service info not available yet", "error", err.Error()) | ||
| statusCode = http.StatusServiceUnavailable | ||
| http.Error(w, "Service temporarily unavailable: "+err.Error(), statusCode) |
There was a problem hiding this comment.
Keep the 503 body generic instead of echoing err.Error().
This exposes internal backend details through the public API response. The concrete failure should stay in logs; clients only need a stable 503 service temporarily unavailable message.
Proposed fix
logger.Info("service info not available yet", "error", err.Error())
statusCode = http.StatusServiceUnavailable
- http.Error(w, "Service temporarily unavailable: "+err.Error(), statusCode)
+ http.Error(w, "service temporarily unavailable", statusCode)
api.recordInfoMetrics(statusCode, startTime)
return📝 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
| logger.Info("service info not available yet", "error", err.Error()) | |
| statusCode = http.StatusServiceUnavailable | |
| http.Error(w, "Service temporarily unavailable: "+err.Error(), statusCode) | |
| logger.Info("service info not available yet", "error", err.Error()) | |
| statusCode = http.StatusServiceUnavailable | |
| http.Error(w, "service temporarily unavailable", statusCode) |
🤖 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/info.go` around lines 53 -
55, The response body currently echoes internal error text; change the
http.Error call in internal/scheduling/reservations/commitments/api/info.go so
it returns a generic message (e.g., "Service temporarily unavailable") instead
of concatenating err.Error(), keep statusCode = http.StatusServiceUnavailable,
and ensure the detailed error remains in logs (the existing logger.Info("service
info not available yet", "error", err.Error()) can stay or be promoted to
logger.Error if desired); update the http.Error invocation that references
statusCode and err.Error() to use the generic message only.
Comment on lines
+66
to
+67
| ramPerSlotGiB := groupData.SmallestFlavor.MemoryMB / 1024 | ||
| vcpusPerSlot := groupData.SmallestFlavor.VCPUs |
There was a problem hiding this comment.
Do not floor per-slot RAM to GiB.
MemoryMB / 1024 underreports any smallest flavor that is not an exact GiB multiple. For example, a 1536 MiB smallest flavor becomes 1 GiB/slot here, so the API would advertise less RAM usage and more placeable commitment capacity than the scheduler can really honor. This needs ceiling division as well.
Proposed fix
- ramPerSlotGiB := groupData.SmallestFlavor.MemoryMB / 1024
+ ramPerSlotGiB := uint64(0)
+ if groupData.SmallestFlavor.MemoryMB > 0 {
+ ramPerSlotGiB = (groupData.SmallestFlavor.MemoryMB + 1023) / 1024
+ }
vcpusPerSlot := groupData.SmallestFlavor.VCPUsAlso applies to: 107-118
🤖 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/capacity.go` around lines 66 -
67, The code currently computes ramPerSlotGiB by doing integer division of
groupData.SmallestFlavor.MemoryMB / 1024 which floors GiB and underreports RAM;
change this to use ceiling division so any fractional GiB rounds up (e.g.
(MemoryMB + 1023) / 1024 or use math.Ceil on a float conversion) and keep the
result as an integer type, and apply the same ceiling logic to the other
occurrence doing the same calculation (the block referencing ramPerSlotGiB and
groupData.SmallestFlavor.MemoryMB around lines 107-118).
Comment on lines
+298
to
+301
| // Calculate usage in GiB (FlavorRAM is in MiB) | ||
| var usageMultiple uint64 | ||
| if smallestMem := flavorToSmallestMemory[row.FlavorName]; smallestMem > 0 { | ||
| usageMultiple = row.FlavorRAM / smallestMem | ||
| if row.FlavorRAM > 0 { | ||
| usageMultiple = row.FlavorRAM / 1024 |
There was a problem hiding this comment.
Round VM RAM up when converting to GiB.
This now truncates partial GiB values. A 1536 MiB flavor reports as 1, and a 512 MiB flavor reports as 0, which undercounts RAM usage against GiB-based commitments and quotas. Use ceiling division here instead.
Proposed fix
var usageMultiple uint64
if row.FlavorRAM > 0 {
- usageMultiple = row.FlavorRAM / 1024
+ usageMultiple = (row.FlavorRAM + 1023) / 1024
}📝 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
| // Calculate usage in GiB (FlavorRAM is in MiB) | |
| var usageMultiple uint64 | |
| if smallestMem := flavorToSmallestMemory[row.FlavorName]; smallestMem > 0 { | |
| usageMultiple = row.FlavorRAM / smallestMem | |
| if row.FlavorRAM > 0 { | |
| usageMultiple = row.FlavorRAM / 1024 | |
| // Calculate usage in GiB (FlavorRAM is in MiB) | |
| var usageMultiple uint64 | |
| if row.FlavorRAM > 0 { | |
| usageMultiple = (row.FlavorRAM + 1023) / 1024 |
🤖 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/usage.go` around lines 298 -
301, The conversion from MiB to GiB in
internal/scheduling/reservations/commitments/usage.go currently truncates
partial GiB because usageMultiple is computed as row.FlavorRAM / 1024; change
this to use ceiling division so partial GiB rounds up (e.g., compute
(row.FlavorRAM + 1024 - 1) / 1024 or equivalent) when setting usageMultiple for
row.FlavorRAM to ensure 1536 MiB becomes 2 GiB and 512 MiB becomes 1 GiB.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/scheduling/reservations/quota/controller.go (3)
421-423: 💤 Low valueRedundant
flavorGroups[groupName]existence checks afterflavorToGrouplookup.
flavorToGroupis built bybuildFlavorToGroupMap(flavorGroups), so every value inflavorToGroupis by construction a key inflavorGroups. The newif _, ok := flavorGroups[groupName]; !okguards inaccumulateAddedVM(Lines 421-423),accumulateRemovedVM(Lines 520-522) andcomputeTotalUsage(Lines 691-693) are unreachable. Now that the unit size is no longer derived fromfg.SmallestFlavor.MemoryMB, these guards have no remaining purpose and theflavorGroupsparameter on these three call sites becomes effectively unused — consider dropping the guards (and optionally the parameter) to keep the API clean.Also applies to: 520-522, 691-693
🤖 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/quota/controller.go` around lines 421 - 423, The checks "if _, ok := flavorGroups[groupName]; !ok { return }" in accumulateAddedVM, accumulateRemovedVM and computeTotalUsage are redundant because groupName comes from flavorToGroup built by buildFlavorToGroupMap(flavorGroups), so every groupName is guaranteed to exist; remove these unreachable guards and any related dead usage of the flavorGroups parameter at those call sites; optionally simplify the function signatures for accumulateAddedVM/accumulateRemovedVM/computeTotalUsage by removing the flavorGroups parameter if it's no longer used elsewhere, updating all callers accordingly to keep the API clean.
762-784: ⚡ Quick winAsymmetric flavor-group existence check between memory and cores branches.
In
computeCRUsage, the memory branch skips the CR when itsFlavorGroupNameis absent fromflavorGroups(Lines 771-773), but the cores branch has no equivalent check, so a CR whose flavor group has been removed from knowledge will be excluded for RAM but still contribute cores under the (now orphaned)commitments.ResourceNameCores(spec.FlavorGroupName)key. Apply the existence check uniformly (e.g., once at the top of the loop), or drop it altogether — the unit conversion no longer needsflavorGroups, so the simpler choice is to remove the check unless filtering on “tracked group” is intentional.♻️ Proposed consolidation
// Filter: only matching states if !c.isCRStateIncluded(spec.State) { continue } // Get used amount from UsedResources map if len(cr.Status.UsedResources) == 0 { continue } + if _, ok := flavorGroups[spec.FlavorGroupName]; !ok { + continue + } + // Map ResourceType to resource name and extract used amount var resourceName string var usedAmount int64 switch spec.ResourceType { case v1alpha1.CommittedResourceTypeMemory: resourceName = commitments.ResourceNameRAM(spec.FlavorGroupName) memQty, ok := cr.Status.UsedResources["memory"] if !ok { continue } - // Convert bytes to GiB (1 GiB per commitment unit) usedBytes := memQty.Value() - if _, ok := flavorGroups[spec.FlavorGroupName]; !ok { - continue - } + // Convert bytes to GiB (1 GiB per commitment unit) usedAmount = usedBytes / (1024 * 1024 * 1024)🤖 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/quota/controller.go` around lines 762 - 784, computeCRUsage has an asymmetric check for flavorGroups: the memory branch skips CRs when spec.FlavorGroupName is missing but the cores branch does not, causing orphaned cores entries via commitments.ResourceNameCores(spec.FlavorGroupName); fix by performing the flavorGroups existence check once before the switch (or remove it entirely if you don't intend to filter by tracked groups) so both branches treat spec.FlavorGroupName uniformly; adjust around the switch in computeCRUsage and keep references to spec.FlavorGroupName, commitments.ResourceNameRAM and commitments.ResourceNameCores consistent.
669-677: ⚡ Quick winUpdate stale doc comment for
computeTotalUsage.The doc still describes the previous behavior ("Each flavor group has a 'smallest flavor' defining the unit size", with the 64 GiB / 32 GiB example), but the implementation now uses a fixed 1 GiB unit via
vmResourceUnits. Please align the comment with the new semantics so it doesn't mislead future readers.📝 Proposed doc update
// computeTotalUsage aggregates VM resources by project/AZ/resource. // // The RAM calculation converts server RAM into LIQUID commitment units: -// - Each flavor group has a "smallest flavor" defining the unit size (e.g., 32768 MiB) -// - A VM's RAM usage in units = VM_RAM_MiB / unit_size_MiB -// - Example: a 64 GiB VM in a group with 32 GiB smallest flavor = 2 units +// - Each commitment unit corresponds to a fixed 1 GiB of RAM +// - A VM's RAM usage in units = VM_RAM_GiB (integer GiB of memory) +// - Example: a 64 GiB VM contributes 64 RAM units regardless of flavor group // // This matches the unit system used by LIQUID for commitment tracking. // The per-AZ breakdown allows Limes to enforce AZ-level quota limits.🤖 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/quota/controller.go` around lines 669 - 677, The computeTotalUsage doc comment is stale: it describes a per-flavor-group "smallest flavor" unit but the code now uses vmResourceUnits (fixed 1 GiB) to convert VM RAM into units; update the comment for computeTotalUsage to state that RAM is converted to LIQUID units by dividing VM RAM (MiB) by vmResourceUnits (1 GiB in MiB), give a short example (e.g., 64 GiB VM → 64 units), and keep the note about per-AZ breakdown for quota enforcement; reference the computeTotalUsage function and the vmResourceUnits constant so readers can locate the implementation.
🤖 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.
Nitpick comments:
In `@internal/scheduling/reservations/quota/controller.go`:
- Around line 421-423: The checks "if _, ok := flavorGroups[groupName]; !ok {
return }" in accumulateAddedVM, accumulateRemovedVM and computeTotalUsage are
redundant because groupName comes from flavorToGroup built by
buildFlavorToGroupMap(flavorGroups), so every groupName is guaranteed to exist;
remove these unreachable guards and any related dead usage of the flavorGroups
parameter at those call sites; optionally simplify the function signatures for
accumulateAddedVM/accumulateRemovedVM/computeTotalUsage by removing the
flavorGroups parameter if it's no longer used elsewhere, updating all callers
accordingly to keep the API clean.
- Around line 762-784: computeCRUsage has an asymmetric check for flavorGroups:
the memory branch skips CRs when spec.FlavorGroupName is missing but the cores
branch does not, causing orphaned cores entries via
commitments.ResourceNameCores(spec.FlavorGroupName); fix by performing the
flavorGroups existence check once before the switch (or remove it entirely if
you don't intend to filter by tracked groups) so both branches treat
spec.FlavorGroupName uniformly; adjust around the switch in computeCRUsage and
keep references to spec.FlavorGroupName, commitments.ResourceNameRAM and
commitments.ResourceNameCores consistent.
- Around line 669-677: The computeTotalUsage doc comment is stale: it describes
a per-flavor-group "smallest flavor" unit but the code now uses vmResourceUnits
(fixed 1 GiB) to convert VM RAM into units; update the comment for
computeTotalUsage to state that RAM is converted to LIQUID units by dividing VM
RAM (MiB) by vmResourceUnits (1 GiB in MiB), give a short example (e.g., 64 GiB
VM → 64 units), and keep the note about per-AZ breakdown for quota enforcement;
reference the computeTotalUsage function and the vmResourceUnits constant so
readers can locate the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6f966ef-1b1b-469b-8438-d71521d5b183
📒 Files selected for processing (5)
internal/scheduling/reservations/commitments/capacity.gointernal/scheduling/reservations/commitments/usage.gointernal/scheduling/reservations/quota/controller.gointernal/scheduling/reservations/quota/controller_test.gointernal/scheduling/reservations/quota/integration_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/scheduling/reservations/commitments/usage.go
- internal/scheduling/reservations/commitments/capacity.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.0% |
Merged
This was referenced May 11, 2026
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.
Switch the LIQUID API commitment unit from "multiples of the flavor group's smallest flavor RAM" to a fixed 1 GiB per unit.