diff --git a/api/v1alpha1/committed_resource_change_types.go b/api/v1alpha1/committed_resource_change_types.go index 29fb90e13..6ca6bedf0 100644 --- a/api/v1alpha1/committed_resource_change_types.go +++ b/api/v1alpha1/committed_resource_change_types.go @@ -58,6 +58,8 @@ const ( ChangeRequestReasonAccepted = "Accepted" ChangeRequestReasonRejected = "Rejected" ChangeRequestReasonSuperseded = "Superseded" + // ChangeReasonRollbackFailed is the Finished reason when rollback exhausted its timeout. + ChangeReasonRollbackFailed = "RollbackFailed" ) // CommittedResourceChangeSpec defines the desired change for a CommittedResourceChange. @@ -110,6 +112,12 @@ type CommittedResourceChangeStatus struct { // +kubebuilder:validation:Optional UpsertedCommitments []corev1.ObjectReference `json:"upsertedCommitments,omitempty"` + // RollbackStartedAt is the time the CRC entered RollingBack or DryRunCleanup. + // Used to measure RollbackTimeout from the correct starting point rather than relying on + // the Progressing condition's LastTransitionTime, which is only updated on Status changes. + // +kubebuilder:validation:Optional + RollbackStartedAt *metav1.Time `json:"rollbackStartedAt,omitempty"` + // Conditions holds Progressing and Finished conditions. // +kubebuilder:validation:Optional Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 5ba04a7e4..30956d1ab 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -231,6 +231,10 @@ func (in *CommittedResourceChangeStatus) DeepCopyInto(out *CommittedResourceChan *out = make([]v1.ObjectReference, len(*in)) copy(*out, *in) } + if in.RollbackStartedAt != nil { + in, out := &in.RollbackStartedAt, &out.RollbackStartedAt + *out = (*in).DeepCopy() + } if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions *out = make([]metav1.Condition, len(*in)) diff --git a/helm/library/cortex/files/crds/cortex.cloud_committedresourcechanges.yaml b/helm/library/cortex/files/crds/cortex.cloud_committedresourcechanges.yaml index ecf607f4c..410278265 100644 --- a/helm/library/cortex/files/crds/cortex.cloud_committedresourcechanges.yaml +++ b/helm/library/cortex/files/crds/cortex.cloud_committedresourcechanges.yaml @@ -262,6 +262,13 @@ spec: - message: phase cannot transition out of a terminal state rule: oldSelf == '' || !['Accepted','Rejected','Superseded'].exists(t, t == oldSelf) || self == oldSelf + rollbackStartedAt: + description: |- + RollbackStartedAt is the time the CRC entered RollingBack or DryRunCleanup. + Used to measure RollbackTimeout from the correct starting point rather than relying on + the Progressing condition's LastTransitionTime, which is only updated on Status changes. + format: date-time + type: string snapshotBefore: additionalProperties: description: CommittedResourceSpec defines the desired state of diff --git a/internal/scheduling/reservations/commitments/changes/reconciler.go b/internal/scheduling/reservations/commitments/changes/reconciler.go index ec2be9b82..cf278577c 100644 --- a/internal/scheduling/reservations/commitments/changes/reconciler.go +++ b/internal/scheduling/reservations/commitments/changes/reconciler.go @@ -35,6 +35,10 @@ type CommittedResourceChangeReconciler struct { // AwaitingSchedulingTimeout is how long to wait in AwaitingScheduling before rolling back. // Zero (default) means no timeout. AwaitingSchedulingTimeout time.Duration + // RollbackTimeout is how long to attempt snapshot restore in RollingBack/DryRunCleanup + // before giving up and terminating with RollbackFailed. + // Zero (default) means no timeout. + RollbackTimeout time.Duration } func (r *CommittedResourceChangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -227,17 +231,114 @@ func (r *CommittedResourceChangeReconciler) reconcileAwaitingScheduling(ctx cont // On restore complete: advances to Rejected. // On permanent cleanup failure: advances to Rejected with reason RollbackFailed. func (r *CommittedResourceChangeReconciler) reconcileRollingBack(ctx context.Context, logger logr.Logger, tx *v1alpha1.CommittedResourceChange) (ctrl.Result, error) { - // TODO (#780): for each entry in SnapshotBefore, converge CR spec back to snapshot value - // using SSA with fieldManager=txFieldManager. - // nil snapshot entry → CR did not exist before → delete it. - // Gate terminal transition on each CR confirming its spec reverted (check observedGeneration). - return ctrl.Result{}, nil + allSettled, err := r.restoreFromSnapshot(ctx, logger, tx) + if err != nil { + return ctrl.Result{}, err + } + if !allSettled { + if r.RollbackTimeout > 0 && tx.Status.RollbackStartedAt != nil && time.Since(tx.Status.RollbackStartedAt.Time) > r.RollbackTimeout { + logger.Info("rollback timed out, terminalizing", "az", tx.Spec.AvailabilityZone, "correlationID", tx.Spec.CorrelationID, "timeout", r.RollbackTimeout) + return r.terminateWithRollbackFailed(ctx, tx, v1alpha1.ChangePhaseRejected, "timed out restoring pre-change state") + } + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + return r.advanceTo(ctx, tx, v1alpha1.ChangePhaseRejected, nil) } // reconcileDryRunCleanup restores pre-change state after a dry-run would-accept outcome. // Re-uses snapshot rollback logic; terminal phase is Accepted (not Rejected). func (r *CommittedResourceChangeReconciler) reconcileDryRunCleanup(ctx context.Context, logger logr.Logger, tx *v1alpha1.CommittedResourceChange) (ctrl.Result, error) { - // TODO (#780): same convergence as reconcileRollingBack; terminal → advanceTo(Accepted, nil). + allSettled, err := r.restoreFromSnapshot(ctx, logger, tx) + if err != nil { + return ctrl.Result{}, err + } + if !allSettled { + if r.RollbackTimeout > 0 && tx.Status.RollbackStartedAt != nil && time.Since(tx.Status.RollbackStartedAt.Time) > r.RollbackTimeout { + logger.Info("dry-run cleanup timed out, terminalizing", "az", tx.Spec.AvailabilityZone, "correlationID", tx.Spec.CorrelationID, "timeout", r.RollbackTimeout) + return r.terminateWithRollbackFailed(ctx, tx, v1alpha1.ChangePhaseAccepted, "timed out restoring pre-change state after dry run") + } + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + return r.advanceTo(ctx, tx, v1alpha1.ChangePhaseAccepted, nil) +} + +// restoreFromSnapshot converges CommittedResources back to their pre-change state recorded in +// tx.Status.SnapshotBefore. It is idempotent and safe to call on every requeue. +// +// Pass 1 (write): for each snapshot entry, delete the CR (nil entry = CR was new) or restore +// its spec via CreateOrUpdate (non-nil entry = CR existed before). +// Pass 2 (settle): for each non-nil entry, verify the CR controller has processed the restored +// spec by checking cond.ObservedGeneration == cr.Generation on the Ready condition. +// +// Returns (true, nil) when all non-nil entries are settled. +// Returns (false, nil) when still waiting for settlement. +// Returns (false, err) on API errors that should cause a requeue. +func (r *CommittedResourceChangeReconciler) restoreFromSnapshot(ctx context.Context, logger logr.Logger, tx *v1alpha1.CommittedResourceChange) (bool, error) { + for uuid, snapSpec := range tx.Status.SnapshotBefore { + name := commitmentName(uuid) + if snapSpec == nil { + toDelete := &v1alpha1.CommittedResource{ObjectMeta: metav1.ObjectMeta{Name: name}} + if err := r.Delete(ctx, toDelete); client.IgnoreNotFound(err) != nil { + logger.Error(err, "rollback: failed to delete CR", "name", name, "az", tx.Spec.AvailabilityZone, "correlationID", tx.Spec.CorrelationID) + r.markError(ctx, tx, fmt.Sprintf("rollback: failed to delete %s: %v", name, err)) + return false, err + } + continue + } + cr := &v1alpha1.CommittedResource{ObjectMeta: metav1.ObjectMeta{Name: name}} + if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, cr, func() error { + cr.Spec = *snapSpec + return nil + }); err != nil { + logger.Error(err, "rollback: failed to restore CR spec", "name", name, "az", tx.Spec.AvailabilityZone, "correlationID", tx.Spec.CorrelationID) + r.markError(ctx, tx, fmt.Sprintf("rollback: failed to restore %s: %v", name, err)) + return false, err + } + } + + for uuid, snapSpec := range tx.Status.SnapshotBefore { + if snapSpec == nil { + continue + } + name := commitmentName(uuid) + var cr v1alpha1.CommittedResource + if err := r.Get(ctx, client.ObjectKey{Name: name}, &cr); err != nil { + if client.IgnoreNotFound(err) == nil { + return false, nil + } + return false, err + } + cond := meta.FindStatusCondition(cr.Status.Conditions, v1alpha1.CommittedResourceConditionReady) + if cond == nil || cond.ObservedGeneration != cr.Generation { + return false, nil + } + } + return true, nil +} + +// terminateWithRollbackFailed stamps Finished=True/RollbackFailed and Progressing=False/RetryExhausted. +// phase controls the terminal FSM phase: Rejected for a failed rollback, Accepted for a DryRunCleanup +// timeout (the scheduler accepted the change; only the post-probe cleanup stalled). +func (r *CommittedResourceChangeReconciler) terminateWithRollbackFailed(ctx context.Context, tx *v1alpha1.CommittedResourceChange, phase v1alpha1.ChangePhase, msg string) (ctrl.Result, error) { + tx.Status.Phase = phase + tx.Status.Message = msg + meta.SetStatusCondition(&tx.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ChangeConditionFinished, + Status: metav1.ConditionTrue, + Reason: v1alpha1.ChangeReasonRollbackFailed, + Message: msg, + ObservedGeneration: tx.Generation, + }) + meta.SetStatusCondition(&tx.Status.Conditions, metav1.Condition{ + Type: v1alpha1.ChangeConditionProgressing, + Status: metav1.ConditionFalse, + Reason: v1alpha1.ChangeReasonRetryExhausted, + Message: msg, + ObservedGeneration: tx.Generation, + }) + if err := r.Status().Update(ctx, tx); err != nil { + return ctrl.Result{}, err + } return ctrl.Result{}, nil } @@ -259,6 +360,10 @@ func (r *CommittedResourceChangeReconciler) advanceTo( if mutate != nil { mutate(&tx.Status) } + if (phase == v1alpha1.ChangePhaseRollingBack || phase == v1alpha1.ChangePhaseDryRunCleanup) && tx.Status.RollbackStartedAt == nil { + now := metav1.Now() + tx.Status.RollbackStartedAt = &now + } progressingStatus, progressingReason := metav1.ConditionTrue, "Advancing" if tx.IsTerminal() { progressingStatus, progressingReason = metav1.ConditionFalse, string(phase) diff --git a/internal/scheduling/reservations/commitments/changes/reconciler_test.go b/internal/scheduling/reservations/commitments/changes/reconciler_test.go index cfbdcebc8..efbf96d71 100644 --- a/internal/scheduling/reservations/commitments/changes/reconciler_test.go +++ b/internal/scheduling/reservations/commitments/changes/reconciler_test.go @@ -158,6 +158,18 @@ func backdateProgressingCondition(t *testing.T, c client.Client, txName string, } } +// backdateRollbackStartedAt sets RollbackStartedAt to simulate a rollback that has been running +// for the given duration. Used to trigger the RollbackTimeout check in tests. +func backdateRollbackStartedAt(t *testing.T, c client.Client, txName string, age time.Duration) { + t.Helper() + tx := getTx(t, c, txName) + past := metav1.NewTime(time.Now().Add(-age)) + tx.Status.RollbackStartedAt = &past + if err := c.Status().Update(testCtx, tx); err != nil { + t.Fatalf("backdate RollbackStartedAt on %q: %v", txName, err) + } +} + // assertCondition verifies a condition in the given list by type, status, and optional reason. // Pass an empty wantReason to skip the reason check. func assertCondition(t *testing.T, conditions []metav1.Condition, condType string, wantStatus metav1.ConditionStatus, wantReason string) { @@ -1080,3 +1092,435 @@ func TestCRTReconciler_Terminal(t *testing.T) { }) } } + +// ============================================================================ +// TestCRTReconciler_RollingBack +// ============================================================================ + +// driveToRollingBack drives a CRC to RollingBack by having a CR reject placement. +func driveToRollingBack(t *testing.T, r *CommittedResourceChangeReconciler, c client.Client, txName, crName string) { + t.Helper() + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseAwaitingScheduling) + setCRReady(t, c, crName, metav1.ConditionFalse, v1alpha1.CommittedResourceReasonRejected, "no capacity") + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseRollingBack) +} + +func TestCRTReconciler_RollingBack(t *testing.T) { + tests := []struct { + name string + preObjects []client.Object + txSpec v1alpha1.CommittedResourceChangeSpec + reconcilerOpt func(r *CommittedResourceChangeReconciler) + afterRollingBack func(t *testing.T, c client.Client, txName string) + wantPhase v1alpha1.ChangePhase + wantFinishedReason string + wantProgressingStatus metav1.ConditionStatus + wantProgressingReason string + // crName is derived as commitment- from txSpec.NewState (single-entry tests). + // For multi-CR tests the check is done inline. + }{ + { + name: "nil snapshot: new CR is deleted and rollback completes", + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-rb1": minimalCRSpec("az-1")}, + }, + // SnapshotBefore[uuid-rb1] = nil (CR did not exist before). + // After rollback, the CR must be deleted; no settlement gate. + wantPhase: v1alpha1.ChangePhaseRejected, + wantFinishedReason: string(v1alpha1.ChangePhaseRejected), + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: string(v1alpha1.ChangePhaseRejected), + }, + { + name: "non-nil snapshot: existing CR spec is restored and rollback completes", + preObjects: []client.Object{ + &v1alpha1.CommittedResource{ + ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-rb2"}, + Spec: *minimalCRSpec("az-old"), + }, + }, + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-rb2": minimalCRSpec("az-new")}, + }, + // SnapshotBefore[uuid-rb2] = minimalCRSpec("az-old"). + // After rollback, commitment-uuid-rb2 must have spec az-old. + // Simulate CR controller settling the restored spec. + afterRollingBack: func(t *testing.T, c client.Client, _ string) { + setCRReady(t, c, "commitment-uuid-rb2", metav1.ConditionTrue, v1alpha1.CommittedResourceReasonAccepted, "") + }, + wantPhase: v1alpha1.ChangePhaseRejected, + wantFinishedReason: string(v1alpha1.ChangePhaseRejected), + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: string(v1alpha1.ChangePhaseRejected), + }, + { + name: "CRs not yet settled: stays in RollingBack", + preObjects: []client.Object{ + &v1alpha1.CommittedResource{ + ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-rb3"}, + Spec: *minimalCRSpec("az-old"), + }, + }, + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-rb3": minimalCRSpec("az-new")}, + }, + // The fake client does not increment Generation on spec updates, so we must explicitly + // stale the condition to simulate "CR controller has not yet processed the restored spec". + afterRollingBack: func(t *testing.T, c client.Client, _ string) { + setCRReadyStaleGen(t, c, "commitment-uuid-rb3") + }, + wantPhase: v1alpha1.ChangePhaseRollingBack, + wantProgressingStatus: metav1.ConditionTrue, + wantProgressingReason: "Advancing", + }, + { + name: "rollback timeout: terminates with RollbackFailed", + preObjects: []client.Object{ + &v1alpha1.CommittedResource{ + ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-rb4"}, + Spec: *minimalCRSpec("az-old"), + }, + }, + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-rb4": minimalCRSpec("az-new")}, + }, + reconcilerOpt: func(r *CommittedResourceChangeReconciler) { + r.RollbackTimeout = 50 * time.Millisecond + }, + // Stale the CR condition AND backdate RollbackStartedAt to trigger the timeout. + afterRollingBack: func(t *testing.T, c client.Client, txName string) { + setCRReadyStaleGen(t, c, "commitment-uuid-rb4") + backdateRollbackStartedAt(t, c, txName, 1*time.Second) + }, + wantPhase: v1alpha1.ChangePhaseRejected, + wantFinishedReason: v1alpha1.ChangeReasonRollbackFailed, + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: v1alpha1.ChangeReasonRetryExhausted, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := testScheme(t) + txName := "tx-rb-" + tt.name[:5] + tx := &v1alpha1.CommittedResourceChange{ + ObjectMeta: metav1.ObjectMeta{Name: txName}, + Spec: tt.txSpec, + } + objs := append([]client.Object{tx}, tt.preObjects...) + c := testClient(t, scheme, objs...) + r := testReconciler(c, scheme) + if tt.reconcilerOpt != nil { + tt.reconcilerOpt(r) + } + + // Pick the first UUID for the rejection trigger (single-CR tests). + var crName string + for uuid := range tt.txSpec.NewState { + crName = commitmentName(uuid) + break + } + + driveToRollingBack(t, r, c, txName, crName) + + if tt.afterRollingBack != nil { + tt.afterRollingBack(t, c, txName) + } + + doReconcile(t, r, txName) + + tx = getTx(t, c, txName) + if tx.Status.Phase != tt.wantPhase { + t.Errorf("phase: want %q, got %q", tt.wantPhase, tx.Status.Phase) + } + if tt.wantFinishedReason != "" { + assertCondition(t, tx.Status.Conditions, v1alpha1.ChangeConditionFinished, metav1.ConditionTrue, tt.wantFinishedReason) + } + if tt.wantProgressingStatus != "" { + assertCondition(t, tx.Status.Conditions, v1alpha1.ChangeConditionProgressing, tt.wantProgressingStatus, tt.wantProgressingReason) + } + }) + } +} + +// TestCRTReconciler_RollingBack_MultiCR verifies that all non-nil SnapshotBefore entries must +// have settled (observedGeneration == generation) before the CRC advances to Rejected. +func TestCRTReconciler_RollingBack_MultiCR(t *testing.T) { + const txName = "tx-rb-multi" + scheme := testScheme(t) + tx := &v1alpha1.CommittedResourceChange{ + ObjectMeta: metav1.ObjectMeta{Name: txName}, + Spec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{ + "uuid-m1": minimalCRSpec("az-new"), + "uuid-m2": minimalCRSpec("az-new"), + }, + }, + } + // Both CRs pre-exist so their snapshots are non-nil; rollback must restore both. + preObjects := []client.Object{ + &v1alpha1.CommittedResource{ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-m1"}, Spec: *minimalCRSpec("az-old")}, + &v1alpha1.CommittedResource{ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-m2"}, Spec: *minimalCRSpec("az-old")}, + } + objs := append([]client.Object{tx}, preObjects...) + c := testClient(t, scheme, objs...) + r := testReconciler(c, scheme) + + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseAwaitingScheduling) + setCRReady(t, c, "commitment-uuid-m1", metav1.ConditionFalse, v1alpha1.CommittedResourceReasonRejected, "no capacity") + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseRollingBack) + + // Stale both CRs to simulate the CR controller not having processed the restored spec yet. + // (The fake client does not increment Generation on spec writes.) + setCRReadyStaleGen(t, c, "commitment-uuid-m1") + setCRReadyStaleGen(t, c, "commitment-uuid-m2") + + // Neither CR settled — must stay in RollingBack. + doReconcile(t, r, txName) + if got := getTx(t, c, txName).Status.Phase; got != v1alpha1.ChangePhaseRollingBack { + t.Fatalf("after first rollback reconcile: want RollingBack, got %q", got) + } + + // Settle only the first CR — still waiting on the second. + setCRReady(t, c, "commitment-uuid-m1", metav1.ConditionTrue, v1alpha1.CommittedResourceReasonAccepted, "") + doReconcile(t, r, txName) + if got := getTx(t, c, txName).Status.Phase; got != v1alpha1.ChangePhaseRollingBack { + t.Fatalf("after one CR settled: want RollingBack, got %q", got) + } + + // Settle the second CR — now all restored. + setCRReady(t, c, "commitment-uuid-m2", metav1.ConditionTrue, v1alpha1.CommittedResourceReasonAccepted, "") + doReconcile(t, r, txName) + if got := getTx(t, c, txName).Status.Phase; got != v1alpha1.ChangePhaseRejected { + t.Errorf("after all CRs settled: want Rejected, got %q", got) + } +} + +// TestCRTReconciler_RollingBack_Idempotent verifies that re-running RollingBack multiple +// times after restoration is safe and produces a stable Rejected terminal state. +func TestCRTReconciler_RollingBack_Idempotent(t *testing.T) { + const txName = "tx-rb-idem" + scheme := testScheme(t) + tx := &v1alpha1.CommittedResourceChange{ + ObjectMeta: metav1.ObjectMeta{Name: txName}, + Spec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-idem-rb": minimalCRSpec("az-1")}, + }, + } + c := testClient(t, scheme, tx) + r := testReconciler(c, scheme) + + // nil snapshot entry — CR was new, rollback deletes it. No settlement gate needed. + driveToRollingBack(t, r, c, txName, "commitment-uuid-idem-rb") + + reconcileN(t, r, txName, 3) + + if got := getTx(t, c, txName).Status.Phase; got != v1alpha1.ChangePhaseRejected { + t.Errorf("phase: want Rejected, got %q", got) + } +} + +// ============================================================================ +// TestCRTReconciler_DryRunCleanup +// ============================================================================ + +// driveToDryRunCleanup drives a DryRun CRC to DryRunCleanup. +func driveToDryRunCleanup(t *testing.T, r *CommittedResourceChangeReconciler, c client.Client, txName, crName string) { + t.Helper() + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseAwaitingScheduling) + setCRReady(t, c, crName, metav1.ConditionTrue, v1alpha1.CommittedResourceReasonAccepted, "") + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseDryRunCleanup) +} + +func TestCRTReconciler_DryRunCleanup(t *testing.T) { + tests := []struct { + name string + preObjects []client.Object + txSpec v1alpha1.CommittedResourceChangeSpec + reconcilerOpt func(r *CommittedResourceChangeReconciler) + afterDryRunCleanup func(t *testing.T, c client.Client, txName string) + wantPhase v1alpha1.ChangePhase + wantFinishedReason string + wantProgressingStatus metav1.ConditionStatus + wantProgressingReason string + }{ + { + name: "nil snapshot: new CR deleted and dry-run completes as Accepted", + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + DryRun: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-dry1": minimalCRSpec("az-1")}, + }, + wantPhase: v1alpha1.ChangePhaseAccepted, + wantFinishedReason: string(v1alpha1.ChangePhaseAccepted), + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: string(v1alpha1.ChangePhaseAccepted), + }, + { + name: "non-nil snapshot: existing CR spec restored and dry-run completes as Accepted", + preObjects: []client.Object{ + &v1alpha1.CommittedResource{ + ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-dry2"}, + Spec: *minimalCRSpec("az-old"), + }, + }, + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + DryRun: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-dry2": minimalCRSpec("az-new")}, + }, + afterDryRunCleanup: func(t *testing.T, c client.Client, _ string) { + setCRReady(t, c, "commitment-uuid-dry2", metav1.ConditionTrue, v1alpha1.CommittedResourceReasonAccepted, "") + }, + wantPhase: v1alpha1.ChangePhaseAccepted, + wantFinishedReason: string(v1alpha1.ChangePhaseAccepted), + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: string(v1alpha1.ChangePhaseAccepted), + }, + { + name: "CRs not yet settled: stays in DryRunCleanup", + preObjects: []client.Object{ + &v1alpha1.CommittedResource{ + ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-dry3"}, + Spec: *minimalCRSpec("az-old"), + }, + }, + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + DryRun: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-dry3": minimalCRSpec("az-new")}, + }, + // Stale the condition to simulate "CR controller has not yet processed the restored spec". + afterDryRunCleanup: func(t *testing.T, c client.Client, _ string) { + setCRReadyStaleGen(t, c, "commitment-uuid-dry3") + }, + wantPhase: v1alpha1.ChangePhaseDryRunCleanup, + wantProgressingStatus: metav1.ConditionTrue, + wantProgressingReason: "Advancing", + }, + { + name: "dry-run cleanup timeout: terminates with RollbackFailed", + preObjects: []client.Object{ + &v1alpha1.CommittedResource{ + ObjectMeta: metav1.ObjectMeta{Name: "commitment-uuid-dry4"}, + Spec: *minimalCRSpec("az-old"), + }, + }, + txSpec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + DryRun: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-dry4": minimalCRSpec("az-new")}, + }, + reconcilerOpt: func(r *CommittedResourceChangeReconciler) { + r.RollbackTimeout = 50 * time.Millisecond + }, + afterDryRunCleanup: func(t *testing.T, c client.Client, txName string) { + setCRReadyStaleGen(t, c, "commitment-uuid-dry4") + backdateRollbackStartedAt(t, c, txName, 1*time.Second) + }, + wantPhase: v1alpha1.ChangePhaseAccepted, + wantFinishedReason: v1alpha1.ChangeReasonRollbackFailed, + wantProgressingStatus: metav1.ConditionFalse, + wantProgressingReason: v1alpha1.ChangeReasonRetryExhausted, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := testScheme(t) + txName := "tx-dry-" + tt.name[:5] + tx := &v1alpha1.CommittedResourceChange{ + ObjectMeta: metav1.ObjectMeta{Name: txName}, + Spec: tt.txSpec, + } + objs := append([]client.Object{tx}, tt.preObjects...) + c := testClient(t, scheme, objs...) + r := testReconciler(c, scheme) + if tt.reconcilerOpt != nil { + tt.reconcilerOpt(r) + } + + var crName string + for uuid := range tt.txSpec.NewState { + crName = commitmentName(uuid) + break + } + + driveToDryRunCleanup(t, r, c, txName, crName) + + if tt.afterDryRunCleanup != nil { + tt.afterDryRunCleanup(t, c, txName) + } + + doReconcile(t, r, txName) + + tx = getTx(t, c, txName) + if tx.Status.Phase != tt.wantPhase { + t.Errorf("phase: want %q, got %q", tt.wantPhase, tx.Status.Phase) + } + if tt.wantFinishedReason != "" { + assertCondition(t, tx.Status.Conditions, v1alpha1.ChangeConditionFinished, metav1.ConditionTrue, tt.wantFinishedReason) + } + if tt.wantProgressingStatus != "" { + assertCondition(t, tx.Status.Conditions, v1alpha1.ChangeConditionProgressing, tt.wantProgressingStatus, tt.wantProgressingReason) + } + }) + } +} + +// TestCRTReconciler_DryRun_WouldReject verifies that DryRun=true + rejection follows +// the RollingBack path and terminates as Rejected (not Accepted). +func TestCRTReconciler_DryRun_WouldReject(t *testing.T) { + const txName = "tx-dry-reject" + scheme := testScheme(t) + tx := &v1alpha1.CommittedResourceChange{ + ObjectMeta: metav1.ObjectMeta{Name: txName}, + Spec: v1alpha1.CommittedResourceChangeSpec{ + AvailabilityZone: "az-1", + Source: v1alpha1.TxSourceAPI, + AllowRejection: true, + DryRun: true, + NewState: map[string]*v1alpha1.CommittedResourceSpec{"uuid-dr": minimalCRSpec("az-1")}, + }, + } + c := testClient(t, scheme, tx) + r := testReconciler(c, scheme) + + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseAwaitingScheduling) + // Reject placement → must go to RollingBack (not DryRunCleanup). + setCRReady(t, c, "commitment-uuid-dr", metav1.ConditionFalse, v1alpha1.CommittedResourceReasonRejected, "no capacity") + driveToPhase(t, r, c, txName, v1alpha1.ChangePhaseRollingBack) + + // nil snapshot → delete CR → settle immediately → Rejected. + doReconcile(t, r, txName) + if got := getTx(t, c, txName).Status.Phase; got != v1alpha1.ChangePhaseRejected { + t.Errorf("DryRun would-reject: want Rejected, got %q", got) + } + assertCondition(t, getTx(t, c, txName).Status.Conditions, v1alpha1.ChangeConditionFinished, metav1.ConditionTrue, string(v1alpha1.ChangePhaseRejected)) +}