Add finalizer on LogicalClusterMigration while migration is in progress

pkg/reconciler/migration/logicalclustermigration/controller.go

@@ -66,6 +66,11 @@ const (
 	// indicate it is currently being migrated. The value is the cluster path
 	// of the LogicalClusterMigration object that triggered the migration.
 	MigratingAnnotationKey = "internal.kcp.io/migrating"
+
+	// MigrationFinalizer is set on the LogicalClusterMigration by the origin
+	// shard to prevent its deletion while the migration is in progress. It
+	// is removed once the migration reaches a terminal phase.
+	MigrationFinalizer = "internal.kcp.io/logicalclustermigration"
 )
 
 func NewController(

pkg/reconciler/migration/logicalclustermigration/reconciler.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"slices"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -72,11 +73,27 @@ func (c *Controller) reconcile(ctx context.Context, migration *migrationv1alpha1
 	// Route through the process. The overview of the process id
 	// described in the doc.go, details are in the methods.
 	switch migration.Status.Phase {
-	case migrationv1alpha1.LogicalClusterMigrationPhaseCompleted, migrationv1alpha1.LogicalClusterMigrationPhaseFailed:
+	case migrationv1alpha1.LogicalClusterMigrationPhaseCompleted:
+		// The migration is done, the destination releases the finalizer.
+		if isDestination {
+			removeFinalizer(migration)
+		}
+		return false, nil
+
+	case migrationv1alpha1.LogicalClusterMigrationPhaseFailed:
+		// A failed migration may never have reached the destination, so
+		// whichever shard sees it first releases the finalizer.
+		removeFinalizer(migration)
 		return false, nil
 
 	case "":
 		if isOrigin {
+			// Add the finalizer before starting so the migration cannot be
+			// deleted while it is in progress. Metadata and status cannot
+			// be committed together, so requeue to set the phase afterwards.
+			if addFinalizer(migration) {
+				return true, nil
+			}
 			migration.Status.Phase = migrationv1alpha1.LogicalClusterMigrationPhasePreparing
 			return true, nil
 		}
@@ -402,6 +419,28 @@ func (c *Controller) reconcileAborting(ctx context.Context, migration *migration
 	return false, nil
 }
 
+// addFinalizer adds MigrationFinalizer to the migration and reports whether
+// the migration was changed.
+func addFinalizer(migration *migrationv1alpha1.LogicalClusterMigration) bool {
+	if slices.Contains(migration.Finalizers, MigrationFinalizer) {
+		return false
+	}
+	migration.Finalizers = append(migration.Finalizers, MigrationFinalizer)
+	return true
+}
+
+// removeFinalizer removes MigrationFinalizer from the migration and reports
+// whether the migration was changed.
+func removeFinalizer(migration *migrationv1alpha1.LogicalClusterMigration) bool {
+	if !slices.Contains(migration.Finalizers, MigrationFinalizer) {
+		return false
+	}
+	migration.Finalizers = slices.DeleteFunc(migration.Finalizers, func(f string) bool {
+		return f == MigrationFinalizer
+	})
+	return true
+}
+
 // originOwnsLC returns true for phases in which the origin shard still holds
 // the LogicalCluster's data, so a missing or deletion-marked LC is a genuine
 // deletion signal. After OriginCleanup the origin deletes its copy by design,

pkg/reconciler/migration/logicalclustermigration/reconciler_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package logicalclustermigration
 
 import (
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -117,3 +118,66 @@ func TestApplyDumpPageResult_resumesAfterSimulatedRestart(t *testing.T) {
 	require.Empty(t, restarted.Status.DumpContinue)
 	require.Equal(t, migrationv1alpha1.LogicalClusterMigrationPhaseOriginCleanup, restarted.Status.Phase)
 }
+
+func TestAddFinalizer(t *testing.T) {
+	t.Parallel()
+
+	migration := &migrationv1alpha1.LogicalClusterMigration{}
+	migration.Finalizers = []string{"other"}
+
+	require.True(t, addFinalizer(migration))
+	require.Equal(t, []string{"other", MigrationFinalizer}, migration.Finalizers)
+
+	require.False(t, addFinalizer(migration), "adding the finalizer twice must be a noop")
+	require.Equal(t, []string{"other", MigrationFinalizer}, migration.Finalizers)
+}
+
+func TestReconcile_removesFinalizerInTerminalPhases(t *testing.T) {
+	t.Parallel()
+
+	tests := map[string]struct {
+		shard              string
+		phase              migrationv1alpha1.LogicalClusterMigrationPhaseType
+		expectedFinalizers []string
+	}{
+		"completed on destination removes finalizer": {
+			shard:              "destination",
+			phase:              migrationv1alpha1.LogicalClusterMigrationPhaseCompleted,
+			expectedFinalizers: []string{"other"},
+		},
+		"completed on origin keeps finalizer": {
+			shard:              "origin",
+			phase:              migrationv1alpha1.LogicalClusterMigrationPhaseCompleted,
+			expectedFinalizers: []string{"other", MigrationFinalizer},
+		},
+		"failed on origin removes finalizer": {
+			shard:              "origin",
+			phase:              migrationv1alpha1.LogicalClusterMigrationPhaseFailed,
+			expectedFinalizers: []string{"other"},
+		},
+		"failed on destination removes finalizer": {
+			shard:              "destination",
+			phase:              migrationv1alpha1.LogicalClusterMigrationPhaseFailed,
+			expectedFinalizers: []string{"other"},
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			migration := &migrationv1alpha1.LogicalClusterMigration{}
+			migration.Finalizers = []string{"other", MigrationFinalizer}
+			migration.Spec.DestinationShard = "destination"
+			migration.Status.OriginShard = "origin"
+			migration.Status.Phase = tc.phase
+
+			c := &Controller{shardName: tc.shard}
+			requeue, err := c.reconcile(context.Background(), migration)
+
+			require.NoError(t, err)
+			require.False(t, requeue)
+			require.Equal(t, tc.expectedFinalizers, migration.Finalizers)
+		})
+	}
+}