Add finalizer-guarded deletion for VCFM to prevent interrupted migrat…

api/v1alpha1/vmwarecloudfoundationmigration_types.go

@@ -39,6 +39,17 @@ const (
 // infrastructures.config.openshift.io/cluster).
 const SingletonName = "cluster"
 
+// Finalizer is set on the singleton VmwareCloudFoundationMigration while a
+// migration is in progress, so the object is not removed out from under an
+// interrupted migration (e.g. a test timeout or accidental delete), leaving
+// the cluster split across vCenters with nothing left to track or finish it.
+const Finalizer = "migration.openshift.io/vcfm-protection"
+
+// ForceDeleteAnnotation, when set to "true" on the singleton
+// VmwareCloudFoundationMigration, allows the object to be deleted even while
+// a migration is in progress, abandoning it deliberately.
+const ForceDeleteAnnotation = "migration.openshift.io/force-delete"
+
 // SecretReference references a secret by name and namespace.
 type SecretReference struct {
 	// Name is the secret name.

internal/controller/vmwarecloudfoundationmigration_controller.go

@@ -19,6 +19,7 @@ package controller
 import (
 	"context"
 	"fmt"
+	"strconv"
 	"strings"
 	"time"
 
@@ -38,6 +39,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	migrationv1alpha1 "github.com/openshift/vcf-migration-operator/api/v1alpha1"
@@ -71,6 +73,7 @@ var conditionOrder = []string{
 }
 
 const reasonWaitingForVSpherePods = "WaitingForVSpherePods"
+const reasonDeletionBlocked = "DeletionBlocked"
 
 // +kubebuilder:rbac:groups=migration.openshift.io,resources=vmwarecloudfoundationmigrations,verbs=get;list;watch;update;patch
 // +kubebuilder:rbac:groups=migration.openshift.io,resources=vmwarecloudfoundationmigrations/status,verbs=get;update;patch
@@ -121,9 +124,17 @@ func (r *VmwareCloudFoundationMigrationReconciler) Reconcile(ctx context.Context
 		return ctrl.Result{}, nil
 	}
 
+	result, done, err := r.handleFinalizer(ctx, migration)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+	if done {
+		return result, nil
+	}
+
 	if migration.Spec.State != migrationv1alpha1.MigrationStateRunning {
 		log.V(1).Info("migration not in Running state, skipping", "state", migration.Spec.State)
-		return ctrl.Result{}, nil
+		return result, nil
 	}
 
 	// Set start time on first reconcile in Running state.
@@ -181,6 +192,59 @@ func (r *VmwareCloudFoundationMigrationReconciler) Reconcile(ctx context.Context
 	return ctrl.Result{}, nil
 }
 
+// handleFinalizer ensures the singleton migration carries the protection
+// finalizer while not being deleted, and guards deletion while a migration is
+// in progress. It returns done=true when the caller should return immediately
+// with the given result/error; done=false means the caller should continue
+// with normal reconciliation (e.g. a Running migration keeps progressing
+// toward Ready even while the object is Terminating, so it can finish and
+// finalize its own deletion).
+func (r *VmwareCloudFoundationMigrationReconciler) handleFinalizer(ctx context.Context, migration *migrationv1alpha1.VmwareCloudFoundationMigration) (ctrl.Result, bool, error) {
+	if migration.DeletionTimestamp.IsZero() {
+		if controllerutil.AddFinalizer(migration, migrationv1alpha1.Finalizer) {
+			if err := r.Update(ctx, migration); err != nil {
+				return ctrl.Result{}, true, fmt.Errorf("adding finalizer: %w", err)
+			}
+			return ctrl.Result{}, true, nil
+		}
+		return ctrl.Result{RequeueAfter: 10 * time.Second}, false, nil
+	}
+
+	if !controllerutil.ContainsFinalizer(migration, migrationv1alpha1.Finalizer) {
+		return ctrl.Result{}, true, nil
+	}
+
+	blocked := migration.Status.StartTime != nil && !r.isConditionTrue(migration, migrationv1alpha1.ConditionReady)
+	forced := false
+	if val, ok := migration.Annotations[migrationv1alpha1.ForceDeleteAnnotation]; ok {
+		forced, _ = strconv.ParseBool(strings.TrimSpace(val))
+	}
+	if !blocked || forced {
+		controllerutil.RemoveFinalizer(migration, migrationv1alpha1.Finalizer)
+		if err := r.Update(ctx, migration); err != nil {
+			return ctrl.Result{}, true, fmt.Errorf("removing finalizer: %w", err)
+		}
+		return ctrl.Result{}, true, nil
+	}
+
+	cond := apimeta.FindStatusCondition(migration.Status.Conditions, migrationv1alpha1.ConditionReady)
+	alreadyRecorded := cond != nil &&
+		cond.Status == metav1.ConditionFalse &&
+		cond.Reason == reasonDeletionBlocked
+	if !alreadyRecorded {
+		r.setCondition(migration, migrationv1alpha1.ConditionReady, metav1.ConditionFalse, reasonDeletionBlocked,
+			fmt.Sprintf("migration is in progress; deletion is deferred until it reaches %s, or add the %q annotation to force it",
+				migrationv1alpha1.ConditionReady, migrationv1alpha1.ForceDeleteAnnotation))
+		if err := r.updateStatus(ctx, migration); err != nil {
+			return ctrl.Result{}, true, fmt.Errorf("updating deletion-blocked status: %w", err)
+		}
+		r.Recorder.Eventf(migration, "Warning", reasonDeletionBlocked,
+			"migration is in progress; deletion is deferred until it reaches %s, or add the %q annotation to force it",
+			migrationv1alpha1.ConditionReady, migrationv1alpha1.ForceDeleteAnnotation)
+	}
+	return ctrl.Result{RequeueAfter: 30 * time.Second}, false, nil
+}
+
 // ensureInfrastructurePrepared validates preflight checks and selects the
 // migration path without performing disruptive cluster changes.
 func (r *VmwareCloudFoundationMigrationReconciler) ensureInfrastructurePrepared(ctx context.Context, migration *migrationv1alpha1.VmwareCloudFoundationMigration) (ctrl.Result, error) {

internal/controller/vmwarecloudfoundationmigration_controller_test.go

@@ -19,6 +19,7 @@ package controller
 import (
 	"context"
 	"testing"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -27,6 +28,7 @@ import (
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -157,6 +159,20 @@ var _ = Describe("VmwareCloudFoundationMigration Controller", func() {
 
 			By("Cleanup the specific resource instance VmwareCloudFoundationMigration")
 			Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+
+			// The migration never started (State stayed Pending), so
+			// deletion is not blocked; one more reconcile removes the
+			// protection finalizer and lets the object actually go away.
+			controllerReconciler := &VmwareCloudFoundationMigrationReconciler{
+				Client:   k8sClient,
+				Scheme:   k8sClient.Scheme(),
+				Recorder: record.NewFakeRecorder(10),
+			}
+			_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
+				NamespacedName: typeNamespacedName,
+			})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(errors.IsNotFound(k8sClient.Get(ctx, typeNamespacedName, &migrationv1alpha1.VmwareCloudFoundationMigration{}))).To(BeTrue())
 		})
 		It("should successfully reconcile the resource", func() {
 			By("Reconciling the created resource")
@@ -255,4 +271,230 @@ var _ = Describe("VmwareCloudFoundationMigration Controller", func() {
 			Expect(resource.Status.StartTime).To(BeNil())
 		})
 	})
+
+	Context("When the singleton migration is deleted", func() {
+		const resourceName = migrationv1alpha1.SingletonName
+
+		ctx := context.Background()
+
+		typeNamespacedName := types.NamespacedName{
+			Name:      resourceName,
+			Namespace: "default",
+		}
+
+		newReconciler := func() *VmwareCloudFoundationMigrationReconciler {
+			return &VmwareCloudFoundationMigrationReconciler{
+				Client:   k8sClient,
+				Scheme:   k8sClient.Scheme(),
+				Recorder: record.NewFakeRecorder(10),
+			}
+		}
+
+		newResource := func() *migrationv1alpha1.VmwareCloudFoundationMigration {
+			return &migrationv1alpha1.VmwareCloudFoundationMigration{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      resourceName,
+					Namespace: "default",
+				},
+				Spec: migrationv1alpha1.VmwareCloudFoundationMigrationSpec{
+					State: migrationv1alpha1.MigrationStatePending,
+					TargetVCenterCredentialsSecret: migrationv1alpha1.SecretReference{
+						Name:      "target-vcenter-creds",
+						Namespace: "default",
+					},
+					FailureDomains: []configv1.VSpherePlatformFailureDomainSpec{
+						{
+							Name:   "target-fd-1",
+							Region: "target-region",
+							Zone:   "target-zone-1",
+							Server: "vcenter-target.example.com",
+							Topology: configv1.VSpherePlatformTopology{
+								Datacenter:     "TargetDC",
+								ComputeCluster: "/TargetDC/host/TargetCluster",
+								Datastore:      "/TargetDC/datastore/TargetDatastore",
+								Networks:       []string{"VM Network"},
+								ResourcePool:   "/TargetDC/host/TargetCluster/Resources",
+								Template:       "/TargetDC/vm/rhcos-template",
+								Folder:         "/TargetDC/vm/my-cluster-infra-id",
+							},
+						},
+					},
+				},
+			}
+		}
+
+		AfterEach(func() {
+			// Force any still-blocked deletion through so a failed assertion
+			// earlier in the test doesn't hang the suite. Re-fetches on every
+			// attempt so a resourceVersion conflict just causes a retry.
+			reconciler := newReconciler()
+			Eventually(func() bool {
+				resource := &migrationv1alpha1.VmwareCloudFoundationMigration{}
+				if err := k8sClient.Get(ctx, typeNamespacedName, resource); err != nil {
+					return errors.IsNotFound(err)
+				}
+
+				if resource.Annotations[migrationv1alpha1.ForceDeleteAnnotation] != "true" {
+					if resource.Annotations == nil {
+						resource.Annotations = map[string]string{}
+					}
+					resource.Annotations[migrationv1alpha1.ForceDeleteAnnotation] = "true"
+					if err := k8sClient.Update(ctx, resource); err != nil {
+						return false
+					}
+				}
+
+				if resource.DeletionTimestamp.IsZero() {
+					if err := k8sClient.Delete(ctx, resource); err != nil {
+						return false
+					}
+					if err := k8sClient.Get(ctx, typeNamespacedName, resource); err != nil {
+						return errors.IsNotFound(err)
+					}
+				}
+
+				if _, _, err := reconciler.handleFinalizer(ctx, resource); err != nil {
+					return false
+				}
+				return errors.IsNotFound(k8sClient.Get(ctx, typeNamespacedName, &migrationv1alpha1.VmwareCloudFoundationMigration{}))
+			}).WithTimeout(10 * time.Second).WithPolling(500 * time.Millisecond).Should(BeTrue())
+		})
+
+		It("adds the protection finalizer on first reconcile", func() {
+			Expect(k8sClient.Create(ctx, newResource())).To(Succeed())
+			_, err := newReconciler().Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+			Expect(err).NotTo(HaveOccurred())
+
+			resource := &migrationv1alpha1.VmwareCloudFoundationMigration{}
+			Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+			Expect(controllerutil.ContainsFinalizer(resource, migrationv1alpha1.Finalizer)).To(BeTrue())
+		})
+
+		It("deletes immediately when the migration never started", func() {
+			Expect(k8sClient.Create(ctx, newResource())).To(Succeed())
+			reconciler := newReconciler()
+			_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+			Expect(err).NotTo(HaveOccurred())
+
+			resource := &migrationv1alpha1.VmwareCloudFoundationMigration{}
+			Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+			Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+
+			_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(errors.IsNotFound(k8sClient.Get(ctx, typeNamespacedName, &migrationv1alpha1.VmwareCloudFoundationMigration{}))).To(BeTrue())
+		})
+
+		Context("deletion blocking", func() {
+			var resource *migrationv1alpha1.VmwareCloudFoundationMigration
+			var reconciler *VmwareCloudFoundationMigrationReconciler
+
+			BeforeEach(func() {
+				// Exercised directly against handleFinalizer (rather than the
+				// full Reconcile) for the deletion steps: once deletion is
+				// blocked, Reconcile deliberately falls through to keep driving
+				// the migration's state machine, which needs the real vSphere/
+				// Kubernetes clients this test doesn't wire up.
+				resource = newResource()
+				Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+				reconciler = newReconciler()
+				_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+				Expect(err).NotTo(HaveOccurred())
+
+				Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+				now := metav1.Now()
+				resource.Status.StartTime = &now
+				Expect(k8sClient.Status().Update(ctx, resource)).To(Succeed())
+
+				Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+				Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+
+				Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+			})
+
+			It("blocks deletion while running and not yet Ready", func() {
+				fakeRecorder := record.NewFakeRecorder(10)
+				reconciler.Recorder = fakeRecorder
+				result, done, err := reconciler.handleFinalizer(ctx, resource)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(done).To(BeFalse())
+				Expect(result.RequeueAfter).To(BeNumerically(">", 0))
+
+				Expect(fakeRecorder.Events).To(Receive(SatisfyAll(
+					ContainSubstring("Warning"),
+					ContainSubstring(reasonDeletionBlocked),
+				)))
+
+				blocked := &migrationv1alpha1.VmwareCloudFoundationMigration{}
+				Expect(k8sClient.Get(ctx, typeNamespacedName, blocked)).To(Succeed())
+				Expect(controllerutil.ContainsFinalizer(blocked, migrationv1alpha1.Finalizer)).To(BeTrue())
+				Expect(blocked.DeletionTimestamp.IsZero()).To(BeFalse())
+
+				cond := apimeta.FindStatusCondition(blocked.Status.Conditions, migrationv1alpha1.ConditionReady)
+				Expect(cond).NotTo(BeNil())
+				Expect(cond.Status).To(Equal(metav1.ConditionFalse))
+				Expect(cond.Reason).To(Equal(reasonDeletionBlocked))
+			})
+
+			It("does not emit a duplicate warning event on a second pass", func() {
+				// First call to establish the blocked condition.
+				fakeRecorder1 := record.NewFakeRecorder(10)
+				reconciler.Recorder = fakeRecorder1
+				_, _, err := reconciler.handleFinalizer(ctx, resource)
+				Expect(err).NotTo(HaveOccurred())
+
+				// A second call while still blocked must NOT emit another event.
+				fakeRecorder2 := record.NewFakeRecorder(10)
+				reconciler.Recorder = fakeRecorder2
+				Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+				_, done, err := reconciler.handleFinalizer(ctx, resource)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(done).To(BeFalse())
+				Expect(fakeRecorder2.Events).ToNot(Receive())
+			})
+
+			It("unblocks on force-delete annotation", func() {
+				// First call to establish the blocked condition.
+				fakeRecorder := record.NewFakeRecorder(10)
+				reconciler.Recorder = fakeRecorder
+				_, _, err := reconciler.handleFinalizer(ctx, resource)
+				Expect(err).NotTo(HaveOccurred())
+
+				// Force it through.
+				Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+				resource.Annotations = map[string]string{migrationv1alpha1.ForceDeleteAnnotation: "true"}
+				Expect(k8sClient.Update(ctx, resource)).To(Succeed())
+
+				Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+				_, done, err := reconciler.handleFinalizer(ctx, resource)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(done).To(BeTrue())
+				Expect(errors.IsNotFound(k8sClient.Get(ctx, typeNamespacedName, &migrationv1alpha1.VmwareCloudFoundationMigration{}))).To(BeTrue())
+			})
+		})
+
+		It("deletes immediately once the migration has reached Ready", func() {
+			resource := newResource()
+			Expect(k8sClient.Create(ctx, resource)).To(Succeed())
+			reconciler := newReconciler()
+			_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(k8sClient.Get(ctx, typeNamespacedName, resource)).To(Succeed())
+			now := metav1.Now()
+			resource.Status.StartTime = &now
+			apimeta.SetStatusCondition(&resource.Status.Conditions, metav1.Condition{
+				Type:    migrationv1alpha1.ConditionReady,
+				Status:  metav1.ConditionTrue,
+				Reason:  migrationv1alpha1.ReasonCompleted,
+				Message: "migration complete",
+			})
+			Expect(k8sClient.Status().Update(ctx, resource)).To(Succeed())
+
+			Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
+			_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: typeNamespacedName})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(errors.IsNotFound(k8sClient.Get(ctx, typeNamespacedName, &migrationv1alpha1.VmwareCloudFoundationMigration{}))).To(BeTrue())
+		})
+	})
 })