feat(reservations): cr migration slot filter#981
Open
juliusclausnitzer wants to merge 7 commits into
Open
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
Contributor
Test Coverage ReportTest Coverage 📊: 70.3% |
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
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.