From 501052c1644d71ecb1862a75cb1c8ab5239e4d27 Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Tue, 3 Nov 2026 11:51:37 +0100 Subject: [PATCH 1/7] feat(nova): match failover host reservations inside scheduling pipeline Route evacuation requests onto failover host reservations from inside the scheduling pipeline, pinning a source host to a single target for gang- evacuation, and block ordinary placement onto those reserved hosts. - Add the failover host Matcher (reservations/failoverhost/matcher.go) that reroutes evacuations after the pipeline, under the pipeline controller's exclusive lock, writing the InUse condition when it pins a fresh host. - Wire the Matcher into the FilterWeigherPipelineController. - Add the filter_no_failover_host pipeline filter that blocks hosts with a failover host reservation for non-evacuation requests. Named to make clear it only avoids placing new workload onto failover hosts. Disabled by default until the failover host draining controller logic lands. - Give the Matcher its own small MatcherMonitor collector so the pipeline and failover-host-reservations controllers can both register without colliding on shared metric names. Signed-off-by: Philipp Matthes --- cmd/manager/main.go | 6 + .../04-failover-host-reservations.md | 58 ++++++ .../bundles/cortex-nova/templates/alerts.yaml | 27 +++ .../cortex-nova/templates/pipelines_kvm.yaml | 33 +++ helm/bundles/cortex-nova/values.yaml | 8 + .../filter_weigher_pipeline_controller.go | 33 +++ ...filter_weigher_pipeline_controller_test.go | 145 +++++++++++++ .../filters/filter_no_failover_host.go | 90 ++++++++ .../filters/filter_no_failover_host_test.go | 149 +++++++++++++ .../reservations/failoverhost/matcher.go | 159 ++++++++++++++ .../failoverhost/matcher_monitor.go | 48 +++++ .../reservations/failoverhost/matcher_test.go | 197 ++++++++++++++++++ 12 files changed, 953 insertions(+) create mode 100644 internal/scheduling/nova/plugins/filters/filter_no_failover_host.go create mode 100644 internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go create mode 100644 internal/scheduling/reservations/failoverhost/matcher.go create mode 100644 internal/scheduling/reservations/failoverhost/matcher_monitor.go create mode 100644 internal/scheduling/reservations/failoverhost/matcher_test.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 9ba24d770..3ce317c79 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -419,6 +419,8 @@ func main() { featureGates := conf.GetConfigOrDie[nova.FeatureGates]() noHostFoundCounter := crs.NewNoHostFoundCounter() placementCounter := crs.NewPlacementCounter() + failoverHostMatcherMonitor := failoverhost.NewMatcherMonitor() + metrics.Registry.MustRegister(failoverHostMatcherMonitor) // Filter-weigher pipeline controller setup. filterWeigherController := &nova.FilterWeigherPipelineController{ Monitor: filterWeigherPipelineMonitor, @@ -427,6 +429,10 @@ func main() { NoHostFoundCounter: noHostFoundCounter, PlacementCounter: placementCounter, }, + FailoverMatcher: &failoverhost.Matcher{ + Client: multiclusterClient, + Monitor: failoverHostMatcherMonitor, + }, } metrics.Registry.MustRegister(noHostFoundCounter) metrics.Registry.MustRegister(placementCounter) diff --git a/docs/03-reservations-and-inventory/04-failover-host-reservations.md b/docs/03-reservations-and-inventory/04-failover-host-reservations.md index 757ab26db..4e8c23a50 100644 --- a/docs/03-reservations-and-inventory/04-failover-host-reservations.md +++ b/docs/03-reservations-and-inventory/04-failover-host-reservations.md @@ -475,6 +475,64 @@ stateDiagram-v2 > independently rather than as a single current-state field, so the arrows show the normal path rather than the > only possible transitions. +## How failover host reservations affect scheduling + +Creating a reservation is only half the story — the scheduler has to *honor* it. Two distinct behaviors wire +failover host reservations into the Nova scheduling pipeline, split along a natural seam: one keeps new +workload *off* reserved hosts, the other steers a dead host's workload *onto* them. + +### Normal placement: reserved hosts are blocked + +For every non-evacuation request, the **`filter_no_failover_host`** pipeline filter removes any host that has a +failover host reservation from the candidate set, so no new VM is placed on a host that is being drained or +kept empty as a landing zone. The filter is purely *subtractive* — it only ever removes hosts — so its +position in the pipeline does not matter and all other filters and weighers still apply around it. It is +toggled by the `kvm.filterNoFailoverHost.enabled` Helm value (default `false`) and is a no-op when disabled. + +The filter also honors the **call-time** `ignoredReservationTypes` request option. This is what keeps the +**path finder probe** working: the probe asks the scheduler which hosts would accept a flavor, and it passes +`ignoredReservationTypes: [FailoverHostReservation]` precisely so a reserved host is *not* hidden from its own +partition. Because the filter obeys that option, the probe still discovers already-reserved hosts and the +partitioning does not fragment. + +### Evacuation: routing onto a failover host (gang-evacuation) + +Evacuations are **not** handled by the filter — it deliberately leaves evacuation requests untouched. +Instead, a dedicated **Matcher** runs *after* the filter/weigher pipeline has produced its ordered +candidates, from inside the pipeline's global execution lock. For an evacuation request (identified by the +`evacuate` intent) it reads the dead host from the `source_host` scheduler hint and: + +1. finds the failover host reservations whose `sourceHosts` include that source host; +2. if **none** match, it changes nothing — the request falls back to the ordinary candidate set so the VM + still has somewhere to go — and increments a metric + (`cortex_failover_host_evacuations_without_reservation_total`) that drives an alert on frequent + occurrences; +3. if one of them is already **In Use** *by this same source host*, it reuses that one — this is the + gang-evacuation path, so the whole gang lands on the same target; +4. otherwise it pins the first reservation that is *not* already in use, marking it **In Use** and recording + the source host on its status, then returns that reservation's host as the sole candidate. + +A reservation can legitimately rescue several source hosts (the planner lists every partition peer as a +source), so two hosts in the same partition can fail over concurrently, one reservation each. The Matcher +therefore never pins a reservation that is already **In Use** by a *different* source — that would steal a +landing zone actively absorbing another host's gang. If every matching reservation is already taken by other +sources, it falls back to the ordinary candidate set (and records the same metric as the no-reservation +case). + +Running inside the global pipeline lock is what makes **gang-evacuation** work. The *first* VM evacuated from +a dead host pins a target and writes `In Use` while holding the lock; every subsequent VM from the same +source host — serialized behind the same lock — sees that `In Use` reservation and lands on the *same* +target. Writing the pin *after* the pipeline (rather than inside the filter) avoids a dangling `In Use` +reservation if a later step fails, and keeping it under the lock guarantees the next request observes it. + +As a safety mechanism, if the chosen reserved host is not among the pipeline's candidates (the reservation +conflicts with the request's other scheduling constraints), the Matcher falls back to the original candidate +set rather than returning an empty one, so the VM is never left with nowhere to schedule. + +The Matcher only ever advances a reservation *to* `In Use`. Moving it back out — to `Active` once the +evacuation completes, or on cleanup — stays with the failover host lifecycle reconciler, based on the real +workload present on the host. + ## Configuration Enable the controller and configure it under the nested `failover.hosts` key: diff --git a/helm/bundles/cortex-nova/templates/alerts.yaml b/helm/bundles/cortex-nova/templates/alerts.yaml index bb82e889a..43605b36b 100644 --- a/helm/bundles/cortex-nova/templates/alerts.yaml +++ b/helm/bundles/cortex-nova/templates/alerts.yaml @@ -1133,5 +1133,32 @@ spec: path finder are failing (transport errors or non-2xx responses). The path finder cannot determine valid evacuation targets, so failover partitions and reservations cannot be recomputed. Check that the external scheduler is reachable and healthy. + - alert: CortexNovaFailoverHostEvacuationsWithoutReservation + # An evacuation came in whose source host matched no failover host + # reservation, so the request fell back to ordinary candidates instead of a + # pre-validated, gang-evacuable landing zone. Frequent occurrences mean the + # partitioning is not keeping failover headroom where failures actually + # happen (e.g. the source host's partition reserves nothing, or the host + # left its partition), so evacuations are re-solved under pressure. + expr: sum by (az) (increase(cortex_failover_host_evacuations_without_reservation_total{service="cortex-nova-metrics"}[15m])) > 0 + for: 15m + labels: +{{- if .Values.alerts.persesDashboardBaseURL }} + persesDashboard: "{{ .Values.alerts.persesDashboardBaseURL }}/cortex_status?var-service=cortex-nova-metrics" +{{- end }} + context: failover-hosts + service: cortex + severity: warning + support_group: workload-management + playbook: "" + annotations: + summary: "Evacuations landing without a failover host reservation in `{{ "{{" }} $labels.az {{ "}}" }}`" + description: > + Evacuation scheduling requests in `{{ "{{" }} $labels.az {{ "}}" }}` are matching no + failover host reservation for their source host, so they fall back to the ordinary + candidate set rather than a pre-validated, gang-evacuable landing zone. This means + the source hosts have no reserved failover target — check whether their partition + reserves anything (a partition of fewer than two hosts reserves nothing) and the + failover host calculation loop is healthy. {{- end }} {{- end }} diff --git a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml index 817e75ab6..568475805 100644 --- a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml +++ b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml @@ -84,6 +84,17 @@ spec: # FailoverHostReservation is a new feature and is not yet counted as # reserved capacity during scheduling. - {key: ignoredReservationTypes, stringListValue: ["FailoverHostReservation"]} + - name: filter_no_failover_host + description: | + This step blocks hosts that have a failover host reservation, so no new + virtual machines are placed on a host being drained or kept empty as a + gang-evacuation landing zone. It only applies to non-evacuation requests; + evacuations are routed onto reserved hosts after the pipeline. The filter + is purely subtractive, so its position does not matter. It honors the + call-time ignoredReservationTypes option so the failover host path finder + probe still discovers reserved hosts. + params: + - {key: enabled, boolValue: {{ .Values.kvm.filterNoFailoverHost.enabled }}} - name: filter_allowed_projects description: | This step filters hosts based on allowed projects defined in the @@ -258,6 +269,17 @@ spec: # FailoverHostReservation is a new feature and is not yet counted as # reserved capacity during scheduling. - {key: ignoredReservationTypes, stringListValue: ["FailoverHostReservation"]} + - name: filter_no_failover_host + description: | + This step blocks hosts that have a failover host reservation, so no new + virtual machines are placed on a host being drained or kept empty as a + gang-evacuation landing zone. It only applies to non-evacuation requests; + evacuations are routed onto reserved hosts after the pipeline. The filter + is purely subtractive, so its position does not matter. It honors the + call-time ignoredReservationTypes option so the failover host path finder + probe still discovers reserved hosts. + params: + - {key: enabled, boolValue: {{ .Values.kvm.filterNoFailoverHost.enabled }}} - name: filter_allowed_projects description: | This step filters hosts based on allowed projects defined in the @@ -582,6 +604,17 @@ spec: # FailoverHostReservation is a new feature and is not yet counted as # reserved capacity during scheduling. - {key: ignoredReservationTypes, stringListValue: ["FailoverHostReservation"]} + - name: filter_no_failover_host + description: | + This step blocks hosts that have a failover host reservation, so no new + virtual machines are placed on a host being drained or kept empty as a + gang-evacuation landing zone. It only applies to non-evacuation requests; + evacuations are routed onto reserved hosts after the pipeline. The filter + is purely subtractive, so its position does not matter. It honors the + call-time ignoredReservationTypes option so the failover host path finder + probe still discovers reserved hosts. + params: + - {key: enabled, boolValue: {{ .Values.kvm.filterNoFailoverHost.enabled }}} - name: filter_has_requested_traits description: | This step filters hosts that do not have the requested traits given by the diff --git a/helm/bundles/cortex-nova/values.yaml b/helm/bundles/cortex-nova/values.yaml index 755bb8f42..e002b86bc 100644 --- a/helm/bundles/cortex-nova/values.yaml +++ b/helm/bundles/cortex-nova/values.yaml @@ -109,6 +109,14 @@ kvm: # Domain names that are explicitly treated as non-external-customer domains and # skipped by the filter, even if they match one of the prefixes above. ignoredDomainNames: [] + # Options for the filter_no_failover_host pipeline step, which blocks hosts with a + # failover host reservation so no new VMs land on a host being kept empty as a + # gang-evacuation landing zone. Applied to every KVM pipeline that uses the step. + filterNoFailoverHost: + # Disabled by default until the failover host draining controller logic is + # implemented. Set to true to block hosts with a failover host reservation + # from ordinary placement. + enabled: false cortex: &cortex crd: {enable: false} diff --git a/internal/scheduling/nova/filter_weigher_pipeline_controller.go b/internal/scheduling/nova/filter_weigher_pipeline_controller.go index a81074072..f92809731 100644 --- a/internal/scheduling/nova/filter_weigher_pipeline_controller.go +++ b/internal/scheduling/nova/filter_weigher_pipeline_controller.go @@ -56,6 +56,21 @@ type FilterWeigherPipelineController struct { // CRRecorder receives scheduling events and updates CR reservations and metrics. CRRecorder crs.Recorder + + // FailoverMatcher routes evacuation requests onto failover host reservations + // after the pipeline runs, inside the exclusive lock. Optional; when nil, no + // failover host rerouting is applied. + FailoverMatcher FailoverHostMatcher +} + +// FailoverHostMatcher reroutes an evacuation onto a failover host reservation. +// Implemented by reservations/failoverhost.Matcher; declared here as an +// interface so the nova package does not import failoverhost (which would form +// an import cycle via that package's integration tests). +type FailoverHostMatcher interface { + // Match returns the candidate hosts to use, possibly rewritten to the chosen + // failover host, or the originals unchanged when nothing applies. + Match(ctx context.Context, request api.ExternalSchedulerRequest, orig []string) ([]string, error) } // The type of pipeline this controller manages. @@ -177,6 +192,24 @@ func (c *FilterWeigherPipelineController) process(ctx context.Context, decision result, err := pipeline.Run(request) if err == nil { + // Route evacuations onto failover host reservations and pin the + // source->target relationship. This runs inside the exclusive lock (write + // runs only) so the InUse write is visible to subsequent evacuations from + // the same source, implementing gang-evacuation. Read-only runs (probes, + // capacity checks) never reroute or write. + if c.FailoverMatcher != nil && !request.Options.ReadOnly { + hosts, mErr := c.FailoverMatcher.Match(ctx, request, result.OrderedHosts) + if mErr != nil { + log.Error(mErr, "failover host matcher failed") + return &request, mErr + } + result.OrderedHosts = hosts + if len(hosts) > 0 { + result.TargetHost = &hosts[0] + } else { + result.TargetHost = nil + } + } decision.Status.Result = &result } // Record a human-readable explanation of the decision so it propagates to diff --git a/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go b/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go index f7c4f5a41..31542cdfb 100644 --- a/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go +++ b/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go @@ -961,3 +961,148 @@ func TestFilterWeigherPipelineController_PeekReadOnly(t *testing.T) { }) } } + +// fakeFailoverMatcher records the candidates it was handed and returns a +// preconfigured result, so controller wiring can be asserted in isolation. +type fakeFailoverMatcher struct { + called bool + gotHosts []string + ret []string + err error +} + +func (f *fakeFailoverMatcher) Match(_ context.Context, _ api.ExternalSchedulerRequest, orig []string) ([]string, error) { + f.called = true + f.gotHosts = orig + if f.err != nil { + return nil, f.err + } + return f.ret, nil +} + +// runProcessWithMatcher builds a controller with an empty pipeline and the given +// matcher, runs the decision through ProcessNewDecisionFromAPI, and returns the +// decision and any error. The request's weights make the pipeline order the two +// hosts [compute-1, compute-2]. +func runProcessWithMatcher(t *testing.T, matcher FailoverHostMatcher, readOnly bool) (*v1alpha1.Decision, error) { + t.Helper() + scheme := runtime.NewScheme() + if err := v1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("add v1alpha1 scheme: %v", err) + } + + req := api.ExternalSchedulerRequest{ + Spec: api.NovaObject[api.NovaSpec]{Data: api.NovaSpec{InstanceUUID: "vm-1", NumInstances: 1}}, + Hosts: []api.ExternalSchedulerHost{{ComputeHost: "compute-1"}, {ComputeHost: "compute-2"}}, + Weights: map[string]float64{"compute-1": 1.0, "compute-2": 0.5}, + Pipeline: "p", + } + if readOnly { + // ReadOnly runs must also skip inflight and CR tracking, else Options.Validate rejects them. + req.Options = scheduling.Options{ReadOnly: true, SkipInflight: true, SkipCommittedResourceTracking: true} + } + raw, err := json.Marshal(req) + if err != nil { + t.Fatalf("marshal request: %v", err) + } + pipeline := &v1alpha1.Pipeline{ + ObjectMeta: metav1.ObjectMeta{Name: "p"}, + Spec: v1alpha1.PipelineSpec{ + Type: v1alpha1.PipelineTypeFilterWeigher, + SchedulingDomain: v1alpha1.SchedulingDomainNova, + }, + } + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(pipeline). + WithStatusSubresource(&v1alpha1.Decision{}). + Build() + controller := &FilterWeigherPipelineController{ + BasePipelineController: lib.BasePipelineController[lib.FilterWeigherPipeline[api.ExternalSchedulerRequest]]{ + Client: fakeClient, + Pipelines: make(map[string]lib.FilterWeigherPipeline[api.ExternalSchedulerRequest]), + PipelineConfigs: make(map[string]v1alpha1.Pipeline), + }, + Monitor: lib.FilterWeigherPipelineMonitor{}, + FailoverMatcher: matcher, + } + controller.PipelineConfigs["p"] = *pipeline + initResult := controller.InitPipeline(context.Background(), *pipeline) + if len(initResult.FilterErrors) > 0 || len(initResult.WeigherErrors) > 0 { + t.Fatalf("init pipeline: %v %v", initResult.FilterErrors, initResult.WeigherErrors) + } + controller.Pipelines["p"] = initResult.Pipeline + + decision := &v1alpha1.Decision{ + ObjectMeta: metav1.ObjectMeta{Name: "d", Namespace: "default"}, + Spec: v1alpha1.DecisionSpec{ + SchedulingDomain: v1alpha1.SchedulingDomainNova, + PipelineRef: corev1.ObjectReference{Name: "p"}, + NovaRaw: &runtime.RawExtension{Raw: raw}, + }, + } + return decision, controller.ProcessNewDecisionFromAPI(context.Background(), decision) +} + +func TestFilterWeigherPipelineController_FailoverMatcherRewritesResult(t *testing.T) { + m := &fakeFailoverMatcher{ret: []string{"compute-2"}} + decision, err := runProcessWithMatcher(t, m, false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !m.called { + t.Fatal("expected matcher to be called") + } + // Matcher receives the pipeline's ordered hosts. + if len(m.gotHosts) != 2 || m.gotHosts[0] != "compute-1" || m.gotHosts[1] != "compute-2" { + t.Errorf("matcher got unexpected hosts: %v", m.gotHosts) + } + // Its returned hosts become the result, with TargetHost pointing at the first. + if decision.Status.Result == nil { + t.Fatal("expected result to be set") + } + if got := decision.Status.Result.OrderedHosts; len(got) != 1 || got[0] != "compute-2" { + t.Errorf("expected OrderedHosts [compute-2], got %v", got) + } + if decision.Status.Result.TargetHost == nil || *decision.Status.Result.TargetHost != "compute-2" { + t.Errorf("expected TargetHost compute-2, got %v", decision.Status.Result.TargetHost) + } +} + +func TestFilterWeigherPipelineController_FailoverMatcherSkippedOnReadOnly(t *testing.T) { + m := &fakeFailoverMatcher{ret: []string{"compute-2"}} + decision, err := runProcessWithMatcher(t, m, true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if m.called { + t.Error("expected matcher NOT to be called on a read-only run") + } + // The pipeline's original ordering is preserved. + if got := decision.Status.Result.OrderedHosts; len(got) != 2 || got[0] != "compute-1" { + t.Errorf("expected original ordering, got %v", got) + } +} + +func TestFilterWeigherPipelineController_FailoverMatcherErrorFailsDecision(t *testing.T) { + m := &fakeFailoverMatcher{err: errors.New("patch failed")} + decision, err := runProcessWithMatcher(t, m, false) + if err == nil || !strings.Contains(err.Error(), "patch failed") { + t.Fatalf("expected matcher error to propagate, got %v", err) + } + // On matcher failure the decision carries no result. + if decision.Status.Result != nil { + t.Errorf("expected no result on matcher error, got %v", decision.Status.Result) + } +} + +func TestFilterWeigherPipelineController_FailoverMatcherEmptyClearsTargetHost(t *testing.T) { + m := &fakeFailoverMatcher{ret: []string{}} + decision, err := runProcessWithMatcher(t, m, false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if decision.Status.Result.TargetHost != nil { + t.Errorf("expected nil TargetHost for empty host list, got %v", *decision.Status.Result.TargetHost) + } +} diff --git a/internal/scheduling/nova/plugins/filters/filter_no_failover_host.go b/internal/scheduling/nova/plugins/filters/filter_no_failover_host.go new file mode 100644 index 000000000..a6e9b535a --- /dev/null +++ b/internal/scheduling/nova/plugins/filters/filter_no_failover_host.go @@ -0,0 +1,90 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package filters + +import ( + "context" + "slices" + + "github.com/go-logr/logr" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "github.com/cobaltcore-dev/cortex/internal/scheduling/lib" +) + +// FilterNoFailoverHostOpts configures the no-failover-host filter. +type FilterNoFailoverHostOpts struct { + // Enabled toggles the filter. When false, the filter is a no-op and all + // hosts pass. This lets the pipeline carry the step while keeping the + // behavior off until the feature is rolled out. + Enabled bool `json:"enabled"` +} + +func (FilterNoFailoverHostOpts) Validate() error { return nil } + +// FilterNoFailoverHost blocks hosts that have a failover host reservation, so no +// new virtual machines are placed on a host that is being drained or kept empty +// as a gang-evacuation landing zone. +// +// This only applies to non-evacuation requests. Evacuation requests are routed +// onto reserved hosts separately, after the pipeline, by the failover host +// Matcher (see reservations/failoverhost/matcher.go), so this filter leaves them +// untouched. +// +// The filter is purely subtractive, so its position in the pipeline does not +// matter: other filters and weighers still run around it. +type FilterNoFailoverHost struct { + lib.BaseFilter[api.ExternalSchedulerRequest, FilterNoFailoverHostOpts] +} + +func (s *FilterNoFailoverHost) Run(traceLog logr.Logger, request api.ExternalSchedulerRequest) (*lib.FilterWeigherPipelineStepResult, error) { + result := s.IncludeAllHostsFromRequest(request) + if !s.Options.Enabled { + return result, nil + } + + // Honor the call-time ignored reservation types. When failover host + // reservations are ignored — as the path finder probe does (see + // reservations/failoverhost/pathfinding.go) — block nothing so reserved + // hosts stay visible to their own partition. + if slices.Contains(request.GetOptions().IgnoredReservationTypes, v1alpha1.ReservationTypeFailoverHost) { + traceLog.V(1).Info("failover host reservations ignored, not blocking any hosts") + return result, nil + } + + // Evacuations are routed onto reserved hosts by the Matcher, not blocked here. + if intent, err := request.GetIntent(); err == nil && intent == api.EvacuateIntent { + traceLog.V(1).Info("evacuation request, leaving hosts untouched for the failover host matcher") + return result, nil + } + + var reservations v1alpha1.ReservationList + if err := s.Client.List(context.Background(), &reservations); err != nil { + return nil, err + } + for _, reservation := range reservations.Items { + if reservation.Spec.Type != v1alpha1.ReservationTypeFailoverHost { + continue + } + // Block both the desired (TargetHost) and actual (Status.Host) host, as + // the capacity filter does, to cover the window while the reservation is + // being placed. A reservation only exists from the Designated stage + // onward (the controller deletes Revoked/Used ones), so its presence is + // enough to keep new virtual machines off the host. + for _, host := range []string{reservation.Spec.TargetHost, reservation.Status.Host} { + if host == "" { + continue + } + delete(result.Activations, host) // noop if host is not in the map + traceLog.V(1).Info("filtering out failover host reservation target", + "host", host, "reservation", reservation.Name) + } + } + return result, nil +} + +func init() { + Index["filter_no_failover_host"] = func() NovaFilter { return &FilterNoFailoverHost{} } +} diff --git a/internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go b/internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go new file mode 100644 index 000000000..c1f343805 --- /dev/null +++ b/internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go @@ -0,0 +1,149 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package filters + +import ( + "testing" + + "github.com/go-logr/logr" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/scheduling" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func failoverHostReservation(targetHost, statusHost string) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeFailoverHost, + TargetHost: targetHost, + FailoverHostReservation: &v1alpha1.FailoverHostReservationSpec{}, + }, + Status: v1alpha1.ReservationStatus{Host: statusHost}, + } +} + +func committedResourceReservationOn(targetHost string) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "cr-" + targetHost}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: targetHost, + }, + } +} + +func requestWithHosts(hints map[string]any, hosts ...string) api.ExternalSchedulerRequest { + hvHosts := make([]api.ExternalSchedulerHost, len(hosts)) + for i, h := range hosts { + hvHosts[i] = api.ExternalSchedulerHost{ComputeHost: h} + } + return api.ExternalSchedulerRequest{ + Hosts: hvHosts, + Spec: api.NovaObject[api.NovaSpec]{Data: api.NovaSpec{SchedulerHints: hints}}, + } +} + +func TestFilterNoFailoverHost_Run(t *testing.T) { + scheme := buildTestScheme(t) + + tests := []struct { + name string + opts FilterNoFailoverHostOpts + reservations []client.Object + request api.ExternalSchedulerRequest + expectedHosts []string + }{ + { + name: "disabled - all hosts pass even with reservations", + opts: FilterNoFailoverHostOpts{Enabled: false}, + reservations: []client.Object{failoverHostReservation("host2", "")}, + request: requestWithHosts(nil, "host1", "host2", "host3"), + expectedHosts: []string{"host1", "host2", "host3"}, + }, + { + name: "enabled - blocks reserved target host", + opts: FilterNoFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("host2", "")}, + request: requestWithHosts(nil, "host1", "host2", "host3"), + expectedHosts: []string{"host1", "host3"}, + }, + { + name: "enabled - blocks both target and status host", + opts: FilterNoFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("host3", "host4")}, + request: requestWithHosts(nil, "host1", "host2", "host3", "host4"), + expectedHosts: []string{"host1", "host2"}, + }, + { + name: "enabled - ignores non-failover-host reservations", + opts: FilterNoFailoverHostOpts{Enabled: true}, + reservations: []client.Object{committedResourceReservationOn("host2")}, + request: requestWithHosts(nil, "host1", "host2", "host3"), + expectedHosts: []string{"host1", "host2", "host3"}, + }, + { + name: "evacuation request - leaves hosts untouched for the matcher", + opts: FilterNoFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("host2", "")}, + request: requestWithHosts(map[string]any{"_nova_check_type": "evacuate"}, "host1", "host2", "host3"), + expectedHosts: []string{"host1", "host2", "host3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + step := &FilterNoFailoverHost{} + step.Options = tt.opts + step.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.reservations...).Build() + + result, err := step.Run(logr.Discard(), tt.request) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(result.Activations) != len(tt.expectedHosts) { + t.Errorf("expected %d hosts, got %d: %v", len(tt.expectedHosts), len(result.Activations), result.Activations) + } + for _, host := range tt.expectedHosts { + if _, ok := result.Activations[host]; !ok { + t.Errorf("expected host %s to be present", host) + } + } + }) + } +} + +func TestFilterNoFailoverHost_CallTimeIgnoredReservationTypes(t *testing.T) { + scheme := buildTestScheme(t) + step := &FilterNoFailoverHost{} + step.Options = FilterNoFailoverHostOpts{Enabled: true} + step.Client = fake.NewClientBuilder().WithScheme(scheme). + WithObjects(failoverHostReservation("host2", "")).Build() + + request := requestWithHosts(nil, "host1", "host2", "host3") + request.Options = scheduling.Options{ + IgnoredReservationTypes: []v1alpha1.ReservationType{v1alpha1.ReservationTypeFailoverHost}, + } + result, err := step.Run(logr.Discard(), request) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if _, ok := result.Activations["host2"]; !ok { + t.Error("expected host2 to survive when failover host reservations are ignored at call time") + } +} + +func TestFilterNoFailoverHost_Index(t *testing.T) { + factory, ok := Index["filter_no_failover_host"] + if !ok { + t.Fatal("expected filter_no_failover_host to be registered in Index") + } + if _, ok := factory().(*FilterNoFailoverHost); !ok { + t.Fatal("expected factory to return a *FilterNoFailoverHost") + } +} diff --git a/internal/scheduling/reservations/failoverhost/matcher.go b/internal/scheduling/reservations/failoverhost/matcher.go new file mode 100644 index 000000000..c6aa719f6 --- /dev/null +++ b/internal/scheduling/reservations/failoverhost/matcher.go @@ -0,0 +1,159 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package failoverhost + +import ( + "context" + "slices" + "sort" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + logf "sigs.k8s.io/controller-runtime/pkg/log" + + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// Matcher routes an evacuation request onto a failover host reservation and +// pins the source host to a single target, implementing gang-evacuation. +// +// It is called after the scheduling pipeline has produced its ordered +// candidates, from inside the pipeline controller's exclusive lock, so that the +// InUse status write below is serialized against — and visible to — subsequent +// evacuation requests from the same source host. The write is committed only +// once (on the first evacuation from a given source); later requests reuse the +// already-InUse reservation and so land on the same target. +// +// The Matcher only ever advances a reservation to InUse. Moving it back out +// (to Active/Used once the evacuation completes) is owned by the failover host +// lifecycle reconciler, based on the real workload present on the host. +type Matcher struct { + client.Client + // Monitor records the evacuation-without-reservation metric. May be nil. + Monitor *MatcherMonitor +} + +// Match returns the candidate hosts to use for this request. For a +// non-evacuation request, or an evacuation the Matcher cannot or should not +// reroute, it returns orig unchanged. For an evacuation it finds the failover +// host reservations whose SourceHosts include the request's source host and +// returns the chosen failover host as the sole candidate, writing the InUse +// condition when it pins a fresh one. +// +// Fallbacks preserve a valid placement: if no reservation matches, or the +// chosen host is not among orig (so it conflicts with other scheduling +// constraints), orig is returned unchanged so the VM still has candidates. +func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerRequest, orig []string) ([]string, error) { + log := logf.FromContext(ctx) + + intent, err := request.GetIntent() + if err != nil || intent != api.EvacuateIntent { + return orig, nil //nolint:nilerr // non-evacuation requests are not rerouted + } + sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host") + if err != nil || sourceHost == "" { + log.V(1).Info("no source_host hint on evacuation, leaving candidates untouched") + return orig, nil //nolint:nilerr // without a source host there is nothing to match + } + + var reservations v1alpha1.ReservationList + if err := m.List(ctx, &reservations); err != nil { + return nil, err + } + // Collect the failover host reservations whose partition includes the source + // host, i.e. the reservations this host may evacuate onto. + var matching []v1alpha1.Reservation + for _, res := range reservations.Items { + if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { + continue + } + if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { + matching = append(matching, res) + } + } + if len(matching) == 0 { + log.V(1).Info("no failover host reservation for source host, falling back to ordinary candidates", + "sourceHost", sourceHost) + m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) + return orig, nil + } + + // Sort deterministically by target host so selection is stable and testable; + // the InUse write below is what makes a fresh choice binding for subsequent + // requests from the same source. + sort.Slice(matching, func(i, j int) bool { + return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost + }) + + // Classify each candidate reservation by its InUse status. A reservation can + // legitimately rescue many source hosts (the planner lists every partition + // peer), so more than one source may evacuate concurrently: one reservation + // per source. We must therefore (a) reuse the reservation already in use by + // *this* source, and (b) never pin a reservation already in use by a + // *different* source, which would steal an active landing zone. + var free *v1alpha1.Reservation + for i := range matching { + res := &matching[i] + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + if free == nil { + free = res + } + continue + } + // In use: reuse it only if it is in use by this very source host, which + // is the gang-evacuation path — the whole gang lands on the same target. + if res.Status.FailoverHostReservation != nil && + res.Status.FailoverHostReservation.SourceHost == sourceHost { + log.V(1).Info("reusing in-use failover host reservation for gang-evacuation", + "sourceHost", sourceHost, "reservation", res.Name, "targetHost", res.Spec.TargetHost) + return m.pick(*res, orig), nil + } + } + + // None already in use by this source. Pin the first free reservation. + if free == nil { + // Every matching reservation is already absorbing another source host's + // gang. Fall back to ordinary candidates rather than stealing a landing + // zone, and surface it like the no-reservation case. + log.V(1).Info("all failover host reservations for source host are in use by other sources, falling back", + "sourceHost", sourceHost) + m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) + return orig, nil + } + if err := m.markInUse(ctx, free, sourceHost); err != nil { + return nil, err + } + log.V(1).Info("pinned failover host reservation for evacuation", + "sourceHost", sourceHost, "reservation", free.Name, "targetHost", free.Spec.TargetHost) + return m.pick(*free, orig), nil +} + +// pick returns the reservation's target host as the sole candidate, unless that +// host is not among the pipeline's candidates (it conflicts with other +// scheduling constraints), in which case it falls back to the original set. +func (m *Matcher) pick(res v1alpha1.Reservation, orig []string) []string { + if slices.Contains(orig, res.Spec.TargetHost) { + return []string{res.Spec.TargetHost} + } + return orig +} + +// markInUse sets the InUse condition and records the source host on the +// reservation status. +func (m *Matcher) markInUse(ctx context.Context, res *v1alpha1.Reservation, sourceHost string) error { + old := res.DeepCopy() + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostInUse, + Status: metav1.ConditionTrue, + Reason: "EvacuationStarted", + Message: "a gang evacuation from " + sourceHost + " is underway", + }) + if res.Status.FailoverHostReservation == nil { + res.Status.FailoverHostReservation = &v1alpha1.FailoverHostReservationStatus{} + } + res.Status.FailoverHostReservation.SourceHost = sourceHost + return m.Status().Patch(ctx, res, client.MergeFrom(old)) +} diff --git a/internal/scheduling/reservations/failoverhost/matcher_monitor.go b/internal/scheduling/reservations/failoverhost/matcher_monitor.go new file mode 100644 index 000000000..5ec35a99e --- /dev/null +++ b/internal/scheduling/reservations/failoverhost/matcher_monitor.go @@ -0,0 +1,48 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package failoverhost + +import ( + "github.com/prometheus/client_golang/prometheus" +) + +// MatcherMonitor is the small Prometheus collector owned by the scheduling +// pipeline's failover host Matcher. It is kept separate from the failover host +// controller's Monitor so both can be registered independently: the pipeline +// controller and the failover-host-reservations-controller can be enabled at +// once without their collectors colliding on shared metric names. +type MatcherMonitor struct { + evacuationsWithoutReservation *prometheus.CounterVec +} + +// NewMatcherMonitor creates a new matcher monitor with Prometheus metrics. +func NewMatcherMonitor() *MatcherMonitor { + return &MatcherMonitor{ + evacuationsWithoutReservation: prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "cortex_failover_host_evacuations_without_reservation_total", + Help: "Total number of evacuation scheduling requests for which no failover host reservation matched the source host, by availability zone", + }, []string{"az"}), + } +} + +// RecordEvacuationWithoutReservation records an evacuation scheduling request +// whose source host matched no failover host reservation, so the request fell +// back to the ordinary candidate set. az is the request's availability zone +// (may be empty if unknown). +func (m *MatcherMonitor) RecordEvacuationWithoutReservation(az string) { + if m == nil { + return + } + m.evacuationsWithoutReservation.WithLabelValues(az).Inc() +} + +// Describe implements prometheus.Collector. +func (m *MatcherMonitor) Describe(ch chan<- *prometheus.Desc) { + m.evacuationsWithoutReservation.Describe(ch) +} + +// Collect implements prometheus.Collector. +func (m *MatcherMonitor) Collect(ch chan<- prometheus.Metric) { + m.evacuationsWithoutReservation.Collect(ch) +} diff --git a/internal/scheduling/reservations/failoverhost/matcher_test.go b/internal/scheduling/reservations/failoverhost/matcher_test.go new file mode 100644 index 000000000..a60ab7085 --- /dev/null +++ b/internal/scheduling/reservations/failoverhost/matcher_test.go @@ -0,0 +1,197 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package failoverhost + +import ( + "context" + "slices" + "testing" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// matcherReservation builds a failover host reservation targeting targetHost +// and able to rescue the given source hosts. +func matcherReservation(targetHost string, sourceHosts ...string) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeFailoverHost, + TargetHost: targetHost, + FailoverHostReservation: &v1alpha1.FailoverHostReservationSpec{ + SourceHosts: sourceHosts, + }, + }, + } +} + +// inUse marks a reservation as in use by the given source host. +func inUse(res *v1alpha1.Reservation, sourceHost string) *v1alpha1.Reservation { + res.Status.FailoverHostReservation = &v1alpha1.FailoverHostReservationStatus{SourceHost: sourceHost} + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostInUse, + Status: metav1.ConditionTrue, + Reason: "EvacuationStarted", + }) + return res +} + +func evacuationRequest(sourceHost string) api.ExternalSchedulerRequest { + hints := map[string]any{"_nova_check_type": "evacuate"} + if sourceHost != "" { + hints["source_host"] = sourceHost + } + return api.ExternalSchedulerRequest{ + Spec: api.NovaObject[api.NovaSpec]{Data: api.NovaSpec{ + AvailabilityZone: "az1", + SchedulerHints: hints, + }}, + } +} + +func getReservation(t *testing.T, c client.Client) v1alpha1.Reservation { + t.Helper() + var res v1alpha1.Reservation + if err := c.Get(context.Background(), types.NamespacedName{Name: "failover-host-target1"}, &res); err != nil { + t.Fatalf("failed to get reservation: %v", err) + } + return res +} + +func TestMatcher_Match(t *testing.T) { + tests := []struct { + name string + reservations []*v1alpha1.Reservation + request api.ExternalSchedulerRequest + orig []string + want []string + // check runs after Match to assert reservation state in the client. + check func(t *testing.T, c client.Client) + }{ + { + name: "non-evacuation is a no-op", + reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, + request: api.ExternalSchedulerRequest{ + Spec: api.NovaObject[api.NovaSpec]{Data: api.NovaSpec{ + SchedulerHints: map[string]any{"_nova_check_type": "live_migrate", "source_host": "source1"}, + }}, + }, + orig: []string{"host1", "host2"}, + want: []string{"host1", "host2"}, + }, + { + name: "evacuation without source hint is a no-op", + reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, + request: evacuationRequest(""), + orig: []string{"host1", "host2"}, + want: []string{"host1", "host2"}, + }, + { + name: "no matching reservation falls back to original candidates", + reservations: []*v1alpha1.Reservation{matcherReservation("target1", "otherSource")}, + request: evacuationRequest("source1"), + orig: []string{"host1", "host2"}, + want: []string{"host1", "host2"}, + }, + { + name: "pins a fresh reservation and writes InUse", + reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, + request: evacuationRequest("source1"), + orig: []string{"host1", "target1", "host2"}, + want: []string{"target1"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c) + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + t.Error("expected InUse condition to be True") + } + if res.Status.FailoverHostReservation == nil || res.Status.FailoverHostReservation.SourceHost != "source1" { + t.Errorf("expected status SourceHost=source1, got %+v", res.Status.FailoverHostReservation) + } + }, + }, + { + name: "reuses the in-use reservation for the same source", + // A second, not-in-use reservation (target0 sorts first) must be + // ignored in favour of the one already in use by this source. + reservations: []*v1alpha1.Reservation{ + inUse(matcherReservation("target1", "source1"), "source1"), + matcherReservation("target0", "source1"), + }, + request: evacuationRequest("source1"), + orig: []string{"target0", "target1", "host2"}, + want: []string{"target1"}, + }, + { + name: "does not steal a reservation in use by another source", + // source2 evacuates while source1 already holds target1, so source2 + // must pin the free target2 and leave target1's pin intact. + reservations: []*v1alpha1.Reservation{ + inUse(matcherReservation("target1", "source1", "source2"), "source1"), + matcherReservation("target2", "source1", "source2"), + }, + request: evacuationRequest("source2"), + orig: []string{"target1", "target2", "host3"}, + want: []string{"target2"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c) + if res.Status.FailoverHostReservation == nil || res.Status.FailoverHostReservation.SourceHost != "source1" { + t.Errorf("expected target1 to stay pinned to source1, got %+v", res.Status.FailoverHostReservation) + } + }, + }, + { + name: "all reservations in use by others falls back", + reservations: []*v1alpha1.Reservation{ + inUse(matcherReservation("target1", "source1", "source2"), "source1"), + }, + request: evacuationRequest("source2"), + orig: []string{"target1", "host3"}, + want: []string{"target1", "host3"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c) + if res.Status.FailoverHostReservation.SourceHost != "source1" { + t.Errorf("expected target1 to stay pinned to source1, got %q", res.Status.FailoverHostReservation.SourceHost) + } + }, + }, + { + name: "chosen host not in candidates falls back but still pins", + reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, + request: evacuationRequest("source1"), + orig: []string{"host1", "host2"}, // target1 absent: conflicts with other constraints + want: []string{"host1", "host2"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c) + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + t.Error("expected InUse condition to be True even on candidate fallback") + } + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objs := make([]client.Object, len(tt.reservations)) + for i, r := range tt.reservations { + objs[i] = r + } + c := newTestClient(t, objs...) + m := &Matcher{Client: c, Monitor: NewMatcherMonitor()} + got, err := m.Match(context.Background(), tt.request, tt.orig) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !slices.Equal(got, tt.want) { + t.Fatalf("got %v, want %v", got, tt.want) + } + if tt.check != nil { + tt.check(t, c) + } + }) + } +} From 83e2593f7eb610574f615769d7221c7d4fd1ff53 Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Tue, 3 Nov 2026 14:23:28 +0100 Subject: [PATCH 2/7] refactor(nova): rename Match parameter orig to originalHosts Signed-off-by: Philipp Matthes --- .../nova/filter_weigher_pipeline_controller.go | 2 +- .../filter_weigher_pipeline_controller_test.go | 4 ++-- .../reservations/failoverhost/matcher.go | 14 +++++++------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/scheduling/nova/filter_weigher_pipeline_controller.go b/internal/scheduling/nova/filter_weigher_pipeline_controller.go index f92809731..894b19670 100644 --- a/internal/scheduling/nova/filter_weigher_pipeline_controller.go +++ b/internal/scheduling/nova/filter_weigher_pipeline_controller.go @@ -70,7 +70,7 @@ type FilterWeigherPipelineController struct { type FailoverHostMatcher interface { // Match returns the candidate hosts to use, possibly rewritten to the chosen // failover host, or the originals unchanged when nothing applies. - Match(ctx context.Context, request api.ExternalSchedulerRequest, orig []string) ([]string, error) + Match(ctx context.Context, request api.ExternalSchedulerRequest, originalHosts []string) ([]string, error) } // The type of pipeline this controller manages. diff --git a/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go b/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go index 31542cdfb..9d514833b 100644 --- a/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go +++ b/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go @@ -971,9 +971,9 @@ type fakeFailoverMatcher struct { err error } -func (f *fakeFailoverMatcher) Match(_ context.Context, _ api.ExternalSchedulerRequest, orig []string) ([]string, error) { +func (f *fakeFailoverMatcher) Match(_ context.Context, _ api.ExternalSchedulerRequest, originalHosts []string) ([]string, error) { f.called = true - f.gotHosts = orig + f.gotHosts = originalHosts if f.err != nil { return nil, f.err } diff --git a/internal/scheduling/reservations/failoverhost/matcher.go b/internal/scheduling/reservations/failoverhost/matcher.go index c6aa719f6..eadb37bc0 100644 --- a/internal/scheduling/reservations/failoverhost/matcher.go +++ b/internal/scheduling/reservations/failoverhost/matcher.go @@ -46,17 +46,17 @@ type Matcher struct { // Fallbacks preserve a valid placement: if no reservation matches, or the // chosen host is not among orig (so it conflicts with other scheduling // constraints), orig is returned unchanged so the VM still has candidates. -func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerRequest, orig []string) ([]string, error) { +func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerRequest, originalHosts []string) ([]string, error) { log := logf.FromContext(ctx) intent, err := request.GetIntent() if err != nil || intent != api.EvacuateIntent { - return orig, nil //nolint:nilerr // non-evacuation requests are not rerouted + return originalHosts, nil //nolint:nilerr // non-evacuation requests are not rerouted } sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host") if err != nil || sourceHost == "" { log.V(1).Info("no source_host hint on evacuation, leaving candidates untouched") - return orig, nil //nolint:nilerr // without a source host there is nothing to match + return originalHosts, nil //nolint:nilerr // without a source host there is nothing to match } var reservations v1alpha1.ReservationList @@ -78,7 +78,7 @@ func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerReques log.V(1).Info("no failover host reservation for source host, falling back to ordinary candidates", "sourceHost", sourceHost) m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) - return orig, nil + return originalHosts, nil } // Sort deterministically by target host so selection is stable and testable; @@ -109,7 +109,7 @@ func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerReques res.Status.FailoverHostReservation.SourceHost == sourceHost { log.V(1).Info("reusing in-use failover host reservation for gang-evacuation", "sourceHost", sourceHost, "reservation", res.Name, "targetHost", res.Spec.TargetHost) - return m.pick(*res, orig), nil + return m.pick(*res, originalHosts), nil } } @@ -121,14 +121,14 @@ func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerReques log.V(1).Info("all failover host reservations for source host are in use by other sources, falling back", "sourceHost", sourceHost) m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) - return orig, nil + return originalHosts, nil } if err := m.markInUse(ctx, free, sourceHost); err != nil { return nil, err } log.V(1).Info("pinned failover host reservation for evacuation", "sourceHost", sourceHost, "reservation", free.Name, "targetHost", free.Spec.TargetHost) - return m.pick(*free, orig), nil + return m.pick(*free, originalHosts), nil } // pick returns the reservation's target host as the sole candidate, unless that From c1dc46a8650b9261d40ea554758ced69b3adfc56 Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Wed, 4 Nov 2026 13:03:31 +0100 Subject: [PATCH 3/7] refactor(nova): match failover evacuations via filter + weigher Replace the post-pipeline Matcher that reduced evacuation candidates to a single reserved host (and speculatively wrote InUse) with a filter, a weigher, and a thin outcome-based pin: - filter_failover_host (replaces filter_no_failover_host): removes all failover hosts for ordinary requests, and for evacuations keeps only the deterministically selected target, pruning the other reachable failover targets so a second failure keeps a free landing zone while ordinary candidates are retained for Nova retries. - weigher_failover_host: lifts the selected target to the front via a dominating weight, leaving the retained candidates behind it as fallbacks. - Matcher.ConfirmAndMarkInUse: no longer rewrites the candidate list; marks the reservation InUse only when its target is the pipeline's top candidate, so a target that lost to other constraints leaves no dangling InUse. Both plugins and the matcher apply the same deterministic selection rule (sort matching reservations by target host name; prefer one already InUse by this source; never steal one InUse by another source) over the same snapshot. Rename the kvm.filterNoFailoverHost.enabled Helm value to kvm.failoverHost.enabled (shared by both steps), wire the new weigher into the KVM pipelines, and rewrite the scheduling docs with the rationale. Signed-off-by: Philipp Matthes --- .../04-failover-host-reservations.md | 105 +++++---- .../cortex-nova/templates/pipelines_kvm.yaml | 96 +++++--- helm/bundles/cortex-nova/values.yaml | 14 +- .../filter_weigher_pipeline_controller.go | 43 ++-- ...filter_weigher_pipeline_controller_test.go | 45 ++-- .../plugins/filters/filter_failover_host.go | 144 ++++++++++++ .../filters/filter_failover_host_test.go | 218 ++++++++++++++++++ .../filters/filter_no_failover_host.go | 90 -------- .../filters/filter_no_failover_host_test.go | 149 ------------ .../plugins/weighers/weigher_failover_host.go | 129 +++++++++++ .../weighers/weigher_failover_host_test.go | 163 +++++++++++++ .../reservations/failoverhost/matcher.go | 129 ++++++----- .../reservations/failoverhost/matcher_test.go | 132 ++++++----- 13 files changed, 968 insertions(+), 489 deletions(-) create mode 100644 internal/scheduling/nova/plugins/filters/filter_failover_host.go create mode 100644 internal/scheduling/nova/plugins/filters/filter_failover_host_test.go delete mode 100644 internal/scheduling/nova/plugins/filters/filter_no_failover_host.go delete mode 100644 internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go create mode 100644 internal/scheduling/nova/plugins/weighers/weigher_failover_host.go create mode 100644 internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go diff --git a/docs/03-reservations-and-inventory/04-failover-host-reservations.md b/docs/03-reservations-and-inventory/04-failover-host-reservations.md index 4e8c23a50..f7163c529 100644 --- a/docs/03-reservations-and-inventory/04-failover-host-reservations.md +++ b/docs/03-reservations-and-inventory/04-failover-host-reservations.md @@ -477,17 +477,46 @@ stateDiagram-v2 ## How failover host reservations affect scheduling -Creating a reservation is only half the story — the scheduler has to *honor* it. Two distinct behaviors wire -failover host reservations into the Nova scheduling pipeline, split along a natural seam: one keeps new -workload *off* reserved hosts, the other steers a dead host's workload *onto* them. +Creating a reservation is only half the story — the scheduler has to *honor* it. Three cooperating pieces +wire failover host reservations into the Nova scheduling pipeline, split along a natural seam: a **filter** +and a **weigher** shape the candidate list *inside* the pipeline, and a thin **post-pipeline step** records +the outcome. Together they keep new workload *off* reserved hosts and steer a dead host's workload *onto* the +one reserved host it should gang-evacuate to — while deliberately *keeping* the ordinary candidates so Nova +can retry if that landing fails. + +### Selecting the target: one deterministic rule + +For an evacuation (identified by the `evacuate` intent) the dead host is read from the `source_host` +scheduler hint, and a single **deterministic rule** picks which reserved host this source should evacuate +onto. Both the filter and the weigher apply this same rule, independently, over the *same* reservation +snapshot within one pipeline run, so they always agree without coordinating: + +1. Keep the failover host reservations whose `sourceHosts` include the source host — the reservations this + host may evacuate onto. +2. If one is already **In Use** *by this same source host*, that is the selected target (the + gang-evacuation path — the whole gang lands on the same target). +3. Otherwise ignore any reservation **In Use** by a *different* source (pinning it would steal a landing + zone actively absorbing another host's gang), sort the rest by target host name, and take the first. +4. If nothing remains, there is no match and the evacuation simply uses the ordinary candidates. + +### The filter: block reserved hosts, keep only the selected target + +The **`filter_failover_host`** pipeline filter is purely *subtractive* — it only ever removes hosts — so its +position in the pipeline does not matter and all other filters and weighers still apply around it. It is +toggled by the `kvm.failoverHost.enabled` Helm value (default `false`) and is a no-op when disabled. What it +removes depends on the request: -### Normal placement: reserved hosts are blocked +- **Ordinary (non-evacuation) requests:** it removes *every* failover host, so no new VM is placed on a host + being drained or kept empty as a landing zone. +- **Evacuations:** it removes every failover host *except* the one selected by the rule above. The ordinary, + non-reserved candidates are left untouched. -For every non-evacuation request, the **`filter_no_failover_host`** pipeline filter removes any host that has a -failover host reservation from the candidate set, so no new VM is placed on a host that is being drained or -kept empty as a landing zone. The filter is purely *subtractive* — it only ever removes hosts — so its -position in the pipeline does not matter and all other filters and weighers still apply around it. It is -toggled by the `kvm.filterNoFailoverHost.enabled` Helm value (default `false`) and is a no-op when disabled. +Pruning the *other* reserved hosts is the key to mutual exclusion between failover targets. Consider a VM +that could go to reserved `[h1, h2]` (both failover targets reachable from this source) plus ordinary +`[h3, h4]`, where only `h1` is selected. The filter emits `[h1, h3, h4]` — **not** `[h1, h2, h3, h4]`: `h2` +is held back so a *second*, concurrent host failure still has its own free landing zone rather than finding +it already consumed. Keeping `h3` and `h4` means that if the evacuation onto `h1` fails, Nova retries onto an +ordinary host instead of being forced onto an unmatched reserved slot. The filter also honors the **call-time** `ignoredReservationTypes` request option. This is what keeps the **path finder probe** working: the probe asks the scheduler which hosts would accept a flavor, and it passes @@ -495,39 +524,35 @@ The filter also honors the **call-time** `ignoredReservationTypes` request optio partition. Because the filter obeys that option, the probe still discovers already-reserved hosts and the partitioning does not fragment. -### Evacuation: routing onto a failover host (gang-evacuation) - -Evacuations are **not** handled by the filter — it deliberately leaves evacuation requests untouched. -Instead, a dedicated **Matcher** runs *after* the filter/weigher pipeline has produced its ordered -candidates, from inside the pipeline's global execution lock. For an evacuation request (identified by the -`evacuate` intent) it reads the dead host from the `source_host` scheduler hint and: - -1. finds the failover host reservations whose `sourceHosts` include that source host; -2. if **none** match, it changes nothing — the request falls back to the ordinary candidate set so the VM - still has somewhere to go — and increments a metric - (`cortex_failover_host_evacuations_without_reservation_total`) that drives an alert on frequent - occurrences; -3. if one of them is already **In Use** *by this same source host*, it reuses that one — this is the - gang-evacuation path, so the whole gang lands on the same target; -4. otherwise it pins the first reservation that is *not* already in use, marking it **In Use** and recording - the source host on its status, then returns that reservation's host as the sole candidate. - -A reservation can legitimately rescue several source hosts (the planner lists every partition peer as a -source), so two hosts in the same partition can fail over concurrently, one reservation each. The Matcher -therefore never pins a reservation that is already **In Use** by a *different* source — that would steal a -landing zone actively absorbing another host's gang. If every matching reservation is already taken by other -sources, it falls back to the ordinary candidate set (and records the same metric as the no-reservation -case). +### The weigher: lift the selected target to the front + +The **`weigher_failover_host`** weigher gives the selected target a dominating weight — far larger than any +other weigher can contribute — so the pipeline's ordinary descending-weight sort places it first. It selects +the same host the filter kept (same rule, same snapshot), so it simply lifts the one reserved host that +survived filtering above the retained ordinary candidates. For non-evacuation requests, or when no target is +selected, it has no effect. Because the weigher only *weighs*, the ordinary candidates keep their relative +order behind the target and remain available as retry fallbacks. + +### The post-pipeline step: pin the target for gang-evacuation + +A thin **Matcher** runs *after* the pipeline has produced its ordered candidates, from inside the pipeline's +global execution lock, on write runs only. It does **not** touch the candidate list — the filter and weigher +have already shaped it. Its sole job is the stateful write: it re-derives the selected reservation with the +same rule and, **only if that target is actually the top candidate**, marks it **In Use** and records the +source host on its status. + +Gating the write on the *outcome* is deliberate. If the selected target lost to other scheduling constraints +(the filter removed it, so an ordinary host ended up on top), the evacuation falls back to that ordinary host +and the Matcher writes nothing — no dangling `In Use` is left behind. In that case, and when no reservation +matches or all are taken by other sources, it increments a metric +(`cortex_failover_host_evacuations_without_reservation_total`) that drives an alert on frequent occurrences. Running inside the global pipeline lock is what makes **gang-evacuation** work. The *first* VM evacuated from -a dead host pins a target and writes `In Use` while holding the lock; every subsequent VM from the same -source host — serialized behind the same lock — sees that `In Use` reservation and lands on the *same* -target. Writing the pin *after* the pipeline (rather than inside the filter) avoids a dangling `In Use` -reservation if a later step fails, and keeping it under the lock guarantees the next request observes it. - -As a safety mechanism, if the chosen reserved host is not among the pipeline's candidates (the reservation -conflicts with the request's other scheduling constraints), the Matcher falls back to the original candidate -set rather than returning an empty one, so the VM is never left with nowhere to schedule. +a dead host pins its target and writes `In Use` while holding the lock; every subsequent VM from the same +source host — serialized behind the same lock — sees that `In Use` reservation, so the selection rule keeps +choosing it (step 2) and the whole gang lands on the *same* target. The reuse is idempotent: a reservation +already `In Use` by this source needs no second write. Writing the pin *after* the pipeline (rather than +inside the filter) is what lets the write be gated on the final ordering. The Matcher only ever advances a reservation *to* `In Use`. Moving it back out — to `Active` once the evacuation completes, or on cleanup — stays with the failover host lifecycle reconciler, based on the real diff --git a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml index 568475805..6b6cb8952 100644 --- a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml +++ b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml @@ -84,17 +84,19 @@ spec: # FailoverHostReservation is a new feature and is not yet counted as # reserved capacity during scheduling. - {key: ignoredReservationTypes, stringListValue: ["FailoverHostReservation"]} - - name: filter_no_failover_host - description: | - This step blocks hosts that have a failover host reservation, so no new - virtual machines are placed on a host being drained or kept empty as a - gang-evacuation landing zone. It only applies to non-evacuation requests; - evacuations are routed onto reserved hosts after the pipeline. The filter - is purely subtractive, so its position does not matter. It honors the - call-time ignoredReservationTypes option so the failover host path finder - probe still discovers reserved hosts. + - name: filter_failover_host + description: | + This step keeps new virtual machines off hosts that have a failover host + reservation. For ordinary requests it removes every such host; for + evacuations it removes all but the one failover host deterministically + selected for the source host, so another failure keeps a free landing + zone, while ordinary candidates are retained so Nova can retry on failure. + The weigher_failover_host weigher then lifts the selected host to the + front. The filter is purely subtractive, so its position does not matter. + It honors the call-time ignoredReservationTypes option so the failover + host path finder probe still discovers reserved hosts. params: - - {key: enabled, boolValue: {{ .Values.kvm.filterNoFailoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} - name: filter_allowed_projects description: | This step filters hosts based on allowed projects defined in the @@ -168,6 +170,16 @@ spec: VM is allocated get a higher weight, encouraging placement on pre-reserved failover capacity. For non-evacuation requests, this weigher has no effect. + - name: weigher_failover_host + description: | + For an evacuation, this weigher lifts the failover host selected for the + request's source host to the front of the candidate list by giving it a + dominating weight, so it becomes Nova's first choice while the retained + ordinary candidates stay behind it as retry fallbacks. It selects the same + host the filter_failover_host filter kept. For non-evacuation requests it + has no effect. + params: + - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} - name: kvm_committed_resource_reservation description: | This weigher boosts hosts that have a ready CommittedResourceReservation @@ -269,17 +281,19 @@ spec: # FailoverHostReservation is a new feature and is not yet counted as # reserved capacity during scheduling. - {key: ignoredReservationTypes, stringListValue: ["FailoverHostReservation"]} - - name: filter_no_failover_host - description: | - This step blocks hosts that have a failover host reservation, so no new - virtual machines are placed on a host being drained or kept empty as a - gang-evacuation landing zone. It only applies to non-evacuation requests; - evacuations are routed onto reserved hosts after the pipeline. The filter - is purely subtractive, so its position does not matter. It honors the - call-time ignoredReservationTypes option so the failover host path finder - probe still discovers reserved hosts. + - name: filter_failover_host + description: | + This step keeps new virtual machines off hosts that have a failover host + reservation. For ordinary requests it removes every such host; for + evacuations it removes all but the one failover host deterministically + selected for the source host, so another failure keeps a free landing + zone, while ordinary candidates are retained so Nova can retry on failure. + The weigher_failover_host weigher then lifts the selected host to the + front. The filter is purely subtractive, so its position does not matter. + It honors the call-time ignoredReservationTypes option so the failover + host path finder probe still discovers reserved hosts. params: - - {key: enabled, boolValue: {{ .Values.kvm.filterNoFailoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} - name: filter_allowed_projects description: | This step filters hosts based on allowed projects defined in the @@ -350,6 +364,16 @@ spec: VM is allocated get a higher weight, encouraging placement on pre-reserved failover capacity. For non-evacuation requests, this weigher has no effect. + - name: weigher_failover_host + description: | + For an evacuation, this weigher lifts the failover host selected for the + request's source host to the front of the candidate list by giving it a + dominating weight, so it becomes Nova's first choice while the retained + ordinary candidates stay behind it as retry fallbacks. It selects the same + host the filter_failover_host filter kept. For non-evacuation requests it + has no effect. + params: + - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} - name: kvm_committed_resource_reservation description: | This weigher boosts hosts that have a ready CommittedResourceReservation @@ -604,17 +628,19 @@ spec: # FailoverHostReservation is a new feature and is not yet counted as # reserved capacity during scheduling. - {key: ignoredReservationTypes, stringListValue: ["FailoverHostReservation"]} - - name: filter_no_failover_host - description: | - This step blocks hosts that have a failover host reservation, so no new - virtual machines are placed on a host being drained or kept empty as a - gang-evacuation landing zone. It only applies to non-evacuation requests; - evacuations are routed onto reserved hosts after the pipeline. The filter - is purely subtractive, so its position does not matter. It honors the - call-time ignoredReservationTypes option so the failover host path finder - probe still discovers reserved hosts. + - name: filter_failover_host + description: | + This step keeps new virtual machines off hosts that have a failover host + reservation. For ordinary requests it removes every such host; for + evacuations it removes all but the one failover host deterministically + selected for the source host, so another failure keeps a free landing + zone, while ordinary candidates are retained so Nova can retry on failure. + The weigher_failover_host weigher then lifts the selected host to the + front. The filter is purely subtractive, so its position does not matter. + It honors the call-time ignoredReservationTypes option so the failover + host path finder probe still discovers reserved hosts. params: - - {key: enabled, boolValue: {{ .Values.kvm.filterNoFailoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} - name: filter_has_requested_traits description: | This step filters hosts that do not have the requested traits given by the @@ -655,6 +681,16 @@ spec: VM is allocated get a higher weight, encouraging placement on pre-reserved failover capacity. For non-evacuation requests, this weigher has no effect. + - name: weigher_failover_host + description: | + For an evacuation, this weigher lifts the failover host selected for the + request's source host to the front of the candidate list by giving it a + dominating weight, so it becomes Nova's first choice while the retained + ordinary candidates stay behind it as retry fallbacks. It selects the same + host the filter_failover_host filter kept. For non-evacuation requests it + has no effect. + params: + - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} --- apiVersion: cortex.cloud/v1alpha1 kind: Pipeline diff --git a/helm/bundles/cortex-nova/values.yaml b/helm/bundles/cortex-nova/values.yaml index e002b86bc..416eb5305 100644 --- a/helm/bundles/cortex-nova/values.yaml +++ b/helm/bundles/cortex-nova/values.yaml @@ -109,13 +109,15 @@ kvm: # Domain names that are explicitly treated as non-external-customer domains and # skipped by the filter, even if they match one of the prefixes above. ignoredDomainNames: [] - # Options for the filter_no_failover_host pipeline step, which blocks hosts with a - # failover host reservation so no new VMs land on a host being kept empty as a - # gang-evacuation landing zone. Applied to every KVM pipeline that uses the step. - filterNoFailoverHost: + # Options for the failover host scheduling steps. The filter_failover_host + # filter keeps new VMs off hosts with a failover host reservation (and, for + # evacuations, prunes all but the deterministically selected failover host so + # another failure keeps a free landing zone); the weigher_failover_host weigher + # lifts that selected host to the front of the candidate list. Applied to every + # KVM pipeline that uses the steps. + failoverHost: # Disabled by default until the failover host draining controller logic is - # implemented. Set to true to block hosts with a failover host reservation - # from ordinary placement. + # implemented. Set to true to enable failover-host-aware scheduling. enabled: false cortex: &cortex diff --git a/internal/scheduling/nova/filter_weigher_pipeline_controller.go b/internal/scheduling/nova/filter_weigher_pipeline_controller.go index 894b19670..c893d568a 100644 --- a/internal/scheduling/nova/filter_weigher_pipeline_controller.go +++ b/internal/scheduling/nova/filter_weigher_pipeline_controller.go @@ -57,20 +57,21 @@ type FilterWeigherPipelineController struct { // CRRecorder receives scheduling events and updates CR reservations and metrics. CRRecorder crs.Recorder - // FailoverMatcher routes evacuation requests onto failover host reservations - // after the pipeline runs, inside the exclusive lock. Optional; when nil, no - // failover host rerouting is applied. + // FailoverMatcher pins an evacuation's gang target to a failover host + // reservation after the pipeline runs, inside the exclusive lock. Optional; + // when nil, no failover host pinning is applied. FailoverMatcher FailoverHostMatcher } -// FailoverHostMatcher reroutes an evacuation onto a failover host reservation. -// Implemented by reservations/failoverhost.Matcher; declared here as an -// interface so the nova package does not import failoverhost (which would form -// an import cycle via that package's integration tests). +// FailoverHostMatcher confirms an evacuation's failover host target and marks the +// reservation InUse. Implemented by reservations/failoverhost.Matcher; declared +// here as an interface so the nova package does not import failoverhost (which +// would form an import cycle via that package's integration tests). type FailoverHostMatcher interface { - // Match returns the candidate hosts to use, possibly rewritten to the chosen - // failover host, or the originals unchanged when nothing applies. - Match(ctx context.Context, request api.ExternalSchedulerRequest, originalHosts []string) ([]string, error) + // ConfirmAndMarkInUse marks the failover host reservation selected for the + // request's source host InUse, but only when it is the pipeline's top + // candidate. It does not change the candidate list. + ConfirmAndMarkInUse(ctx context.Context, request api.ExternalSchedulerRequest, orderedHosts []string) error } // The type of pipeline this controller manages. @@ -192,23 +193,19 @@ func (c *FilterWeigherPipelineController) process(ctx context.Context, decision result, err := pipeline.Run(request) if err == nil { - // Route evacuations onto failover host reservations and pin the - // source->target relationship. This runs inside the exclusive lock (write - // runs only) so the InUse write is visible to subsequent evacuations from - // the same source, implementing gang-evacuation. Read-only runs (probes, - // capacity checks) never reroute or write. + // Mark the failover host reservation selected for an evacuation InUse, + // pinning the source->target relationship for gang-evacuation. The + // candidate shaping (removing other reserved hosts, lifting the selected + // target to the front) is already done inside the pipeline by the + // filter_failover_host filter and weigher_failover_host weigher; this only + // performs the serialized InUse write. It runs inside the exclusive lock + // (write runs only) so the write is visible to subsequent evacuations from + // the same source. Read-only runs (probes, capacity checks) never pin. if c.FailoverMatcher != nil && !request.Options.ReadOnly { - hosts, mErr := c.FailoverMatcher.Match(ctx, request, result.OrderedHosts) - if mErr != nil { + if mErr := c.FailoverMatcher.ConfirmAndMarkInUse(ctx, request, result.OrderedHosts); mErr != nil { log.Error(mErr, "failover host matcher failed") return &request, mErr } - result.OrderedHosts = hosts - if len(hosts) > 0 { - result.TargetHost = &hosts[0] - } else { - result.TargetHost = nil - } } decision.Status.Result = &result } diff --git a/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go b/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go index 9d514833b..b3a2623d1 100644 --- a/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go +++ b/internal/scheduling/nova/filter_weigher_pipeline_controller_test.go @@ -962,22 +962,20 @@ func TestFilterWeigherPipelineController_PeekReadOnly(t *testing.T) { } } -// fakeFailoverMatcher records the candidates it was handed and returns a -// preconfigured result, so controller wiring can be asserted in isolation. +// fakeFailoverMatcher records the candidates it was handed, so controller wiring +// can be asserted in isolation. It no longer rewrites the candidate list — the +// InUse write it performs in production is a side effect the controller does not +// observe. type fakeFailoverMatcher struct { called bool gotHosts []string - ret []string err error } -func (f *fakeFailoverMatcher) Match(_ context.Context, _ api.ExternalSchedulerRequest, originalHosts []string) ([]string, error) { +func (f *fakeFailoverMatcher) ConfirmAndMarkInUse(_ context.Context, _ api.ExternalSchedulerRequest, orderedHosts []string) error { f.called = true - f.gotHosts = originalHosts - if f.err != nil { - return nil, f.err - } - return f.ret, nil + f.gotHosts = orderedHosts + return f.err } // runProcessWithMatcher builds a controller with an empty pipeline and the given @@ -1044,8 +1042,8 @@ func runProcessWithMatcher(t *testing.T, matcher FailoverHostMatcher, readOnly b return decision, controller.ProcessNewDecisionFromAPI(context.Background(), decision) } -func TestFilterWeigherPipelineController_FailoverMatcherRewritesResult(t *testing.T) { - m := &fakeFailoverMatcher{ret: []string{"compute-2"}} +func TestFilterWeigherPipelineController_FailoverMatcherConfirmsResult(t *testing.T) { + m := &fakeFailoverMatcher{} decision, err := runProcessWithMatcher(t, m, false) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -1053,24 +1051,24 @@ func TestFilterWeigherPipelineController_FailoverMatcherRewritesResult(t *testin if !m.called { t.Fatal("expected matcher to be called") } - // Matcher receives the pipeline's ordered hosts. + // Matcher receives the pipeline's ordered hosts but does not rewrite them. if len(m.gotHosts) != 2 || m.gotHosts[0] != "compute-1" || m.gotHosts[1] != "compute-2" { t.Errorf("matcher got unexpected hosts: %v", m.gotHosts) } - // Its returned hosts become the result, with TargetHost pointing at the first. + // The pipeline's ordering and target host flow through unchanged. if decision.Status.Result == nil { t.Fatal("expected result to be set") } - if got := decision.Status.Result.OrderedHosts; len(got) != 1 || got[0] != "compute-2" { - t.Errorf("expected OrderedHosts [compute-2], got %v", got) + if got := decision.Status.Result.OrderedHosts; len(got) != 2 || got[0] != "compute-1" { + t.Errorf("expected OrderedHosts [compute-1 compute-2], got %v", got) } - if decision.Status.Result.TargetHost == nil || *decision.Status.Result.TargetHost != "compute-2" { - t.Errorf("expected TargetHost compute-2, got %v", decision.Status.Result.TargetHost) + if decision.Status.Result.TargetHost == nil || *decision.Status.Result.TargetHost != "compute-1" { + t.Errorf("expected TargetHost compute-1, got %v", decision.Status.Result.TargetHost) } } func TestFilterWeigherPipelineController_FailoverMatcherSkippedOnReadOnly(t *testing.T) { - m := &fakeFailoverMatcher{ret: []string{"compute-2"}} + m := &fakeFailoverMatcher{} decision, err := runProcessWithMatcher(t, m, true) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -1095,14 +1093,3 @@ func TestFilterWeigherPipelineController_FailoverMatcherErrorFailsDecision(t *te t.Errorf("expected no result on matcher error, got %v", decision.Status.Result) } } - -func TestFilterWeigherPipelineController_FailoverMatcherEmptyClearsTargetHost(t *testing.T) { - m := &fakeFailoverMatcher{ret: []string{}} - decision, err := runProcessWithMatcher(t, m, false) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if decision.Status.Result.TargetHost != nil { - t.Errorf("expected nil TargetHost for empty host list, got %v", *decision.Status.Result.TargetHost) - } -} diff --git a/internal/scheduling/nova/plugins/filters/filter_failover_host.go b/internal/scheduling/nova/plugins/filters/filter_failover_host.go new file mode 100644 index 000000000..1d81acc1b --- /dev/null +++ b/internal/scheduling/nova/plugins/filters/filter_failover_host.go @@ -0,0 +1,144 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package filters + +import ( + "context" + "slices" + "sort" + + "github.com/go-logr/logr" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "github.com/cobaltcore-dev/cortex/internal/scheduling/lib" + "k8s.io/apimachinery/pkg/api/meta" +) + +// FilterFailoverHostOpts configures the failover-host filter. +type FilterFailoverHostOpts struct { + // Enabled toggles the filter. When false, the filter is a no-op and all + // hosts pass. This lets the pipeline carry the step while keeping the + // behavior off until the feature is rolled out. + Enabled bool `json:"enabled"` +} + +func (FilterFailoverHostOpts) Validate() error { return nil } + +// FilterFailoverHost keeps ordinary virtual machines off hosts that have a +// failover host reservation, and for evacuations narrows the reserved hosts down +// to the single one this source host should gang-evacuate onto. +// +// For a non-evacuation request it removes every failover host, so no new virtual +// machine lands on a host being drained or kept empty as a gang-evacuation +// landing zone. +// +// For an evacuation request it removes every failover host except the one +// deterministically selected for the request's source host (see selectFailoverHost). +// Keeping only the selected target — and leaving the ordinary, non-reserved +// candidates untouched — means the other reserved hosts stay free for a second +// host failure, while Nova can still retry onto an ordinary candidate if the +// evacuation onto the selected target fails. The weigher_failover_host weigher +// then lifts the selected target to the front of the candidate list. +// +// The filter is purely subtractive, so its position in the pipeline does not +// matter: other filters and weighers still run around it. +type FilterFailoverHost struct { + lib.BaseFilter[api.ExternalSchedulerRequest, FilterFailoverHostOpts] +} + +func (s *FilterFailoverHost) Run(traceLog logr.Logger, request api.ExternalSchedulerRequest) (*lib.FilterWeigherPipelineStepResult, error) { + result := s.IncludeAllHostsFromRequest(request) + if !s.Options.Enabled { + return result, nil + } + + // Honor the call-time ignored reservation types. When failover host + // reservations are ignored — as the path finder probe does (see + // reservations/failoverhost/pathfinding.go) — block nothing so reserved + // hosts stay visible to their own partition. + if slices.Contains(request.GetOptions().IgnoredReservationTypes, v1alpha1.ReservationTypeFailoverHost) { + traceLog.V(1).Info("failover host reservations ignored, not blocking any hosts") + return result, nil + } + + var reservations v1alpha1.ReservationList + if err := s.Client.List(context.Background(), &reservations); err != nil { + return nil, err + } + + // For an evacuation, keep the single failover host selected for this source + // so the gang lands there; for everything else, keep none. + var keep string + if intent, err := request.GetIntent(); err == nil && intent == api.EvacuateIntent { + if sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host"); err == nil && sourceHost != "" { + keep = selectFailoverHost(reservations.Items, sourceHost) + } + } + + for _, reservation := range reservations.Items { + if reservation.Spec.Type != v1alpha1.ReservationTypeFailoverHost { + continue + } + // Block both the desired (TargetHost) and actual (Status.Host) host to + // cover the window while the reservation is being placed. A reservation + // only exists from the Designated stage onward (the controller deletes + // Revoked/Used ones), so its presence is enough to keep new virtual + // machines off the host. + for _, host := range []string{reservation.Spec.TargetHost, reservation.Status.Host} { + if host == "" || host == keep { + continue + } + delete(result.Activations, host) // noop if host is not in the map + traceLog.V(1).Info("filtering out failover host reservation target", + "host", host, "reservation", reservation.Name) + } + } + return result, nil +} + +// selectFailoverHost picks the target host of the one failover host reservation a +// source host should gang-evacuate onto, deterministically. It is a pure function +// of the reservation snapshot, so the filter and weigher (which run over the same +// snapshot) select the same target without coordinating. +// +// Among the reservations whose partition includes sourceHost it prefers the one +// already in use by this very source (the gang-evacuation path — the whole gang +// lands on the same target), never picks one in use by a different source (which +// would steal an active landing zone), and otherwise takes the first by target +// host name. Returns "" when nothing matches. +func selectFailoverHost(reservations []v1alpha1.Reservation, sourceHost string) string { + var matching []v1alpha1.Reservation + for _, res := range reservations { + if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { + continue + } + if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { + matching = append(matching, res) + } + } + sort.Slice(matching, func(i, j int) bool { + return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost + }) + var firstFree string + for i := range matching { + res := &matching[i] + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + if firstFree == "" { + firstFree = res.Spec.TargetHost + } + continue + } + // In use: reuse it only if it is in use by this very source host. + if res.Status.FailoverHostReservation != nil && + res.Status.FailoverHostReservation.SourceHost == sourceHost { + return res.Spec.TargetHost + } + } + return firstFree +} + +func init() { + Index["filter_failover_host"] = func() NovaFilter { return &FilterFailoverHost{} } +} diff --git a/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go b/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go new file mode 100644 index 000000000..b99c0953b --- /dev/null +++ b/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go @@ -0,0 +1,218 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package filters + +import ( + "testing" + + "github.com/go-logr/logr" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/scheduling" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// failoverHostReservation builds a failover host reservation targeting targetHost +// (with statusHost as its actual host), able to rescue the given source hosts. +func failoverHostReservation(targetHost, statusHost string, sourceHosts ...string) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeFailoverHost, + TargetHost: targetHost, + FailoverHostReservation: &v1alpha1.FailoverHostReservationSpec{ + SourceHosts: sourceHosts, + }, + }, + Status: v1alpha1.ReservationStatus{Host: statusHost}, + } +} + +// inUseBy marks a failover host reservation as in use by the given source host. +func inUseBy(res *v1alpha1.Reservation, sourceHost string) *v1alpha1.Reservation { + res.Status.FailoverHostReservation = &v1alpha1.FailoverHostReservationStatus{SourceHost: sourceHost} + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostInUse, + Status: metav1.ConditionTrue, + Reason: "EvacuationStarted", + }) + return res +} + +func committedResourceReservationOn(targetHost string) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "cr-" + targetHost}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: targetHost, + }, + } +} + +func requestWithHosts(hints map[string]any, hosts ...string) api.ExternalSchedulerRequest { + hvHosts := make([]api.ExternalSchedulerHost, len(hosts)) + for i, h := range hosts { + hvHosts[i] = api.ExternalSchedulerHost{ComputeHost: h} + } + return api.ExternalSchedulerRequest{ + Hosts: hvHosts, + Spec: api.NovaObject[api.NovaSpec]{Data: api.NovaSpec{SchedulerHints: hints}}, + } +} + +func evacuationHints(sourceHost string) map[string]any { + hints := map[string]any{"_nova_check_type": "evacuate"} + if sourceHost != "" { + hints["source_host"] = sourceHost + } + return hints +} + +func TestFilterFailoverHost_Run(t *testing.T) { + scheme := buildTestScheme(t) + + tests := []struct { + name string + opts FilterFailoverHostOpts + reservations []client.Object + request api.ExternalSchedulerRequest + expectedHosts []string + }{ + { + name: "disabled - all hosts pass even with reservations", + opts: FilterFailoverHostOpts{Enabled: false}, + reservations: []client.Object{failoverHostReservation("host2", "")}, + request: requestWithHosts(nil, "host1", "host2", "host3"), + expectedHosts: []string{"host1", "host2", "host3"}, + }, + { + name: "non-evacuation - blocks reserved target host", + opts: FilterFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("host2", "")}, + request: requestWithHosts(nil, "host1", "host2", "host3"), + expectedHosts: []string{"host1", "host3"}, + }, + { + name: "non-evacuation - blocks both target and status host", + opts: FilterFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("host3", "host4")}, + request: requestWithHosts(nil, "host1", "host2", "host3", "host4"), + expectedHosts: []string{"host1", "host2"}, + }, + { + name: "non-evacuation - ignores non-failover-host reservations", + opts: FilterFailoverHostOpts{Enabled: true}, + reservations: []client.Object{committedResourceReservationOn("host2")}, + request: requestWithHosts(nil, "host1", "host2", "host3"), + expectedHosts: []string{"host1", "host2", "host3"}, + }, + { + name: "evacuation - keeps the selected target and prunes other failover targets", + opts: FilterFailoverHostOpts{Enabled: true}, + // h1 and h2 are both reachable from source1; only the name-first (h1) + // is kept, h2 is pruned to stay free for another failure. h3/h4 ordinary. + reservations: []client.Object{ + failoverHostReservation("h1", "", "source1"), + failoverHostReservation("h2", "", "source1"), + }, + request: requestWithHosts(evacuationHints("source1"), "h1", "h2", "h3", "h4"), + expectedHosts: []string{"h1", "h3", "h4"}, + }, + { + name: "evacuation - reuses the in-use target for the same source", + opts: FilterFailoverHostOpts{Enabled: true}, + // h2 is in use by source1, so it is the selected target even though h1 + // sorts first; h1 is pruned. + reservations: []client.Object{ + failoverHostReservation("h1", "", "source1"), + inUseBy(failoverHostReservation("h2", "", "source1"), "source1"), + }, + request: requestWithHosts(evacuationHints("source1"), "h1", "h2", "h3"), + expectedHosts: []string{"h2", "h3"}, + }, + { + name: "evacuation - never keeps a target in use by another source", + opts: FilterFailoverHostOpts{Enabled: true}, + // h1 is in use by source2, so source1 selects the free h2; both reserved + // hosts except the selected h2 are pruned. + reservations: []client.Object{ + inUseBy(failoverHostReservation("h1", "", "source1", "source2"), "source2"), + failoverHostReservation("h2", "", "source1", "source2"), + }, + request: requestWithHosts(evacuationHints("source1"), "h1", "h2", "h3"), + expectedHosts: []string{"h2", "h3"}, + }, + { + name: "evacuation - no matching reservation removes all failover hosts", + opts: FilterFailoverHostOpts{Enabled: true}, + reservations: []client.Object{ + failoverHostReservation("h1", "", "otherSource"), + }, + request: requestWithHosts(evacuationHints("source1"), "h1", "h2", "h3"), + expectedHosts: []string{"h2", "h3"}, + }, + { + name: "evacuation - no source hint removes all failover hosts", + opts: FilterFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("h1", "", "source1")}, + request: requestWithHosts(evacuationHints(""), "h1", "h2", "h3"), + expectedHosts: []string{"h2", "h3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + step := &FilterFailoverHost{} + step.Options = tt.opts + step.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.reservations...).Build() + + result, err := step.Run(logr.Discard(), tt.request) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(result.Activations) != len(tt.expectedHosts) { + t.Errorf("expected %d hosts, got %d: %v", len(tt.expectedHosts), len(result.Activations), result.Activations) + } + for _, host := range tt.expectedHosts { + if _, ok := result.Activations[host]; !ok { + t.Errorf("expected host %s to be present", host) + } + } + }) + } +} + +func TestFilterFailoverHost_CallTimeIgnoredReservationTypes(t *testing.T) { + scheme := buildTestScheme(t) + step := &FilterFailoverHost{} + step.Options = FilterFailoverHostOpts{Enabled: true} + step.Client = fake.NewClientBuilder().WithScheme(scheme). + WithObjects(failoverHostReservation("host2", "")).Build() + + request := requestWithHosts(nil, "host1", "host2", "host3") + request.Options = scheduling.Options{ + IgnoredReservationTypes: []v1alpha1.ReservationType{v1alpha1.ReservationTypeFailoverHost}, + } + result, err := step.Run(logr.Discard(), request) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if _, ok := result.Activations["host2"]; !ok { + t.Error("expected host2 to survive when failover host reservations are ignored at call time") + } +} + +func TestFilterFailoverHost_Index(t *testing.T) { + factory, ok := Index["filter_failover_host"] + if !ok { + t.Fatal("expected filter_failover_host to be registered in Index") + } + if _, ok := factory().(*FilterFailoverHost); !ok { + t.Fatal("expected factory to return a *FilterFailoverHost") + } +} diff --git a/internal/scheduling/nova/plugins/filters/filter_no_failover_host.go b/internal/scheduling/nova/plugins/filters/filter_no_failover_host.go deleted file mode 100644 index a6e9b535a..000000000 --- a/internal/scheduling/nova/plugins/filters/filter_no_failover_host.go +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright SAP SE -// SPDX-License-Identifier: Apache-2.0 - -package filters - -import ( - "context" - "slices" - - "github.com/go-logr/logr" - - api "github.com/cobaltcore-dev/cortex/api/external/nova" - "github.com/cobaltcore-dev/cortex/api/v1alpha1" - "github.com/cobaltcore-dev/cortex/internal/scheduling/lib" -) - -// FilterNoFailoverHostOpts configures the no-failover-host filter. -type FilterNoFailoverHostOpts struct { - // Enabled toggles the filter. When false, the filter is a no-op and all - // hosts pass. This lets the pipeline carry the step while keeping the - // behavior off until the feature is rolled out. - Enabled bool `json:"enabled"` -} - -func (FilterNoFailoverHostOpts) Validate() error { return nil } - -// FilterNoFailoverHost blocks hosts that have a failover host reservation, so no -// new virtual machines are placed on a host that is being drained or kept empty -// as a gang-evacuation landing zone. -// -// This only applies to non-evacuation requests. Evacuation requests are routed -// onto reserved hosts separately, after the pipeline, by the failover host -// Matcher (see reservations/failoverhost/matcher.go), so this filter leaves them -// untouched. -// -// The filter is purely subtractive, so its position in the pipeline does not -// matter: other filters and weighers still run around it. -type FilterNoFailoverHost struct { - lib.BaseFilter[api.ExternalSchedulerRequest, FilterNoFailoverHostOpts] -} - -func (s *FilterNoFailoverHost) Run(traceLog logr.Logger, request api.ExternalSchedulerRequest) (*lib.FilterWeigherPipelineStepResult, error) { - result := s.IncludeAllHostsFromRequest(request) - if !s.Options.Enabled { - return result, nil - } - - // Honor the call-time ignored reservation types. When failover host - // reservations are ignored — as the path finder probe does (see - // reservations/failoverhost/pathfinding.go) — block nothing so reserved - // hosts stay visible to their own partition. - if slices.Contains(request.GetOptions().IgnoredReservationTypes, v1alpha1.ReservationTypeFailoverHost) { - traceLog.V(1).Info("failover host reservations ignored, not blocking any hosts") - return result, nil - } - - // Evacuations are routed onto reserved hosts by the Matcher, not blocked here. - if intent, err := request.GetIntent(); err == nil && intent == api.EvacuateIntent { - traceLog.V(1).Info("evacuation request, leaving hosts untouched for the failover host matcher") - return result, nil - } - - var reservations v1alpha1.ReservationList - if err := s.Client.List(context.Background(), &reservations); err != nil { - return nil, err - } - for _, reservation := range reservations.Items { - if reservation.Spec.Type != v1alpha1.ReservationTypeFailoverHost { - continue - } - // Block both the desired (TargetHost) and actual (Status.Host) host, as - // the capacity filter does, to cover the window while the reservation is - // being placed. A reservation only exists from the Designated stage - // onward (the controller deletes Revoked/Used ones), so its presence is - // enough to keep new virtual machines off the host. - for _, host := range []string{reservation.Spec.TargetHost, reservation.Status.Host} { - if host == "" { - continue - } - delete(result.Activations, host) // noop if host is not in the map - traceLog.V(1).Info("filtering out failover host reservation target", - "host", host, "reservation", reservation.Name) - } - } - return result, nil -} - -func init() { - Index["filter_no_failover_host"] = func() NovaFilter { return &FilterNoFailoverHost{} } -} diff --git a/internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go b/internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go deleted file mode 100644 index c1f343805..000000000 --- a/internal/scheduling/nova/plugins/filters/filter_no_failover_host_test.go +++ /dev/null @@ -1,149 +0,0 @@ -// Copyright SAP SE -// SPDX-License-Identifier: Apache-2.0 - -package filters - -import ( - "testing" - - "github.com/go-logr/logr" - - api "github.com/cobaltcore-dev/cortex/api/external/nova" - "github.com/cobaltcore-dev/cortex/api/scheduling" - "github.com/cobaltcore-dev/cortex/api/v1alpha1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" -) - -func failoverHostReservation(targetHost, statusHost string) *v1alpha1.Reservation { - return &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeFailoverHost, - TargetHost: targetHost, - FailoverHostReservation: &v1alpha1.FailoverHostReservationSpec{}, - }, - Status: v1alpha1.ReservationStatus{Host: statusHost}, - } -} - -func committedResourceReservationOn(targetHost string) *v1alpha1.Reservation { - return &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: "cr-" + targetHost}, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeCommittedResource, - TargetHost: targetHost, - }, - } -} - -func requestWithHosts(hints map[string]any, hosts ...string) api.ExternalSchedulerRequest { - hvHosts := make([]api.ExternalSchedulerHost, len(hosts)) - for i, h := range hosts { - hvHosts[i] = api.ExternalSchedulerHost{ComputeHost: h} - } - return api.ExternalSchedulerRequest{ - Hosts: hvHosts, - Spec: api.NovaObject[api.NovaSpec]{Data: api.NovaSpec{SchedulerHints: hints}}, - } -} - -func TestFilterNoFailoverHost_Run(t *testing.T) { - scheme := buildTestScheme(t) - - tests := []struct { - name string - opts FilterNoFailoverHostOpts - reservations []client.Object - request api.ExternalSchedulerRequest - expectedHosts []string - }{ - { - name: "disabled - all hosts pass even with reservations", - opts: FilterNoFailoverHostOpts{Enabled: false}, - reservations: []client.Object{failoverHostReservation("host2", "")}, - request: requestWithHosts(nil, "host1", "host2", "host3"), - expectedHosts: []string{"host1", "host2", "host3"}, - }, - { - name: "enabled - blocks reserved target host", - opts: FilterNoFailoverHostOpts{Enabled: true}, - reservations: []client.Object{failoverHostReservation("host2", "")}, - request: requestWithHosts(nil, "host1", "host2", "host3"), - expectedHosts: []string{"host1", "host3"}, - }, - { - name: "enabled - blocks both target and status host", - opts: FilterNoFailoverHostOpts{Enabled: true}, - reservations: []client.Object{failoverHostReservation("host3", "host4")}, - request: requestWithHosts(nil, "host1", "host2", "host3", "host4"), - expectedHosts: []string{"host1", "host2"}, - }, - { - name: "enabled - ignores non-failover-host reservations", - opts: FilterNoFailoverHostOpts{Enabled: true}, - reservations: []client.Object{committedResourceReservationOn("host2")}, - request: requestWithHosts(nil, "host1", "host2", "host3"), - expectedHosts: []string{"host1", "host2", "host3"}, - }, - { - name: "evacuation request - leaves hosts untouched for the matcher", - opts: FilterNoFailoverHostOpts{Enabled: true}, - reservations: []client.Object{failoverHostReservation("host2", "")}, - request: requestWithHosts(map[string]any{"_nova_check_type": "evacuate"}, "host1", "host2", "host3"), - expectedHosts: []string{"host1", "host2", "host3"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - step := &FilterNoFailoverHost{} - step.Options = tt.opts - step.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.reservations...).Build() - - result, err := step.Run(logr.Discard(), tt.request) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if len(result.Activations) != len(tt.expectedHosts) { - t.Errorf("expected %d hosts, got %d: %v", len(tt.expectedHosts), len(result.Activations), result.Activations) - } - for _, host := range tt.expectedHosts { - if _, ok := result.Activations[host]; !ok { - t.Errorf("expected host %s to be present", host) - } - } - }) - } -} - -func TestFilterNoFailoverHost_CallTimeIgnoredReservationTypes(t *testing.T) { - scheme := buildTestScheme(t) - step := &FilterNoFailoverHost{} - step.Options = FilterNoFailoverHostOpts{Enabled: true} - step.Client = fake.NewClientBuilder().WithScheme(scheme). - WithObjects(failoverHostReservation("host2", "")).Build() - - request := requestWithHosts(nil, "host1", "host2", "host3") - request.Options = scheduling.Options{ - IgnoredReservationTypes: []v1alpha1.ReservationType{v1alpha1.ReservationTypeFailoverHost}, - } - result, err := step.Run(logr.Discard(), request) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if _, ok := result.Activations["host2"]; !ok { - t.Error("expected host2 to survive when failover host reservations are ignored at call time") - } -} - -func TestFilterNoFailoverHost_Index(t *testing.T) { - factory, ok := Index["filter_no_failover_host"] - if !ok { - t.Fatal("expected filter_no_failover_host to be registered in Index") - } - if _, ok := factory().(*FilterNoFailoverHost); !ok { - t.Fatal("expected factory to return a *FilterNoFailoverHost") - } -} diff --git a/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go new file mode 100644 index 000000000..e42859d7f --- /dev/null +++ b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go @@ -0,0 +1,129 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package weighers + +import ( + "context" + "slices" + "sort" + + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/api/meta" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "github.com/cobaltcore-dev/cortex/internal/scheduling/lib" +) + +// failoverTargetWeight is the activation assigned to the failover host a source +// host should gang-evacuate onto. It is deliberately far larger than any other +// weigher's contribution: the pipeline squashes each step with tanh (bounding it +// to ~[-1, 1]) before summing, so a value this large saturates to ~+1 and, being +// the only host lifted at all, is guaranteed to sort to the front of the +// candidate list. That makes the selected target Nova's first choice while the +// ordinary candidates keep their relative order behind it as retry fallbacks. +const failoverTargetWeight = 1e6 + +// WeigherFailoverHostOpts configures the failover-host weigher. +type WeigherFailoverHostOpts struct { + // Enabled toggles the weigher. When false, the weigher is a no-op and has no + // effect on any host's weight. + Enabled bool `json:"enabled"` +} + +func (WeigherFailoverHostOpts) Validate() error { return nil } + +// WeigherFailoverHost lifts the failover host an evacuation should land on to the +// front of the candidate list, by giving it a dominating weight. +// +// It selects the target with the same deterministic rule as the +// filter_failover_host filter (which has already removed the other reserved +// hosts), so the two agree without coordinating: both run over the same +// reservation snapshot in one pipeline execution. The filter keeps only the +// selected target among the reserved hosts; this weigher then ensures that +// target outranks the retained ordinary candidates. +type WeigherFailoverHost struct { + lib.BaseWeigher[api.ExternalSchedulerRequest, WeigherFailoverHostOpts] +} + +func (s *WeigherFailoverHost) Run(traceLog logr.Logger, request api.ExternalSchedulerRequest) (*lib.FilterWeigherPipelineStepResult, error) { + result := s.IncludeAllHostsFromRequest(request) + if !s.Options.Enabled { + return result, nil + } + intent, err := request.GetIntent() + if err != nil || intent != api.EvacuateIntent { + return result, nil //nolint:nilerr // non-evacuation requests are not weighted + } + sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host") + if err != nil || sourceHost == "" { + return result, nil //nolint:nilerr // without a source host there is nothing to weigh + } + + var reservations v1alpha1.ReservationList + if err := s.Client.List(context.Background(), &reservations); err != nil { + return nil, err + } + target := selectFailoverHost(reservations.Items, sourceHost) + if target == "" { + return result, nil + } + if _, ok := result.Activations[target]; !ok { + // The selected target conflicts with other scheduling constraints and was + // filtered out, so there is nothing to lift. + traceLog.V(1).Info("selected failover host not among candidates, not weighing", + "sourceHost", sourceHost, "targetHost", target) + return result, nil + } + result.Activations[target] = failoverTargetWeight + stats := s.PrepareStats(request, "failover host target [bool]") + stats.Hosts[target] = 1 + result.Statistics["failover host target"] = stats + traceLog.V(1).Info("weighting failover host target to the front", + "sourceHost", sourceHost, "targetHost", target) + return result, nil +} + +// selectFailoverHost picks the target host of the one failover host reservation a +// source host should gang-evacuate onto, deterministically. It is a pure function +// of the reservation snapshot, so the filter and weigher (which run over the same +// snapshot) select the same target without coordinating. +// +// Among the reservations whose partition includes sourceHost it prefers the one +// already in use by this very source (the gang-evacuation path), never picks one +// in use by a different source (which would steal an active landing zone), and +// otherwise takes the first by target host name. Returns "" when nothing matches. +func selectFailoverHost(reservations []v1alpha1.Reservation, sourceHost string) string { + var matching []v1alpha1.Reservation + for _, res := range reservations { + if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { + continue + } + if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { + matching = append(matching, res) + } + } + sort.Slice(matching, func(i, j int) bool { + return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost + }) + var firstFree string + for i := range matching { + res := &matching[i] + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + if firstFree == "" { + firstFree = res.Spec.TargetHost + } + continue + } + if res.Status.FailoverHostReservation != nil && + res.Status.FailoverHostReservation.SourceHost == sourceHost { + return res.Spec.TargetHost + } + } + return firstFree +} + +func init() { + Index["weigher_failover_host"] = func() NovaWeigher { return &WeigherFailoverHost{} } +} diff --git a/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go b/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go new file mode 100644 index 000000000..32397822b --- /dev/null +++ b/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go @@ -0,0 +1,163 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package weighers + +import ( + "testing" + + "github.com/go-logr/logr" + + api "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func failoverHostReservation(targetHost string, sourceHosts ...string) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeFailoverHost, + TargetHost: targetHost, + FailoverHostReservation: &v1alpha1.FailoverHostReservationSpec{ + SourceHosts: sourceHosts, + }, + }, + } +} + +func failoverInUseBy(res *v1alpha1.Reservation, sourceHost string) *v1alpha1.Reservation { + res.Status.FailoverHostReservation = &v1alpha1.FailoverHostReservationStatus{SourceHost: sourceHost} + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostInUse, + Status: metav1.ConditionTrue, + Reason: "EvacuationStarted", + }) + return res +} + +func weigherRequest(hints map[string]any, hosts ...string) api.ExternalSchedulerRequest { + hvHosts := make([]api.ExternalSchedulerHost, len(hosts)) + for i, h := range hosts { + hvHosts[i] = api.ExternalSchedulerHost{ComputeHost: h} + } + return api.ExternalSchedulerRequest{ + Hosts: hvHosts, + Spec: api.NovaObject[api.NovaSpec]{Data: api.NovaSpec{SchedulerHints: hints}}, + } +} + +func evacHints(sourceHost string) map[string]any { + hints := map[string]any{"_nova_check_type": "evacuate"} + if sourceHost != "" { + hints["source_host"] = sourceHost + } + return hints +} + +func TestWeigherFailoverHost_Run(t *testing.T) { + scheme := buildTestScheme(t) + + tests := []struct { + name string + opts WeigherFailoverHostOpts + reservations []client.Object + request api.ExternalSchedulerRequest + // wantTarget is the host expected to receive the dominating weight, or "" + // when no host should be lifted. + wantTarget string + }{ + { + name: "disabled - no effect", + opts: WeigherFailoverHostOpts{Enabled: false}, + reservations: []client.Object{failoverHostReservation("h1", "source1")}, + request: weigherRequest(evacHints("source1"), "h1", "h2"), + wantTarget: "", + }, + { + name: "non-evacuation - no effect", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("h1", "source1")}, + request: weigherRequest(map[string]any{"_nova_check_type": "live_migrate", "source_host": "source1"}, "h1", "h2"), + wantTarget: "", + }, + { + name: "evacuation without source hint - no effect", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("h1", "source1")}, + request: weigherRequest(evacHints(""), "h1", "h2"), + wantTarget: "", + }, + { + name: "evacuation - lifts the name-first matching target", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("h1", "source1"), failoverHostReservation("h2", "source1")}, + request: weigherRequest(evacHints("source1"), "h1", "h2", "h3"), + wantTarget: "h1", + }, + { + name: "evacuation - lifts the in-use target for the same source", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{ + failoverHostReservation("h1", "source1"), + failoverInUseBy(failoverHostReservation("h2", "source1"), "source1"), + }, + request: weigherRequest(evacHints("source1"), "h1", "h2", "h3"), + wantTarget: "h2", + }, + { + name: "evacuation - no matching reservation - no effect", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("h1", "otherSource")}, + request: weigherRequest(evacHints("source1"), "h1", "h2"), + wantTarget: "", + }, + { + name: "evacuation - selected target not among candidates - no effect", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{failoverHostReservation("h1", "source1")}, + request: weigherRequest(evacHints("source1"), "h2", "h3"), + wantTarget: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + step := &WeigherFailoverHost{} + step.Options = tt.opts + step.Client = fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.reservations...).Build() + + result, err := step.Run(logr.Discard(), tt.request) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + for host, weight := range result.Activations { + if host == tt.wantTarget { + if weight != failoverTargetWeight { + t.Errorf("expected target %s to get weight %v, got %v", host, failoverTargetWeight, weight) + } + } else if weight != 0 { + t.Errorf("expected host %s to have no effect (0), got %v", host, weight) + } + } + if tt.wantTarget != "" { + if _, ok := result.Activations[tt.wantTarget]; !ok { + t.Errorf("expected target %s to be present in activations", tt.wantTarget) + } + } + }) + } +} + +func TestWeigherFailoverHost_Index(t *testing.T) { + factory, ok := Index["weigher_failover_host"] + if !ok { + t.Fatal("expected weigher_failover_host to be registered in Index") + } + if _, ok := factory().(*WeigherFailoverHost); !ok { + t.Fatal("expected factory to return a *WeigherFailoverHost") + } +} diff --git a/internal/scheduling/reservations/failoverhost/matcher.go b/internal/scheduling/reservations/failoverhost/matcher.go index eadb37bc0..1ece9d88a 100644 --- a/internal/scheduling/reservations/failoverhost/matcher.go +++ b/internal/scheduling/reservations/failoverhost/matcher.go @@ -17,15 +17,23 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// Matcher routes an evacuation request onto a failover host reservation and -// pins the source host to a single target, implementing gang-evacuation. +// Matcher confirms an evacuation's gang target after the scheduling pipeline has +// run and pins the source host to that target, implementing gang-evacuation. // -// It is called after the scheduling pipeline has produced its ordered -// candidates, from inside the pipeline controller's exclusive lock, so that the -// InUse status write below is serialized against — and visible to — subsequent -// evacuation requests from the same source host. The write is committed only -// once (on the first evacuation from a given source); later requests reuse the -// already-InUse reservation and so land on the same target. +// The candidate shaping — removing the other reserved hosts and lifting the +// selected failover host to the front — is done inside the pipeline by the +// filter_failover_host filter and the weigher_failover_host weigher. The Matcher +// does not touch the candidate list. Its sole job is the stateful write: once the +// pipeline has settled on a target host, it marks the matching reservation InUse +// so that subsequent evacuations from the same source host deterministically land +// on the same target. +// +// It runs from inside the pipeline controller's exclusive lock, on write runs +// only, so the InUse write is serialized against — and visible to — subsequent +// evacuation requests from the same source host. The write is committed only once +// (on the first evacuation from a given source); later requests see the +// already-InUse reservation (the filter/weigher keep selecting it) and so land on +// the same target. // // The Matcher only ever advances a reservation to InUse. Moving it back out // (to Active/Used once the evacuation completes) is owned by the failover host @@ -36,32 +44,31 @@ type Matcher struct { Monitor *MatcherMonitor } -// Match returns the candidate hosts to use for this request. For a -// non-evacuation request, or an evacuation the Matcher cannot or should not -// reroute, it returns orig unchanged. For an evacuation it finds the failover -// host reservations whose SourceHosts include the request's source host and -// returns the chosen failover host as the sole candidate, writing the InUse -// condition when it pins a fresh one. +// ConfirmAndMarkInUse inspects the pipeline's ordered candidates for an evacuation +// request and, if the top candidate is the failover host reservation selected for +// the request's source host, marks that reservation InUse. // -// Fallbacks preserve a valid placement: if no reservation matches, or the -// chosen host is not among orig (so it conflicts with other scheduling -// constraints), orig is returned unchanged so the VM still has candidates. -func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerRequest, originalHosts []string) ([]string, error) { +// It is a no-op for non-evacuation requests and for evacuations without a source +// host. When no failover host reservation matches the source host — or the +// selected one did not end up as the top candidate (it was filtered out by other +// constraints, so the evacuation falls back to an ordinary host) — it records the +// evacuation-without-reservation metric and writes nothing. +func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalSchedulerRequest, orderedHosts []string) error { log := logf.FromContext(ctx) intent, err := request.GetIntent() if err != nil || intent != api.EvacuateIntent { - return originalHosts, nil //nolint:nilerr // non-evacuation requests are not rerouted + return nil //nolint:nilerr // non-evacuation requests are not matched } sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host") if err != nil || sourceHost == "" { - log.V(1).Info("no source_host hint on evacuation, leaving candidates untouched") - return originalHosts, nil //nolint:nilerr // without a source host there is nothing to match + log.V(1).Info("no source_host hint on evacuation, nothing to confirm") + return nil //nolint:nilerr // without a source host there is nothing to match } var reservations v1alpha1.ReservationList if err := m.List(ctx, &reservations); err != nil { - return nil, err + return err } // Collect the failover host reservations whose partition includes the source // host, i.e. the reservations this host may evacuate onto. @@ -75,70 +82,64 @@ func (m *Matcher) Match(ctx context.Context, request api.ExternalSchedulerReques } } if len(matching) == 0 { - log.V(1).Info("no failover host reservation for source host, falling back to ordinary candidates", - "sourceHost", sourceHost) + log.V(1).Info("no failover host reservation for source host", "sourceHost", sourceHost) m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) - return originalHosts, nil + return nil } - // Sort deterministically by target host so selection is stable and testable; - // the InUse write below is what makes a fresh choice binding for subsequent - // requests from the same source. + // Re-derive the selected reservation with the same deterministic rule the + // filter and weigher used, so we confirm the one they shaped the candidates + // around. Sort by target host so selection is stable. sort.Slice(matching, func(i, j int) bool { return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost }) - - // Classify each candidate reservation by its InUse status. A reservation can - // legitimately rescue many source hosts (the planner lists every partition - // peer), so more than one source may evacuate concurrently: one reservation - // per source. We must therefore (a) reuse the reservation already in use by - // *this* source, and (b) never pin a reservation already in use by a - // *different* source, which would steal an active landing zone. - var free *v1alpha1.Reservation + var selected *v1alpha1.Reservation for i := range matching { res := &matching[i] if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if free == nil { - free = res + if selected == nil { + selected = res } continue } - // In use: reuse it only if it is in use by this very source host, which - // is the gang-evacuation path — the whole gang lands on the same target. + // In use: reuse it only if it is in use by this very source host, which is + // the gang-evacuation path — the whole gang lands on the same target. if res.Status.FailoverHostReservation != nil && res.Status.FailoverHostReservation.SourceHost == sourceHost { - log.V(1).Info("reusing in-use failover host reservation for gang-evacuation", - "sourceHost", sourceHost, "reservation", res.Name, "targetHost", res.Spec.TargetHost) - return m.pick(*res, originalHosts), nil + selected = res + break } } - - // None already in use by this source. Pin the first free reservation. - if free == nil { + if selected == nil { // Every matching reservation is already absorbing another source host's - // gang. Fall back to ordinary candidates rather than stealing a landing - // zone, and surface it like the no-reservation case. - log.V(1).Info("all failover host reservations for source host are in use by other sources, falling back", + // gang. Surface it like the no-reservation case. + log.V(1).Info("all failover host reservations for source host are in use by other sources", "sourceHost", sourceHost) m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) - return originalHosts, nil - } - if err := m.markInUse(ctx, free, sourceHost); err != nil { - return nil, err + return nil } - log.V(1).Info("pinned failover host reservation for evacuation", - "sourceHost", sourceHost, "reservation", free.Name, "targetHost", free.Spec.TargetHost) - return m.pick(*free, originalHosts), nil -} -// pick returns the reservation's target host as the sole candidate, unless that -// host is not among the pipeline's candidates (it conflicts with other -// scheduling constraints), in which case it falls back to the original set. -func (m *Matcher) pick(res v1alpha1.Reservation, orig []string) []string { - if slices.Contains(orig, res.Spec.TargetHost) { - return []string{res.Spec.TargetHost} + // Only pin when the pipeline actually settled on the selected target. If it is + // not the top candidate (filtered out by other constraints, so the evacuation + // falls back to an ordinary host), we must not leave a dangling InUse. + if len(orderedHosts) == 0 || orderedHosts[0] != selected.Spec.TargetHost { + log.V(1).Info("selected failover host is not the top candidate, not pinning", + "sourceHost", sourceHost, "targetHost", selected.Spec.TargetHost) + m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) + return nil + } + // Already in use by this source (gang-evacuation reuse): nothing to write. + if meta.IsStatusConditionTrue(selected.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + log.V(1).Info("failover host reservation already in use for this source, reusing", + "sourceHost", sourceHost, "reservation", selected.Name, "targetHost", selected.Spec.TargetHost) + return nil } - return orig + if err := m.markInUse(ctx, selected, sourceHost); err != nil { + return err + } + log.V(1).Info("pinned failover host reservation for evacuation", + "sourceHost", sourceHost, "reservation", selected.Name, "targetHost", selected.Spec.TargetHost) + return nil } // markInUse sets the InUse condition and records the source host on the diff --git a/internal/scheduling/reservations/failoverhost/matcher_test.go b/internal/scheduling/reservations/failoverhost/matcher_test.go index a60ab7085..8c28340d3 100644 --- a/internal/scheduling/reservations/failoverhost/matcher_test.go +++ b/internal/scheduling/reservations/failoverhost/matcher_test.go @@ -5,7 +5,6 @@ package failoverhost import ( "context" - "slices" "testing" api "github.com/cobaltcore-dev/cortex/api/external/nova" @@ -55,23 +54,30 @@ func evacuationRequest(sourceHost string) api.ExternalSchedulerRequest { } } -func getReservation(t *testing.T, c client.Client) v1alpha1.Reservation { +func getReservation(t *testing.T, c client.Client, name string) v1alpha1.Reservation { t.Helper() var res v1alpha1.Reservation - if err := c.Get(context.Background(), types.NamespacedName{Name: "failover-host-target1"}, &res); err != nil { + if err := c.Get(context.Background(), types.NamespacedName{Name: name}, &res); err != nil { t.Fatalf("failed to get reservation: %v", err) } return res } -func TestMatcher_Match(t *testing.T) { +func isInUseBy(res v1alpha1.Reservation, sourceHost string) bool { + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + return false + } + return res.Status.FailoverHostReservation != nil && + res.Status.FailoverHostReservation.SourceHost == sourceHost +} + +func TestMatcher_ConfirmAndMarkInUse(t *testing.T) { tests := []struct { name string reservations []*v1alpha1.Reservation request api.ExternalSchedulerRequest - orig []string - want []string - // check runs after Match to assert reservation state in the client. + orderedHosts []string + // check runs after the call to assert reservation state in the client. check func(t *testing.T, c client.Client) }{ { @@ -82,96 +88,110 @@ func TestMatcher_Match(t *testing.T) { SchedulerHints: map[string]any{"_nova_check_type": "live_migrate", "source_host": "source1"}, }}, }, - orig: []string{"host1", "host2"}, - want: []string{"host1", "host2"}, + orderedHosts: []string{"target1", "host2"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c, "failover-host-target1") + if meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + t.Error("expected target1 to not be in use for a non-evacuation request") + } + }, }, { name: "evacuation without source hint is a no-op", reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, request: evacuationRequest(""), - orig: []string{"host1", "host2"}, - want: []string{"host1", "host2"}, + orderedHosts: []string{"target1", "host2"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c, "failover-host-target1") + if meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + t.Error("expected target1 to not be in use without a source hint") + } + }, }, { - name: "no matching reservation falls back to original candidates", - reservations: []*v1alpha1.Reservation{matcherReservation("target1", "otherSource")}, + name: "pins the selected reservation when it is the top candidate", + reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, request: evacuationRequest("source1"), - orig: []string{"host1", "host2"}, - want: []string{"host1", "host2"}, + orderedHosts: []string{"target1", "host1", "host2"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c, "failover-host-target1") + if !isInUseBy(res, "source1") { + t.Errorf("expected target1 to be in use by source1, got %+v", res.Status.FailoverHostReservation) + } + }, }, { - name: "pins a fresh reservation and writes InUse", + name: "does not pin when the selected target is not the top candidate", reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, request: evacuationRequest("source1"), - orig: []string{"host1", "target1", "host2"}, - want: []string{"target1"}, + // target1 lost to an ordinary host (e.g. filtered out by other + // constraints), so the evacuation falls back and must not pin. + orderedHosts: []string{"host1", "host2"}, check: func(t *testing.T, c client.Client) { - res := getReservation(t, c) - if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - t.Error("expected InUse condition to be True") - } - if res.Status.FailoverHostReservation == nil || res.Status.FailoverHostReservation.SourceHost != "source1" { - t.Errorf("expected status SourceHost=source1, got %+v", res.Status.FailoverHostReservation) + res := getReservation(t, c, "failover-host-target1") + if meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + t.Error("expected target1 to NOT be pinned when it is not the top candidate") } }, }, { - name: "reuses the in-use reservation for the same source", - // A second, not-in-use reservation (target0 sorts first) must be - // ignored in favour of the one already in use by this source. + name: "reuses the in-use reservation for the same source, no new write", reservations: []*v1alpha1.Reservation{ inUse(matcherReservation("target1", "source1"), "source1"), matcherReservation("target0", "source1"), }, - request: evacuationRequest("source1"), - orig: []string{"target0", "target1", "host2"}, - want: []string{"target1"}, + request: evacuationRequest("source1"), + orderedHosts: []string{"target1", "host2"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c, "failover-host-target1") + if !isInUseBy(res, "source1") { + t.Errorf("expected target1 to stay in use by source1, got %+v", res.Status.FailoverHostReservation) + } + // target0 must remain untouched. + res0 := getReservation(t, c, "failover-host-target0") + if meta.IsStatusConditionTrue(res0.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + t.Error("expected target0 to remain free") + } + }, }, { name: "does not steal a reservation in use by another source", - // source2 evacuates while source1 already holds target1, so source2 - // must pin the free target2 and leave target1's pin intact. reservations: []*v1alpha1.Reservation{ inUse(matcherReservation("target1", "source1", "source2"), "source1"), matcherReservation("target2", "source1", "source2"), }, - request: evacuationRequest("source2"), - orig: []string{"target1", "target2", "host3"}, - want: []string{"target2"}, + request: evacuationRequest("source2"), + orderedHosts: []string{"target2", "host3"}, check: func(t *testing.T, c client.Client) { - res := getReservation(t, c) - if res.Status.FailoverHostReservation == nil || res.Status.FailoverHostReservation.SourceHost != "source1" { - t.Errorf("expected target1 to stay pinned to source1, got %+v", res.Status.FailoverHostReservation) + res1 := getReservation(t, c, "failover-host-target1") + if !isInUseBy(res1, "source1") { + t.Errorf("expected target1 to stay pinned to source1, got %+v", res1.Status.FailoverHostReservation) + } + res2 := getReservation(t, c, "failover-host-target2") + if !isInUseBy(res2, "source2") { + t.Errorf("expected target2 to be pinned to source2, got %+v", res2.Status.FailoverHostReservation) } }, }, { - name: "all reservations in use by others falls back", + name: "all reservations in use by others is a no-op", reservations: []*v1alpha1.Reservation{ inUse(matcherReservation("target1", "source1", "source2"), "source1"), }, - request: evacuationRequest("source2"), - orig: []string{"target1", "host3"}, - want: []string{"target1", "host3"}, + request: evacuationRequest("source2"), + orderedHosts: []string{"host3"}, check: func(t *testing.T, c client.Client) { - res := getReservation(t, c) + res := getReservation(t, c, "failover-host-target1") if res.Status.FailoverHostReservation.SourceHost != "source1" { t.Errorf("expected target1 to stay pinned to source1, got %q", res.Status.FailoverHostReservation.SourceHost) } }, }, { - name: "chosen host not in candidates falls back but still pins", - reservations: []*v1alpha1.Reservation{matcherReservation("target1", "source1")}, + name: "no matching reservation is a no-op", + reservations: []*v1alpha1.Reservation{matcherReservation("target1", "otherSource")}, request: evacuationRequest("source1"), - orig: []string{"host1", "host2"}, // target1 absent: conflicts with other constraints - want: []string{"host1", "host2"}, - check: func(t *testing.T, c client.Client) { - res := getReservation(t, c) - if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - t.Error("expected InUse condition to be True even on candidate fallback") - } - }, + orderedHosts: []string{"host1", "host2"}, }, } for _, tt := range tests { @@ -182,13 +202,9 @@ func TestMatcher_Match(t *testing.T) { } c := newTestClient(t, objs...) m := &Matcher{Client: c, Monitor: NewMatcherMonitor()} - got, err := m.Match(context.Background(), tt.request, tt.orig) - if err != nil { + if err := m.ConfirmAndMarkInUse(context.Background(), tt.request, tt.orderedHosts); err != nil { t.Fatalf("unexpected error: %v", err) } - if !slices.Equal(got, tt.want) { - t.Fatalf("got %v, want %v", got, tt.want) - } if tt.check != nil { tt.check(t, c) } From 685bcdd3ccaf7560603adb6bb31b85f28ea86d64 Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Wed, 4 Nov 2026 13:31:43 +0100 Subject: [PATCH 4/7] refactor(nova): rename failover flag, inline host selection - Rename the Helm toggle kvm.failoverHost.enabled to kvm.failoverHostMatching.enabled (names the behavior, not the resource). - Rename matcher.go/matcher_test.go to matching.go/matching_test.go. - Inline the deterministic host-selection rule directly into each call site (filter, weigher, and the post-pipeline matcher) rather than a shared helper. The plugin packages cannot import failoverhost (its in-package integration test imports them, which would cycle), so the small sort-by-name rule is duplicated inline in each, keeping every step self-contained. Signed-off-by: Philipp Matthes --- .../04-failover-host-reservations.md | 2 +- .../cortex-nova/templates/pipelines_kvm.yaml | 12 +-- helm/bundles/cortex-nova/values.yaml | 14 +-- .../plugins/filters/filter_failover_host.go | 90 +++++++++---------- .../plugins/weighers/weigher_failover_host.go | 68 +++++++------- .../failoverhost/{matcher.go => matching.go} | 26 ++---- .../{matcher_test.go => matching_test.go} | 0 7 files changed, 94 insertions(+), 118 deletions(-) rename internal/scheduling/reservations/failoverhost/{matcher.go => matching.go} (87%) rename internal/scheduling/reservations/failoverhost/{matcher_test.go => matching_test.go} (100%) diff --git a/docs/03-reservations-and-inventory/04-failover-host-reservations.md b/docs/03-reservations-and-inventory/04-failover-host-reservations.md index f7163c529..84051f1f8 100644 --- a/docs/03-reservations-and-inventory/04-failover-host-reservations.md +++ b/docs/03-reservations-and-inventory/04-failover-host-reservations.md @@ -503,7 +503,7 @@ snapshot within one pipeline run, so they always agree without coordinating: The **`filter_failover_host`** pipeline filter is purely *subtractive* — it only ever removes hosts — so its position in the pipeline does not matter and all other filters and weighers still apply around it. It is -toggled by the `kvm.failoverHost.enabled` Helm value (default `false`) and is a no-op when disabled. What it +toggled by the `kvm.failoverHostMatching.enabled` Helm value (default `false`) and is a no-op when disabled. What it removes depends on the request: - **Ordinary (non-evacuation) requests:** it removes *every* failover host, so no new VM is placed on a host diff --git a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml index 6b6cb8952..e2e3f3ab3 100644 --- a/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml +++ b/helm/bundles/cortex-nova/templates/pipelines_kvm.yaml @@ -96,7 +96,7 @@ spec: It honors the call-time ignoredReservationTypes option so the failover host path finder probe still discovers reserved hosts. params: - - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHostMatching.enabled }}} - name: filter_allowed_projects description: | This step filters hosts based on allowed projects defined in the @@ -179,7 +179,7 @@ spec: host the filter_failover_host filter kept. For non-evacuation requests it has no effect. params: - - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHostMatching.enabled }}} - name: kvm_committed_resource_reservation description: | This weigher boosts hosts that have a ready CommittedResourceReservation @@ -293,7 +293,7 @@ spec: It honors the call-time ignoredReservationTypes option so the failover host path finder probe still discovers reserved hosts. params: - - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHostMatching.enabled }}} - name: filter_allowed_projects description: | This step filters hosts based on allowed projects defined in the @@ -373,7 +373,7 @@ spec: host the filter_failover_host filter kept. For non-evacuation requests it has no effect. params: - - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHostMatching.enabled }}} - name: kvm_committed_resource_reservation description: | This weigher boosts hosts that have a ready CommittedResourceReservation @@ -640,7 +640,7 @@ spec: It honors the call-time ignoredReservationTypes option so the failover host path finder probe still discovers reserved hosts. params: - - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHostMatching.enabled }}} - name: filter_has_requested_traits description: | This step filters hosts that do not have the requested traits given by the @@ -690,7 +690,7 @@ spec: host the filter_failover_host filter kept. For non-evacuation requests it has no effect. params: - - {key: enabled, boolValue: {{ .Values.kvm.failoverHost.enabled }}} + - {key: enabled, boolValue: {{ .Values.kvm.failoverHostMatching.enabled }}} --- apiVersion: cortex.cloud/v1alpha1 kind: Pipeline diff --git a/helm/bundles/cortex-nova/values.yaml b/helm/bundles/cortex-nova/values.yaml index 416eb5305..d82d0e280 100644 --- a/helm/bundles/cortex-nova/values.yaml +++ b/helm/bundles/cortex-nova/values.yaml @@ -109,13 +109,13 @@ kvm: # Domain names that are explicitly treated as non-external-customer domains and # skipped by the filter, even if they match one of the prefixes above. ignoredDomainNames: [] - # Options for the failover host scheduling steps. The filter_failover_host - # filter keeps new VMs off hosts with a failover host reservation (and, for - # evacuations, prunes all but the deterministically selected failover host so - # another failure keeps a free landing zone); the weigher_failover_host weigher - # lifts that selected host to the front of the candidate list. Applied to every - # KVM pipeline that uses the steps. - failoverHost: + # Options for the failover host matching scheduling steps. The + # filter_failover_host filter keeps new VMs off hosts with a failover host + # reservation (and, for evacuations, prunes all but the deterministically + # selected failover host so another failure keeps a free landing zone); the + # weigher_failover_host weigher lifts that selected host to the front of the + # candidate list. Applied to every KVM pipeline that uses the steps. + failoverHostMatching: # Disabled by default until the failover host draining controller logic is # implemented. Set to true to enable failover-host-aware scheduling. enabled: false diff --git a/internal/scheduling/nova/plugins/filters/filter_failover_host.go b/internal/scheduling/nova/plugins/filters/filter_failover_host.go index 1d81acc1b..0cfc05b3a 100644 --- a/internal/scheduling/nova/plugins/filters/filter_failover_host.go +++ b/internal/scheduling/nova/plugins/filters/filter_failover_host.go @@ -35,12 +35,12 @@ func (FilterFailoverHostOpts) Validate() error { return nil } // landing zone. // // For an evacuation request it removes every failover host except the one -// deterministically selected for the request's source host (see selectFailoverHost). -// Keeping only the selected target — and leaving the ordinary, non-reserved -// candidates untouched — means the other reserved hosts stay free for a second -// host failure, while Nova can still retry onto an ordinary candidate if the -// evacuation onto the selected target fails. The weigher_failover_host weigher -// then lifts the selected target to the front of the candidate list. +// deterministically selected for the request's source host. Keeping only the +// selected target — and leaving the ordinary, non-reserved candidates untouched — +// means the other reserved hosts stay free for a second host failure, while Nova +// can still retry onto an ordinary candidate if the evacuation onto the selected +// target fails. The weigher_failover_host weigher then lifts the selected target +// to the front of the candidate list, applying the same selection rule. // // The filter is purely subtractive, so its position in the pipeline does not // matter: other filters and weighers still run around it. @@ -68,12 +68,43 @@ func (s *FilterFailoverHost) Run(traceLog logr.Logger, request api.ExternalSched return nil, err } - // For an evacuation, keep the single failover host selected for this source - // so the gang lands there; for everything else, keep none. + // For an evacuation, keep the single failover host selected for this source so + // the gang lands there; for everything else, keep none. The selection is a + // pure function of the reservation snapshot — among the reservations whose + // partition includes the source host, prefer the one already in use by this + // very source (gang-evacuation reuse), never one in use by a different source + // (that would steal an active landing zone), otherwise the first by target + // host name — so the weigher_failover_host weigher reaches the same target + // over the same snapshot without coordinating. var keep string if intent, err := request.GetIntent(); err == nil && intent == api.EvacuateIntent { if sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host"); err == nil && sourceHost != "" { - keep = selectFailoverHost(reservations.Items, sourceHost) + var matching []v1alpha1.Reservation + for _, res := range reservations.Items { + if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { + continue + } + if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { + matching = append(matching, res) + } + } + sort.Slice(matching, func(i, j int) bool { + return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost + }) + for i := range matching { + res := &matching[i] + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + if keep == "" { + keep = res.Spec.TargetHost + } + continue + } + if res.Status.FailoverHostReservation != nil && + res.Status.FailoverHostReservation.SourceHost == sourceHost { + keep = res.Spec.TargetHost + break + } + } } } @@ -98,47 +129,6 @@ func (s *FilterFailoverHost) Run(traceLog logr.Logger, request api.ExternalSched return result, nil } -// selectFailoverHost picks the target host of the one failover host reservation a -// source host should gang-evacuate onto, deterministically. It is a pure function -// of the reservation snapshot, so the filter and weigher (which run over the same -// snapshot) select the same target without coordinating. -// -// Among the reservations whose partition includes sourceHost it prefers the one -// already in use by this very source (the gang-evacuation path — the whole gang -// lands on the same target), never picks one in use by a different source (which -// would steal an active landing zone), and otherwise takes the first by target -// host name. Returns "" when nothing matches. -func selectFailoverHost(reservations []v1alpha1.Reservation, sourceHost string) string { - var matching []v1alpha1.Reservation - for _, res := range reservations { - if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { - continue - } - if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { - matching = append(matching, res) - } - } - sort.Slice(matching, func(i, j int) bool { - return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost - }) - var firstFree string - for i := range matching { - res := &matching[i] - if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if firstFree == "" { - firstFree = res.Spec.TargetHost - } - continue - } - // In use: reuse it only if it is in use by this very source host. - if res.Status.FailoverHostReservation != nil && - res.Status.FailoverHostReservation.SourceHost == sourceHost { - return res.Spec.TargetHost - } - } - return firstFree -} - func init() { Index["filter_failover_host"] = func() NovaFilter { return &FilterFailoverHost{} } } diff --git a/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go index e42859d7f..5d5ef2948 100644 --- a/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go +++ b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go @@ -41,8 +41,8 @@ func (WeigherFailoverHostOpts) Validate() error { return nil } // filter_failover_host filter (which has already removed the other reserved // hosts), so the two agree without coordinating: both run over the same // reservation snapshot in one pipeline execution. The filter keeps only the -// selected target among the reserved hosts; this weigher then ensures that -// target outranks the retained ordinary candidates. +// selected target among the reserved hosts; this weigher then ensures that target +// outranks the retained ordinary candidates. type WeigherFailoverHost struct { lib.BaseWeigher[api.ExternalSchedulerRequest, WeigherFailoverHostOpts] } @@ -65,38 +65,15 @@ func (s *WeigherFailoverHost) Run(traceLog logr.Logger, request api.ExternalSche if err := s.Client.List(context.Background(), &reservations); err != nil { return nil, err } - target := selectFailoverHost(reservations.Items, sourceHost) - if target == "" { - return result, nil - } - if _, ok := result.Activations[target]; !ok { - // The selected target conflicts with other scheduling constraints and was - // filtered out, so there is nothing to lift. - traceLog.V(1).Info("selected failover host not among candidates, not weighing", - "sourceHost", sourceHost, "targetHost", target) - return result, nil - } - result.Activations[target] = failoverTargetWeight - stats := s.PrepareStats(request, "failover host target [bool]") - stats.Hosts[target] = 1 - result.Statistics["failover host target"] = stats - traceLog.V(1).Info("weighting failover host target to the front", - "sourceHost", sourceHost, "targetHost", target) - return result, nil -} -// selectFailoverHost picks the target host of the one failover host reservation a -// source host should gang-evacuate onto, deterministically. It is a pure function -// of the reservation snapshot, so the filter and weigher (which run over the same -// snapshot) select the same target without coordinating. -// -// Among the reservations whose partition includes sourceHost it prefers the one -// already in use by this very source (the gang-evacuation path), never picks one -// in use by a different source (which would steal an active landing zone), and -// otherwise takes the first by target host name. Returns "" when nothing matches. -func selectFailoverHost(reservations []v1alpha1.Reservation, sourceHost string) string { + // Select the one failover host this source should gang-evacuate onto, with the + // same deterministic rule the filter_failover_host filter used over the same + // snapshot: among the reservations whose partition includes the source host, + // prefer the one already in use by this very source (gang-evacuation reuse), + // never one in use by a different source, otherwise the first by target host + // name. var matching []v1alpha1.Reservation - for _, res := range reservations { + for _, res := range reservations.Items { if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { continue } @@ -107,21 +84,38 @@ func selectFailoverHost(reservations []v1alpha1.Reservation, sourceHost string) sort.Slice(matching, func(i, j int) bool { return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost }) - var firstFree string + var target string for i := range matching { res := &matching[i] if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if firstFree == "" { - firstFree = res.Spec.TargetHost + if target == "" { + target = res.Spec.TargetHost } continue } if res.Status.FailoverHostReservation != nil && res.Status.FailoverHostReservation.SourceHost == sourceHost { - return res.Spec.TargetHost + target = res.Spec.TargetHost + break } } - return firstFree + if target == "" { + return result, nil + } + if _, ok := result.Activations[target]; !ok { + // The selected target conflicts with other scheduling constraints and was + // filtered out, so there is nothing to lift. + traceLog.V(1).Info("selected failover host not among candidates, not weighing", + "sourceHost", sourceHost, "targetHost", target) + return result, nil + } + result.Activations[target] = failoverTargetWeight + stats := s.PrepareStats(request, "failover host target [bool]") + stats.Hosts[target] = 1 + result.Statistics["failover host target"] = stats + traceLog.V(1).Info("weighting failover host target to the front", + "sourceHost", sourceHost, "targetHost", target) + return result, nil } func init() { diff --git a/internal/scheduling/reservations/failoverhost/matcher.go b/internal/scheduling/reservations/failoverhost/matching.go similarity index 87% rename from internal/scheduling/reservations/failoverhost/matcher.go rename to internal/scheduling/reservations/failoverhost/matching.go index 1ece9d88a..a7af9a364 100644 --- a/internal/scheduling/reservations/failoverhost/matcher.go +++ b/internal/scheduling/reservations/failoverhost/matching.go @@ -70,8 +70,12 @@ func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalS if err := m.List(ctx, &reservations); err != nil { return err } - // Collect the failover host reservations whose partition includes the source - // host, i.e. the reservations this host may evacuate onto. + // Re-derive the selected reservation with the same deterministic rule the + // filter and weigher used over the same snapshot, so we confirm the one they + // shaped the candidates around: among the reservations whose partition + // includes the source host, prefer the one already in use by this very source + // (gang-evacuation reuse), never one in use by a different source (that would + // steal an active landing zone), otherwise the first by target host name. var matching []v1alpha1.Reservation for _, res := range reservations.Items { if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { @@ -81,15 +85,6 @@ func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalS matching = append(matching, res) } } - if len(matching) == 0 { - log.V(1).Info("no failover host reservation for source host", "sourceHost", sourceHost) - m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) - return nil - } - - // Re-derive the selected reservation with the same deterministic rule the - // filter and weigher used, so we confirm the one they shaped the candidates - // around. Sort by target host so selection is stable. sort.Slice(matching, func(i, j int) bool { return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost }) @@ -102,8 +97,6 @@ func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalS } continue } - // In use: reuse it only if it is in use by this very source host, which is - // the gang-evacuation path — the whole gang lands on the same target. if res.Status.FailoverHostReservation != nil && res.Status.FailoverHostReservation.SourceHost == sourceHost { selected = res @@ -111,10 +104,9 @@ func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalS } } if selected == nil { - // Every matching reservation is already absorbing another source host's - // gang. Surface it like the no-reservation case. - log.V(1).Info("all failover host reservations for source host are in use by other sources", - "sourceHost", sourceHost) + // Either no reservation can rescue this source, or every matching one is + // already absorbing another source host's gang. Surface it either way. + log.V(1).Info("no selectable failover host reservation for source host", "sourceHost", sourceHost) m.Monitor.RecordEvacuationWithoutReservation(request.Spec.Data.AvailabilityZone) return nil } diff --git a/internal/scheduling/reservations/failoverhost/matcher_test.go b/internal/scheduling/reservations/failoverhost/matching_test.go similarity index 100% rename from internal/scheduling/reservations/failoverhost/matcher_test.go rename to internal/scheduling/reservations/failoverhost/matching_test.go From 7d59bf0ee821004fe7ca5d735155a1c3aab82270 Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Wed, 4 Nov 2026 13:55:03 +0100 Subject: [PATCH 5/7] feat(nova): require Active before selecting a failover target Only select a failover host reservation as an evacuation target once it is Active (host fully drained and empty). A not-yet-Active reservation (freshly created, still draining) is still blocked for ordinary placements but is no longer routed an evacuation onto a host that is not actually empty. The already-InUse gang-evacuation reuse path is unchanged, and when the feature is disabled nothing changes for existing scheduling. Apply the Active gate consistently in the filter, the weigher and the post-pipeline matcher. Also remove the FailoverHostDesignated status condition: the draining stage is simply the absence of FailoverHostActive=True, the same as a freshly created reservation, so a dedicated condition is redundant. Signed-off-by: Philipp Matthes --- api/v1alpha1/reservation_types.go | 25 ++++------ .../04-failover-host-reservations.md | 18 +++++-- .../plugins/filters/filter_failover_host.go | 20 +++++--- .../filters/filter_failover_host_test.go | 48 +++++++++++++++++-- .../plugins/weighers/weigher_failover_host.go | 7 +-- .../weighers/weigher_failover_host_test.go | 38 ++++++++++++++- .../reservations/failoverhost/matching.go | 6 ++- .../failoverhost/matching_test.go | 38 +++++++++++++-- 8 files changed, 162 insertions(+), 38 deletions(-) diff --git a/api/v1alpha1/reservation_types.go b/api/v1alpha1/reservation_types.go index 34ee24f10..b14f8a018 100644 --- a/api/v1alpha1/reservation_types.go +++ b/api/v1alpha1/reservation_types.go @@ -35,11 +35,11 @@ const ( // its workloads to a designated spot. If this type is set, the reservation // will follow this lifecycle: // - // 1. Designation. The reservation has been created on a host. There may be - // virtual machines running on this host that need live migration away. As - // long as not all virtual machines can be migrated away from the host, the - // reservation stays in this state. For pipeline: no new virtual machine - // onto this host. + // 1. Designation. The reservation has been created on a host, but is not yet + // Active (it has no FailoverHostActive=True condition). There may be virtual + // machines running on this host that need live migration away. As long as not + // all virtual machines can be migrated away from the host, the reservation + // stays in this state. For pipeline: no new virtual machine onto this host. // // 2. Active. No more virtual machines on this host. On evacuation this // space can be used. @@ -308,22 +308,15 @@ type FailoverHostReservationStatus struct { type ReservationConditionType = string const ( - // ConditionTypeFailoverHostDesignated indicates that the reservation has - // been created on a host. There may be virtual machines running on this - // host that need live migration away. As long as not all virtual machines - // can be migrated away from the host, the reservation stays in this state. - // For pipeline: no new virtual machine onto this host. - // - // status=Unknown: It is unknown if the failover host has been designated. - // status=False: The failover host has not been designated (there was an error). - // status=True: The failover host has been designated successfully. - ConditionTypeFailoverHostDesignated ReservationConditionType = "FailoverHostDesignated" - // ConditionTypeFailoverHostActive indicates that the failover host is // active and ready to accept evacuations. No more virtual machines have // been seen on this host, so it is empty and ready to receive all vms // from another host. // + // A reservation without this condition set to True (including a freshly + // created one) represents the draining stage: it already blocks new virtual + // machines from the host, but is not yet a selectable evacuation target. + // // status=Unknown: The failover host is not yet ready to be used. // status=False: There has been some error or issue preventing the failover // host from being active. diff --git a/docs/03-reservations-and-inventory/04-failover-host-reservations.md b/docs/03-reservations-and-inventory/04-failover-host-reservations.md index 84051f1f8..5ced35d17 100644 --- a/docs/03-reservations-and-inventory/04-failover-host-reservations.md +++ b/docs/03-reservations-and-inventory/04-failover-host-reservations.md @@ -460,8 +460,11 @@ stateDiagram-v2 ``` - **Designated** — the host has been chosen as a target but may still have VMs that need draining off it. No - *new* VMs are placed here. It stays in this stage until the host is empty. -- **Active** — the host is empty and ready to absorb an evacuation. + *new* VMs are placed here. It stays in this stage until the host is empty. This is the *absence* of the + `FailoverHostActive` condition rather than a condition of its own: a freshly created reservation is + Designated by default. +- **Active** — the host is empty and ready to absorb an evacuation (the `FailoverHostActive` condition is + True). Only from here on can an evacuation be routed onto it. - **Failed** — draining failed repeatedly or took too long; the host is held back from being re-chosen for a cooldown period. - **In Use** — a gang evacuation onto this host is underway (and may absorb further failures until it @@ -496,9 +499,18 @@ snapshot within one pipeline run, so they always agree without coordinating: 2. If one is already **In Use** *by this same source host*, that is the selected target (the gang-evacuation path — the whole gang lands on the same target). 3. Otherwise ignore any reservation **In Use** by a *different* source (pinning it would steal a landing - zone actively absorbing another host's gang), sort the rest by target host name, and take the first. + zone actively absorbing another host's gang), and any reservation that is not yet **Active** (still + draining, so not a usable landing zone yet — see the lifecycle below), then sort the rest by target + host name and take the first. 4. If nothing remains, there is no match and the evacuation simply uses the ordinary candidates. +> [!NOTE] +> A reservation only becomes a *selectable* evacuation target once it is **Active** (its host has been +> fully drained). Before that — from the moment it is created, while it is still draining — it already +> **blocks** ordinary placements (the filter removes it for non-evacuation requests), but it is skipped by +> step 3 above. This keeps VMs off a host that is being emptied without yet routing an evacuation onto a +> host that is not actually empty. + ### The filter: block reserved hosts, keep only the selected target The **`filter_failover_host`** pipeline filter is purely *subtractive* — it only ever removes hosts — so its diff --git a/internal/scheduling/nova/plugins/filters/filter_failover_host.go b/internal/scheduling/nova/plugins/filters/filter_failover_host.go index 0cfc05b3a..6251c33fc 100644 --- a/internal/scheduling/nova/plugins/filters/filter_failover_host.go +++ b/internal/scheduling/nova/plugins/filters/filter_failover_host.go @@ -73,9 +73,15 @@ func (s *FilterFailoverHost) Run(traceLog logr.Logger, request api.ExternalSched // pure function of the reservation snapshot — among the reservations whose // partition includes the source host, prefer the one already in use by this // very source (gang-evacuation reuse), never one in use by a different source - // (that would steal an active landing zone), otherwise the first by target - // host name — so the weigher_failover_host weigher reaches the same target - // over the same snapshot without coordinating. + // (that would steal an active landing zone), otherwise the first Active one + // by target host name — so the weigher_failover_host weigher reaches the same + // target over the same snapshot without coordinating. + // + // A not-yet-in-use reservation is only eligible once it is Active (host + // drained and empty, ready to absorb an evacuation). A reservation still being + // drained (not yet Active, e.g. freshly created) is not a valid landing zone + // yet, so it is never selected here — but note it is still blocked below, + // keeping ordinary VMs off it while it drains. var keep string if intent, err := request.GetIntent(); err == nil && intent == api.EvacuateIntent { if sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host"); err == nil && sourceHost != "" { @@ -94,7 +100,7 @@ func (s *FilterFailoverHost) Run(traceLog logr.Logger, request api.ExternalSched for i := range matching { res := &matching[i] if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if keep == "" { + if keep == "" && meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostActive) { keep = res.Spec.TargetHost } continue @@ -114,9 +120,9 @@ func (s *FilterFailoverHost) Run(traceLog logr.Logger, request api.ExternalSched } // Block both the desired (TargetHost) and actual (Status.Host) host to // cover the window while the reservation is being placed. A reservation - // only exists from the Designated stage onward (the controller deletes - // Revoked/Used ones), so its presence is enough to keep new virtual - // machines off the host. + // blocks from creation onward (the controller deletes Revoked/Used ones), + // so its presence is enough to keep new virtual machines off the host, + // whether or not it is Active yet. for _, host := range []string{reservation.Spec.TargetHost, reservation.Status.Host} { if host == "" || host == keep { continue diff --git a/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go b/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go index b99c0953b..d10844d0b 100644 --- a/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go +++ b/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go @@ -17,10 +17,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) -// failoverHostReservation builds a failover host reservation targeting targetHost -// (with statusHost as its actual host), able to rescue the given source hosts. +// failoverHostReservation builds an Active failover host reservation targeting +// targetHost (with statusHost as its actual host), able to rescue the given +// source hosts. Active is the state in which a reservation can be selected as an +// evacuation target; use notActive() to drop it back to the draining state. func failoverHostReservation(targetHost, statusHost string, sourceHosts ...string) *v1alpha1.Reservation { - return &v1alpha1.Reservation{ + res := &v1alpha1.Reservation{ ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, Spec: v1alpha1.ReservationSpec{ Type: v1alpha1.ReservationTypeFailoverHost, @@ -31,6 +33,24 @@ func failoverHostReservation(targetHost, statusHost string, sourceHosts ...strin }, Status: v1alpha1.ReservationStatus{Host: statusHost}, } + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostActive, + Status: metav1.ConditionTrue, + Reason: "HostDrained", + }) + return res +} + +// notActive drops a reservation out of the Active state, back to the draining +// stage (the same as a freshly created reservation): it still blocks ordinary +// VMs but cannot yet be selected for evacuation. +func notActive(res *v1alpha1.Reservation) *v1alpha1.Reservation { + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostActive, + Status: metav1.ConditionFalse, + Reason: "Draining", + }) + return res } // inUseBy marks a failover host reservation as in use by the given source host. @@ -163,6 +183,28 @@ func TestFilterFailoverHost_Run(t *testing.T) { request: requestWithHosts(evacuationHints(""), "h1", "h2", "h3"), expectedHosts: []string{"h2", "h3"}, }, + { + name: "evacuation - a not-yet-active target is still blocked", + opts: FilterFailoverHostOpts{Enabled: true}, + // h1 is freshly created (not Active yet), so it cannot be a landing + // zone and is removed like any other reserved host, even though the + // source could otherwise evacuate onto it. + reservations: []client.Object{notActive(failoverHostReservation("h1", "", "source1"))}, + request: requestWithHosts(evacuationHints("source1"), "h1", "h2", "h3"), + expectedHosts: []string{"h2", "h3"}, + }, + { + name: "evacuation - selects the active target and prunes a not-yet-active one", + opts: FilterFailoverHostOpts{Enabled: true}, + // h1 sorts first but is not Active yet; h2 is Active, so h2 is the + // selected target and h1 is pruned as an unusable reserved host. + reservations: []client.Object{ + notActive(failoverHostReservation("h1", "", "source1")), + failoverHostReservation("h2", "", "source1"), + }, + request: requestWithHosts(evacuationHints("source1"), "h1", "h2", "h3"), + expectedHosts: []string{"h2", "h3"}, + }, } for _, tt := range tests { diff --git a/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go index 5d5ef2948..27b1f3aaf 100644 --- a/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go +++ b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go @@ -70,8 +70,9 @@ func (s *WeigherFailoverHost) Run(traceLog logr.Logger, request api.ExternalSche // same deterministic rule the filter_failover_host filter used over the same // snapshot: among the reservations whose partition includes the source host, // prefer the one already in use by this very source (gang-evacuation reuse), - // never one in use by a different source, otherwise the first by target host - // name. + // never one in use by a different source, otherwise the first Active one by + // target host name. A not-yet-in-use reservation is only eligible once it is + // Active (drained and empty); a still-draining, not-yet-Active one is never lifted. var matching []v1alpha1.Reservation for _, res := range reservations.Items { if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { @@ -88,7 +89,7 @@ func (s *WeigherFailoverHost) Run(traceLog logr.Logger, request api.ExternalSche for i := range matching { res := &matching[i] if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if target == "" { + if target == "" && meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostActive) { target = res.Spec.TargetHost } continue diff --git a/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go b/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go index 32397822b..7079eb2a8 100644 --- a/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go +++ b/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go @@ -16,8 +16,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +// failoverHostReservation builds an Active failover host reservation (the state +// in which it can be selected as an evacuation target). func failoverHostReservation(targetHost string, sourceHosts ...string) *v1alpha1.Reservation { - return &v1alpha1.Reservation{ + res := &v1alpha1.Reservation{ ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, Spec: v1alpha1.ReservationSpec{ Type: v1alpha1.ReservationTypeFailoverHost, @@ -27,6 +29,23 @@ func failoverHostReservation(targetHost string, sourceHosts ...string) *v1alpha1 }, }, } + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostActive, + Status: metav1.ConditionTrue, + Reason: "HostDrained", + }) + return res +} + +// notActive drops a reservation out of the Active state (the same as a freshly +// created, still-draining reservation). +func notActive(res *v1alpha1.Reservation) *v1alpha1.Reservation { + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostActive, + Status: metav1.ConditionFalse, + Reason: "Draining", + }) + return res } func failoverInUseBy(res *v1alpha1.Reservation, sourceHost string) *v1alpha1.Reservation { @@ -115,6 +134,23 @@ func TestWeigherFailoverHost_Run(t *testing.T) { request: weigherRequest(evacHints("source1"), "h1", "h2"), wantTarget: "", }, + { + name: "evacuation - not-yet-active target is not lifted", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{notActive(failoverHostReservation("h1", "source1"))}, + request: weigherRequest(evacHints("source1"), "h1", "h2"), + wantTarget: "", + }, + { + name: "evacuation - lifts the active target over a not-yet-active one", + opts: WeigherFailoverHostOpts{Enabled: true}, + reservations: []client.Object{ + notActive(failoverHostReservation("h1", "source1")), + failoverHostReservation("h2", "source1"), + }, + request: weigherRequest(evacHints("source1"), "h1", "h2", "h3"), + wantTarget: "h2", + }, { name: "evacuation - selected target not among candidates - no effect", opts: WeigherFailoverHostOpts{Enabled: true}, diff --git a/internal/scheduling/reservations/failoverhost/matching.go b/internal/scheduling/reservations/failoverhost/matching.go index a7af9a364..9d3276a24 100644 --- a/internal/scheduling/reservations/failoverhost/matching.go +++ b/internal/scheduling/reservations/failoverhost/matching.go @@ -75,7 +75,9 @@ func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalS // shaped the candidates around: among the reservations whose partition // includes the source host, prefer the one already in use by this very source // (gang-evacuation reuse), never one in use by a different source (that would - // steal an active landing zone), otherwise the first by target host name. + // steal an active landing zone), otherwise the first Active one by target host + // name. A not-yet-in-use reservation is only eligible once it is Active + // (drained and empty); a still-draining, not-yet-Active one is never pinned. var matching []v1alpha1.Reservation for _, res := range reservations.Items { if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { @@ -92,7 +94,7 @@ func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalS for i := range matching { res := &matching[i] if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if selected == nil { + if selected == nil && meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostActive) { selected = res } continue diff --git a/internal/scheduling/reservations/failoverhost/matching_test.go b/internal/scheduling/reservations/failoverhost/matching_test.go index 8c28340d3..a59a40f41 100644 --- a/internal/scheduling/reservations/failoverhost/matching_test.go +++ b/internal/scheduling/reservations/failoverhost/matching_test.go @@ -15,10 +15,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -// matcherReservation builds a failover host reservation targeting targetHost -// and able to rescue the given source hosts. +// matcherReservation builds an Active failover host reservation targeting +// targetHost and able to rescue the given source hosts. Active is the state in +// which a reservation can be selected as an evacuation target. func matcherReservation(targetHost string, sourceHosts ...string) *v1alpha1.Reservation { - return &v1alpha1.Reservation{ + res := &v1alpha1.Reservation{ ObjectMeta: metav1.ObjectMeta{Name: "failover-host-" + targetHost}, Spec: v1alpha1.ReservationSpec{ Type: v1alpha1.ReservationTypeFailoverHost, @@ -28,6 +29,23 @@ func matcherReservation(targetHost string, sourceHosts ...string) *v1alpha1.Rese }, }, } + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostActive, + Status: metav1.ConditionTrue, + Reason: "HostDrained", + }) + return res +} + +// notActive drops a reservation out of the Active state (the same as a freshly +// created, still-draining reservation). +func notActive(res *v1alpha1.Reservation) *v1alpha1.Reservation { + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ConditionTypeFailoverHostActive, + Status: metav1.ConditionFalse, + Reason: "Draining", + }) + return res } // inUse marks a reservation as in use by the given source host. @@ -134,6 +152,20 @@ func TestMatcher_ConfirmAndMarkInUse(t *testing.T) { } }, }, + { + name: "does not pin a not-yet-active reservation", + // target1 is the top candidate but is still draining (not Active), so + // it is not a selectable landing zone and must not be pinned. + reservations: []*v1alpha1.Reservation{notActive(matcherReservation("target1", "source1"))}, + request: evacuationRequest("source1"), + orderedHosts: []string{"target1", "host2"}, + check: func(t *testing.T, c client.Client) { + res := getReservation(t, c, "failover-host-target1") + if meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + t.Error("expected a not-yet-active target1 to NOT be pinned") + } + }, + }, { name: "reuses the in-use reservation for the same source, no new write", reservations: []*v1alpha1.Reservation{ From 942544a6bf7c3afb4124b45d10a6398ee618d64c Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Wed, 4 Nov 2026 14:41:54 +0100 Subject: [PATCH 6/7] refactor(failoverhost): centralize host selection, break import cycle Extract the deterministic failover-host selection rule into a single exported SelectFailoverHost in the failoverhost package, called by the filter, the weigher and the matcher, removing the three inline copies. This requires the plugin packages to import failoverhost, which previously cycled because failoverhost's in-package integration_test.go imported the real nova pipeline (filters/weighers/nova). Replace that white-box integration test with stub-scheduler scenarios in generation_test.go: the five whole-host guarantee cases (trait incompatibility, homogeneous, undersized host, AZ isolation, sibling sources) now drive the generation pipeline against a stub scheduler whose per-(flavor, AZ) accept-sets stand in for the real pipeline's answers. The real filter semantics remain covered by each filter's own unit tests. Signed-off-by: Philipp Matthes --- .../plugins/filters/filter_failover_host.go | 50 +- .../plugins/weighers/weigher_failover_host.go | 42 +- .../failoverhost/generation_test.go | 283 ++++++++++ .../failoverhost/integration_test.go | 520 ------------------ .../reservations/failoverhost/matching.go | 80 +-- 5 files changed, 346 insertions(+), 629 deletions(-) delete mode 100644 internal/scheduling/reservations/failoverhost/integration_test.go diff --git a/internal/scheduling/nova/plugins/filters/filter_failover_host.go b/internal/scheduling/nova/plugins/filters/filter_failover_host.go index 6251c33fc..a429454c8 100644 --- a/internal/scheduling/nova/plugins/filters/filter_failover_host.go +++ b/internal/scheduling/nova/plugins/filters/filter_failover_host.go @@ -6,14 +6,13 @@ package filters import ( "context" "slices" - "sort" "github.com/go-logr/logr" api "github.com/cobaltcore-dev/cortex/api/external/nova" "github.com/cobaltcore-dev/cortex/api/v1alpha1" "github.com/cobaltcore-dev/cortex/internal/scheduling/lib" - "k8s.io/apimachinery/pkg/api/meta" + "github.com/cobaltcore-dev/cortex/internal/scheduling/reservations/failoverhost" ) // FilterFailoverHostOpts configures the failover-host filter. @@ -69,47 +68,18 @@ func (s *FilterFailoverHost) Run(traceLog logr.Logger, request api.ExternalSched } // For an evacuation, keep the single failover host selected for this source so - // the gang lands there; for everything else, keep none. The selection is a - // pure function of the reservation snapshot — among the reservations whose - // partition includes the source host, prefer the one already in use by this - // very source (gang-evacuation reuse), never one in use by a different source - // (that would steal an active landing zone), otherwise the first Active one - // by target host name — so the weigher_failover_host weigher reaches the same - // target over the same snapshot without coordinating. - // - // A not-yet-in-use reservation is only eligible once it is Active (host - // drained and empty, ready to absorb an evacuation). A reservation still being - // drained (not yet Active, e.g. freshly created) is not a valid landing zone - // yet, so it is never selected here — but note it is still blocked below, - // keeping ordinary VMs off it while it drains. + // the gang lands there; for everything else, keep none. The selection + // (failoverhost.SelectFailoverHost) is a pure function of the reservation + // snapshot, so the weigher_failover_host weigher reaches the same target over + // the same snapshot without coordinating. A reservation is only selectable + // once it is Active (drained and empty); a still-draining one is not kept here + // — but note it is still blocked below, keeping ordinary VMs off it while it + // drains. var keep string if intent, err := request.GetIntent(); err == nil && intent == api.EvacuateIntent { if sourceHost, err := request.Spec.Data.GetSchedulerHintStr("source_host"); err == nil && sourceHost != "" { - var matching []v1alpha1.Reservation - for _, res := range reservations.Items { - if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { - continue - } - if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { - matching = append(matching, res) - } - } - sort.Slice(matching, func(i, j int) bool { - return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost - }) - for i := range matching { - res := &matching[i] - if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if keep == "" && meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostActive) { - keep = res.Spec.TargetHost - } - continue - } - if res.Status.FailoverHostReservation != nil && - res.Status.FailoverHostReservation.SourceHost == sourceHost { - keep = res.Spec.TargetHost - break - } + if selected := failoverhost.SelectFailoverHost(reservations.Items, sourceHost); selected != nil { + keep = selected.Spec.TargetHost } } } diff --git a/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go index 27b1f3aaf..2296cddb8 100644 --- a/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go +++ b/internal/scheduling/nova/plugins/weighers/weigher_failover_host.go @@ -5,15 +5,13 @@ package weighers import ( "context" - "slices" - "sort" "github.com/go-logr/logr" - "k8s.io/apimachinery/pkg/api/meta" api "github.com/cobaltcore-dev/cortex/api/external/nova" "github.com/cobaltcore-dev/cortex/api/v1alpha1" "github.com/cobaltcore-dev/cortex/internal/scheduling/lib" + "github.com/cobaltcore-dev/cortex/internal/scheduling/reservations/failoverhost" ) // failoverTargetWeight is the activation assigned to the failover host a source @@ -68,41 +66,13 @@ func (s *WeigherFailoverHost) Run(traceLog logr.Logger, request api.ExternalSche // Select the one failover host this source should gang-evacuate onto, with the // same deterministic rule the filter_failover_host filter used over the same - // snapshot: among the reservations whose partition includes the source host, - // prefer the one already in use by this very source (gang-evacuation reuse), - // never one in use by a different source, otherwise the first Active one by - // target host name. A not-yet-in-use reservation is only eligible once it is - // Active (drained and empty); a still-draining, not-yet-Active one is never lifted. - var matching []v1alpha1.Reservation - for _, res := range reservations.Items { - if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { - continue - } - if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { - matching = append(matching, res) - } - } - sort.Slice(matching, func(i, j int) bool { - return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost - }) - var target string - for i := range matching { - res := &matching[i] - if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if target == "" && meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostActive) { - target = res.Spec.TargetHost - } - continue - } - if res.Status.FailoverHostReservation != nil && - res.Status.FailoverHostReservation.SourceHost == sourceHost { - target = res.Spec.TargetHost - break - } - } - if target == "" { + // snapshot (failoverhost.SelectFailoverHost), so the filter and weigher agree + // without coordinating. + selected := failoverhost.SelectFailoverHost(reservations.Items, sourceHost) + if selected == nil { return result, nil } + target := selected.Spec.TargetHost if _, ok := result.Activations[target]; !ok { // The selected target conflicts with other scheduling constraints and was // filtered out, so there is nothing to lift. diff --git a/internal/scheduling/reservations/failoverhost/generation_test.go b/internal/scheduling/reservations/failoverhost/generation_test.go index 074ea1a98..6782a8f8b 100644 --- a/internal/scheduling/reservations/failoverhost/generation_test.go +++ b/internal/scheduling/reservations/failoverhost/generation_test.go @@ -5,11 +5,18 @@ package failoverhost import ( "context" + "encoding/json" + "net/http" + "net/http/httptest" + "slices" "testing" + api "github.com/cobaltcore-dev/cortex/api/external/nova" "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "github.com/cobaltcore-dev/cortex/internal/knowledge/extractor/plugins/compute" hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // fakeSeeder returns a fixed set of seeds, so generator tests drive the pipeline @@ -190,3 +197,279 @@ func TestGenerateAbortsOnMissingAvailabilityZone(t *testing.T) { t.Fatalf("expected Generate to abort (ok=false) on a hypervisor with no AZ") } } + +// ============================================================================ +// Whole-host guarantee scenarios +// +// These exercise the whole failover-host generation pipeline (seeder -> +// path finder -> clique builder -> planner) end to end against a *stubbed* +// scheduler whose per-(flavor, availability zone) accept-sets stand in for what +// the real Nova pipeline would answer. The accept-sets encode the compatibility +// axes the scenarios care about (traits, availability zone); that the real +// filters actually produce those answers is covered by each filter's own unit +// tests (filter_correct_az, filter_has_requested_traits, filter_has_enough_capacity). +// +// The invariant under test is the ADR 0011 whole-host guarantee: a reserved +// failover target must be able to absorb the entire workload of every host it is +// recorded as rescuing — i.e. the target must accept every flavor its source +// hosts accept. +// ============================================================================ + +const traitX = "CUSTOM_TRAIT_X" + +func TestWholeHostGuarantee(t *testing.T) { + tests := []struct { + name string + hypervisors []*hv1.Hypervisor + flavors []compute.FlavorInGroup + // acceptsByFlavorAZ[flavor][az] is the set of hosts the stub scheduler + // returns for a seed of that flavor in that availability zone — the + // compatibility the real pipeline would compute. Capacity fit is NOT + // modeled here; the path finder applies the real whole-host size check on + // top, so an undersized host can still be dropped despite accepting. + acceptsByFlavorAZ map[string]map[string][]string + ratio float64 + minimum int + // wantReservations, when non-zero, asserts the exact reservation count. + wantReservations int + expectAny bool + forbiddenTargets []string + expectSiblings bool + }{ + { + // A single shared flavor forges a rescue link between every host, but a + // CUSTOM_TRAIT_X VM could never restart on a basic host (the scheduler + // rejects traitx there). The two special hosts form a valid trait-X + // partition and the two basics a general one; the same-flavor-set clique + // check must not merge them, or a basic host gets reserved as the + // specials' target and the whole-host guarantee breaks. + name: "trait-incompatible host is not reserved as a failover target", + hypervisors: []*hv1.Hypervisor{ + hvTraits("basic-1"), hvTraits("basic-2"), + hvTraits("special-1", traitX), hvTraits("special-2", traitX), + }, + flavors: []compute.FlavorInGroup{traitFlavor("general", ""), traitFlavor("traitx", traitX)}, + acceptsByFlavorAZ: map[string]map[string][]string{ + "general": {"az1": {"basic-1", "basic-2", "special-1", "special-2"}}, + "traitx": {"az1": {"special-1", "special-2"}}, + }, + ratio: 0.1, minimum: 1, expectAny: true, + }, + { + // Three identical hosts accept the one flavor, so whichever becomes the + // target trivially satisfies the invariant. Guards against the check + // above being vacuously true. + name: "homogeneous partition reserves a compatible target", + hypervisors: []*hv1.Hypervisor{hvTraits("hv-1"), hvTraits("hv-2"), hvTraits("hv-3")}, + flavors: []compute.FlavorInGroup{traitFlavor("general", "")}, + acceptsByFlavorAZ: map[string]map[string][]string{ + "general": {"az1": {"hv-1", "hv-2", "hv-3"}}, + }, + expectAny: true, + }, + { + // Two large hosts and one small host all accept the flavor, but an + // evacuation moves a whole host's workload at once, so a large host's + // VMs could never fit on the small one. The path finder's whole-host fit + // check must drop every large->small edge, leaving "small" only ever a + // source, never a target — so it must never be reserved. + name: "undersized host is never reserved as a failover target", + hypervisors: []*hv1.Hypervisor{ + buildHV("large-1", 128, 64), buildHV("large-2", 128, 64), buildHV("small-1", 16, 8), + }, + flavors: []compute.FlavorInGroup{traitFlavor("general", "")}, + acceptsByFlavorAZ: map[string]map[string][]string{ + "general": {"az1": {"large-1", "large-2", "small-1"}}, + }, + expectAny: true, forbiddenTargets: []string{"small-1"}, + }, + { + // Two hosts per zone all accept the flavor, but the AZ filter only admits + // hosts of the seed's own zone, so each zone forms its own partition and + // the fleet yields exactly two reservations — a wrong cross-AZ merge would + // cap at one three-host partition's worth instead. + name: "hosts in different AZs are not merged into one partition", + hypervisors: []*hv1.Hypervisor{ + hvInZone("az1-1", "az1"), hvInZone("az1-2", "az1"), + hvInZone("az2-1", "az2"), hvInZone("az2-2", "az2"), + }, + flavors: []compute.FlavorInGroup{traitFlavor("general", "")}, + acceptsByFlavorAZ: map[string]map[string][]string{ + "general": { + "az1": {"az1-1", "az1-2"}, + "az2": {"az2-1", "az2-2"}, + }, + }, + expectAny: true, wantReservations: 2, + }, + { + // A ratio of 1.0 would reserve the whole partition, but the + // never-reserve-all rule caps a three-host partition at two targets. Each + // reservation must then list the other reserved host among its sources, + // the post-failover shortcut a multi-target partition relies on. + name: "multi-target partition records sibling reserved hosts as sources", + hypervisors: []*hv1.Hypervisor{hvTraits("hv-1"), hvTraits("hv-2"), hvTraits("hv-3")}, + flavors: []compute.FlavorInGroup{traitFlavor("general", "")}, + acceptsByFlavorAZ: map[string]map[string][]string{ + "general": {"az1": {"hv-1", "hv-2", "hv-3"}}, + }, + ratio: 1.0, minimum: 1, expectAny: true, wantReservations: 2, expectSiblings: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objs := make([]client.Object, 0, len(tt.hypervisors)+1) + hvsByName := make(map[string]hv1.Hypervisor, len(tt.hypervisors)) + for _, hv := range tt.hypervisors { + objs = append(objs, hv) + hvsByName[hv.Name] = *hv + } + objs = append(objs, flavorGroupsKnowledge(t, true, tt.flavors...)) + + finder, server := azAwareStubScheduler(t, tt.acceptsByFlavorAZ) + defer server.Close() + + ratio, minimum := tt.ratio, tt.minimum + if ratio == 0 { + ratio = 1.0 + } + if minimum == 0 { + minimum = 1 + } + g := &pipelineGenerator{ + Client: newTestClient(t, objs...), + seeder: &seeder{k8s: newTestClient(t, objs...)}, + pathFinder: finder, + partitionBuilder: cliquePartitionBuilder{}, + reservationPlanner: ratioReservationPlanner{ratio: ratio, minimum: minimum}, + } + desired, _, ok := g.Generate(context.Background()) + if !ok { + t.Fatal("Generate returned ok=false") + } + if tt.expectAny && len(desired) == 0 { + t.Fatal("expected at least one failover host reservation, got none") + } + if tt.wantReservations != 0 && len(desired) != tt.wantReservations { + t.Fatalf("expected %d reservations, got %d", tt.wantReservations, len(desired)) + } + for _, res := range desired { + if slices.Contains(tt.forbiddenTargets, res.Spec.TargetHost) { + t.Errorf("host %q was reserved as a failover target but must never be", res.Spec.TargetHost) + } + } + verifyWholeHostEvacuable(t, desired, tt.acceptsByFlavorAZ) + if tt.expectSiblings { + verifySiblingSources(t, desired) + } + }) + } +} + +// verifyWholeHostEvacuable asserts the ADR 0011 invariant: for every reservation, +// the target host accepts every flavor each of its recorded source hosts accepts. +func verifyWholeHostEvacuable(t *testing.T, desired map[corev1.ObjectReference]v1alpha1.Reservation, acceptsByFlavorAZ map[string]map[string][]string) { + t.Helper() + // Invert the accept-sets into per-host flavor sets, across all AZs. + acceptsByHost := map[string]map[string]bool{} + for flavor, byAZ := range acceptsByFlavorAZ { + for _, hosts := range byAZ { + for _, h := range hosts { + if acceptsByHost[h] == nil { + acceptsByHost[h] = map[string]bool{} + } + acceptsByHost[h][flavor] = true + } + } + } + for _, res := range desired { + target := res.Spec.TargetHost + if res.Spec.FailoverHostReservation == nil { + t.Fatalf("reservation %s has no FailoverHostReservation spec", res.Name) + } + for _, source := range res.Spec.FailoverHostReservation.SourceHosts { + for flavor := range acceptsByHost[source] { + if !acceptsByHost[target][flavor] { + t.Errorf("reservation %s: target %q cannot run flavor %q that source %q accepts — %q's VMs could not evacuate onto it", + res.Name, target, flavor, source, source) + } + } + } + } +} + +// verifySiblingSources asserts that in a partition reserving more than one +// failover host, every reservation lists each other reserved host among its +// source hosts (the post-failover shortcut the planner relies on). +func verifySiblingSources(t *testing.T, desired map[corev1.ObjectReference]v1alpha1.Reservation) { + t.Helper() + targets := make([]string, 0, len(desired)) + for _, res := range desired { + targets = append(targets, res.Spec.TargetHost) + } + for _, res := range desired { + for _, sibling := range targets { + if sibling == res.Spec.TargetHost { + continue + } + if !slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sibling) { + t.Errorf("reservation for target %q does not list sibling reserved host %q as a source", + res.Spec.TargetHost, sibling) + } + } + } +} + +// azAwareStubScheduler serves a scheduler that answers each probe with the +// accept-set for the request's flavor *and* availability zone, so a scenario can +// model the AZ filter (a seed in az1 only ever accepts az1 hosts). It performs +// the same request-shape assertions as the simpler stubScheduler. +func azAwareStubScheduler(t *testing.T, acceptsByFlavorAZ map[string]map[string][]string) (*evacuationPathFinder, *httptest.Server) { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req api.ExternalSchedulerRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Errorf("decode request: %v", err) + } + if !req.Options.AssumeEmptyHosts { + t.Errorf("request for flavor %q must set AssumeEmptyHosts", req.Spec.Data.Flavor.Data.Name) + } + if intent, err := req.GetIntent(); err != nil || intent != api.EvacuateIntent { + t.Errorf("request for flavor %q must use evacuate intent, got %v (err=%v)", req.Spec.Data.Flavor.Data.Name, intent, err) + } + flavor := req.Spec.Data.Flavor.Data.Name + az := req.Spec.Data.AvailabilityZone + if err := json.NewEncoder(w).Encode(api.ExternalSchedulerResponse{Hosts: acceptsByFlavorAZ[flavor][az]}); err != nil { + t.Errorf("encode response: %v", err) + } + })) + finder := &evacuationPathFinder{httpClient: server.Client(), schedulerURL: server.URL} + return finder, server +} + +// hvTraits builds a fixed-size (64Gi/32cpu) hypervisor in az1 with the given +// traits, so a scenario isolates trait (not capacity) as the compatibility axis. +func hvTraits(name string, traits ...string) *hv1.Hypervisor { + h := buildHV(name, 64, 32) + h.Status.Traits = traits + return h +} + +// hvInZone builds a fixed-size hypervisor (matching hvTraits) in the given +// availability zone, so a scenario can exercise AZ partitioning. +func hvInZone(name, az string) *hv1.Hypervisor { + h := buildHV(name, 64, 32) + h.Labels[corev1.LabelTopologyZone] = az + return h +} + +// traitFlavor builds a generic flavor requiring the given trait (or none). Its +// hypervisor type is a supported value so the seeder keeps it. +func traitFlavor(name, requiredTrait string) compute.FlavorInGroup { + extra := map[string]string{"capabilities:hypervisor_type": "ch"} + if requiredTrait != "" { + extra["trait:"+requiredTrait] = "required" + } + return compute.FlavorInGroup{Name: name, VCPUs: 1, MemoryMB: 1024, ExtraSpecs: extra} +} diff --git a/internal/scheduling/reservations/failoverhost/integration_test.go b/internal/scheduling/reservations/failoverhost/integration_test.go deleted file mode 100644 index 013d8e269..000000000 --- a/internal/scheduling/reservations/failoverhost/integration_test.go +++ /dev/null @@ -1,520 +0,0 @@ -// Copyright SAP SE -// SPDX-License-Identifier: Apache-2.0 - -package failoverhost - -import ( - "context" - "encoding/json" - "net/http" - "net/http/httptest" - "slices" - "sync" - "testing" - - novaapi "github.com/cobaltcore-dev/cortex/api/external/nova" - "github.com/cobaltcore-dev/cortex/api/v1alpha1" - "github.com/cobaltcore-dev/cortex/internal/knowledge/extractor/plugins/compute" - "github.com/cobaltcore-dev/cortex/internal/scheduling/lib" - "github.com/cobaltcore-dev/cortex/internal/scheduling/nova" - "github.com/cobaltcore-dev/cortex/internal/scheduling/nova/plugins/filters" - "github.com/cobaltcore-dev/cortex/internal/scheduling/nova/plugins/weighers" - hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// ============================================================================ -// Integration Tests -// -// These exercise the whole failover-host generation pipeline (seeder -> -// path finder -> clique builder -> planner) against a *real* Nova -// filter/weigher pipeline served over HTTP, rather than a stubbed scheduler. -// The invariant under test is the ADR 0011 whole-host guarantee: a reserved -// failover target must be able to absorb the entire workload of every host it -// is recorded as rescuing. Since eligibility is static (ADR A5) that reduces -// to: the target must accept every protected flavor its source hosts accept. -// ============================================================================ - -// IntegrationTestCase defines one failover-host generation scenario. -type IntegrationTestCase struct { - Name string - Hypervisors []*hv1.Hypervisor - Flavors []compute.FlavorInGroup - - // Ratio/Minimum configure the planner. Ratio defaults to 1.0 and Minimum to - // 1 when left zero, so a scenario reserves as many targets as the partition - // allows unless it says otherwise. - Ratio float64 - Minimum int - - // ExpectReservations asserts at least one reservation is produced (most - // scenarios want this; a scenario proving nothing is reserved sets it false). - ExpectReservations bool - - // ForbiddenTargets names hosts that must never be reserved as a failover - // target, e.g. a host too small to absorb a larger peer's whole workload. - ForbiddenTargets []string - - // ExpectReservationCount, when non-zero, asserts the exact number of - // reservations produced. Used by scenarios that must reserve more than one - // target so the sibling-source wiring can be checked. - ExpectReservationCount int - - // ExpectSiblingSources asserts every reservation lists all the other - // reservations' target hosts among its source hosts — the post-failover - // shortcut a multi-target partition relies on. - ExpectSiblingSources bool -} - -func TestIntegration(t *testing.T) { - const traitX = "CUSTOM_TRAIT_X" - testCases := []IntegrationTestCase{ - // ===================================================================== - // Whole-host evacuability - // ===================================================================== - { - // The general flavor makes all four hosts mutually evacuable, so a - // naive clustering would lump the two plain "basic" hosts into the same - // partition as the two trait-X hosts and could reserve a basic host as - // their failover target. But a CUSTOM_TRAIT_X VM on a special host could - // never restart on a basic host: the real scheduler rejects the trait-X - // flavor there. The two special hosts on their own form a valid trait-X - // partition (and the two basic hosts a valid general one), so the - // coverage is genuinely achievable — the algorithm just must not merge - // the incompatible hosts together. A single shared flavor is enough to - // forge a rescue link, so this is the regression guard for the - // same-flavor-set check that keeps the clique step from merging hosts - // that do not accept exactly the same flavors. - Name: "trait-incompatible host is not reserved as a failover target", - Hypervisors: []*hv1.Hypervisor{ - hvWithTraits("basic-1"), - hvWithTraits("basic-2"), - hvWithTraits("special-1", traitX), - hvWithTraits("special-2", traitX), - }, - Flavors: []compute.FlavorInGroup{ - flavor("general", ""), - flavor("traitx", traitX), - }, - // Production defaults: floor(4 × 0.1) = 0, raised to the minimum of 1, - // so exactly one host is reserved. It is the cheapest/first by name, - // "basic-1", which cannot run traitx — so even a single reservation - // violates the whole-host guarantee. - Ratio: 0.1, - Minimum: 1, - ExpectReservations: true, - }, - // ===================================================================== - // Baseline: a homogeneous partition is safe - // ===================================================================== - { - // Three identical hosts accept the one flavor, so any of them can - // rescue any other: whichever becomes the target trivially satisfies - // the whole-host invariant. Guards against the check above being - // vacuously true. - Name: "homogeneous partition reserves a compatible target", - Hypervisors: []*hv1.Hypervisor{ - hvWithTraits("hv-1"), - hvWithTraits("hv-2"), - hvWithTraits("hv-3"), - }, - Flavors: []compute.FlavorInGroup{flavor("general", "")}, - ExpectReservations: true, - }, - // ===================================================================== - // Whole-host capacity fit - // ===================================================================== - { - // Two large hosts and one small host all accept the one general flavor, - // so under AssumeEmptyHosts the real capacity filter admits all three and - // they co-accept the seed. But an evacuation moves a whole host's - // workload at once, so a large host's VMs could never fit on the small - // one: the path finder's whole-host fit check must drop every - // large→small edge, leaving "small" only ever a source, never a - // mutual-edge target. The small host must therefore never be reserved — - // even though it happily accepts the flavor — or a large peer's workload - // would have nowhere to land. - Name: "undersized host is never reserved as a failover target", - Hypervisors: []*hv1.Hypervisor{ - buildHV("large-1", 128, 64), - buildHV("large-2", 128, 64), - buildHV("small-1", 16, 8), - }, - Flavors: []compute.FlavorInGroup{flavor("general", "")}, - ExpectReservations: true, - ForbiddenTargets: []string{"small-1"}, - }, - // ===================================================================== - // Availability-zone isolation - // ===================================================================== - { - // Two hosts in az1 and two in az2 all accept the one flavor, but the real - // AZ filter only ever admits hosts of the seed's own zone, so a seed in - // az1 co-accepts only the az1 hosts and vice versa. The two zones must - // therefore form two separate partitions: no reservation may record a - // cross-AZ source, because a VM in az1 could not be evacuated onto an az2 - // host. Each zone reserves its own target, so the fleet yields exactly - // two reservations — a wrong merge into one four-host partition would, at - // the default ratio of 1.0, cap at three instead. - Name: "hosts in different AZs are not merged into one partition", - Hypervisors: []*hv1.Hypervisor{ - hvPtrInAZ("az1-1", "az1"), - hvPtrInAZ("az1-2", "az1"), - hvPtrInAZ("az2-1", "az2"), - hvPtrInAZ("az2-2", "az2"), - }, - Flavors: []compute.FlavorInGroup{flavor("general", "")}, - ExpectReservations: true, - ExpectReservationCount: 2, - }, - // ===================================================================== - // Multi-target partition: siblings list each other as sources - // ===================================================================== - { - // A ratio of 1.0 would reserve the whole partition, but the never-reserve- - // all rule caps it at len(partition)-1, so a three-host partition reserves - // two targets. Each reservation must then list the *other* reserved host - // among its sources: once a reserved host absorbs an evacuation and - // returns to service it instantly needs a target of its own, which is why - // the planner keeps the sibling reserved hosts in each other's source - // lists. Whole-host evacuability still holds since the hosts are identical. - Name: "multi-target partition records sibling reserved hosts as sources", - Hypervisors: []*hv1.Hypervisor{ - hvWithTraits("hv-1"), - hvWithTraits("hv-2"), - hvWithTraits("hv-3"), - }, - Flavors: []compute.FlavorInGroup{flavor("general", "")}, - Ratio: 1.0, - Minimum: 1, - ExpectReservations: true, - ExpectReservationCount: 2, - ExpectSiblingSources: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.Name, func(t *testing.T) { - env := newIntegrationTestEnv(t, tc.Hypervisors, tc.Flavors) - defer env.Close() - - ratio, minimum := tc.Ratio, tc.Minimum - if ratio == 0 { - ratio = 1.0 - } - if minimum == 0 { - minimum = 1 - } - - desired := env.Generate(t, ratio, minimum) - if tc.ExpectReservations && len(desired) == 0 { - t.Fatal("expected at least one failover host reservation, got none") - } - if tc.ExpectReservationCount != 0 && len(desired) != tc.ExpectReservationCount { - t.Fatalf("expected %d reservations, got %d", tc.ExpectReservationCount, len(desired)) - } - for _, res := range desired { - if slices.Contains(tc.ForbiddenTargets, res.Spec.TargetHost) { - t.Errorf("host %q was reserved as a failover target but must never be", res.Spec.TargetHost) - } - } - if tc.ExpectSiblingSources { - env.VerifySiblingSources(t, desired) - } - - env.VerifyWholeHostEvacuable(t, desired) - }) - } -} - -// ============================================================================ -// Test Environment -// ============================================================================ - -// integrationTestEnv wires a real Nova filter/weigher pipeline (served over -// HTTP) on top of a fake k8s client holding the fleet and the flavor-groups -// Knowledge CRD, so the generation pipeline can be driven end to end. -type integrationTestEnv struct { - k8sClient client.Client - server *httptest.Server - schedulerURL string - flavors []compute.FlavorInGroup - hypervisors map[string]hv1.Hypervisor -} - -func (env *integrationTestEnv) Close() { env.server.Close() } - -// newIntegrationTestEnv builds the fake cluster and the real scheduler pipeline. -func newIntegrationTestEnv(t *testing.T, hypervisors []*hv1.Hypervisor, flavors []compute.FlavorInGroup) *integrationTestEnv { - t.Helper() - - objects := make([]client.Object, 0, len(hypervisors)+1) - hvsByName := make(map[string]hv1.Hypervisor, len(hypervisors)) - for _, hv := range hypervisors { - objects = append(objects, hv) - hvsByName[hv.Name] = *hv - } - objects = append(objects, flavorGroupsKnowledge(t, true, flavors...)) - - k8sClient := newTestClient(t, objects...) - - controller := &nova.FilterWeigherPipelineController{ - BasePipelineController: lib.BasePipelineController[lib.FilterWeigherPipeline[novaapi.ExternalSchedulerRequest]]{ - Client: k8sClient, - Pipelines: make(map[string]lib.FilterWeigherPipeline[novaapi.ExternalSchedulerRequest]), - PipelineConfigs: make(map[string]v1alpha1.Pipeline), - }, - Monitor: sharedPipelineMonitor(), - } - // Mirror the production KVM pipeline for the dimensions that shape a failover - // partition: AZ, trait compatibility and whole-host capacity. - pipeline := v1alpha1.Pipeline{ - ObjectMeta: metav1.ObjectMeta{Name: evacProbePipeline}, - Spec: v1alpha1.PipelineSpec{ - Type: v1alpha1.PipelineTypeFilterWeigher, - Filters: []v1alpha1.FilterSpec{ - {Name: "filter_correct_az"}, - {Name: "filter_has_requested_traits"}, - {Name: "filter_has_enough_capacity"}, - }, - }, - } - result := lib.InitNewFilterWeigherPipeline( - context.Background(), k8sClient, pipeline.Name, - filters.Index, pipeline.Spec.Filters, - weighers.Index, pipeline.Spec.Weighers, - controller.Monitor, - ) - if len(result.FilterErrors) > 0 || len(result.WeigherErrors) > 0 { - t.Fatalf("init pipeline: filters=%v weighers=%v", result.FilterErrors, result.WeigherErrors) - } - controller.Pipelines[pipeline.Name] = result.Pipeline - controller.PipelineConfigs[pipeline.Name] = pipeline - - api := &integrationHTTPAPI{delegate: controller} - mux := http.NewServeMux() - mux.HandleFunc("/scheduler/nova/external", api.handle) - server := httptest.NewServer(mux) - - return &integrationTestEnv{ - k8sClient: k8sClient, - server: server, - schedulerURL: server.URL + "/scheduler/nova/external", - flavors: flavors, - hypervisors: hvsByName, - } -} - -// ============================================================================ -// Environment Helper Methods -// ============================================================================ - -// Generate runs the real generation pipeline (real seeder, HTTP path finder, -// clique builder and planner) and returns the desired reservations. -func (env *integrationTestEnv) Generate(t *testing.T, ratio float64, minimum int) map[corev1.ObjectReference]v1alpha1.Reservation { - t.Helper() - g := &pipelineGenerator{ - Client: env.k8sClient, - seeder: &seeder{k8s: env.k8sClient}, - pathFinder: &evacuationPathFinder{httpClient: http.DefaultClient, schedulerURL: env.schedulerURL}, - partitionBuilder: cliquePartitionBuilder{}, - reservationPlanner: ratioReservationPlanner{ratio: ratio, minimum: minimum}, - } - desired, _, ok := g.Generate(context.Background()) - if !ok { - t.Fatal("Generate returned ok=false") - } - return desired -} - -// AcceptsByHost returns, per host, the set of flavors the real scheduler accepts -// on it — the ground truth the reservations must respect. It reuses the path -// finder's own evacuation-intent empty-host probe so the answer matches exactly -// what the generation pipeline sees. It probes every availability zone present -// in the fleet (the AZ filter only ever admits hosts of the seed's own zone), so -// the ground truth covers multi-AZ fleets too. -func (env *integrationTestEnv) AcceptsByHost(t *testing.T) map[string]map[string]bool { - t.Helper() - zones := map[string]bool{} - for _, hv := range env.hypervisors { - zones[hv.Labels[corev1.LabelTopologyZone]] = true - } - finder := &evacuationPathFinder{httpClient: http.DefaultClient, schedulerURL: env.schedulerURL} - accepts := map[string]map[string]bool{} - for _, flavor := range env.flavors { - for zone := range zones { - accepting, err := finder.fetchSeedCandidates(context.Background(), Seed{ - Flavor: Flavor{ - Name: flavor.Name, - MemoryMB: flavor.MemoryMB, - VCPUs: flavor.VCPUs, - ExtraSpecs: flavor.ExtraSpecs, - }, - AvailabilityZone: zone, - }, env.hypervisors) - if err != nil { - t.Fatalf("probe accepting for %q in %q: %v", flavor.Name, zone, err) - } - for _, hv := range accepting { - if accepts[hv.Name] == nil { - accepts[hv.Name] = map[string]bool{} - } - accepts[hv.Name][flavor.Name] = true - } - } - } - return accepts -} - -// ============================================================================ -// Verification Helpers -// ============================================================================ - -// VerifyWholeHostEvacuable asserts the ADR 0011 invariant: for every reservation, -// the target host must accept every protected flavor each of its recorded source -// hosts accepts. Otherwise that source's VMs of the uncovered flavor would have -// nowhere to restart on the reserved target. -func (env *integrationTestEnv) VerifyWholeHostEvacuable(t *testing.T, desired map[corev1.ObjectReference]v1alpha1.Reservation) { - t.Helper() - accepts := env.AcceptsByHost(t) - for _, res := range desired { - target := res.Spec.TargetHost - if res.Spec.FailoverHostReservation == nil { - t.Fatalf("reservation %s has no FailoverHostReservation spec", res.Name) - } - for _, source := range res.Spec.FailoverHostReservation.SourceHosts { - for flavor := range accepts[source] { - if !accepts[target][flavor] { - t.Errorf("reservation %s: target %q is recorded as a failover host for source %q, "+ - "but cannot run flavor %q that %q accepts — the source's %q VMs could not be evacuated onto it", - res.Name, target, source, flavor, source, flavor) - } - } - } - } -} - -// VerifySiblingSources asserts that in a partition reserving more than one -// failover host, every reservation lists each *other* reserved host among its -// source hosts. The planner keeps the reserved hosts in each other's source -// lists on purpose: once a reserved host absorbs an evacuation and returns to -// service it instantly needs a failover target of its own, and leaving it a -// recorded source means that target is already in place. -func (env *integrationTestEnv) VerifySiblingSources(t *testing.T, desired map[corev1.ObjectReference]v1alpha1.Reservation) { - t.Helper() - targets := make([]string, 0, len(desired)) - for _, res := range desired { - targets = append(targets, res.Spec.TargetHost) - } - for _, res := range desired { - if res.Spec.FailoverHostReservation == nil { - t.Fatalf("reservation %s has no FailoverHostReservation spec", res.Name) - } - for _, sibling := range targets { - if sibling == res.Spec.TargetHost { - continue - } - if !slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sibling) { - t.Errorf("reservation for target %q does not list sibling reserved host %q as a source", - res.Spec.TargetHost, sibling) - } - } - } -} - -// evacProbePipeline is the name of the real Nova filter/weigher pipeline the -// probe exercises. -const evacProbePipeline = "kvm-general-purpose-load-balancing" - -// integrationHTTPAPI is a thin, deterministic HTTP front-end for the real Nova -// filter/weigher pipeline — the same shape the failover package's integration -// tests use. It does NOT shuffle, so the probe answers are stable. -type integrationHTTPAPI struct { - delegate nova.HTTPAPIDelegate -} - -func (a *integrationHTTPAPI) handle(w http.ResponseWriter, r *http.Request) { - defer r.Body.Close() - var reqData novaapi.ExternalSchedulerRequest - if err := json.NewDecoder(r.Body).Decode(&reqData); err != nil { - http.Error(w, "decode", http.StatusBadRequest) - return - } - rawBytes, err := json.Marshal(reqData) - if err != nil { - http.Error(w, "marshal", http.StatusInternalServerError) - return - } - decision := &v1alpha1.Decision{ - TypeMeta: metav1.TypeMeta{Kind: "Decision", APIVersion: "cortex.cloud/v1alpha1"}, - ObjectMeta: metav1.ObjectMeta{GenerateName: "nova-"}, - Spec: v1alpha1.DecisionSpec{ - SchedulingDomain: v1alpha1.SchedulingDomainNova, - PipelineRef: corev1.ObjectReference{Name: evacProbePipeline}, - ResourceID: reqData.Spec.Data.InstanceUUID, - NovaRaw: &runtime.RawExtension{Raw: rawBytes}, - }, - } - if err := a.delegate.ProcessNewDecisionFromAPI(r.Context(), decision); err != nil { - http.Error(w, "process", http.StatusInternalServerError) - return - } - if decision.Status.Result == nil { - http.Error(w, "no result", http.StatusInternalServerError) - return - } - w.Header().Set("Content-Type", "application/json") - if err := json.NewEncoder(w).Encode(novaapi.ExternalSchedulerResponse{Hosts: decision.Status.Result.OrderedHosts}); err != nil { - http.Error(w, "encode", http.StatusInternalServerError) - } -} - -// ============================================================================ -// Object Creation Functions -// ============================================================================ - -// hvWithTraits builds a fixed-size hypervisor in az1 with the given traits. The -// size is uniform across the fleet so every host is a valid gang-fit target for -// every other, isolating the trait (not capacity) as the compatibility axis. -func hvWithTraits(name string, traits ...string) *hv1.Hypervisor { - h := buildHV(name, 64, 32) - h.Status.Traits = traits - return h -} - -// hvPtrInAZ builds a fixed-size hypervisor (matching hvWithTraits) in the given -// availability zone, so a scenario can exercise the AZ filter rather than only -// az1 (buildHV, which it reuses, hardcodes az1). It is distinct from seeding -// test's hvInAZ, which returns a value and sets no capacity. -func hvPtrInAZ(name, az string) *hv1.Hypervisor { - h := buildHV(name, 64, 32) - h.Labels[corev1.LabelTopologyZone] = az - return h -} - -// flavor builds a generic flavor that requires the given trait (or none). Its -// hypervisor type is set to a supported value ("ch") only so the seeder keeps -// it; the specific type is irrelevant to what this test exercises. -func flavor(name, requiredTrait string) compute.FlavorInGroup { - extra := map[string]string{"capabilities:hypervisor_type": "ch"} - if requiredTrait != "" { - extra["trait:"+requiredTrait] = "required" - } - return compute.FlavorInGroup{Name: name, VCPUs: 1, MemoryMB: 1024, ExtraSpecs: extra} -} - -// sharedPipelineMonitor guards the one-time Prometheus collector registration -// the pipeline monitor performs, so multiple environments can be built in one -// test run without a duplicate-registration panic. -var ( - pipelineMonitorOnce sync.Once - pipelineMonitor lib.FilterWeigherPipelineMonitor -) - -func sharedPipelineMonitor() lib.FilterWeigherPipelineMonitor { - pipelineMonitorOnce.Do(func() { pipelineMonitor = lib.NewPipelineMonitor() }) - return pipelineMonitor -} diff --git a/internal/scheduling/reservations/failoverhost/matching.go b/internal/scheduling/reservations/failoverhost/matching.go index 9d3276a24..5e0c689a9 100644 --- a/internal/scheduling/reservations/failoverhost/matching.go +++ b/internal/scheduling/reservations/failoverhost/matching.go @@ -72,39 +72,8 @@ func (m *Matcher) ConfirmAndMarkInUse(ctx context.Context, request api.ExternalS } // Re-derive the selected reservation with the same deterministic rule the // filter and weigher used over the same snapshot, so we confirm the one they - // shaped the candidates around: among the reservations whose partition - // includes the source host, prefer the one already in use by this very source - // (gang-evacuation reuse), never one in use by a different source (that would - // steal an active landing zone), otherwise the first Active one by target host - // name. A not-yet-in-use reservation is only eligible once it is Active - // (drained and empty); a still-draining, not-yet-Active one is never pinned. - var matching []v1alpha1.Reservation - for _, res := range reservations.Items { - if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { - continue - } - if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { - matching = append(matching, res) - } - } - sort.Slice(matching, func(i, j int) bool { - return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost - }) - var selected *v1alpha1.Reservation - for i := range matching { - res := &matching[i] - if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { - if selected == nil && meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostActive) { - selected = res - } - continue - } - if res.Status.FailoverHostReservation != nil && - res.Status.FailoverHostReservation.SourceHost == sourceHost { - selected = res - break - } - } + // shaped the candidates around. + selected := SelectFailoverHost(reservations.Items, sourceHost) if selected == nil { // Either no reservation can rescue this source, or every matching one is // already absorbing another source host's gang. Surface it either way. @@ -152,3 +121,48 @@ func (m *Matcher) markInUse(ctx context.Context, res *v1alpha1.Reservation, sour res.Status.FailoverHostReservation.SourceHost = sourceHost return m.Status().Patch(ctx, res, client.MergeFrom(old)) } + +// SelectFailoverHost picks the one failover host reservation a source host should +// gang-evacuate onto, deterministically. It is a pure function of the reservation +// snapshot, so the filter_failover_host filter, the weigher_failover_host weigher +// and this Matcher — which all run over the same snapshot within one pipeline +// execution — select the same target without coordinating. +// +// Among the reservations whose partition includes sourceHost it prefers the one +// already in use by this very source (the gang-evacuation path — the whole gang +// lands on the same target), never picks one in use by a different source (which +// would steal an active landing zone), and otherwise takes the first Active one +// by target host name. A not-yet-in-use reservation is only eligible once it is +// Active (its host drained and empty); a still-draining, not-yet-Active one is +// never selected (though the filter still blocks it from ordinary placement). +// Returns nil when nothing is selectable. +func SelectFailoverHost(reservations []v1alpha1.Reservation, sourceHost string) *v1alpha1.Reservation { + var matching []v1alpha1.Reservation + for _, res := range reservations { + if res.Spec.Type != v1alpha1.ReservationTypeFailoverHost || res.Spec.FailoverHostReservation == nil { + continue + } + if slices.Contains(res.Spec.FailoverHostReservation.SourceHosts, sourceHost) { + matching = append(matching, res) + } + } + sort.Slice(matching, func(i, j int) bool { + return matching[i].Spec.TargetHost < matching[j].Spec.TargetHost + }) + var firstActiveFree *v1alpha1.Reservation + for i := range matching { + res := &matching[i] + if !meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostInUse) { + if firstActiveFree == nil && meta.IsStatusConditionTrue(res.Status.Conditions, v1alpha1.ConditionTypeFailoverHostActive) { + firstActiveFree = res + } + continue + } + // In use: reuse it only if it is in use by this very source host. + if res.Status.FailoverHostReservation != nil && + res.Status.FailoverHostReservation.SourceHost == sourceHost { + return res + } + } + return firstActiveFree +} From cabd041eb6cafc8343b746396458b35852213dcb Mon Sep 17 00:00:00 2001 From: Philipp Matthes Date: Wed, 4 Nov 2026 14:44:52 +0100 Subject: [PATCH 7/7] test(failoverhost): cover shared selection and plugin options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a direct table test for SelectFailoverHost — the deterministic rule shared by the filter, weigher and matcher — pinning every branch: non-failover and nil-spec reservations are ignored, partition membership is required, the name-first Active reservation wins, a not-yet-Active one is skipped, an in-use reservation for this source is reused over a name-earlier free one, and one in use by another source is never stolen (bringing it to 100% coverage). Also add Validate() tests for both plugin option structs, and rename matcher_monitor.go to matching_monitor.go to match the matching.go rename. Signed-off-by: Philipp Matthes --- .../filters/filter_failover_host_test.go | 9 ++ .../weighers/weigher_failover_host_test.go | 9 ++ ...matcher_monitor.go => matching_monitor.go} | 0 .../failoverhost/matching_test.go | 127 ++++++++++++++++++ 4 files changed, 145 insertions(+) rename internal/scheduling/reservations/failoverhost/{matcher_monitor.go => matching_monitor.go} (100%) diff --git a/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go b/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go index d10844d0b..5929a5f81 100644 --- a/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go +++ b/internal/scheduling/nova/plugins/filters/filter_failover_host_test.go @@ -258,3 +258,12 @@ func TestFilterFailoverHost_Index(t *testing.T) { t.Fatal("expected factory to return a *FilterFailoverHost") } } + +func TestFilterFailoverHost_OptsValidate(t *testing.T) { + if err := (FilterFailoverHostOpts{}).Validate(); err != nil { + t.Errorf("expected nil error, got %v", err) + } + if err := (FilterFailoverHostOpts{Enabled: true}).Validate(); err != nil { + t.Errorf("expected nil error, got %v", err) + } +} diff --git a/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go b/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go index 7079eb2a8..b01558ade 100644 --- a/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go +++ b/internal/scheduling/nova/plugins/weighers/weigher_failover_host_test.go @@ -197,3 +197,12 @@ func TestWeigherFailoverHost_Index(t *testing.T) { t.Fatal("expected factory to return a *WeigherFailoverHost") } } + +func TestWeigherFailoverHost_OptsValidate(t *testing.T) { + if err := (WeigherFailoverHostOpts{}).Validate(); err != nil { + t.Errorf("expected nil error, got %v", err) + } + if err := (WeigherFailoverHostOpts{Enabled: true}).Validate(); err != nil { + t.Errorf("expected nil error, got %v", err) + } +} diff --git a/internal/scheduling/reservations/failoverhost/matcher_monitor.go b/internal/scheduling/reservations/failoverhost/matching_monitor.go similarity index 100% rename from internal/scheduling/reservations/failoverhost/matcher_monitor.go rename to internal/scheduling/reservations/failoverhost/matching_monitor.go diff --git a/internal/scheduling/reservations/failoverhost/matching_test.go b/internal/scheduling/reservations/failoverhost/matching_test.go index a59a40f41..98333b02d 100644 --- a/internal/scheduling/reservations/failoverhost/matching_test.go +++ b/internal/scheduling/reservations/failoverhost/matching_test.go @@ -243,3 +243,130 @@ func TestMatcher_ConfirmAndMarkInUse(t *testing.T) { }) } } + +// nonFailoverReservation builds a reservation of a different type, to prove +// SelectFailoverHost ignores anything that is not a failover host reservation. +func nonFailoverReservation(targetHost string) v1alpha1.Reservation { + return v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "cr-" + targetHost}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: targetHost, + }, + } +} + +// TestSelectFailoverHost pins the deterministic selection rule that the filter, +// the weigher and the matcher all share. Each case lists the reservation +// snapshot and asserts which target host (if any) is selected for source1. +func TestSelectFailoverHost(t *testing.T) { + // nilSpec is a failover host reservation whose FailoverHostReservation spec is + // nil; it must be skipped rather than panic. + nilSpec := matcherReservation("nilspec", "source1") + nilSpec.Spec.FailoverHostReservation = nil + + tests := []struct { + name string + reservations []v1alpha1.Reservation + sourceHost string + want string // target host, or "" for no selection + }{ + { + name: "no reservations at all", + reservations: nil, + sourceHost: "source1", + want: "", + }, + { + name: "ignores non-failover-host reservations", + reservations: []v1alpha1.Reservation{nonFailoverReservation("t1")}, + sourceHost: "source1", + want: "", + }, + { + name: "ignores a failover host reservation with a nil spec", + reservations: []v1alpha1.Reservation{*nilSpec}, + sourceHost: "source1", + want: "", + }, + { + name: "ignores a reservation whose partition excludes the source", + reservations: []v1alpha1.Reservation{*matcherReservation("t1", "otherSource")}, + sourceHost: "source1", + want: "", + }, + { + name: "picks the single active match", + reservations: []v1alpha1.Reservation{*matcherReservation("t1", "source1")}, + sourceHost: "source1", + want: "t1", + }, + { + name: "picks the first active match by target host name", + reservations: []v1alpha1.Reservation{ + *matcherReservation("t2", "source1"), + *matcherReservation("t1", "source1"), + }, + sourceHost: "source1", + want: "t1", + }, + { + name: "a not-yet-active reservation is not selectable", + reservations: []v1alpha1.Reservation{*notActive(matcherReservation("t1", "source1"))}, + sourceHost: "source1", + want: "", + }, + { + name: "skips a not-yet-active one and picks the next active by name", + reservations: []v1alpha1.Reservation{ + *notActive(matcherReservation("t1", "source1")), + *matcherReservation("t2", "source1"), + }, + sourceHost: "source1", + want: "t2", + }, + { + name: "reuses the in-use reservation for this source over a name-earlier free one", + reservations: []v1alpha1.Reservation{ + *matcherReservation("t0", "source1"), + *inUse(matcherReservation("t1", "source1"), "source1"), + }, + sourceHost: "source1", + want: "t1", + }, + { + name: "never selects a reservation in use by another source", + reservations: []v1alpha1.Reservation{ + *inUse(matcherReservation("t1", "source1", "source2"), "source2"), + *matcherReservation("t2", "source1", "source2"), + }, + sourceHost: "source1", + want: "t2", + }, + { + name: "all matches in use by others yields no selection", + reservations: []v1alpha1.Reservation{ + *inUse(matcherReservation("t1", "source1", "source2"), "source2"), + }, + sourceHost: "source1", + want: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SelectFailoverHost(tt.reservations, tt.sourceHost) + if tt.want == "" { + if got != nil { + t.Fatalf("expected no selection, got %q", got.Spec.TargetHost) + } + return + } + if got == nil { + t.Fatalf("expected %q, got no selection", tt.want) + } + if got.Spec.TargetHost != tt.want { + t.Fatalf("expected %q, got %q", tt.want, got.Spec.TargetHost) + } + }) + } +}