diff --git a/pkg/reconciler/migration/logicalclustermigration/controller.go b/pkg/reconciler/migration/logicalclustermigration/controller.go index 46f224c6471..12bcb9a466d 100644 --- a/pkg/reconciler/migration/logicalclustermigration/controller.go +++ b/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( diff --git a/pkg/reconciler/migration/logicalclustermigration/reconciler.go b/pkg/reconciler/migration/logicalclustermigration/reconciler.go index bfb6dc09b42..02151474f5c 100644 --- a/pkg/reconciler/migration/logicalclustermigration/reconciler.go +++ b/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, diff --git a/pkg/reconciler/migration/logicalclustermigration/reconciler_test.go b/pkg/reconciler/migration/logicalclustermigration/reconciler_test.go index 4bbde3ddded..c73b59c6562 100644 --- a/pkg/reconciler/migration/logicalclustermigration/reconciler_test.go +++ b/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) + }) + } +}