feat: Add fallback to capacity filter and external customer filter#593
Merged
feat: Add fallback to capacity filter and external customer filter#593
Conversation
umswmayj
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
mblos
as code owners
📝 WalkthroughWalkthroughThis PR modifies Nova scheduling filter error handling and capacity fallback logic. When domain_name scheduler hints are unavailable, the external customer filter now logs and gracefully skips instead of returning an error. Additionally, when a hypervisor's EffectiveCapacity is nil, capacity filtering now falls back to using Capacity for calculations instead of skipping the host. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
Contributor
Test Coverage ReportTest Coverage 📊: 69.6% |
PhilippMatthes
approved these changes
Mar 18, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go (1)
59-65: Inconsistent resource map handling—consider deep copying for consistency with reservation pattern.The current code safely handles resource quantity mutations through a retrieve-modify-reassign pattern (lines 69–78), so the original Capacity and EffectiveCapacity maps are not mutated. However, the hypervisor resource initialization (lines 61, 64) assigns map descriptors directly, which is inconsistent with the reservation handling pattern elsewhere in the file (lines 162–165), where a new map is created and values are explicitly deep copied.
For consistency and defensive programming, consider adopting the same deep copy pattern used for reservations:
Proposed change for consistency
if hv.Status.EffectiveCapacity == nil { traceLog.Warn("hypervisor with nil effective capacity, use capacity instead (overprovisioning not considered)", "host", hv.Name) - freeResourcesByHost[hv.Name] = hv.Status.Capacity + freeResourcesByHost[hv.Name] = make(map[hv1.ResourceName]resource.Quantity) + for k, v := range hv.Status.Capacity { + freeResourcesByHost[hv.Name][k] = v.DeepCopy() + } } else { // Start with the total effective capacity which is capacity * overcommit ratio. - freeResourcesByHost[hv.Name] = hv.Status.EffectiveCapacity + freeResourcesByHost[hv.Name] = make(map[hv1.ResourceName]resource.Quantity) + for k, v := range hv.Status.EffectiveCapacity { + freeResourcesByHost[hv.Name][k] = v.DeepCopy() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go` around lines 59 - 65, The initialization assigns hv.Status.Capacity or hv.Status.EffectiveCapacity map descriptors directly into freeResourcesByHost[hv.Name], risking shared-mutation; change both branches to allocate a new map and deep-copy each resource key/value into it (same pattern used for reservation handling where a fresh map is created and values copied), i.e., when hv.Status.EffectiveCapacity == nil copy from hv.Status.Capacity into a new map, otherwise copy from hv.Status.EffectiveCapacity into a new map before assigning to freeResourcesByHost[hv.Name]; keep traceLog.Warn as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go`:
- Around line 59-65: The initialization assigns hv.Status.Capacity or
hv.Status.EffectiveCapacity map descriptors directly into
freeResourcesByHost[hv.Name], risking shared-mutation; change both branches to
allocate a new map and deep-copy each resource key/value into it (same pattern
used for reservation handling where a fresh map is created and values copied),
i.e., when hv.Status.EffectiveCapacity == nil copy from hv.Status.Capacity into
a new map, otherwise copy from hv.Status.EffectiveCapacity into a new map before
assigning to freeResourcesByHost[hv.Name]; keep traceLog.Warn as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2418bec-03f7-4d18-8cfe-e65271bb189a
📒 Files selected for processing (4)
internal/scheduling/nova/plugins/filters/filter_external_customer.gointernal/scheduling/nova/plugins/filters/filter_external_customer_test.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity.gointernal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go
umswmayj
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
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.
No description provided.