Support concatenated aggregates in filter requested destination#637
Merged
SoWieMarkus merged 1 commit intomainfrom Mar 25, 2026
Merged
Support concatenated aggregates in filter requested destination#637SoWieMarkus merged 1 commit intomainfrom
SoWieMarkus merged 1 commit intomainfrom
Conversation
umswmayj
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
mblos
as code owners
📝 WalkthroughWalkthroughUpdated the aggregate matching logic in the requested destination filter from OR semantics (any aggregate matches) to AND-of-OR-groups: comma-separated aggregate UUIDs are interpreted as OR within groups, while multiple groups must all be satisfied. Ignored aggregates are filtered out upfront, and test coverage expanded to verify the new matching semantics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
Comment |
Contributor
Test Coverage ReportTest Coverage 📊: 68.3% |
PhilippMatthes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/scheduling/nova/plugins/filters/filter_requested_destination.go (2)
84-84: Consider trimming whitespace from split aggregate UUIDs.
strings.Splitdoesn't trim whitespace, so input like"agg1, agg2"(with space after comma) would produce[" agg2"]which won't match the hypervisor's aggregate UUID.Suggested fix
- reqAggs := strings.Split(reqAggGroup, ",") + reqAggs := strings.Split(reqAggGroup, ",") groupMatch := false for _, reqAgg := range reqAggs { - if slices.Contains(hvAggregateUUIDs, reqAgg) { + if slices.Contains(hvAggregateUUIDs, strings.TrimSpace(reqAgg)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_requested_destination.go` at line 84, The split on reqAggGroup uses strings.Split and can leave surrounding whitespace on entries (reqAggs), so update the parsing of reqAggGroup (where reqAggs is created) to trim whitespace from each split element (e.g., map or loop calling strings.TrimSpace on every entry and ignore empty strings) before using reqAggs in the matching logic in filter_requested_destination.go; ensure the rest of the code that compares aggregate UUIDs uses the cleaned values.
58-64: Ignored aggregate check applies to raw comma-separated groups, not individual UUIDs.The ignored aggregate filtering checks the entire comma-separated string (e.g.,
"agg1,agg2") againstIgnoredAggregates. If a user wants to ignoreagg1, but the request contains"agg1,agg2", this group won't be filtered out—hosts inagg1could still pass via the OR logic.This may be intentional given the documented use case (AZ-level filtering), but consider:
- Documenting this behavior explicitly in the struct comment
- Or applying the ignore check after splitting by comma for finer granularity
Option: Apply ignore check to individual UUIDs within groups
- // Filter out ignored aggregates - aggregatesToConsider := make([]string, 0, len(aggregates)) - for _, agg := range aggregates { - if !slices.Contains(s.Options.IgnoredAggregates, agg) { - aggregatesToConsider = append(aggregatesToConsider, agg) - } - } + // Filter out ignored aggregates from comma-separated groups + aggregatesToConsider := make([]string, 0, len(aggregates)) + for _, aggGroup := range aggregates { + filteredUUIDs := make([]string, 0) + for _, uuid := range strings.Split(aggGroup, ",") { + if !slices.Contains(s.Options.IgnoredAggregates, uuid) { + filteredUUIDs = append(filteredUUIDs, uuid) + } + } + if len(filteredUUIDs) > 0 { + aggregatesToConsider = append(aggregatesToConsider, strings.Join(filteredUUIDs, ",")) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_requested_destination.go` around lines 58 - 64, The current loop that builds aggregatesToConsider iterates over raw comma-separated group strings and checks each full group string against s.Options.IgnoredAggregates, so a group like "agg1,agg2" won't be filtered if only "agg1" is listed; update the logic in the block that constructs aggregatesToConsider to split each agg on commas (e.g., strings.Split(agg, ",")), check each resulting UUID against s.Options.IgnoredAggregates, and only include the original group if none of its constituent UUIDs are ignored (or alternatively document this behavior in the struct comment for s.Options.IgnoredAggregates if you intend group-level matching). Ensure you reference aggregatesToConsider, the loop over aggregates, and s.Options.IgnoredAggregates when making the change.internal/scheduling/nova/plugins/filters/filter_requested_destination_test.go (1)
634-684: Consider adding a test for ignored aggregates within comma-separated groups.The
processRequestedAggregatestests don't cover the edge case where an ignored aggregate appears within a comma-separated group. If the current behavior (ignoring at the group level, not individual UUID level) is intentional, a test documenting this would be valuable.Suggested additional test case
{ name: "ignored aggregate within comma-separated group - group not filtered", aggregates: []string{"agg1,agg2"}, ignoredAggregates: []string{"agg1"}, hypervisors: map[string]hv1.Hypervisor{ "host1": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg1"}}}}, "host2": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg2"}}}}, "host3": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg3"}}}}, }, activations: map[string]float64{"host1": 1.0, "host2": 1.0, "host3": 1.0}, // Current behavior: "agg1,agg2" as a whole is not in ignoredAggregates, // so both agg1 and agg2 are considered (OR). host1 and host2 pass. expectedHosts: []string{"host1", "host2"}, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/nova/plugins/filters/filter_requested_destination_test.go` around lines 634 - 684, Add a table-driven test case to the existing tests in filter_requested_destination_test.go for processRequestedAggregates that covers an ignored aggregate appearing inside a comma-separated group: create a case named like "ignored aggregate within comma-separated group - group not filtered" with aggregates set to []string{"agg1,agg2"}, ignoredAggregates []string{"agg1"}, three hypervisors ("host1" with agg1, "host2" with agg2, "host3" with agg3), activations for each host, and expectedHosts []string{"host1","host2"} to document the current behavior where ignore matching is applied at the whole-group level rather than per-UUID. Ensure the new case follows the same test struct fields (aggregates, ignoredAggregates, hypervisors, activations, expectedHosts) used by the existing table.
🤖 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_requested_destination_test.go`:
- Around line 634-684: Add a table-driven test case to the existing tests in
filter_requested_destination_test.go for processRequestedAggregates that covers
an ignored aggregate appearing inside a comma-separated group: create a case
named like "ignored aggregate within comma-separated group - group not filtered"
with aggregates set to []string{"agg1,agg2"}, ignoredAggregates
[]string{"agg1"}, three hypervisors ("host1" with agg1, "host2" with agg2,
"host3" with agg3), activations for each host, and expectedHosts
[]string{"host1","host2"} to document the current behavior where ignore matching
is applied at the whole-group level rather than per-UUID. Ensure the new case
follows the same test struct fields (aggregates, ignoredAggregates, hypervisors,
activations, expectedHosts) used by the existing table.
In `@internal/scheduling/nova/plugins/filters/filter_requested_destination.go`:
- Line 84: The split on reqAggGroup uses strings.Split and can leave surrounding
whitespace on entries (reqAggs), so update the parsing of reqAggGroup (where
reqAggs is created) to trim whitespace from each split element (e.g., map or
loop calling strings.TrimSpace on every entry and ignore empty strings) before
using reqAggs in the matching logic in filter_requested_destination.go; ensure
the rest of the code that compares aggregate UUIDs uses the cleaned values.
- Around line 58-64: The current loop that builds aggregatesToConsider iterates
over raw comma-separated group strings and checks each full group string against
s.Options.IgnoredAggregates, so a group like "agg1,agg2" won't be filtered if
only "agg1" is listed; update the logic in the block that constructs
aggregatesToConsider to split each agg on commas (e.g., strings.Split(agg,
",")), check each resulting UUID against s.Options.IgnoredAggregates, and only
include the original group if none of its constituent UUIDs are ignored (or
alternatively document this behavior in the struct comment for
s.Options.IgnoredAggregates if you intend group-level matching). Ensure you
reference aggregatesToConsider, the loop over aggregates, and
s.Options.IgnoredAggregates when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3753ecc7-4b85-4091-a225-2edb9915ef14
📒 Files selected for processing (2)
internal/scheduling/nova/plugins/filters/filter_requested_destination.gointernal/scheduling/nova/plugins/filters/filter_requested_destination_test.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
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.