From dedcc396876d7f5ede4652da3dcb06a8d5bccd43 Mon Sep 17 00:00:00 2001 From: Malte Viering Date: Wed, 25 Mar 2026 17:21:12 +0100 Subject: [PATCH] Cleanup requested destination filter --- .../cortex-nova/templates/pipelines_kvm.yaml | 12 +- helm/bundles/cortex-nova/values.yaml | 6 - .../filters/filter_requested_destination.go | 49 +-- .../filter_requested_destination_test.go | 299 ++---------------- 4 files changed, 39 insertions(+), 327 deletions(-) diff --git a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml index 445a7f4c1..8b2519207 100644 --- a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml +++ b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml @@ -97,11 +97,11 @@ spec: the migrating VM, by checking cpu architecture, cpu features, emulated devices, and cpu modes. - name: filter_requested_destination - params: {{ .Values.kvm.filterRequestedDestinationParams | toYaml | nindent 8 }} description: | This step filters hosts based on the `requested_destination` instruction from the nova scheduler request spec. It supports filtering by host and - by aggregates. + by aggregates. Aggregates use AND logic between list elements, with + comma-separated UUIDs within an element using OR logic. weighers: - name: kvm_prefer_smaller_hosts params: @@ -236,11 +236,11 @@ spec: the migrating VM, by checking cpu architecture, cpu features, emulated devices, and cpu modes. - name: filter_requested_destination - params: {{ .Values.kvm.filterRequestedDestinationParams | toYaml | nindent 8 }} description: | This step filters hosts based on the `requested_destination` instruction from the nova scheduler request spec. It supports filtering by host and - by aggregates. + by aggregates. Aggregates use AND logic between list elements, with + comma-separated UUIDs within an element using OR logic. weighers: - name: kvm_prefer_smaller_hosts params: @@ -362,11 +362,11 @@ spec: the migrating VM, by checking cpu architecture, cpu features, emulated devices, and cpu modes. - name: filter_requested_destination - params: {{ .Values.kvm.filterRequestedDestinationParams | toYaml | nindent 8 }} description: | This step filters hosts based on the `requested_destination` instruction from the nova scheduler request spec. It supports filtering by host and - by aggregates. + by aggregates. Aggregates use AND logic between list elements, with + comma-separated UUIDs within an element using OR logic. weighers: - name: kvm_prefer_smaller_hosts params: diff --git a/helm/bundles/cortex-nova/values.yaml b/helm/bundles/cortex-nova/values.yaml index 192d714c2..9779e551c 100644 --- a/helm/bundles/cortex-nova/values.yaml +++ b/helm/bundles/cortex-nova/values.yaml @@ -65,12 +65,6 @@ openstack: kvm: # Use this flag to enable/disable KVM host related features. enabled: false - # Params for the filter_requested_destination filter step. - filterRequestedDestinationParams: [] - # Example: - # filterRequestedDestinationParams: - # - {key: ignoredHostnames, stringListValue: []} - # - {key: ignoredAggregates, stringListValue: []} cortex: &cortex crd: {enable: false} diff --git a/internal/scheduling/nova/plugins/filters/filter_requested_destination.go b/internal/scheduling/nova/plugins/filters/filter_requested_destination.go index dd1804dc8..8922ab8c4 100644 --- a/internal/scheduling/nova/plugins/filters/filter_requested_destination.go +++ b/internal/scheduling/nova/plugins/filters/filter_requested_destination.go @@ -14,28 +14,8 @@ import ( hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" ) -type FilterRequestedDestinationStepOpts struct { - // IgnoredAggregates specifies a list of aggregates to ignore when filtering - // hosts based on the requested destination. This can be used to exclude - // certain aggregates from consideration, for example AZ aggregates - // that are already considered by the availability zone filter. - IgnoredAggregates []string - // IgnoredHostnames specifies a list of hostnames to ignore when filtering - // hosts based on the requested destination. This can be used to exclude - // certain hosts from consideration, for example if they are known to be - // unsuitable for the workload. - IgnoredHostnames []string -} - -// Validate the options to ensure they are correct before running the weigher. -func (o FilterRequestedDestinationStepOpts) Validate() error { - // No specific validation needed for this filter, but we could add checks here - // if we wanted to enforce certain constraints on the options. - return nil -} - type FilterRequestedDestinationStep struct { - lib.BaseFilter[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts] + lib.BaseFilter[api.ExternalSchedulerRequest, lib.EmptyFilterWeigherPipelineStepOpts] } // processRequestedAggregates filters hosts based on the requested aggregates. @@ -43,8 +23,6 @@ type FilterRequestedDestinationStep struct { // ALL elements to pass. Each element can contain comma-separated UUIDs which use // OR logic, meaning the host only needs to match ONE of the UUIDs in that group. // Example: ["agg1", "agg2,agg3"] means host must be in agg1 AND (agg2 OR agg3). -// Respects the IgnoredAggregates option and returns early without filtering -// if all requested aggregates are in the ignored list. func (s *FilterRequestedDestinationStep) processRequestedAggregates( traceLog *slog.Logger, aggregates []string, @@ -55,17 +33,6 @@ func (s *FilterRequestedDestinationStep) processRequestedAggregates( if len(aggregates) == 0 { return } - // 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) - } - } - if len(aggregatesToConsider) == 0 { - traceLog.Info("all aggregates in requested_destination are in the ignored list, skipping aggregate filtering") - return - } for host := range activations { hv, exists := hvsByName[host] if !exists { @@ -80,7 +47,7 @@ func (s *FilterRequestedDestinationStep) processRequestedAggregates( // All outer elements must match (AND logic) // Each element can be comma-separated UUIDs (OR logic within the group) allMatch := true - for _, reqAggGroup := range aggregatesToConsider { + for _, reqAggGroup := range aggregates { reqAggs := strings.Split(reqAggGroup, ",") groupMatch := false for _, reqAgg := range reqAggs { @@ -100,8 +67,6 @@ func (s *FilterRequestedDestinationStep) processRequestedAggregates( "filtered out host not in requested_destination aggregates", "host", host, "hostAggregates", hvAggregateUUIDs, "requestedAggregates", aggregates, - "ignoredAggregates", s.Options.IgnoredAggregates, - "aggregatesConsidered", aggregatesToConsider, ) continue } @@ -110,8 +75,7 @@ func (s *FilterRequestedDestinationStep) processRequestedAggregates( } // processRequestedHost filters hosts based on the requested specific host. -// It removes all hosts except the one matching the requested hostname, -// respecting the IgnoredHostnames option. +// It removes all hosts except the one matching the requested hostname. func (s *FilterRequestedDestinationStep) processRequestedHost( traceLog *slog.Logger, host string, @@ -122,10 +86,6 @@ func (s *FilterRequestedDestinationStep) processRequestedHost( traceLog.Info("no specific host in requested_destination, skipping host filtering") return } - if slices.Contains(s.Options.IgnoredHostnames, host) { - traceLog.Info("requested_destination host is in the ignored hostnames list, skipping host filtering", "host", host) - return - } for h := range activations { if h != host { delete(activations, h) @@ -139,8 +99,7 @@ func (s *FilterRequestedDestinationStep) processRequestedHost( // Run filters hosts based on the requested destination specified in the request. // The requested destination can include a specific host, aggregates, or both. // When both are specified, aggregate filtering is applied first, followed by -// host filtering. This filter respects the IgnoredAggregates and IgnoredHostnames -// options to skip filtering for specific aggregates or hosts. +// host filtering. func (s *FilterRequestedDestinationStep) Run(traceLog *slog.Logger, request api.ExternalSchedulerRequest) (*lib.FilterWeigherPipelineStepResult, error) { result := s.IncludeAllHostsFromRequest(request) rd := request.Spec.Data.RequestedDestination diff --git a/internal/scheduling/nova/plugins/filters/filter_requested_destination_test.go b/internal/scheduling/nova/plugins/filters/filter_requested_destination_test.go index a2b7e1055..5a752160e 100644 --- a/internal/scheduling/nova/plugins/filters/filter_requested_destination_test.go +++ b/internal/scheduling/nova/plugins/filters/filter_requested_destination_test.go @@ -571,8 +571,8 @@ func TestFilterRequestedDestinationStep_Run(t *testing.T) { Build() step := &FilterRequestedDestinationStep{ - BaseFilter: lib.BaseFilter[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - BaseFilterWeigherPipelineStep: lib.BaseFilterWeigherPipelineStep[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ + BaseFilter: lib.BaseFilter[api.ExternalSchedulerRequest, lib.EmptyFilterWeigherPipelineStepOpts]{ + BaseFilterWeigherPipelineStep: lib.BaseFilterWeigherPipelineStep[api.ExternalSchedulerRequest, lib.EmptyFilterWeigherPipelineStepOpts]{ Client: fakeClient, }, }, @@ -625,12 +625,11 @@ func TestFilterRequestedDestinationStep_Run(t *testing.T) { func TestFilterRequestedDestinationStep_processRequestedAggregates(t *testing.T) { tests := []struct { - name string - aggregates []string - ignoredAggregates []string - hypervisors map[string]hv1.Hypervisor - activations map[string]float64 - expectedHosts []string + name string + aggregates []string + hypervisors map[string]hv1.Hypervisor + activations map[string]float64 + expectedHosts []string }{ { name: "empty aggregates - no filtering", @@ -641,17 +640,6 @@ func TestFilterRequestedDestinationStep_processRequestedAggregates(t *testing.T) activations: map[string]float64{"host1": 1.0, "host2": 1.0}, expectedHosts: []string{"host1", "host2"}, }, - { - name: "all aggregates ignored - no filtering", - aggregates: []string{"agg1", "agg2"}, - ignoredAggregates: []string{"agg1", "agg2"}, - hypervisors: map[string]hv1.Hypervisor{ - "host1": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg1"}}}}, - "host2": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg3"}}}}, - }, - activations: map[string]float64{"host1": 1.0, "host2": 1.0}, - expectedHosts: []string{"host1", "host2"}, - }, { name: "filter by aggregate", aggregates: []string{"agg1"}, @@ -672,28 +660,30 @@ func TestFilterRequestedDestinationStep_processRequestedAggregates(t *testing.T) expectedHosts: []string{"host1"}, }, { - name: "partial ignore - some aggregates considered", - aggregates: []string{"agg1", "agg2"}, - ignoredAggregates: []string{"agg1"}, + name: "AND logic - host must be in all aggregates", + aggregates: []string{"agg1", "agg2"}, hypervisors: map[string]hv1.Hypervisor{ "host1": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg1"}}}}, - "host2": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg2"}}}}, + "host2": {Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "agg1"}, {UUID: "agg2"}}}}, }, activations: map[string]float64{"host1": 1.0, "host2": 1.0}, expectedHosts: []string{"host2"}, }, + { + name: "OR logic with comma-separated UUIDs", + aggregates: []string{"agg1,agg2"}, + 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}, + expectedHosts: []string{"host1", "host2"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - step := &FilterRequestedDestinationStep{ - BaseFilter: lib.BaseFilter[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - BaseFilterWeigherPipelineStep: lib.BaseFilterWeigherPipelineStep[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - Options: FilterRequestedDestinationStepOpts{ - IgnoredAggregates: tt.ignoredAggregates, - }, - }, - }, - } + step := &FilterRequestedDestinationStep{} step.processRequestedAggregates(slog.Default(), tt.aggregates, tt.hypervisors, tt.activations) if len(tt.activations) != len(tt.expectedHosts) { t.Errorf("expected %d hosts, got %d", len(tt.expectedHosts), len(tt.activations)) @@ -709,11 +699,10 @@ func TestFilterRequestedDestinationStep_processRequestedAggregates(t *testing.T) func TestFilterRequestedDestinationStep_processRequestedHost(t *testing.T) { tests := []struct { - name string - host string - ignoredHostnames []string - activations map[string]float64 - expectedHosts []string + name string + host string + activations map[string]float64 + expectedHosts []string }{ { name: "empty host - no filtering", @@ -721,13 +710,6 @@ func TestFilterRequestedDestinationStep_processRequestedHost(t *testing.T) { activations: map[string]float64{"host1": 1.0, "host2": 1.0}, expectedHosts: []string{"host1", "host2"}, }, - { - name: "host in ignored list - no filtering", - host: "host1", - ignoredHostnames: []string{"host1"}, - activations: map[string]float64{"host1": 1.0, "host2": 1.0}, - expectedHosts: []string{"host1", "host2"}, - }, { name: "filter to specific host", host: "host2", @@ -743,15 +725,7 @@ func TestFilterRequestedDestinationStep_processRequestedHost(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - step := &FilterRequestedDestinationStep{ - BaseFilter: lib.BaseFilter[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - BaseFilterWeigherPipelineStep: lib.BaseFilterWeigherPipelineStep[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - Options: FilterRequestedDestinationStepOpts{ - IgnoredHostnames: tt.ignoredHostnames, - }, - }, - }, - } + step := &FilterRequestedDestinationStep{} step.processRequestedHost(slog.Default(), tt.host, tt.activations) if len(tt.activations) != len(tt.expectedHosts) { t.Errorf("expected %d hosts, got %d", len(tt.expectedHosts), len(tt.activations)) @@ -765,221 +739,6 @@ func TestFilterRequestedDestinationStep_processRequestedHost(t *testing.T) { } } -func TestFilterRequestedDestinationStepOpts_Validate(t *testing.T) { - tests := []struct { - name string - opts FilterRequestedDestinationStepOpts - wantErr bool - }{ - { - name: "Empty options - valid", - opts: FilterRequestedDestinationStepOpts{}, - wantErr: false, - }, - { - name: "With ignored aggregates - valid", - opts: FilterRequestedDestinationStepOpts{ - IgnoredAggregates: []string{"aggregate1", "aggregate2"}, - }, - wantErr: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.opts.Validate() - if (err != nil) != tt.wantErr { - t.Errorf("Validate() error = %v, wantErr %v", err, tt.wantErr) - } - }) - } -} - -func TestFilterRequestedDestinationStepOpts_Combined(t *testing.T) { - tests := []struct { - name string - request api.ExternalSchedulerRequest - hypervisors []hv1.Hypervisor - ignoredAggregates []string - ignoredHostnames []string - expectedHosts []string - filteredHosts []string - }{ - { - name: "Both aggregate and host ignored - all filtering skipped", - request: api.ExternalSchedulerRequest{ - Spec: api.NovaObject[api.NovaSpec]{ - Data: api.NovaSpec{ - RequestedDestination: &api.NovaObject[api.NovaRequestedDestination]{ - Data: api.NovaRequestedDestination{ - Aggregates: []string{"az-west"}, - Host: "host1", - }, - }, - }, - }, - Hosts: []api.ExternalSchedulerHost{ - {ComputeHost: "host1"}, - {ComputeHost: "host2"}, - {ComputeHost: "host3"}, - }, - }, - hypervisors: []hv1.Hypervisor{ - { - ObjectMeta: metav1.ObjectMeta{Name: "host1"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "az-west"}}}, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "host2"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "az-east"}}}, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "host3"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "az-west"}}}, - }, - }, - ignoredAggregates: []string{"az-west"}, - ignoredHostnames: []string{"host1"}, - expectedHosts: []string{"host1", "host2", "host3"}, - filteredHosts: []string{}, - }, - { - name: "Some aggregates ignored with host not ignored - host filtering still applies after aggregate filtering", - request: api.ExternalSchedulerRequest{ - Spec: api.NovaObject[api.NovaSpec]{ - Data: api.NovaSpec{ - RequestedDestination: &api.NovaObject[api.NovaRequestedDestination]{ - Data: api.NovaRequestedDestination{ - Aggregates: []string{"az-west", "production"}, - Host: "host1", - }, - }, - }, - }, - Hosts: []api.ExternalSchedulerHost{ - {ComputeHost: "host1"}, - {ComputeHost: "host2"}, - {ComputeHost: "host3"}, - }, - }, - hypervisors: []hv1.Hypervisor{ - { - ObjectMeta: metav1.ObjectMeta{Name: "host1"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "production"}}}, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "host2"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "production"}}}, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "host3"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "staging"}}}, - }, - }, - // az-west is ignored, so only production is considered - // host1 and host2 have production, host3 is filtered out - // Then host filtering applies and only host1 remains - ignoredAggregates: []string{"az-west"}, - ignoredHostnames: []string{}, - expectedHosts: []string{"host1"}, - filteredHosts: []string{"host2", "host3"}, - }, - { - // Regression test: when all requested aggregates are ignored, the aggregate - // filtering short-circuits but the explicit host filtering must still apply. - // This ensures the explicit host is kept even when aggregate filtering is skipped. - name: "All aggregates ignored with explicit host - host filtering still applies", - request: api.ExternalSchedulerRequest{ - Spec: api.NovaObject[api.NovaSpec]{ - Data: api.NovaSpec{ - RequestedDestination: &api.NovaObject[api.NovaRequestedDestination]{ - Data: api.NovaRequestedDestination{ - Aggregates: []string{"az-west", "az-east"}, - Host: "host2", - }, - }, - }, - }, - Hosts: []api.ExternalSchedulerHost{ - {ComputeHost: "host1"}, - {ComputeHost: "host2"}, - {ComputeHost: "host3"}, - }, - }, - hypervisors: []hv1.Hypervisor{ - { - ObjectMeta: metav1.ObjectMeta{Name: "host1"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "az-west"}}}, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "host2"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "az-east"}}}, - }, - { - ObjectMeta: metav1.ObjectMeta{Name: "host3"}, - Status: hv1.HypervisorStatus{Aggregates: []hv1.Aggregate{{UUID: "az-west"}}}, - }, - }, - // All aggregates (az-west, az-east) are ignored, so aggregate filtering is skipped - // But host filtering still applies and only host2 should remain - ignoredAggregates: []string{"az-west", "az-east"}, - ignoredHostnames: []string{}, - expectedHosts: []string{"host2"}, - filteredHosts: []string{"host1", "host3"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - scheme := runtime.NewScheme() - if err := hv1.AddToScheme(scheme); err != nil { - t.Fatalf("Failed to add hv1 scheme: %v", err) - } - objs := make([]client.Object, len(tt.hypervisors)) - for i := range tt.hypervisors { - objs[i] = &tt.hypervisors[i] - } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(objs...). - Build() - - step := &FilterRequestedDestinationStep{ - BaseFilter: lib.BaseFilter[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - BaseFilterWeigherPipelineStep: lib.BaseFilterWeigherPipelineStep[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - Client: fakeClient, - Options: FilterRequestedDestinationStepOpts{ - IgnoredAggregates: tt.ignoredAggregates, - IgnoredHostnames: tt.ignoredHostnames, - }, - }, - }, - } - - result, err := step.Run(slog.Default(), tt.request) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - - for _, host := range tt.expectedHosts { - if _, ok := result.Activations[host]; !ok { - t.Errorf("expected host %s to be present in activations", host) - } - } - - for _, host := range tt.filteredHosts { - if _, ok := result.Activations[host]; ok { - t.Errorf("expected host %s to be filtered out", host) - } - } - - if len(result.Activations) != len(tt.expectedHosts) { - t.Errorf("expected %d hosts, got %d", len(tt.expectedHosts), len(result.Activations)) - } - }) - } -} - func TestFilterRequestedDestinationStep_Run_ClientError(t *testing.T) { request := api.ExternalSchedulerRequest{ Spec: api.NovaObject[api.NovaSpec]{ @@ -1011,8 +770,8 @@ func TestFilterRequestedDestinationStep_Run_ClientError(t *testing.T) { Build() step := &FilterRequestedDestinationStep{ - BaseFilter: lib.BaseFilter[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ - BaseFilterWeigherPipelineStep: lib.BaseFilterWeigherPipelineStep[api.ExternalSchedulerRequest, FilterRequestedDestinationStepOpts]{ + BaseFilter: lib.BaseFilter[api.ExternalSchedulerRequest, lib.EmptyFilterWeigherPipelineStepOpts]{ + BaseFilterWeigherPipelineStep: lib.BaseFilterWeigherPipelineStep[api.ExternalSchedulerRequest, lib.EmptyFilterWeigherPipelineStepOpts]{ Client: fakeClient, }, },