From 03e163106ef9bf64e34ce3c11fcf5289dbf59679 Mon Sep 17 00:00:00 2001 From: Julius Clausnitzer Date: Wed, 15 Jul 2026 11:30:17 +0200 Subject: [PATCH 01/18] WIP Signed-off-by: juliusclausnitzer --- api/v1alpha1/reservation_types.go | 6 + .../reservations/capacity_accounting.go | 68 ++++ .../reservations/capacity_accounting_test.go | 162 +++++++++ .../commitments/reservation_controller.go | 149 +++++++- .../reservation_controller_test.go | 326 ++++++++++++++++++ 5 files changed, 708 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/reservation_types.go b/api/v1alpha1/reservation_types.go index f52797654..f4b1b1b56 100644 --- a/api/v1alpha1/reservation_types.go +++ b/api/v1alpha1/reservation_types.go @@ -181,6 +181,12 @@ type ReservationSpec struct { const ( // ReservationConditionReady indicates whether the reservation is active and ready. ReservationConditionReady = "Ready" + + // ReservationConditionVMMisplaced indicates that one or more VMs have been detected + // on a host other than TargetHost (e.g. after a live migration), but the new host + // lacks sufficient capacity to accept the full reservation slot. The VM is tracked in + // its new location but TargetHost is not updated until capacity becomes available. + ReservationConditionVMMisplaced = "VMMisplaced" ) // CommittedResourceReservationStatus defines the status fields specific to committed resource reservations. diff --git a/internal/scheduling/reservations/capacity_accounting.go b/internal/scheduling/reservations/capacity_accounting.go index 2ccca9685..ed5208136 100644 --- a/internal/scheduling/reservations/capacity_accounting.go +++ b/internal/scheduling/reservations/capacity_accounting.go @@ -10,6 +10,74 @@ import ( "github.com/cobaltcore-dev/cortex/api/v1alpha1" ) +// HostHasCapacityForReservation reports whether hv has sufficient remaining capacity to +// accommodate the full Spec.Resources slot of res. +// +// It uses the same accounting as the scheduler's filter_has_enough_capacity: +// 1. Start from EffectiveCapacity (or Capacity when EffectiveCapacity is nil). +// 2. Subtract hv.Status.Allocation (VMs already running on this host). +// 3. For each other reservation in allReservations that is assigned to this host +// (via Spec.TargetHost or Status.Host), subtract its UnusedReservationCapacity. +// 4. Check that the remainder is ≥ res.Spec.Resources for every resource. +// +// The target reservation itself (matched by name) is excluded from the blocking +// calculation so we don't double-count it. +// Returns false when the hypervisor has no capacity data. +func HostHasCapacityForReservation(allReservations []v1alpha1.Reservation, hv hv1.Hypervisor, res *v1alpha1.Reservation) bool { + effCap := hv.Status.EffectiveCapacity + if effCap == nil { + effCap = hv.Status.Capacity + } + if effCap == nil { + return false + } + + free := make(map[hv1.ResourceName]resource.Quantity, len(effCap)) + for rn, qty := range effCap { + free[rn] = qty.DeepCopy() + } + + for rn, allocated := range hv.Status.Allocation { + if f, ok := free[rn]; ok { + f.Sub(allocated) + free[rn] = f + } + } + + for i := range allReservations { + other := &allReservations[i] + if other.Name == res.Name { + continue + } + // Only block resources from reservations that target or are confirmed on this host. + targetsThisHost := other.Spec.TargetHost == hv.Name || other.Status.Host == hv.Name + if !targetsThisHost { + continue + } + for rn, block := range UnusedReservationCapacity(other, false) { + if f, ok := free[rn]; ok { + f.Sub(block) + free[rn] = f + } + } + } + + zero := resource.Quantity{} + for rn, required := range res.Spec.Resources { + remaining, ok := free[rn] + if !ok { + return false + } + if remaining.Cmp(zero) < 0 { + return false + } + if remaining.Cmp(required) < 0 { + return false + } + } + return true +} + // UnusedReservationCapacity returns the resources a Reservation should block on its host(s). // This is the single source of truth used by both the capacity controller and // filter_has_enough_capacity to ensure consistent accounting. diff --git a/internal/scheduling/reservations/capacity_accounting_test.go b/internal/scheduling/reservations/capacity_accounting_test.go index 815a0a07f..db5caa63d 100644 --- a/internal/scheduling/reservations/capacity_accounting_test.go +++ b/internal/scheduling/reservations/capacity_accounting_test.go @@ -8,6 +8,7 @@ import ( hv1 "github.com/cobaltcore-dev/openstack-hypervisor-operator/api/v1" "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/cobaltcore-dev/cortex/api/v1alpha1" ) @@ -170,3 +171,164 @@ func TestUnusedReservationCapacity(t *testing.T) { }) } } + +func TestHostHasCapacityForReservation(t *testing.T) { + gib := func(n int64) resource.Quantity { return *resource.NewQuantity(n*1024*1024*1024, resource.BinarySI) } + cpu := func(n int64) resource.Quantity { return *resource.NewQuantity(n, resource.DecimalSI) } + + hvWithCapacity := func(name string, memGiB, cpuCores int64) hv1.Hypervisor { + return hv1.Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: hv1.HypervisorStatus{ + EffectiveCapacity: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: gib(memGiB), + hv1.ResourceCPU: cpu(cpuCores), + }, + }, + } + } + + resWithSlot := func(name, targetHost string, memGiB, cpuCores int64) v1alpha1.Reservation { + return v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: targetHost, + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: gib(memGiB), + hv1.ResourceCPU: cpu(cpuCores), + }, + }, + Status: v1alpha1.ReservationStatus{Host: targetHost}, + } + } + + tests := []struct { + name string + hv hv1.Hypervisor + res *v1alpha1.Reservation + others []v1alpha1.Reservation + wantFits bool + }{ + { + name: "empty host: slot fits easily", + hv: hvWithCapacity("host-new", 960, 80), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-1", "host-old", 480, 40); return &r }(), + wantFits: true, + }, + { + name: "host fully consumed by another reservation: no capacity", + hv: hvWithCapacity("host-new", 480, 40), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + others: []v1alpha1.Reservation{ + resWithSlot("res-blocker", "host-new", 480, 40), + }, + wantFits: false, + }, + { + name: "host partially consumed, enough room left", + hv: hvWithCapacity("host-new", 960, 80), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + others: []v1alpha1.Reservation{ + resWithSlot("res-blocker", "host-new", 480, 40), + }, + wantFits: true, + }, + { + name: "host partially consumed, exactly at boundary: fits", + hv: hvWithCapacity("host-new", 960, 80), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + others: []v1alpha1.Reservation{ + resWithSlot("res-blocker-a", "host-new", 240, 20), + resWithSlot("res-blocker-b", "host-new", 240, 20), + }, + wantFits: true, + }, + { + name: "host partially consumed, one resource short (CPU)", + hv: hvWithCapacity("host-new", 960, 60), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + others: []v1alpha1.Reservation{ + resWithSlot("res-blocker", "host-new", 480, 40), + }, + // 960-480=480 memory OK, but 60-40=20 CPU < 40 required + wantFits: false, + }, + { + name: "target reservation itself excluded from blocking calculation", + hv: hvWithCapacity("host-new", 480, 40), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-new", 480, 40); return &r }(), + others: []v1alpha1.Reservation{ + // Same name as res — should be ignored + resWithSlot("res-target", "host-new", 480, 40), + }, + wantFits: true, + }, + { + name: "reservations on other hosts do not count", + hv: hvWithCapacity("host-new", 480, 40), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + others: []v1alpha1.Reservation{ + resWithSlot("res-on-other-host", "host-unrelated", 480, 40), + }, + wantFits: true, + }, + { + name: "hv with no capacity data: always false", + hv: hv1.Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: "host-nocap"}, + }, + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + wantFits: false, + }, + { + name: "hv allocation already consumed memory: no room", + hv: hv1.Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: "host-new"}, + Status: hv1.HypervisorStatus{ + EffectiveCapacity: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: gib(480), + hv1.ResourceCPU: cpu(40), + }, + Allocation: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: gib(100), + }, + }, + }, + res: func() *v1alpha1.Reservation { + r := resWithSlot("res-target", "host-old", 480, 40) + return &r + }(), + wantFits: false, // 480-100 = 380 GiB < 480 GiB required + }, + { + name: "reservation targeting via Status.Host (not TargetHost) still blocks", + hv: hvWithCapacity("host-new", 480, 40), + res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + others: []v1alpha1.Reservation{ + // TargetHost empty but Status.Host = host-new + { + ObjectMeta: metav1.ObjectMeta{Name: "res-status-host"}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: gib(480), + hv1.ResourceCPU: cpu(40), + }, + }, + Status: v1alpha1.ReservationStatus{Host: "host-new"}, + }, + }, + wantFits: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := HostHasCapacityForReservation(tt.others, tt.hv, tt.res) + if got != tt.wantFits { + t.Errorf("HostHasCapacityForReservation() = %v, want %v", got, tt.wantFits) + } + }) + } +} diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index 598f3a667..b59b9027a 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -386,6 +386,16 @@ type reconcileAllocationsResult struct { // (still spawning), so we skip verification and requeue with a short interval. // For older allocations: we check the HV CRD; VMs not found are considered leaving and // removed from the reservation. +// +// Live migration: when a confirmed VM is absent from the expected host, all HV CRDs are +// scanned to detect whether it moved to a different host. +// - New host has capacity: Spec.TargetHost is updated; the existing Branch B in Reconcile +// will then advance Status.Host on the next cycle. +// - New host has no capacity: TargetHost is left unchanged; the VM's actual location is +// recorded in Status; the VMMisplaced condition is set. +// +// Only the migrated VM's host is considered — other allocated VMs in the reservation are +// not moved, consistent with the single-VM scope defined in issue #373. func (r *CommitmentReservationController) reconcileAllocations(ctx context.Context, res *v1alpha1.Reservation) (*reconcileAllocationsResult, error) { logger := LoggerFromContext(ctx) result := &reconcileAllocationsResult{} @@ -436,11 +446,39 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte existingStatusAllocations[k] = v } + // hvList is fetched lazily and only once — only needed when a confirmed VM is missing + // from its expected host and we need to search for it across all hypervisors. + var allHVs *hv1.HypervisorList + // allReservations is fetched lazily — needed alongside allHVs to check capacity. + var allReservations *v1alpha1.ReservationList + + // ensureHVsAndReservations fetches allHVs and allReservations on first call. + ensureHVsAndReservations := func() error { + if allHVs != nil { + return nil + } + allHVs = &hv1.HypervisorList{} + if err := r.List(ctx, allHVs); err != nil { + return fmt.Errorf("failed to list hypervisors: %w", err) + } + allReservations = &v1alpha1.ReservationList{} + if err := r.List(ctx, allReservations); err != nil { + return fmt.Errorf("failed to list reservations: %w", err) + } + return nil + } + // Build new Status.Allocations map based on HV CRD state. newStatusAllocations := make(map[string]string) // Track allocations to remove from Spec (stale/leaving VMs). var allocationsToRemove []string + // migrationTargetHost is set when exactly one confirmed VM is detected on a new host + // that has capacity — in that case we update Spec.TargetHost to the new host. + migrationTargetHost := "" + // misplacedVMs accumulates VM UUIDs that moved to a host without capacity. + var misplacedVMs []string + for vmUUID, allocation := range res.Spec.CommittedResourceReservation.Allocations { allocationAge := now.Sub(allocation.CreationTimestamp.Time) isInGracePeriod := allocationAge < r.Conf.AllocationGracePeriod.Duration @@ -464,7 +502,12 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte logger.V(1).Info("verified VM allocation via Hypervisor CRD", "vm", vmUUID, "host", expectedHost) - } else { + continue + } + + // VM not on the expected host. For unconfirmed post-grace VMs this is a clean + // stale allocation — remove it without further searching. + if !isConfirmed { allocationsToRemove = append(allocationsToRemove, vmUUID) logger.Info("removing stale allocation (VM not found on hypervisor)", "vm", vmUUID, @@ -472,6 +515,69 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte "expectedHost", expectedHost, "allocationAge", allocationAge, "gracePeriod", r.Conf.AllocationGracePeriod.Duration) + continue + } + + // Confirmed VM missing from expected host — could be a live migration. + // Scan all hypervisors to find where the VM actually landed. + if err := ensureHVsAndReservations(); err != nil { + return nil, err + } + + var foundHost string + for i := range allHVs.Items { + if allHVs.Items[i].Name == expectedHost { + continue // already checked + } + for _, inst := range allHVs.Items[i].Status.Instances { + if inst.ID == vmUUID { + foundHost = allHVs.Items[i].Name + break + } + } + if foundHost != "" { + break + } + } + + if foundHost == "" { + // VM is not on any known hypervisor — it has been terminated or evacuated. + allocationsToRemove = append(allocationsToRemove, vmUUID) + logger.Info("removing confirmed allocation (VM not found on any hypervisor)", + "vm", vmUUID, + "reservation", res.Name, + "expectedHost", expectedHost) + continue + } + + // VM found on a different host — live migration detected. + // Check whether the new host can absorb the full reservation slot. + var foundHV hv1.Hypervisor + for i := range allHVs.Items { + if allHVs.Items[i].Name == foundHost { + foundHV = allHVs.Items[i] + break + } + } + + if reservations.HostHasCapacityForReservation(allReservations.Items, foundHV, res) { + // New host has enough room — follow the VM. + logger.Info("VM live-migrated to host with capacity, updating TargetHost", + "vm", vmUUID, + "reservation", res.Name, + "oldHost", expectedHost, + "newHost", foundHost) + migrationTargetHost = foundHost + newStatusAllocations[vmUUID] = foundHost + } else { + // New host is over capacity — record misplacement; keep old TargetHost. + logger.Info("VM live-migrated to host without sufficient capacity, marking misplaced", + "vm", vmUUID, + "reservation", res.Name, + "expectedHost", expectedHost, + "actualHost", foundHost) + misplacedVMs = append(misplacedVMs, vmUUID) + newStatusAllocations[vmUUID] = foundHost } } @@ -487,10 +593,34 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte specChanged = true } + // Update TargetHost when the migrated VM moved to a host with capacity. + // This will be picked up by the TargetHost→Status.Host sync in the next Reconcile + // cycle (Branch B), which advances Status.Host and marks the reservation active on + // the new host. We do NOT update Status.Host here to avoid bypassing that sync path. + if migrationTargetHost != "" { + res.Spec.TargetHost = migrationTargetHost + specChanged = true + } + // Update Status.Allocations res.Status.CommittedResourceReservation.Allocations = newStatusAllocations - // Patch Spec if changed (stale allocations removed) + // Set or clear the VMMisplaced condition. + if len(misplacedVMs) > 0 { + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ReservationConditionVMMisplaced, + Status: metav1.ConditionTrue, + Reason: "MigratedToFullHost", + Message: fmt.Sprintf( + "VM(s) live-migrated to a host that lacks capacity for the reservation slot: %v", + misplacedVMs, + ), + }) + } else { + meta.RemoveStatusCondition(&res.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) + } + + // Patch Spec if changed (stale allocations removed and/or TargetHost updated) if specChanged { if err := r.Patch(ctx, res, client.MergeFrom(old)); err != nil { if client.IgnoreNotFound(err) == nil { @@ -509,8 +639,21 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte // the status update. Otherwise MergeFrom(old) would see no diff // and the status patch would be a no-op. old = res.DeepCopy() - // Re-apply the status update that was overwritten by the re-fetch. + // Re-apply status updates that were overwritten by the re-fetch. res.Status.CommittedResourceReservation.Allocations = newStatusAllocations + if len(misplacedVMs) > 0 { + meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ReservationConditionVMMisplaced, + Status: metav1.ConditionTrue, + Reason: "MigratedToFullHost", + Message: fmt.Sprintf( + "VM(s) live-migrated to a host that lacks capacity for the reservation slot: %v", + misplacedVMs, + ), + }) + } else { + meta.RemoveStatusCondition(&res.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) + } } // Proactively remove this VM UUID from all other candidate reservations that still diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index 651852c2c..3b0edb281 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "testing" @@ -982,3 +983,328 @@ func TestCommitmentReservationController_DomainNameHint(t *testing.T) { }) } } + +// ============================================================================ +// Tests: live migration detection in reconcileAllocations +// ============================================================================ + +// newHVWithCapacity creates a Hypervisor CRD with the given instances and effective capacity. +func newHVWithCapacity(name string, memGiB, cpuCores int64, instances []hv1.Instance) *hv1.Hypervisor { + return &hv1.Hypervisor{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Status: hv1.HypervisorStatus{ + EffectiveCapacity: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), + hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + }, + Instances: instances, + }, + } +} + +// newConfirmedCRReservation creates a ready CR reservation with one confirmed VM on host. +func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: host, + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), + hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + }, + CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ + ProjectID: "test-project", + ResourceName: "test-flavor", + Allocations: map[string]v1alpha1.CommittedResourceAllocation{ + vmUUID: { + // allocation age well past any grace period + CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), + hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + }, + }, + }, + }, + }, + Status: v1alpha1.ReservationStatus{ + Host: host, + Conditions: []metav1.Condition{ + {Type: v1alpha1.ReservationConditionReady, Status: metav1.ConditionTrue, Reason: "ReservationActive"}, + }, + CommittedResourceReservation: &v1alpha1.CommittedResourceReservationStatus{ + Allocations: map[string]string{vmUUID: host}, + }, + }, + } +} + +// TestReconcileAllocations_LiveMigration_CapacityAvailable verifies that when a confirmed VM +// is detected on a different host that has sufficient capacity, TargetHost is updated to the +// new host and the VM is tracked at its actual location in Status.Allocations. +func TestReconcileAllocations_LiveMigration_CapacityAvailable(t *testing.T) { + scheme := newCRTestScheme(t) + config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} + + const ( + vmUUID = "vm-migrated" + oldHost = "host-old" + newHost = "host-new" + ) + + res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) + + // Old host: VM is gone. + hvOld := newTestHypervisorCRD(oldHost, []hv1.Instance{}) + + // New host: VM is present, has plenty of capacity, no other reservations blocking it. + hvNew := newHVWithCapacity(newHost, 960, 80, []hv1.Instance{ + {ID: vmUUID, Name: "vm-name", Active: true}, + }) + + k8sClient := newCRTestClient(scheme, res, hvOld, hvNew) + ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} + ctx := WithNewGlobalRequestID(context.Background()) + + if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { + t.Fatalf("reconcileAllocations() error = %v", err) + } + + var updated v1alpha1.Reservation + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { + t.Fatalf("failed to get updated reservation: %v", err) + } + + // Spec.TargetHost must be updated to the new host. + if updated.Spec.TargetHost != newHost { + t.Errorf("expected Spec.TargetHost=%q, got %q", newHost, updated.Spec.TargetHost) + } + + // VM must still be in Spec.Allocations (not removed). + if _, ok := updated.Spec.CommittedResourceReservation.Allocations[vmUUID]; !ok { + t.Errorf("expected VM %s to remain in Spec.Allocations after migration", vmUUID) + } + + // Status.Allocations must record the VM at its new actual host. + if updated.Status.CommittedResourceReservation == nil { + t.Fatal("expected Status.CommittedResourceReservation to be set") + } + if got := updated.Status.CommittedResourceReservation.Allocations[vmUUID]; got != newHost { + t.Errorf("expected Status.Allocations[%s]=%q, got %q", vmUUID, newHost, got) + } + + // VMMisplaced condition must NOT be set. + if cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced); cond != nil && cond.Status == metav1.ConditionTrue { + t.Errorf("expected VMMisplaced condition to be absent or false, got %+v", cond) + } +} + +// TestReconcileAllocations_LiveMigration_NoCapacity verifies that when a confirmed VM is +// detected on a different host that lacks sufficient capacity, TargetHost is left unchanged +// and the VMMisplaced condition is set. +func TestReconcileAllocations_LiveMigration_NoCapacity(t *testing.T) { + scheme := newCRTestScheme(t) + config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} + + const ( + vmUUID = "vm-migrated-no-cap" + oldHost = "host-old" + newHost = "host-new-full" + ) + + res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) + + // Old host: VM is gone. + hvOld := newTestHypervisorCRD(oldHost, []hv1.Instance{}) + + // New host: VM is present, but another reservation already blocks all capacity. + hvNew := newHVWithCapacity(newHost, 480, 40, []hv1.Instance{ + {ID: vmUUID, Name: "vm-name", Active: true}, + }) + blocker := &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "res-blocker"}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: newHost, + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: resource.MustParse("480Gi"), + hv1.ResourceCPU: resource.MustParse("40"), + }, + }, + Status: v1alpha1.ReservationStatus{Host: newHost}, + } + + k8sClient := newCRTestClient(scheme, res, hvOld, hvNew, blocker) + ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} + ctx := WithNewGlobalRequestID(context.Background()) + + if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { + t.Fatalf("reconcileAllocations() error = %v", err) + } + + var updated v1alpha1.Reservation + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { + t.Fatalf("failed to get updated reservation: %v", err) + } + + // Spec.TargetHost must remain unchanged (old host). + if updated.Spec.TargetHost != oldHost { + t.Errorf("expected Spec.TargetHost to remain %q, got %q", oldHost, updated.Spec.TargetHost) + } + + // VM must still be in Spec.Allocations (not removed). + if _, ok := updated.Spec.CommittedResourceReservation.Allocations[vmUUID]; !ok { + t.Errorf("expected VM %s to remain in Spec.Allocations after misplaced migration", vmUUID) + } + + // Status.Allocations must record the VM at its actual (new) host even though we can't follow it. + if updated.Status.CommittedResourceReservation == nil { + t.Fatal("expected Status.CommittedResourceReservation to be set") + } + if got := updated.Status.CommittedResourceReservation.Allocations[vmUUID]; got != newHost { + t.Errorf("expected Status.Allocations[%s]=%q (actual location), got %q", vmUUID, newHost, got) + } + + // VMMisplaced condition must be set to True. + cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) + if cond == nil { + t.Fatal("expected VMMisplaced condition to be set") + } + if cond.Status != metav1.ConditionTrue { + t.Errorf("expected VMMisplaced condition status True, got %s", cond.Status) + } + if cond.Reason != "MigratedToFullHost" { + t.Errorf("expected VMMisplaced reason MigratedToFullHost, got %s", cond.Reason) + } +} + +// TestReconcileAllocations_LiveMigration_VMGone verifies that when a confirmed VM is absent +// from its expected host and cannot be found on any other hypervisor, it is treated as +// terminated and removed from Spec.Allocations normally. +func TestReconcileAllocations_LiveMigration_VMGone(t *testing.T) { + scheme := newCRTestScheme(t) + config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} + + const ( + vmUUID = "vm-gone" + oldHost = "host-old" + ) + + res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) + + // Old host: VM is gone. No other hypervisor has the VM either. + hvOld := newTestHypervisorCRD(oldHost, []hv1.Instance{}) + hvOther := newTestHypervisorCRD("host-other", []hv1.Instance{ + {ID: "some-other-vm", Name: "other-vm", Active: true}, + }) + + k8sClient := newCRTestClient(scheme, res, hvOld, hvOther) + ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} + ctx := WithNewGlobalRequestID(context.Background()) + + if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { + t.Fatalf("reconcileAllocations() error = %v", err) + } + + var updated v1alpha1.Reservation + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { + t.Fatalf("failed to get updated reservation: %v", err) + } + + // VM must be removed from Spec.Allocations. + if _, ok := updated.Spec.CommittedResourceReservation.Allocations[vmUUID]; ok { + t.Errorf("expected VM %s to be removed from Spec.Allocations when not found anywhere", vmUUID) + } + + // Status.Allocations must be empty. + if updated.Status.CommittedResourceReservation != nil && + len(updated.Status.CommittedResourceReservation.Allocations) != 0 { + t.Errorf("expected empty Status.Allocations after VM removal, got %v", + updated.Status.CommittedResourceReservation.Allocations) + } + + // Spec.TargetHost must remain unchanged. + if updated.Spec.TargetHost != oldHost { + t.Errorf("expected Spec.TargetHost to remain %q, got %q", oldHost, updated.Spec.TargetHost) + } + + // VMMisplaced condition must NOT be set. + if cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced); cond != nil && cond.Status == metav1.ConditionTrue { + t.Errorf("expected VMMisplaced condition to be absent or false after removal, got %+v", cond) + } +} + +// TestReconcileAllocations_LiveMigration_ClearsStaleVMMisplaced verifies that the +// VMMisplaced condition is removed when the misplaced VM is no longer present in +// Spec.Allocations on a subsequent reconcile (e.g. after it was removed by the operator). +func TestReconcileAllocations_LiveMigration_ClearsStaleVMMisplaced(t *testing.T) { + scheme := newCRTestScheme(t) + config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} + + const ( + vmUUID = "vm-was-misplaced" + host = "host-1" + ) + + // Reservation still carries the VM in Spec but it's now confirmed back on its host. + // The VMMisplaced condition is stale from a previous cycle. + res := &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "res-1"}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: host, + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: resource.MustParse("480Gi"), + hv1.ResourceCPU: resource.MustParse("40"), + }, + CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ + ProjectID: "test-project", + ResourceName: "test-flavor", + Allocations: map[string]v1alpha1.CommittedResourceAllocation{ + vmUUID: { + CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: resource.MustParse("480Gi"), + hv1.ResourceCPU: resource.MustParse("40"), + }, + }, + }, + }, + }, + Status: v1alpha1.ReservationStatus{ + Host: host, + Conditions: []metav1.Condition{ + {Type: v1alpha1.ReservationConditionReady, Status: metav1.ConditionTrue, Reason: "ReservationActive"}, + {Type: v1alpha1.ReservationConditionVMMisplaced, Status: metav1.ConditionTrue, Reason: "MigratedToFullHost"}, + }, + CommittedResourceReservation: &v1alpha1.CommittedResourceReservationStatus{ + Allocations: map[string]string{vmUUID: host}, + }, + }, + } + + // VM is now back on the expected host. + hv := newTestHypervisorCRD(host, []hv1.Instance{ + {ID: vmUUID, Name: "vm-name", Active: true}, + }) + + k8sClient := newCRTestClient(scheme, res, hv) + ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} + ctx := WithNewGlobalRequestID(context.Background()) + + if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { + t.Fatalf("reconcileAllocations() error = %v", err) + } + + var updated v1alpha1.Reservation + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { + t.Fatalf("failed to get updated reservation: %v", err) + } + + // VMMisplaced condition must be removed (VM is healthy on its expected host). + cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) + if cond != nil && cond.Status == metav1.ConditionTrue { + t.Errorf("expected VMMisplaced condition to be cleared, still present: %+v", cond) + } +} From 844a38cb657a12cd95fa033d9c41d9a4bc415b74 Mon Sep 17 00:00:00 2001 From: Julius Clausnitzer Date: Thu, 16 Jul 2026 10:24:36 +0200 Subject: [PATCH 02/18] add reverse map for vm-hypervisor Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller.go | 59 +++++++++++-------- 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index b59b9027a..5fac8f611 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -468,6 +468,33 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte return nil } + // vmUUIDToHost and hvByName are built lazily alongside allHVs/allReservations. + // vmUUIDToHost is a reverse index from VM UUID to hypervisor name, covering all + // hypervisors — the same pattern used by vm_source.go and failover/controller.go. + // Building it once amortises the cost across all confirmed-missing VMs in one reconcile. + var vmUUIDToHost map[string]string + var hvByName map[string]hv1.Hypervisor + + ensureReverseIndex := func() error { + if vmUUIDToHost != nil { + return nil + } + if err := ensureHVsAndReservations(); err != nil { + return err + } + vmUUIDToHost = make(map[string]string) + hvByName = make(map[string]hv1.Hypervisor, len(allHVs.Items)) + for _, hv := range allHVs.Items { + hvByName[hv.Name] = hv + for _, inst := range hv.Status.Instances { + if _, seen := vmUUIDToHost[inst.ID]; !seen { + vmUUIDToHost[inst.ID] = hv.Name + } + } + } + return nil + } + // Build new Status.Allocations map based on HV CRD state. newStatusAllocations := make(map[string]string) // Track allocations to remove from Spec (stale/leaving VMs). @@ -519,25 +546,17 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } // Confirmed VM missing from expected host — could be a live migration. - // Scan all hypervisors to find where the VM actually landed. - if err := ensureHVsAndReservations(); err != nil { + // Build the reverse index (vmUUID → hvName) lazily on first miss. + if err := ensureReverseIndex(); err != nil { return nil, err } - var foundHost string - for i := range allHVs.Items { - if allHVs.Items[i].Name == expectedHost { - continue // already checked - } - for _, inst := range allHVs.Items[i].Status.Instances { - if inst.ID == vmUUID { - foundHost = allHVs.Items[i].Name - break - } - } - if foundHost != "" { - break - } + foundHost := vmUUIDToHost[vmUUID] + if foundHost == expectedHost { + // Index says it's still on the expected host — treat as present + // (race between HV CRD update and our per-host set built above). + newStatusAllocations[vmUUID] = expectedHost + continue } if foundHost == "" { @@ -552,13 +571,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte // VM found on a different host — live migration detected. // Check whether the new host can absorb the full reservation slot. - var foundHV hv1.Hypervisor - for i := range allHVs.Items { - if allHVs.Items[i].Name == foundHost { - foundHV = allHVs.Items[i] - break - } - } + foundHV := hvByName[foundHost] if reservations.HostHasCapacityForReservation(allReservations.Items, foundHV, res) { // New host has enough room — follow the VM. From 9bc0d646a9a1a1c850a85cacb0195eb55a609fd6 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 28 Jul 2026 14:42:23 +0200 Subject: [PATCH 03/18] fix Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller.go | 21 +++++++------------ .../reservation_controller_test.go | 8 +++---- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index 5fac8f611..e8c906c39 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -382,20 +382,15 @@ type reconcileAllocationsResult struct { // reconcileAllocations verifies all allocations in Spec against actual VM state using the // Hypervisor CRD as the sole source of truth. // -// For new allocations (within grace period): the VM may not yet appear in the HV CRD -// (still spawning), so we skip verification and requeue with a short interval. -// For older allocations: we check the HV CRD; VMs not found are considered leaving and -// removed from the reservation. +// New allocations within the grace period are skipped — the VM may not yet appear in the +// HV CRD while it is still spawning. Older allocations are verified; VMs no longer present +// on their expected host are either followed to a new host (live migration) or removed. // -// Live migration: when a confirmed VM is absent from the expected host, all HV CRDs are -// scanned to detect whether it moved to a different host. -// - New host has capacity: Spec.TargetHost is updated; the existing Branch B in Reconcile -// will then advance Status.Host on the next cycle. -// - New host has no capacity: TargetHost is left unchanged; the VM's actual location is -// recorded in Status; the VMMisplaced condition is set. -// -// Only the migrated VM's host is considered — other allocated VMs in the reservation are -// not moved, consistent with the single-VM scope defined in issue #373. +// When a confirmed VM is absent from its expected host, all HV CRDs are searched to +// determine whether it live-migrated. If the new host has capacity for the reservation +// slot, Spec.TargetHost is updated so the reservation follows the VM. If not, TargetHost +// is left unchanged and the VMMisplaced condition is set. Each VM is evaluated +// independently; other allocated VMs in the same reservation are not affected. func (r *CommitmentReservationController) reconcileAllocations(ctx context.Context, res *v1alpha1.Reservation) (*reconcileAllocationsResult, error) { logger := LoggerFromContext(ctx) result := &reconcileAllocationsResult{} diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index 3b0edb281..cb0133a29 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -447,7 +447,7 @@ func newTestCRReservation(allocations map[string]metav1.Time) *v1alpha1.Reservat // newTestHypervisorCRD creates a test Hypervisor CRD with instances. // -//nolint:unparam // name parameter allows future test flexibility + func newTestHypervisorCRD(name string, instances []hv1.Instance) *hv1.Hypervisor { return &hv1.Hypervisor{ ObjectMeta: metav1.ObjectMeta{ @@ -995,7 +995,7 @@ func newHVWithCapacity(name string, memGiB, cpuCores int64, instances []hv1.Inst Status: hv1.HypervisorStatus{ EffectiveCapacity: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), }, Instances: instances, }, @@ -1011,7 +1011,7 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 TargetHost: host, Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), }, CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ ProjectID: "test-project", @@ -1022,7 +1022,7 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), }, }, }, From ec9a0c8cb2d639ff17de872387f5e260de3642d2 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 28 Jul 2026 15:37:20 +0200 Subject: [PATCH 04/18] fiix Signed-off-by: juliusclausnitzer --- .../reservations/commitments/reservation_controller_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index cb0133a29..2ec7bff54 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -995,7 +995,7 @@ func newHVWithCapacity(name string, memGiB, cpuCores int64, instances []hv1.Inst Status: hv1.HypervisorStatus{ EffectiveCapacity: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), + hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), }, Instances: instances, }, @@ -1011,7 +1011,7 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 TargetHost: host, Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), + hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), }, CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ ProjectID: "test-project", @@ -1022,7 +1022,7 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), + hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), }, }, }, From bd6101488ac4378c18bb65893e19b93f52b36a5c Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 28 Jul 2026 16:23:20 +0200 Subject: [PATCH 05/18] fix Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index 2ec7bff54..c25106158 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -10,6 +10,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strconv" "testing" "time" @@ -995,7 +996,7 @@ func newHVWithCapacity(name string, memGiB, cpuCores int64, instances []hv1.Inst Status: hv1.HypervisorStatus{ EffectiveCapacity: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), }, Instances: instances, }, @@ -1011,7 +1012,7 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 TargetHost: host, Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), }, CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ ProjectID: "test-project", @@ -1022,7 +1023,7 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(fmt.Sprintf("%d", cpuCores)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), }, }, }, From 7dc91a5f4d8f060d0574a79a1a0328aafc27d6a9 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 28 Jul 2026 17:08:49 +0200 Subject: [PATCH 06/18] simplify Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller.go | 56 +++++++------------ 1 file changed, 19 insertions(+), 37 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index e8c906c39..18711ef35 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -463,33 +463,6 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte return nil } - // vmUUIDToHost and hvByName are built lazily alongside allHVs/allReservations. - // vmUUIDToHost is a reverse index from VM UUID to hypervisor name, covering all - // hypervisors — the same pattern used by vm_source.go and failover/controller.go. - // Building it once amortises the cost across all confirmed-missing VMs in one reconcile. - var vmUUIDToHost map[string]string - var hvByName map[string]hv1.Hypervisor - - ensureReverseIndex := func() error { - if vmUUIDToHost != nil { - return nil - } - if err := ensureHVsAndReservations(); err != nil { - return err - } - vmUUIDToHost = make(map[string]string) - hvByName = make(map[string]hv1.Hypervisor, len(allHVs.Items)) - for _, hv := range allHVs.Items { - hvByName[hv.Name] = hv - for _, inst := range hv.Status.Instances { - if _, seen := vmUUIDToHost[inst.ID]; !seen { - vmUUIDToHost[inst.ID] = hv.Name - } - } - } - return nil - } - // Build new Status.Allocations map based on HV CRD state. newStatusAllocations := make(map[string]string) // Track allocations to remove from Spec (stale/leaving VMs). @@ -541,17 +514,28 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } // Confirmed VM missing from expected host — could be a live migration. - // Build the reverse index (vmUUID → hvName) lazily on first miss. - if err := ensureReverseIndex(); err != nil { + // Scan all HVs to find where the VM is now. The list is fetched lazily + // and shared across any further misses in this reconcile cycle. + if err := ensureHVsAndReservations(); err != nil { return nil, err } - foundHost := vmUUIDToHost[vmUUID] - if foundHost == expectedHost { - // Index says it's still on the expected host — treat as present - // (race between HV CRD update and our per-host set built above). - newStatusAllocations[vmUUID] = expectedHost - continue + var foundHost string + var foundHV hv1.Hypervisor + for _, hv := range allHVs.Items { + if hv.Name == expectedHost { + continue // already checked via hvInstanceSet above + } + for _, inst := range hv.Status.Instances { + if inst.ID == vmUUID { + foundHost = hv.Name + foundHV = hv + break + } + } + if foundHost != "" { + break + } } if foundHost == "" { @@ -566,8 +550,6 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte // VM found on a different host — live migration detected. // Check whether the new host can absorb the full reservation slot. - foundHV := hvByName[foundHost] - if reservations.HostHasCapacityForReservation(allReservations.Items, foundHV, res) { // New host has enough room — follow the VM. logger.Info("VM live-migrated to host with capacity, updating TargetHost", From c9b86886a5e20476311a582ffbee00120d701b7e Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 28 Jul 2026 17:19:45 +0200 Subject: [PATCH 07/18] consolidate tests Signed-off-by: juliusclausnitzer --- .../reservation_controller_test.go | 336 +++++------------- 1 file changed, 95 insertions(+), 241 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index c25106158..eb05a730a 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -1019,7 +1019,6 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 ResourceName: "test-flavor", Allocations: map[string]v1alpha1.CommittedResourceAllocation{ vmUUID: { - // allocation age well past any grace period CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), @@ -1041,271 +1040,126 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 } } -// TestReconcileAllocations_LiveMigration_CapacityAvailable verifies that when a confirmed VM -// is detected on a different host that has sufficient capacity, TargetHost is updated to the -// new host and the VM is tracked at its actual location in Status.Allocations. -func TestReconcileAllocations_LiveMigration_CapacityAvailable(t *testing.T) { - scheme := newCRTestScheme(t) - config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} - +func TestReconcileAllocations_LiveMigration(t *testing.T) { const ( - vmUUID = "vm-migrated" + vmUUID = "vm-uuid" oldHost = "host-old" newHost = "host-new" ) - res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) - - // Old host: VM is gone. - hvOld := newTestHypervisorCRD(oldHost, []hv1.Instance{}) - - // New host: VM is present, has plenty of capacity, no other reservations blocking it. - hvNew := newHVWithCapacity(newHost, 960, 80, []hv1.Instance{ - {ID: vmUUID, Name: "vm-name", Active: true}, - }) - - k8sClient := newCRTestClient(scheme, res, hvOld, hvNew) - ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} - ctx := WithNewGlobalRequestID(context.Background()) - - if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { - t.Fatalf("reconcileAllocations() error = %v", err) - } - - var updated v1alpha1.Reservation - if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { - t.Fatalf("failed to get updated reservation: %v", err) - } - - // Spec.TargetHost must be updated to the new host. - if updated.Spec.TargetHost != newHost { - t.Errorf("expected Spec.TargetHost=%q, got %q", newHost, updated.Spec.TargetHost) - } - - // VM must still be in Spec.Allocations (not removed). - if _, ok := updated.Spec.CommittedResourceReservation.Allocations[vmUUID]; !ok { - t.Errorf("expected VM %s to remain in Spec.Allocations after migration", vmUUID) - } - - // Status.Allocations must record the VM at its new actual host. - if updated.Status.CommittedResourceReservation == nil { - t.Fatal("expected Status.CommittedResourceReservation to be set") - } - if got := updated.Status.CommittedResourceReservation.Allocations[vmUUID]; got != newHost { - t.Errorf("expected Status.Allocations[%s]=%q, got %q", vmUUID, newHost, got) - } - - // VMMisplaced condition must NOT be set. - if cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced); cond != nil && cond.Status == metav1.ConditionTrue { - t.Errorf("expected VMMisplaced condition to be absent or false, got %+v", cond) - } -} - -// TestReconcileAllocations_LiveMigration_NoCapacity verifies that when a confirmed VM is -// detected on a different host that lacks sufficient capacity, TargetHost is left unchanged -// and the VMMisplaced condition is set. -func TestReconcileAllocations_LiveMigration_NoCapacity(t *testing.T) { - scheme := newCRTestScheme(t) config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} - const ( - vmUUID = "vm-migrated-no-cap" - oldHost = "host-old" - newHost = "host-new-full" - ) - - res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) - - // Old host: VM is gone. - hvOld := newTestHypervisorCRD(oldHost, []hv1.Instance{}) - - // New host: VM is present, but another reservation already blocks all capacity. - hvNew := newHVWithCapacity(newHost, 480, 40, []hv1.Instance{ - {ID: vmUUID, Name: "vm-name", Active: true}, - }) - blocker := &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: "res-blocker"}, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeCommittedResource, - TargetHost: newHost, - Resources: map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceMemory: resource.MustParse("480Gi"), - hv1.ResourceCPU: resource.MustParse("40"), + tests := []struct { + name string + // extra objects beyond the base reservation and old host HV + extraObjects []client.Object + startConditions []metav1.Condition + // expected outcomes + wantTargetHost string + wantStatusHost string // expected in Status.Allocations[vmUUID]; "" means absent + wantSpecHasVM bool + wantVMMisplaced bool + }{ + { + name: "migrated to host with capacity: follow the VM", + extraObjects: []client.Object{ + newHVWithCapacity(newHost, 960, 80, []hv1.Instance{{ID: vmUUID, Active: true}}), }, + wantTargetHost: newHost, + wantStatusHost: newHost, + wantSpecHasVM: true, + wantVMMisplaced: false, }, - Status: v1alpha1.ReservationStatus{Host: newHost}, - } - - k8sClient := newCRTestClient(scheme, res, hvOld, hvNew, blocker) - ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} - ctx := WithNewGlobalRequestID(context.Background()) - - if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { - t.Fatalf("reconcileAllocations() error = %v", err) - } - - var updated v1alpha1.Reservation - if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { - t.Fatalf("failed to get updated reservation: %v", err) - } - - // Spec.TargetHost must remain unchanged (old host). - if updated.Spec.TargetHost != oldHost { - t.Errorf("expected Spec.TargetHost to remain %q, got %q", oldHost, updated.Spec.TargetHost) - } - - // VM must still be in Spec.Allocations (not removed). - if _, ok := updated.Spec.CommittedResourceReservation.Allocations[vmUUID]; !ok { - t.Errorf("expected VM %s to remain in Spec.Allocations after misplaced migration", vmUUID) - } - - // Status.Allocations must record the VM at its actual (new) host even though we can't follow it. - if updated.Status.CommittedResourceReservation == nil { - t.Fatal("expected Status.CommittedResourceReservation to be set") - } - if got := updated.Status.CommittedResourceReservation.Allocations[vmUUID]; got != newHost { - t.Errorf("expected Status.Allocations[%s]=%q (actual location), got %q", vmUUID, newHost, got) - } - - // VMMisplaced condition must be set to True. - cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) - if cond == nil { - t.Fatal("expected VMMisplaced condition to be set") - } - if cond.Status != metav1.ConditionTrue { - t.Errorf("expected VMMisplaced condition status True, got %s", cond.Status) - } - if cond.Reason != "MigratedToFullHost" { - t.Errorf("expected VMMisplaced reason MigratedToFullHost, got %s", cond.Reason) - } -} - -// TestReconcileAllocations_LiveMigration_VMGone verifies that when a confirmed VM is absent -// from its expected host and cannot be found on any other hypervisor, it is treated as -// terminated and removed from Spec.Allocations normally. -func TestReconcileAllocations_LiveMigration_VMGone(t *testing.T) { - scheme := newCRTestScheme(t) - config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} - - const ( - vmUUID = "vm-gone" - oldHost = "host-old" - ) - - res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) - - // Old host: VM is gone. No other hypervisor has the VM either. - hvOld := newTestHypervisorCRD(oldHost, []hv1.Instance{}) - hvOther := newTestHypervisorCRD("host-other", []hv1.Instance{ - {ID: "some-other-vm", Name: "other-vm", Active: true}, - }) - - k8sClient := newCRTestClient(scheme, res, hvOld, hvOther) - ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} - ctx := WithNewGlobalRequestID(context.Background()) - - if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { - t.Fatalf("reconcileAllocations() error = %v", err) - } - - var updated v1alpha1.Reservation - if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { - t.Fatalf("failed to get updated reservation: %v", err) - } - - // VM must be removed from Spec.Allocations. - if _, ok := updated.Spec.CommittedResourceReservation.Allocations[vmUUID]; ok { - t.Errorf("expected VM %s to be removed from Spec.Allocations when not found anywhere", vmUUID) - } - - // Status.Allocations must be empty. - if updated.Status.CommittedResourceReservation != nil && - len(updated.Status.CommittedResourceReservation.Allocations) != 0 { - t.Errorf("expected empty Status.Allocations after VM removal, got %v", - updated.Status.CommittedResourceReservation.Allocations) - } - - // Spec.TargetHost must remain unchanged. - if updated.Spec.TargetHost != oldHost { - t.Errorf("expected Spec.TargetHost to remain %q, got %q", oldHost, updated.Spec.TargetHost) - } - - // VMMisplaced condition must NOT be set. - if cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced); cond != nil && cond.Status == metav1.ConditionTrue { - t.Errorf("expected VMMisplaced condition to be absent or false after removal, got %+v", cond) - } -} - -// TestReconcileAllocations_LiveMigration_ClearsStaleVMMisplaced verifies that the -// VMMisplaced condition is removed when the misplaced VM is no longer present in -// Spec.Allocations on a subsequent reconcile (e.g. after it was removed by the operator). -func TestReconcileAllocations_LiveMigration_ClearsStaleVMMisplaced(t *testing.T) { - scheme := newCRTestScheme(t) - config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} - - const ( - vmUUID = "vm-was-misplaced" - host = "host-1" - ) - - // Reservation still carries the VM in Spec but it's now confirmed back on its host. - // The VMMisplaced condition is stale from a previous cycle. - res := &v1alpha1.Reservation{ - ObjectMeta: metav1.ObjectMeta{Name: "res-1"}, - Spec: v1alpha1.ReservationSpec{ - Type: v1alpha1.ReservationTypeCommittedResource, - TargetHost: host, - Resources: map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceMemory: resource.MustParse("480Gi"), - hv1.ResourceCPU: resource.MustParse("40"), - }, - CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ - ProjectID: "test-project", - ResourceName: "test-flavor", - Allocations: map[string]v1alpha1.CommittedResourceAllocation{ - vmUUID: { - CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), + { + name: "migrated to full host: mark misplaced, keep TargetHost", + extraObjects: []client.Object{ + newHVWithCapacity(newHost, 480, 40, []hv1.Instance{{ID: vmUUID, Active: true}}), + &v1alpha1.Reservation{ + ObjectMeta: metav1.ObjectMeta{Name: "res-blocker"}, + Spec: v1alpha1.ReservationSpec{ + Type: v1alpha1.ReservationTypeCommittedResource, + TargetHost: newHost, Resources: map[hv1.ResourceName]resource.Quantity{ hv1.ResourceMemory: resource.MustParse("480Gi"), hv1.ResourceCPU: resource.MustParse("40"), }, }, + Status: v1alpha1.ReservationStatus{Host: newHost}, }, }, + wantTargetHost: oldHost, + wantStatusHost: newHost, + wantSpecHasVM: true, + wantVMMisplaced: true, }, - Status: v1alpha1.ReservationStatus{ - Host: host, - Conditions: []metav1.Condition{ - {Type: v1alpha1.ReservationConditionReady, Status: metav1.ConditionTrue, Reason: "ReservationActive"}, + { + name: "VM gone from all hosts: remove allocation", + extraObjects: []client.Object{newTestHypervisorCRD("host-other", []hv1.Instance{{ID: "other-vm"}})}, + wantTargetHost: oldHost, + wantStatusHost: "", + wantSpecHasVM: false, + wantVMMisplaced: false, + }, + { + name: "stale VMMisplaced cleared when VM back on expected host", + startConditions: []metav1.Condition{ {Type: v1alpha1.ReservationConditionVMMisplaced, Status: metav1.ConditionTrue, Reason: "MigratedToFullHost"}, }, - CommittedResourceReservation: &v1alpha1.CommittedResourceReservationStatus{ - Allocations: map[string]string{vmUUID: host}, - }, + // VM is back on oldHost — newHVWithCapacity for oldHost provided via the base setup below + wantTargetHost: oldHost, + wantStatusHost: oldHost, + wantSpecHasVM: true, + wantVMMisplaced: false, }, } - // VM is now back on the expected host. - hv := newTestHypervisorCRD(host, []hv1.Instance{ - {ID: vmUUID, Name: "vm-name", Active: true}, - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := newCRTestScheme(t) + res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) + res.Status.Conditions = append(res.Status.Conditions, tt.startConditions...) + + // oldHost HV always has the VM absent (already migrated away), except for the + // "stale VMMisplaced" case where the VM is back. + oldInstances := []hv1.Instance{} + if tt.wantStatusHost == oldHost { + oldInstances = []hv1.Instance{{ID: vmUUID, Active: true}} + } + objects := []client.Object{res, newTestHypervisorCRD(oldHost, oldInstances)} + objects = append(objects, tt.extraObjects...) - k8sClient := newCRTestClient(scheme, res, hv) - ctrl := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} - ctx := WithNewGlobalRequestID(context.Background()) + k8sClient := newCRTestClient(scheme, objects...) + controller := &CommitmentReservationController{Client: k8sClient, Scheme: scheme, Conf: config} + ctx := WithNewGlobalRequestID(context.Background()) - if _, err := ctrl.reconcileAllocations(ctx, res); err != nil { - t.Fatalf("reconcileAllocations() error = %v", err) - } + if _, err := controller.reconcileAllocations(ctx, res); err != nil { + t.Fatalf("reconcileAllocations() error = %v", err) + } - var updated v1alpha1.Reservation - if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { - t.Fatalf("failed to get updated reservation: %v", err) - } + var updated v1alpha1.Reservation + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated); err != nil { + t.Fatalf("failed to get updated reservation: %v", err) + } - // VMMisplaced condition must be removed (VM is healthy on its expected host). - cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) - if cond != nil && cond.Status == metav1.ConditionTrue { - t.Errorf("expected VMMisplaced condition to be cleared, still present: %+v", cond) + if updated.Spec.TargetHost != tt.wantTargetHost { + t.Errorf("Spec.TargetHost = %q, want %q", updated.Spec.TargetHost, tt.wantTargetHost) + } + _, specHasVM := updated.Spec.CommittedResourceReservation.Allocations[vmUUID] + if specHasVM != tt.wantSpecHasVM { + t.Errorf("VM in Spec.Allocations = %v, want %v", specHasVM, tt.wantSpecHasVM) + } + var statusHost string + if updated.Status.CommittedResourceReservation != nil { + statusHost = updated.Status.CommittedResourceReservation.Allocations[vmUUID] + } + if statusHost != tt.wantStatusHost { + t.Errorf("Status.Allocations[%s] = %q, want %q", vmUUID, statusHost, tt.wantStatusHost) + } + cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) + isMisplaced := cond != nil && cond.Status == metav1.ConditionTrue + if isMisplaced != tt.wantVMMisplaced { + t.Errorf("VMMisplaced condition = %v, want %v", isMisplaced, tt.wantVMMisplaced) + } + }) } } From c7eff459f1d857b4e8bf93b7dbabf3c9e147a0c0 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Mon, 3 Aug 2026 11:35:03 +0200 Subject: [PATCH 08/18] fix: run make crds deepcopy lint-fix Signed-off-by: Julius Clausnitzer Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index eb05a730a..1d952ebe1 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -1055,10 +1055,10 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { extraObjects []client.Object startConditions []metav1.Condition // expected outcomes - wantTargetHost string - wantStatusHost string // expected in Status.Allocations[vmUUID]; "" means absent - wantSpecHasVM bool - wantVMMisplaced bool + wantTargetHost string + wantStatusHost string // expected in Status.Allocations[vmUUID]; "" means absent + wantSpecHasVM bool + wantVMMisplaced bool }{ { name: "migrated to host with capacity: follow the VM", From c11fb921389064e4cb4108b2577d619dd45b15f4 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Mon, 3 Aug 2026 11:40:30 +0200 Subject: [PATCH 09/18] fix Signed-off-by: juliusclausnitzer --- .../reservations/commitments/reservation_controller.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index 18711ef35..ce3df1581 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -584,9 +584,9 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } // Update TargetHost when the migrated VM moved to a host with capacity. - // This will be picked up by the TargetHost→Status.Host sync in the next Reconcile - // cycle (Branch B), which advances Status.Host and marks the reservation active on - // the new host. We do NOT update Status.Host here to avoid bypassing that sync path. + // Setting Spec.TargetHost triggers the TargetHost→Status.Host sync path in Reconcile, + // which advances Status.Host and marks the reservation active on the new host. + // We do NOT update Status.Host here to avoid bypassing that sync path. if migrationTargetHost != "" { res.Spec.TargetHost = migrationTargetHost specChanged = true From 9c8879f4a291c8546b12e2660c6d056fc7e49ccc Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Mon, 3 Aug 2026 12:02:05 +0200 Subject: [PATCH 10/18] fix host capacity check Signed-off-by: juliusclausnitzer --- .../reservations/capacity_accounting.go | 16 ++++++++++------ .../reservations/capacity_accounting_test.go | 2 +- .../commitments/reservation_controller_test.go | 17 +++++++++++------ 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/internal/scheduling/reservations/capacity_accounting.go b/internal/scheduling/reservations/capacity_accounting.go index ed5208136..e9b437276 100644 --- a/internal/scheduling/reservations/capacity_accounting.go +++ b/internal/scheduling/reservations/capacity_accounting.go @@ -11,17 +11,21 @@ import ( ) // HostHasCapacityForReservation reports whether hv has sufficient remaining capacity to -// accommodate the full Spec.Resources slot of res. +// accommodate the reservation's unfilled slot portion. // // It uses the same accounting as the scheduler's filter_has_enough_capacity: // 1. Start from EffectiveCapacity (or Capacity when EffectiveCapacity is nil). // 2. Subtract hv.Status.Allocation (VMs already running on this host). -// 3. For each other reservation in allReservations that is assigned to this host +// 3. For each other reservation in allReservations assigned to this host // (via Spec.TargetHost or Status.Host), subtract its UnusedReservationCapacity. -// 4. Check that the remainder is ≥ res.Spec.Resources for every resource. +// 4. Check that the remainder is ≥ UnusedReservationCapacity(res). // -// The target reservation itself (matched by name) is excluded from the blocking -// calculation so we don't double-count it. +// Step 4 uses UnusedReservationCapacity rather than the full Spec.Resources because +// confirmed VMs already appear in hv.Status.Allocation (step 2); comparing against the +// full slot would count those resources twice. UnusedReservationCapacity returns the +// unfilled portion (slot − confirmed VMs), which is what the host still needs to absorb. +// +// The target reservation itself is excluded from step 3 to avoid double-counting it. // Returns false when the hypervisor has no capacity data. func HostHasCapacityForReservation(allReservations []v1alpha1.Reservation, hv hv1.Hypervisor, res *v1alpha1.Reservation) bool { effCap := hv.Status.EffectiveCapacity @@ -63,7 +67,7 @@ func HostHasCapacityForReservation(allReservations []v1alpha1.Reservation, hv hv } zero := resource.Quantity{} - for rn, required := range res.Spec.Resources { + for rn, required := range UnusedReservationCapacity(res, false) { remaining, ok := free[rn] if !ok { return false diff --git a/internal/scheduling/reservations/capacity_accounting_test.go b/internal/scheduling/reservations/capacity_accounting_test.go index db5caa63d..aef24fe71 100644 --- a/internal/scheduling/reservations/capacity_accounting_test.go +++ b/internal/scheduling/reservations/capacity_accounting_test.go @@ -299,7 +299,7 @@ func TestHostHasCapacityForReservation(t *testing.T) { r := resWithSlot("res-target", "host-old", 480, 40) return &r }(), - wantFits: false, // 480-100 = 380 GiB < 480 GiB required + wantFits: false, // 480-100 = 380 GiB remaining < 480 GiB slot (no confirmed VMs, so full slot is required) }, { name: "reservation targeting via Status.Host (not TargetHost) still blocks", diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index 1d952ebe1..be4f1194b 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -1004,15 +1004,17 @@ func newHVWithCapacity(name string, memGiB, cpuCores int64, instances []hv1.Inst } // newConfirmedCRReservation creates a ready CR reservation with one confirmed VM on host. -func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64) *v1alpha1.Reservation { +// slotMemGiB/slotCPU define the full reservation slot; vmMemGiB/vmCPU define what the VM +// actually consumes — these may be smaller, leaving an unfilled remainder in the slot. +func newConfirmedCRReservation(name, host, vmUUID string, slotMemGiB, slotCPU, vmMemGiB, vmCPU int64) *v1alpha1.Reservation { return &v1alpha1.Reservation{ ObjectMeta: metav1.ObjectMeta{Name: name}, Spec: v1alpha1.ReservationSpec{ Type: v1alpha1.ReservationTypeCommittedResource, TargetHost: host, Resources: map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), + hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", slotMemGiB)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(slotCPU, 10)), }, CommittedResourceReservation: &v1alpha1.CommittedResourceReservationSpec{ ProjectID: "test-project", @@ -1021,8 +1023,8 @@ func newConfirmedCRReservation(name, host, vmUUID string, memGiB, cpuCores int64 vmUUID: { CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), Resources: map[hv1.ResourceName]resource.Quantity{ - hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", memGiB)), - hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(cpuCores, 10)), + hv1.ResourceMemory: resource.MustParse(fmt.Sprintf("%dGi", vmMemGiB)), + hv1.ResourceCPU: resource.MustParse(strconv.FormatInt(vmCPU, 10)), }, }, }, @@ -1063,6 +1065,7 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { { name: "migrated to host with capacity: follow the VM", extraObjects: []client.Object{ + // VM (240Gi/20) already running here; slot remainder is 240Gi/20 — fits easily. newHVWithCapacity(newHost, 960, 80, []hv1.Instance{{ID: vmUUID, Active: true}}), }, wantTargetHost: newHost, @@ -1073,6 +1076,8 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { { name: "migrated to full host: mark misplaced, keep TargetHost", extraObjects: []client.Object{ + // VM (240Gi/20) already running here; a full-slot blocker leaves no room + // for the slot remainder (240Gi/20). newHVWithCapacity(newHost, 480, 40, []hv1.Instance{{ID: vmUUID, Active: true}}), &v1alpha1.Reservation{ ObjectMeta: metav1.ObjectMeta{Name: "res-blocker"}, @@ -1116,7 +1121,7 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { scheme := newCRTestScheme(t) - res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40) + res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40, 240, 20) res.Status.Conditions = append(res.Status.Conditions, tt.startConditions...) // oldHost HV always has the VM absent (already migrated away), except for the From e32a2daf99b8b35f44c7acce6f79f7402a8fa2fd Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Mon, 3 Aug 2026 12:04:19 +0200 Subject: [PATCH 11/18] fix comment Signed-off-by: juliusclausnitzer --- .../reservations/capacity_accounting.go | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/internal/scheduling/reservations/capacity_accounting.go b/internal/scheduling/reservations/capacity_accounting.go index e9b437276..356055387 100644 --- a/internal/scheduling/reservations/capacity_accounting.go +++ b/internal/scheduling/reservations/capacity_accounting.go @@ -11,21 +11,18 @@ import ( ) // HostHasCapacityForReservation reports whether hv has sufficient remaining capacity to -// accommodate the reservation's unfilled slot portion. +// absorb res moving to it — i.e. whether the unfilled portion of res's slot fits alongside +// everything already committed on the host. // -// It uses the same accounting as the scheduler's filter_has_enough_capacity: // 1. Start from EffectiveCapacity (or Capacity when EffectiveCapacity is nil). -// 2. Subtract hv.Status.Allocation (VMs already running on this host). -// 3. For each other reservation in allReservations assigned to this host -// (via Spec.TargetHost or Status.Host), subtract its UnusedReservationCapacity. -// 4. Check that the remainder is ≥ UnusedReservationCapacity(res). +// 2. Subtract hv.Status.Allocation (VMs physically running on this host). +// 3. For each other reservation assigned to this host (via Spec.TargetHost or Status.Host), +// subtract its UnusedReservationCapacity. +// 4. Check that the remainder is ≥ UnusedReservationCapacity(res): the unfilled portion of +// res's slot. Confirmed VMs in res already appear in hv.Status.Allocation (step 2), so +// comparing against the full slot would count them twice. // -// Step 4 uses UnusedReservationCapacity rather than the full Spec.Resources because -// confirmed VMs already appear in hv.Status.Allocation (step 2); comparing against the -// full slot would count those resources twice. UnusedReservationCapacity returns the -// unfilled portion (slot − confirmed VMs), which is what the host still needs to absorb. -// -// The target reservation itself is excluded from step 3 to avoid double-counting it. +// res itself is excluded from step 3 to avoid subtracting its own block from free capacity. // Returns false when the hypervisor has no capacity data. func HostHasCapacityForReservation(allReservations []v1alpha1.Reservation, hv hv1.Hypervisor, res *v1alpha1.Reservation) bool { effCap := hv.Status.EffectiveCapacity From 113d89a98cb08d2b36ae63e71f6db465f3aff99e Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Mon, 3 Aug 2026 12:09:50 +0200 Subject: [PATCH 12/18] add comment for race condition Signed-off-by: juliusclausnitzer --- .../reservations/commitments/reservation_controller.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index ce3df1581..751d153f8 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -539,7 +539,13 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } if foundHost == "" { - // VM is not on any known hypervisor — it has been terminated or evacuated. + // VM is not on any known hypervisor. This covers two cases: + // 1. The VM was terminated or evacuated — correct to remove. + // 2. The VM is mid-live-migration: it has left host-old's HV CRD but + // host-new's CRD has not been updated yet. In this window the VM + // is incorrectly treated as gone and removed from the reservation. + // A VM CRD with lifecycle state (migrating/active) would close this + // gap; without one we accept this narrow race as a known limitation. allocationsToRemove = append(allocationsToRemove, vmUUID) logger.Info("removing confirmed allocation (VM not found on any hypervisor)", "vm", vmUUID, From a1b7dcac35e2f618541fb7576212081f640f3ddc Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 4 Aug 2026 10:30:15 +0200 Subject: [PATCH 13/18] review changes Signed-off-by: juliusclausnitzer --- api/v1alpha1/reservation_types.go | 6 - .../commitments/reservation_controller.go | 78 ++++------- .../reservation_controller_test.go | 124 ++++++++++-------- 3 files changed, 98 insertions(+), 110 deletions(-) diff --git a/api/v1alpha1/reservation_types.go b/api/v1alpha1/reservation_types.go index f4b1b1b56..f52797654 100644 --- a/api/v1alpha1/reservation_types.go +++ b/api/v1alpha1/reservation_types.go @@ -181,12 +181,6 @@ type ReservationSpec struct { const ( // ReservationConditionReady indicates whether the reservation is active and ready. ReservationConditionReady = "Ready" - - // ReservationConditionVMMisplaced indicates that one or more VMs have been detected - // on a host other than TargetHost (e.g. after a live migration), but the new host - // lacks sufficient capacity to accept the full reservation slot. The VM is tracked in - // its new location but TargetHost is not updated until capacity becomes available. - ReservationConditionVMMisplaced = "VMMisplaced" ) // CommittedResourceReservationStatus defines the status fields specific to committed resource reservations. diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index 751d153f8..269555e42 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -384,13 +384,14 @@ type reconcileAllocationsResult struct { // // New allocations within the grace period are skipped — the VM may not yet appear in the // HV CRD while it is still spawning. Older allocations are verified; VMs no longer present -// on their expected host are either followed to a new host (live migration) or removed. +// on their expected host are handled as follows: // -// When a confirmed VM is absent from its expected host, all HV CRDs are searched to -// determine whether it live-migrated. If the new host has capacity for the reservation -// slot, Spec.TargetHost is updated so the reservation follows the VM. If not, TargetHost -// is left unchanged and the VMMisplaced condition is set. Each VM is evaluated -// independently; other allocated VMs in the same reservation are not affected. +// Live migration: when a confirmed VM is found on a different host, the reservation follows +// it only when the reservation has exactly one allocated VM and the new host has capacity. +// In all other cases (multiple VMs, or new host at capacity), the migrated VM is removed +// from the reservation so the slot remains available for re-use on the original host. +// Moving TargetHost when other VMs are present would cause those remaining VMs to appear +// misplaced on the next reconcile cycle. func (r *CommitmentReservationController) reconcileAllocations(ctx context.Context, res *v1alpha1.Reservation) (*reconcileAllocationsResult, error) { logger := LoggerFromContext(ctx) result := &reconcileAllocationsResult{} @@ -441,13 +442,11 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte existingStatusAllocations[k] = v } - // hvList is fetched lazily and only once — only needed when a confirmed VM is missing - // from its expected host and we need to search for it across all hypervisors. + // allHVs and allReservations are fetched lazily — only needed when a confirmed VM is + // missing from its expected host and we need to scan for a live migration. var allHVs *hv1.HypervisorList - // allReservations is fetched lazily — needed alongside allHVs to check capacity. var allReservations *v1alpha1.ReservationList - // ensureHVsAndReservations fetches allHVs and allReservations on first call. ensureHVsAndReservations := func() error { if allHVs != nil { return nil @@ -468,11 +467,10 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte // Track allocations to remove from Spec (stale/leaving VMs). var allocationsToRemove []string - // migrationTargetHost is set when exactly one confirmed VM is detected on a new host - // that has capacity — in that case we update Spec.TargetHost to the new host. + // migrationTargetHost is set when the reservation has exactly one VM, that VM + // live-migrated to a new host, and the new host has capacity. In that case + // Spec.TargetHost is updated so the reservation follows the VM. migrationTargetHost := "" - // misplacedVMs accumulates VM UUIDs that moved to a host without capacity. - var misplacedVMs []string for vmUUID, allocation := range res.Spec.CommittedResourceReservation.Allocations { allocationAge := now.Sub(allocation.CreationTimestamp.Time) @@ -555,9 +553,14 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } // VM found on a different host — live migration detected. - // Check whether the new host can absorb the full reservation slot. - if reservations.HostHasCapacityForReservation(allReservations.Items, foundHV, res) { - // New host has enough room — follow the VM. + // + // Follow the VM only when this is the sole VM in the reservation and the new + // host has capacity. Moving TargetHost with multiple VMs present would cause + // the remaining VMs to appear misplaced on the next reconcile. When there are + // multiple VMs, or the new host is at capacity, remove this VM so the slot + // on the original host remains available for re-use. + isSingleVM := len(res.Spec.CommittedResourceReservation.Allocations) == 1 + if isSingleVM && reservations.HostHasCapacityForReservation(allReservations.Items, foundHV, res) { logger.Info("VM live-migrated to host with capacity, updating TargetHost", "vm", vmUUID, "reservation", res.Name, @@ -566,14 +569,13 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte migrationTargetHost = foundHost newStatusAllocations[vmUUID] = foundHost } else { - // New host is over capacity — record misplacement; keep old TargetHost. - logger.Info("VM live-migrated to host without sufficient capacity, marking misplaced", + logger.Info("removing VM from reservation after live migration: either multiple VMs present or new host lacks capacity", "vm", vmUUID, "reservation", res.Name, "expectedHost", expectedHost, - "actualHost", foundHost) - misplacedVMs = append(misplacedVMs, vmUUID) - newStatusAllocations[vmUUID] = foundHost + "actualHost", foundHost, + "singleVM", isSingleVM) + allocationsToRemove = append(allocationsToRemove, vmUUID) } } @@ -589,7 +591,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte specChanged = true } - // Update TargetHost when the migrated VM moved to a host with capacity. + // Update TargetHost when the single migrated VM moved to a host with capacity. // Setting Spec.TargetHost triggers the TargetHost→Status.Host sync path in Reconcile, // which advances Status.Host and marks the reservation active on the new host. // We do NOT update Status.Host here to avoid bypassing that sync path. @@ -601,21 +603,6 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte // Update Status.Allocations res.Status.CommittedResourceReservation.Allocations = newStatusAllocations - // Set or clear the VMMisplaced condition. - if len(misplacedVMs) > 0 { - meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ - Type: v1alpha1.ReservationConditionVMMisplaced, - Status: metav1.ConditionTrue, - Reason: "MigratedToFullHost", - Message: fmt.Sprintf( - "VM(s) live-migrated to a host that lacks capacity for the reservation slot: %v", - misplacedVMs, - ), - }) - } else { - meta.RemoveStatusCondition(&res.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) - } - // Patch Spec if changed (stale allocations removed and/or TargetHost updated) if specChanged { if err := r.Patch(ctx, res, client.MergeFrom(old)); err != nil { @@ -635,21 +622,8 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte // the status update. Otherwise MergeFrom(old) would see no diff // and the status patch would be a no-op. old = res.DeepCopy() - // Re-apply status updates that were overwritten by the re-fetch. + // Re-apply the status update that was overwritten by the re-fetch. res.Status.CommittedResourceReservation.Allocations = newStatusAllocations - if len(misplacedVMs) > 0 { - meta.SetStatusCondition(&res.Status.Conditions, metav1.Condition{ - Type: v1alpha1.ReservationConditionVMMisplaced, - Status: metav1.ConditionTrue, - Reason: "MigratedToFullHost", - Message: fmt.Sprintf( - "VM(s) live-migrated to a host that lacks capacity for the reservation slot: %v", - misplacedVMs, - ), - }) - } else { - meta.RemoveStatusCondition(&res.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) - } } // Proactively remove this VM UUID from all other candidate reservations that still diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index be4f1194b..be3f54b64 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -1044,40 +1044,39 @@ func newConfirmedCRReservation(name, host, vmUUID string, slotMemGiB, slotCPU, v func TestReconcileAllocations_LiveMigration(t *testing.T) { const ( - vmUUID = "vm-uuid" - oldHost = "host-old" - newHost = "host-new" + vmUUID = "vm-uuid" + vm2UUID = "vm-uuid-2" + oldHost = "host-old" + newHost = "host-new" ) config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} tests := []struct { - name string + name string + // reservation to use; nil uses the default single-VM reservation + reservation *v1alpha1.Reservation // extra objects beyond the base reservation and old host HV - extraObjects []client.Object - startConditions []metav1.Condition + extraObjects []client.Object // expected outcomes - wantTargetHost string - wantStatusHost string // expected in Status.Allocations[vmUUID]; "" means absent - wantSpecHasVM bool - wantVMMisplaced bool + wantTargetHost string + wantStatusHost string // expected in Status.Allocations[vmUUID]; "" means absent + wantSpecHasVM bool }{ { - name: "migrated to host with capacity: follow the VM", + name: "single VM, new host has capacity: follow the VM", extraObjects: []client.Object{ - // VM (240Gi/20) already running here; slot remainder is 240Gi/20 — fits easily. newHVWithCapacity(newHost, 960, 80, []hv1.Instance{{ID: vmUUID, Active: true}}), }, - wantTargetHost: newHost, - wantStatusHost: newHost, - wantSpecHasVM: true, - wantVMMisplaced: false, + wantTargetHost: newHost, + wantStatusHost: newHost, + wantSpecHasVM: true, }, { - name: "migrated to full host: mark misplaced, keep TargetHost", + name: "single VM, new host at capacity: remove VM, slot stays on old host", extraObjects: []client.Object{ - // VM (240Gi/20) already running here; a full-slot blocker leaves no room - // for the slot remainder (240Gi/20). + // VM (240Gi/20) running on newHost; a full-slot blocker leaves no room for + // the 240Gi/20 slot remainder. newHVWithCapacity(newHost, 480, 40, []hv1.Instance{{ID: vmUUID, Active: true}}), &v1alpha1.Reservation{ ObjectMeta: metav1.ObjectMeta{Name: "res-blocker"}, @@ -1092,45 +1091,71 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { Status: v1alpha1.ReservationStatus{Host: newHost}, }, }, - wantTargetHost: oldHost, - wantStatusHost: newHost, - wantSpecHasVM: true, - wantVMMisplaced: true, + wantTargetHost: oldHost, + wantStatusHost: "", + wantSpecHasVM: false, }, { - name: "VM gone from all hosts: remove allocation", - extraObjects: []client.Object{newTestHypervisorCRD("host-other", []hv1.Instance{{ID: "other-vm"}})}, - wantTargetHost: oldHost, - wantStatusHost: "", - wantSpecHasVM: false, - wantVMMisplaced: false, + name: "multiple VMs, one migrated: remove migrated VM, never update TargetHost", + reservation: func() *v1alpha1.Reservation { + res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40, 240, 20) + // Add a second VM confirmed on oldHost. + res.Spec.CommittedResourceReservation.Allocations[vm2UUID] = v1alpha1.CommittedResourceAllocation{ + CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Hour)), + Resources: map[hv1.ResourceName]resource.Quantity{ + hv1.ResourceMemory: resource.MustParse("240Gi"), + hv1.ResourceCPU: resource.MustParse("20"), + }, + } + res.Status.CommittedResourceReservation.Allocations[vm2UUID] = oldHost + return res + }(), + extraObjects: []client.Object{ + // vmUUID has migrated to newHost with plenty of capacity; vm2UUID stays on oldHost. + // Supply oldHost HV explicitly so vm2UUID is present on it. + newTestHypervisorCRD(oldHost, []hv1.Instance{{ID: vm2UUID, Active: true}}), + newHVWithCapacity(newHost, 960, 80, []hv1.Instance{{ID: vmUUID, Active: true}}), + }, + wantTargetHost: oldHost, + wantStatusHost: "", + wantSpecHasVM: false, }, { - name: "stale VMMisplaced cleared when VM back on expected host", - startConditions: []metav1.Condition{ - {Type: v1alpha1.ReservationConditionVMMisplaced, Status: metav1.ConditionTrue, Reason: "MigratedToFullHost"}, - }, - // VM is back on oldHost — newHVWithCapacity for oldHost provided via the base setup below - wantTargetHost: oldHost, - wantStatusHost: oldHost, - wantSpecHasVM: true, - wantVMMisplaced: false, + name: "VM gone from all hosts: remove allocation", + extraObjects: []client.Object{newTestHypervisorCRD("host-other", []hv1.Instance{{ID: "other-vm"}})}, + wantTargetHost: oldHost, + wantStatusHost: "", + wantSpecHasVM: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { scheme := newCRTestScheme(t) - res := newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40, 240, 20) - res.Status.Conditions = append(res.Status.Conditions, tt.startConditions...) - - // oldHost HV always has the VM absent (already migrated away), except for the - // "stale VMMisplaced" case where the VM is back. - oldInstances := []hv1.Instance{} - if tt.wantStatusHost == oldHost { - oldInstances = []hv1.Instance{{ID: vmUUID, Active: true}} + + res := tt.reservation + if res == nil { + res = newConfirmedCRReservation("res-1", oldHost, vmUUID, 480, 40, 240, 20) + } + + var objects []client.Object + objects = append(objects, res) + + // Add an oldHost HV unless the test provides its own via extraObjects. + addsOldHostHV := false + for _, obj := range tt.extraObjects { + if hv, ok := obj.(*hv1.Hypervisor); ok && hv.Name == oldHost { + addsOldHostHV = true + break + } + } + if !addsOldHostHV { + oldInstances := []hv1.Instance{} + if tt.wantStatusHost == oldHost { + oldInstances = []hv1.Instance{{ID: vmUUID, Active: true}} + } + objects = append(objects, newTestHypervisorCRD(oldHost, oldInstances)) } - objects := []client.Object{res, newTestHypervisorCRD(oldHost, oldInstances)} objects = append(objects, tt.extraObjects...) k8sClient := newCRTestClient(scheme, objects...) @@ -1160,11 +1185,6 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { if statusHost != tt.wantStatusHost { t.Errorf("Status.Allocations[%s] = %q, want %q", vmUUID, statusHost, tt.wantStatusHost) } - cond := meta.FindStatusCondition(updated.Status.Conditions, v1alpha1.ReservationConditionVMMisplaced) - isMisplaced := cond != nil && cond.Status == metav1.ConditionTrue - if isMisplaced != tt.wantVMMisplaced { - t.Errorf("VMMisplaced condition = %v, want %v", isMisplaced, tt.wantVMMisplaced) - } }) } } From b3fc1df59777f0849efd728de5734672e5880d52 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 4 Aug 2026 11:53:25 +0200 Subject: [PATCH 14/18] feat(reservations): update reservation on VM live migration Signed-off-by: Julius Clausnitzer Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index be3f54b64..1a30be0a8 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -1044,18 +1044,18 @@ func newConfirmedCRReservation(name, host, vmUUID string, slotMemGiB, slotCPU, v func TestReconcileAllocations_LiveMigration(t *testing.T) { const ( - vmUUID = "vm-uuid" - vm2UUID = "vm-uuid-2" - oldHost = "host-old" - newHost = "host-new" + vmUUID = "vm-uuid" + vm2UUID = "vm-uuid-2" + oldHost = "host-old" + newHost = "host-new" ) config := ReservationControllerConfig{AllocationGracePeriod: metav1.Duration{Duration: 15 * time.Minute}} tests := []struct { - name string + name string // reservation to use; nil uses the default single-VM reservation - reservation *v1alpha1.Reservation + reservation *v1alpha1.Reservation // extra objects beyond the base reservation and old host HV extraObjects []client.Object // expected outcomes From 97c2974be56ef3d4599c4783d318c21ba89d2825 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 4 Aug 2026 12:00:19 +0200 Subject: [PATCH 15/18] review fixes Signed-off-by: juliusclausnitzer --- .../reservations/capacity_accounting.go | 6 +-- .../reservations/capacity_accounting_test.go | 42 ++++++++--------- .../commitments/reservation_controller.go | 27 +++++++---- .../reservation_controller_test.go | 46 +++++++++++++++---- 4 files changed, 75 insertions(+), 46 deletions(-) diff --git a/internal/scheduling/reservations/capacity_accounting.go b/internal/scheduling/reservations/capacity_accounting.go index 356055387..46cde2903 100644 --- a/internal/scheduling/reservations/capacity_accounting.go +++ b/internal/scheduling/reservations/capacity_accounting.go @@ -47,7 +47,7 @@ func HostHasCapacityForReservation(allReservations []v1alpha1.Reservation, hv hv for i := range allReservations { other := &allReservations[i] - if other.Name == res.Name { + if other.Name == res.Name && other.Namespace == res.Namespace { continue } // Only block resources from reservations that target or are confirmed on this host. @@ -63,15 +63,11 @@ func HostHasCapacityForReservation(allReservations []v1alpha1.Reservation, hv hv } } - zero := resource.Quantity{} for rn, required := range UnusedReservationCapacity(res, false) { remaining, ok := free[rn] if !ok { return false } - if remaining.Cmp(zero) < 0 { - return false - } if remaining.Cmp(required) < 0 { return false } diff --git a/internal/scheduling/reservations/capacity_accounting_test.go b/internal/scheduling/reservations/capacity_accounting_test.go index aef24fe71..d13a1c400 100644 --- a/internal/scheduling/reservations/capacity_accounting_test.go +++ b/internal/scheduling/reservations/capacity_accounting_test.go @@ -188,8 +188,8 @@ func TestHostHasCapacityForReservation(t *testing.T) { } } - resWithSlot := func(name, targetHost string, memGiB, cpuCores int64) v1alpha1.Reservation { - return v1alpha1.Reservation{ + resWithSlot := func(name, targetHost string, memGiB, cpuCores int64) *v1alpha1.Reservation { + return &v1alpha1.Reservation{ ObjectMeta: metav1.ObjectMeta{Name: name}, Spec: v1alpha1.ReservationSpec{ Type: v1alpha1.ReservationTypeCommittedResource, @@ -202,6 +202,7 @@ func TestHostHasCapacityForReservation(t *testing.T) { Status: v1alpha1.ReservationStatus{Host: targetHost}, } } + deref := func(r *v1alpha1.Reservation) v1alpha1.Reservation { return *r } tests := []struct { name string @@ -213,43 +214,43 @@ func TestHostHasCapacityForReservation(t *testing.T) { { name: "empty host: slot fits easily", hv: hvWithCapacity("host-new", 960, 80), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-1", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-1", "host-old", 480, 40), wantFits: true, }, { name: "host fully consumed by another reservation: no capacity", hv: hvWithCapacity("host-new", 480, 40), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-old", 480, 40), others: []v1alpha1.Reservation{ - resWithSlot("res-blocker", "host-new", 480, 40), + deref(resWithSlot("res-blocker", "host-new", 480, 40)), }, wantFits: false, }, { name: "host partially consumed, enough room left", hv: hvWithCapacity("host-new", 960, 80), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-old", 480, 40), others: []v1alpha1.Reservation{ - resWithSlot("res-blocker", "host-new", 480, 40), + deref(resWithSlot("res-blocker", "host-new", 480, 40)), }, wantFits: true, }, { name: "host partially consumed, exactly at boundary: fits", hv: hvWithCapacity("host-new", 960, 80), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-old", 480, 40), others: []v1alpha1.Reservation{ - resWithSlot("res-blocker-a", "host-new", 240, 20), - resWithSlot("res-blocker-b", "host-new", 240, 20), + deref(resWithSlot("res-blocker-a", "host-new", 240, 20)), + deref(resWithSlot("res-blocker-b", "host-new", 240, 20)), }, wantFits: true, }, { name: "host partially consumed, one resource short (CPU)", hv: hvWithCapacity("host-new", 960, 60), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-old", 480, 40), others: []v1alpha1.Reservation{ - resWithSlot("res-blocker", "host-new", 480, 40), + deref(resWithSlot("res-blocker", "host-new", 480, 40)), }, // 960-480=480 memory OK, but 60-40=20 CPU < 40 required wantFits: false, @@ -257,19 +258,19 @@ func TestHostHasCapacityForReservation(t *testing.T) { { name: "target reservation itself excluded from blocking calculation", hv: hvWithCapacity("host-new", 480, 40), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-new", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-new", 480, 40), others: []v1alpha1.Reservation{ // Same name as res — should be ignored - resWithSlot("res-target", "host-new", 480, 40), + deref(resWithSlot("res-target", "host-new", 480, 40)), }, wantFits: true, }, { name: "reservations on other hosts do not count", hv: hvWithCapacity("host-new", 480, 40), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-old", 480, 40), others: []v1alpha1.Reservation{ - resWithSlot("res-on-other-host", "host-unrelated", 480, 40), + deref(resWithSlot("res-on-other-host", "host-unrelated", 480, 40)), }, wantFits: true, }, @@ -278,7 +279,7 @@ func TestHostHasCapacityForReservation(t *testing.T) { hv: hv1.Hypervisor{ ObjectMeta: metav1.ObjectMeta{Name: "host-nocap"}, }, - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-old", 480, 40), wantFits: false, }, { @@ -295,16 +296,13 @@ func TestHostHasCapacityForReservation(t *testing.T) { }, }, }, - res: func() *v1alpha1.Reservation { - r := resWithSlot("res-target", "host-old", 480, 40) - return &r - }(), + res: resWithSlot("res-target", "host-old", 480, 40), wantFits: false, // 480-100 = 380 GiB remaining < 480 GiB slot (no confirmed VMs, so full slot is required) }, { name: "reservation targeting via Status.Host (not TargetHost) still blocks", hv: hvWithCapacity("host-new", 480, 40), - res: func() *v1alpha1.Reservation { r := resWithSlot("res-target", "host-old", 480, 40); return &r }(), + res: resWithSlot("res-target", "host-old", 480, 40), others: []v1alpha1.Reservation{ // TargetHost empty but Status.Host = host-new { diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index 269555e42..2ec29ff74 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -448,17 +448,19 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte var allReservations *v1alpha1.ReservationList ensureHVsAndReservations := func() error { - if allHVs != nil { + if allHVs != nil && allReservations != nil { return nil } - allHVs = &hv1.HypervisorList{} - if err := r.List(ctx, allHVs); err != nil { + hvs := &hv1.HypervisorList{} + if err := r.List(ctx, hvs); err != nil { return fmt.Errorf("failed to list hypervisors: %w", err) } - allReservations = &v1alpha1.ReservationList{} - if err := r.List(ctx, allReservations); err != nil { + res := &v1alpha1.ReservationList{} + if err := r.List(ctx, res); err != nil { return fmt.Errorf("failed to list reservations: %w", err) } + allHVs = hvs + allReservations = res return nil } @@ -591,12 +593,14 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte specChanged = true } - // Update TargetHost when the single migrated VM moved to a host with capacity. - // Setting Spec.TargetHost triggers the TargetHost→Status.Host sync path in Reconcile, - // which advances Status.Host and marks the reservation active on the new host. - // We do NOT update Status.Host here to avoid bypassing that sync path. + // Update TargetHost and Status.Host when the single migrated VM moved to a host + // with capacity. Setting Spec.TargetHost makes the new host the desired state. + // Advancing Status.Host here in the same patch cycle avoids a second reconcile + // cycle where Status.Host would lag behind TargetHost and temporarily block + // capacity accounting on the old host. if migrationTargetHost != "" { res.Spec.TargetHost = migrationTargetHost + res.Status.Host = migrationTargetHost specChanged = true } @@ -622,8 +626,11 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte // the status update. Otherwise MergeFrom(old) would see no diff // and the status patch would be a no-op. old = res.DeepCopy() - // Re-apply the status update that was overwritten by the re-fetch. + // Re-apply status updates that were overwritten by the re-fetch. res.Status.CommittedResourceReservation.Allocations = newStatusAllocations + if migrationTargetHost != "" { + res.Status.Host = migrationTargetHost + } } // Proactively remove this VM UUID from all other candidate reservations that still diff --git a/internal/scheduling/reservations/commitments/reservation_controller_test.go b/internal/scheduling/reservations/commitments/reservation_controller_test.go index 1a30be0a8..a776ac0cf 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller_test.go +++ b/internal/scheduling/reservations/commitments/reservation_controller_test.go @@ -1058,19 +1058,22 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { reservation *v1alpha1.Reservation // extra objects beyond the base reservation and old host HV extraObjects []client.Object - // expected outcomes + // expected outcomes after first reconcile pass wantTargetHost string wantStatusHost string // expected in Status.Allocations[vmUUID]; "" means absent wantSpecHasVM bool + // if true, run a second reconcile pass and assert state is stable + assertSecondPass bool }{ { name: "single VM, new host has capacity: follow the VM", extraObjects: []client.Object{ newHVWithCapacity(newHost, 960, 80, []hv1.Instance{{ID: vmUUID, Active: true}}), }, - wantTargetHost: newHost, - wantStatusHost: newHost, - wantSpecHasVM: true, + wantTargetHost: newHost, + wantStatusHost: newHost, + wantSpecHasVM: true, + assertSecondPass: true, }, { name: "single VM, new host at capacity: remove VM, slot stays on old host", @@ -1150,11 +1153,7 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { } } if !addsOldHostHV { - oldInstances := []hv1.Instance{} - if tt.wantStatusHost == oldHost { - oldInstances = []hv1.Instance{{ID: vmUUID, Active: true}} - } - objects = append(objects, newTestHypervisorCRD(oldHost, oldInstances)) + objects = append(objects, newTestHypervisorCRD(oldHost, []hv1.Instance{})) } objects = append(objects, tt.extraObjects...) @@ -1185,6 +1184,35 @@ func TestReconcileAllocations_LiveMigration(t *testing.T) { if statusHost != tt.wantStatusHost { t.Errorf("Status.Allocations[%s] = %q, want %q", vmUUID, statusHost, tt.wantStatusHost) } + + // For the migration-follow case: run a second reconcile to confirm state is + // stable and Status.Host was advanced to the new host in the first pass. + if tt.assertSecondPass { + if _, err := controller.reconcileAllocations(ctx, &updated); err != nil { + t.Fatalf("second reconcileAllocations() error = %v", err) + } + var updated2 v1alpha1.Reservation + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(res), &updated2); err != nil { + t.Fatalf("failed to get reservation after second pass: %v", err) + } + if updated2.Spec.TargetHost != tt.wantTargetHost { + t.Errorf("second pass: Spec.TargetHost = %q, want %q", updated2.Spec.TargetHost, tt.wantTargetHost) + } + if updated2.Status.Host != tt.wantTargetHost { + t.Errorf("second pass: Status.Host = %q, want %q", updated2.Status.Host, tt.wantTargetHost) + } + _, specHasVM2 := updated2.Spec.CommittedResourceReservation.Allocations[vmUUID] + if !specHasVM2 { + t.Errorf("second pass: VM unexpectedly removed from Spec.Allocations") + } + var statusHost2 string + if updated2.Status.CommittedResourceReservation != nil { + statusHost2 = updated2.Status.CommittedResourceReservation.Allocations[vmUUID] + } + if statusHost2 != tt.wantStatusHost { + t.Errorf("second pass: Status.Allocations[%s] = %q, want %q", vmUUID, statusHost2, tt.wantStatusHost) + } + } }) } } From 8ea202f0cd2c285ea37a7cd0b83eb3e1efd3d24a Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 4 Aug 2026 12:08:05 +0200 Subject: [PATCH 16/18] remove redundant comments Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller.go | 52 +++++-------------- 1 file changed, 14 insertions(+), 38 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index 2ec29ff74..2fe9b4b1e 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -397,12 +397,10 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte result := &reconcileAllocationsResult{} now := time.Now() - // Skip if no CommittedResourceReservation if res.Spec.CommittedResourceReservation == nil { return result, nil } - // Skip if no allocations to verify if len(res.Spec.CommittedResourceReservation.Allocations) == 0 { logger.V(1).Info("no allocations to verify", "reservation", res.Name) return result, nil @@ -410,7 +408,6 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte expectedHost := res.Status.Host - // Fetch the Hypervisor CRD for the expected host. var hypervisor hv1.Hypervisor hvInstanceSet := make(map[string]bool) if expectedHost != "" { @@ -418,10 +415,8 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte if client.IgnoreNotFound(err) != nil { return nil, fmt.Errorf("failed to get hypervisor %s: %w", expectedHost, err) } - // Hypervisor not found — treat all post-grace-period VMs as stale. logger.Info("hypervisor CRD not found", "host", expectedHost) } else { - // Build set of all VM UUIDs on this hypervisor for O(1) lookup. // Include both active and inactive VMs — stopped/shelved VMs still hold the slot. for _, inst := range hypervisor.Status.Instances { hvInstanceSet[inst.ID] = true @@ -430,7 +425,6 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } } - // Initialize status if res.Status.CommittedResourceReservation == nil { res.Status.CommittedResourceReservation = &v1alpha1.CommittedResourceReservationStatus{} } @@ -464,24 +458,20 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte return nil } - // Build new Status.Allocations map based on HV CRD state. newStatusAllocations := make(map[string]string) - // Track allocations to remove from Spec (stale/leaving VMs). var allocationsToRemove []string // migrationTargetHost is set when the reservation has exactly one VM, that VM - // live-migrated to a new host, and the new host has capacity. In that case - // Spec.TargetHost is updated so the reservation follows the VM. + // live-migrated to a new host, and the new host has capacity. migrationTargetHost := "" for vmUUID, allocation := range res.Spec.CommittedResourceReservation.Allocations { allocationAge := now.Sub(allocation.CreationTimestamp.Time) isInGracePeriod := allocationAge < r.Conf.AllocationGracePeriod.Duration - // Confirmed VMs (already in Status.Allocations) bypass the grace period: - // their departure from the HV CRD is authoritative and must be acted on immediately. - // Unconfirmed VMs still within the grace period may not yet appear in the HV CRD - // (still spawning), so defer verification and requeue with a short interval. + // Confirmed VMs bypass the grace period: their departure from the HV CRD is + // authoritative and must be acted on immediately. Unconfirmed VMs within the + // grace period may not yet appear in the HV CRD (still spawning). _, isConfirmed := existingStatusAllocations[vmUUID] if !isConfirmed && isInGracePeriod { result.HasAllocationsInGracePeriod = true @@ -491,7 +481,6 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte continue } - // Post-grace-period or confirmed VM: use HV CRD as authoritative source. if hvInstanceSet[vmUUID] { newStatusAllocations[vmUUID] = expectedHost logger.V(1).Info("verified VM allocation via Hypervisor CRD", @@ -500,8 +489,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte continue } - // VM not on the expected host. For unconfirmed post-grace VMs this is a clean - // stale allocation — remove it without further searching. + // Unconfirmed post-grace VM not on the expected host — stale, remove it. if !isConfirmed { allocationsToRemove = append(allocationsToRemove, vmUUID) logger.Info("removing stale allocation (VM not found on hypervisor)", @@ -514,8 +502,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } // Confirmed VM missing from expected host — could be a live migration. - // Scan all HVs to find where the VM is now. The list is fetched lazily - // and shared across any further misses in this reconcile cycle. + // Scan all HVs lazily; the list is shared across any further misses this cycle. if err := ensureHVsAndReservations(); err != nil { return nil, err } @@ -581,11 +568,9 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } } - // Patch the reservation old := res.DeepCopy() specChanged := false - // Remove stale allocations from Spec if len(allocationsToRemove) > 0 { for _, vmUUID := range allocationsToRemove { delete(res.Spec.CommittedResourceReservation.Allocations, vmUUID) @@ -593,21 +578,17 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte specChanged = true } - // Update TargetHost and Status.Host when the single migrated VM moved to a host - // with capacity. Setting Spec.TargetHost makes the new host the desired state. - // Advancing Status.Host here in the same patch cycle avoids a second reconcile - // cycle where Status.Host would lag behind TargetHost and temporarily block - // capacity accounting on the old host. + // Advance both TargetHost and Status.Host in the same patch cycle to avoid a + // transient state where Status.Host lags behind TargetHost and blocks capacity + // accounting on the old host during the next reconcile. if migrationTargetHost != "" { res.Spec.TargetHost = migrationTargetHost res.Status.Host = migrationTargetHost specChanged = true } - // Update Status.Allocations res.Status.CommittedResourceReservation.Allocations = newStatusAllocations - // Patch Spec if changed (stale allocations removed and/or TargetHost updated) if specChanged { if err := r.Patch(ctx, res, client.MergeFrom(old)); err != nil { if client.IgnoreNotFound(err) == nil { @@ -615,28 +596,24 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } return nil, fmt.Errorf("failed to patch reservation spec: %w", err) } - // Re-fetch to get the updated resource version for status patch if err := r.Get(ctx, client.ObjectKeyFromObject(res), res); err != nil { if client.IgnoreNotFound(err) == nil { return result, nil } return nil, fmt.Errorf("failed to re-fetch reservation: %w", err) } - // Capture the re-fetched state as the patch base BEFORE re-applying - // the status update. Otherwise MergeFrom(old) would see no diff - // and the status patch would be a no-op. + // Re-set old after re-fetch so MergeFrom sees the status changes as a diff, + // not a no-op. old = res.DeepCopy() - // Re-apply status updates that were overwritten by the re-fetch. res.Status.CommittedResourceReservation.Allocations = newStatusAllocations if migrationTargetHost != "" { res.Status.Host = migrationTargetHost } } - // Proactively remove this VM UUID from all other candidate reservations that still - // carry it in their Spec.Allocations. Only do this for VMs that are newly confirmed - // in this reconcile cycle (present in newStatusAllocations but absent in the snapshot - // taken before any patch) to avoid redundant work on subsequent reconciles. + // Only call cleanupCandidateReservations for VMs newly confirmed this cycle + // (present in newStatusAllocations but absent in the pre-patch snapshot) to + // avoid redundant work on subsequent reconciles. for vmUUID := range newStatusAllocations { if _, wasAlreadyConfirmed := existingStatusAllocations[vmUUID]; wasAlreadyConfirmed { continue @@ -646,7 +623,6 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } } - // Patch Status patch := client.MergeFrom(old) if err := r.Status().Patch(ctx, res, patch); err != nil { if client.IgnoreNotFound(err) == nil { From e93bb6bcce4c3cfa17454b697af6887e42f15f4f Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 4 Aug 2026 12:20:04 +0200 Subject: [PATCH 17/18] fix Signed-off-by: juliusclausnitzer --- .../commitments/reservation_controller.go | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/internal/scheduling/reservations/commitments/reservation_controller.go b/internal/scheduling/reservations/commitments/reservation_controller.go index 2fe9b4b1e..f6bf5a385 100644 --- a/internal/scheduling/reservations/commitments/reservation_controller.go +++ b/internal/scheduling/reservations/commitments/reservation_controller.go @@ -397,10 +397,12 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte result := &reconcileAllocationsResult{} now := time.Now() + // Skip if no CommittedResourceReservation if res.Spec.CommittedResourceReservation == nil { return result, nil } + // Skip if no allocations to verify if len(res.Spec.CommittedResourceReservation.Allocations) == 0 { logger.V(1).Info("no allocations to verify", "reservation", res.Name) return result, nil @@ -408,6 +410,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte expectedHost := res.Status.Host + // Fetch the Hypervisor CRD for the expected host. var hypervisor hv1.Hypervisor hvInstanceSet := make(map[string]bool) if expectedHost != "" { @@ -415,8 +418,10 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte if client.IgnoreNotFound(err) != nil { return nil, fmt.Errorf("failed to get hypervisor %s: %w", expectedHost, err) } + // Hypervisor not found — treat all post-grace-period VMs as stale. logger.Info("hypervisor CRD not found", "host", expectedHost) } else { + // Build set of all VM UUIDs on this hypervisor for O(1) lookup. // Include both active and inactive VMs — stopped/shelved VMs still hold the slot. for _, inst := range hypervisor.Status.Instances { hvInstanceSet[inst.ID] = true @@ -425,6 +430,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } } + // Initialize status if res.Status.CommittedResourceReservation == nil { res.Status.CommittedResourceReservation = &v1alpha1.CommittedResourceReservationStatus{} } @@ -458,7 +464,9 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte return nil } + // Build new Status.Allocations map based on HV CRD state. newStatusAllocations := make(map[string]string) + // Track allocations to remove from Spec (stale/leaving VMs). var allocationsToRemove []string // migrationTargetHost is set when the reservation has exactly one VM, that VM @@ -469,9 +477,10 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte allocationAge := now.Sub(allocation.CreationTimestamp.Time) isInGracePeriod := allocationAge < r.Conf.AllocationGracePeriod.Duration - // Confirmed VMs bypass the grace period: their departure from the HV CRD is - // authoritative and must be acted on immediately. Unconfirmed VMs within the - // grace period may not yet appear in the HV CRD (still spawning). + // Confirmed VMs (already in Status.Allocations) bypass the grace period: + // their departure from the HV CRD is authoritative and must be acted on immediately. + // Unconfirmed VMs still within the grace period may not yet appear in the HV CRD + // (still spawning), so defer verification and requeue with a short interval. _, isConfirmed := existingStatusAllocations[vmUUID] if !isConfirmed && isInGracePeriod { result.HasAllocationsInGracePeriod = true @@ -481,6 +490,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte continue } + // Post-grace-period or confirmed VM: use HV CRD as authoritative source. if hvInstanceSet[vmUUID] { newStatusAllocations[vmUUID] = expectedHost logger.V(1).Info("verified VM allocation via Hypervisor CRD", @@ -489,7 +499,8 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte continue } - // Unconfirmed post-grace VM not on the expected host — stale, remove it. + // VM not on the expected host. For unconfirmed post-grace VMs this is a clean + // stale allocation — remove it without further searching. if !isConfirmed { allocationsToRemove = append(allocationsToRemove, vmUUID) logger.Info("removing stale allocation (VM not found on hypervisor)", @@ -568,9 +579,11 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } } + // Patch the reservation old := res.DeepCopy() specChanged := false + // Remove stale allocations from Spec if len(allocationsToRemove) > 0 { for _, vmUUID := range allocationsToRemove { delete(res.Spec.CommittedResourceReservation.Allocations, vmUUID) @@ -587,8 +600,10 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte specChanged = true } + // Update Status.Allocations res.Status.CommittedResourceReservation.Allocations = newStatusAllocations + // Patch Spec if changed (stale allocations removed and/or TargetHost updated) if specChanged { if err := r.Patch(ctx, res, client.MergeFrom(old)); err != nil { if client.IgnoreNotFound(err) == nil { @@ -596,24 +611,28 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } return nil, fmt.Errorf("failed to patch reservation spec: %w", err) } + // Re-fetch to get the updated resource version for status patch if err := r.Get(ctx, client.ObjectKeyFromObject(res), res); err != nil { if client.IgnoreNotFound(err) == nil { return result, nil } return nil, fmt.Errorf("failed to re-fetch reservation: %w", err) } - // Re-set old after re-fetch so MergeFrom sees the status changes as a diff, - // not a no-op. + // Capture the re-fetched state as the patch base BEFORE re-applying + // the status update. Otherwise MergeFrom(old) would see no diff + // and the status patch would be a no-op. old = res.DeepCopy() + // Re-apply status updates that were overwritten by the re-fetch. res.Status.CommittedResourceReservation.Allocations = newStatusAllocations if migrationTargetHost != "" { res.Status.Host = migrationTargetHost } } - // Only call cleanupCandidateReservations for VMs newly confirmed this cycle - // (present in newStatusAllocations but absent in the pre-patch snapshot) to - // avoid redundant work on subsequent reconciles. + // Proactively remove this VM UUID from all other candidate reservations that still + // carry it in their Spec.Allocations. Only do this for VMs that are newly confirmed + // in this reconcile cycle (present in newStatusAllocations but absent in the snapshot + // taken before any patch) to avoid redundant work on subsequent reconciles. for vmUUID := range newStatusAllocations { if _, wasAlreadyConfirmed := existingStatusAllocations[vmUUID]; wasAlreadyConfirmed { continue @@ -623,6 +642,7 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte } } + // Patch Status patch := client.MergeFrom(old) if err := r.Status().Patch(ctx, res, patch); err != nil { if client.IgnoreNotFound(err) == nil { From e5c21daa5f0d6c24d2920b368802b00e5f99c7c1 Mon Sep 17 00:00:00 2001 From: juliusclausnitzer Date: Tue, 4 Aug 2026 14:58:19 +0200 Subject: [PATCH 18/18] fix Signed-off-by: juliusclausnitzer --- .../scheduling/reservations/capacity_accounting.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/scheduling/reservations/capacity_accounting.go b/internal/scheduling/reservations/capacity_accounting.go index 46cde2903..ab305d5d5 100644 --- a/internal/scheduling/reservations/capacity_accounting.go +++ b/internal/scheduling/reservations/capacity_accounting.go @@ -55,16 +55,16 @@ func HostHasCapacityForReservation(allReservations []v1alpha1.Reservation, hv hv if !targetsThisHost { continue } - for rn, block := range UnusedReservationCapacity(other, false) { - if f, ok := free[rn]; ok { + for resourceName, block := range UnusedReservationCapacity(other, false) { + if f, ok := free[resourceName]; ok { f.Sub(block) - free[rn] = f + free[resourceName] = f } } } - for rn, required := range UnusedReservationCapacity(res, false) { - remaining, ok := free[rn] + for resourceName, required := range UnusedReservationCapacity(res, false) { + remaining, ok := free[resourceName] if !ok { return false }