Skip to content

Commit 2028a3e

Browse files
mblosmblos
authored
fix: capacity filter considers also reservations that are placed, but waiting for 2nd reconcile cycle for updating the status (#880)
The capacity filter had a timing gap where newly placed reservations were invisible to concurrent scheduling calls, allowing multiple reservations to be assigned to the same host.
1 parent d307b7a commit 2028a3e

2 files changed

Lines changed: 139 additions & 50 deletions

File tree

‎internal/scheduling/nova/plugins/filters/filter_has_enough_capacity.go‎

Lines changed: 62 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -107,67 +107,79 @@ func (s *FilterHasEnoughCapacity) Run(traceLog *slog.Logger, request api.Externa
107107
return nil, err
108108
}
109109
for _, reservation := range reservations.Items {
110-
if !reservation.IsReady() {
111-
continue // Only consider active reservations (Ready=True).
112-
}
113-
114-
// Check if this reservation type should be ignored
110+
// Check if this reservation type should be ignored — applies regardless of ready state.
115111
if slices.Contains(s.Options.IgnoredReservationTypes, reservation.Spec.Type) {
116112
traceLog.Debug("ignoring reservation type", "type", reservation.Spec.Type, "reservation", reservation.Name)
117113
continue
118114
}
119115

120-
// Handle reservation based on its type.
121-
switch reservation.Spec.Type {
122-
case v1alpha1.ReservationTypeCommittedResource, "": // Empty string for backward compatibility
123-
// Skip if no CommittedResourceReservation spec or no resource group set.
124-
if reservation.Spec.CommittedResourceReservation == nil || reservation.Spec.CommittedResourceReservation.ResourceGroup == "" {
125-
continue // Not handled by us (no resource group set).
116+
if !reservation.IsReady() {
117+
if reservation.Spec.TargetHost == "" && reservation.Status.Host == "" {
118+
continue // not placed yet, nothing to block
126119
}
120+
// TargetHost is set but Ready has not been synced yet: the reservation controller
121+
// wrote the host in reconcile cycle 1 but hasn't completed cycle 2 (status sync).
122+
// Block the full slot now to prevent a concurrent scheduling call from picking the
123+
// same host. Unlock logic is intentionally skipped — an unconfirmed slot must not
124+
// be unlocked for any VM or project.
125+
traceLog.Warn("reservation has target host set but is not yet ready — blocking host to prevent double-booking",
126+
"reservation", reservation.Name,
127+
"targetHost", reservation.Spec.TargetHost,
128+
"statusHost", reservation.Status.Host,
129+
)
130+
} else {
131+
// Ready reservation: apply type-specific unlock logic.
132+
switch reservation.Spec.Type {
133+
case v1alpha1.ReservationTypeCommittedResource, "": // Empty string for backward compatibility
134+
// Skip if no CommittedResourceReservation spec or no resource group set.
135+
if reservation.Spec.CommittedResourceReservation == nil || reservation.Spec.CommittedResourceReservation.ResourceGroup == "" {
136+
continue // Not handled by us (no resource group set).
137+
}
127138

128-
// Check if this is a CR reservation scheduling request.
129-
// If so, we should NOT unlock any CR reservations to prevent overbooking.
130-
// CR capacity should only be unlocked for actual VM scheduling.
131-
intent, err := request.GetIntent()
132-
switch {
133-
case err == nil && intent == api.ReserveForCommittedResourceIntent:
134-
traceLog.Debug("keeping CR reservation locked for CR reservation scheduling",
135-
"reservation", reservation.Name,
136-
"intent", intent)
137-
// Don't continue - fall through to block the resources
138-
case !s.Options.LockReserved &&
139-
// For committed resource reservations: unlock resources only if:
140-
// 1. Project ID matches
141-
// 2. ResourceGroup matches the flavor's hw_version
142-
reservation.Spec.CommittedResourceReservation.ProjectID == request.Spec.Data.ProjectID &&
143-
reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"]:
144-
traceLog.Info("unlocking resources reserved by matching committed resource reservation with allocation",
145-
"reservation", reservation.Name,
146-
"instanceUUID", request.Spec.Data.InstanceUUID,
147-
"projectID", request.Spec.Data.ProjectID,
148-
"resourceGroup", reservation.Spec.CommittedResourceReservation.ResourceGroup)
149-
continue
150-
}
139+
// Check if this is a CR reservation scheduling request.
140+
// If so, we should NOT unlock any CR reservations to prevent overbooking.
141+
// CR capacity should only be unlocked for actual VM scheduling.
142+
intent, err := request.GetIntent()
143+
switch {
144+
case err == nil && intent == api.ReserveForCommittedResourceIntent:
145+
traceLog.Debug("keeping CR reservation locked for CR reservation scheduling",
146+
"reservation", reservation.Name,
147+
"intent", intent)
148+
// Don't continue - fall through to block the resources
149+
case !s.Options.LockReserved &&
150+
// For committed resource reservations: unlock resources only if:
151+
// 1. Project ID matches
152+
// 2. ResourceGroup matches the flavor's hw_version
153+
reservation.Spec.CommittedResourceReservation.ProjectID == request.Spec.Data.ProjectID &&
154+
reservation.Spec.CommittedResourceReservation.ResourceGroup == request.Spec.Data.Flavor.Data.ExtraSpecs["hw_version"]:
155+
traceLog.Info("unlocking resources reserved by matching committed resource reservation with allocation",
156+
"reservation", reservation.Name,
157+
"instanceUUID", request.Spec.Data.InstanceUUID,
158+
"projectID", request.Spec.Data.ProjectID,
159+
"resourceGroup", reservation.Spec.CommittedResourceReservation.ResourceGroup)
160+
continue
161+
}
151162

152-
case v1alpha1.ReservationTypeFailover:
153-
// For failover reservations: if the requested VM is contained in the allocations map
154-
// AND this is an evacuation request, unlock the resources.
155-
// We only unlock during evacuations because:
156-
// 1. Failover reservations are specifically for HA/evacuation scenarios.
157-
// 2. During live migrations or other operations, we don't want to use failover capacity.
158-
// Note: we cannot use failover reservations from other VMs, as that can invalidate our HA guarantees.
159-
intent, err := request.GetIntent()
160-
if err == nil && intent == api.EvacuateIntent {
161-
if reservation.Status.FailoverReservation != nil {
162-
if _, contained := reservation.Status.FailoverReservation.Allocations[request.Spec.Data.InstanceUUID]; contained {
163-
traceLog.Info("unlocking resources reserved by failover reservation for VM in allocations (evacuation)",
164-
"reservation", reservation.Name,
165-
"instanceUUID", request.Spec.Data.InstanceUUID)
166-
continue
163+
case v1alpha1.ReservationTypeFailover:
164+
// For failover reservations: if the requested VM is contained in the allocations map
165+
// AND this is an evacuation request, unlock the resources.
166+
// We only unlock during evacuations because:
167+
// 1. Failover reservations are specifically for HA/evacuation scenarios.
168+
// 2. During live migrations or other operations, we don't want to use failover capacity.
169+
// Note: we cannot use failover reservations from other VMs, as that can invalidate our HA guarantees.
170+
intent, err := request.GetIntent()
171+
if err == nil && intent == api.EvacuateIntent {
172+
if reservation.Status.FailoverReservation != nil {
173+
if _, contained := reservation.Status.FailoverReservation.Allocations[request.Spec.Data.InstanceUUID]; contained {
174+
traceLog.Info("unlocking resources reserved by failover reservation for VM in allocations (evacuation)",
175+
"reservation", reservation.Name,
176+
"instanceUUID", request.Spec.Data.InstanceUUID)
177+
continue
178+
}
167179
}
168180
}
181+
traceLog.Debug("processing failover reservation", "reservation", reservation.Name)
169182
}
170-
traceLog.Debug("processing failover reservation", "reservation", reservation.Name)
171183
}
172184

173185
// Block resources on BOTH Spec.TargetHost (desired) AND Status.Host (actual).

‎internal/scheduling/nova/plugins/filters/filter_has_enough_capacity_test.go‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,32 @@ func newHypervisorWithBothCapacities(name, cpuCap, cpuEffCap, memCap, memEffCap
9494
}
9595
}
9696

97+
// newUnconfirmedReservation creates a CR reservation in the "cycle-1-done, cycle-2-pending" state:
98+
// Spec.TargetHost is set but Status.Host is empty and Ready condition is not yet written.
99+
// This models the window between the reservation controller writing the target host and
100+
// completing the status patch in the next reconcile cycle.
101+
func newUnconfirmedReservation(name, targetHost, projectID, flavorGroup, cpu, memory string) *v1alpha1.Reservation {
102+
return &v1alpha1.Reservation{
103+
ObjectMeta: metav1.ObjectMeta{
104+
Name: name,
105+
},
106+
Spec: v1alpha1.ReservationSpec{
107+
Type: v1alpha1.ReservationTypeCommittedResource,
108+
TargetHost: targetHost,
109+
Resources: map[hv1.ResourceName]resource.Quantity{
110+
hv1.ResourceCPU: resource.MustParse(cpu),
111+
hv1.ResourceMemory: resource.MustParse(memory),
112+
},
113+
CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{
114+
ProjectID: projectID,
115+
ResourceGroup: flavorGroup,
116+
},
117+
},
118+
// No Status.Conditions, no Status.Host — Ready=false, cycle 2 not yet run.
119+
Status: v1alpha1.ReservationStatus{},
120+
}
121+
}
122+
97123
// newCommittedReservation creates a reservation where TargetHost and Status.Host are the same.
98124
func newCommittedReservation(
99125
name, host, projectID, flavorName, flavorGroup, cpu, memory string,
@@ -567,6 +593,57 @@ func TestFilterHasEnoughCapacity_ReservationTypes(t *testing.T) {
567593
expectedHosts: []string{"host1", "host2", "host3"},
568594
filteredHosts: []string{"host4"},
569595
},
596+
{
597+
// Regression test: unconfirmed reservation (TargetHost set, Ready=false, Status.Host="")
598+
// models the window between reconcile cycle 1 (TargetHost written) and cycle 2 (Ready synced).
599+
// The filter must block the target host even though IsReady() returns false.
600+
name: "Unconfirmed reservation (TargetHost set, not ready) blocks target host",
601+
reservations: []*v1alpha1.Reservation{
602+
newUnconfirmedReservation("unconfirmed-res", "host1", "project-X", "gp-1", "8", "16Gi"),
603+
},
604+
request: newNovaRequest("instance-123", "project-A", "m1.small", "gp-1", 4, "8Gi", false, []string{"host1", "host2", "host3", "host4"}),
605+
opts: FilterHasEnoughCapacityOpts{LockReserved: false},
606+
expectedHosts: []string{"host2", "host3"},
607+
filteredHosts: []string{"host1", "host4"},
608+
},
609+
{
610+
// Regression test: multiple unconfirmed reservations (cycle 1 done, not yet ready) targeting
611+
// different hosts must each block their respective target host.
612+
name: "Multiple unconfirmed reservations each block their target host",
613+
reservations: []*v1alpha1.Reservation{
614+
newUnconfirmedReservation("unconfirmed-1", "host1", "project-X", "gp-1", "8", "16Gi"),
615+
newUnconfirmedReservation("unconfirmed-2", "host2", "project-X", "gp-1", "4", "8Gi"),
616+
},
617+
request: newNovaRequest("instance-123", "project-A", "m1.small", "gp-1", 4, "8Gi", false, []string{"host1", "host2", "host3", "host4"}),
618+
opts: FilterHasEnoughCapacityOpts{LockReserved: false},
619+
expectedHosts: []string{"host3"},
620+
filteredHosts: []string{"host1", "host2", "host4"},
621+
},
622+
{
623+
// Regression test: unconfirmed failover reservation (TargetHost set, Ready=false)
624+
// must block the target host just like unconfirmed CR reservations.
625+
name: "Unconfirmed failover reservation (TargetHost set, not ready) blocks target host",
626+
reservations: []*v1alpha1.Reservation{
627+
{
628+
ObjectMeta: metav1.ObjectMeta{Name: "failover-unconfirmed"},
629+
Spec: v1alpha1.ReservationSpec{
630+
Type: v1alpha1.ReservationTypeFailover,
631+
TargetHost: "host1",
632+
Resources: map[hv1.ResourceName]resource.Quantity{
633+
hv1.ResourceCPU: resource.MustParse("8"),
634+
hv1.ResourceMemory: resource.MustParse("16Gi"),
635+
},
636+
FailoverReservation: &v1alpha1.FailoverReservationSpec{ResourceGroup: "gp-1"},
637+
},
638+
// No Status.Conditions, no Status.Host — Ready=false.
639+
Status: v1alpha1.ReservationStatus{},
640+
},
641+
},
642+
request: newNovaRequest("instance-123", "project-A", "m1.small", "gp-1", 4, "8Gi", false, []string{"host1", "host2", "host3", "host4"}),
643+
opts: FilterHasEnoughCapacityOpts{LockReserved: false},
644+
expectedHosts: []string{"host2", "host3"},
645+
filteredHosts: []string{"host1", "host4"},
646+
},
570647
}
571648

572649
for _, tt := range tests {

0 commit comments

Comments
 (0)