feat: cr migration slot filter#981
Open
juliusclausnitzer wants to merge 10 commits into
Open
Conversation
📝 WalkthroughWalkthroughIntroduces a new Nova scheduler filter, 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->>FilterCRMigrationSlotStep: check LiveMigrationIntent
FilterCRMigrationSlotStep->>K8sClient: list committed-resource Reservations
K8sClient-->>FilterCRMigrationSlotStep: reservation list
FilterCRMigrationSlotStep->>FilterCRMigrationSlotStep: find confirmed slot holding VM instance
FilterCRMigrationSlotStep->>SlotEvaluator: BuildSlotEvaluatorFromReservations(reservations)
FilterCRMigrationSlotStep->>SlotEvaluator: HasSlotWithCapacity(host, project, flavorGroup, slotMemory)
SlotEvaluator-->>FilterCRMigrationSlotStep: true/false per host
FilterCRMigrationSlotStep-->>Scheduler: filtered hosts or fallback to 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
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
mblos and
umswmayj
as code owners
![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
Contributor
Test Coverage ReportTest Coverage 📊: 70.2% |
juliusclausnitzer
changed the title
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.