feat: use hypervisor CRD while watching CR reservation allocation events#711
Merged
feat: use hypervisor CRD while watching CR reservation allocation events#711
Conversation
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
📝 WalkthroughWalkthroughThe changes refactor committed resource reservation verification from a dual-source approach (Nova API + Hypervisor CRD) to use Hypervisor CRD exclusively as the source of truth. This includes removing Nova client usage, adding reactive event handling for Hypervisor changes via field indexing, updating controller reconciliation logic, and corresponding documentation and test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Ctrl as CommitmentReservationController
participant Rsv as Reservation Object
participant HV as Hypervisor CRD
participant Status as Status.CommittedResourceReservation
rect rgba(100, 150, 200, 0.5)
Note over Ctrl,Status: New Flow: Hypervisor CRD Only
end
Rsv->>Ctrl: Watch event (Reservation created/updated)
HV->>Ctrl: Watch event (Hypervisor changed)
Ctrl->>Ctrl: hypervisorToReservations() maps HV to Reservations
Ctrl->>Ctrl: Reconcile allocation (status.host in grace period?)
rect rgba(200, 150, 100, 0.5)
Note over Ctrl,Status: During Grace Period
end
Ctrl->>Rsv: Skip verification, keep in Spec
Ctrl->>Status: Set HasAllocationsInGracePeriod=true
rect rgba(150, 200, 100, 0.5)
Note over Ctrl,Status: After Grace Period
end
Ctrl->>HV: Query HV CRD for VM ID
alt VM exists on HV CRD
Ctrl->>Status: Confirm allocation
else VM missing on HV CRD
Ctrl->>Rsv: Remove from Spec.Allocations
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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/controller.go (1)
446-456:⚠️ Potential issue | 🟡 MinorPotential nil pointer dereference after re-fetch.
After
r.Getat line 447, the resource is re-fetched from the API server, overwriting the localresobject. If the stored resource hasStatus.CommittedResourceReservation == nil(e.g., status was never set), line 454 will panic when assigning tonil.Allocations.🐛 Proposed fix: re-initialize status after re-fetch
// Re-fetch to get the updated resource version for status patch if err := r.Get(ctx, client.ObjectKeyFromObject(res), res); err != nil { if client.IgnoreNotFound(err) == nil { return result, nil } return nil, fmt.Errorf("failed to re-fetch reservation: %w", err) } + // Re-initialize status struct if nil after re-fetch + if res.Status.CommittedResourceReservation == nil { + res.Status.CommittedResourceReservation = &v1alpha1.CommittedResourceReservationStatus{} + } // Re-apply the status update that was overwritten by the re-fetch. res.Status.CommittedResourceReservation.Allocations = newStatusAllocations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/commitments/controller.go` around lines 446 - 456, The re-fetch can leave res.Status.CommittedResourceReservation nil which will panic when assigning to res.Status.CommittedResourceReservation.Allocations; after the r.Get(...) call and before assigning newStatusAllocations, ensure res.Status and res.Status.CommittedResourceReservation are non-nil (instantiate them if nil) so the assignment is safe, then proceed to set Allocations and update old = res.DeepCopy() (referencing r.Get, res.Status.CommittedResourceReservation, newStatusAllocations, and old = res.DeepCopy()).
🤖 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/controller.go`:
- Around line 446-456: The re-fetch can leave
res.Status.CommittedResourceReservation nil which will panic when assigning to
res.Status.CommittedResourceReservation.Allocations; after the r.Get(...) call
and before assigning newStatusAllocations, ensure res.Status and
res.Status.CommittedResourceReservation are non-nil (instantiate them if nil) so
the assignment is safe, then proceed to set Allocations and update old =
res.DeepCopy() (referencing r.Get, res.Status.CommittedResourceReservation,
newStatusAllocations, and old = res.DeepCopy()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb867e7c-653d-4327-8f0d-25dd40db839b
📒 Files selected for processing (4)
docs/reservations/committed-resource-reservations.mdinternal/scheduling/reservations/commitments/config.gointernal/scheduling/reservations/commitments/controller.gointernal/scheduling/reservations/commitments/controller_test.go
💤 Files with no reviewable changes (1)
- internal/scheduling/reservations/commitments/config.go
This was referenced Apr 17, 2026
Merged
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.