fix: refactor commitments package#726
Merged
Merged
Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
📝 WalkthroughWalkthroughThis PR refactors the commitments API package structure by moving HTTP API handlers from the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Contributor
Test Coverage ReportTest Coverage 📊: 69.2% |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/api/change_commitments_metrics.go (1)
25-32:⚠️ Potential issue | 🟠 MajorUse the pre-initialized
acceptedlabel for successful changes.
recordMetrics()emitsresult="success", but the monitor pre-initializesaccepted/rejected. This creates a separate runtime series and leaves the intendedacceptedseries at zero, which can break dashboards or alerts keyed on the pre-initialized label.Proposed fix
- result := "success" + result := "accepted" if resp.RejectionReason != "" { result = "rejected" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/api/change_commitments_metrics.go` around lines 25 - 32, The metrics use the wrong label value for successful commitment changes: replace the hardcoded "success" with the pre-initialized "accepted" label in the code that computes the result (the variable currently set in the block above the call to api.monitor.commitmentChanges.WithLabelValues(...)). Ensure the logic in the function (the result variable used before calling api.monitor.commitmentChanges.WithLabelValues(result).Add(...)) sets result = "accepted" when resp.RejectionReason == "" and keeps "rejected" when non-empty so the metric updates the existing pre-initialized series.
🧹 Nitpick comments (2)
internal/scheduling/reservations/commitments/api/report_usage.go (1)
36-36: Logger inconsistency with peer handlers.
report_capacity.go,info.go, andchange_commitments.goall construct their logger viacommitments.LoggerFromContext(ctx).WithValues(...)(after wrapping the request context withreservations.WithGlobalRequestID), which attachesgreq/reqtracing IDs. Here you useapiLog.WithValues(...)directly, so report-usage logs will lack those correlation IDs and won't match the rest of the API's log schema.Consider aligning with the other handlers for consistent observability.
♻️ Proposed fix
- log := apiLog.WithValues("requestID", requestID, "endpoint", "report-usage") + ctx := reservations.WithGlobalRequestID(r.Context(), "committed-resource-"+requestID) + log := commitments.LoggerFromContext(ctx).WithValues("component", "api", "endpoint", "report-usage")Then pass
ctxintocalculator.CalculateUsage(ctx, ...)instead ofr.Context().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/api/report_usage.go` at line 36, The handler builds its logger from apiLog directly which omits request-scoped tracing IDs; replace apiLog.WithValues(...) with commitments.LoggerFromContext(ctx).WithValues("requestID", requestID, "endpoint", "report-usage") after wrapping the request context via reservations.WithGlobalRequestID so the greq/req IDs are present, and ensure you pass the request-scoped ctx into calculator.CalculateUsage(ctx, ...) (not r.Context()) so downstream code receives the same enriched context and logs/traces consistently.internal/scheduling/reservations/commitments/usage_internals_test.go (1)
79-144: Add import and usetestlib.Ptrinstead of local-variable addresses for pointer fields.Lines 81-82, 97-98, 105-106, 113-114, 121-123 create
time1/time2locals solely to take their address. The same pattern appears at line 229 with&vram. Per coding guidelines, usetestlib.Ptrfor test pointer values. This requires adding the import:testlib "github.com/cobaltcore-dev/cortex/pkg/testing"Then replace
&time1,&time2, and&vramwithtestlib.Ptr(time1),testlib.Ptr(time2), andtestlib.Ptr(vram)respectively.♻️ Proposed changes
+import ( + "testing" + "time" + + testlib "github.com/cobaltcore-dev/cortex/pkg/testing" +)Then in
TestSortCommitmentsForAssignment:- baseTime := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) - time1 := baseTime - time2 := baseTime.Add(1 * time.Hour) + time1 := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC) + time2 := time1.Add(1 * time.Hour)And replace all
&time1,&time2withtestlib.Ptr(time1),testlib.Ptr(time2).In the "with video RAM" test around line 229, replace
&vramwithtestlib.Ptr(vram).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/usage_internals_test.go` around lines 79 - 144, Import testlib ("github.com/cobaltcore-dev/cortex/pkg/testing") and replace all occurrences of taking addresses of local time or numeric vars in TestSortCommitmentsForAssignment (and the similar test around the vram variable) with testlib.Ptr calls: instead of &time1 / &time2 / &vram use testlib.Ptr(time1) / testlib.Ptr(time2) / testlib.Ptr(vram) on the CommitmentState fields in the CommitmentStateWithUsage test inputs used by sortCommitmentsForAssignment so pointer values follow the project test helper convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@internal/scheduling/reservations/commitments/api/change_commitments_metrics.go`:
- Around line 25-32: The metrics use the wrong label value for successful
commitment changes: replace the hardcoded "success" with the pre-initialized
"accepted" label in the code that computes the result (the variable currently
set in the block above the call to
api.monitor.commitmentChanges.WithLabelValues(...)). Ensure the logic in the
function (the result variable used before calling
api.monitor.commitmentChanges.WithLabelValues(result).Add(...)) sets result =
"accepted" when resp.RejectionReason == "" and keeps "rejected" when non-empty
so the metric updates the existing pre-initialized series.
---
Nitpick comments:
In `@internal/scheduling/reservations/commitments/api/report_usage.go`:
- Line 36: The handler builds its logger from apiLog directly which omits
request-scoped tracing IDs; replace apiLog.WithValues(...) with
commitments.LoggerFromContext(ctx).WithValues("requestID", requestID,
"endpoint", "report-usage") after wrapping the request context via
reservations.WithGlobalRequestID so the greq/req IDs are present, and ensure you
pass the request-scoped ctx into calculator.CalculateUsage(ctx, ...) (not
r.Context()) so downstream code receives the same enriched context and
logs/traces consistently.
In `@internal/scheduling/reservations/commitments/usage_internals_test.go`:
- Around line 79-144: Import testlib
("github.com/cobaltcore-dev/cortex/pkg/testing") and replace all occurrences of
taking addresses of local time or numeric vars in
TestSortCommitmentsForAssignment (and the similar test around the vram variable)
with testlib.Ptr calls: instead of &time1 / &time2 / &vram use
testlib.Ptr(time1) / testlib.Ptr(time2) / testlib.Ptr(vram) on the
CommitmentState fields in the CommitmentStateWithUsage test inputs used by
sortCommitmentsForAssignment so pointer values follow the project test helper
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07a51646-efab-4819-82f4-00d9f74f0473
📒 Files selected for processing (27)
cmd/manager/main.gointernal/scheduling/reservations/commitments/api/change_commitments.gointernal/scheduling/reservations/commitments/api/change_commitments_metrics.gointernal/scheduling/reservations/commitments/api/change_commitments_monitor.gointernal/scheduling/reservations/commitments/api/change_commitments_monitor_test.gointernal/scheduling/reservations/commitments/api/change_commitments_test.gointernal/scheduling/reservations/commitments/api/handler.gointernal/scheduling/reservations/commitments/api/info.gointernal/scheduling/reservations/commitments/api/info_monitor.gointernal/scheduling/reservations/commitments/api/info_test.gointernal/scheduling/reservations/commitments/api/quota.gointernal/scheduling/reservations/commitments/api/report_capacity.gointernal/scheduling/reservations/commitments/api/report_capacity_monitor.gointernal/scheduling/reservations/commitments/api/report_capacity_test.gointernal/scheduling/reservations/commitments/api/report_usage.gointernal/scheduling/reservations/commitments/api/report_usage_monitor.gointernal/scheduling/reservations/commitments/api/report_usage_test.gointernal/scheduling/reservations/commitments/api/usage_test.gointernal/scheduling/reservations/commitments/state.gointernal/scheduling/reservations/commitments/state_test.gointernal/scheduling/reservations/commitments/syncer.gointernal/scheduling/reservations/commitments/syncer_monitor.gointernal/scheduling/reservations/commitments/syncer_monitor_test.gointernal/scheduling/reservations/commitments/syncer_test.gointernal/scheduling/reservations/commitments/usage.gointernal/scheduling/reservations/commitments/usage_db.gointernal/scheduling/reservations/commitments/usage_internals_test.go
💤 Files with no reviewable changes (1)
- internal/scheduling/reservations/commitments/usage_db.go
3 tasks
mblos
pushed a commit
that referenced
this pull request
Apr 27, 2026
…tion (#758) ## Summary - **Bug**: In `reconcileAllocations`, when stale VM allocations are removed from the spec (specChanged=true), the status patch silently becomes a no-op. This happens because `old = res.DeepCopy()` was called AFTER `res.Status.CommittedResourceReservation.Allocations = newStatusAllocations`, making `old` and `res` identical for the status fields. The subsequent `client.MergeFrom(old)` patch contained no status diff. - **Found by**: Weekly automated review of recent changes (#726, #730 refactors) - **Fix**: Move the `res.DeepCopy()` call before the status re-application so the merge-from base correctly reflects the pre-update state, producing the intended status diff. ## Impact When a committed-resource reservation has stale allocations removed (VMs that left the hypervisor), the status.allocations field was not being updated to reflect verified allocations. This could cause the usage reconciler to work with stale allocation data and prevent accurate tracking of committed resource usage. ## Test plan - [x] Existing `TestReconcileAllocations_HypervisorCRDPath` tests pass - [x] All commitments package tests pass - [ ] Verify in staging that status.allocations is updated when stale allocations are removed from spec 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Bug Detective <claude@anthropic.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.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
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.