feat(reservations): steer CR VM live migrations toward reservation slot hosts - #981
Open
juliusclausnitzer wants to merge 9 commits into
Open
feat(reservations): steer CR VM live migrations toward reservation slot hosts#981juliusclausnitzer wants to merge 9 commits into
juliusclausnitzer wants to merge 9 commits into
Conversation
📝 WalkthroughWalkthroughIntroduces ChangesCR migration slot filter
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant FilterCRMigrationSlotStep
participant K8sClient
participant SlotEvaluator
Scheduler->>FilterCRMigrationSlotStep: Run(request)
FilterCRMigrationSlotStep->>K8sClient: list committed-resource Reservations
K8sClient-->>FilterCRMigrationSlotStep: reservation list
FilterCRMigrationSlotStep->>FilterCRMigrationSlotStep: find VM reservation slot and memory
FilterCRMigrationSlotStep->>SlotEvaluator: build evaluator from reservations
FilterCRMigrationSlotStep->>SlotEvaluator: check slot capacity per host
SlotEvaluator-->>FilterCRMigrationSlotStep: capacity result
FilterCRMigrationSlotStep-->>Scheduler: filtered hosts or all candidates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
marked this pull request as ready for review
July 7, 2026 12:14
juliusclausnitzer
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
mblos and
umswmayj
as code owners
July 7, 2026 12:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/scheduling/nova/plugins/filters/filter_cr_migration_slot_test.go (2)
131-294: 📐 Maintainability & Code Quality | 🔵 TrivialConsider a table-driven test consolidation.
These six tests share the same build-request/run/assert shape and mostly differ in fixtures and expected activation sets. As per coding guidelines, "Use struct-based test cases when applicable, but limit to the most relevant cases." A single table with fields like
reservations,hvs,hosts,wantHostswould reduce duplication while keeping this file short.🤖 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/nova/plugins/filters/filter_cr_migration_slot_test.go` around lines 131 - 294, Consolidate the repeated CR migration slot tests into a single table-driven test around TestFilterKVMCRMigrationSlot_* and TestFilterCRMigrationSlot_ZeroSlotMemory, since they all follow the same arrange-run-assert pattern with only fixtures and expected activations changing. Create one struct-based set of cases covering the different reservation/host/request combinations, then loop over them to build the filter, run filter.Run, and assert the expected host set or passthrough behavior. Keep only the meaningful edge cases and remove the duplicate one-off test bodies.Source: Coding guidelines
253-294: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing regression coverage for nil
Spec.CommittedResourceReservationon a confirmed source slot.None of these tests cover a
sourceSlotwhereStatus.CommittedResourceReservationis set (confirmed allocation) butSpec.CommittedResourceReservationis nil — the scenario that triggers the NPE risk flagged infilter_cr_migration_slot.goLine 90. Once that guard is added, a case like this (mirroringTestFilterCRMigrationSlot_ZeroSlotMemory_Passthrough, but withSpec.CommittedResourceReservation: nil) would lock in the fix.Want me to draft this test case?
🤖 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/nova/plugins/filters/filter_cr_migration_slot_test.go` around lines 253 - 294, Add regression coverage in the CR migration slot filter tests for a confirmed source slot whose Status.CommittedResourceReservation exists but Spec.CommittedResourceReservation is nil, since that is the nil dereference path in newCRMigrationSlotFilter.Run. Mirror the existing TestFilterCRMigrationSlot_ZeroSlotMemory_Passthrough setup, but explicitly leave Spec.CommittedResourceReservation unset while keeping the confirmed allocation in Status, then assert the filter runs without error and still passes candidates through. Use the existing helper functions and symbols like newCRMigrationSlotFilter, liveMigrateRequest, and TestFilterCRMigrationSlot_ZeroSlotMemory_Passthrough to keep the new case aligned with the current test structure.
🤖 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/nova/plugins/filters/filter_cr_migration_slot_test.go`:
- Around line 131-294: Consolidate the repeated CR migration slot tests into a
single table-driven test around TestFilterKVMCRMigrationSlot_* and
TestFilterCRMigrationSlot_ZeroSlotMemory, since they all follow the same
arrange-run-assert pattern with only fixtures and expected activations changing.
Create one struct-based set of cases covering the different
reservation/host/request combinations, then loop over them to build the filter,
run filter.Run, and assert the expected host set or passthrough behavior. Keep
only the meaningful edge cases and remove the duplicate one-off test bodies.
- Around line 253-294: Add regression coverage in the CR migration slot filter
tests for a confirmed source slot whose Status.CommittedResourceReservation
exists but Spec.CommittedResourceReservation is nil, since that is the nil
dereference path in newCRMigrationSlotFilter.Run. Mirror the existing
TestFilterCRMigrationSlot_ZeroSlotMemory_Passthrough setup, but explicitly leave
Spec.CommittedResourceReservation unset while keeping the confirmed allocation
in Status, then assert the filter runs without error and still passes candidates
through. Use the existing helper functions and symbols like
newCRMigrationSlotFilter, liveMigrateRequest, and
TestFilterCRMigrationSlot_ZeroSlotMemory_Passthrough to keep the new case
aligned with the current test structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b81f8df4-d57e-4627-8f64-79e053a8782c
📒 Files selected for processing (4)
helm/bundles/cortex-nova/templates/pipelines_kvm.yamlinternal/scheduling/nova/crs/evaluator.gointernal/scheduling/nova/plugins/filters/filter_cr_migration_slot.gointernal/scheduling/nova/plugins/filters/filter_cr_migration_slot_test.go
juliusclausnitzer
changed the title
Signed-off-by: Julius Clausnitzer <julius.clausnitzer@sap.com>
Signed-off-by: Julius Clausnitzer <julius.clausnitzer@sap.com>
Signed-off-by: Julius Clausnitzer <julius.clausnitzer@sap.com>
juliusclausnitzer
force-pushed
the
feat/213-cr-migration-slot-filter
branch
from
July 20, 2026 07:57
7109bc1 to
1a986c0
Compare
juliusclausnitzer
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/scheduling/nova/plugins/filters/filter_cr_migration_slot_test.go (3)
35-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
instanceUUIDas a parameter to avoid brittle hardcoding.The function hardcodes
"vm-migrating", while the test function defines its owninstanceUUIDconstant with the same value. If the constant is ever modified, the test will fail obscurely. Consider passing the UUID as a parameter to keep them explicitly synchronized.(Note: If you apply this change, you will also need to update the callers in the test cases, e.g.,
liveMigrateRequest(instanceUUID, "host-a"))♻️ Proposed fix
-func liveMigrateRequest(hosts ...string) api.ExternalSchedulerRequest { +func liveMigrateRequest(instanceUUID string, hosts ...string) api.ExternalSchedulerRequest { hostList := make([]api.ExternalSchedulerHost, len(hosts)) for i, h := range hosts { hostList[i] = api.ExternalSchedulerHost{ComputeHost: h} } return api.ExternalSchedulerRequest{ Spec: api.NovaObject[api.NovaSpec]{ Data: api.NovaSpec{ - InstanceUUID: "vm-migrating", + InstanceUUID: instanceUUID, ProjectID: "proj-1", SchedulerHints: map[string]any{ "_nova_check_type": "live_migrate", }, }, }, Hosts: hostList, } }🤖 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/nova/plugins/filters/filter_cr_migration_slot_test.go` around lines 35 - 53, Update liveMigrateRequest to accept instanceUUID as a parameter and use it for Spec.Data.InstanceUUID instead of hardcoding "vm-migrating"; update every test caller to pass the existing instanceUUID value before the host arguments.
134-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
sourceSlotForto constructzeroMemorySourceSlot.
zeroMemorySourceSlotis functionally identical tosourceSlotFor(instanceUUID)except that itsResourcesmap is omitted. Reusing the helper eliminates duplicated boilerplate.♻️ Proposed fix
// zeroMemorySourceSlot is a source slot with no memory resource — used to test // the zero-slot-memory guard. - zeroMemorySourceSlot := &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{ - Name: "slot-src", - Labels: map[string]string{ - v1alpha1.LabelReservationType: v1alpha1.ReservationTypeLabelCommittedResource, - }, - }, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeCommittedResource, - TargetHost: "host-src", - CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ - ProjectID: "proj-1", - ResourceGroup: "hana-v2", - }, - }, - Status: v1alpha1.ReservationStatus{ - Host: "host-src", - Conditions: []metav1.Condition{ - {Type: v1alpha1.ReservationConditionReady, Status: metav1.ConditionTrue, Reason: "ReservationActive"}, - }, - CommittedResourceReservation: &v1alpha1.CommittedResourceReservationStatus{ - Allocations: map[string]string{instanceUUID: "host-src"}, - }, - }, - } + zeroMemorySourceSlot := sourceSlotFor(instanceUUID) + zeroMemorySourceSlot.Spec.Resources = 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/nova/plugins/filters/filter_cr_migration_slot_test.go` around lines 134 - 160, Update the test setup to construct zeroMemorySourceSlot by reusing sourceSlotFor(instanceUUID), then omit or clear its Resources map to preserve the zero-memory scenario. Remove the duplicated Reservation metadata, spec, and status construction while keeping the existing slot identity and allocation behavior unchanged.
162-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
wantHostCountfield from the test struct.The
wantHostCountfield is consistently equal tolen(wantHosts)across all test cases. You can simplify the struct and instances by removing this field and directly comparing lengths in the assertion block.♻️ Proposed struct and assertion updates
tests := []struct { name string objects []client.Object request api.ExternalSchedulerRequest wantHosts []string // hosts that must appear in Activations wantFiltered []string // hosts that must NOT appear in Activations - wantHostCount int // total expected Activations size }{Then, update the assertion on line 262 to verify the length directly against
wantHosts:- if len(result.Activations) != tt.wantHostCount { - t.Errorf("expected %d hosts, got %d: %v", tt.wantHostCount, len(result.Activations), result.Activations) + if len(result.Activations) != len(tt.wantHosts) { + t.Errorf("expected %d hosts, got %d: %v", len(tt.wantHosts), len(result.Activations), result.Activations) }🤖 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/nova/plugins/filters/filter_cr_migration_slot_test.go` around lines 162 - 169, Remove the redundant wantHostCount field from the test-case struct and all test instances in the filter migration tests. Update the assertion block to compare the Activations length directly with len(wantHosts), preserving the existing expected-count validation.
🤖 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/nova/plugins/filters/filter_cr_migration_slot_test.go`:
- Around line 35-53: Update liveMigrateRequest to accept instanceUUID as a
parameter and use it for Spec.Data.InstanceUUID instead of hardcoding
"vm-migrating"; update every test caller to pass the existing instanceUUID value
before the host arguments.
- Around line 134-160: Update the test setup to construct zeroMemorySourceSlot
by reusing sourceSlotFor(instanceUUID), then omit or clear its Resources map to
preserve the zero-memory scenario. Remove the duplicated Reservation metadata,
spec, and status construction while keeping the existing slot identity and
allocation behavior unchanged.
- Around line 162-169: Remove the redundant wantHostCount field from the
test-case struct and all test instances in the filter migration tests. Update
the assertion block to compare the Activations length directly with
len(wantHosts), preserving the existing expected-count validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 468416af-6952-4872-b297-3b8dc3b99204
📒 Files selected for processing (4)
helm/bundles/cortex-nova/templates/pipelines_kvm.yamlinternal/scheduling/nova/crs/evaluator.gointernal/scheduling/nova/plugins/filters/filter_cr_migration_slot.gointernal/scheduling/nova/plugins/filters/filter_cr_migration_slot_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- helm/bundles/cortex-nova/templates/pipelines_kvm.yaml
- internal/scheduling/nova/crs/evaluator.go
- internal/scheduling/nova/plugins/filters/filter_cr_migration_slot.go
Signed-off-by: juliusclausnitzer <julius.clausnitzer@sap.com>
juliusclausnitzer
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Nova KVM weigher that steers live migrations toward candidate hosts with sufficient committed-resource (CR) reservation slot capacity, so the reservation can “follow” the migrating VM and reduce double-blocking during migrations.
Changes:
- Introduces
kvm_cr_migration_slotweigher + configurable weights to prefer hosts with compatible CR slot capacity duringlive_migrate. - Extends the CR slot evaluator to support building from a pre-fetched reservation list and adds a strict capacity check (
HasSlotWithCapacity). - Adds Prometheus metrics + manager initialization and documents the new weigher in the KVM pipeline Helm templates; includes unit tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/scheduling/nova/plugins/weighers/kvm_cr_migration_slot.go | Implements the live-migration CR slot capacity weigher and records outcome metrics. |
| internal/scheduling/nova/plugins/weighers/kvm_cr_migration_slot_test.go | Adds unit tests covering intent gating, missing source slot, matching slot, and default weights. |
| internal/scheduling/nova/plugins/weighers/kvm_cr_migration_slot_metrics.go | Adds Prometheus counter for weigher outcomes and singleton wiring point. |
| internal/scheduling/nova/crs/evaluator.go | Adds BuildSlotEvaluatorFromReservations and HasSlotWithCapacity for strict slot-fit checks. |
| helm/bundles/cortex-nova/templates/pipelines_kvm.yaml | Documents the new kvm_cr_migration_slot weigher in KVM pipelines. |
| cmd/manager/main.go | Registers the new CR migration slot metrics collector. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+87
to
+96
| for i := range allReservations.Items { | ||
| res := &allReservations.Items[i] | ||
| if res.Status.CommittedResourceReservation == nil { | ||
| continue | ||
| } | ||
| if _, ok := res.Status.CommittedResourceReservation.Allocations[instanceUUID]; ok { | ||
| sourceSlot = res | ||
| break | ||
| } | ||
| } |
Comment on lines
+106
to
+110
| if slotMemoryBytes.IsZero() { | ||
| traceLog.Info("source CR slot has no memory resource, skipping slot weigher", | ||
| "instanceUUID", instanceUUID, | ||
| "reservation", sourceSlot.Name) | ||
| CRMigrationSlotMetricsSingleton.RecordResult("no_source_slot") |
Contributor
Test Coverage ReportTest Coverage 📊: 73.1% |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
internal/scheduling/nova/plugins/weighers/kvm_cr_migration_slot.go:38
- GetSlotHostWeight() returns 0.1 when SlotHostWeight is unset, but the option comment (and expected behavior) says the default should be 1.0. This flips the intended weighting unless every pipeline explicitly sets params.
This issue also appears on line 40 of the same file.
func (o KVMCRMigrationSlotOpts) GetSlotHostWeight() float64 {
if o.SlotHostWeight == nil {
return 0.1
}
return *o.SlotHostWeight
}
internal/scheduling/nova/plugins/weighers/kvm_cr_migration_slot.go:45
- GetDefaultHostWeight() returns 0.0 when DefaultHostWeight is unset, but the option comment says the default should be 0.1. With the current default, non-slot hosts get no activation even when a source slot exists.
func (o KVMCRMigrationSlotOpts) GetDefaultHostWeight() float64 {
if o.DefaultHostWeight == nil {
return 0.0
}
return *o.DefaultHostWeight
}
internal/scheduling/nova/plugins/weighers/kvm_cr_migration_slot.go:96
- The source-slot scan only checks Status.CommittedResourceReservation, but later code assumes the Reservation is Ready and that Spec.CommittedResourceReservation is non-nil (used at line 114). This can select a non-ready or malformed Reservation and can panic on nil Spec.CommittedResourceReservation.
// Find the source slot that has this VM confirmed.
var sourceSlot *v1alpha1.Reservation
for i := range allReservations.Items {
res := &allReservations.Items[i]
if res.Status.CommittedResourceReservation == nil {
continue
}
if _, ok := res.Status.CommittedResourceReservation.Allocations[instanceUUID]; ok {
sourceSlot = res
break
}
}
helm/bundles/cortex-nova/templates/pipelines_kvm.yaml:319
- This description says the weigher only boosts hosts that already have a ready matching CR reservation slot, but the implementation also boosts hosts that can accommodate the slot based on free hypervisor memory (CanAccommodateSlot). Update the docs to reflect the reconcile/accommodate path.
During live migrations of VMs that occupy a committed-resource reservation
slot, this weigher boosts hosts that have a ready CR reservation with
sufficient remaining capacity for the full slot size. This steers migrations
toward hosts where the reservation can follow the VM, minimising the
double-blocking window. If no candidate has a matching slot, or the VM has
helm/bundles/cortex-nova/templates/pipelines_kvm.yaml:785
- This description says the weigher only boosts hosts that already have a ready matching CR reservation slot, but the implementation also boosts hosts that can accommodate the slot based on free hypervisor memory (CanAccommodateSlot). Update the docs to reflect the reconcile/accommodate path.
During live migrations of VMs that occupy a committed-resource reservation
slot, this weigher boosts hosts that have a ready CR reservation with
sufficient remaining capacity for the full slot size. This steers migrations
toward hosts where the reservation can follow the VM, minimising the
double-blocking window. If no candidate has a matching slot, or the VM has
helm/bundles/cortex-nova/templates/pipelines_kvm.yaml:944
- This description says the weigher only boosts hosts that already have a ready matching CR reservation slot, but the implementation also boosts hosts that can accommodate the slot based on free hypervisor memory (CanAccommodateSlot). Update the docs to reflect the reconcile/accommodate path.
During live migrations of VMs that occupy a committed-resource reservation
slot, this weigher boosts hosts that have a ready CR reservation with
sufficient remaining capacity for the full slot size. This steers migrations
toward hosts where the reservation can follow the VM, minimising the
double-blocking window. If no candidate has a matching slot, or the VM has
Comment on lines
+55
to
+56
| // If the VM has no CR reservation, or no candidate can accommodate the slot, | ||
| // all candidates receive zero weight (no effect on ranking). |
Comment on lines
+221
to
+230
| { | ||
| name: "nil opts use default weights", | ||
| objects: []client.Object{ | ||
| confirmedSourceSlot(instanceUUID, "host-src", "hana-v2", "16Gi"), | ||
| emptyTargetSlot("slot-a", "host-a", "hana-v2", "16Gi"), | ||
| }, | ||
| request: migrationRequest(instanceUUID, projectID, "host-a", "host-b"), | ||
| opts: KVMCRMigrationSlotOpts{}, // nil → defaults: slot=0.1, default=0.0 | ||
| expectedWeights: map[string]float64{"host-a": 0.1, "host-b": 0.0}, | ||
| }, |
| // a migrating VM to this host via the reconciler, even when no existing | ||
| // compatible slot is present. | ||
| // | ||
| // Free memory is computed as: hvFreeMemory - sum(all reservation blocks on host). |
Comment on lines
+156
to
+163
| During live migrations of VMs that occupy a committed-resource reservation | ||
| slot, this weigher boosts hosts that have a ready CR reservation with | ||
| sufficient remaining capacity for the full slot size. This steers migrations | ||
| toward hosts where the reservation can follow the VM, minimising the | ||
| double-blocking window. If no candidate has a matching slot, or the VM has | ||
| no CR reservation, all candidates receive equal weight. | ||
| Only activates for live_migrate requests. All other intents pass through. | ||
| Emits cortex_nova_weigh_cr_migration_slot_requests_total. |
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
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
kvm_cr_migration_slotweigher that nudges live migrations of CR VMs towardhosts that can accommodate the VM's reservation slot — either by already having a
compatible CR slot, or by having enough free capacity for the slot to follow via
the reconciler
cortex_nova_weigh_cr_migration_slot_requests_totalmetric tracking per-migrationoutcome:
slot_found,no_slot,no_source_slotMotivation
When a CR VM migrates, its reservation slot stays behind on the source host until the
reconciler catches up — temporarily double-blocking resources. Steering migrations toward
hosts that can accommodate the slot (either existing slot or free capacity) avoids or
shortens this window.
The reconciler already handles moving reservations after migration. This weigher only
influences where the VM lands.
Behaviour
Only activates for
LiveMigrationIntent. For each candidate host:slotHostWeight(default 0.1)slotHostWeight(default 0.1)defaultHostWeight(default 0.0, neutral)Both weights are configurable via pipeline params. Start small; raise based on the
alert (follow-up) once metric data shows how often a good host is found.
Out of scope