feat: cleanup candidate reservations when confirming vm (#871)

When a VM is confirmed onto a CommittedResource reservation, immediately
remove that VM's UUID from Spec.Allocations on all other candidate
reservations that still carry it. Previously those slots were only freed
after their grace period or the next periodic requeue, leaving transient
phantom blocks on non-selected hosts.

Author: juliusclausnitzer

internal/scheduling/reservations/commitments/committed_resource_controller_test.go

@@ -126,6 +126,17 @@ func newCRTestClient(scheme *runtime.Scheme, objects ...client.Object) client.Cl
 			}
 			return []string{cr.Spec.ProjectID}
 		}).
+		WithIndex(&v1alpha1.Reservation{}, idxReservationByAllocationVMUUID, func(obj client.Object) []string {
+			res, ok := obj.(*v1alpha1.Reservation)
+			if !ok || res.Spec.CommittedResourceReservation == nil {
+				return nil
+			}
+			uuids := make([]string, 0, len(res.Spec.CommittedResourceReservation.Allocations))
+			for vmUUID := range res.Spec.CommittedResourceReservation.Allocations {
+				uuids = append(uuids, vmUUID)
+			}
+			return uuids
+		}).
 		Build()
 }
 

internal/scheduling/reservations/commitments/field_index.go

@@ -18,15 +18,17 @@ const idxCommittedResourceByUUID = "spec.commitmentUUID"
 const idxCommittedResourceByProjectID = "spec.projectID"
 const idxReservationByCommitmentUUID = "spec.committedResourceReservation.commitmentUUID"
 const idxProjectQuotaByProjectID = "spec.projectID"
+const idxReservationByAllocationVMUUID = "spec.committedResourceReservation.allocations"
 
 // once guards ensure each field index is registered exactly once.
 // Both CommittedResourceController and UsageReconciler call indexCommittedResourceByUUID;
 // the underlying cache returns "indexer conflict" on double registration.
 var (
-	onceIndexCRByUUID          sync.Once
-	onceIndexCRByProjectID     sync.Once
-	onceIndexReservationByUUID sync.Once
-	onceIndexPQByProjectID     sync.Once
+	onceIndexCRByUUID                    sync.Once
+	onceIndexCRByProjectID               sync.Once
+	onceIndexReservationByUUID           sync.Once
+	onceIndexPQByProjectID               sync.Once
+	onceIndexReservationByAllocationVMID sync.Once
 )
 
 // indexCommittedResourceByUUID registers the index used by UsageReconciler to look up
@@ -128,3 +130,34 @@ func indexProjectQuotaByProjectID(ctx context.Context, mcl *multicluster.Client)
 	})
 	return err
 }
+
+// indexReservationByAllocationVMUUID registers an index over all VM UUIDs present in
+// Spec.CommittedResourceReservation.Allocations. This allows the reservation controller
+// to efficiently find all other Reservation CRDs carrying a specific VM UUID without
+// scanning every reservation in the cluster.
+func indexReservationByAllocationVMUUID(ctx context.Context, mcl *multicluster.Client) (err error) {
+	onceIndexReservationByAllocationVMID.Do(func() {
+		log := logf.FromContext(ctx)
+		err = mcl.IndexField(ctx,
+			&v1alpha1.Reservation{},
+			&v1alpha1.ReservationList{},
+			idxReservationByAllocationVMUUID,
+			func(obj client.Object) []string {
+				res, ok := obj.(*v1alpha1.Reservation)
+				if !ok {
+					log.Error(errors.New("unexpected type"), "expected Reservation", "object", obj)
+					return nil
+				}
+				if res.Spec.CommittedResourceReservation == nil {
+					return nil
+				}
+				uuids := make([]string, 0, len(res.Spec.CommittedResourceReservation.Allocations))
+				for vmUUID := range res.Spec.CommittedResourceReservation.Allocations {
+					uuids = append(uuids, vmUUID)
+				}
+				return uuids
+			},
+		)
+	})
+	return err
+}

internal/scheduling/reservations/commitments/integration_test.go

@@ -359,6 +359,17 @@ func newIntgEnv(t *testing.T, initialObjects []client.Object, schedulerFn http.H
 			}
 			return []string{cr.Spec.CommitmentUUID}
 		}).
+		WithIndex(&v1alpha1.Reservation{}, idxReservationByAllocationVMUUID, func(obj client.Object) []string {
+			res, ok := obj.(*v1alpha1.Reservation)
+			if !ok || res.Spec.CommittedResourceReservation == nil {
+				return nil
+			}
+			uuids := make([]string, 0, len(res.Spec.CommittedResourceReservation.Allocations))
+			for vmUUID := range res.Spec.CommittedResourceReservation.Allocations {
+				uuids = append(uuids, vmUUID)
+			}
+			return uuids
+		}).
 		Build()
 
 	schedulerSrv := httptest.NewServer(schedulerFn)

internal/scheduling/reservations/commitments/reservation_controller.go

@@ -395,6 +395,13 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte
 		res.Status.CommittedResourceReservation = &v1alpha1.CommittedResourceReservationStatus{}
 	}
 
+	// Snapshot existing Status.Allocations before we overwrite it so we can detect
+	// which VM UUIDs are newly confirmed after the patch.
+	existingStatusAllocations := make(map[string]string, len(res.Status.CommittedResourceReservation.Allocations))
+	for k, v := range res.Status.CommittedResourceReservation.Allocations {
+		existingStatusAllocations[k] = v
+	}
+
 	// Build new Status.Allocations map based on HV CRD state.
 	newStatusAllocations := make(map[string]string)
 	// Track allocations to remove from Spec (stale/leaving VMs).
@@ -469,6 +476,19 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte
 		res.Status.CommittedResourceReservation.Allocations = newStatusAllocations
 	}
 
+	// 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
+		}
+		if err := r.cleanupCandidateReservations(ctx, res.Name, vmUUID); err != nil {
+			return nil, fmt.Errorf("failed to cleanup candidate reservations for vm %s: %w", vmUUID, err)
+		}
+	}
+
 	// Patch Status
 	patch := client.MergeFrom(old)
 	if err := r.Status().Patch(ctx, res, patch); err != nil {
@@ -487,6 +507,41 @@ func (r *CommitmentReservationController) reconcileAllocations(ctx context.Conte
 	return result, nil
 }
 
+// cleanupCandidateReservations removes vmUUID from Spec.Allocations of all Reservation CRDs
+// other than the one that just confirmed the VM. This is called once per newly confirmed VM
+// so that candidate slots on non-selected hosts are freed immediately rather than waiting
+// for those reservations' own grace period or periodic requeue.
+func (r *CommitmentReservationController) cleanupCandidateReservations(ctx context.Context, confirmedReservationName, vmUUID string) error {
+	logger := LoggerFromContext(ctx).WithValues("component", "controller", "vm", vmUUID)
+
+	var candidates v1alpha1.ReservationList
+	if err := r.List(ctx, &candidates, client.MatchingFields{idxReservationByAllocationVMUUID: vmUUID}); err != nil {
+		return fmt.Errorf("failed to list candidate reservations: %w", err)
+	}
+
+	for i := range candidates.Items {
+		candidate := &candidates.Items[i]
+		if candidate.Name == confirmedReservationName {
+			continue
+		}
+		if candidate.Spec.CommittedResourceReservation == nil {
+			continue
+		}
+		if _, ok := candidate.Spec.CommittedResourceReservation.Allocations[vmUUID]; !ok {
+			continue
+		}
+		old := candidate.DeepCopy()
+		delete(candidate.Spec.CommittedResourceReservation.Allocations, vmUUID)
+		if err := r.Patch(ctx, candidate, client.MergeFrom(old)); err != nil {
+			if client.IgnoreNotFound(err) != nil {
+				return fmt.Errorf("failed to patch candidate reservation %s: %w", candidate.Name, err)
+			}
+		}
+		logger.Info("removed vm from candidate reservation", "reservation", candidate.Name, "host", candidate.Status.Host)
+	}
+	return nil
+}
+
 // getPipelineForFlavorGroup returns the pipeline name for a given flavor group.
 func (r *CommitmentReservationController) getPipelineForFlavorGroup(flavorGroupName string, logger logr.Logger) string {
 	// Try exact match first (e.g., "2152" -> "kvm-cr-hana")
@@ -581,6 +636,10 @@ func (r *CommitmentReservationController) SetupWithManager(mgr ctrl.Manager, mcl
 		return err
 	}
 
+	if err := indexReservationByAllocationVMUUID(context.Background(), mcl); err != nil {
+		return fmt.Errorf("failed to set up reservation allocation VM UUID index: %w", err)
+	}
+
 	// Use WatchesMulticluster to watch Reservations across all configured clusters
 	// (home + remotes). This is required because Reservation CRDs may be stored
 	// in remote clusters, not just the home cluster. Without this, the controller