Flavor group mem/cpu ratio; Committed resource APIs only for fixed ratio#612
Merged
Flavor group mem/cpu ratio; Committed resource APIs only for fixed ratio#612
Conversation
… flavor groups with fixed ratio
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
mblos
changed the title
|
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:
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 (1)
📝 WalkthroughWalkthroughAdds RAM-per-vCPU metadata to flavor groups, computes fixed-or-range MiB/vCPU ratios in the extractor and tests, exposes ratio attributes in service info, and gates commitment acceptance so only flavor groups with a fixed RAM/core ratio accept committed resources. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant API as Commitments API
participant Knowledge as Knowledge CR
participant Extractor as Flavor Extractor
participant Eligibility as Eligibility Check
Client->>API: POST /v1/commitments/change
API->>Knowledge: Read flavor-group data
Knowledge-->>API: FlavorGroupFeature (includes ramCoreRatio fields)
API->>Eligibility: FlavorGroupAcceptsCommitments(fg)
Eligibility->>Extractor: fg.HasFixedRamCoreRatio()
alt fixed ratio (accept)
Eligibility-->>API: accept
API->>API: process commitments
API-->>Client: success
else variable/missing ratio (reject)
Eligibility-->>API: reject + reason
API-->>Client: rejection with reason
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
mblos
changed the title
mblos
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/knowledge/extractor/plugins/compute/flavor_groups.go`:
- Around line 143-166: The integer division when computing ratio := f.MemoryMB /
f.VCPUs truncates fractional MB/vCPU values and can make distinct ratios compare
equal; update the logic in the loop that sets minRatio/maxRatio (and the
subsequent ramCoreRatio/ramCoreRatioMin/ramCoreRatioMax assignments) to compare
exact ratios by normalizing MemoryMB and VCPUs using a GCD (implement a local
gcd(a,b uint64) helper) and store/compare the normalized numerator/denominator
pair (or a single reduced numerator/denominator key) instead of the truncated
quotient so HasFixedRamCoreRatio() and FlavorGroupAcceptsCommitments() get
correct fixed/variable ratio classification.
In `@internal/scheduling/reservations/commitments/api_info_test.go`:
- Line 105: Update the stale expectation comment in the test that describes
variable-ratio groups: locate the comment near the variable-ratio group test in
internal/scheduling/reservations/commitments/api_info_test.go (the comment above
the test case for variable-ratio groups) and change the text so it correctly
states HasCapacity=false (and still HandlesCommitments=false) to match the test
and implementation expectations for variable-ratio groups.
In `@internal/scheduling/reservations/commitments/flavor_group_eligibility.go`:
- Around line 21-33: The rejection message currently prints min=0,max=0 when
both fg.RamCoreRatioMin and fg.RamCoreRatioMax are nil which is misleading; in
FlavorGroupCommitmentRejectionReason, detect when both pointers are nil and
return a distinct message like "flavor group %q has no RAM/core ratio metadata
and does not accept commitments" instead of formatting min/max zeros, otherwise
keep the existing behavior that dereferences RamCoreRatioMin and RamCoreRatioMax
and formats the variable ratio message; reference the function
FlavorGroupCommitmentRejectionReason, the helper FlavorGroupAcceptsCommitments,
and the fields RamCoreRatioMin, RamCoreRatioMax and Name to locate where to add
the nil-check and alternate message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9605a033-7b3a-4bcb-9565-d42debdb19b1
📒 Files selected for processing (10)
internal/knowledge/extractor/plugins/compute/flavor_groups.gointernal/knowledge/extractor/plugins/compute/flavor_groups_test.gointernal/scheduling/reservations/commitments/api.gointernal/scheduling/reservations/commitments/api_change_commitments.gointernal/scheduling/reservations/commitments/api_change_commitments_test.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/capacity.gointernal/scheduling/reservations/commitments/flavor_group_eligibility.go
Comment on lines
+21
to
+33
| func FlavorGroupCommitmentRejectionReason(fg *compute.FlavorGroupFeature) string { | ||
| if FlavorGroupAcceptsCommitments(fg) { | ||
| return "" | ||
| } | ||
| var minRatio, maxRatio uint64 | ||
| if fg.RamCoreRatioMin != nil { | ||
| minRatio = *fg.RamCoreRatioMin | ||
| } | ||
| if fg.RamCoreRatioMax != nil { | ||
| maxRatio = *fg.RamCoreRatioMax | ||
| } | ||
| return fmt.Sprintf("flavor group %q has variable RAM/core ratio (min=%d, max=%d) and does not accept commitments", | ||
| fg.Name, minRatio, maxRatio) |
There was a problem hiding this comment.
Differentiate “missing ratio metadata” from “variable ratio” in rejection reason.
Line 32 currently reports min=0, max=0 when both pointers are nil, which can mislead operators. Return a dedicated message when no computable ratio metadata exists.
💡 Suggested adjustment
func FlavorGroupCommitmentRejectionReason(fg *compute.FlavorGroupFeature) string {
if FlavorGroupAcceptsCommitments(fg) {
return ""
}
+ if fg.RamCoreRatioMin == nil && fg.RamCoreRatioMax == nil {
+ return fmt.Sprintf("flavor group %q has no computable RAM/core ratio and does not accept commitments", fg.Name)
+ }
var minRatio, maxRatio uint64
if fg.RamCoreRatioMin != nil {
minRatio = *fg.RamCoreRatioMin
}
if fg.RamCoreRatioMax != nil {
maxRatio = *fg.RamCoreRatioMax
}
return fmt.Sprintf("flavor group %q has variable RAM/core ratio (min=%d, max=%d) and does not accept commitments",
fg.Name, minRatio, maxRatio)
}📝 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
| func FlavorGroupCommitmentRejectionReason(fg *compute.FlavorGroupFeature) string { | |
| if FlavorGroupAcceptsCommitments(fg) { | |
| return "" | |
| } | |
| var minRatio, maxRatio uint64 | |
| if fg.RamCoreRatioMin != nil { | |
| minRatio = *fg.RamCoreRatioMin | |
| } | |
| if fg.RamCoreRatioMax != nil { | |
| maxRatio = *fg.RamCoreRatioMax | |
| } | |
| return fmt.Sprintf("flavor group %q has variable RAM/core ratio (min=%d, max=%d) and does not accept commitments", | |
| fg.Name, minRatio, maxRatio) | |
| func FlavorGroupCommitmentRejectionReason(fg *compute.FlavorGroupFeature) string { | |
| if FlavorGroupAcceptsCommitments(fg) { | |
| return "" | |
| } | |
| if fg.RamCoreRatioMin == nil && fg.RamCoreRatioMax == nil { | |
| return fmt.Sprintf("flavor group %q has no computable RAM/core ratio and does not accept commitments", fg.Name) | |
| } | |
| var minRatio, maxRatio uint64 | |
| if fg.RamCoreRatioMin != nil { | |
| minRatio = *fg.RamCoreRatioMin | |
| } | |
| if fg.RamCoreRatioMax != nil { | |
| maxRatio = *fg.RamCoreRatioMax | |
| } | |
| return fmt.Sprintf("flavor group %q has variable RAM/core ratio (min=%d, max=%d) and does not accept commitments", | |
| fg.Name, minRatio, maxRatio) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/scheduling/reservations/commitments/flavor_group_eligibility.go`
around lines 21 - 33, The rejection message currently prints min=0,max=0 when
both fg.RamCoreRatioMin and fg.RamCoreRatioMax are nil which is misleading; in
FlavorGroupCommitmentRejectionReason, detect when both pointers are nil and
return a distinct message like "flavor group %q has no RAM/core ratio metadata
and does not accept commitments" instead of formatting min/max zeros, otherwise
keep the existing behavior that dereferences RamCoreRatioMin and RamCoreRatioMax
and formats the variable ratio message; reference the function
FlavorGroupCommitmentRejectionReason, the helper FlavorGroupAcceptsCommitments,
and the fields RamCoreRatioMin, RamCoreRatioMax and Name to locate where to add
the nil-check and alternate message.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/scheduling/reservations/commitments/api_info.go`:
- Around line 96-100: The code uses an undefined identifier `log` when handling
the json.Marshal error for `attrs` (attrsJSON, err := json.Marshal(attrs)),
causing a compile error; replace the `log.Error(...)` call with the provided
`logger` parameter (i.e., use `logger.Error(...)`) to match the rest of the
function and ensure the error is logged via the existing `logger logr.Logger`
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19eed3c5-4de3-438b-bfba-b32343ed607d
📒 Files selected for processing (4)
internal/scheduling/reservations/commitments/api.gointernal/scheduling/reservations/commitments/api_change_commitments.gointernal/scheduling/reservations/commitments/api_change_commitments_test.gointernal/scheduling/reservations/commitments/api_info.go
✅ Files skipped from review due to trivial changes (1)
- internal/scheduling/reservations/commitments/api.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/scheduling/reservations/commitments/api_change_commitments.go
Contributor
Test Coverage ReportTest Coverage 📊: 68.0% |
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.
No description provided.