From ab8f70ccc9b54c2015daa1bc1f990c4e9283ab90 Mon Sep 17 00:00:00 2001 From: Malte <140147670+umswmayj@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:56:35 +0100 Subject: [PATCH 1/3] api: add OperationID to in-flight reservation spec Add OperationID to InFlightReservationSpec to group all in-flight reservations created within a single scheduling call, and refine the VMID and Intent field documentation. Regenerate the Reservation CRD. Signed-off-by: Malte <140147670+umswmayj@users.noreply.github.com> --- api/v1alpha1/reservation_types.go | 28 +++++++++++++++++-- .../files/crds/cortex.cloud_reservations.yaml | 28 +++++++++++++++++-- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/api/v1alpha1/reservation_types.go b/api/v1alpha1/reservation_types.go index 0847c27ca..ac3b005df 100644 --- a/api/v1alpha1/reservation_types.go +++ b/api/v1alpha1/reservation_types.go @@ -178,7 +178,14 @@ type FailoverReservationSpec struct { // in buildup. type InFlightReservationSpec struct { // VMID is the OpenStack server uuid from Nova assigned to the virtual - // machine expected to land on this reservation slot. + // machine expected to land on this reservation slot. It identifies the + // subject of the reservation (what is being placed), which is distinct + // from the reservation's owner (what controls its lifecycle, expressed via + // ownerReferences). + // + // TODO: generalize this into a domain-agnostic subject identifier once a + // non-Nova use case needs in-flight reservations. For now (KISS) we always + // have a VM uuid, so we keep this Nova-specific field. VMID string `json:"vmID,omitempty"` // UserID is the identifier of the user who owns the virtual machine. UserID string `json:"userID,omitempty"` @@ -186,9 +193,26 @@ type InFlightReservationSpec struct { // the virtual machine. ProjectID string `json:"projectID,omitempty"` // Intent defines which kind of virtual machine lifecycle operation - // triggered the placement of this in-flight reservation. + // triggered the placement of this in-flight reservation. The reconciler + // dispatches on this to decide which terminal signal releases the + // reservation. // +kubebuilder:validation:Optional Intent SchedulingIntent `json:"intent"` + // OperationID groups all in-flight reservations created within a single + // scheduling call (e.g. one reservation per candidate host for an initial + // placement, or target + source host for a live migration). It is a + // Cortex-generated, time-sortable uuid (UUIDv7), independent of any + // external caller's request id, so uniqueness does not depend on callers. + // + // When one operation reaches a terminal state, the reconciler releases + // exactly the reservations sharing this OperationID, leaving concurrent + // operations on the same VM untouched. For VM-owned in-flight reservations, + // when multiple operations exist for the same VM the newest one wins: + // operations are ordered by (metadata.creationTimestamp, OperationID), with + // the UUIDv7 OperationID breaking ties within the timestamp's one-second + // resolution, and older operations' reservations are released. + // +kubebuilder:validation:Optional + OperationID string `json:"operationID,omitempty"` } // FailoverHostReservationSpec defines the spec fields specific to failover diff --git a/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml b/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml index 24d7a2b41..b1e53fdef 100644 --- a/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml +++ b/helm/library/cortex/files/crds/cortex.cloud_reservations.yaml @@ -282,7 +282,25 @@ spec: intent: description: |- Intent defines which kind of virtual machine lifecycle operation - triggered the placement of this in-flight reservation. + triggered the placement of this in-flight reservation. The reconciler + dispatches on this to decide which terminal signal releases the + reservation. + type: string + operationID: + description: |- + OperationID groups all in-flight reservations created within a single + scheduling call (e.g. one reservation per candidate host for an initial + placement, or target + source host for a live migration). It is a + Cortex-generated, time-sortable uuid (UUIDv7), independent of any + external caller's request id, so uniqueness does not depend on callers. + + When one operation reaches a terminal state, the reconciler releases + exactly the reservations sharing this OperationID, leaving concurrent + operations on the same VM untouched. For VM-owned in-flight reservations, + when multiple operations exist for the same VM the newest one wins: + operations are ordered by (metadata.creationTimestamp, OperationID), with + the UUIDv7 OperationID breaking ties within the timestamp's one-second + resolution, and older operations' reservations are released. type: string projectID: description: |- @@ -296,7 +314,13 @@ spec: vmID: description: |- VMID is the OpenStack server uuid from Nova assigned to the virtual - machine expected to land on this reservation slot. + machine expected to land on this reservation slot. It identifies the + subject of the reservation (what is being placed), which is distinct + from the reservation's owner (what controls its lifecycle, expressed via + ownerReferences). + + non-Nova use case needs in-flight reservations. For now (KISS) we always + have a VM uuid, so we keep this Nova-specific field. type: string type: object resources: From 626373ddda69537b3832e991195864a66526c266 Mon Sep 17 00:00:00 2001 From: Malte <140147670+umswmayj@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:56:35 +0100 Subject: [PATCH 2/3] inflight: rework reservation controller for operation-based release Rework the in-flight reservation controller to release reservations by operation: an OwnerHandler dispatches on the owner-reference Kind and decides when a terminal signal releases the reservations sharing an OperationID. Add owner-handler dispatch, controller metrics, and wire the Monitor and default VirtualMachine owner kind in the manager. Signed-off-by: Malte <140147670+umswmayj@users.noreply.github.com> --- cmd/manager/main.go | 5 +- .../reservations/inflight/controller.go | 571 ++++----- .../reservations/inflight/controller_test.go | 1072 ++++++++--------- .../reservations/inflight/metrics.go | 70 ++ .../reservations/inflight/owner_handler.go | 480 ++++++++ .../inflight/owner_handler_test.go | 293 +++++ .../reservations/inflight/vm_client.go | 120 -- .../reservations/inflight/vm_client_test.go | 167 --- 8 files changed, 1633 insertions(+), 1145 deletions(-) create mode 100644 internal/scheduling/reservations/inflight/metrics.go create mode 100644 internal/scheduling/reservations/inflight/owner_handler.go create mode 100644 internal/scheduling/reservations/inflight/owner_handler_test.go delete mode 100644 internal/scheduling/reservations/inflight/vm_client.go delete mode 100644 internal/scheduling/reservations/inflight/vm_client_test.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index a567fd7f4..a3b00c65d 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -494,9 +494,8 @@ func main() { if slices.Contains(mainConfig.EnabledControllers, "inflight-reservation-controller") { setupLog.Info("enabling controller", "controller", "inflight-reservation-controller") - config := conf.GetConfigOrDie[inflight.NovaVMClientConfig]() - vmClient := inflight.NewNovaVMClient(config) - controller := &inflight.Controller{Client: multiclusterClient, VMClient: vmClient} + monitor := inflight.NewMonitor(metrics.Registry) + controller := &inflight.Controller{Client: multiclusterClient, Monitor: monitor, DefaultOwnerKind: "VirtualMachine"} if err := controller.SetupWithManager(ctx, mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "inflight-reservation-controller") diff --git a/internal/scheduling/reservations/inflight/controller.go b/internal/scheduling/reservations/inflight/controller.go index 8ce4f22fe..3b4d606f3 100644 --- a/internal/scheduling/reservations/inflight/controller.go +++ b/internal/scheduling/reservations/inflight/controller.go @@ -7,50 +7,86 @@ import ( "context" "errors" "reflect" - "slices" "time" - novaapi "github.com/cobaltcore-dev/cortex/api/external/nova" - hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - "github.com/cobaltcore-dev/cortex/api/v1alpha1" "github.com/cobaltcore-dev/cortex/pkg/multicluster" + hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" - "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -var ( - idxReservationByTargetHost = "spec.targetHost" - idxReservationByTargetHostFn = func(obj client.Object) []string { - res, ok := obj.(*v1alpha1.Reservation) - if !ok { - return nil - } - if res.Spec.Type != v1alpha1.ReservationTypeInFlight { - return nil - } - if res.Spec.TargetHost == "" { - return nil - } - return []string{res.Spec.TargetHost} - } +// Field index keys used by the controller and the owner handlers. +const ( + idxReservationByTargetHost = "spec.targetHost" + idxReservationByOperationID = "spec.inFlightReservation.operationID" + idxReservationByVMID = "spec.inFlightReservation.vmID" ) -// Controller owns the lifecycle of in-flight reservations. +// indexReservationByTargetHost indexes in-flight reservations by their target +// host so handlers can requeue reservations affected by a hypervisor change. +func indexReservationByTargetHost(obj client.Object) []string { + res, ok := obj.(*v1alpha1.Reservation) + if !ok || res.Spec.Type != v1alpha1.ReservationTypeInFlight || res.Spec.TargetHost == "" { + return nil + } + return []string{res.Spec.TargetHost} +} + +// indexReservationByOperationID indexes in-flight reservations by their +// operation id so the whole group can be released together. +func indexReservationByOperationID(obj client.Object) []string { + res, ok := obj.(*v1alpha1.Reservation) + if !ok || res.Spec.Type != v1alpha1.ReservationTypeInFlight { + return nil + } + if res.Spec.InFlightReservation == nil || res.Spec.InFlightReservation.OperationID == "" { + return nil + } + return []string{res.Spec.InFlightReservation.OperationID} +} + +// indexReservationByVMID indexes in-flight reservations by their subject VM uuid +// so a handler can find all operations for one VM (e.g. for supersession). +func indexReservationByVMID(obj client.Object) []string { + res, ok := obj.(*v1alpha1.Reservation) + if !ok || res.Spec.Type != v1alpha1.ReservationTypeInFlight { + return nil + } + if res.Spec.InFlightReservation == nil || res.Spec.InFlightReservation.VMID == "" { + return nil + } + return []string{res.Spec.InFlightReservation.VMID} +} + +// Controller owns the lifecycle of in-flight reservations. It reconciles each +// in-flight reservation as the primary resource and, based on the reservation's +// owner-reference Kind, dispatches to an OwnerHandler that decides when the +// owning operation is complete. On completion all reservations sharing the same +// OperationID are released together; a TTL backstop releases reservations whose +// owning operation never signals completion. type Controller struct { client.Client - // VMClient is a client to call the source of truth for VMs. - VMClient VMClient + // Monitor records controller metrics (e.g. TTL expiries). May be nil. + Monitor *Monitor + + // DefaultOwnerKind is the owner handler Kind applied to in-flight + // reservations that carry no recognised owner reference (fallback during + // the transition before producers set owner references). Only applied to + // Nova reservations that carry a VM subject. Typically "VirtualMachine". + DefaultOwnerKind string + + // handlers maps an owner-reference Kind to its OwnerHandler. Built in + // SetupWithManager from the registered handlers. + handlers map[string]OwnerHandler } // Reconcile is part of the main kubernetes reconciliation loop which aims to @@ -68,213 +104,220 @@ func (c *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu obj := new(v1alpha1.Reservation) if err := c.Get(ctx, req.NamespacedName, obj); err != nil { if apierrors.IsNotFound(err) { - // If the custom resource is not found then it usually means - // that it was deleted or not created. + // If the custom resource is not found then it usually means that it + // was deleted (e.g. by a sibling reservation's batch delete) or not + // created. log.V(1).Info("Resource not found. Ignoring since object must be deleted") return ctrl.Result{}, nil } - // Error reading the object - requeue the request. log.Error(err, "Failed to get resource") return ctrl.Result{}, err } - // Sanity checks which should always succeed due to the predicate. + // Sanity checks which should always succeed due to the predicate. If a + // non-in-flight reservation slips through we must not touch it: its status + // belongs to another controller (committed/failover), so we only log. if obj.Spec.Type != v1alpha1.ReservationTypeInFlight { log.Error(errors.New("unexpected reservation type"), - "Received a reservation with an unexpected type", + "Received a reservation with an unexpected type, ignoring", "reservationType", obj.Spec.Type) - orig := obj.DeepCopy() - meta.SetStatusCondition(&obj.Status.Conditions, metav1.Condition{ - Type: v1alpha1.ReservationConditionReady, - Status: metav1.ConditionFalse, - Reason: "UnexpectedType", - Message: "Expected reservation type to be InFlightReservation", - }) - return ctrl.Result{}, c.Status().Patch(ctx, obj, client.MergeFrom(orig)) + return ctrl.Result{}, nil } if obj.Spec.InFlightReservation == nil { log.Error(errors.New("missing in-flight reservation spec"), "Received a reservation with missing in-flight reservation spec") - orig := obj.DeepCopy() - meta.SetStatusCondition(&obj.Status.Conditions, metav1.Condition{ - Type: v1alpha1.ReservationConditionReady, - Status: metav1.ConditionFalse, - Reason: "MissingSpec", - Message: "In-flight reservation spec is required when type is InFlightReservation", - }) - return ctrl.Result{}, c.Status().Patch(ctx, obj, client.MergeFrom(orig)) - } - - // Get a list of all hypervisors and check if the instance - // has spawned on any of them. - hvs := new(hv1.HypervisorList) - if err := c.List(ctx, hvs); err != nil { - log.Error(err, "Failed to list hypervisors") + return ctrl.Result{}, c.setReadyCondition(ctx, obj, metav1.ConditionFalse, + "MissingSpec", "In-flight reservation spec is required when type is InFlightReservation") + } + + // TTL backstop: if the owning operation never signalled completion and the + // reservation outlived its EndTime, emit a metric and release it so an + // abandoned operation cannot strand capacity forever. + if expired, err := c.handleTTL(ctx, obj); err != nil || expired { return ctrl.Result{}, err } - found := false - hypervisorName := "" - for _, hv := range hvs.Items { - for _, instance := range hv.Status.Instances { - if instance.ID == obj.Spec.InFlightReservation.VMID { - found = true - hypervisorName = hv.Name - break - } - } - if found { - break - } + + handler := c.handlerFor(ctx, obj) + if handler == nil { + // No handler for this owner kind: we cannot determine terminality, so we + // surface it on the status, emit a metric to alert on, and rely on the + // TTL backstop for cleanup. + log.Info("No owner handler for reservation, awaiting TTL", + "name", obj.Name, "ownerReferences", obj.OwnerReferences) + c.Monitor.RecordAnomaly(AnomalyNoOwnerHandler) + return ctrl.Result{}, c.setReadyCondition(ctx, obj, metav1.ConditionUnknown, + "NoOwnerHandler", "No owner handler registered for this reservation's owner") } - if !found { - // The instance has not spawned on any hypervisor (yet). - // Requeue and check again later. We'll alert on this if there are - // too many requeues without the instance spawning. - - // TODO: delete reservation if spec.endTime is exceeded - log.V(1).Info("Instance has not spawned on any hypervisor yet, requeuing", - "vmID", obj.Spec.InFlightReservation.VMID) - orig := obj.DeepCopy() - meta.SetStatusCondition(&obj.Status.Conditions, metav1.Condition{ - Type: v1alpha1.ReservationConditionReady, - Status: metav1.ConditionUnknown, - Reason: "InstanceNotFound", - Message: "The instance has not spawned on any hypervisor yet", - }) - if err := c.Status().Patch(ctx, obj, client.MergeFrom(orig)); err != nil { - log.Error(err, "Failed to update reservation status") - return ctrl.Result{}, err + // Bind a per-subject reservation lookup scoped to the selected handler, so + // the handler sees exactly the reservations reconciliation would route to + // it (same ownership selection), without reimplementing that logic. + lookup := func(ctx context.Context, vmID string) ([]v1alpha1.Reservation, error) { + return c.reservationsForSubjectHandler(ctx, handler.Kind(), vmID) + } + result, err := handler.TerminalState(ctx, obj, lookup) + if err != nil { + // Surface transient errors so controller-runtime applies its backoff. + log.Error(err, "Owner handler failed to determine terminal state") + return ctrl.Result{}, err + } + if !result.Done { + // Record why we are still waiting; a nil error keeps our RequeueAfter. + reason, message := result.Reason, result.Message + if reason == "" { + reason, message = "AwaitingCompletion", "The owning operation has not reached a terminal state yet" } - return ctrl.Result{RequeueAfter: 10 * time.Second}, nil - } - - // We cannot free this reservation if the instance is currently being - // resized (=migrated to the same hypervisor), i.e. the reservation - // doesn't match the actual size of the vm yet. To check the vm size, - // we need to query the source of truth for vms. Only relevant when the - // instance actually landed on the reservation's target host — for a - // stale reservation (instance on a different host) this branch would - // otherwise fire an unnecessary vmClient call and mark the reservation - // VMSizeMismatch when it should just fall through to the - // awaiting-deletion no-op below. - if hypervisorName == obj.Spec.TargetHost && slices.Contains([]v1alpha1.SchedulingIntent{ - novaapi.RebuildIntent, - novaapi.ResizeIntent, - }, obj.Spec.InFlightReservation.Intent) { - vmSize, err := c.VMClient.GetCurrentVMSize(ctx, obj.Spec.InFlightReservation.VMID) - if err != nil { - log.Error(err, "Failed to get current VM size from vmClient", - "vmID", obj.Spec.InFlightReservation.VMID) + if err := c.setReadyCondition(ctx, obj, metav1.ConditionUnknown, reason, message); err != nil { return ctrl.Result{}, err } - // Compare the resource maps semantically: reflect.DeepEqual on - // resource.Quantity can return false for numerically equal values - // because Quantity's unexported cached string/format state may - // differ between values coming from API decoding (reservation - // spec) and values freshly constructed by the vm client. Using - // Quantity.Cmp avoids getting stuck in VMSizeMismatch forever. - sizeMatches := len(vmSize) == len(obj.Spec.Resources) - if sizeMatches { - for k, want := range obj.Spec.Resources { - got, ok := vmSize[k] - if !ok || got.Cmp(want) != 0 { - sizeMatches = false - break - } - } + return ctrl.Result{RequeueAfter: result.RequeueAfter}, nil + } + + // The owning operation completed: release all reservations in the group. + return ctrl.Result{}, c.releaseOperation(ctx, obj) +} + +// handlerFor returns the OwnerHandler responsible for the reservation's owner +// kind, emitting the ambiguous-owner anomaly metric when more than one handler +// matches. It wraps resolveHandler, which carries the pure selection logic so it +// can be reused (e.g. by the per-subject lookup) without inflating metrics. +func (c *Controller) handlerFor(ctx context.Context, res *v1alpha1.Reservation) OwnerHandler { + chosen, ambiguous := c.resolveHandler(res) + if ambiguous { + c.Monitor.RecordAnomaly(AnomalyAmbiguousOwner) + ctrl.LoggerFrom(ctx).Info("Reservation has multiple owner references mapping to different handlers, using the first", + "name", res.Name, "chosenKind", chosen.Kind()) + } + return chosen +} + +// resolveHandler selects the OwnerHandler for a reservation without side +// effects. Owner references are inspected in order; the first one mapping to a +// registered handler wins, and ambiguous is true when more than one distinct +// handler matched (a misconfigured ownership). +// +// The DefaultOwnerKind fallback applies only to a Nova VM-subject reservation +// that carries NO owner references at all (the transition period before +// producers set them). A reservation that does carry an owner reference but no +// registered handler matches is deliberately left without a handler (surfacing +// as NoOwnerHandler), so e.g. a Rescheduling-owned reservation is never handled +// by the VirtualMachine handler just because it also carries a vmID. +func (c *Controller) resolveHandler(res *v1alpha1.Reservation) (handler OwnerHandler, ambiguous bool) { + var chosen OwnerHandler + matched := map[string]bool{} + for _, ref := range res.OwnerReferences { + h, ok := c.handlers[ref.Kind] + if !ok { + continue } - if !sizeMatches { - log.V(1).Info("VM size does not match reservation size yet, requeuing", - "vmID", obj.Spec.InFlightReservation.VMID, - "reservationSize", obj.Spec.Resources, - "currentVMSize", vmSize) - orig := obj.DeepCopy() - meta.SetStatusCondition(&obj.Status.Conditions, metav1.Condition{ - Type: v1alpha1.ReservationConditionReady, - Status: metav1.ConditionUnknown, - Reason: "VMSizeMismatch", - Message: "The current VM size does not match the reservation size yet", - }) - if err := c.Status().Patch(ctx, obj, client.MergeFrom(orig)); err != nil { - log.Error(err, "Failed to update reservation status") - return ctrl.Result{}, err - } - // TODO: EndTime check needed to catch canceled scenarios. Waiting for VM crd to consider this scenario - return ctrl.Result{RequeueAfter: 10 * time.Second}, nil + matched[ref.Kind] = true + if chosen == nil { + chosen = h } } + if chosen != nil { + return chosen, len(matched) > 1 + } + // Fallback: a Nova VM-subject reservation with no owner references at all is + // handled by the configured default owner handler. A reservation that has + // owner references but none recognised is intentionally not handled here. + if len(res.OwnerReferences) == 0 && + c.DefaultOwnerKind != "" && + res.Spec.SchedulingDomain == v1alpha1.SchedulingDomainNova && + res.Spec.InFlightReservation != nil && res.Spec.InFlightReservation.VMID != "" { + return c.handlers[c.DefaultOwnerKind], false + } + return nil, false +} - // If the instance spawned on the hypervisor expected by the reservation, - // we batch-delete all reservations for that instance, including the - // reconciled one. - if hypervisorName == obj.Spec.TargetHost { - log.V(1).Info("Instance has spawned on the expected hypervisor, deleting reservations", - "vmID", obj.Spec.InFlightReservation.VMID, - "hypervisor", hypervisorName) - reservations := new(v1alpha1.ReservationList) - if err := c.List(ctx, reservations, client.MatchingFields{ - idxReservationByTargetHost: hypervisorName, - }); err != nil { - log.Error(err, "Failed to list reservations for hypervisor", - "hypervisor", hypervisorName) - return ctrl.Result{}, err - } - for _, res := range reservations.Items { - if res.Spec.InFlightReservation == nil { - continue // Not an in-flight reservation, skip. - } - if res.Spec.InFlightReservation.VMID != obj.Spec.InFlightReservation.VMID { - continue // Not the same instance, skip. - } - if err := c.Delete(ctx, &res); err != nil { - log.Error(err, "Failed to delete reservation", - "reservation", res.Name, - "vmID", res.Spec.InFlightReservation.VMID) - return ctrl.Result{}, err - } - log.V(1).Info("Deleted reservation", - "reservation", res.Name, - "vmID", res.Spec.InFlightReservation.VMID) +// reservationsForSubjectHandler returns the in-flight reservations with the +// given VM subject that resolve to the given handler kind. It applies the same +// ownership selection as reconciliation (resolveHandler), so "owned by this +// handler" means the same thing when dispatching and when a handler inspects +// sibling reservations. This backs the SubjectReservations lookup passed to +// TerminalState and is read-only (no metrics). +func (c *Controller) reservationsForSubjectHandler(ctx context.Context, handlerKind, vmID string) ([]v1alpha1.Reservation, error) { + list := &v1alpha1.ReservationList{} + if err := c.List(ctx, list, client.MatchingFields{idxReservationByVMID: vmID}); err != nil { + return nil, err + } + out := list.Items[:0] + for i := range list.Items { + res := &list.Items[i] + if h, _ := c.resolveHandler(res); h != nil && h.Kind() == handlerKind { + out = append(out, *res) } - return ctrl.Result{}, nil } + return out, nil +} - // This reservation will be deleted by the controller when the - // instance spawns on the expected hypervisor, so we don't need to do - // anything else here. - log.V(1).Info("Reservation stale -- awaiting deletion", - "vmID", obj.Spec.InFlightReservation.VMID, - "expectedHypervisor", obj.Spec.TargetHost, - "actualHypervisor", hypervisorName) - return ctrl.Result{}, nil +// handleTTL releases the reservation if it has exceeded its EndTime. It returns +// expired=true when the reservation was released (or its release failed). +func (c *Controller) handleTTL(ctx context.Context, res *v1alpha1.Reservation) (bool, error) { + if res.Spec.EndTime == nil { + return false, nil + } + if time.Now().Before(res.Spec.EndTime.Time) { + return false, nil + } + log := ctrl.LoggerFrom(ctx) + log.Info("In-flight reservation exceeded its TTL, releasing", + "name", res.Name, + "endTime", res.Spec.EndTime.Time, + "intent", res.Spec.InFlightReservation.Intent) + c.Monitor.RecordTTLExpiry(res.Spec.InFlightReservation.Intent) + // Release the whole operation: a TTL expiry means the operation is + // abandoned, so its sibling reservations are stale too. + return true, c.releaseOperation(ctx, res) } -// handleReservations generates a new event handler for in flight reservations. -func (c *Controller) handleReservations() handler.EventHandler { - handler := handler.Funcs{} - handler.CreateFunc = func(ctx context.Context, evt event.CreateEvent, - queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { +// releaseOperation deletes every in-flight reservation that shares the given +// reservation's OperationID (including the reservation itself). When no +// OperationID is set we fall back to deleting just this reservation, so legacy +// reservations are still cleaned up. Deletion is idempotent: siblings already +// removed by a concurrent reconcile are ignored. +func (c *Controller) releaseOperation(ctx context.Context, res *v1alpha1.Reservation) error { + log := ctrl.LoggerFrom(ctx) + operationID := res.Spec.InFlightReservation.OperationID - queue.Add(ctrl.Request{NamespacedName: client.ObjectKey{ - Name: evt.Object.(*v1alpha1.Reservation).Name, // cluster-scoped crd - }}) + if operationID == "" { + log.V(1).Info("Releasing single in-flight reservation (no operation id)", "name", res.Name) + return client.IgnoreNotFound(c.Delete(ctx, res)) } - handler.UpdateFunc = func(ctx context.Context, evt event.UpdateEvent, - queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { - queue.Add(ctrl.Request{NamespacedName: client.ObjectKey{ - Name: evt.ObjectOld.(*v1alpha1.Reservation).Name, // cluster-scoped crd - }}) + siblings := new(v1alpha1.ReservationList) + if err := c.List(ctx, siblings, client.MatchingFields{idxReservationByOperationID: operationID}); err != nil { + log.Error(err, "Failed to list reservations for operation", "operationID", operationID) + return err + } + for i := range siblings.Items { + sibling := &siblings.Items[i] + if err := client.IgnoreNotFound(c.Delete(ctx, sibling)); err != nil { + log.Error(err, "Failed to delete reservation", + "reservation", sibling.Name, "operationID", operationID) + return err + } + log.V(1).Info("Released in-flight reservation", + "reservation", sibling.Name, "operationID", operationID) } - handler.DeleteFunc = func(ctx context.Context, evt event.DeleteEvent, - queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { + return nil +} - queue.Add(ctrl.Request{NamespacedName: client.ObjectKey{ - Name: evt.Object.(*v1alpha1.Reservation).Name, // cluster-scoped crd - }}) +// setReadyCondition patches the reservation's Ready condition via the status +// subresource. +func (c *Controller) setReadyCondition(ctx context.Context, res *v1alpha1.Reservation, status metav1.ConditionStatus, reason, message string) error { + orig := res.DeepCopy() + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ReservationConditionReady, + Status: status, + Reason: reason, + Message: message, + }) + if err := c.Status().Patch(ctx, res, client.MergeFrom(orig)); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "Failed to update reservation status") + return err } - return handler + return nil } // predicateReservations generates a new predicate for in flight reservations. @@ -294,63 +337,12 @@ func (c *Controller) predicateReservations() predicate.Predicate { }) } -// handleHypervisors generates a new event handler for hypervisors. -func (c *Controller) handleHypervisors() handler.EventHandler { - handler := handler.Funcs{} - enqueueCorrespondingReservations := func(ctx context.Context, hvName string, - queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { - - log := ctrl.LoggerFrom(ctx) - log.V(1).Info("Enqueuing reservations corresponding to hypervisor", - "hypervisor", hvName) - // Requeue all reservations targeting this hypervisor, since the - // instance list has changed and we might find the instance for - // some in-flight reservation now. - reservations := &v1alpha1.ReservationList{} - if err := c.List(ctx, reservations, client.MatchingFields{ - idxReservationByTargetHost: hvName, - }); err != nil { - log.Error(err, "Failed to list reservations for hypervisor", - "hypervisor", hvName) - return - } - for _, res := range reservations.Items { - log.V(1).Info("Enqueuing reservation for reconciliation", - "reservation", res.Name, - "targetHost", res.Spec.TargetHost, - "hypervisor", hvName) - queue.Add(ctrl.Request{NamespacedName: client.ObjectKey{ - Name: res.Name, // cluster-scoped crd - }}) - } - } - handler.CreateFunc = func(ctx context.Context, evt event.CreateEvent, - queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { - - hv := evt.Object.(*hv1.Hypervisor) - enqueueCorrespondingReservations(ctx, hv.Name, queue) - } - handler.UpdateFunc = func(ctx context.Context, evt event.UpdateEvent, - queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { - - hv := evt.ObjectNew.(*hv1.Hypervisor) - enqueueCorrespondingReservations(ctx, hv.Name, queue) - } - handler.DeleteFunc = func(ctx context.Context, evt event.DeleteEvent, - queue workqueue.TypedRateLimitingInterface[reconcile.Request]) { - - hv := evt.Object.(*hv1.Hypervisor) - enqueueCorrespondingReservations(ctx, hv.Name, queue) - } - return handler -} - -// predicateHypervisors generates a new predicate for hypervisors. Update -// events are filtered to only trigger when the Status.Instances list actually -// changes, since that is the only field this controller consumes; without -// this filter, unrelated status updates from the hypervisor operator would -// cause a list + enqueue of every reservation targeting the host. -func (c *Controller) predicateHypervisors() predicate.Predicate { +// hypervisorInstancesChangedPredicate filters Hypervisor events to only those +// that change Status.Instances, since that is the only field the VM owner +// handler consumes. Without it, unrelated status updates from the hypervisor +// operator would cause a list + enqueue of every reservation targeting the +// host. +func hypervisorInstancesChangedPredicate() predicate.Predicate { return predicate.Funcs{ CreateFunc: func(evt event.CreateEvent) bool { _, ok := evt.Object.(*hv1.Hypervisor) @@ -381,49 +373,68 @@ func (c *Controller) predicateHypervisors() predicate.Predicate { // SetupWithManager sets up the controller with the Manager and a multicluster // client. The multicluster client is used to watch for changes in the // Reservation CRD across all clusters and trigger reconciliations accordingly. +// The watch set is the union of the primary Reservation watch and every +// registered owner handler's declared secondary watches (deduplicated by type). func (c *Controller) SetupWithManager(ctx context.Context, mgr ctrl.Manager) (err error) { // Check that the provided client is a multicluster client, since we need - // that to watch for hypervisors across clusters. Do this before adding - // any runnables so a misconfigured setup fails fast. + // that to watch across clusters. Do this before adding any runnables so a + // misconfigured setup fails fast. mcl, ok := c.Client.(*multicluster.Client) if !ok { return errors.New("provided client must be a multicluster client") } - // Add the vm client as runnable to the manager. - if err := mgr.Add(manager.RunnableFunc(func(ctx context.Context) error { - return c.VMClient.StartWithKubernetesSecrets(ctx, c.Client) - })); err != nil { + + // Register the built-in owner handlers. + vmHandler := &VirtualMachineHandler{Client: c.Client} + c.handlers = map[string]OwnerHandler{vmHandler.Kind(): vmHandler} + + // Index reservations by their target host, operation id and VM uuid so + // handlers can requeue by host, release a whole operation group, and find + // all operations for one VM (supersession). + if err := mcl.IndexField(ctx, &v1alpha1.Reservation{}, &v1alpha1.ReservationList{}, + idxReservationByTargetHost, indexReservationByTargetHost); err != nil { return err } - bldr := multicluster.BuildController(mcl, mgr) - // The reservation crd & hypervisor crd may be distributed across multiple - // remote clusters. - bldr, err = bldr.WatchesMulticluster(&v1alpha1.Reservation{}, - c.handleReservations(), - c.predicateReservations(), - ) - if err != nil { + if err := mcl.IndexField(ctx, &v1alpha1.Reservation{}, &v1alpha1.ReservationList{}, + idxReservationByOperationID, indexReservationByOperationID); err != nil { return err } - // Index reservations by their target host so we can requeue reservations - // for which the list of instances on a hypervisor has changed. - if err := mcl.IndexField(ctx, - &v1alpha1.Reservation{}, - &v1alpha1.ReservationList{}, - idxReservationByTargetHost, - idxReservationByTargetHostFn, - ); err != nil { + if err := mcl.IndexField(ctx, &v1alpha1.Reservation{}, &v1alpha1.ReservationList{}, + idxReservationByVMID, indexReservationByVMID); err != nil { return err } - // Watch hypervisor changes and requeue reservations targeting - // the changed hypervisor. - bldr, err = bldr.WatchesMulticluster(&hv1.Hypervisor{}, - c.handleHypervisors(), - c.predicateHypervisors(), + + bldr := multicluster.BuildController(mcl, mgr) + // Primary resource: the in-flight reservation. + bldr, err = bldr.WatchesMulticluster(&v1alpha1.Reservation{}, + &handler.EnqueueRequestForObject{}, + c.predicateReservations(), ) if err != nil { return err } + + // Secondary resources: one watch per (handler, declared object). We do not + // deduplicate across handlers watching the same type — controller-runtime + // shares the informer/cache per GVK, each handler keeps its own MapFn and + // predicate, and the workqueue coalesces any duplicate enqueues. Each + // handler's MapFn only enqueues reservations it owns, and Reconcile + // re-dispatches by owner kind, so cross-handler events are harmless. + for _, h := range c.handlers { + for _, w := range h.WatchObjects() { + preds := []predicate.Predicate{} + if w.Predicate != nil { + preds = append(preds, w.Predicate) + } + bldr, err = bldr.WatchesMulticluster(w.Object, + handler.EnqueueRequestsFromMapFunc(w.MapFn), preds...) + if err != nil { + return err + } + } + } + return bldr.Named("inflight-reservation-controller"). + WithOptions(controller.Options{MaxConcurrentReconciles: 10}). Complete(c) } diff --git a/internal/scheduling/reservations/inflight/controller_test.go b/internal/scheduling/reservations/inflight/controller_test.go index 220048e87..c692cb4ea 100644 --- a/internal/scheduling/reservations/inflight/controller_test.go +++ b/internal/scheduling/reservations/inflight/controller_test.go @@ -12,34 +12,19 @@ import ( novaapi "github.com/cobaltcore-dev/cortex/api/external/nova" "github.com/cobaltcore-dev/cortex/api/v1alpha1" hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + vmv1alpha1 "github.com/cobaltcore-dev/vm-operator/api/v1alpha1" + "github.com/prometheus/client_golang/prometheus/testutil" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -// stubVMClient is a test double for vmClient. If err is non-nil, GetCurrentVMSize -// returns it; otherwise it returns size. -type stubVMClient struct { - size map[hv1.ResourceName]resource.Quantity - err error -} - -func (s *stubVMClient) StartWithKubernetesSecrets(ctx context.Context, client client.Client) error { - return nil -} - -func (s *stubVMClient) GetCurrentVMSize(ctx context.Context, vmID string) (map[hv1.ResourceName]resource.Quantity, error) { - return s.size, s.err -} - // newTestScheme returns a runtime.Scheme with all required types registered. func newTestScheme(t *testing.T) *runtime.Scheme { t.Helper() @@ -50,6 +35,9 @@ func newTestScheme(t *testing.T) *runtime.Scheme { if err := hv1.AddToScheme(s); err != nil { t.Fatalf("failed to add hypervisor scheme: %v", err) } + if err := vmv1alpha1.AddToScheme(s); err != nil { + t.Fatalf("failed to add vm-operator scheme: %v", err) + } return s } @@ -58,24 +46,56 @@ func newTestClient(scheme *runtime.Scheme, objects ...client.Object) client.Clie return fake.NewClientBuilder(). WithScheme(scheme). WithObjects(objects...). - WithStatusSubresource(&v1alpha1.Reservation{}). - WithIndex(&v1alpha1.Reservation{}, idxReservationByTargetHost, idxReservationByTargetHostFn). + WithStatusSubresource(&v1alpha1.Reservation{}, &vmv1alpha1.VirtualMachine{}). + WithIndex(&v1alpha1.Reservation{}, idxReservationByTargetHost, indexReservationByTargetHost). + WithIndex(&v1alpha1.Reservation{}, idxReservationByOperationID, indexReservationByOperationID). + WithIndex(&v1alpha1.Reservation{}, idxReservationByVMID, indexReservationByVMID). Build() } -// newInFlightReservation builds an in-flight reservation with the given name and VM ID. -func newInFlightReservation(name, vmID, targetHost string) *v1alpha1.Reservation { - return &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: name}, +// newTestController builds a Controller wired with the VirtualMachine owner +// handler, mirroring the production registry so Reconcile can dispatch. +func newTestController(k8sClient client.Client) *Controller { + vmHandler := &VirtualMachineHandler{Client: k8sClient} + return &Controller{ + Client: k8sClient, + DefaultOwnerKind: "VirtualMachine", + handlers: map[string]OwnerHandler{vmHandler.Kind(): vmHandler}, + } +} + +// inFlightOpts configures the in-flight reservation built by newInFlightReservation. +type inFlightOpts struct { + intent v1alpha1.SchedulingIntent + operationID string + resources map[hv1.ResourceName]resource.Quantity + endTime *metav1.Time + createdAt *metav1.Time // reservation creationTimestamp, for condition-freshness checks + labels map[string]string +} + +// newInFlightReservation builds an in-flight reservation with the given name, VM +// ID and target host plus optional extras. +func newInFlightReservation(name, vmID, targetHost string, opts inFlightOpts) *v1alpha1.Reservation { + res := &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: name, Labels: opts.labels}, Spec: v1alpha1.ReservationSpec{ Type: v1alpha1.ReservationTypeInFlight, SchedulingDomain: v1alpha1.SchedulingDomainNova, TargetHost: targetHost, + Resources: opts.resources, + EndTime: opts.endTime, InFlightReservation: &v1alpha1.InFlightReservationSpec{ - VMID: vmID, + VMID: vmID, + Intent: opts.intent, + OperationID: opts.operationID, }, }, } + if opts.createdAt != nil { + res.CreationTimestamp = *opts.createdAt + } + return res } // newHypervisor builds a Hypervisor with the given name and instance IDs. @@ -87,8 +107,13 @@ func newHypervisor(name string, instanceIDs ...string) *hv1.Hypervisor { return hv } -// assertReadyCondition fetches the named reservation and asserts the Ready condition's -// status and reason. Fails fast if the condition is missing. +func exists(t *testing.T, k8sClient client.Client, name string) bool { + t.Helper() + var got v1alpha1.Reservation + err := k8sClient.Get(context.Background(), types.NamespacedName{Name: name}, &got) + return err == nil +} + func assertReadyCondition(t *testing.T, k8sClient client.Client, name string, wantStatus metav1.ConditionStatus, wantReason string) { t.Helper() var got v1alpha1.Reservation @@ -98,7 +123,6 @@ func assertReadyCondition(t *testing.T, k8sClient client.Client, name string, wa cond := meta.FindStatusCondition(got.Status.Conditions, v1alpha1.ReservationConditionReady) if cond == nil { t.Fatalf("Ready condition was not set on %q", name) - return } if cond.Status != wantStatus { t.Errorf("%s: Ready status = %q, want %q", name, cond.Status, wantStatus) @@ -108,633 +132,531 @@ func assertReadyCondition(t *testing.T, k8sClient client.Client, name string, wa } } -func TestReconcile_NotFoundIsIgnored(t *testing.T) { - scheme := newTestScheme(t) - k8sClient := newTestClient(scheme) - c := &Controller{Client: k8sClient} - - res, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "missing"}, - }) - if err != nil { - t.Fatalf("Reconcile returned error for missing object: %v", err) - } - if res.RequeueAfter != 0 { - t.Errorf("expected empty result, got %+v", res) - } -} - -func TestReconcile_UnexpectedTypeSetsConditionFalse(t *testing.T) { - scheme := newTestScheme(t) - wrong := &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: "wrong-type"}, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeFailover, +func TestReconcile(t *testing.T) { + cpu4 := map[hv1.ResourceName]resource.Quantity{"cpu": resource.MustParse("4")} + past := metav1.NewTime(time.Now().Add(-time.Minute)) + future := metav1.NewTime(time.Now().Add(time.Hour)) + // Freshness is judged against the reservation's creationTimestamp. Set it + // explicitly (the fake client does not stamp it) so a condition's + // LastTransitionTime can be placed clearly before (stale) or after (fresh). + resCreated := metav1.NewTime(time.Now()) + freshFuture := metav1.NewTime(resCreated.Add(time.Hour)) + stalePast := metav1.NewTime(resCreated.Add(-time.Hour)) + createA := inFlightOpts{operationID: "op-a", intent: novaapi.CreateIntent} + expiredA := inFlightOpts{operationID: "op-a", endTime: &past, intent: novaapi.CreateIntent} + + type testCase struct { + name string + objects []client.Object + reconcile string + wantErr error + wantRequeue time.Duration + wantDeleted []string + wantKept []string + wantReadyStatus metav1.ConditionStatus + wantReadyReason string + wantAnomaly string + } + tests := []testCase{ + {name: "not found is ignored", reconcile: "missing"}, + { + name: "missing spec sets condition false", + objects: []client.Object{&v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "res-1"}, + Spec: v1alpha1.ReservationSpec{Type: v1alpha1.ReservationTypeInFlight}, + }}, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionFalse, wantReadyReason: "MissingSpec", }, - } - k8sClient := newTestClient(scheme, wrong) - c := &Controller{Client: k8sClient} - - if _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "wrong-type"}, - }); err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - - assertReadyCondition(t, k8sClient, "wrong-type", metav1.ConditionFalse, "UnexpectedType") -} - -func TestReconcile_MissingSpecSetsConditionFalse(t *testing.T) { - scheme := newTestScheme(t) - noSpec := &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: "no-spec"}, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeInFlight, - // InFlightReservation deliberately nil. + { + name: "no owner handler sets condition and metric", + objects: []client.Object{&v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "res-1", OwnerReferences: []metav1.OwnerReference{{Kind: "ReschedulingRequest"}}}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeInFlight, + SchedulingDomain: v1alpha1.SchedulingDomainNova, + InFlightReservation: &v1alpha1.InFlightReservationSpec{}, // no VMID -> no fallback + }, + }}, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "NoOwnerHandler", + wantAnomaly: AnomalyNoOwnerHandler, }, - } - k8sClient := newTestClient(scheme, noSpec) - c := &Controller{Client: k8sClient} - - if _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "no-spec"}, - }); err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - - assertReadyCondition(t, k8sClient, "no-spec", metav1.ConditionFalse, "MissingSpec") -} - -func TestReconcile_InstanceNotSpawnedRequeues(t *testing.T) { - scheme := newTestScheme(t) - res := newInFlightReservation("res-1", "vm-uuid-1", "host-1") - hv := newHypervisor("host-1", "other-vm-uuid") - k8sClient := newTestClient(scheme, res, hv) - c := &Controller{Client: k8sClient} - - result, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }) - if err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - if result.RequeueAfter != 10*time.Second { - t.Errorf("RequeueAfter = %v, want 10s", result.RequeueAfter) - } - - // Reservation still exists. - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err != nil { - t.Fatalf("reservation was unexpectedly deleted: %v", err) - } - assertReadyCondition(t, k8sClient, "res-1", metav1.ConditionUnknown, "InstanceNotFound") -} - -func TestReconcile_InstanceOnDifferentHostAwaitsDeletion(t *testing.T) { - // Instance landed on a *different* host than the target. The reservation - // is now stale but the hypervisor operator (not this controller) removes - // it, so Reconcile should be a no-op that leaves the reservation intact. - scheme := newTestScheme(t) - res := newInFlightReservation("res-1", "vm-uuid-1", "host-1") - hv1Obj := newHypervisor("host-1") - hv2Obj := newHypervisor("host-2", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv1Obj, hv2Obj) - c := &Controller{Client: k8sClient} - - result, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }) - if err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - if result.RequeueAfter != 0 { - t.Errorf("expected empty result, got %+v", result) - } - - // Reservation still exists — it's waiting for the hypervisor operator. - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err != nil { - t.Fatalf("expected reservation to still exist, got error: %v", err) - } -} - -func TestReconcile_InstanceOnTargetHostDeletesReservation(t *testing.T) { - scheme := newTestScheme(t) - res := newInFlightReservation("res-1", "vm-uuid-1", "host-1") - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv) - c := &Controller{Client: k8sClient} - - if _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }); err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err == nil { - t.Fatal("expected reservation to be deleted, but Get succeeded") - } -} - -func TestReconcile_InstanceOnTargetHostBatchDeletesReservationsForSameVM(t *testing.T) { - // Multiple in-flight reservations pointing at the same target host for the - // same VM (e.g. left over from earlier scheduling attempts). Once the - // instance is confirmed on that host, all of them must be cleaned up in a - // single reconcile. - scheme := newTestScheme(t) - res1 := newInFlightReservation("res-1", "vm-uuid-1", "host-1") - res2 := newInFlightReservation("res-2", "vm-uuid-1", "host-1") - // A reservation for a different VM on the same host must be left alone. - other := newInFlightReservation("res-other", "vm-uuid-2", "host-1") - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res1, res2, other, hv) - c := &Controller{Client: k8sClient} - - if _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }); err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err == nil { - t.Fatal("expected res-1 to be deleted, but Get succeeded") - } - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-2"}, &got); err == nil { - t.Fatal("expected res-2 to be deleted, but Get succeeded") - } - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-other"}, &got); err != nil { - t.Fatalf("expected res-other to survive, got error: %v", err) - } -} - -// newResizeReservation builds an in-flight reservation with the given intent -// and resource requirements. Used to exercise the resize/rebuild size-check -// branch of Reconcile. -// -//nolint:unparam -func newResizeReservation(name, vmID, targetHost string, intent v1alpha1.SchedulingIntent, resources map[hv1.ResourceName]resource.Quantity) *v1alpha1.Reservation { - return &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: name}, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeInFlight, - SchedulingDomain: v1alpha1.SchedulingDomainNova, - TargetHost: targetHost, - Resources: resources, - InFlightReservation: &v1alpha1.InFlightReservationSpec{ - VMID: vmID, - Intent: intent, + { + name: "instance not spawned requeues", + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{}), newHypervisor("host-1", "other-vm-uuid")}, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", + }, + { + // HV fallback, no VM CRD, initial placement: landing elsewhere is + // terminal, so the stale candidate is released. + name: "create: instance on different host releases stale candidate", + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), newHypervisor("host-1"), newHypervisor("host-2", "vm-uuid-1")}, + wantDeleted: []string{"res-1"}, + }, + { + // HV fallback, no VM CRD, non-create intent: landing is NOT a + // completion signal, so the hold is kept and only the TTL releases it. + name: "non-create: instance on different host is kept until TTL", + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{}), newHypervisor("host-1"), newHypervisor("host-2", "vm-uuid-1")}, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", + }, + { + // UC1: reconciling the losing candidate releases the whole group. + name: "multi-candidate group released on landing", + objects: []client.Object{ + newInFlightReservation("a", "vm-uuid-1", "host-1", createA), + newInFlightReservation("b", "vm-uuid-1", "host-2", createA), + newInFlightReservation("c", "vm-uuid-1", "host-3", createA), + newHypervisor("host-2", "vm-uuid-1"), }, + reconcile: "a", + wantDeleted: []string{"a", "b", "c"}, + }, + { + // HV fallback, no VM CRD, initial placement: landing on the target host + // is terminal. + name: "create: instance on target host deletes reservation", + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), newHypervisor("host-1", "vm-uuid-1")}, + wantDeleted: []string{"res-1"}, + }, + { + // HV fallback, no VM CRD, non-create intent (resize keeps the VM on its + // host throughout): observing the VM on the target is not completion, so + // the hold is kept until the TTL backstop. + name: "non-create: instance on target host is kept until TTL", + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.ResizeIntent}), newHypervisor("host-1", "vm-uuid-1")}, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", + }, + { + // Completing op-a must leave a concurrent op-b on the same VM untouched. + name: "operation ID scoped delete", + objects: []client.Object{ + newInFlightReservation("a1", "vm-uuid-1", "host-1", inFlightOpts{operationID: "op-a", intent: novaapi.CreateIntent}), + newInFlightReservation("a2", "vm-uuid-1", "host-2", inFlightOpts{operationID: "op-a", intent: novaapi.CreateIntent}), + newInFlightReservation("b1", "vm-uuid-1", "host-1", inFlightOpts{operationID: "op-b", intent: novaapi.CreateIntent}), + newHypervisor("host-1", "vm-uuid-1"), + }, + reconcile: "a1", + wantDeleted: []string{"a1", "a2"}, + wantKept: []string{"b1"}, + }, + { + // No hypervisor hosts the VM -> without TTL this would just requeue. + name: "TTL expiry releases operation group", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", expiredA), + newInFlightReservation("res-2", "vm-uuid-1", "host-2", expiredA), + }, + wantDeleted: []string{"res-1", "res-2"}, + }, + { + name: "TTL not yet exceeded requeues", + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{endTime: &future}), newHypervisor("host-1", "other")}, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + }, + { + // Live migration without a VM CRD: the VM is observed on the source host + // throughout, which is not a completion signal, so the holds are kept + // until the TTL backstop rather than released on landing. + name: "non-create: live migration is kept until TTL", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.LiveMigrationIntent, resources: cpu4}), + newHypervisor("host-1", "vm-uuid-1"), + }, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", + }, + // --- VM-CRD driven cases (primary release path through Reconcile) --- + { + // UC1: VM CRD reports placed and Running; the whole create group is + // released through Reconcile, not via the hypervisor fallback. + name: "create: VM placed and running releases group", + objects: []client.Object{ + newInFlightReservation("a", "vm-uuid-1", "host-1", createA), + newInFlightReservation("b", "vm-uuid-1", "host-2", createA), + newVM("vm-uuid-1", "host-1", cond(vmv1alpha1.ConditionTypeRunning, metav1.ConditionTrue, "Running")), + }, + reconcile: "a", + wantDeleted: []string{"a", "b"}, + }, + { + // UC1: Nova ACTIVE on a placed VM is also a live signal. + name: "create: VM placed and nova ACTIVE deletes reservation", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), + withNovaStatus(newVM("vm-uuid-1", "host-1"), "ACTIVE"), + }, + wantDeleted: []string{"res-1"}, + }, + { + // UC1: placed but not yet live (Nova BUILD) must keep blocking and must + // not fall through to the hypervisor landing heuristic. + name: "create: VM building requeues", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), + withNovaStatus(newVM("vm-uuid-1", "host-1"), "BUILD"), + }, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", + }, + { + // UC1: Nova ERROR releases a failed initial placement through Reconcile. + name: "create: nova ERROR deletes reservation", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), + withNovaStatus(newVM("vm-uuid-1", ""), "ERROR"), + }, + wantDeleted: []string{"res-1"}, + }, + { + // UC2: an ongoing resize keeps the hold and surfaces the Resizing reason. + name: "resize: ongoing requeues", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.ResizeIntent}), + newVM("vm-uuid-1", "host-1", cond(vmv1alpha1.ConditionTypeResizing, metav1.ConditionTrue, vmv1alpha1.ConditionReasonResizingOngoing)), + }, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "Resizing", + }, + { + // UC2: a terminal Resizing reason that transitioned before the hold was + // created is stale (previous operation) and must not release it. + name: "resize: stale terminal condition is kept", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.ResizeIntent, createdAt: &resCreated}), + vmWithCond("vm-uuid-1", vmv1alpha1.ConditionTypeResizing, vmv1alpha1.ConditionReasonResizingCompleted, stalePast), + }, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "Resizing", + }, + { + // VM-CRD precedence: an ongoing resize keeps the hold even though the VM + // is observed on a hypervisor (which alone would look "landed"). + name: "resize: VM-CRD ongoing wins over hypervisor landing", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.ResizeIntent}), + newVM("vm-uuid-1", "host-1", cond(vmv1alpha1.ConditionTypeResizing, metav1.ConditionTrue, vmv1alpha1.ConditionReasonResizingOngoing)), + newHypervisor("host-1", "vm-uuid-1"), + }, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "Resizing", + }, + { + // UC3: an ongoing live migration keeps both holds. + name: "live migration: ongoing requeues", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.LiveMigrationIntent}), + newVM("vm-uuid-1", "host-1", cond(vmv1alpha1.ConditionTypeMigrating, metav1.ConditionTrue, vmv1alpha1.ConditionReasonMigratingOngoing)), + }, + wantRequeue: requeueInterval, + wantKept: []string{"res-1"}, + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "Migrating", + }, + { + // UC3: a fresh terminal migration releases both target and source holds + // (the whole operation group) through Reconcile. + name: "live migration: fresh terminal releases target and source group", + objects: []client.Object{ + newInFlightReservation("target", "vm-uuid-1", "host-2", inFlightOpts{operationID: "op-m", intent: novaapi.LiveMigrationIntent, createdAt: &resCreated}), + newInFlightReservation("source", "vm-uuid-1", "host-1", inFlightOpts{operationID: "op-m", intent: novaapi.LiveMigrationIntent, createdAt: &resCreated}), + vmWithCond("vm-uuid-1", vmv1alpha1.ConditionTypeMigrating, vmv1alpha1.ConditionReasonMigratingCompleted, freshFuture), + }, + reconcile: "source", + wantDeleted: []string{"target", "source"}, }, } -} - -func TestReconcile_ResizeSizeMismatchRequeues(t *testing.T) { - // For a resize/rebuild reservation, the instance being present on the - // target host isn't sufficient — the VM must have grown/shrunk to the - // reserved size. Until it has, the controller must requeue and set the - // Ready condition to VMSizeMismatch. - scheme := newTestScheme(t) - reserved := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("4"), - hv1.ResourceMemory: resource.MustParse("8Gi"), - } - current := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("2"), - hv1.ResourceMemory: resource.MustParse("4Gi"), - } - res := newResizeReservation("res-1", "vm-uuid-1", "host-1", novaapi.ResizeIntent, reserved) - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv) - c := &Controller{Client: k8sClient, VMClient: &stubVMClient{size: current}} - - result, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }) - if err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - if result.RequeueAfter != 10*time.Second { - t.Errorf("RequeueAfter = %v, want 10s", result.RequeueAfter) - } - - // Reservation must still exist and carry the VMSizeMismatch condition. - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err != nil { - t.Fatalf("reservation was unexpectedly deleted: %v", err) + // Every terminal resize reason, when fresh, releases the hold through Reconcile. + for _, r := range []string{ + vmv1alpha1.ConditionReasonResizingCompleted, + vmv1alpha1.ConditionReasonResizingReverted, + vmv1alpha1.ConditionReasonResizingFailed, + } { + tests = append(tests, testCase{ + name: "resize: fresh terminal " + r + " deletes reservation", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.ResizeIntent, createdAt: &resCreated}), + vmWithCond("vm-uuid-1", vmv1alpha1.ConditionTypeResizing, r, freshFuture), + }, + wantDeleted: []string{"res-1"}, + }) } - assertReadyCondition(t, k8sClient, "res-1", metav1.ConditionUnknown, "VMSizeMismatch") -} - -func TestReconcile_ResizeSizeMatchesDeletesReservation(t *testing.T) { - // Once the VM has been resized to the reserved dimensions, the resize - // reservation can be freed like a normal in-flight reservation. - scheme := newTestScheme(t) - reserved := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("4"), - hv1.ResourceMemory: resource.MustParse("8Gi"), - } - res := newResizeReservation("res-1", "vm-uuid-1", "host-1", novaapi.ResizeIntent, reserved) - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv) - c := &Controller{Client: k8sClient, VMClient: &stubVMClient{size: reserved}} - - if _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }); err != nil { - t.Fatalf("Reconcile returned error: %v", err) + // Every terminal live-migration reason, when fresh, releases the hold. + for _, r := range []string{ + vmv1alpha1.ConditionReasonMigratingCompleted, + vmv1alpha1.ConditionReasonMigratingRolledBack, + vmv1alpha1.ConditionReasonMigratingAborting, + vmv1alpha1.ConditionReasonMigratingFailed, + } { + tests = append(tests, testCase{ + name: "live migration: fresh terminal " + r + " deletes reservation", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.LiveMigrationIntent, createdAt: &resCreated}), + vmWithCond("vm-uuid-1", vmv1alpha1.ConditionTypeMigrating, r, freshFuture), + }, + wantDeleted: []string{"res-1"}, + }) } - - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err == nil { - t.Fatal("expected reservation to be deleted, but Get succeeded") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + k8sClient := newTestClient(newTestScheme(t), tt.objects...) + c := newTestController(k8sClient) + c.Monitor = NewMonitor(nil) + name := tt.reconcile + if name == "" { + name = "res-1" + } + got, err := c.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: name}}) + if !errors.Is(err, tt.wantErr) { + t.Fatalf("Reconcile err = %v, want %v", err, tt.wantErr) + } + if got.RequeueAfter != tt.wantRequeue { + t.Errorf("RequeueAfter = %v, want %v", got.RequeueAfter, tt.wantRequeue) + } + for _, n := range tt.wantDeleted { + if exists(t, k8sClient, n) { + t.Errorf("expected %s to be deleted", n) + } + } + for _, n := range tt.wantKept { + if !exists(t, k8sClient, n) { + t.Errorf("expected %s to be kept", n) + } + } + if tt.wantReadyReason != "" { + assertReadyCondition(t, k8sClient, name, tt.wantReadyStatus, tt.wantReadyReason) + } + if tt.wantAnomaly != "" { + if v := testutil.ToFloat64(c.Monitor.anomaliesVec.WithLabelValues(tt.wantAnomaly)); v != 1 { + t.Errorf("%s anomaly count = %v, want 1", tt.wantAnomaly, v) + } + } + }) } } -// TestReconcile_ResizeSizeMatchesSemanticallyDeletesReservation guards the -// semantic quantity comparison in Reconcile: the reservation spec and the vm -// client return numerically-identical quantities that were constructed -// differently (MustParse vs NewQuantity), so reflect.DeepEqual returns false -// even though the resources match. If the controller ever regresses to -// reflect.DeepEqual, this test starts failing. -func TestReconcile_ResizeSizeMatchesSemanticallyDeletesReservation(t *testing.T) { +func TestReconcile_UnexpectedTypeIsIgnored(t *testing.T) { + // A non-in-flight reservation must not be touched: its status belongs to + // another controller. Reconcile logs and returns without writing status. scheme := newTestScheme(t) - // Reservation spec side: built like the API server would (MustParse). - reserved := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("4"), - hv1.ResourceMemory: resource.MustParse("8Gi"), - } - // VM client side: built like the nova vm client does (NewQuantity). - current := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: *resource.NewQuantity(4, resource.DecimalSI), - hv1.ResourceMemory: *resource.NewQuantity(8*1024*1024*1024, resource.BinarySI), - } - res := newResizeReservation("res-1", "vm-uuid-1", "host-1", novaapi.ResizeIntent, reserved) - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv) - c := &Controller{Client: k8sClient, VMClient: &stubVMClient{size: current}} - - if _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }); err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err == nil { - t.Fatal("expected reservation to be deleted (semantic size match), but Get succeeded") + wrong := &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "wrong-type"}, + Spec: v1alpha1.ReservationSpec{Type: v1alpha1.ReservationTypeFailover}, } -} - -// TestReconcile_ResizeStaleReservationSkipsVMClient covers the guard that -// stops the size-check branch from running when the instance landed on a host -// other than the reservation's target. A stale resize reservation should fall -// through to the awaiting-deletion no-op — not fire a vmClient call and get -// marked VMSizeMismatch. VMClient is left nil so an accidental call panics. -func TestReconcile_ResizeStaleReservationSkipsVMClient(t *testing.T) { - scheme := newTestScheme(t) - reserved := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("4"), - } - res := newResizeReservation("res-1", "vm-uuid-1", "host-1", novaapi.ResizeIntent, reserved) - // Instance ended up on host-2, not the target host-1. - hv1Obj := newHypervisor("host-1") - hv2Obj := newHypervisor("host-2", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv1Obj, hv2Obj) - c := &Controller{Client: k8sClient, VMClient: nil} - - result, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }) - if err != nil { + k8sClient := newTestClient(scheme, wrong) + c := newTestController(k8sClient) + if _, err := c.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: "wrong-type"}}); err != nil { t.Fatalf("Reconcile returned error: %v", err) } - if result.RequeueAfter != 0 { - t.Errorf("expected empty result for stale reservation, got %+v", result) - } - // Reservation must still exist and must NOT have been marked with - // VMSizeMismatch — it's simply awaiting the hypervisor operator to - // prune it. var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err != nil { - t.Fatalf("expected reservation to still exist, got error: %v", err) - } - if cond := meta.FindStatusCondition(got.Status.Conditions, v1alpha1.ReservationConditionReady); cond != nil && cond.Reason == "VMSizeMismatch" { - t.Errorf("stale reservation was incorrectly marked VMSizeMismatch") + if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "wrong-type"}, &got); err != nil { + t.Fatalf("failed to get reservation: %v", err) } -} - -func TestReconcile_RebuildSizeMismatchRequeues(t *testing.T) { - // Same branch as resize but exercised via the rebuild intent to make sure - // both intents actually trip the vmClient check. - scheme := newTestScheme(t) - reserved := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("4"), + if len(got.Status.Conditions) != 0 { + t.Errorf("expected no status conditions on foreign reservation, got %+v", got.Status.Conditions) } - current := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("2"), - } - res := newResizeReservation("res-1", "vm-uuid-1", "host-1", novaapi.RebuildIntent, reserved) - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv) - c := &Controller{Client: k8sClient, VMClient: &stubVMClient{size: current}} - - result, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }) - if err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - if result.RequeueAfter != 10*time.Second { - t.Errorf("RequeueAfter = %v, want 10s", result.RequeueAfter) - } - assertReadyCondition(t, k8sClient, "res-1", metav1.ConditionUnknown, "VMSizeMismatch") } -func TestReconcile_ResizeVMClientErrorReturnsError(t *testing.T) { - // If the source of truth for VMs can't be reached we must surface the - // error so controller-runtime backs off — silently deleting the - // reservation here would be a resource-accounting bug. - scheme := newTestScheme(t) - reserved := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: resource.MustParse("4"), - } - res := newResizeReservation("res-1", "vm-uuid-1", "host-1", novaapi.ResizeIntent, reserved) - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv) - vmErr := errors.New("vm client boom") - c := &Controller{Client: k8sClient, VMClient: &stubVMClient{err: vmErr}} - - _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }) - if !errors.Is(err, vmErr) { - t.Fatalf("Reconcile err = %v, want %v", err, vmErr) - } - - // Reservation must survive the error so it can be retried. - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err != nil { - t.Fatalf("reservation was unexpectedly deleted: %v", err) +func TestHandlerFor(t *testing.T) { + vmOwned := metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Kind: "VirtualMachine"}}} + reschedOwned := metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Kind: "Rescheduling"}}} + novaVM := v1alpha1.ReservationSpec{ + SchedulingDomain: v1alpha1.SchedulingDomainNova, + InFlightReservation: &v1alpha1.InFlightReservationSpec{VMID: "vm-1"}, } -} - -func TestReconcile_NonResizeIntentSkipsVMClient(t *testing.T) { - // For non-resize/rebuild intents the vmClient must not be consulted at - // all — we leave it nil to make an accidental call panic loudly. - scheme := newTestScheme(t) - res := newResizeReservation("res-1", "vm-uuid-1", "host-1", novaapi.LiveMigrationIntent, - map[hv1.ResourceName]resource.Quantity{hv1.ResourceCPU: resource.MustParse("4")}) - hv := newHypervisor("host-1", "vm-uuid-1") - k8sClient := newTestClient(scheme, res, hv) - c := &Controller{Client: k8sClient, VMClient: nil} + emptySpec := v1alpha1.ReservationSpec{InFlightReservation: &v1alpha1.InFlightReservationSpec{}} - if _, err := c.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: types.NamespacedName{Name: "res-1"}, - }); err != nil { - t.Fatalf("Reconcile returned error: %v", err) - } - - var got v1alpha1.Reservation - if err := k8sClient.Get(context.Background(), types.NamespacedName{Name: "res-1"}, &got); err == nil { - t.Fatal("expected reservation to be deleted, but Get succeeded") - } -} - -func TestIdxReservationByTargetHostFn(t *testing.T) { tests := []struct { - name string - obj client.Object - want []string + name string + extra []string // additionally registered stub handler kinds + res *v1alpha1.Reservation + wantKind string // "" means no handler + wantAnomaly string }{ + {name: "owner reference kind", res: &v1alpha1.Reservation{ObjectMeta: vmOwned, Spec: emptySpec}, wantKind: "VirtualMachine"}, + {name: "vmID fallback", res: &v1alpha1.Reservation{Spec: novaVM}, wantKind: "VirtualMachine"}, + {name: "fallback requires nova domain", res: &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{ + SchedulingDomain: v1alpha1.SchedulingDomainPods, + InFlightReservation: &v1alpha1.InFlightReservationSpec{VMID: "vm-1"}, + }}}, + {name: "unknown owner, no vmID", res: &v1alpha1.Reservation{ObjectMeta: reschedOwned, Spec: emptySpec}}, + // An unregistered owner carrying a vmID must not fall back to the VM handler. + {name: "unregistered owner with vmID does not fall back to VM", res: &v1alpha1.Reservation{ObjectMeta: reschedOwned, Spec: novaVM}}, + {name: "registered non-VM owner wins over vmID", extra: []string{"Rescheduling"}, res: &v1alpha1.Reservation{ObjectMeta: reschedOwned, Spec: novaVM}, wantKind: "Rescheduling"}, { - name: "wrong type", - obj: &hv1.Hypervisor{}, - want: nil, + // An owner reference is present and other handlers are registered, but + // none matches this owner's kind: no handler, no fallback (owner refs + // exist), no ambiguity. + name: "owner present but no handler matches", + extra: []string{"Rescheduling", "Backup"}, + res: &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Kind: "Snapshot"}}}, + Spec: emptySpec, + }, }, { - name: "empty target host", - obj: &v1alpha1.Reservation{ - Spec: v1alpha1.ReservationSpec{TargetHost: ""}, - }, - want: nil, + // With more than two handlers registered, the one matching the owner's + // kind is selected (not the first registered, not the default). + name: "selects correct handler among several", + extra: []string{"Rescheduling", "Backup", "Snapshot"}, + res: &v1alpha1.Reservation{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Kind: "Backup"}}}, Spec: emptySpec}, + wantKind: "Backup", }, { - name: "non-inflight reservation ignored", - obj: &v1alpha1.Reservation{ - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeCommittedResource, - TargetHost: "host-1", - }, + // Several handlers registered and several owner references, only one of + // which maps to a registered handler: that one wins, unambiguously. + name: "one of several owner refs matches among several handlers", + extra: []string{"Rescheduling", "Backup", "Snapshot"}, + res: &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Kind: "Unknown"}, {Kind: "Snapshot"}}}, + Spec: emptySpec, }, - want: nil, + wantKind: "Snapshot", }, { - name: "target host set", - obj: &v1alpha1.Reservation{ - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeInFlight, - TargetHost: "host-1", - }, + name: "ambiguous owner records metric", + extra: []string{"ReschedulingRequest"}, + res: &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Kind: "VirtualMachine"}, {Kind: "ReschedulingRequest"}}}, + Spec: emptySpec, }, - want: []string{"host-1"}, + wantKind: "*", // any handler, chosen deterministically + wantAnomaly: AnomalyAmbiguousOwner, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := idxReservationByTargetHostFn(tt.obj) - if len(got) != len(tt.want) { - t.Fatalf("got %v, want %v", got, tt.want) + c := newTestController(newTestClient(newTestScheme(t))) + c.Monitor = NewMonitor(nil) + for _, k := range tt.extra { + c.handlers[k] = &stubHandler{kind: k} } - for i := range got { - if got[i] != tt.want[i] { - t.Errorf("got[%d] = %q, want %q", i, got[i], tt.want[i]) + h := c.handlerFor(context.Background(), tt.res) + switch { + case tt.wantKind == "" && h != nil: + t.Errorf("expected no handler, got %s", h.Kind()) + case tt.wantKind != "" && h == nil: + t.Fatalf("expected %s handler, got nil", tt.wantKind) + case tt.wantKind != "" && tt.wantKind != "*" && h.Kind() != tt.wantKind: + t.Errorf("handler kind = %s, want %s", h.Kind(), tt.wantKind) + } + if tt.wantAnomaly != "" { + if v := testutil.ToFloat64(c.Monitor.anomaliesVec.WithLabelValues(tt.wantAnomaly)); v != 1 { + t.Errorf("%s anomaly count = %v, want 1", tt.wantAnomaly, v) } } }) } } +// stubHandler is a minimal OwnerHandler for registry/dispatch tests. +type stubHandler struct{ kind string } + +func (s *stubHandler) Kind() string { return s.kind } +func (s *stubHandler) WatchObjects() []WatchSpec { return nil } +func (s *stubHandler) TerminalState(context.Context, *v1alpha1.Reservation, SubjectReservations) (TerminalResult, error) { + return TerminalResult{Done: true}, nil +} + +func TestIdxReservationByOperationIDFn(t *testing.T) { + tests := []struct { + name string + obj client.Object + want []string + }{ + {"wrong type", &hv1.Hypervisor{}, nil}, + {"non-inflight", &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{Type: v1alpha1.ReservationTypeCommittedResource}}, nil}, + {"no operation id", &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeInFlight, + InFlightReservation: &v1alpha1.InFlightReservationSpec{}, + }}, nil}, + {"operation id set", &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeInFlight, + InFlightReservation: &v1alpha1.InFlightReservationSpec{OperationID: "op-1"}, + }}, []string{"op-1"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := indexReservationByOperationID(tt.obj) + if len(got) != len(tt.want) || (len(got) == 1 && got[0] != tt.want[0]) { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} + func TestPredicateReservations(t *testing.T) { c := &Controller{} pred := c.predicateReservations() - tests := []struct { name string obj client.Object want bool }{ - { - name: "wrong type", - obj: &hv1.Hypervisor{}, - want: false, - }, - { - name: "wrong reservation type", - obj: &v1alpha1.Reservation{ - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeFailover, - SchedulingDomain: v1alpha1.SchedulingDomainNova, - }, - }, - want: false, - }, - { - name: "wrong scheduling domain", - obj: &v1alpha1.Reservation{ - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeInFlight, - SchedulingDomain: v1alpha1.SchedulingDomainPods, - }, - }, - want: false, - }, - { - name: "in-flight nova reservation", - obj: &v1alpha1.Reservation{ - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeInFlight, - SchedulingDomain: v1alpha1.SchedulingDomainNova, - }, - }, - want: true, - }, + {"wrong type", &hv1.Hypervisor{}, false}, + {"wrong reservation type", &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeFailover, SchedulingDomain: v1alpha1.SchedulingDomainNova}}, false}, + {"wrong domain", &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeInFlight, SchedulingDomain: v1alpha1.SchedulingDomainPods}}, false}, + {"in-flight nova", &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeInFlight, SchedulingDomain: v1alpha1.SchedulingDomainNova}}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if got := pred.Create(event.CreateEvent{Object: tt.obj}); got != tt.want { t.Errorf("Create = %v, want %v", got, tt.want) } - if got := pred.Update(event.UpdateEvent{ObjectNew: tt.obj, ObjectOld: tt.obj}); got != tt.want { - t.Errorf("Update = %v, want %v", got, tt.want) - } - if got := pred.Delete(event.DeleteEvent{Object: tt.obj}); got != tt.want { - t.Errorf("Delete = %v, want %v", got, tt.want) - } }) } } -func TestPredicateHypervisors(t *testing.T) { - c := &Controller{} - pred := c.predicateHypervisors() +func TestHypervisorInstancesChangedPredicate(t *testing.T) { + pred := hypervisorInstancesChangedPredicate() - if got := pred.Create(event.CreateEvent{Object: &hv1.Hypervisor{}}); !got { - t.Errorf("Create(Hypervisor) = false, want true") - } - if got := pred.Create(event.CreateEvent{Object: &v1alpha1.Reservation{}}); got { - t.Errorf("Create(Reservation) = true, want false") + if !pred.Create(event.CreateEvent{Object: &hv1.Hypervisor{}}) { + t.Error("Create(Hypervisor) = false, want true") } - - // Update events must only pass when Status.Instances actually changes, - // so unrelated status churn on the Hypervisor doesn't trigger a list + - // enqueue of all reservations targeting the host. same := newHypervisor("host-1", "vm-1") - if got := pred.Update(event.UpdateEvent{ObjectOld: same, ObjectNew: same.DeepCopy()}); got { - t.Errorf("Update(no instance change) = true, want false") + if pred.Update(event.UpdateEvent{ObjectOld: same, ObjectNew: same.DeepCopy()}) { + t.Error("Update(no instance change) = true, want false") } changed := same.DeepCopy() changed.Status.Instances = append(changed.Status.Instances, hv1.Instance{ID: "vm-2"}) - if got := pred.Update(event.UpdateEvent{ObjectOld: same, ObjectNew: changed}); !got { - t.Errorf("Update(instance added) = false, want true") + if !pred.Update(event.UpdateEvent{ObjectOld: same, ObjectNew: changed}) { + t.Error("Update(instance added) = false, want true") } } -// mockWorkQueue captures items added during handler invocations. -type mockWorkQueue struct { - workqueue.TypedRateLimitingInterface[reconcile.Request] - items []reconcile.Request -} - -func (m *mockWorkQueue) Add(item reconcile.Request) { - m.items = append(m.items, item) -} - -func TestHandleReservations(t *testing.T) { - c := &Controller{} - h := c.handleReservations() - res := &v1alpha1.Reservation{ObjectMeta: metav1.ObjectMeta{Name: "res-1"}} - ctx := context.Background() - - t.Run("Create", func(t *testing.T) { - q := &mockWorkQueue{} - h.Create(ctx, event.CreateEvent{Object: res}, q) - if len(q.items) != 1 || q.items[0].Name != "res-1" { - t.Errorf("queue = %+v, want one entry for res-1", q.items) - } - }) - t.Run("Update", func(t *testing.T) { - q := &mockWorkQueue{} - h.Update(ctx, event.UpdateEvent{ObjectOld: res, ObjectNew: res}, q) - if len(q.items) != 1 || q.items[0].Name != "res-1" { - t.Errorf("queue = %+v, want one entry for res-1", q.items) - } - }) - t.Run("Delete", func(t *testing.T) { - q := &mockWorkQueue{} - h.Delete(ctx, event.DeleteEvent{Object: res}, q) - if len(q.items) != 1 || q.items[0].Name != "res-1" { - t.Errorf("queue = %+v, want one entry for res-1", q.items) - } - }) -} - -func TestHandleHypervisors_EnqueuesMatchingReservations(t *testing.T) { +func TestVMHandlerMapHypervisorToReservations(t *testing.T) { scheme := newTestScheme(t) - matching := newInFlightReservation("res-1", "vm-1", "host-1") - other := newInFlightReservation("res-2", "vm-2", "host-2") + matching := newInFlightReservation("res-1", "vm-1", "host-1", inFlightOpts{}) + other := newInFlightReservation("res-2", "vm-2", "host-2", inFlightOpts{}) k8sClient := newTestClient(scheme, matching, other) - c := &Controller{Client: k8sClient} - h := c.handleHypervisors() - - hv := &hv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: "host-1"}} - ctx := context.Background() - - t.Run("Create", func(t *testing.T) { - q := &mockWorkQueue{} - h.Create(ctx, event.CreateEvent{Object: hv}, q) - if len(q.items) != 1 || q.items[0].Name != "res-1" { - t.Errorf("queue = %+v, want only res-1", q.items) - } - }) - t.Run("Update", func(t *testing.T) { - q := &mockWorkQueue{} - h.Update(ctx, event.UpdateEvent{ObjectOld: hv, ObjectNew: hv}, q) - if len(q.items) != 1 || q.items[0].Name != "res-1" { - t.Errorf("queue = %+v, want only res-1", q.items) - } - }) - t.Run("Delete", func(t *testing.T) { - q := &mockWorkQueue{} - h.Delete(ctx, event.DeleteEvent{Object: hv}, q) - if len(q.items) != 1 || q.items[0].Name != "res-1" { - t.Errorf("queue = %+v, want only res-1", q.items) - } - }) -} - -func TestHandleHypervisors_NoMatchingReservations(t *testing.T) { - scheme := newTestScheme(t) - other := newInFlightReservation("res-2", "vm-2", "host-2") - k8sClient := newTestClient(scheme, other) - c := &Controller{Client: k8sClient} - h := c.handleHypervisors() + h := &VirtualMachineHandler{Client: k8sClient} - hv := &hv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: "host-1"}} - q := &mockWorkQueue{} - h.Create(context.Background(), event.CreateEvent{Object: hv}, q) - if len(q.items) != 0 { - t.Errorf("queue = %+v, want empty", q.items) + reqs := h.mapHypervisorToReservations(context.Background(), &hv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: "host-1"}}) + if len(reqs) != 1 || reqs[0].Name != "res-1" { + t.Errorf("requests = %+v, want only res-1", reqs) } } func TestSetupWithManager_RejectsNonMulticlusterClient(t *testing.T) { scheme := newTestScheme(t) - c := &Controller{Client: newTestClient(scheme), VMClient: &stubVMClient{}} - err := c.SetupWithManager(context.Background(), nil) - if err == nil { + c := &Controller{Client: newTestClient(scheme)} + if err := c.SetupWithManager(context.Background(), nil); err == nil { t.Fatal("expected error for non-multicluster client, got nil") } } diff --git a/internal/scheduling/reservations/inflight/metrics.go b/internal/scheduling/reservations/inflight/metrics.go new file mode 100644 index 000000000..bb79014aa --- /dev/null +++ b/internal/scheduling/reservations/inflight/metrics.go @@ -0,0 +1,70 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package inflight + +import ( + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + "github.com/prometheus/client_golang/prometheus" +) + +// Monitor holds Prometheus metrics for the in-flight reservation controller. +type Monitor struct { + // ttlExpiriesVec counts in-flight reservations released because they + // exceeded their TTL without the owning operation signalling completion, + // labeled by scheduling intent. + ttlExpiriesVec *prometheus.CounterVec + // anomaliesVec counts reconcile anomalies worth alerting on, labeled by a + // stable reason (e.g. no_owner_handler, ambiguous_owner). Alert on any + // increase, then inspect the controller logs for details. + anomaliesVec *prometheus.CounterVec +} + +// Anomaly reasons for the anomaly counter. +const ( + AnomalyNoOwnerHandler = "no_owner_handler" + AnomalyAmbiguousOwner = "ambiguous_owner" +) + +// NewMonitor creates a Monitor and registers its metrics with the given +// registerer. A nil registerer skips registration (useful in tests). +func NewMonitor(reg prometheus.Registerer) *Monitor { + m := &Monitor{ + ttlExpiriesVec: prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "cortex_inflight_reservation_ttl_expired_total", + Help: "Total number of in-flight reservations released after exceeding their TTL", + }, + []string{"intent"}, + ), + anomaliesVec: prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "cortex_inflight_reservation_anomaly_total", + Help: "Total number of in-flight reservation reconcile anomalies, by reason", + }, + []string{"reason"}, + ), + } + if reg != nil { + reg.MustRegister(m.ttlExpiriesVec, m.anomaliesVec) + } + return m +} + +// RecordTTLExpiry increments the TTL-expiry counter for the given intent. Safe +// to call on a nil Monitor. +func (m *Monitor) RecordTTLExpiry(intent v1alpha1.SchedulingIntent) { + if m == nil { + return + } + m.ttlExpiriesVec.WithLabelValues(string(intent)).Inc() +} + +// RecordAnomaly increments the anomaly counter for the given reason. Safe to +// call on a nil Monitor. +func (m *Monitor) RecordAnomaly(reason string) { + if m == nil { + return + } + m.anomaliesVec.WithLabelValues(reason).Inc() +} diff --git a/internal/scheduling/reservations/inflight/owner_handler.go b/internal/scheduling/reservations/inflight/owner_handler.go new file mode 100644 index 000000000..8169fc262 --- /dev/null +++ b/internal/scheduling/reservations/inflight/owner_handler.go @@ -0,0 +1,480 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package inflight + +import ( + "context" + "slices" + "time" + + novaapi "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + vmv1alpha1 "github.com/cobaltcore-dev/vm-operator/api/v1alpha1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +// requeueInterval is how long Reconcile waits before re-checking an in-flight +// reservation whose owning operation has not yet reached a terminal state. +const requeueInterval = 10 * time.Second + +// WatchSpec declares a secondary resource that an OwnerHandler needs the +// in-flight controller to watch, together with how an event on that resource +// maps back to the affected in-flight reservations. The controller merges the +// WatchSpecs of all registered handlers (deduplicated by object type) and wires +// them into its own watch set, so handlers stay slim and don't own any watch +// machinery themselves. +type WatchSpec struct { + // Object is a zero value of the watched type, e.g. &hv1.Hypervisor{}. + Object client.Object + // Predicate optionally filters events before MapFn runs. May be nil. + Predicate predicate.Predicate + // MapFn translates an event on Object into the in-flight reservations that + // should be re-reconciled as a result. It is the standard + // controller-runtime EnqueueRequestsFromMapFunc shape: the controller's + // work queue is keyed by Reservation name, but events arrive for the + // secondary Object, so MapFn is the translation layer from "this secondary + // object changed" to "these reservations must be reconciled". + MapFn handler.MapFunc +} + +// TerminalResult is the decision an OwnerHandler returns for one in-flight +// reservation. +type TerminalResult struct { + // Done indicates the owning operation reached a terminal state; the + // reconciler releases the whole OperationID group. + Done bool + // RequeueAfter is how long to wait before re-checking when Done is false. + RequeueAfter time.Duration + // Reason is a short, stable machine reason surfaced on the reservation's + // Ready condition while waiting (Done false). Suitable for condition-based + // alerting; empty falls back to a generic reason. + Reason string + // Message is a human-readable detail surfaced alongside Reason. + Message string +} + +// SubjectReservations returns the in-flight reservations for the given subject +// (VM uuid) that are handled by the same OwnerHandler. The controller binds it +// per reconcile, applying the same ownership selection it uses to dispatch, so a +// handler sees exactly the sibling reservations it owns and never those owned by +// another kind that merely share the subject. +type SubjectReservations func(ctx context.Context, vmID string) ([]v1alpha1.Reservation, error) + +// OwnerHandler encapsulates the owner-kind-specific lifecycle logic for an +// in-flight reservation. It is intentionally slim and stateless: it only +// declares which secondary resources must be watched and decides whether a +// given reservation's owning operation is done. The generic reconciler owns +// deletion, OperationID grouping, TTL handling and watch wiring. +// +// To support a new owner kind (e.g. a future Rescheduling CRD), add an +// implementation and register it; the controller merges its watches and +// dispatches to it by owner-reference Kind automatically. +type OwnerHandler interface { + // Kind is the owner-reference Kind this handler is responsible for, e.g. + // "VirtualMachine". + Kind() string + // WatchObjects declares the secondary resources this handler needs watched + // to observe terminal signals. May be empty. + WatchObjects() []WatchSpec + // TerminalState decides the fate of a single in-flight reservation given + // the current cluster state. On a transient failure it returns an error so + // controller-runtime applies its exponential backoff; otherwise it returns + // a TerminalResult. The siblings lookup yields the reservations for the same + // subject handled by this handler; a handler that does not need it may + // ignore the argument. + TerminalState(ctx context.Context, res *v1alpha1.Reservation, siblings SubjectReservations) (TerminalResult, error) +} + +// VirtualMachineHandler ties an in-flight reservation's lifecycle to a Nova +// VM placement operation. +// +// Terminal detection currently reads the Hypervisor CRD's Status.Instances to +// learn where the VM landed (the behaviour the controller had before owner +// handlers were introduced). The exact terminal signal for initial placement +// (Hypervisor landing vs. VirtualMachine conditions vs. a hybrid) is still +// under discussion; keeping it behind this interface lets us swap the signal +// without touching the generic reconciler. +type VirtualMachineHandler struct { + client.Client +} + +// Kind implements OwnerHandler. +func (h *VirtualMachineHandler) Kind() string { return "VirtualMachine" } + +// WatchObjects implements OwnerHandler. The handler watches the Hypervisor CRD +// (to observe where a VM landed) and the VirtualMachine CRD (so condition/spec +// changes re-trigger reconciliation, per the ADR 8 secondary-watch design). +func (h *VirtualMachineHandler) WatchObjects() []WatchSpec { + return []WatchSpec{ + { + Object: &hv1.Hypervisor{}, + Predicate: hypervisorInstancesChangedPredicate(), + MapFn: h.mapHypervisorToReservations, + }, + { + Object: &vmv1alpha1.VirtualMachine{}, + MapFn: h.mapVMToReservations, + }, + } +} + +// mapHypervisorToReservations enqueues every in-flight reservation targeting the +// changed hypervisor, since its instance list changing may mean a VM we are +// waiting for has now landed. +func (h *VirtualMachineHandler) mapHypervisorToReservations(ctx context.Context, obj client.Object) []reconcile.Request { + hv, ok := obj.(*hv1.Hypervisor) + if !ok { + return nil + } + return h.reservationsForTargetHost(ctx, hv.Name) +} + +// mapVMToReservations enqueues in-flight reservations for the VM that changed. +// The VirtualMachine resource is named after the Nova instance uuid (== VMID), +// so we look reservations up by that subject. +func (h *VirtualMachineHandler) mapVMToReservations(ctx context.Context, obj client.Object) []reconcile.Request { + vm, ok := obj.(*vmv1alpha1.VirtualMachine) + if !ok { + return nil + } + return h.reservationsForVMID(ctx, vm.Name) +} + +// reservationsForTargetHost lists in-flight reservations targeting the given +// host via the targetHost index and returns one request per reservation. +func (h *VirtualMachineHandler) reservationsForTargetHost(ctx context.Context, host string) []reconcile.Request { + log := ctrl.LoggerFrom(ctx) + reservations := &v1alpha1.ReservationList{} + if err := h.List(ctx, reservations, client.MatchingFields{idxReservationByTargetHost: host}); err != nil { + log.Error(err, "Failed to list reservations for hypervisor", "hypervisor", host) + return nil + } + requests := make([]reconcile.Request, 0, len(reservations.Items)) + for _, res := range reservations.Items { + requests = append(requests, reconcile.Request{NamespacedName: client.ObjectKey{Name: res.Name}}) + } + return requests +} + +// reservationsForVMID lists in-flight reservations whose subject is the given +// VM uuid via the vmID index and returns one request per reservation. +func (h *VirtualMachineHandler) reservationsForVMID(ctx context.Context, vmID string) []reconcile.Request { + log := ctrl.LoggerFrom(ctx) + reservations := &v1alpha1.ReservationList{} + if err := h.List(ctx, reservations, client.MatchingFields{idxReservationByVMID: vmID}); err != nil { + log.Error(err, "Failed to list reservations for VM", "vmID", vmID) + return nil + } + requests := make([]reconcile.Request, 0, len(reservations.Items)) + for _, res := range reservations.Items { + requests = append(requests, reconcile.Request{NamespacedName: client.ObjectKey{Name: res.Name}}) + } + return requests +} + +// Waiting reasons surfaced on the Ready condition while an operation is still +// in flight (TerminalResult.Reason). +const ( + reasonAwaitingPlacement = "AwaitingPlacement" + reasonResizing = "Resizing" + reasonMigrating = "Migrating" +) + +// LabelInFlightSource marks the source classification of a resize in-flight +// reservation. Resize operations whose source host is not KVM-managed (e.g. a +// VMware instance) carry no source hold on a KVM hypervisor, so the VM-operator +// never writes a Resizing condition for them. The producer sets this label to +// LabelInFlightSourceUnmanaged on the holds of such a resize so the handler can +// release them on landing instead of waiting for a condition that never comes. +// The label lives only on the short-lived in-flight Reservation and disappears +// when the reservation is released; it never touches the VM or Hypervisor CRD. +const ( + LabelInFlightSource = "reservations.cortex.cloud/inflight-source" + LabelInFlightSourceUnmanaged = "unmanaged" +) + +// Nova instance status values (Status.Nova.Status) relevant to release +// decisions. The vm-operator CRD exposes this as a free-form string mirroring +// Nova, so we compare against literals here. +const ( + novaStatusActive = "ACTIVE" + novaStatusError = "ERROR" +) + +// TerminalState implements OwnerHandler for Nova VM placement operations. The +// decision has two stages: +// +// 1. Supersession (newest-wins): if a newer operation exists for the same VM, +// this reservation belongs to a superseded older operation and is released. +// Operations are ordered by (creationTimestamp, OperationID); the UUIDv7 +// OperationID breaks ties within the timestamp's one-second resolution. +// This handles the case where a new Nova request arrives for a VM while an +// earlier operation's reservations are still in flight. +// +// 2. Per-intent terminal condition on the VirtualMachine CRD: the reservation +// is released once the operation reaches a terminal state for its intent +// (see terminalForIntent). While the VM CRD or its condition is not yet +// available, the handler falls back to the hypervisor landing signal. +// +// Nova serializes its own operations per VM (see the controller docs): when +// Cortex receives a Nova scheduling request for a VM, no other Nova operation on +// that VM is in flight, and the holds it creates are owned by that VM's CRD. +// This per-VM bound only covers these VM-owned holds; reservations owned by +// other resources (e.g. a Rescheduling CRD) are handled elsewhere. The VM CRD's +// condition may still carry a *stale* terminal reason from a previous operation +// that reconciliation has not yet overwritten, so a terminal reason is only +// trusted when the condition transitioned at or after this reservation was +// created (see terminalByReason). This prevents a leftover Completed/Failed +// condition from immediately releasing the freshly created holds of the new +// operation. +func (h *VirtualMachineHandler) TerminalState(ctx context.Context, res *v1alpha1.Reservation, siblings SubjectReservations) (TerminalResult, error) { + log := ctrl.LoggerFrom(ctx) + spec := res.Spec.InFlightReservation + + superseded, err := h.isSuperseded(ctx, res, siblings) + if err != nil { + return TerminalResult{}, err + } + if superseded { + log.V(1).Info("In-flight reservation superseded by a newer operation, releasing", + "vmID", spec.VMID, "operationID", spec.OperationID) + return TerminalResult{Done: true}, nil + } + + // Read the VM CRD (named after the Nova instance uuid) to evaluate the + // per-intent terminal condition. + vm := &vmv1alpha1.VirtualMachine{} + err = h.Get(ctx, client.ObjectKey{Name: spec.VMID}, vm) + switch { + case apierrors.IsNotFound(err): + vm = nil // Fall back to the hypervisor landing signal below. + case err != nil: + return TerminalResult{}, err + } + + if vm != nil { + if res, ok := h.terminalForIntent(res, vm); ok { + return res, nil + } + // No decision from the VM CRD yet (condition absent). Fall through to + // the hypervisor landing fallback. + } + + return h.terminalByHypervisorLanding(ctx, res) +} + +// terminalForIntent evaluates the VM-CRD terminal condition for the +// reservation's intent. It returns (result, ok): ok=false means the VM CRD does +// not yet carry a decisive signal and the caller should fall back. +func (h *VirtualMachineHandler) terminalForIntent(res *v1alpha1.Reservation, vm *vmv1alpha1.VirtualMachine) (TerminalResult, bool) { + switch res.Spec.InFlightReservation.Intent { + case novaapi.CreateIntent: + // UC1 initial placement: release once the VM is confirmed placed AND + // live (host assigned and the domain running / Nova ACTIVE), or on an + // explicit Nova failure (Nova ERROR). While the VM is still building, + // keep blocking all candidates. + // + // Scheduled=False is intentionally NOT used as a release trigger: it is + // not written on failure today, and it reflects the latest scheduling + // result for the VM rather than this operation, so it cannot be + // correlated to an OperationID. + placed := vm.Status.Host != nil && vm.Status.Host.Name != "" + running := meta.IsStatusConditionTrue(vm.Status.Conditions, vmv1alpha1.ConditionTypeRunning) + novaActive := vm.Status.Nova != nil && vm.Status.Nova.Status == novaStatusActive + if placed && (running || novaActive) { + return TerminalResult{Done: true}, true + } + novaError := vm.Status.Nova != nil && vm.Status.Nova.Status == novaStatusError + if novaError { + return TerminalResult{Done: true}, true + } + // We have at least some VM-CRD signal but it is not yet decisive; keep + // waiting rather than falling back to the host-landing heuristic. + if placed || meta.FindStatusCondition(vm.Status.Conditions, vmv1alpha1.ConditionTypeScheduled) != nil || vm.Status.Nova != nil { + return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, + Message: "VM not yet confirmed running on a host"}, true + } + return TerminalResult{}, false + + case novaapi.ResizeIntent: + // UC2 resize: an unmanaged-source resize (e.g. VMware → KVM) has no KVM + // source hold, so the vm-operator never writes a Resizing condition for + // it. Release such a group on landing plus alive, and never on failure + // (the TTL backstops a failed resize). A KVM-source resize keeps the + // condition-based release below. + if res.Labels[LabelInFlightSource] == LabelInFlightSourceUnmanaged { + placed := vm.Status.Host != nil && vm.Status.Host.Name != "" + running := meta.IsStatusConditionTrue(vm.Status.Conditions, vmv1alpha1.ConditionTypeRunning) + novaActive := vm.Status.Nova != nil && vm.Status.Nova.Status == novaStatusActive + if placed && (running || novaActive) { + return TerminalResult{Done: true}, true + } + return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonResizing, + Message: "unmanaged-source resize not yet confirmed running on target"}, true + } + // KVM-source resize: release when the resize is no longer in progress. + return terminalByReason(res.CreationTimestamp, vm.Status.Conditions, vmv1alpha1.ConditionTypeResizing, + reasonResizing, "resize in progress", + vmv1alpha1.ConditionReasonResizingCompleted, + vmv1alpha1.ConditionReasonResizingReverted, + vmv1alpha1.ConditionReasonResizingFailed) + + case novaapi.LiveMigrationIntent: + // UC3 live migration: release both the forward (target) and rollback + // (source) reservations once the migration settles, including an abort. + return terminalByReason(res.CreationTimestamp, vm.Status.Conditions, vmv1alpha1.ConditionTypeMigrating, + reasonMigrating, "live migration in progress", + vmv1alpha1.ConditionReasonMigratingCompleted, + vmv1alpha1.ConditionReasonMigratingRolledBack, + vmv1alpha1.ConditionReasonMigratingAborting, + vmv1alpha1.ConditionReasonMigratingFailed) + + default: + // Other intents (rebuild, evacuate, ...) have no dedicated condition + // yet; use the hypervisor landing fallback. + return TerminalResult{}, false + } +} + +// terminalByReason returns (result, ok) for a condition whose terminal reasons +// are listed. ok=false when the condition is absent (caller should fall back). +// +// A terminal reason is only trusted when it is *fresh*: the condition's +// LastTransitionTime must be at or after createdAt (the reservation's creation +// time). Nova serializes its own operations per VM, so when this VM-owned +// reservation was created no other Nova operation on the VM was in flight; a +// terminal condition that transitioned *before* this reservation existed +// therefore describes a previous operation and must not release the freshly +// created holds. In that stale case we keep waiting (the vm-operator will +// transition the condition for the current operation), with the TTL backstop +// guarding against a signal that never comes. +// +// When Done is false the result carries the waiting reason/message. +func terminalByReason(createdAt metav1.Time, conditions []metav1.Condition, condType, waitReason, waitMessage string, terminalReasons ...string) (TerminalResult, bool) { + cond := meta.FindStatusCondition(conditions, condType) + if cond == nil { + return TerminalResult{}, false + } + if slices.Contains(terminalReasons, cond.Reason) && !conditionIsStale(cond, createdAt) { + return TerminalResult{Done: true}, true + } + return TerminalResult{RequeueAfter: requeueInterval, Reason: waitReason, Message: waitMessage}, true +} + +// conditionIsStale reports whether cond transitioned before the reservation was +// created, i.e. the condition reflects a previous operation rather than the one +// this reservation belongs to. A zero LastTransitionTime is treated as stale: +// without a trustworthy transition time we cannot attribute the terminal signal +// to the current operation, so we conservatively wait for the TTL backstop +// rather than risk releasing holds early. +func conditionIsStale(cond *metav1.Condition, createdAt metav1.Time) bool { + if cond.LastTransitionTime.IsZero() { + return true + } + return cond.LastTransitionTime.Before(&createdAt) +} + +// terminalByHypervisorLanding is the fallback terminal signal used when the VM +// CRD is absent or does not yet carry a per-intent condition. It concludes the +// operation is terminal ONLY for initial placement (CreateIntent): a VM observed +// on any hypervisor means the create landed, so the whole operation group is +// released, cleaning up all (including stale, non-winning) candidates. +// +// For every other intent (resize, live migration, rebuild, evacuate, or an +// empty/unknown intent) hypervisor observation is not a completion signal: the +// VM already occupies a host throughout a resize, and a live migration holds the +// source and target while the VM still sits on the source. Without a decisive +// VM-CRD condition there is no trustworthy terminal signal for these, so the +// handler keeps waiting and relies on the TTL backstop in the reconcile loop. +func (h *VirtualMachineHandler) terminalByHypervisorLanding(ctx context.Context, res *v1alpha1.Reservation) (TerminalResult, error) { + log := ctrl.LoggerFrom(ctx) + spec := res.Spec.InFlightReservation + + _, found, err := h.findVMHost(ctx, spec.VMID) + if err != nil { + return TerminalResult{}, err + } + if !found { + log.V(1).Info("Instance has not spawned on any hypervisor yet, requeuing", "vmID", spec.VMID) + return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, + Message: "instance not yet observed on any hypervisor"}, nil + } + + // Only initial placement is terminal on hypervisor landing. Any other intent + // waits for its VM-CRD condition or the TTL backstop, never for landing. + if spec.Intent != novaapi.CreateIntent { + log.V(1).Info("Instance observed on a hypervisor but intent is not initial placement, waiting for terminal signal or TTL", + "vmID", spec.VMID, "intent", spec.Intent) + return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, + Message: "instance observed on a hypervisor; awaiting terminal condition or TTL"}, nil + } + + return TerminalResult{Done: true}, nil +} + +// isSuperseded reports whether this reservation belongs to an operation older +// than the newest operation for the same VM. Operations are ordered by +// (creationTimestamp, OperationID). Reservations without an OperationID cannot +// be ordered and are never treated as superseded here. +// +// The siblings lookup already restricts the candidates to reservations handled +// by this VirtualMachine handler, so ownership stays isolated: a reservation +// owned by another kind (e.g. Rescheduling) that happens to carry the same vmID +// cannot supersede, or be superseded by, a VM operation. +func (h *VirtualMachineHandler) isSuperseded(ctx context.Context, res *v1alpha1.Reservation, siblings SubjectReservations) (bool, error) { + spec := res.Spec.InFlightReservation + if spec.OperationID == "" { + return false, nil + } + others, err := siblings(ctx, spec.VMID) + if err != nil { + return false, err + } + for i := range others { + other := &others[i] + otherSpec := other.Spec.InFlightReservation + if otherSpec == nil || otherSpec.OperationID == "" || otherSpec.OperationID == spec.OperationID { + continue + } + if operationLess(res, other) { + return true, nil + } + } + return false, nil +} + +// operationLess reports whether reservation a's operation is older than b's, +// ordered by creationTimestamp then OperationID (UUIDv7 lexical tiebreak). +func operationLess(a, b *v1alpha1.Reservation) bool { + at, bt := a.CreationTimestamp.Time, b.CreationTimestamp.Time + if !at.Equal(bt) { + return at.Before(bt) + } + return a.Spec.InFlightReservation.OperationID < b.Spec.InFlightReservation.OperationID +} + +// findVMHost returns the name of the hypervisor the VM currently runs on, if +// any, by scanning the Hypervisor CRDs' instance lists. +func (h *VirtualMachineHandler) findVMHost(ctx context.Context, vmID string) (host string, found bool, err error) { + hvs := &hv1.HypervisorList{} + if err := h.List(ctx, hvs); err != nil { + return "", false, err + } + for _, hv := range hvs.Items { + for _, instance := range hv.Status.Instances { + if instance.ID == vmID { + return hv.Name, true, nil + } + } + } + return "", false, nil +} diff --git a/internal/scheduling/reservations/inflight/owner_handler_test.go b/internal/scheduling/reservations/inflight/owner_handler_test.go new file mode 100644 index 000000000..6d05eed4e --- /dev/null +++ b/internal/scheduling/reservations/inflight/owner_handler_test.go @@ -0,0 +1,293 @@ +// Copyright SAP SE +// SPDX-License-Identifier: Apache-2.0 + +package inflight + +import ( + "context" + "testing" + "time" + + novaapi "github.com/cobaltcore-dev/cortex/api/external/nova" + "github.com/cobaltcore-dev/cortex/api/v1alpha1" + hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" + vmv1alpha1 "github.com/cobaltcore-dev/vm-operator/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// newVM builds a VirtualMachine with the given name and optional current host +// and conditions. +func newVM(name, host string, conds ...metav1.Condition) *vmv1alpha1.VirtualMachine { + vm := &vmv1alpha1.VirtualMachine{ObjectMeta: metav1.ObjectMeta{Name: name}} + if host != "" { + vm.Status.Host = &vmv1alpha1.HostRef{Name: host} + } + for _, c := range conds { + meta.SetStatusCondition(&vm.Status.Conditions, c) + } + return vm +} + +func cond(condType string, status metav1.ConditionStatus, reason string) metav1.Condition { + return metav1.Condition{Type: condType, Status: status, Reason: reason} +} + +// withNovaStatus sets the VM's Nova status (e.g. ACTIVE, BUILD, ERROR). +func withNovaStatus(vm *vmv1alpha1.VirtualMachine, status string) *vmv1alpha1.VirtualMachine { + vm.Status.Nova = &vmv1alpha1.NovaStatus{Status: status} + return vm +} + +// vmWithCond builds a VM carrying a single condition with an explicit +// LastTransitionTime, so freshness (ltt vs the reservation's creationTimestamp) +// can be controlled by the caller. +func vmWithCond(name, condType, reason string, ltt metav1.Time) *vmv1alpha1.VirtualMachine { + vm := newVM(name, "host-1") + vm.Status.Conditions = []metav1.Condition{{Type: condType, Status: metav1.ConditionFalse, Reason: reason, LastTransitionTime: ltt}} + return vm +} + +// newHandler wires a VirtualMachineHandler against a fake client. +func newHandler(t *testing.T, objects ...client.Object) *VirtualMachineHandler { + t.Helper() + k8sClient := newTestClient(newTestScheme(t), objects...) + return &VirtualMachineHandler{Client: k8sClient} +} + +// vmSiblings builds a SubjectReservations lookup that returns the VM-handled +// reservations for a subject: those with no owner reference or a VirtualMachine +// owner reference. This mirrors the ownership filtering the controller applies +// before calling TerminalState. +func vmSiblings(h *VirtualMachineHandler) SubjectReservations { + return func(ctx context.Context, vmID string) ([]v1alpha1.Reservation, error) { + list := &v1alpha1.ReservationList{} + if err := h.List(ctx, list, client.MatchingFields{idxReservationByVMID: vmID}); err != nil { + return nil, err + } + var out []v1alpha1.Reservation + for i := range list.Items { + res := &list.Items[i] + vmOwned := len(res.OwnerReferences) == 0 + for _, ref := range res.OwnerReferences { + if ref.Kind == "VirtualMachine" { + vmOwned = true + } + } + if vmOwned { + out = append(out, *res) + } + } + return out, nil + } +} + +func assertTerminal(t *testing.T, h *VirtualMachineHandler, res *v1alpha1.Reservation, wantDone bool, wantRequeue time.Duration) { + t.Helper() + result, err := h.TerminalState(context.Background(), res, vmSiblings(h)) + if err != nil { + t.Fatalf("TerminalState error: %v", err) + } + if result.Done != wantDone { + t.Errorf("done = %v, want %v", result.Done, wantDone) + } + if result.RequeueAfter != wantRequeue { + t.Errorf("requeueAfter = %v, want %v", result.RequeueAfter, wantRequeue) + } +} + +func TestVMHandler_KindAndWatchObjects(t *testing.T) { + h := &VirtualMachineHandler{} + if h.Kind() != "VirtualMachine" { + t.Error("Kind() must be VirtualMachine") + } + specs := h.WatchObjects() + if len(specs) != 2 { + t.Fatalf("expected 2 watch specs, got %d", len(specs)) + } + if _, ok := specs[0].Object.(*hv1.Hypervisor); !ok { + t.Error("first watch should be Hypervisor") + } + if specs[0].Predicate == nil { + t.Error("Hypervisor watch should carry a predicate") + } + if _, ok := specs[1].Object.(*vmv1alpha1.VirtualMachine); !ok { + t.Error("second watch should be VirtualMachine") + } +} + +func TestVMHandler_TerminalState(t *testing.T) { + created := metav1.NewTime(time.Now().Truncate(time.Second)) + stale := metav1.NewTime(created.Add(-time.Hour)) + // withCond sets a single condition directly so LastTransitionTime is controlled. + withCond := func(condType, reason string, ltt metav1.Time) *vmv1alpha1.VirtualMachine { + return vmWithCond("vm-1", condType, reason, ltt) + } + type tc struct { + name string + intent v1alpha1.SchedulingIntent + created *metav1.Time + vm *vmv1alpha1.VirtualMachine // nil: no VM CRD -> hypervisor fallback + hypervisors []client.Object + labels map[string]string + wantDone bool + } + tests := []tc{ + // UC1 initial placement. + {name: "create: building -> wait", intent: novaapi.CreateIntent, + vm: newVM("vm-1", "host-9", cond(vmv1alpha1.ConditionTypeScheduled, metav1.ConditionUnknown, "InitialPlacement"))}, + {name: "create: Running=True -> done", intent: novaapi.CreateIntent, wantDone: true, + vm: newVM("vm-1", "host-9", cond(vmv1alpha1.ConditionTypeRunning, metav1.ConditionTrue, "Running"))}, + {name: "create: nova ACTIVE -> done", intent: novaapi.CreateIntent, wantDone: true, vm: withNovaStatus(newVM("vm-1", "host-9"), "ACTIVE")}, + {name: "create: no live signal -> wait", intent: novaapi.CreateIntent, vm: withNovaStatus(newVM("vm-1", "host-9"), "BUILD")}, + {name: "create: scheduled false alone -> wait", intent: novaapi.CreateIntent, + vm: newVM("vm-1", "", cond(vmv1alpha1.ConditionTypeScheduled, metav1.ConditionFalse, "NoHostFound"))}, + {name: "create: nova error -> done", intent: novaapi.CreateIntent, wantDone: true, vm: withNovaStatus(newVM("vm-1", ""), "ERROR")}, + // UC2 resize / UC3 live migration ongoing. + {name: "resize: ongoing -> wait", intent: novaapi.ResizeIntent, + vm: newVM("vm-1", "host-1", cond(vmv1alpha1.ConditionTypeResizing, metav1.ConditionTrue, vmv1alpha1.ConditionReasonResizingOngoing))}, + {name: "live migration: ongoing -> wait", intent: novaapi.LiveMigrationIntent, + vm: newVM("vm-1", "host-1", cond(vmv1alpha1.ConditionTypeMigrating, metav1.ConditionTrue, vmv1alpha1.ConditionReasonMigratingOngoing))}, + // No VM CRD -> hypervisor landing fallback. + {name: "fallback: not landed -> wait", intent: novaapi.CreateIntent, hypervisors: []client.Object{newHypervisor("host-1", "other")}}, + {name: "fallback: create landed -> done", intent: novaapi.CreateIntent, wantDone: true, hypervisors: []client.Object{newHypervisor("host-2", "vm-1")}}, + // Non-create intents are not terminal on landing: a resize keeps the VM on + // its host and a live migration holds source+target while the VM sits on + // the source, so the fallback must wait for the TTL backstop instead. + {name: "fallback: resize landed -> wait", intent: novaapi.ResizeIntent, hypervisors: []client.Object{newHypervisor("host-1", "vm-1")}}, + {name: "fallback: live migration landed -> wait", intent: novaapi.LiveMigrationIntent, hypervisors: []client.Object{newHypervisor("host-1", "vm-1")}}, + {name: "fallback: empty intent landed -> wait", hypervisors: []client.Object{newHypervisor("host-1", "vm-1")}}, + } + // Unmanaged-source resize (VMware → KVM): no KVM source hold, so there is no + // Resizing condition. Release on landing plus alive; never on failure. + unmanaged := map[string]string{LabelInFlightSource: LabelInFlightSourceUnmanaged} + tests = append(tests, + tc{name: "unmanaged resize: landed and running -> done", intent: novaapi.ResizeIntent, labels: unmanaged, wantDone: true, + vm: newVM("vm-1", "host-1", cond(vmv1alpha1.ConditionTypeRunning, metav1.ConditionTrue, "Running"))}, + tc{name: "unmanaged resize: landed and nova ACTIVE -> done", intent: novaapi.ResizeIntent, labels: unmanaged, wantDone: true, + vm: withNovaStatus(newVM("vm-1", "host-1"), "ACTIVE")}, + tc{name: "unmanaged resize: placed but not running -> wait", intent: novaapi.ResizeIntent, labels: unmanaged, + vm: withNovaStatus(newVM("vm-1", "host-1"), "BUILD")}, + tc{name: "unmanaged resize: nova ERROR -> wait", intent: novaapi.ResizeIntent, labels: unmanaged, + vm: withNovaStatus(newVM("vm-1", "host-1"), "ERROR")}, + ) + for _, r := range []string{vmv1alpha1.ConditionReasonResizingCompleted, vmv1alpha1.ConditionReasonResizingReverted, vmv1alpha1.ConditionReasonResizingFailed} { + tests = append(tests, tc{name: "resize: terminal " + r, intent: novaapi.ResizeIntent, wantDone: true, + vm: newVM("vm-1", "host-1", cond(vmv1alpha1.ConditionTypeResizing, metav1.ConditionFalse, r))}) + } + for _, r := range []string{vmv1alpha1.ConditionReasonMigratingCompleted, vmv1alpha1.ConditionReasonMigratingRolledBack, + vmv1alpha1.ConditionReasonMigratingAborting, vmv1alpha1.ConditionReasonMigratingFailed} { + tests = append(tests, tc{name: "live migration: terminal " + r, intent: novaapi.LiveMigrationIntent, wantDone: true, + vm: newVM("vm-1", "host-1", cond(vmv1alpha1.ConditionTypeMigrating, metav1.ConditionFalse, r))}) + } + // A terminal condition left over from a previous operation (transition before + // the reservation was created, or unknown) must not release fresh holds. + for _, op := range []struct { + name string + intent v1alpha1.SchedulingIntent + condType, reason string + }{ + {"resize", novaapi.ResizeIntent, vmv1alpha1.ConditionTypeResizing, vmv1alpha1.ConditionReasonResizingCompleted}, + {"live_migration", novaapi.LiveMigrationIntent, vmv1alpha1.ConditionTypeMigrating, vmv1alpha1.ConditionReasonMigratingCompleted}, + } { + tests = append(tests, + tc{name: "stale " + op.name + " -> wait", intent: op.intent, created: &created, vm: withCond(op.condType, op.reason, stale)}, + tc{name: "fresh " + op.name + " -> done", intent: op.intent, created: &created, vm: withCond(op.condType, op.reason, created), wantDone: true}, + tc{name: "missing transition time " + op.name + " -> wait", intent: op.intent, created: &created, vm: withCond(op.condType, op.reason, metav1.Time{})}, + ) + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res := newInFlightReservation("r", "vm-1", "host-1", inFlightOpts{intent: tt.intent, labels: tt.labels}) + if tt.created != nil { + res.CreationTimestamp = *tt.created + } + objs := tt.hypervisors + if tt.vm != nil { + objs = append(objs, tt.vm) + } + wantRequeue := requeueInterval + if tt.wantDone { + wantRequeue = 0 + } + assertTerminal(t, newHandler(t, objs...), res, tt.wantDone, wantRequeue) + }) + } +} + +func TestVMHandler_Supersession(t *testing.T) { + now := time.Now() + older := metav1.NewTime(now.Add(-time.Hour)) + newer := metav1.NewTime(now) + mk := func(name, host, opID string, ts metav1.Time, ownerKind string) *v1alpha1.Reservation { + r := newInFlightReservation(name, "vm-1", host, inFlightOpts{operationID: opID, intent: novaapi.CreateIntent}) + r.CreationTimestamp = ts + if ownerKind != "" { + r.OwnerReferences = []metav1.OwnerReference{{Kind: ownerKind}} + } + return r + } + // No VM CRD and no landing in all cases: without supersession everything waits. + tests := []struct { + name string + objects []*v1alpha1.Reservation + wantDone map[string]bool // checked reservation -> expected Done + }{ + { + name: "older operation superseded by newer", + objects: []*v1alpha1.Reservation{mk("old", "host-1", "op-old", older, ""), mk("new", "host-2", "op-new", newer, "")}, + wantDone: map[string]bool{"old": true, "new": false}, + }, + { + // Ownership is isolated even though the vmID subject is shared. + name: "ignores other owner kinds", + objects: []*v1alpha1.Reservation{mk("vm-op", "host-1", "op-vm", older, ""), mk("resched-op", "host-2", "op-resched", newer, "Rescheduling")}, + wantDone: map[string]bool{"vm-op": false}, + }, + { + // Same creation second: UUIDv7 OperationID breaks the tie lexically. + name: "timestamp tie broken by operation ID", + objects: []*v1alpha1.Reservation{mk("lo", "host-1", "op-aaa", newer, ""), mk("hi", "host-2", "op-bbb", newer, "")}, + wantDone: map[string]bool{"lo": true, "hi": false}, + }, + { + // Without an OperationID reservations cannot be ordered. + name: "no supersession without operation ID", + objects: []*v1alpha1.Reservation{mk("a", "host-1", "", metav1.Time{}, ""), mk("b", "host-2", "", metav1.Time{}, "")}, + wantDone: map[string]bool{"a": false}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + objs := make([]client.Object, len(tt.objects)) + for i, o := range tt.objects { + objs[i] = o + } + h := newHandler(t, objs...) + for name, wantDone := range tt.wantDone { + // Read back so timestamps match the API-truncated values the handler lists. + stored := &v1alpha1.Reservation{} + if err := h.Get(context.Background(), client.ObjectKey{Name: name}, stored); err != nil { + t.Fatal(err) + } + wantRequeue := requeueInterval + if wantDone { + wantRequeue = 0 + } + assertTerminal(t, h, stored, wantDone, wantRequeue) + } + }) + } +} + +func TestVMHandler_MapVMToReservations(t *testing.T) { + matching := newInFlightReservation("r1", "vm-1", "host-1", inFlightOpts{}) + other := newInFlightReservation("r2", "vm-2", "host-2", inFlightOpts{}) + h := newHandler(t, matching, other) + + reqs := h.mapVMToReservations(context.Background(), newVM("vm-2", "")) + if len(reqs) != 1 || reqs[0].Name != "r2" { + t.Errorf("requests = %+v, want only r2", reqs) + } +} diff --git a/internal/scheduling/reservations/inflight/vm_client.go b/internal/scheduling/reservations/inflight/vm_client.go deleted file mode 100644 index 2c3854f0d..000000000 --- a/internal/scheduling/reservations/inflight/vm_client.go +++ /dev/null @@ -1,120 +0,0 @@ -// Copyright SAP SE -// SPDX-License-Identifier: Apache-2.0 - -package inflight - -import ( - "context" - "errors" - "net/http" - - "github.com/cobaltcore-dev/cortex/pkg/keystone" - "github.com/cobaltcore-dev/cortex/pkg/sso" - hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - "github.com/gophercloud/gophercloud/v2" - "github.com/gophercloud/gophercloud/v2/openstack/compute/v2/servers" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -type VMClient interface { - StartWithKubernetesSecrets(ctx context.Context, client client.Client) error - // GetCurrentVMSize returns the size of the VM with the given ID, - // or an error if the VM cannot be found. - GetCurrentVMSize(ctx context.Context, vmID string) (map[hv1.ResourceName]resource.Quantity, error) -} - -type novaVMClient struct { - // config is the configuration for the novaVMClient, including keystone and SSO credentials. - config NovaVMClientConfig - // sc is the service client for the OpenStack Nova API. - sc *gophercloud.ServiceClient -} - -type NovaVMClientConfig struct { - // Secret ref to keystone credentials stored in a k8s secret. - KeystoneSecretRef corev1.SecretReference `json:"keystoneSecretRef"` - // Secret ref to SSO credentials stored in a k8s secret, if applicable. - SSOSecretRef *corev1.SecretReference `json:"ssoSecretRef"` -} - -func NewNovaVMClient(config NovaVMClientConfig) VMClient { - return &novaVMClient{ - config: config, - } -} - -func (c *novaVMClient) StartWithKubernetesSecrets(ctx context.Context, client client.Client) error { - log := ctrl.LoggerFrom(ctx) - log.Info("starting novaVMClient with Kubernetes secrets") - var authenticatedHTTP = http.DefaultClient - if c.config.SSOSecretRef != nil { - var err error - authenticatedHTTP, err = sso.Connector{Client: client}. - FromSecretRef(ctx, *c.config.SSOSecretRef) - if err != nil { - log.Error(err, "failed to create SSO authenticated HTTP client") - return err - } - log.V(1).Info("successfully created SSO authenticated HTTP client") - } - authenticatedKeystone, err := keystone. - Connector{Client: client, HTTPClient: authenticatedHTTP}. - FromSecretRef(ctx, c.config.KeystoneSecretRef) - if err != nil { - log.Error(err, "failed to create authenticated keystone client") - return err - } - log.V(1).Info("successfully created authenticated keystone client") - // Automatically fetch the nova endpoint from the keystone service catalog. - provider := authenticatedKeystone.Client() - serviceType := "compute" - url, err := authenticatedKeystone.FindEndpoint( - authenticatedKeystone.Availability(), serviceType, - ) - if err != nil { - log.Error(err, "failed to find nova endpoint in keystone service catalog") - return err - } - log.Info("successfully found nova endpoint in keystone service catalog", "url", url) - c.sc = &gophercloud.ServiceClient{ - ProviderClient: provider, - Endpoint: url, - Type: serviceType, - // Since microversion 2.53, the hypervisor id and service id is a UUID. - // We need that to find placement resource providers for hypervisors. - Microversion: "2.53", - } - return nil -} - -// GetCurrentVMSize returns the size of the VM with the given ID, or an -// error if the VM cannot be found. -func (c *novaVMClient) GetCurrentVMSize(ctx context.Context, vmID string) (map[hv1.ResourceName]resource.Quantity, error) { - log := ctrl.LoggerFrom(ctx) - if c.sc == nil { - log.Error(nil, "nova service client not initialized yet") - return nil, errors.New("nova service client not initialized yet") - } - var server struct { - Flavor struct { - RAM int64 `json:"ram"` - VCPUs int64 `json:"vcpus"` - } `json:"flavor"` - } - err := servers.Get(ctx, c.sc, vmID).ExtractInto(&server) - if err != nil { - log.Error(err, "failed to get server details from nova", "vmID", vmID) - return nil, err - } - size := map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceCPU: *resource. - NewQuantity(server.Flavor.VCPUs, resource.DecimalSI), - hv1.ResourceMemory: *resource. - NewQuantity(server.Flavor.RAM*1024*1024, resource.BinarySI), - } - log.V(1).Info("successfully retrieved VM size from nova", "vmID", vmID, "size", size) - return size, nil -} diff --git a/internal/scheduling/reservations/inflight/vm_client_test.go b/internal/scheduling/reservations/inflight/vm_client_test.go deleted file mode 100644 index d8488fe05..000000000 --- a/internal/scheduling/reservations/inflight/vm_client_test.go +++ /dev/null @@ -1,167 +0,0 @@ -// Copyright SAP SE -// SPDX-License-Identifier: Apache-2.0 - -package inflight - -import ( - "net/http" - "net/http/httptest" - "strings" - "testing" - - "github.com/cobaltcore-dev/cortex/pkg/keystone" - testlibKeystone "github.com/cobaltcore-dev/cortex/pkg/keystone/testing" - hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" - "github.com/gophercloud/gophercloud/v2" - "k8s.io/apimachinery/pkg/api/resource" -) - -// setupNovaMockServer starts an httptest server backed by handler and returns -// a mock keystone client whose provider client can drive the gophercloud -// service client at that server's URL. -func setupNovaMockServer(handler http.HandlerFunc) (*httptest.Server, keystone.KeystoneClient) { - server := httptest.NewServer(handler) - return server, &testlibKeystone.MockKeystoneClient{Url: server.URL + "/"} -} - -// newTestNovaVMClient wires a novaVMClient against the given test server and -// keystone mock. -func newTestNovaVMClient(server *httptest.Server, k keystone.KeystoneClient) *novaVMClient { - return &novaVMClient{ - sc: &gophercloud.ServiceClient{ - ProviderClient: k.Client(), - Endpoint: server.URL + "/", - Type: "compute", - Microversion: "2.53", - }, - } -} - -func TestNovaVMClient_GetCurrentVMSize(t *testing.T) { - const vmID = "vm-abc" - handler := func(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodGet { - t.Fatalf("expected GET method, got %s", r.Method) - } - if !strings.HasSuffix(r.URL.Path, "/servers/"+vmID) { - t.Fatalf("unexpected path: %s", r.URL.Path) - } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - _, err := w.Write([]byte(`{"server": {"id": "vm-abc", "flavor": {"ram": 2048, "vcpus": 4}}}`)) - if err != nil { - t.Fatalf("failed to write response: %v", err) - } - } - server, k := setupNovaMockServer(handler) - defer server.Close() - c := newTestNovaVMClient(server, k) - - size, err := c.GetCurrentVMSize(t.Context(), vmID) - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - // CPU is DecimalSI-encoded quantity of vcpus. - cpu, ok := size[hv1.ResourceCPU] - if !ok { - t.Fatalf("expected CPU resource in size map: %+v", size) - } - if got := cpu.Value(); got != 4 { - t.Errorf("expected 4 vCPUs, got %d", got) - } - // Memory is reported in bytes: ram (MiB) * 1024 * 1024. - mem, ok := size[hv1.ResourceMemory] - if !ok { - t.Fatalf("expected Memory resource in size map: %+v", size) - } - wantMem := int64(2048) * 1024 * 1024 - if got := mem.Value(); got != wantMem { - t.Errorf("expected %d bytes memory, got %d", wantMem, got) - } - if mem.Format != resource.BinarySI { - t.Errorf("expected BinarySI format for memory, got %v", mem.Format) - } - if cpu.Format != resource.DecimalSI { - t.Errorf("expected DecimalSI format for cpu, got %v", cpu.Format) - } -} - -func TestNovaVMClient_GetCurrentVMSize_ZeroValues(t *testing.T) { - // Server that returns a flavor with zero ram/vcpus — we still expect - // a valid, non-nil size map (values are zero) and no error. - handler := func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - _, err := w.Write([]byte(`{"server": {"id": "vm-zero", "flavor": {"ram": 0, "vcpus": 0}}}`)) - if err != nil { - t.Fatalf("failed to write response: %v", err) - } - } - server, k := setupNovaMockServer(handler) - defer server.Close() - c := newTestNovaVMClient(server, k) - - size, err := c.GetCurrentVMSize(t.Context(), "vm-zero") - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - cpu := size[hv1.ResourceCPU] - if got := cpu.Value(); got != 0 { - t.Errorf("expected 0 vCPUs, got %d", got) - } - mem := size[hv1.ResourceMemory] - if got := mem.Value(); got != 0 { - t.Errorf("expected 0 bytes memory, got %d", got) - } -} - -func TestNovaVMClient_GetCurrentVMSize_NotFound(t *testing.T) { - handler := func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - _, err := w.Write([]byte(`{"itemNotFound": {"message": "Instance not found", "code": 404}}`)) - if err != nil { - t.Fatalf("failed to write response: %v", err) - } - } - server, k := setupNovaMockServer(handler) - defer server.Close() - c := newTestNovaVMClient(server, k) - - _, err := c.GetCurrentVMSize(t.Context(), "missing-vm") - if err == nil { - t.Fatal("expected error for 404, got nil") - } -} - -func TestNovaVMClient_GetCurrentVMSize_ServerError(t *testing.T) { - handler := func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - } - server, k := setupNovaMockServer(handler) - defer server.Close() - c := newTestNovaVMClient(server, k) - - _, err := c.GetCurrentVMSize(t.Context(), "any-vm") - if err == nil { - t.Fatal("expected error for 500, got nil") - } -} - -func TestNovaVMClient_GetCurrentVMSize_MalformedJSON(t *testing.T) { - handler := func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - _, err := w.Write([]byte(`not a json`)) - if err != nil { - t.Fatalf("failed to write response: %v", err) - } - } - server, k := setupNovaMockServer(handler) - defer server.Close() - c := newTestNovaVMClient(server, k) - - _, err := c.GetCurrentVMSize(t.Context(), "any-vm") - if err == nil { - t.Fatal("expected error for malformed JSON, got nil") - } -} From 5648edde48033dfc8f3fc33ab4785f43b7d18c2d Mon Sep 17 00:00:00 2001 From: Malte <140147670+umswmayj@users.noreply.github.com> Date: Thu, 5 Nov 2026 15:24:27 +0100 Subject: [PATCH 3/3] Nova request should not have a ttl for inflights Signed-off-by: Malte <140147670+umswmayj@users.noreply.github.com> --- cmd/manager/main.go | 2 +- ...-concurrency-and-in-flight-reservations.md | 18 ++- .../reservations/inflight/controller.go | 83 +--------- .../reservations/inflight/controller_test.go | 153 +++++------------- .../reservations/inflight/owner_handler.go | 138 +++------------- .../inflight/owner_handler_test.go | 45 ++---- 6 files changed, 103 insertions(+), 336 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index a3b00c65d..0ab43502b 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -495,7 +495,7 @@ func main() { setupLog.Info("enabling controller", "controller", "inflight-reservation-controller") monitor := inflight.NewMonitor(metrics.Registry) - controller := &inflight.Controller{Client: multiclusterClient, Monitor: monitor, DefaultOwnerKind: "VirtualMachine"} + controller := &inflight.Controller{Client: multiclusterClient, Monitor: monitor} if err := controller.SetupWithManager(ctx, mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "inflight-reservation-controller") diff --git a/docs/03-reservations-and-inventory/05-concurrency-and-in-flight-reservations.md b/docs/03-reservations-and-inventory/05-concurrency-and-in-flight-reservations.md index 9f54deeec..4f82ada4c 100644 --- a/docs/03-reservations-and-inventory/05-concurrency-and-in-flight-reservations.md +++ b/docs/03-reservations-and-inventory/05-concurrency-and-in-flight-reservations.md @@ -66,14 +66,16 @@ confirmed anything: ## Learning what the platform actually did In-flight reservations are only half the story: they express Cortex's *guess*, and the guess has to be -reconciled with reality. Two feedback channels close that loop: - -- **Exclusion hints on the request.** When the platform retries a placement, it tells Cortex which hosts - it has already ruled out (an `ignore_hosts`-style list). Cortex feeds those forward so the next scoring - round does not re-offer a host the platform has already rejected. -- **Watching the reservation resources.** Cortex watches its own reservation resources and reconciles - in-flight state against confirmed placements as they arrive — promoting a guess to a confirmed slot when - the platform lands where expected, and dropping the other per-candidate guesses for that workload. +reconciled with reality. The feedback loop is closed through the **VirtualMachine CRD**: every in-flight +reservation is owned by the VM CRD of the workload it reserves for, and Cortex watches that CRD to learn +when the owning operation has reached a terminal state (placed and running, resized, migrated, or failed). +Once the VM CRD confirms the outcome, Cortex releases the whole group of per-candidate guesses for that +operation. A VM CRD is therefore required: without it there is no trustworthy terminal signal, and the +hold is kept until the operation confirms or the TTL backstop expires it. + +Exclusion hints also flow back on the request itself: when the platform retries a placement, it tells +Cortex which hosts it has already ruled out (an `ignore_hosts`-style list). Cortex feeds those forward so +the next scoring round does not re-offer a host the platform has already rejected. Because the platform's retry behaviour is not uniform, the number of candidates Cortex offers is matched to how the caller retries: diff --git a/internal/scheduling/reservations/inflight/controller.go b/internal/scheduling/reservations/inflight/controller.go index 3b4d606f3..ccd2bb141 100644 --- a/internal/scheduling/reservations/inflight/controller.go +++ b/internal/scheduling/reservations/inflight/controller.go @@ -6,40 +6,26 @@ package inflight import ( "context" "errors" - "reflect" "time" "github.com/cobaltcore-dev/cortex/api/v1alpha1" "github.com/cobaltcore-dev/cortex/pkg/multicluster" - hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" ) // Field index keys used by the controller and the owner handlers. const ( - idxReservationByTargetHost = "spec.targetHost" idxReservationByOperationID = "spec.inFlightReservation.operationID" idxReservationByVMID = "spec.inFlightReservation.vmID" ) -// indexReservationByTargetHost indexes in-flight reservations by their target -// host so handlers can requeue reservations affected by a hypervisor change. -func indexReservationByTargetHost(obj client.Object) []string { - res, ok := obj.(*v1alpha1.Reservation) - if !ok || res.Spec.Type != v1alpha1.ReservationTypeInFlight || res.Spec.TargetHost == "" { - return nil - } - return []string{res.Spec.TargetHost} -} - // indexReservationByOperationID indexes in-flight reservations by their // operation id so the whole group can be released together. func indexReservationByOperationID(obj client.Object) []string { @@ -78,12 +64,6 @@ type Controller struct { // Monitor records controller metrics (e.g. TTL expiries). May be nil. Monitor *Monitor - // DefaultOwnerKind is the owner handler Kind applied to in-flight - // reservations that carry no recognised owner reference (fallback during - // the transition before producers set owner references). Only applied to - // Nova reservations that carry a VM subject. Typically "VirtualMachine". - DefaultOwnerKind string - // handlers maps an owner-reference Kind to its OwnerHandler. Built in // SetupWithManager from the registered handlers. handlers map[string]OwnerHandler @@ -196,12 +176,11 @@ func (c *Controller) handlerFor(ctx context.Context, res *v1alpha1.Reservation) // registered handler wins, and ambiguous is true when more than one distinct // handler matched (a misconfigured ownership). // -// The DefaultOwnerKind fallback applies only to a Nova VM-subject reservation -// that carries NO owner references at all (the transition period before -// producers set them). A reservation that does carry an owner reference but no -// registered handler matches is deliberately left without a handler (surfacing -// as NoOwnerHandler), so e.g. a Rescheduling-owned reservation is never handled -// by the VirtualMachine handler just because it also carries a vmID. +// An in-flight reservation must carry an owner reference that maps to a +// registered handler. A reservation with no owner references, or whose owner +// references map to no registered handler, is left without a handler (surfacing +// as NoOwnerHandler): e.g. a Rescheduling-owned reservation is never handled by +// the VirtualMachine handler just because it also carries a vmID. func (c *Controller) resolveHandler(res *v1alpha1.Reservation) (handler OwnerHandler, ambiguous bool) { var chosen OwnerHandler matched := map[string]bool{} @@ -218,15 +197,6 @@ func (c *Controller) resolveHandler(res *v1alpha1.Reservation) (handler OwnerHan if chosen != nil { return chosen, len(matched) > 1 } - // Fallback: a Nova VM-subject reservation with no owner references at all is - // handled by the configured default owner handler. A reservation that has - // owner references but none recognised is intentionally not handled here. - if len(res.OwnerReferences) == 0 && - c.DefaultOwnerKind != "" && - res.Spec.SchedulingDomain == v1alpha1.SchedulingDomainNova && - res.Spec.InFlightReservation != nil && res.Spec.InFlightReservation.VMID != "" { - return c.handlers[c.DefaultOwnerKind], false - } return nil, false } @@ -337,39 +307,6 @@ func (c *Controller) predicateReservations() predicate.Predicate { }) } -// hypervisorInstancesChangedPredicate filters Hypervisor events to only those -// that change Status.Instances, since that is the only field the VM owner -// handler consumes. Without it, unrelated status updates from the hypervisor -// operator would cause a list + enqueue of every reservation targeting the -// host. -func hypervisorInstancesChangedPredicate() predicate.Predicate { - return predicate.Funcs{ - CreateFunc: func(evt event.CreateEvent) bool { - _, ok := evt.Object.(*hv1.Hypervisor) - return ok - }, - DeleteFunc: func(evt event.DeleteEvent) bool { - _, ok := evt.Object.(*hv1.Hypervisor) - return ok - }, - GenericFunc: func(evt event.GenericEvent) bool { - _, ok := evt.Object.(*hv1.Hypervisor) - return ok - }, - UpdateFunc: func(evt event.UpdateEvent) bool { - oldHV, ok := evt.ObjectOld.(*hv1.Hypervisor) - if !ok { - return false - } - newHV, ok := evt.ObjectNew.(*hv1.Hypervisor) - if !ok { - return false - } - return !reflect.DeepEqual(oldHV.Status.Instances, newHV.Status.Instances) - }, - } -} - // SetupWithManager sets up the controller with the Manager and a multicluster // client. The multicluster client is used to watch for changes in the // Reservation CRD across all clusters and trigger reconciliations accordingly. @@ -388,13 +325,9 @@ func (c *Controller) SetupWithManager(ctx context.Context, mgr ctrl.Manager) (er vmHandler := &VirtualMachineHandler{Client: c.Client} c.handlers = map[string]OwnerHandler{vmHandler.Kind(): vmHandler} - // Index reservations by their target host, operation id and VM uuid so - // handlers can requeue by host, release a whole operation group, and find - // all operations for one VM (supersession). - if err := mcl.IndexField(ctx, &v1alpha1.Reservation{}, &v1alpha1.ReservationList{}, - idxReservationByTargetHost, indexReservationByTargetHost); err != nil { - return err - } + // Index reservations by their operation id and VM uuid so handlers can + // release a whole operation group and find all operations for one VM + // (supersession). if err := mcl.IndexField(ctx, &v1alpha1.Reservation{}, &v1alpha1.ReservationList{}, idxReservationByOperationID, indexReservationByOperationID); err != nil { return err diff --git a/internal/scheduling/reservations/inflight/controller_test.go b/internal/scheduling/reservations/inflight/controller_test.go index c692cb4ea..ff56e5b3b 100644 --- a/internal/scheduling/reservations/inflight/controller_test.go +++ b/internal/scheduling/reservations/inflight/controller_test.go @@ -47,7 +47,6 @@ func newTestClient(scheme *runtime.Scheme, objects ...client.Object) client.Clie WithScheme(scheme). WithObjects(objects...). WithStatusSubresource(&v1alpha1.Reservation{}, &vmv1alpha1.VirtualMachine{}). - WithIndex(&v1alpha1.Reservation{}, idxReservationByTargetHost, indexReservationByTargetHost). WithIndex(&v1alpha1.Reservation{}, idxReservationByOperationID, indexReservationByOperationID). WithIndex(&v1alpha1.Reservation{}, idxReservationByVMID, indexReservationByVMID). Build() @@ -58,9 +57,8 @@ func newTestClient(scheme *runtime.Scheme, objects ...client.Object) client.Clie func newTestController(k8sClient client.Client) *Controller { vmHandler := &VirtualMachineHandler{Client: k8sClient} return &Controller{ - Client: k8sClient, - DefaultOwnerKind: "VirtualMachine", - handlers: map[string]OwnerHandler{vmHandler.Kind(): vmHandler}, + Client: k8sClient, + handlers: map[string]OwnerHandler{vmHandler.Kind(): vmHandler}, } } @@ -72,13 +70,20 @@ type inFlightOpts struct { endTime *metav1.Time createdAt *metav1.Time // reservation creationTimestamp, for condition-freshness checks labels map[string]string + ownerRefs []metav1.OwnerReference // defaults to a VirtualMachine owner when nil } // newInFlightReservation builds an in-flight reservation with the given name, VM -// ID and target host plus optional extras. +// ID and target host plus optional extras. Unless overridden via opts.ownerRefs, +// the reservation carries a VirtualMachine owner reference so it dispatches to +// the VM owner handler (in-flight reservations require a VM CRD owner). func newInFlightReservation(name, vmID, targetHost string, opts inFlightOpts) *v1alpha1.Reservation { + ownerRefs := opts.ownerRefs + if ownerRefs == nil { + ownerRefs = []metav1.OwnerReference{{Kind: "VirtualMachine"}} + } res := &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: name, Labels: opts.labels}, + ObjectMeta: metav1.ObjectMeta{Name: name, Labels: opts.labels, OwnerReferences: ownerRefs}, Spec: v1alpha1.ReservationSpec{ Type: v1alpha1.ReservationTypeInFlight, SchedulingDomain: v1alpha1.SchedulingDomainNova, @@ -98,15 +103,6 @@ func newInFlightReservation(name, vmID, targetHost string, opts inFlightOpts) *v return res } -// newHypervisor builds a Hypervisor with the given name and instance IDs. -func newHypervisor(name string, instanceIDs ...string) *hv1.Hypervisor { - hv := &hv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: name}} - for _, id := range instanceIDs { - hv.Status.Instances = append(hv.Status.Instances, hv1.Instance{ID: id}) - } - return hv -} - func exists(t *testing.T, k8sClient client.Client, name string) bool { t.Helper() var got v1alpha1.Reservation @@ -183,57 +179,38 @@ func TestReconcile(t *testing.T) { wantAnomaly: AnomalyNoOwnerHandler, }, { - name: "instance not spawned requeues", - objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{}), newHypervisor("host-1", "other-vm-uuid")}, + // No VM CRD yet: there is no terminal signal, so the hold is kept and + // the controller waits (TTL is the only release for an abandoned op). + name: "no VM CRD requeues", + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent})}, wantRequeue: requeueInterval, wantKept: []string{"res-1"}, wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", }, { - // HV fallback, no VM CRD, initial placement: landing elsewhere is - // terminal, so the stale candidate is released. - name: "create: instance on different host releases stale candidate", - objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), newHypervisor("host-1"), newHypervisor("host-2", "vm-uuid-1")}, + // UC1: VM landed and running on a different host than this candidate + // targeted; the placement is confirmed so the stale candidate is + // released. + name: "create: VM placed and running on different host releases stale candidate", + objects: []client.Object{ + newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), + newVM("vm-uuid-1", "host-2", cond(vmv1alpha1.ConditionTypeRunning, metav1.ConditionTrue, "Running")), + }, wantDeleted: []string{"res-1"}, }, { - // HV fallback, no VM CRD, non-create intent: landing is NOT a - // completion signal, so the hold is kept and only the TTL releases it. - name: "non-create: instance on different host is kept until TTL", - objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{}), newHypervisor("host-1"), newHypervisor("host-2", "vm-uuid-1")}, - wantRequeue: requeueInterval, - wantKept: []string{"res-1"}, - wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", - }, - { - // UC1: reconciling the losing candidate releases the whole group. + // UC1: reconciling the losing candidate releases the whole group once + // the VM is confirmed placed and running. name: "multi-candidate group released on landing", objects: []client.Object{ newInFlightReservation("a", "vm-uuid-1", "host-1", createA), newInFlightReservation("b", "vm-uuid-1", "host-2", createA), newInFlightReservation("c", "vm-uuid-1", "host-3", createA), - newHypervisor("host-2", "vm-uuid-1"), + newVM("vm-uuid-1", "host-2", cond(vmv1alpha1.ConditionTypeRunning, metav1.ConditionTrue, "Running")), }, reconcile: "a", wantDeleted: []string{"a", "b", "c"}, }, - { - // HV fallback, no VM CRD, initial placement: landing on the target host - // is terminal. - name: "create: instance on target host deletes reservation", - objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), newHypervisor("host-1", "vm-uuid-1")}, - wantDeleted: []string{"res-1"}, - }, - { - // HV fallback, no VM CRD, non-create intent (resize keeps the VM on its - // host throughout): observing the VM on the target is not completion, so - // the hold is kept until the TTL backstop. - name: "non-create: instance on target host is kept until TTL", - objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.ResizeIntent}), newHypervisor("host-1", "vm-uuid-1")}, - wantRequeue: requeueInterval, - wantKept: []string{"res-1"}, - wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", - }, { // Completing op-a must leave a concurrent op-b on the same VM untouched. name: "operation ID scoped delete", @@ -241,14 +218,14 @@ func TestReconcile(t *testing.T) { newInFlightReservation("a1", "vm-uuid-1", "host-1", inFlightOpts{operationID: "op-a", intent: novaapi.CreateIntent}), newInFlightReservation("a2", "vm-uuid-1", "host-2", inFlightOpts{operationID: "op-a", intent: novaapi.CreateIntent}), newInFlightReservation("b1", "vm-uuid-1", "host-1", inFlightOpts{operationID: "op-b", intent: novaapi.CreateIntent}), - newHypervisor("host-1", "vm-uuid-1"), + newVM("vm-uuid-1", "host-1", cond(vmv1alpha1.ConditionTypeRunning, metav1.ConditionTrue, "Running")), }, reconcile: "a1", wantDeleted: []string{"a1", "a2"}, wantKept: []string{"b1"}, }, { - // No hypervisor hosts the VM -> without TTL this would just requeue. + // No VM CRD hosts the VM -> without TTL this would just requeue. name: "TTL expiry releases operation group", objects: []client.Object{ newInFlightReservation("res-1", "vm-uuid-1", "host-1", expiredA), @@ -258,27 +235,26 @@ func TestReconcile(t *testing.T) { }, { name: "TTL not yet exceeded requeues", - objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{endTime: &future}), newHypervisor("host-1", "other")}, + objects: []client.Object{newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{endTime: &future})}, wantRequeue: requeueInterval, wantKept: []string{"res-1"}, }, { - // Live migration without a VM CRD: the VM is observed on the source host - // throughout, which is not a completion signal, so the holds are kept - // until the TTL backstop rather than released on landing. - name: "non-create: live migration is kept until TTL", + // Live migration with a VM CRD that reports an ongoing migration: the + // holds are kept until the migration settles or the TTL backstop fires. + name: "non-create: live migration ongoing is kept", objects: []client.Object{ newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.LiveMigrationIntent, resources: cpu4}), - newHypervisor("host-1", "vm-uuid-1"), + newVM("vm-uuid-1", "host-1", cond(vmv1alpha1.ConditionTypeMigrating, metav1.ConditionTrue, vmv1alpha1.ConditionReasonMigratingOngoing)), }, wantRequeue: requeueInterval, wantKept: []string{"res-1"}, - wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "AwaitingPlacement", + wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "Migrating", }, // --- VM-CRD driven cases (primary release path through Reconcile) --- { // UC1: VM CRD reports placed and Running; the whole create group is - // released through Reconcile, not via the hypervisor fallback. + // released through Reconcile. name: "create: VM placed and running releases group", objects: []client.Object{ newInFlightReservation("a", "vm-uuid-1", "host-1", createA), @@ -298,8 +274,7 @@ func TestReconcile(t *testing.T) { wantDeleted: []string{"res-1"}, }, { - // UC1: placed but not yet live (Nova BUILD) must keep blocking and must - // not fall through to the hypervisor landing heuristic. + // UC1: placed but not yet live (Nova BUILD) must keep blocking. name: "create: VM building requeues", objects: []client.Object{ newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.CreateIntent}), @@ -341,19 +316,6 @@ func TestReconcile(t *testing.T) { wantKept: []string{"res-1"}, wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "Resizing", }, - { - // VM-CRD precedence: an ongoing resize keeps the hold even though the VM - // is observed on a hypervisor (which alone would look "landed"). - name: "resize: VM-CRD ongoing wins over hypervisor landing", - objects: []client.Object{ - newInFlightReservation("res-1", "vm-uuid-1", "host-1", inFlightOpts{intent: novaapi.ResizeIntent}), - newVM("vm-uuid-1", "host-1", cond(vmv1alpha1.ConditionTypeResizing, metav1.ConditionTrue, vmv1alpha1.ConditionReasonResizingOngoing)), - newHypervisor("host-1", "vm-uuid-1"), - }, - wantRequeue: requeueInterval, - wantKept: []string{"res-1"}, - wantReadyStatus: metav1.ConditionUnknown, wantReadyReason: "Resizing", - }, { // UC3: an ongoing live migration keeps both holds. name: "live migration: ongoing requeues", @@ -487,19 +449,16 @@ func TestHandlerFor(t *testing.T) { wantAnomaly string }{ {name: "owner reference kind", res: &v1alpha1.Reservation{ObjectMeta: vmOwned, Spec: emptySpec}, wantKind: "VirtualMachine"}, - {name: "vmID fallback", res: &v1alpha1.Reservation{Spec: novaVM}, wantKind: "VirtualMachine"}, - {name: "fallback requires nova domain", res: &v1alpha1.Reservation{Spec: v1alpha1.ReservationSpec{ - SchedulingDomain: v1alpha1.SchedulingDomainPods, - InFlightReservation: &v1alpha1.InFlightReservationSpec{VMID: "vm-1"}, - }}}, + // A VM subject without any owner reference is no longer handled: in-flight + // reservations must carry a VirtualMachine owner reference. + {name: "vmID without owner ref is not handled", res: &v1alpha1.Reservation{Spec: novaVM}}, {name: "unknown owner, no vmID", res: &v1alpha1.Reservation{ObjectMeta: reschedOwned, Spec: emptySpec}}, // An unregistered owner carrying a vmID must not fall back to the VM handler. {name: "unregistered owner with vmID does not fall back to VM", res: &v1alpha1.Reservation{ObjectMeta: reschedOwned, Spec: novaVM}}, {name: "registered non-VM owner wins over vmID", extra: []string{"Rescheduling"}, res: &v1alpha1.Reservation{ObjectMeta: reschedOwned, Spec: novaVM}, wantKind: "Rescheduling"}, { // An owner reference is present and other handlers are registered, but - // none matches this owner's kind: no handler, no fallback (owner refs - // exist), no ambiguity. + // none matches this owner's kind: no handler, no ambiguity. name: "owner present but no handler matches", extra: []string{"Rescheduling", "Backup"}, res: &v1alpha1.Reservation{ @@ -509,7 +468,7 @@ func TestHandlerFor(t *testing.T) { }, { // With more than two handlers registered, the one matching the owner's - // kind is selected (not the first registered, not the default). + // kind is selected (not the first registered). name: "selects correct handler among several", extra: []string{"Rescheduling", "Backup", "Snapshot"}, res: &v1alpha1.Reservation{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Kind: "Backup"}}}, Spec: emptySpec}, @@ -623,36 +582,6 @@ func TestPredicateReservations(t *testing.T) { } } -func TestHypervisorInstancesChangedPredicate(t *testing.T) { - pred := hypervisorInstancesChangedPredicate() - - if !pred.Create(event.CreateEvent{Object: &hv1.Hypervisor{}}) { - t.Error("Create(Hypervisor) = false, want true") - } - same := newHypervisor("host-1", "vm-1") - if pred.Update(event.UpdateEvent{ObjectOld: same, ObjectNew: same.DeepCopy()}) { - t.Error("Update(no instance change) = true, want false") - } - changed := same.DeepCopy() - changed.Status.Instances = append(changed.Status.Instances, hv1.Instance{ID: "vm-2"}) - if !pred.Update(event.UpdateEvent{ObjectOld: same, ObjectNew: changed}) { - t.Error("Update(instance added) = false, want true") - } -} - -func TestVMHandlerMapHypervisorToReservations(t *testing.T) { - scheme := newTestScheme(t) - matching := newInFlightReservation("res-1", "vm-1", "host-1", inFlightOpts{}) - other := newInFlightReservation("res-2", "vm-2", "host-2", inFlightOpts{}) - k8sClient := newTestClient(scheme, matching, other) - h := &VirtualMachineHandler{Client: k8sClient} - - reqs := h.mapHypervisorToReservations(context.Background(), &hv1.Hypervisor{ObjectMeta: metav1.ObjectMeta{Name: "host-1"}}) - if len(reqs) != 1 || reqs[0].Name != "res-1" { - t.Errorf("requests = %+v, want only res-1", reqs) - } -} - func TestSetupWithManager_RejectsNonMulticlusterClient(t *testing.T) { scheme := newTestScheme(t) c := &Controller{Client: newTestClient(scheme)} diff --git a/internal/scheduling/reservations/inflight/owner_handler.go b/internal/scheduling/reservations/inflight/owner_handler.go index 8169fc262..881d6df7d 100644 --- a/internal/scheduling/reservations/inflight/owner_handler.go +++ b/internal/scheduling/reservations/inflight/owner_handler.go @@ -10,7 +10,6 @@ import ( novaapi "github.com/cobaltcore-dev/cortex/api/external/nova" "github.com/cobaltcore-dev/cortex/api/v1alpha1" - hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" vmv1alpha1 "github.com/cobaltcore-dev/vm-operator/api/v1alpha1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -97,12 +96,10 @@ type OwnerHandler interface { // VirtualMachineHandler ties an in-flight reservation's lifecycle to a Nova // VM placement operation. // -// Terminal detection currently reads the Hypervisor CRD's Status.Instances to -// learn where the VM landed (the behaviour the controller had before owner -// handlers were introduced). The exact terminal signal for initial placement -// (Hypervisor landing vs. VirtualMachine conditions vs. a hybrid) is still -// under discussion; keeping it behind this interface lets us swap the signal -// without touching the generic reconciler. +// Terminal detection reads the VirtualMachine CRD: the reservation is required +// to have a VM CRD, whose per-intent conditions (and Nova status) decide when +// the owning operation is complete. Keeping it behind this interface lets us +// evolve the terminal signal without touching the generic reconciler. type VirtualMachineHandler struct { client.Client } @@ -110,16 +107,11 @@ type VirtualMachineHandler struct { // Kind implements OwnerHandler. func (h *VirtualMachineHandler) Kind() string { return "VirtualMachine" } -// WatchObjects implements OwnerHandler. The handler watches the Hypervisor CRD -// (to observe where a VM landed) and the VirtualMachine CRD (so condition/spec -// changes re-trigger reconciliation, per the ADR 8 secondary-watch design). +// WatchObjects implements OwnerHandler. The handler watches the VirtualMachine +// CRD so condition/spec changes re-trigger reconciliation, per the ADR 8 +// secondary-watch design. func (h *VirtualMachineHandler) WatchObjects() []WatchSpec { return []WatchSpec{ - { - Object: &hv1.Hypervisor{}, - Predicate: hypervisorInstancesChangedPredicate(), - MapFn: h.mapHypervisorToReservations, - }, { Object: &vmv1alpha1.VirtualMachine{}, MapFn: h.mapVMToReservations, @@ -127,17 +119,6 @@ func (h *VirtualMachineHandler) WatchObjects() []WatchSpec { } } -// mapHypervisorToReservations enqueues every in-flight reservation targeting the -// changed hypervisor, since its instance list changing may mean a VM we are -// waiting for has now landed. -func (h *VirtualMachineHandler) mapHypervisorToReservations(ctx context.Context, obj client.Object) []reconcile.Request { - hv, ok := obj.(*hv1.Hypervisor) - if !ok { - return nil - } - return h.reservationsForTargetHost(ctx, hv.Name) -} - // mapVMToReservations enqueues in-flight reservations for the VM that changed. // The VirtualMachine resource is named after the Nova instance uuid (== VMID), // so we look reservations up by that subject. @@ -149,22 +130,6 @@ func (h *VirtualMachineHandler) mapVMToReservations(ctx context.Context, obj cli return h.reservationsForVMID(ctx, vm.Name) } -// reservationsForTargetHost lists in-flight reservations targeting the given -// host via the targetHost index and returns one request per reservation. -func (h *VirtualMachineHandler) reservationsForTargetHost(ctx context.Context, host string) []reconcile.Request { - log := ctrl.LoggerFrom(ctx) - reservations := &v1alpha1.ReservationList{} - if err := h.List(ctx, reservations, client.MatchingFields{idxReservationByTargetHost: host}); err != nil { - log.Error(err, "Failed to list reservations for hypervisor", "hypervisor", host) - return nil - } - requests := make([]reconcile.Request, 0, len(reservations.Items)) - for _, res := range reservations.Items { - requests = append(requests, reconcile.Request{NamespacedName: client.ObjectKey{Name: res.Name}}) - } - return requests -} - // reservationsForVMID lists in-flight reservations whose subject is the given // VM uuid via the vmID index and returns one request per reservation. func (h *VirtualMachineHandler) reservationsForVMID(ctx context.Context, vmID string) []reconcile.Request { @@ -222,8 +187,9 @@ const ( // // 2. Per-intent terminal condition on the VirtualMachine CRD: the reservation // is released once the operation reaches a terminal state for its intent -// (see terminalForIntent). While the VM CRD or its condition is not yet -// available, the handler falls back to the hypervisor landing signal. +// (see terminalForIntent). The VM CRD is required; while it is absent or +// does not yet carry a decisive condition, the hold is kept and only the +// TTL backstop in the reconcile loop releases it. // // Nova serializes its own operations per VM (see the controller docs): when // Cortex receives a Nova scheduling request for a VM, no other Nova operation on @@ -251,25 +217,28 @@ func (h *VirtualMachineHandler) TerminalState(ctx context.Context, res *v1alpha1 } // Read the VM CRD (named after the Nova instance uuid) to evaluate the - // per-intent terminal condition. + // per-intent terminal condition. The VM CRD is required: without it there is + // no trustworthy terminal signal, so we keep the hold and let the TTL + // backstop release it if the operation is abandoned. vm := &vmv1alpha1.VirtualMachine{} err = h.Get(ctx, client.ObjectKey{Name: spec.VMID}, vm) switch { case apierrors.IsNotFound(err): - vm = nil // Fall back to the hypervisor landing signal below. + log.V(1).Info("VM CRD not found for in-flight reservation, awaiting it or TTL", "vmID", spec.VMID) + return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, + Message: "VM CRD not yet observed; awaiting terminal condition or TTL"}, nil case err != nil: return TerminalResult{}, err } - if vm != nil { - if res, ok := h.terminalForIntent(res, vm); ok { - return res, nil - } - // No decision from the VM CRD yet (condition absent). Fall through to - // the hypervisor landing fallback. + if result, ok := h.terminalForIntent(res, vm); ok { + return result, nil } - - return h.terminalByHypervisorLanding(ctx, res) + // The VM CRD exists but does not yet carry a decisive condition for this + // intent. Keep waiting; the TTL backstop guards against a signal that never + // comes. + return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, + Message: "VM CRD does not yet carry a terminal condition"}, nil } // terminalForIntent evaluates the VM-CRD terminal condition for the @@ -298,7 +267,7 @@ func (h *VirtualMachineHandler) terminalForIntent(res *v1alpha1.Reservation, vm return TerminalResult{Done: true}, true } // We have at least some VM-CRD signal but it is not yet decisive; keep - // waiting rather than falling back to the host-landing heuristic. + // waiting for a confirmed running/ACTIVE (or ERROR) signal. if placed || meta.FindStatusCondition(vm.Status.Conditions, vmv1alpha1.ConditionTypeScheduled) != nil || vm.Status.Nova != nil { return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, Message: "VM not yet confirmed running on a host"}, true @@ -340,13 +309,13 @@ func (h *VirtualMachineHandler) terminalForIntent(res *v1alpha1.Reservation, vm default: // Other intents (rebuild, evacuate, ...) have no dedicated condition - // yet; use the hypervisor landing fallback. + // yet; keep the hold and let the TTL backstop release it. return TerminalResult{}, false } } // terminalByReason returns (result, ok) for a condition whose terminal reasons -// are listed. ok=false when the condition is absent (caller should fall back). +// are listed. ok=false when the condition is absent (caller keeps waiting). // // A terminal reason is only trusted when it is *fresh*: the condition's // LastTransitionTime must be at or after createdAt (the reservation's creation @@ -383,44 +352,6 @@ func conditionIsStale(cond *metav1.Condition, createdAt metav1.Time) bool { return cond.LastTransitionTime.Before(&createdAt) } -// terminalByHypervisorLanding is the fallback terminal signal used when the VM -// CRD is absent or does not yet carry a per-intent condition. It concludes the -// operation is terminal ONLY for initial placement (CreateIntent): a VM observed -// on any hypervisor means the create landed, so the whole operation group is -// released, cleaning up all (including stale, non-winning) candidates. -// -// For every other intent (resize, live migration, rebuild, evacuate, or an -// empty/unknown intent) hypervisor observation is not a completion signal: the -// VM already occupies a host throughout a resize, and a live migration holds the -// source and target while the VM still sits on the source. Without a decisive -// VM-CRD condition there is no trustworthy terminal signal for these, so the -// handler keeps waiting and relies on the TTL backstop in the reconcile loop. -func (h *VirtualMachineHandler) terminalByHypervisorLanding(ctx context.Context, res *v1alpha1.Reservation) (TerminalResult, error) { - log := ctrl.LoggerFrom(ctx) - spec := res.Spec.InFlightReservation - - _, found, err := h.findVMHost(ctx, spec.VMID) - if err != nil { - return TerminalResult{}, err - } - if !found { - log.V(1).Info("Instance has not spawned on any hypervisor yet, requeuing", "vmID", spec.VMID) - return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, - Message: "instance not yet observed on any hypervisor"}, nil - } - - // Only initial placement is terminal on hypervisor landing. Any other intent - // waits for its VM-CRD condition or the TTL backstop, never for landing. - if spec.Intent != novaapi.CreateIntent { - log.V(1).Info("Instance observed on a hypervisor but intent is not initial placement, waiting for terminal signal or TTL", - "vmID", spec.VMID, "intent", spec.Intent) - return TerminalResult{RequeueAfter: requeueInterval, Reason: reasonAwaitingPlacement, - Message: "instance observed on a hypervisor; awaiting terminal condition or TTL"}, nil - } - - return TerminalResult{Done: true}, nil -} - // isSuperseded reports whether this reservation belongs to an operation older // than the newest operation for the same VM. Operations are ordered by // (creationTimestamp, OperationID). Reservations without an OperationID cannot @@ -461,20 +392,3 @@ func operationLess(a, b *v1alpha1.Reservation) bool { } return a.Spec.InFlightReservation.OperationID < b.Spec.InFlightReservation.OperationID } - -// findVMHost returns the name of the hypervisor the VM currently runs on, if -// any, by scanning the Hypervisor CRDs' instance lists. -func (h *VirtualMachineHandler) findVMHost(ctx context.Context, vmID string) (host string, found bool, err error) { - hvs := &hv1.HypervisorList{} - if err := h.List(ctx, hvs); err != nil { - return "", false, err - } - for _, hv := range hvs.Items { - for _, instance := range hv.Status.Instances { - if instance.ID == vmID { - return hv.Name, true, nil - } - } - } - return "", false, nil -} diff --git a/internal/scheduling/reservations/inflight/owner_handler_test.go b/internal/scheduling/reservations/inflight/owner_handler_test.go index 6d05eed4e..7767e4c4a 100644 --- a/internal/scheduling/reservations/inflight/owner_handler_test.go +++ b/internal/scheduling/reservations/inflight/owner_handler_test.go @@ -10,7 +10,6 @@ import ( novaapi "github.com/cobaltcore-dev/cortex/api/external/nova" "github.com/cobaltcore-dev/cortex/api/v1alpha1" - hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" vmv1alpha1 "github.com/cobaltcore-dev/vm-operator/api/v1alpha1" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -103,17 +102,11 @@ func TestVMHandler_KindAndWatchObjects(t *testing.T) { t.Error("Kind() must be VirtualMachine") } specs := h.WatchObjects() - if len(specs) != 2 { - t.Fatalf("expected 2 watch specs, got %d", len(specs)) + if len(specs) != 1 { + t.Fatalf("expected 1 watch spec, got %d", len(specs)) } - if _, ok := specs[0].Object.(*hv1.Hypervisor); !ok { - t.Error("first watch should be Hypervisor") - } - if specs[0].Predicate == nil { - t.Error("Hypervisor watch should carry a predicate") - } - if _, ok := specs[1].Object.(*vmv1alpha1.VirtualMachine); !ok { - t.Error("second watch should be VirtualMachine") + if _, ok := specs[0].Object.(*vmv1alpha1.VirtualMachine); !ok { + t.Error("watch should be VirtualMachine") } } @@ -125,13 +118,12 @@ func TestVMHandler_TerminalState(t *testing.T) { return vmWithCond("vm-1", condType, reason, ltt) } type tc struct { - name string - intent v1alpha1.SchedulingIntent - created *metav1.Time - vm *vmv1alpha1.VirtualMachine // nil: no VM CRD -> hypervisor fallback - hypervisors []client.Object - labels map[string]string - wantDone bool + name string + intent v1alpha1.SchedulingIntent + created *metav1.Time + vm *vmv1alpha1.VirtualMachine // nil: no VM CRD -> wait (no terminal signal) + labels map[string]string + wantDone bool } tests := []tc{ // UC1 initial placement. @@ -149,15 +141,12 @@ func TestVMHandler_TerminalState(t *testing.T) { vm: newVM("vm-1", "host-1", cond(vmv1alpha1.ConditionTypeResizing, metav1.ConditionTrue, vmv1alpha1.ConditionReasonResizingOngoing))}, {name: "live migration: ongoing -> wait", intent: novaapi.LiveMigrationIntent, vm: newVM("vm-1", "host-1", cond(vmv1alpha1.ConditionTypeMigrating, metav1.ConditionTrue, vmv1alpha1.ConditionReasonMigratingOngoing))}, - // No VM CRD -> hypervisor landing fallback. - {name: "fallback: not landed -> wait", intent: novaapi.CreateIntent, hypervisors: []client.Object{newHypervisor("host-1", "other")}}, - {name: "fallback: create landed -> done", intent: novaapi.CreateIntent, wantDone: true, hypervisors: []client.Object{newHypervisor("host-2", "vm-1")}}, - // Non-create intents are not terminal on landing: a resize keeps the VM on - // its host and a live migration holds source+target while the VM sits on - // the source, so the fallback must wait for the TTL backstop instead. - {name: "fallback: resize landed -> wait", intent: novaapi.ResizeIntent, hypervisors: []client.Object{newHypervisor("host-1", "vm-1")}}, - {name: "fallback: live migration landed -> wait", intent: novaapi.LiveMigrationIntent, hypervisors: []client.Object{newHypervisor("host-1", "vm-1")}}, - {name: "fallback: empty intent landed -> wait", hypervisors: []client.Object{newHypervisor("host-1", "vm-1")}}, + // No VM CRD -> no terminal signal, keep waiting for the VM CRD or TTL + // (regardless of intent). + {name: "no VM CRD: create -> wait", intent: novaapi.CreateIntent}, + {name: "no VM CRD: resize -> wait", intent: novaapi.ResizeIntent}, + {name: "no VM CRD: live migration -> wait", intent: novaapi.LiveMigrationIntent}, + {name: "no VM CRD: empty intent -> wait"}, } // Unmanaged-source resize (VMware → KVM): no KVM source hold, so there is no // Resizing condition. Release on landing plus alive; never on failure. @@ -203,7 +192,7 @@ func TestVMHandler_TerminalState(t *testing.T) { if tt.created != nil { res.CreationTimestamp = *tt.created } - objs := tt.hypervisors + var objs []client.Object if tt.vm != nil { objs = append(objs, tt.vm) }