From cec9f2fb53a335574245f76e08b5596ef502aaec Mon Sep 17 00:00:00 2001 From: mblos Date: Thu, 26 Mar 2026 08:40:54 +0100 Subject: [PATCH 1/4] Committed resources reservations don't unlock resources of other reservations --- api/external/nova/messages.go | 5 + .../filters/filter_has_enough_capacity.go | 38 ++-- .../filter_has_enough_capacity_test.go | 184 ++++++++++++++++++ .../reservations/commitments/controller.go | 5 + 4 files changed, 221 insertions(+), 11 deletions(-) diff --git a/api/external/nova/messages.go b/api/external/nova/messages.go index a0a5a8616..c7b52451e 100644 --- a/api/external/nova/messages.go +++ b/api/external/nova/messages.go @@ -140,6 +140,8 @@ const ( CreateIntent v1alpha1.SchedulingIntent = "create" // ReserveForFailoverIntent indicates that the request is for failover reservation scheduling. ReserveForFailoverIntent v1alpha1.SchedulingIntent = "reserve_for_failover" + // ReserveForCommittedResourceIntent indicates that the request is for CR reservation scheduling. + ReserveForCommittedResourceIntent v1alpha1.SchedulingIntent = "reserve_for_committed_resource" ) // GetIntent analyzes the request spec and determines the intent of the scheduling request. @@ -165,6 +167,9 @@ func (req ExternalSchedulerRequest) GetIntent() (v1alpha1.SchedulingIntent, erro // Used by cortex failover reservation controller case "reserve_for_failover": return ReserveForFailoverIntent, nil + // Used by cortex committed resource reservation controller + case "reserve_for_committed_resource": + return ReserveForCommittedResourceIntent, nil default: return CreateIntent, nil } diff --git a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go index ea37e8c11..0a347a53a 100644 --- a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go +++ b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go @@ -96,12 +96,6 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa continue // Only consider active reservations (Ready=True). } - // Check if this reservation type should be ignored - if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { - traceLog.Debug("ignoring reservation type", "type", reservation.Spec.Type, "reservation", reservation.Name) - continue - } - // Handle reservation based on its type. switch reservation.Spec.Type { case v1alpha1.ReservationTypeCommittedResource, "": // Empty string for backward compatibility @@ -110,12 +104,29 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa continue // Not handled by us (no resource group set). } - // For committed resource reservations: unlock resources only if: - // 1. Project ID matches - // 2. ResourceGroup matches the flavor's hw_version - if !s.Options.LockReserved && + // Check if this is a CR reservation scheduling request. + // If so, we should NOT unlock any CR reservations to prevent overbooking. + // CR capacity should only be unlocked for actual VM scheduling. + // IMPORTANT: This check MUST happen before IgnoredReservationTypes check + // to ensure CR reservation scheduling cannot bypass reservation locking. + intent, err := request.GetIntent() + switch { + case err == nil && intent == api.ReserveForCommittedResourceIntent: + traceLog.Debug("keeping CR reservation locked for CR reservation scheduling", + "reservation", reservation.Name, + "intent", intent) + // Don't continue - fall through to block the resources + case slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type): + // Check IgnoredReservationTypes AFTER intent check for CR reservations. + // This ensures CR reservation scheduling always respects existing CR reservations. + traceLog.Debug("ignoring CR reservation type per config", "reservation", reservation.Name) + continue + case !s.Options.LockReserved && + // For committed resource reservations: unlock resources only if: + // 1. Project ID matches + // 2. ResourceGroup matches the flavor's hw_version reservation.Spec.CommittedResourceReservation.ProjectID == request.Spec.Data.ProjectID && - reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"] { + reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"]: traceLog.Info("unlocking resources reserved by matching committed resource reservation with allocation", "reservation", reservation.Name, "instanceUUID", request.Spec.Data.InstanceUUID, @@ -125,6 +136,11 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa } case v1alpha1.ReservationTypeFailover: + // Check if failover reservations should be ignored + if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { + traceLog.Debug("ignoring failover reservation type per config", "reservation", reservation.Name) + continue + } // For failover reservations: if the requested VM is contained in the allocations map // AND this is an evacuation request, unlock the resources. // We only unlock during evacuations because: diff --git a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go index 002a92dc8..3186f7e99 100644 --- a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go +++ b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go @@ -210,6 +210,12 @@ func parseMemoryToMB(memory string) uint64 { } func newNovaRequest(instanceUUID, projectID, flavorName, flavorGroup string, vcpus int, memory string, evacuation bool, hosts []string) api.ExternalSchedulerRequest { //nolint:unparam // vcpus varies in real usage + return newNovaRequestWithIntent(instanceUUID, projectID, flavorName, flavorGroup, vcpus, memory, "", evacuation, hosts) +} + +// newNovaRequestWithIntent creates a nova request with a specific intent. +// intentHint can be: "evacuate", "reserve_for_committed_resource", "reserve_for_failover", or "" for create. +func newNovaRequestWithIntent(instanceUUID, projectID, flavorName, flavorGroup string, vcpus int, memory, intentHint string, evacuation bool, hosts []string) api.ExternalSchedulerRequest { hostList := make([]api.ExternalSchedulerHost, len(hosts)) for i, h := range hosts { hostList[i] = api.ExternalSchedulerHost{ComputeHost: h} @@ -225,6 +231,10 @@ func newNovaRequest(instanceUUID, projectID, flavorName, flavorGroup string, vcp schedulerHints = map[string]any{ "_nova_check_type": []any{"evacuate"}, } + } else if intentHint != "" { + schedulerHints = map[string]any{ + "_nova_check_type": intentHint, + } } memoryMB := parseMemoryToMB(memory) @@ -802,6 +812,180 @@ func TestFilterHasEnoughCapacity_IgnoredReservationTypes(t *testing.T) { } } +func TestFilterHasEnoughCapacity_ReserveForCommittedResourceIntent(t *testing.T) { + scheme := buildTestScheme(t) + + // Test that when scheduling a CR reservation (with reserve_for_committed_resource intent), + // other CR reservations from the same project+flavor group are NOT unlocked. + // This prevents overbooking when scheduling multiple CR reservations. + tests := []struct { + name string + hypervisors []*hv1.Hypervisor + reservations []*v1alpha1.Reservation + request api.ExternalSchedulerRequest + opts FilterHasEnoughCapacityOpts + expectedHosts []string + filteredHosts []string + }{ + { + name: "CR reservation scheduling: same project+flavor reservations stay locked (prevents overbooking)", + hypervisors: []*hv1.Hypervisor{ + newHypervisor("host1", "16", "8", "32Gi", "16Gi"), // 8 CPU free + newHypervisor("host2", "16", "8", "32Gi", "16Gi"), // 8 CPU free + }, + reservations: []*v1alpha1.Reservation{ + // Existing CR reservation on host1 for same project+flavor group + newCommittedReservation("existing-cr", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + }, + // Request with reserve_for_committed_resource intent (scheduling a new CR reservation) + request: newNovaRequestWithIntent("new-reservation-uuid", "project-A", "m1.large", "gp-1", 4, "8Gi", "reserve_for_committed_resource", false, []string{"host1", "host2"}), + opts: FilterHasEnoughCapacityOpts{LockReserved: false}, // Note: LockReserved is false, but intent overrides + expectedHosts: []string{"host2"}, // host1 blocked because existing-cr stays locked + filteredHosts: []string{"host1"}, + }, + { + name: "Normal VM scheduling: same project+flavor reservations ARE unlocked", + hypervisors: []*hv1.Hypervisor{ + newHypervisor("host1", "16", "8", "32Gi", "16Gi"), // 8 CPU free + newHypervisor("host2", "16", "8", "32Gi", "16Gi"), // 8 CPU free + }, + reservations: []*v1alpha1.Reservation{ + // Existing CR reservation on host1 for same project+flavor group + newCommittedReservation("existing-cr", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + }, + // Normal VM create request (no special intent) - CR reservation should be unlocked + request: newNovaRequest("vm-instance-123", "project-A", "m1.large", "gp-1", 4, "8Gi", false, []string{"host1", "host2"}), + opts: FilterHasEnoughCapacityOpts{LockReserved: false}, + expectedHosts: []string{"host1", "host2"}, // host1 passes because existing-cr is unlocked for matching project+flavor + filteredHosts: []string{}, + }, + { + name: "CR reservation scheduling: different project reservations stay locked (as expected)", + hypervisors: []*hv1.Hypervisor{ + newHypervisor("host1", "16", "8", "32Gi", "16Gi"), // 8 CPU free + newHypervisor("host2", "16", "8", "32Gi", "16Gi"), // 8 CPU free + }, + reservations: []*v1alpha1.Reservation{ + // Existing CR reservation on host1 for different project + newCommittedReservation("other-project-cr", "host1", "host1", "project-B", "m1.large", "gp-1", "8", "16Gi", nil, nil), + }, + // Request with reserve_for_committed_resource intent + request: newNovaRequestWithIntent("new-reservation-uuid", "project-A", "m1.large", "gp-1", 4, "8Gi", "reserve_for_committed_resource", false, []string{"host1", "host2"}), + opts: FilterHasEnoughCapacityOpts{LockReserved: false}, + expectedHosts: []string{"host2"}, + filteredHosts: []string{"host1"}, // host1 blocked by other project's reservation (would be blocked anyway) + }, + { + name: "CR reservation scheduling: multiple reservations - none unlocked", + hypervisors: []*hv1.Hypervisor{ + newHypervisor("host1", "32", "0", "64Gi", "0"), // 32 CPU free + }, + reservations: []*v1alpha1.Reservation{ + // Three existing CR reservations on host1 for same project+flavor group + newCommittedReservation("cr-1", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + newCommittedReservation("cr-2", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + newCommittedReservation("cr-3", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + }, + // Request with reserve_for_committed_resource intent, needs 10 CPU + // After blocking all 3 reservations (24 CPU), only 8 CPU free -> should fail + request: newNovaRequestWithIntent("new-reservation-uuid", "project-A", "m1.large", "gp-1", 10, "20Gi", "reserve_for_committed_resource", false, []string{"host1"}), + opts: FilterHasEnoughCapacityOpts{LockReserved: false}, + expectedHosts: []string{}, + filteredHosts: []string{"host1"}, // All reservations stay locked, not enough capacity + }, + { + name: "Normal VM scheduling: multiple reservations - all unlocked for same project+flavor", + hypervisors: []*hv1.Hypervisor{ + newHypervisor("host1", "32", "0", "64Gi", "0"), // 32 CPU free + }, + reservations: []*v1alpha1.Reservation{ + // Three existing CR reservations on host1 for same project+flavor group + newCommittedReservation("cr-1", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + newCommittedReservation("cr-2", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + newCommittedReservation("cr-3", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + }, + // Normal VM create request, needs 10 CPU + // All 3 reservations unlocked for matching project+flavor -> 32 CPU free -> should pass + request: newNovaRequest("vm-instance-123", "project-A", "m1.large", "gp-1", 10, "20Gi", false, []string{"host1"}), + opts: FilterHasEnoughCapacityOpts{LockReserved: false}, + expectedHosts: []string{"host1"}, // All reservations unlocked, enough capacity + filteredHosts: []string{}, + }, + { + name: "CR reservation scheduling: IgnoredReservationTypes config CANNOT bypass intent protection", + hypervisors: []*hv1.Hypervisor{ + newHypervisor("host1", "16", "8", "32Gi", "16Gi"), // 8 CPU free + }, + reservations: []*v1alpha1.Reservation{ + // Existing CR reservation on host1 blocks all 8 free CPU + newCommittedReservation("existing-cr", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), + }, + // Request with reserve_for_committed_resource intent + // Even with IgnoredReservationTypes set to ignore CR, the intent should still block + request: newNovaRequestWithIntent("new-reservation-uuid", "project-A", "m1.large", "gp-1", 4, "8Gi", "reserve_for_committed_resource", false, []string{"host1"}), + opts: FilterHasEnoughCapacityOpts{ + LockReserved: false, + // Normally this would ignore CR reservations, but intent should override this + IgnoredReservationTypes: []v1alpha1.ReservationType{v1alpha1.ReservationTypeCommittedResource}, + }, + expectedHosts: []string{}, // host1 blocked because intent overrides IgnoredReservationTypes + filteredHosts: []string{"host1"}, // The CR reservation stays locked despite IgnoredReservationTypes + }, + { + name: "Normal VM scheduling: IgnoredReservationTypes config DOES work for normal VMs", + hypervisors: []*hv1.Hypervisor{ + newHypervisor("host1", "16", "8", "32Gi", "16Gi"), // 8 CPU free + }, + reservations: []*v1alpha1.Reservation{ + // Existing CR reservation on host1 blocks all 8 free CPU + newCommittedReservation("existing-cr", "host1", "host1", "project-B", "m1.large", "gp-1", "8", "16Gi", nil, nil), + }, + // Normal VM create request (different project, so unlocking via project match won't work) + // But IgnoredReservationTypes should make it work + request: newNovaRequest("vm-instance-123", "project-A", "m1.large", "gp-1", 4, "8Gi", false, []string{"host1"}), + opts: FilterHasEnoughCapacityOpts{ + LockReserved: false, + IgnoredReservationTypes: []v1alpha1.ReservationType{v1alpha1.ReservationTypeCommittedResource}, + }, + expectedHosts: []string{"host1"}, // CR reservation ignored via IgnoredReservationTypes + filteredHosts: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objects := make([]client.Object, 0, len(tt.hypervisors)+len(tt.reservations)) + for _, h := range tt.hypervisors { + objects = append(objects, h.DeepCopy()) + } + for _, r := range tt.reservations { + objects = append(objects, r.DeepCopy()) + } + + step := &FilterHasEnoughCapacity{} + step.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + step.Options = tt.opts + + 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, but got %+v", host, result.Activations) + } + } + + for _, host := range tt.filteredHosts { + if _, ok := result.Activations[host]; ok { + t.Errorf("expected host %s to be filtered out, but it was present", host) + } + } + }) + } +} + func TestFilterHasEnoughCapacity_NilEffectiveCapacityFallback(t *testing.T) { scheme := buildTestScheme(t) diff --git a/internal/scheduling/reservations/commitments/controller.go b/internal/scheduling/reservations/commitments/controller.go index 4a318d882..9c238aeee 100644 --- a/internal/scheduling/reservations/commitments/controller.go +++ b/internal/scheduling/reservations/commitments/controller.go @@ -264,6 +264,11 @@ func (r *CommitmentReservationController) Reconcile(ctx context.Context, req ctr EligibleHosts: eligibleHosts, Pipeline: pipelineName, AvailabilityZone: availabilityZone, + // Set hint to indicate this is a CR reservation scheduling request. + // This prevents other CR reservations from being unlocked during capacity filtering. + SchedulerHints: map[string]any{ + "_nova_check_type": string(schedulerdelegationapi.ReserveForCommittedResourceIntent), + }, } scheduleResp, err := r.SchedulerClient.ScheduleReservation(ctx, scheduleReq) From e118cf9f2b5af7528874de09b55f6926b7443d54 Mon Sep 17 00:00:00 2001 From: mblos Date: Thu, 26 Mar 2026 09:40:49 +0100 Subject: [PATCH 2/4] refactoring --- .../filters/filter_has_enough_capacity.go | 559 +++++++++++------- 1 file changed, 347 insertions(+), 212 deletions(-) diff --git a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go index 0a347a53a..af6089ad1 100644 --- a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go +++ b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go @@ -5,7 +5,6 @@ package filters import ( "context" - "errors" "log/slog" "slices" @@ -51,17 +50,39 @@ type FilterHasEnoughCapacity struct { func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.ExternalSchedulerRequest) (*lib.FilterWeigherPipelineStepResult, error) { result := s.IncludeAllHostsFromRequest(request) - // This map holds the free resources per host. + // Step 1: Build free resources map from hypervisors + freeResourcesByHost, err := s.buildFreeResourcesFromHypervisors(traceLog) + if err != nil { + return nil, err + } + + // Step 2: Apply reservation blocking + if err := s.applyReservationBlocking(traceLog, request, freeResourcesByHost); err != nil { + return nil, err + } + + // Step 3: Filter hosts by capacity + s.filterHostsByCapacity(traceLog, request, freeResourcesByHost, result) + + return result, nil +} + +// buildFreeResourcesFromHypervisors creates a map of free resources per host. +// The hypervisor resource auto-discovers its current utilization. +// We can use the hypervisor status to calculate the total capacity +// and then subtract the actual resource allocation from virtual machines. +func (s *FilterHasEnoughCapacity) buildFreeResourcesFromHypervisors( + traceLog *slog.Logger, +) (map[string]map[hv1.ResourceName]resource.Quantity, error) { + freeResourcesByHost := make(map[string]map[hv1.ResourceName]resource.Quantity) - // The hypervisor resource auto-discovers its current utilization. - // We can use the hypervisor status to calculate the total capacity - // and then subtract the actual resource allocation from virtual machines. hvs := &hv1.HypervisorList{} if err := s.Client.List(context.Background(), hvs); err != nil { traceLog.Error("failed to list hypervisors", "error", err) return nil, err } + for _, hv := range hvs.Items { if hv.Status.EffectiveCapacity == nil { traceLog.Warn("hypervisor with nil effective capacity, use capacity instead (overprovisioning not considered)", "host", hv.Name) @@ -75,10 +96,8 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa for resourceName, allocated := range hv.Status.Allocation { free, ok := freeResourcesByHost[hv.Name][resourceName] if !ok { - traceLog.Error( - "hypervisor with allocation for unknown resource", - "host", hv.Name, "resource", resourceName, - ) + traceLog.Error("hypervisor with allocation for unknown resource", + "host", hv.Name, "resource", resourceName) continue } free.Sub(allocated) @@ -86,256 +105,372 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa } } - // Subtract reserved resources by Reservations. + return freeResourcesByHost, nil +} + +// applyReservationBlocking subtracts reserved resources from the free resources map. +// It handles unlocking logic for matching CR and failover reservations. +// Only active reservations (Ready=True) are considered. +func (s *FilterHasEnoughCapacity) applyReservationBlocking( + traceLog *slog.Logger, + request api.ExternalSchedulerRequest, + freeResourcesByHost map[string]map[hv1.ResourceName]resource.Quantity, +) error { + var reservations v1alpha1.ReservationList if err := s.Client.List(context.Background(), &reservations); err != nil { - return nil, err + return err } + for _, reservation := range reservations.Items { if !meta.IsStatusConditionTrue(reservation.Status.Conditions, v1alpha1.ReservationConditionReady) { - continue // Only consider active reservations (Ready=True). + continue } - // Handle reservation based on its type. - switch reservation.Spec.Type { - case v1alpha1.ReservationTypeCommittedResource, "": // Empty string for backward compatibility - // Skip if no CommittedResourceReservation spec or no resource group set. - if reservation.Spec.CommittedResourceReservation == nil || reservation.Spec.CommittedResourceReservation.ResourceGroup == "" { - continue // Not handled by us (no resource group set). - } + // Process reservation and get resources/hosts to block (or nil if unlocked) + resourcesToBlock, hostsToBlock := s.processReservationForCapacity(traceLog, request, &reservation) + if resourcesToBlock == nil || len(hostsToBlock) == 0 { + continue + } - // Check if this is a CR reservation scheduling request. - // If so, we should NOT unlock any CR reservations to prevent overbooking. - // CR capacity should only be unlocked for actual VM scheduling. - // IMPORTANT: This check MUST happen before IgnoredReservationTypes check - // to ensure CR reservation scheduling cannot bypass reservation locking. - intent, err := request.GetIntent() - switch { - case err == nil && intent == api.ReserveForCommittedResourceIntent: - traceLog.Debug("keeping CR reservation locked for CR reservation scheduling", - "reservation", reservation.Name, - "intent", intent) - // Don't continue - fall through to block the resources - case slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type): - // Check IgnoredReservationTypes AFTER intent check for CR reservations. - // This ensures CR reservation scheduling always respects existing CR reservations. - traceLog.Debug("ignoring CR reservation type per config", "reservation", reservation.Name) - continue - case !s.Options.LockReserved && - // For committed resource reservations: unlock resources only if: - // 1. Project ID matches - // 2. ResourceGroup matches the flavor's hw_version - reservation.Spec.CommittedResourceReservation.ProjectID == request.Spec.Data.ProjectID && - reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"]: - traceLog.Info("unlocking resources reserved by matching committed resource reservation with allocation", + // Block the calculated resources on each host + s.blockResourcesOnHosts(traceLog, resourcesToBlock, hostsToBlock, freeResourcesByHost, &reservation) + } + + return nil +} + +// processReservationForCapacity determines if a reservation should block capacity and how much. +// Returns nil, nil if the reservation should be unlocked (not block any resources). +func (s *FilterHasEnoughCapacity) processReservationForCapacity( + traceLog *slog.Logger, + request api.ExternalSchedulerRequest, + reservation *v1alpha1.Reservation, +) (resourcesToBlock map[hv1.ResourceName]resource.Quantity, hostsToBlock map[string]struct{}) { + // Handle reservation based on its type + switch reservation.Spec.Type { + case v1alpha1.ReservationTypeCommittedResource, "": + if !s.shouldBlockCRReservation(traceLog, request, reservation) { + return nil, nil + } + case v1alpha1.ReservationTypeFailover: + if !s.shouldBlockFailoverReservation(traceLog, request, reservation) { + return nil, nil + } + } + + // Block resources on BOTH Spec.TargetHost (desired) AND Status.Host (actual). + // This ensures capacity is blocked during the transition period when a reservation + // is being placed (TargetHost set) and after it's placed (Host set). + // If both are the same, we only subtract once (using a map). + hostsToBlock = make(map[string]struct{}) + if reservation.Spec.TargetHost != "" { + hostsToBlock[reservation.Spec.TargetHost] = struct{}{} + } + if reservation.Status.Host != "" { + hostsToBlock[reservation.Status.Host] = struct{}{} + } + if len(hostsToBlock) == 0 { + traceLog.Debug("skipping reservation with no host", "reservation", reservation.Name) + return nil, nil + } + + // Calculate resources to block + resourcesToBlock = s.calculateResourcesToBlock(traceLog, reservation) + + return resourcesToBlock, hostsToBlock +} + +// shouldBlockCRReservation determines if a CR reservation should block capacity. +// Returns false if the reservation should be unlocked (resources available for the request). +func (s *FilterHasEnoughCapacity) shouldBlockCRReservation( + traceLog *slog.Logger, + request api.ExternalSchedulerRequest, + reservation *v1alpha1.Reservation, +) bool { + // Skip if no CommittedResourceReservation spec or no resource group set. + if reservation.Spec.CommittedResourceReservation == nil || + reservation.Spec.CommittedResourceReservation.ResourceGroup == "" { + return false // Not handled by us (no resource group set). + } + + // Check if this is a CR reservation scheduling request. + // If so, we should NOT unlock any CR reservations to prevent overbooking. + // CR capacity should only be unlocked for actual VM scheduling. + // IMPORTANT: This check MUST happen before IgnoredReservationTypes check + // to ensure CR reservation scheduling cannot bypass reservation locking. + intent, err := request.GetIntent() + switch { + case err == nil && intent == api.ReserveForCommittedResourceIntent: + // CR reservation scheduling - always block to prevent overbooking. + traceLog.Debug("keeping CR reservation locked for CR reservation scheduling", + "reservation", reservation.Name, "intent", intent) + return true + + case slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type): + // Check IgnoredReservationTypes AFTER intent check for CR reservations. + // This ensures CR reservation scheduling always respects existing CR reservations. + traceLog.Debug("ignoring CR reservation type per config", "reservation", reservation.Name) + return false + + case !s.Options.LockReserved && + // For committed resource reservations: unlock resources only if: + // 1. Project ID matches + // 2. ResourceGroup matches the flavor's hw_version + reservation.Spec.CommittedResourceReservation.ProjectID == request.Spec.Data.ProjectID && + reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"]: + // Unlock for matching project and resource group. + traceLog.Info("unlocking resources reserved by matching committed resource reservation", + "reservation", reservation.Name, + "instanceUUID", request.Spec.Data.InstanceUUID, + "projectID", request.Spec.Data.ProjectID, + "resourceGroup", reservation.Spec.CommittedResourceReservation.ResourceGroup) + return false + } + + return true +} + +// shouldBlockFailoverReservation determines if a failover reservation should block capacity. +// Returns false if the reservation should be unlocked (resources available for the request). +func (s *FilterHasEnoughCapacity) shouldBlockFailoverReservation( + traceLog *slog.Logger, + request api.ExternalSchedulerRequest, + reservation *v1alpha1.Reservation, +) bool { + // Check if failover reservations should be ignored. + if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { + traceLog.Debug("ignoring failover reservation type per config", "reservation", reservation.Name) + return false + } + + // For failover reservations: if the requested VM is contained in the allocations map + // AND this is an evacuation request, unlock the resources. + // We only unlock during evacuations because: + // 1. Failover reservations are specifically for HA/evacuation scenarios. + // 2. During live migrations or other operations, we don't want to use failover capacity. + // Note: we cannot use failover reservations from other VMs, as that can invalidate our HA guarantees. + intent, err := request.GetIntent() + if err == nil && intent == api.EvacuateIntent { + if reservation.Status.FailoverReservation != nil { + if _, contained := reservation.Status.FailoverReservation.Allocations[request.Spec.Data.InstanceUUID]; contained { + traceLog.Info("unlocking resources reserved by failover reservation for VM in allocations (evacuation)", "reservation", reservation.Name, - "instanceUUID", request.Spec.Data.InstanceUUID, - "projectID", request.Spec.Data.ProjectID, - "resourceGroup", reservation.Spec.CommittedResourceReservation.ResourceGroup) - continue + "instanceUUID", request.Spec.Data.InstanceUUID) + return false } + } + } - case v1alpha1.ReservationTypeFailover: - // Check if failover reservations should be ignored - if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { - traceLog.Debug("ignoring failover reservation type per config", "reservation", reservation.Name) - continue + traceLog.Debug("processing failover reservation", "reservation", reservation.Name) + return true +} + +// calculateResourcesToBlock determines how much resources a reservation should block. +// For CR reservations with running allocations, it subtracts already-consumed resources +// to prevent double-blocking of resources already consumed by running instances. +func (s *FilterHasEnoughCapacity) calculateResourcesToBlock( + traceLog *slog.Logger, + reservation *v1alpha1.Reservation, +) map[hv1.ResourceName]resource.Quantity { + // For CR reservations with allocations, calculate remaining (unallocated) resources to block. + // This prevents double-counting: VMs already running on the host are accounted for + // in the hypervisor's allocation, so we shouldn't also block reservation resources for them. + if reservation.Spec.Type == v1alpha1.ReservationTypeCommittedResource && + reservation.Spec.TargetHost == reservation.Status.Host && // not being migrated + reservation.Spec.CommittedResourceReservation != nil && + reservation.Status.CommittedResourceReservation != nil && + len(reservation.Spec.CommittedResourceReservation.Allocations) > 0 && + len(reservation.Status.CommittedResourceReservation.Allocations) > 0 { + resourcesToBlock := make(map[hv1.ResourceName]resource.Quantity) + for k, v := range reservation.Spec.Resources { + resourcesToBlock[k] = v.DeepCopy() + } + + // Subtract already-allocated resources because those consume already resources on the host. + // Only subtract if allocation is already present in status (VM is actually running). + for instanceUUID, allocation := range reservation.Spec.CommittedResourceReservation.Allocations { + if _, isRunning := reservation.Status.CommittedResourceReservation.Allocations[instanceUUID]; !isRunning { + continue // VM not yet spawned, keep blocking its reserved resources. } - // For failover reservations: if the requested VM is contained in the allocations map - // AND this is an evacuation request, unlock the resources. - // We only unlock during evacuations because: - // 1. Failover reservations are specifically for HA/evacuation scenarios. - // 2. During live migrations or other operations, we don't want to use failover capacity. - // Note: we cannot use failover reservations from other VMs, as that can invalidate our HA guarantees. - intent, err := request.GetIntent() - if err == nil && intent == api.EvacuateIntent { - if reservation.Status.FailoverReservation != nil { - if _, contained := reservation.Status.FailoverReservation.Allocations[request.Spec.Data.InstanceUUID]; contained { - traceLog.Info("unlocking resources reserved by failover reservation for VM in allocations (evacuation)", - "reservation", reservation.Name, - "instanceUUID", request.Spec.Data.InstanceUUID) - continue - } + + for resourceName, quantity := range allocation.Resources { + if current, ok := resourcesToBlock[resourceName]; ok { + current.Sub(quantity) + resourcesToBlock[resourceName] = current + traceLog.Debug("subtracting allocated resources from reservation", + "reservation", reservation.Name, + "instanceUUID", instanceUUID, + "resource", resourceName, + "quantity", quantity.String()) } } - traceLog.Debug("processing failover reservation", "reservation", reservation.Name) } - // Block resources on BOTH Spec.TargetHost (desired) AND Status.Host (actual). - // This ensures capacity is blocked during the transition period when a reservation - // is being placed (TargetHost set) and after it's placed (Host set). - // If both are the same, we only subtract once. - hostsToBlock := make(map[string]struct{}) - if reservation.Spec.TargetHost != "" { - hostsToBlock[reservation.Spec.TargetHost] = struct{}{} - } - if reservation.Status.Host != "" { - hostsToBlock[reservation.Status.Host] = struct{}{} - } - if len(hostsToBlock) == 0 { - traceLog.Debug("skipping reservation with no host", "reservation", reservation.Name) - continue - } + return resourcesToBlock + } - // For CR reservations with allocations, calculate remaining (unallocated) resources to block. - // This prevents double-blocking of resources already consumed by running instances. - var resourcesToBlock map[hv1.ResourceName]resource.Quantity - if reservation.Spec.Type == v1alpha1.ReservationTypeCommittedResource && - // if the reservation is not being migrated, block only unused resources - reservation.Spec.TargetHost == reservation.Status.Host && - reservation.Spec.CommittedResourceReservation != nil && - reservation.Status.CommittedResourceReservation != nil && - len(reservation.Spec.CommittedResourceReservation.Allocations) > 0 && - len(reservation.Status.CommittedResourceReservation.Allocations) > 0 { - // Start with full reservation resources - resourcesToBlock = make(map[hv1.ResourceName]resource.Quantity) - for k, v := range reservation.Spec.Resources { - resourcesToBlock[k] = v.DeepCopy() - } + // For other reservation types or CR without allocations, block full resources + return reservation.Spec.Resources +} - // Subtract already-allocated resources because those consume already resources on the host - for instanceUUID, allocation := range reservation.Spec.CommittedResourceReservation.Allocations { - // Only subtract if allocation is already present in status (VM is actually running) - if _, isRunning := reservation.Status.CommittedResourceReservation.Allocations[instanceUUID]; !isRunning { - continue - } +// blockResourcesOnHosts subtracts the given resources from the free resources map for each host. +func (s *FilterHasEnoughCapacity) blockResourcesOnHosts( + traceLog *slog.Logger, + resourcesToBlock map[hv1.ResourceName]resource.Quantity, + hostsToBlock map[string]struct{}, + freeResourcesByHost map[string]map[hv1.ResourceName]resource.Quantity, + reservation *v1alpha1.Reservation, +) { - for resourceName, quantity := range allocation.Resources { - if current, ok := resourcesToBlock[resourceName]; ok { - current.Sub(quantity) - resourcesToBlock[resourceName] = current - traceLog.Debug("subtracting allocated resources from reservation", - "reservation", reservation.Name, - "instanceUUID", instanceUUID, - "resource", resourceName, - "quantity", quantity.String()) - } - } - } - } else { - // For other reservation types or CR without allocations, block full resources - resourcesToBlock = reservation.Spec.Resources + for host := range hostsToBlock { + if _, hostExists := freeResourcesByHost[host]; !hostExists { + traceLog.Debug("skipping reservation for unknown host", + "reservation", reservation.Name, "host", host) + continue } - // Block the calculated resources on each host - for host := range hostsToBlock { - // Skip hosts that don't have a corresponding Hypervisor resource. - if _, hostExists := freeResourcesByHost[host]; !hostExists { - traceLog.Debug("skipping reservation for unknown host", - "reservation", reservation.Name, - "host", host) - continue - } - if cpu, ok := resourcesToBlock["cpu"]; ok { - if freeCPU, exists := freeResourcesByHost[host]["cpu"]; exists { - freeCPU.Sub(cpu) - if freeCPU.Value() < 0 { - traceLog.Warn("negative free CPU after blocking reservation", - "host", host, - "reservation", reservation.Name, - "reservationType", reservation.Spec.Type, - "freeCPU", freeCPU.String(), - "blocked", cpu.String()) - freeCPU = resource.MustParse("0") - } - freeResourcesByHost[host]["cpu"] = freeCPU + if cpu, ok := resourcesToBlock["cpu"]; ok { + if freeCPU, exists := freeResourcesByHost[host]["cpu"]; exists { + freeCPU.Sub(cpu) + if freeCPU.Value() < 0 { + traceLog.Warn("negative free CPU after blocking reservation", + "host", host, + "reservation", reservation.Name, + "reservationType", reservation.Spec.Type, + "freeCPU", freeCPU.String(), + "blocked", cpu.String()) + freeCPU = resource.MustParse("0") } + freeResourcesByHost[host]["cpu"] = freeCPU } - if memory, ok := resourcesToBlock["memory"]; ok { - if freeMemory, exists := freeResourcesByHost[host]["memory"]; exists { - freeMemory.Sub(memory) - if freeMemory.Value() < 0 { - traceLog.Warn("negative free memory after blocking reservation", - "host", host, - "reservation", reservation.Name, - "reservationType", reservation.Spec.Type, - "freeMemory", freeMemory.String(), - "blocked", memory.String()) - freeMemory = resource.MustParse("0") - } - freeResourcesByHost[host]["memory"] = freeMemory + } + + if memory, ok := resourcesToBlock["memory"]; ok { + if freeMemory, exists := freeResourcesByHost[host]["memory"]; exists { + freeMemory.Sub(memory) + if freeMemory.Value() < 0 { + traceLog.Warn("negative free memory after blocking reservation", + "host", host, + "reservation", reservation.Name, + "reservationType", reservation.Spec.Type, + "freeMemory", freeMemory.String(), + "blocked", memory.String()) + freeMemory = resource.MustParse("0") } + freeResourcesByHost[host]["memory"] = freeMemory } } } +} + +// filterHostsByCapacity removes hosts that don't have enough resources for the request. +func (s *FilterHasEnoughCapacity) filterHostsByCapacity( + traceLog *slog.Logger, + request api.ExternalSchedulerRequest, + freeResourcesByHost map[string]map[hv1.ResourceName]resource.Quantity, + result *lib.FilterWeigherPipelineStepResult, +) { hostsEncountered := make(map[string]struct{}) + for host, free := range freeResourcesByHost { hostsEncountered[host] = struct{}{} - // Check cpu capacity. - if request.Spec.Data.Flavor.Data.VCPUs == 0 { - return nil, errors.New("flavor has 0 vcpus") - } - freeCPU, ok := free["cpu"] - if !ok || freeCPU.Value() < 0 { - traceLog.Error( - "host with invalid CPU capacity", - "host", host, "freeCPU", freeCPU.String(), - ) - continue - } - // Calculate how many instances can fit on this host, based on cpu. - //nolint:gosec // We're checking for underflows above (< 0). - vcpuSlots := uint64(freeCPU.Value()) / - request.Spec.Data.Flavor.Data.VCPUs - if vcpuSlots < request.Spec.Data.NumInstances { - traceLog.Info( - "filtering host due to insufficient CPU capacity", - "host", host, "requested", request.Spec.Data.Flavor.Data.VCPUs, - "available", freeCPU.String(), - ) - delete(result.Activations, host) + // Check CPU capacity + if !s.hostHasEnoughCPU(traceLog, host, free, request, result) { continue } - // Check memory capacity. - if request.Spec.Data.Flavor.Data.MemoryMB == 0 { - return nil, errors.New("flavor has 0 memory") - } - freeMemory, ok := free["memory"] - if !ok || freeMemory.Value() < 0 { - traceLog.Error( - "host with invalid memory capacity", - "host", host, "freeMemory", freeMemory.String(), - ) + // Check memory capacity + if !s.hostHasEnoughMemory(traceLog, host, free, request, result) { continue } - // Calculate how many instances can fit on this host, based on memory. - // Note: according to the OpenStack docs, the memory is in MB, not MiB. - // See: https://docs.openstack.org/nova/latest/user/flavors.html - //nolint:gosec // We're checking for underflows above (< 0). - memorySlots := uint64(freeMemory.Value()/1_000_000 /* MB */) / - request.Spec.Data.Flavor.Data.MemoryMB - if memorySlots < request.Spec.Data.NumInstances { - traceLog.Info( - "filtering host due to insufficient RAM capacity", - "host", host, "requested_mb", request.Spec.Data.Flavor.Data.MemoryMB, - "available_mb", freeMemory.String(), - ) - delete(result.Activations, host) - continue - } - traceLog.Info( - "host has enough capacity", "host", host, + + freeCPU := free["cpu"] + freeMemory := free["memory"] + traceLog.Info("host has enough capacity", "host", host, "requested_cpus", request.Spec.Data.Flavor.Data.VCPUs, "available_cpus", freeCPU.String(), "requested_memory_mb", request.Spec.Data.Flavor.Data.MemoryMB, - "available_memory", freeMemory.String(), - ) + "available_memory", freeMemory.String()) } - // Remove all hosts that weren't encountered. + // Remove hosts that weren't encountered (no hypervisor data) for host := range result.Activations { if _, ok := hostsEncountered[host]; !ok { delete(result.Activations, host) - traceLog.Info( - "removing host with unknown capacity", - "host", host, - ) + traceLog.Info("removing host with unknown capacity", "host", host) } } - return result, nil +} + +// hostHasEnoughCPU checks if a host has enough CPU for the request. +func (s *FilterHasEnoughCapacity) hostHasEnoughCPU( + traceLog *slog.Logger, + host string, + free map[hv1.ResourceName]resource.Quantity, + request api.ExternalSchedulerRequest, + result *lib.FilterWeigherPipelineStepResult, +) bool { + + if request.Spec.Data.Flavor.Data.VCPUs == 0 { + return false // Will be caught as error in caller + } + + freeCPU, ok := free["cpu"] + if !ok || freeCPU.Value() < 0 { + traceLog.Error("host with invalid CPU capacity", "host", host, "freeCPU", freeCPU.String()) + delete(result.Activations, host) + return false + } + + //nolint:gosec // We're checking for underflows above (< 0) + vcpuSlots := uint64(freeCPU.Value()) / request.Spec.Data.Flavor.Data.VCPUs + if vcpuSlots < request.Spec.Data.NumInstances { + traceLog.Info("filtering host due to insufficient CPU capacity", + "host", host, + "requested", request.Spec.Data.Flavor.Data.VCPUs, + "available", freeCPU.String()) + delete(result.Activations, host) + return false + } + + return true +} + +// hostHasEnoughMemory checks if a host has enough memory for the request. +func (s *FilterHasEnoughCapacity) hostHasEnoughMemory( + traceLog *slog.Logger, + host string, + free map[hv1.ResourceName]resource.Quantity, + request api.ExternalSchedulerRequest, + result *lib.FilterWeigherPipelineStepResult, +) bool { + + if request.Spec.Data.Flavor.Data.MemoryMB == 0 { + return false // Will be caught as error in caller + } + + freeMemory, ok := free["memory"] + if !ok || freeMemory.Value() < 0 { + traceLog.Error("host with invalid memory capacity", "host", host, "freeMemory", freeMemory.String()) + delete(result.Activations, host) + return false + } + + //nolint:gosec // We're checking for underflows above (< 0) + memorySlots := uint64(freeMemory.Value()/1_000_000) / request.Spec.Data.Flavor.Data.MemoryMB + if memorySlots < request.Spec.Data.NumInstances { + traceLog.Info("filtering host due to insufficient RAM capacity", + "host", host, + "requested_mb", request.Spec.Data.Flavor.Data.MemoryMB, + "available_mb", freeMemory.String()) + delete(result.Activations, host) + return false + } + + return true } func init() { From 78d8019299ae02a2f736c9551fc09e4a142c4bad Mon Sep 17 00:00:00 2001 From: mblos Date: Thu, 26 Mar 2026 10:21:37 +0100 Subject: [PATCH 3/4] Revert "refactoring" lets do separated This reverts commit e118cf9f2b5af7528874de09b55f6926b7443d54. --- .../filters/filter_has_enough_capacity.go | 559 +++++++----------- 1 file changed, 212 insertions(+), 347 deletions(-) diff --git a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go index af6089ad1..0a347a53a 100644 --- a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go +++ b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go @@ -5,6 +5,7 @@ package filters import ( "context" + "errors" "log/slog" "slices" @@ -50,39 +51,17 @@ type FilterHasEnoughCapacity struct { func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.ExternalSchedulerRequest) (*lib.FilterWeigherPipelineStepResult, error) { result := s.IncludeAllHostsFromRequest(request) - // Step 1: Build free resources map from hypervisors - freeResourcesByHost, err := s.buildFreeResourcesFromHypervisors(traceLog) - if err != nil { - return nil, err - } - - // Step 2: Apply reservation blocking - if err := s.applyReservationBlocking(traceLog, request, freeResourcesByHost); err != nil { - return nil, err - } - - // Step 3: Filter hosts by capacity - s.filterHostsByCapacity(traceLog, request, freeResourcesByHost, result) - - return result, nil -} - -// buildFreeResourcesFromHypervisors creates a map of free resources per host. -// The hypervisor resource auto-discovers its current utilization. -// We can use the hypervisor status to calculate the total capacity -// and then subtract the actual resource allocation from virtual machines. -func (s *FilterHasEnoughCapacity) buildFreeResourcesFromHypervisors( - traceLog *slog.Logger, -) (map[string]map[hv1.ResourceName]resource.Quantity, error) { - + // This map holds the free resources per host. freeResourcesByHost := make(map[string]map[hv1.ResourceName]resource.Quantity) + // The hypervisor resource auto-discovers its current utilization. + // We can use the hypervisor status to calculate the total capacity + // and then subtract the actual resource allocation from virtual machines. hvs := &hv1.HypervisorList{} if err := s.Client.List(context.Background(), hvs); err != nil { traceLog.Error("failed to list hypervisors", "error", err) return nil, err } - for _, hv := range hvs.Items { if hv.Status.EffectiveCapacity == nil { traceLog.Warn("hypervisor with nil effective capacity, use capacity instead (overprovisioning not considered)", "host", hv.Name) @@ -96,8 +75,10 @@ func (s *FilterHasEnoughCapacity) buildFreeResourcesFromHypervisors( for resourceName, allocated := range hv.Status.Allocation { free, ok := freeResourcesByHost[hv.Name][resourceName] if !ok { - traceLog.Error("hypervisor with allocation for unknown resource", - "host", hv.Name, "resource", resourceName) + traceLog.Error( + "hypervisor with allocation for unknown resource", + "host", hv.Name, "resource", resourceName, + ) continue } free.Sub(allocated) @@ -105,372 +86,256 @@ func (s *FilterHasEnoughCapacity) buildFreeResourcesFromHypervisors( } } - return freeResourcesByHost, nil -} - -// applyReservationBlocking subtracts reserved resources from the free resources map. -// It handles unlocking logic for matching CR and failover reservations. -// Only active reservations (Ready=True) are considered. -func (s *FilterHasEnoughCapacity) applyReservationBlocking( - traceLog *slog.Logger, - request api.ExternalSchedulerRequest, - freeResourcesByHost map[string]map[hv1.ResourceName]resource.Quantity, -) error { - + // Subtract reserved resources by Reservations. var reservations v1alpha1.ReservationList if err := s.Client.List(context.Background(), &reservations); err != nil { - return err + return nil, err } - for _, reservation := range reservations.Items { if !meta.IsStatusConditionTrue(reservation.Status.Conditions, v1alpha1.ReservationConditionReady) { - continue - } - - // Process reservation and get resources/hosts to block (or nil if unlocked) - resourcesToBlock, hostsToBlock := s.processReservationForCapacity(traceLog, request, &reservation) - if resourcesToBlock == nil || len(hostsToBlock) == 0 { - continue - } - - // Block the calculated resources on each host - s.blockResourcesOnHosts(traceLog, resourcesToBlock, hostsToBlock, freeResourcesByHost, &reservation) - } - - return nil -} - -// processReservationForCapacity determines if a reservation should block capacity and how much. -// Returns nil, nil if the reservation should be unlocked (not block any resources). -func (s *FilterHasEnoughCapacity) processReservationForCapacity( - traceLog *slog.Logger, - request api.ExternalSchedulerRequest, - reservation *v1alpha1.Reservation, -) (resourcesToBlock map[hv1.ResourceName]resource.Quantity, hostsToBlock map[string]struct{}) { - // Handle reservation based on its type - switch reservation.Spec.Type { - case v1alpha1.ReservationTypeCommittedResource, "": - if !s.shouldBlockCRReservation(traceLog, request, reservation) { - return nil, nil - } - case v1alpha1.ReservationTypeFailover: - if !s.shouldBlockFailoverReservation(traceLog, request, reservation) { - return nil, nil + continue // Only consider active reservations (Ready=True). } - } - - // Block resources on BOTH Spec.TargetHost (desired) AND Status.Host (actual). - // This ensures capacity is blocked during the transition period when a reservation - // is being placed (TargetHost set) and after it's placed (Host set). - // If both are the same, we only subtract once (using a map). - hostsToBlock = make(map[string]struct{}) - if reservation.Spec.TargetHost != "" { - hostsToBlock[reservation.Spec.TargetHost] = struct{}{} - } - if reservation.Status.Host != "" { - hostsToBlock[reservation.Status.Host] = struct{}{} - } - if len(hostsToBlock) == 0 { - traceLog.Debug("skipping reservation with no host", "reservation", reservation.Name) - return nil, nil - } - - // Calculate resources to block - resourcesToBlock = s.calculateResourcesToBlock(traceLog, reservation) - - return resourcesToBlock, hostsToBlock -} - -// shouldBlockCRReservation determines if a CR reservation should block capacity. -// Returns false if the reservation should be unlocked (resources available for the request). -func (s *FilterHasEnoughCapacity) shouldBlockCRReservation( - traceLog *slog.Logger, - request api.ExternalSchedulerRequest, - reservation *v1alpha1.Reservation, -) bool { - // Skip if no CommittedResourceReservation spec or no resource group set. - if reservation.Spec.CommittedResourceReservation == nil || - reservation.Spec.CommittedResourceReservation.ResourceGroup == "" { - return false // Not handled by us (no resource group set). - } - - // Check if this is a CR reservation scheduling request. - // If so, we should NOT unlock any CR reservations to prevent overbooking. - // CR capacity should only be unlocked for actual VM scheduling. - // IMPORTANT: This check MUST happen before IgnoredReservationTypes check - // to ensure CR reservation scheduling cannot bypass reservation locking. - intent, err := request.GetIntent() - switch { - case err == nil && intent == api.ReserveForCommittedResourceIntent: - // CR reservation scheduling - always block to prevent overbooking. - traceLog.Debug("keeping CR reservation locked for CR reservation scheduling", - "reservation", reservation.Name, "intent", intent) - return true - - case slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type): - // Check IgnoredReservationTypes AFTER intent check for CR reservations. - // This ensures CR reservation scheduling always respects existing CR reservations. - traceLog.Debug("ignoring CR reservation type per config", "reservation", reservation.Name) - return false - case !s.Options.LockReserved && - // For committed resource reservations: unlock resources only if: - // 1. Project ID matches - // 2. ResourceGroup matches the flavor's hw_version - reservation.Spec.CommittedResourceReservation.ProjectID == request.Spec.Data.ProjectID && - reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"]: - // Unlock for matching project and resource group. - traceLog.Info("unlocking resources reserved by matching committed resource reservation", - "reservation", reservation.Name, - "instanceUUID", request.Spec.Data.InstanceUUID, - "projectID", request.Spec.Data.ProjectID, - "resourceGroup", reservation.Spec.CommittedResourceReservation.ResourceGroup) - return false - } - - return true -} - -// shouldBlockFailoverReservation determines if a failover reservation should block capacity. -// Returns false if the reservation should be unlocked (resources available for the request). -func (s *FilterHasEnoughCapacity) shouldBlockFailoverReservation( - traceLog *slog.Logger, - request api.ExternalSchedulerRequest, - reservation *v1alpha1.Reservation, -) bool { - // Check if failover reservations should be ignored. - if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { - traceLog.Debug("ignoring failover reservation type per config", "reservation", reservation.Name) - return false - } + // Handle reservation based on its type. + switch reservation.Spec.Type { + case v1alpha1.ReservationTypeCommittedResource, "": // Empty string for backward compatibility + // Skip if no CommittedResourceReservation spec or no resource group set. + if reservation.Spec.CommittedResourceReservation == nil || reservation.Spec.CommittedResourceReservation.ResourceGroup == "" { + continue // Not handled by us (no resource group set). + } - // For failover reservations: if the requested VM is contained in the allocations map - // AND this is an evacuation request, unlock the resources. - // We only unlock during evacuations because: - // 1. Failover reservations are specifically for HA/evacuation scenarios. - // 2. During live migrations or other operations, we don't want to use failover capacity. - // Note: we cannot use failover reservations from other VMs, as that can invalidate our HA guarantees. - intent, err := request.GetIntent() - if err == nil && intent == api.EvacuateIntent { - if reservation.Status.FailoverReservation != nil { - if _, contained := reservation.Status.FailoverReservation.Allocations[request.Spec.Data.InstanceUUID]; contained { - traceLog.Info("unlocking resources reserved by failover reservation for VM in allocations (evacuation)", + // Check if this is a CR reservation scheduling request. + // If so, we should NOT unlock any CR reservations to prevent overbooking. + // CR capacity should only be unlocked for actual VM scheduling. + // IMPORTANT: This check MUST happen before IgnoredReservationTypes check + // to ensure CR reservation scheduling cannot bypass reservation locking. + intent, err := request.GetIntent() + switch { + case err == nil && intent == api.ReserveForCommittedResourceIntent: + traceLog.Debug("keeping CR reservation locked for CR reservation scheduling", "reservation", reservation.Name, - "instanceUUID", request.Spec.Data.InstanceUUID) - return false + "intent", intent) + // Don't continue - fall through to block the resources + case slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type): + // Check IgnoredReservationTypes AFTER intent check for CR reservations. + // This ensures CR reservation scheduling always respects existing CR reservations. + traceLog.Debug("ignoring CR reservation type per config", "reservation", reservation.Name) + continue + case !s.Options.LockReserved && + // For committed resource reservations: unlock resources only if: + // 1. Project ID matches + // 2. ResourceGroup matches the flavor's hw_version + reservation.Spec.CommittedResourceReservation.ProjectID == request.Spec.Data.ProjectID && + reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"]: + traceLog.Info("unlocking resources reserved by matching committed resource reservation with allocation", + "reservation", reservation.Name, + "instanceUUID", request.Spec.Data.InstanceUUID, + "projectID", request.Spec.Data.ProjectID, + "resourceGroup", reservation.Spec.CommittedResourceReservation.ResourceGroup) + continue } - } - } - - traceLog.Debug("processing failover reservation", "reservation", reservation.Name) - return true -} -// calculateResourcesToBlock determines how much resources a reservation should block. -// For CR reservations with running allocations, it subtracts already-consumed resources -// to prevent double-blocking of resources already consumed by running instances. -func (s *FilterHasEnoughCapacity) calculateResourcesToBlock( - traceLog *slog.Logger, - reservation *v1alpha1.Reservation, -) map[hv1.ResourceName]resource.Quantity { - // For CR reservations with allocations, calculate remaining (unallocated) resources to block. - // This prevents double-counting: VMs already running on the host are accounted for - // in the hypervisor's allocation, so we shouldn't also block reservation resources for them. - if reservation.Spec.Type == v1alpha1.ReservationTypeCommittedResource && - reservation.Spec.TargetHost == reservation.Status.Host && // not being migrated - reservation.Spec.CommittedResourceReservation != nil && - reservation.Status.CommittedResourceReservation != nil && - len(reservation.Spec.CommittedResourceReservation.Allocations) > 0 && - len(reservation.Status.CommittedResourceReservation.Allocations) > 0 { - resourcesToBlock := make(map[hv1.ResourceName]resource.Quantity) - for k, v := range reservation.Spec.Resources { - resourcesToBlock[k] = v.DeepCopy() - } - - // Subtract already-allocated resources because those consume already resources on the host. - // Only subtract if allocation is already present in status (VM is actually running). - for instanceUUID, allocation := range reservation.Spec.CommittedResourceReservation.Allocations { - if _, isRunning := reservation.Status.CommittedResourceReservation.Allocations[instanceUUID]; !isRunning { - continue // VM not yet spawned, keep blocking its reserved resources. + case v1alpha1.ReservationTypeFailover: + // Check if failover reservations should be ignored + if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { + traceLog.Debug("ignoring failover reservation type per config", "reservation", reservation.Name) + continue } - - for resourceName, quantity := range allocation.Resources { - if current, ok := resourcesToBlock[resourceName]; ok { - current.Sub(quantity) - resourcesToBlock[resourceName] = current - traceLog.Debug("subtracting allocated resources from reservation", - "reservation", reservation.Name, - "instanceUUID", instanceUUID, - "resource", resourceName, - "quantity", quantity.String()) + // For failover reservations: if the requested VM is contained in the allocations map + // AND this is an evacuation request, unlock the resources. + // We only unlock during evacuations because: + // 1. Failover reservations are specifically for HA/evacuation scenarios. + // 2. During live migrations or other operations, we don't want to use failover capacity. + // Note: we cannot use failover reservations from other VMs, as that can invalidate our HA guarantees. + intent, err := request.GetIntent() + if err == nil && intent == api.EvacuateIntent { + if reservation.Status.FailoverReservation != nil { + if _, contained := reservation.Status.FailoverReservation.Allocations[request.Spec.Data.InstanceUUID]; contained { + traceLog.Info("unlocking resources reserved by failover reservation for VM in allocations (evacuation)", + "reservation", reservation.Name, + "instanceUUID", request.Spec.Data.InstanceUUID) + continue + } } } + traceLog.Debug("processing failover reservation", "reservation", reservation.Name) } - return resourcesToBlock - } - - // For other reservation types or CR without allocations, block full resources - return reservation.Spec.Resources -} - -// blockResourcesOnHosts subtracts the given resources from the free resources map for each host. -func (s *FilterHasEnoughCapacity) blockResourcesOnHosts( - traceLog *slog.Logger, - resourcesToBlock map[hv1.ResourceName]resource.Quantity, - hostsToBlock map[string]struct{}, - freeResourcesByHost map[string]map[hv1.ResourceName]resource.Quantity, - reservation *v1alpha1.Reservation, -) { - - for host := range hostsToBlock { - if _, hostExists := freeResourcesByHost[host]; !hostExists { - traceLog.Debug("skipping reservation for unknown host", - "reservation", reservation.Name, "host", host) + // Block resources on BOTH Spec.TargetHost (desired) AND Status.Host (actual). + // This ensures capacity is blocked during the transition period when a reservation + // is being placed (TargetHost set) and after it's placed (Host set). + // If both are the same, we only subtract once. + hostsToBlock := make(map[string]struct{}) + if reservation.Spec.TargetHost != "" { + hostsToBlock[reservation.Spec.TargetHost] = struct{}{} + } + if reservation.Status.Host != "" { + hostsToBlock[reservation.Status.Host] = struct{}{} + } + if len(hostsToBlock) == 0 { + traceLog.Debug("skipping reservation with no host", "reservation", reservation.Name) continue } - if cpu, ok := resourcesToBlock["cpu"]; ok { - if freeCPU, exists := freeResourcesByHost[host]["cpu"]; exists { - freeCPU.Sub(cpu) - if freeCPU.Value() < 0 { - traceLog.Warn("negative free CPU after blocking reservation", - "host", host, - "reservation", reservation.Name, - "reservationType", reservation.Spec.Type, - "freeCPU", freeCPU.String(), - "blocked", cpu.String()) - freeCPU = resource.MustParse("0") + // For CR reservations with allocations, calculate remaining (unallocated) resources to block. + // This prevents double-blocking of resources already consumed by running instances. + var resourcesToBlock map[hv1.ResourceName]resource.Quantity + if reservation.Spec.Type == v1alpha1.ReservationTypeCommittedResource && + // if the reservation is not being migrated, block only unused resources + reservation.Spec.TargetHost == reservation.Status.Host && + reservation.Spec.CommittedResourceReservation != nil && + reservation.Status.CommittedResourceReservation != nil && + len(reservation.Spec.CommittedResourceReservation.Allocations) > 0 && + len(reservation.Status.CommittedResourceReservation.Allocations) > 0 { + // Start with full reservation resources + resourcesToBlock = make(map[hv1.ResourceName]resource.Quantity) + for k, v := range reservation.Spec.Resources { + resourcesToBlock[k] = v.DeepCopy() + } + + // Subtract already-allocated resources because those consume already resources on the host + for instanceUUID, allocation := range reservation.Spec.CommittedResourceReservation.Allocations { + // Only subtract if allocation is already present in status (VM is actually running) + if _, isRunning := reservation.Status.CommittedResourceReservation.Allocations[instanceUUID]; !isRunning { + continue + } + + for resourceName, quantity := range allocation.Resources { + if current, ok := resourcesToBlock[resourceName]; ok { + current.Sub(quantity) + resourcesToBlock[resourceName] = current + traceLog.Debug("subtracting allocated resources from reservation", + "reservation", reservation.Name, + "instanceUUID", instanceUUID, + "resource", resourceName, + "quantity", quantity.String()) + } } - freeResourcesByHost[host]["cpu"] = freeCPU } + } else { + // For other reservation types or CR without allocations, block full resources + resourcesToBlock = reservation.Spec.Resources } - if memory, ok := resourcesToBlock["memory"]; ok { - if freeMemory, exists := freeResourcesByHost[host]["memory"]; exists { - freeMemory.Sub(memory) - if freeMemory.Value() < 0 { - traceLog.Warn("negative free memory after blocking reservation", - "host", host, - "reservation", reservation.Name, - "reservationType", reservation.Spec.Type, - "freeMemory", freeMemory.String(), - "blocked", memory.String()) - freeMemory = resource.MustParse("0") + // Block the calculated resources on each host + for host := range hostsToBlock { + // Skip hosts that don't have a corresponding Hypervisor resource. + if _, hostExists := freeResourcesByHost[host]; !hostExists { + traceLog.Debug("skipping reservation for unknown host", + "reservation", reservation.Name, + "host", host) + continue + } + if cpu, ok := resourcesToBlock["cpu"]; ok { + if freeCPU, exists := freeResourcesByHost[host]["cpu"]; exists { + freeCPU.Sub(cpu) + if freeCPU.Value() < 0 { + traceLog.Warn("negative free CPU after blocking reservation", + "host", host, + "reservation", reservation.Name, + "reservationType", reservation.Spec.Type, + "freeCPU", freeCPU.String(), + "blocked", cpu.String()) + freeCPU = resource.MustParse("0") + } + freeResourcesByHost[host]["cpu"] = freeCPU + } + } + if memory, ok := resourcesToBlock["memory"]; ok { + if freeMemory, exists := freeResourcesByHost[host]["memory"]; exists { + freeMemory.Sub(memory) + if freeMemory.Value() < 0 { + traceLog.Warn("negative free memory after blocking reservation", + "host", host, + "reservation", reservation.Name, + "reservationType", reservation.Spec.Type, + "freeMemory", freeMemory.String(), + "blocked", memory.String()) + freeMemory = resource.MustParse("0") + } + freeResourcesByHost[host]["memory"] = freeMemory } - freeResourcesByHost[host]["memory"] = freeMemory } } } -} - -// filterHostsByCapacity removes hosts that don't have enough resources for the request. -func (s *FilterHasEnoughCapacity) filterHostsByCapacity( - traceLog *slog.Logger, - request api.ExternalSchedulerRequest, - freeResourcesByHost map[string]map[hv1.ResourceName]resource.Quantity, - result *lib.FilterWeigherPipelineStepResult, -) { hostsEncountered := make(map[string]struct{}) - for host, free := range freeResourcesByHost { hostsEncountered[host] = struct{}{} - // Check CPU capacity - if !s.hostHasEnoughCPU(traceLog, host, free, request, result) { + // Check cpu capacity. + if request.Spec.Data.Flavor.Data.VCPUs == 0 { + return nil, errors.New("flavor has 0 vcpus") + } + freeCPU, ok := free["cpu"] + if !ok || freeCPU.Value() < 0 { + traceLog.Error( + "host with invalid CPU capacity", + "host", host, "freeCPU", freeCPU.String(), + ) continue } - - // Check memory capacity - if !s.hostHasEnoughMemory(traceLog, host, free, request, result) { + // Calculate how many instances can fit on this host, based on cpu. + //nolint:gosec // We're checking for underflows above (< 0). + vcpuSlots := uint64(freeCPU.Value()) / + request.Spec.Data.Flavor.Data.VCPUs + if vcpuSlots < request.Spec.Data.NumInstances { + traceLog.Info( + "filtering host due to insufficient CPU capacity", + "host", host, "requested", request.Spec.Data.Flavor.Data.VCPUs, + "available", freeCPU.String(), + ) + delete(result.Activations, host) continue } - freeCPU := free["cpu"] - freeMemory := free["memory"] - traceLog.Info("host has enough capacity", "host", host, + // Check memory capacity. + if request.Spec.Data.Flavor.Data.MemoryMB == 0 { + return nil, errors.New("flavor has 0 memory") + } + freeMemory, ok := free["memory"] + if !ok || freeMemory.Value() < 0 { + traceLog.Error( + "host with invalid memory capacity", + "host", host, "freeMemory", freeMemory.String(), + ) + continue + } + // Calculate how many instances can fit on this host, based on memory. + // Note: according to the OpenStack docs, the memory is in MB, not MiB. + // See: https://docs.openstack.org/nova/latest/user/flavors.html + //nolint:gosec // We're checking for underflows above (< 0). + memorySlots := uint64(freeMemory.Value()/1_000_000 /* MB */) / + request.Spec.Data.Flavor.Data.MemoryMB + if memorySlots < request.Spec.Data.NumInstances { + traceLog.Info( + "filtering host due to insufficient RAM capacity", + "host", host, "requested_mb", request.Spec.Data.Flavor.Data.MemoryMB, + "available_mb", freeMemory.String(), + ) + delete(result.Activations, host) + continue + } + traceLog.Info( + "host has enough capacity", "host", host, "requested_cpus", request.Spec.Data.Flavor.Data.VCPUs, "available_cpus", freeCPU.String(), "requested_memory_mb", request.Spec.Data.Flavor.Data.MemoryMB, - "available_memory", freeMemory.String()) + "available_memory", freeMemory.String(), + ) } - // Remove hosts that weren't encountered (no hypervisor data) + // Remove all hosts that weren't encountered. for host := range result.Activations { if _, ok := hostsEncountered[host]; !ok { delete(result.Activations, host) - traceLog.Info("removing host with unknown capacity", "host", host) + traceLog.Info( + "removing host with unknown capacity", + "host", host, + ) } } -} - -// hostHasEnoughCPU checks if a host has enough CPU for the request. -func (s *FilterHasEnoughCapacity) hostHasEnoughCPU( - traceLog *slog.Logger, - host string, - free map[hv1.ResourceName]resource.Quantity, - request api.ExternalSchedulerRequest, - result *lib.FilterWeigherPipelineStepResult, -) bool { - - if request.Spec.Data.Flavor.Data.VCPUs == 0 { - return false // Will be caught as error in caller - } - - freeCPU, ok := free["cpu"] - if !ok || freeCPU.Value() < 0 { - traceLog.Error("host with invalid CPU capacity", "host", host, "freeCPU", freeCPU.String()) - delete(result.Activations, host) - return false - } - - //nolint:gosec // We're checking for underflows above (< 0) - vcpuSlots := uint64(freeCPU.Value()) / request.Spec.Data.Flavor.Data.VCPUs - if vcpuSlots < request.Spec.Data.NumInstances { - traceLog.Info("filtering host due to insufficient CPU capacity", - "host", host, - "requested", request.Spec.Data.Flavor.Data.VCPUs, - "available", freeCPU.String()) - delete(result.Activations, host) - return false - } - - return true -} - -// hostHasEnoughMemory checks if a host has enough memory for the request. -func (s *FilterHasEnoughCapacity) hostHasEnoughMemory( - traceLog *slog.Logger, - host string, - free map[hv1.ResourceName]resource.Quantity, - request api.ExternalSchedulerRequest, - result *lib.FilterWeigherPipelineStepResult, -) bool { - - if request.Spec.Data.Flavor.Data.MemoryMB == 0 { - return false // Will be caught as error in caller - } - - freeMemory, ok := free["memory"] - if !ok || freeMemory.Value() < 0 { - traceLog.Error("host with invalid memory capacity", "host", host, "freeMemory", freeMemory.String()) - delete(result.Activations, host) - return false - } - - //nolint:gosec // We're checking for underflows above (< 0) - memorySlots := uint64(freeMemory.Value()/1_000_000) / request.Spec.Data.Flavor.Data.MemoryMB - if memorySlots < request.Spec.Data.NumInstances { - traceLog.Info("filtering host due to insufficient RAM capacity", - "host", host, - "requested_mb", request.Spec.Data.Flavor.Data.MemoryMB, - "available_mb", freeMemory.String()) - delete(result.Activations, host) - return false - } - - return true + return result, nil } func init() { From 6e4cee3426ac2b45004f11c4d4658d2a7fda3cf1 Mon Sep 17 00:00:00 2001 From: mblos Date: Thu, 26 Mar 2026 10:39:37 +0100 Subject: [PATCH 4/4] fix safety guard --- .../filters/filter_has_enough_capacity.go | 18 ++++++------------ .../filters/filter_has_enough_capacity_test.go | 10 +++++----- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go index 0a347a53a..d26c7c940 100644 --- a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go +++ b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go @@ -96,6 +96,12 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa continue // Only consider active reservations (Ready=True). } + // Check if this reservation type should be ignored + if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { + traceLog.Debug("ignoring reservation type", "type", reservation.Spec.Type, "reservation", reservation.Name) + continue + } + // Handle reservation based on its type. switch reservation.Spec.Type { case v1alpha1.ReservationTypeCommittedResource, "": // Empty string for backward compatibility @@ -107,8 +113,6 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa // Check if this is a CR reservation scheduling request. // If so, we should NOT unlock any CR reservations to prevent overbooking. // CR capacity should only be unlocked for actual VM scheduling. - // IMPORTANT: This check MUST happen before IgnoredReservationTypes check - // to ensure CR reservation scheduling cannot bypass reservation locking. intent, err := request.GetIntent() switch { case err == nil && intent == api.ReserveForCommittedResourceIntent: @@ -116,11 +120,6 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa "reservation", reservation.Name, "intent", intent) // Don't continue - fall through to block the resources - case slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type): - // Check IgnoredReservationTypes AFTER intent check for CR reservations. - // This ensures CR reservation scheduling always respects existing CR reservations. - traceLog.Debug("ignoring CR reservation type per config", "reservation", reservation.Name) - continue case !s.Options.LockReserved && // For committed resource reservations: unlock resources only if: // 1. Project ID matches @@ -136,11 +135,6 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa } case v1alpha1.ReservationTypeFailover: - // Check if failover reservations should be ignored - if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) { - traceLog.Debug("ignoring failover reservation type per config", "reservation", reservation.Name) - continue - } // For failover reservations: if the requested VM is contained in the allocations map // AND this is an evacuation request, unlock the resources. // We only unlock during evacuations because: diff --git a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go index 3186f7e99..cabc3a3b4 100644 --- a/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go +++ b/internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go @@ -912,7 +912,7 @@ func TestFilterHasEnoughCapacity_ReserveForCommittedResourceIntent(t *testing.T) filteredHosts: []string{}, }, { - name: "CR reservation scheduling: IgnoredReservationTypes config CANNOT bypass intent protection", + name: "CR reservation scheduling: IgnoredReservationTypes config DOES bypass intent protection (safety override)", hypervisors: []*hv1.Hypervisor{ newHypervisor("host1", "16", "8", "32Gi", "16Gi"), // 8 CPU free }, @@ -921,15 +921,15 @@ func TestFilterHasEnoughCapacity_ReserveForCommittedResourceIntent(t *testing.T) newCommittedReservation("existing-cr", "host1", "host1", "project-A", "m1.large", "gp-1", "8", "16Gi", nil, nil), }, // Request with reserve_for_committed_resource intent - // Even with IgnoredReservationTypes set to ignore CR, the intent should still block + // IgnoredReservationTypes is a safety flag that overrides everything, including intent request: newNovaRequestWithIntent("new-reservation-uuid", "project-A", "m1.large", "gp-1", 4, "8Gi", "reserve_for_committed_resource", false, []string{"host1"}), opts: FilterHasEnoughCapacityOpts{ LockReserved: false, - // Normally this would ignore CR reservations, but intent should override this + // IgnoredReservationTypes is a safety override - ignores CR even for CR scheduling IgnoredReservationTypes: []v1alpha1.ReservationType{v1alpha1.ReservationTypeCommittedResource}, }, - expectedHosts: []string{}, // host1 blocked because intent overrides IgnoredReservationTypes - filteredHosts: []string{"host1"}, // The CR reservation stays locked despite IgnoredReservationTypes + expectedHosts: []string{"host1"}, // CR reservation is ignored via IgnoredReservationTypes (safety override) + filteredHosts: []string{}, }, { name: "Normal VM scheduling: IgnoredReservationTypes config DOES work for normal VMs",