feat(reservations): follow VM live migration in CR reservation reconciler - #1048
Merged
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds reservation capacity evaluation and updates allocation reconciliation to detect confirmed VM migrations. The controller follows eligible migrations when the destination has capacity and removes allocations when migration cannot be followed. ChangesReservation migration reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReservationController
participant Hypervisors
participant Reservations
participant HostHasCapacityForReservation
ReservationController->>Hypervisors: load hypervisors
ReservationController->>Reservations: load reservations
ReservationController->>Hypervisors: find confirmed VM on another host
ReservationController->>HostHasCapacityForReservation: check destination capacity
HostHasCapacityForReservation-->>ReservationController: return capacity result
ReservationController->>Reservations: update target host or remove allocation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
juliusclausnitzer
changed the title
juliusclausnitzer
force-pushed
the
wm-373-reservation-live-migration
branch
from
July 28, 2026 12:43
1463e3b to
edc378c
Compare
juliusclausnitzer
force-pushed
the
wm-373-reservation-live-migration
branch
from
July 28, 2026 12:45
edc378c to
9bc0d64
Compare
juliusclausnitzer
force-pushed
the
wm-373-reservation-live-migration
branch
from
July 28, 2026 13:38
91f3622 to
ec9a0c8
Compare
juliusclausnitzer
marked this pull request as ready for review
August 3, 2026 09:29
juliusclausnitzer
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
mblos and
umswmayj
as code owners
August 3, 2026 09:29
Signed-off-by: Julius Clausnitzer <julius.clausnitzer@sap.com> Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
mblos
requested changes
Aug 3, 2026
mblos
left a comment
mblos
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the PR. Some things have changed since ticket was updated, so I suggest a slightly refined (and simplified) semantic as follows, wdyt:
- Single VM in reservation: if new host fits -> follow (update TargetHost). If not -> remove VM from reservation (slot stays on old host, available for re-use).
- Multiple VMs in reservation: always remove the migrated VM, never update TargetHost. moving it would make the remaining VMs appear "misplaced" on the next reconcile.
Some consequences:
- This makes the
VMMisplacedcondition obsolete, we can remove it. Good to have a proper log message to note what has happened when CR VMs are migrated. For the cases where we used the misplaced condition, we can simply remove the VM from the reservation. - We can guard the TargetHost update with only for
len(allocated VMs) == 1.
More complex cases like cross-reservation reassignment, re-mapping of VMs, .. are considered when the VM crd exists.
Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
![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
🧹 Nitpick comments (6)
internal/scheduling/reservations/commitments/reservation_controller_test.go (2)
1152-1158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
wantStatusHost == oldHostbranch.No test case sets
wantStatusHosttooldHost, sooldInstancesis always empty. The condition adds a second meaning to the expectation field. Delete it and pass an empty instance list.♻️ Proposed change
if !addsOldHostHV { - oldInstances := []hv1.Instance{} - if tt.wantStatusHost == oldHost { - oldInstances = []hv1.Instance{{ID: vmUUID, Active: true}} - } - objects = append(objects, newTestHypervisorCRD(oldHost, oldInstances)) + objects = append(objects, newTestHypervisorCRD(oldHost, []hv1.Instance{})) }🤖 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/reservation_controller_test.go` around lines 1152 - 1158, In the test setup around the !addsOldHostHV branch, remove the wantStatusHost == oldHost condition and oldInstances variable, and pass an empty instance list directly to newTestHypervisorCRD for oldHost.
1066-1074: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a second reconcile pass to this case.
The case asserts only the first pass. It therefore does not detect that
Status.Hostremainshost-oldafterSpec.TargetHostmoves tohost-new. See the comment onreservation_controller.goLines 594-601. CallreconcileAllocationsa second time with the updated object and assert thatSpec.TargetHost,Status.Host, andStatus.Allocations[vmUUID]all stay athost-newand that the VM stays inSpec.Allocations.🤖 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/reservation_controller_test.go` around lines 1066 - 1074, Extend the “single VM, new host has capacity: follow the VM” test to invoke reconcileAllocations a second time using the updated reservation object. Assert on the second pass that Spec.TargetHost, Status.Host, and Status.Allocations[vmUUID] remain at newHost, while the VM remains in Spec.Allocations.internal/scheduling/reservations/capacity_accounting_test.go (1)
191-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn a pointer from
resWithSlotto remove the inline closures.Each
resfield wraps the helper in an immediately-invoked function only to take an address. IfresWithSlotreturns*v1alpha1.Reservation, the table becomes shorter. Theothersentries then need a dereference, or add a second one-line value helper.♻️ Proposed change
- resWithSlot := func(name, targetHost string, memGiB, cpuCores int64) v1alpha1.Reservation { - return v1alpha1.Reservation{ + resWithSlot := func(name, targetHost string, memGiB, cpuCores int64) *v1alpha1.Reservation { + return &v1alpha1.Reservation{Then, for example:
- res: func() *v1alpha1.Reservation { r := resWithSlot("res-1", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-1", "host-old", 480, 40),and for blockers:
- resWithSlot("res-blocker", "host-new", 480, 40), + *resWithSlot("res-blocker", "host-new", 480, 40),🤖 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/capacity_accounting_test.go` around lines 191 - 204, Change the local resWithSlot helper to return *v1alpha1.Reservation, allowing table entries that assign res to call it directly without inline immediately-invoked closures. Update others entries to dereference the returned pointer, or add a concise value-returning helper for those cases while preserving the existing reservation contents.internal/scheduling/reservations/commitments/reservation_controller.go (2)
514-537: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider building the VM→host index once.
The scan iterates all hypervisors and all their instances for each missing confirmed VM. With many hypervisors and several missing VMs the cost is O(misses × HVs × instances). Build one
map[string]string(VM UUID → host name) insideensureHVsAndReservations, then look up each miss in constant time. The current cost is acceptable for a single miss, so treat this as optional.🤖 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/reservation_controller.go` around lines 514 - 537, Optionally optimize the missing-VM lookup by building a VM UUID-to-host map once inside ensureHVsAndReservations, after allHVs is loaded. Populate it by iterating each hypervisor’s Status.Instances, then replace the nested scan in the confirmed-VM reconciliation path with a constant-time lookup while preserving the existing expectedHost exclusion and found-host behavior.
450-463: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAssign
allHVsonly after both lists succeed.
allHVsis assigned before theListcalls. If the secondListfails,allHVsis non-nil andallReservationsis nil. The current code returns that error to the caller, so no nil dereference occurs today. A later refactor that continues after the error would panic atallReservations.Items. Guard on both variables.♻️ Proposed change
ensureHVsAndReservations := func() error { - if allHVs != nil { + if allHVs != nil && allReservations != nil { return nil } - allHVs = &hv1.HypervisorList{} - if err := r.List(ctx, allHVs); err != nil { + hvs := &hv1.HypervisorList{} + if err := r.List(ctx, hvs); err != nil { return fmt.Errorf("failed to list hypervisors: %w", err) } - allReservations = &v1alpha1.ReservationList{} - if err := r.List(ctx, allReservations); err != nil { + resList := &v1alpha1.ReservationList{} + if err := r.List(ctx, resList); err != nil { return fmt.Errorf("failed to list reservations: %w", err) } + allHVs, allReservations = hvs, resList return nil }🤖 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/reservation_controller.go` around lines 450 - 463, The ensureHVsAndReservations closure should only publish the cached allHVs and allReservations values after both List calls succeed. Load the hypervisors and reservations into temporary local lists, return immediately on either error, then assign both closure variables together; update the cache guard to require both variables to be initialized.internal/scheduling/reservations/capacity_accounting.go (1)
66-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing the redundant zero comparison.
remaining.Cmp(required) < 0already rejects any negativeremainingwhenrequiredis non-negative.Spec.Resourcesquantities are non-negative in practice, so thezerocheck adds no behavior. Keep it only if you intend to guard against a negativerequired.🤖 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/capacity_accounting.go` around lines 66 - 78, Remove the redundant zero comparison and the associated zero quantity in the capacity validation loop using UnusedReservationCapacity. Retain the remaining.Cmp(required) < 0 check and existing missing-resource behavior; only keep an explicit zero guard if negative required quantities are intentionally supported.
🤖 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.
Inline comments:
In `@internal/scheduling/reservations/capacity_accounting.go`:
- Around line 48-57: Update the self-reservation check in the allReservations
loop to skip only when both other.Name and other.Namespace match res.Name and
res.Namespace. Preserve the existing host-targeting logic for reservations with
the same name in different namespaces.
In `@internal/scheduling/reservations/commitments/reservation_controller.go`:
- Around line 594-601: Update the migration handling around migrationTargetHost
so Status.Host is advanced immediately alongside the committed allocation
updates, using Status.CommittedResourceReservation.Allocations as the source.
Re-apply Status.Host after the Spec.TargetHost patch and subsequent re-fetch,
next to re-applying Status.CommittedResourceReservation.Allocations =
newStatusAllocations, so the status change is not discarded.
---
Nitpick comments:
In `@internal/scheduling/reservations/capacity_accounting_test.go`:
- Around line 191-204: Change the local resWithSlot helper to return
*v1alpha1.Reservation, allowing table entries that assign res to call it
directly without inline immediately-invoked closures. Update others entries to
dereference the returned pointer, or add a concise value-returning helper for
those cases while preserving the existing reservation contents.
In `@internal/scheduling/reservations/capacity_accounting.go`:
- Around line 66-78: Remove the redundant zero comparison and the associated
zero quantity in the capacity validation loop using UnusedReservationCapacity.
Retain the remaining.Cmp(required) < 0 check and existing missing-resource
behavior; only keep an explicit zero guard if negative required quantities are
intentionally supported.
In `@internal/scheduling/reservations/commitments/reservation_controller_test.go`:
- Around line 1152-1158: In the test setup around the !addsOldHostHV branch,
remove the wantStatusHost == oldHost condition and oldInstances variable, and
pass an empty instance list directly to newTestHypervisorCRD for oldHost.
- Around line 1066-1074: Extend the “single VM, new host has capacity: follow
the VM” test to invoke reconcileAllocations a second time using the updated
reservation object. Assert on the second pass that Spec.TargetHost, Status.Host,
and Status.Allocations[vmUUID] remain at newHost, while the VM remains in
Spec.Allocations.
In `@internal/scheduling/reservations/commitments/reservation_controller.go`:
- Around line 514-537: Optionally optimize the missing-VM lookup by building a
VM UUID-to-host map once inside ensureHVsAndReservations, after allHVs is
loaded. Populate it by iterating each hypervisor’s Status.Instances, then
replace the nested scan in the confirmed-VM reconciliation path with a
constant-time lookup while preserving the existing expectedHost exclusion and
found-host behavior.
- Around line 450-463: The ensureHVsAndReservations closure should only publish
the cached allHVs and allReservations values after both List calls succeed. Load
the hypervisors and reservations into temporary local lists, return immediately
on either error, then assign both closure variables together; update the cache
guard to require both variables to be initialized.
🪄 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 Plus
Run ID: 41e47cc7-527c-4f22-bdae-1db4fa615526
📒 Files selected for processing (4)
internal/scheduling/reservations/capacity_accounting.gointernal/scheduling/reservations/capacity_accounting_test.gointernal/scheduling/reservations/commitments/reservation_controller.gointernal/scheduling/reservations/commitments/reservation_controller_test.go
Signed-off-by: Julius Clausnitzer <julius.clausnitzer@sap.com> Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scheduling/reservations/commitments/reservation_controller_test.go (1)
1144-1158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid deriving fixture input from the expected test outcome.
The old-host hypervisor's instance list is built from
tt.wantStatusHost == oldHost(line 1154) instead of an explicit per-test-case field. This ties the GIVEN state to the THEN assertion, so a future test case that relies on this branch risks masking a bug: if the controller logic is wrong in a way that happens to still satisfywantStatusHost == oldHost, the derived fixture would silently agree with it.None of the current cases exercise this branch meaningfully (they all resolve to an empty instance list), but the pattern is risky for future additions. Add an explicit field, such as
oldHostInstances []hv1.Instance, so each case states its input independently of its expected result.♻️ Proposed refactor to decouple fixture input from expected output
tests := []struct { name string // reservation to use; nil uses the default single-VM reservation reservation *v1alpha1.Reservation + // oldHostInstances explicitly declares instances present on oldHost; + // nil means the default fixture (empty) is used unless extraObjects supplies one. + oldHostInstances []hv1.Instance // extra objects beyond the base reservation and old host HV extraObjects []client.Object ... }if !addsOldHostHV { - oldInstances := []hv1.Instance{} - if tt.wantStatusHost == oldHost { - oldInstances = []hv1.Instance{{ID: vmUUID, Active: true}} - } + oldInstances := tt.oldHostInstances objects = append(objects, newTestHypervisorCRD(oldHost, oldInstances)) }🤖 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/reservation_controller_test.go` around lines 1144 - 1158, Add an explicit per-test-case old-host fixture field, such as oldHostInstances []hv1.Instance, and use it when constructing the old-host Hypervisor in the test setup. Remove the derivation from tt.wantStatusHost, and initialize the new field explicitly in existing test cases as needed while leaving expected outcome assertions independent.
🤖 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/reservation_controller_test.go`:
- Around line 1144-1158: Add an explicit per-test-case old-host fixture field,
such as oldHostInstances []hv1.Instance, and use it when constructing the
old-host Hypervisor in the test setup. Remove the derivation from
tt.wantStatusHost, and initialize the new field explicitly in existing test
cases as needed while leaving expected outcome assertions independent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d303058-260e-4ee0-a7ba-7a179dc63f65
📒 Files selected for processing (1)
internal/scheduling/reservations/commitments/reservation_controller_test.go
Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
mblos
previously approved these changes
Aug 4, 2026
Contributor
Test Coverage ReportTest Coverage 📊: 70.6% |
mblos
approved these changes
Aug 4, 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
2 participants
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.
Summary
Implements issue #373. When the reservation reconciler detects that a
confirmed VM has disappeared from its expected host, it now searches all
hypervisor CRDs to determine whether the VM live-migrated rather than
treating it as terminated.
Spec.TargetHostis updated to the newhost. The existing TargetHost→Status.Host sync path (Branch B in
Reconcile) advancesStatus.Hoston the next cycle, keepingcapacity accounting consistent during the transition.
TargetHostis left unchanged; theVM's actual location is recorded in
Status.Allocations; the newVMMisplacedcondition is set with reasonMigratedToFullHost.unchanged.
Only the migrating VM's host is considered per-reconcile. Other
allocated VMs in the same reservation are not moved.
The HV list is fetched lazily — only when a confirmed VM is actually
missing from its expected host — so the common path (no migration)
pays no extra API cost.