fix(CR): align the RAM resource unit exposed to Limes for CR/quota based on fixed/varying ram/core ratio#841
Merged
Merged
Conversation
…sed on fixed/varying ram/core ratio
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR threads support for fixed-ratio flavor groups through the commitments subsystem by parameterizing RAM units. Entry points compute ChangesRAM Unit Support for Fixed-Ratio and Variable-Ratio Flavor Groups
Sequence Diagram(s)sequenceDiagram
participant Client
participant QuotaHandler
participant FlavorGroupLookup
participant ProjectQuotaAPI
Client->>QuotaHandler: PUT /quota (ram quota in flavor-declared units)
QuotaHandler->>FlavorGroupLookup: GetAllFlavorGroups()
FlavorGroupLookup-->>QuotaHandler: flavor-group data (ram unit info)
QuotaHandler->>QuotaHandler: convert RAM from declared units -> GiB
QuotaHandler->>ProjectQuotaAPI: create/update ProjectQuota CRD (GiB values)
ProjectQuotaAPI-->>QuotaHandler: persistence result
QuotaHandler-->>Client: HTTP 200 / error (503 if flavor-group data missing)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
🧹 Nitpick comments (1)
internal/scheduling/reservations/commitments/api/info.go (1)
95-100: 💤 Low valueUpdate the function doc to reflect conditional RAM unit behavior.
The comment states
_ram: RAM resource (unit = multiples of smallest flavor RAM, ...), but the implementation now selects the unit conditionally (lines 146-155):
- Fixed-ratio groups:
UnitNone(slot-based)- Variable-ratio groups:
UnitGibibytes(GiB-based)Suggest revising the comment to reflect this branching logic, e.g., "unit = slot count for fixed-ratio groups, GiB for variable-ratio groups".
🤖 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 95 - 100, Update the buildServiceInfo function doc to reflect that the RAM resource unit is chosen conditionally: for fixed-ratio flavor groups the `_ram` resource uses slot-based units (UnitNone) and handles commitments, while for variable-ratio groups `_ram` uses GiB units (UnitGibibytes) and does not handle commitments; keep the rest of the description about `_cores` and `_instances` unchanged and mention that only fixed-ratio groups accept commitments.
🤖 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/commitments/api/info.go`:
- Around line 95-100: Update the buildServiceInfo function doc to reflect that
the RAM resource unit is chosen conditionally: for fixed-ratio flavor groups the
`_ram` resource uses slot-based units (UnitNone) and handles commitments, while
for variable-ratio groups `_ram` uses GiB units (UnitGibibytes) and does not
handle commitments; keep the rest of the description about `_cores` and
`_instances` unchanged and mention that only fixed-ratio groups accept
commitments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5400588-63ea-42b8-ae2c-a972517a6f39
📒 Files selected for processing (7)
internal/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/capacity.gointernal/scheduling/reservations/commitments/state.gointernal/scheduling/reservations/commitments/usage.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.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.
All CR LIQUID API endpoints are now unit-aware per flavor group: fixed-ratio groups count RAM in slots, variable-ratio groups count in GiB — consistently across capacity, usage, quota, and commitment endpoints.