Merged
Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
📝 WalkthroughWalkthroughThe changes migrate VM usage retrieval in the commitments API from Nova client calls to direct Postgres database queries. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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: 2
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/commitments/usage.go (1)
197-260:⚠️ Potential issue | 🟠 MajorFallback to Knowledge flavor data when the Postgres flavor join is missing.
This code only uses
row.FlavorRAM,row.FlavorVCPUs, androw.FlavorDisk, but the DB query deliberatelyLEFT JOINs flavors and fills missing rows with zeroes. Any VM whose flavor is absent fromopenstack_flavors_v2will therefore be reported withUsageMultiple == 0, zero core usage, and zeroed flavor attributes even thoughflavorGroupsalready contains the canonical flavor definition by name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/usage.go` around lines 197 - 260, The code currently trusts row.FlavorRAM/FlavorVCPUs/FlavorDisk which are zero when the LEFT JOIN misses — update the VM construction to fall back to the canonical flavor data from flavorGroups when row values are zero: use flavorToGroup[row.FlavorName] to find the group, then locate the matching flavor within flavorGroups[groupName].Flavors (by flavor.Name == row.FlavorName) and if row.FlavorRAM/FlavorVCPUs/FlavorDisk are zero replace them with the canonical flavor's MemoryMB/VCPUs/DiskGB before computing usageMultiple; compute usageMultiple from the effective memory value (canonical or row) and flavorToSmallestMemory[row.FlavorName] as currently done, and then populate VMUsageInfo with those effective values so missing DB join rows no longer produce zeroed usage.
🤖 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.go`:
- Line 42: The usageDB field is being mutated after NewAPI() without
synchronization (SetUsageDB) which allows HandleReportUsage to observe a nil or
race; fix by making the dependency safe: either require wiring usageDB before
registering/serving handlers in NewAPI(), or protect access with a
concurrency-safe accessor (e.g., store UsageDBClient in an atomic.Value or
behind a sync.RWMutex) and have HandleReportUsage call that accessor and return
HTTP 503 when it yields nil until the DB is set; update NewAPI(), SetUsageDB(),
and HandleReportUsage to use the chosen accessor (referencing usageDB, NewAPI(),
SetUsageDB(), HandleReportUsage) so there are no unsynchronized reads/writes.
In `@internal/scheduling/reservations/commitments/usage.go`:
- Around line 187-190: The code currently returns an empty []VMUsageInfo when
c.usageDB is nil which masks missing startup wiring; change this to fail closed
by returning an error when c.usageDB == nil so callers know usage data is
unavailable (e.g., return nil, fmt.Errorf("usage DB client not configured") or a
typed sentinel error). Update the branch that checks c.usageDB (the block that
logs "usage DB client not configured - returning empty VM list") to log the
condition and return an explicit error instead of an empty slice, and ensure
callers of this method propagate/handle that error; reference the c.usageDB
check and the SetUsageDB wiring mentioned in the review.
---
Outside diff comments:
In `@internal/scheduling/reservations/commitments/usage.go`:
- Around line 197-260: The code currently trusts
row.FlavorRAM/FlavorVCPUs/FlavorDisk which are zero when the LEFT JOIN misses —
update the VM construction to fall back to the canonical flavor data from
flavorGroups when row values are zero: use flavorToGroup[row.FlavorName] to find
the group, then locate the matching flavor within
flavorGroups[groupName].Flavors (by flavor.Name == row.FlavorName) and if
row.FlavorRAM/FlavorVCPUs/FlavorDisk are zero replace them with the canonical
flavor's MemoryMB/VCPUs/DiskGB before computing usageMultiple; compute
usageMultiple from the effective memory value (canonical or row) and
flavorToSmallestMemory[row.FlavorName] as currently done, and then populate
VMUsageInfo with those effective values so missing DB join rows no longer
produce zeroed usage.
🪄 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: 2285eae5-4e68-4dd4-b080-0d92a617ebc3
📒 Files selected for processing (9)
cmd/manager/main.gohelm/bundles/cortex-nova/values.yamlinternal/scheduling/reservations/commitments/api.gointernal/scheduling/reservations/commitments/api_report_usage.gointernal/scheduling/reservations/commitments/api_report_usage_test.gointernal/scheduling/reservations/commitments/controller.gointernal/scheduling/reservations/commitments/usage.gointernal/scheduling/reservations/commitments/usage_db.gointernal/scheduling/reservations/commitments/usage_test.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.1% |
mblos
changed the title
This was referenced Apr 14, 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.
No description provided.