Distinguish transient from terminal condition failures

Reconcile previously set every handler error to reason=Failed, even
when the underlying cause was transient (e.g. cluster operators still
stabilizing or an upgrade in progress) and the operator was about to
retry and succeed on its own. Consumers watching for a terminal
Failed reason had no way to tell "needs intervention" apart from
"will resolve itself."

Add a transientError wrapper and a Retrying reason so preflight can
mark known-transient conditions and have Reconcile record the right
reason automatically.

Author: jcpowermac

api/v1alpha1/vmwarecloudfoundationmigration_types.go

@@ -129,6 +129,11 @@ const (
 	ReasonPaused      = "Paused"
 	ReasonPending     = "Pending"
 
+	// ReasonRetrying indicates the operator hit a transient condition (e.g. an
+	// external resource is temporarily unhealthy) and will automatically retry.
+	// Unlike ReasonFailed, this is not a terminal state.
+	ReasonRetrying = "Retrying"
+
 	// ReasonUnsupportedName indicates the object's name is not SingletonName,
 	// so the operator is ignoring it.
 	ReasonUnsupportedName = "UnsupportedName"

internal/controller/errors.go

@@ -0,0 +1,45 @@
+package controller
+
+import (
+	"errors"
+
+	migrationv1alpha1 "github.com/openshift/vcf-migration-operator/api/v1alpha1"
+)
+
+// transientError wraps an error that represents a temporary condition (e.g. an
+// external resource is still stabilizing) rather than a permanent failure
+// requiring user intervention. The reconciler uses this distinction to choose
+// between ReasonFailed and ReasonRetrying when recording condition status.
+type transientError struct {
+	err error
+}
+
+// newTransientError marks err as transient.
+func newTransientError(err error) error {
+	return &transientError{err: err}
+}
+
+func (e *transientError) Error() string {
+	return e.err.Error()
+}
+
+func (e *transientError) Unwrap() error {
+	return e.err
+}
+
+// isTransientError reports whether err (or any error it wraps) was marked
+// transient via newTransientError.
+func isTransientError(err error) bool {
+	var te *transientError
+	return errors.As(err, &te)
+}
+
+// reasonForError returns the condition Reason to record for a handler error:
+// ReasonRetrying for transient errors (the operator will automatically retry
+// on the next reconcile), ReasonFailed otherwise.
+func reasonForError(err error) string {
+	if isTransientError(err) {
+		return migrationv1alpha1.ReasonRetrying
+	}
+	return migrationv1alpha1.ReasonFailed
+}

internal/controller/errors_test.go

@@ -0,0 +1,85 @@
+package controller
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	migrationv1alpha1 "github.com/openshift/vcf-migration-operator/api/v1alpha1"
+)
+
+func TestIsTransientError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{
+			name: "nil error is not transient",
+			err:  nil,
+			want: false,
+		},
+		{
+			name: "plain error is not transient",
+			err:  fmt.Errorf("boom"),
+			want: false,
+		},
+		{
+			name: "transient error is transient",
+			err:  newTransientError(fmt.Errorf("cluster operators are not healthy")),
+			want: true,
+		},
+		{
+			name: "transient error wrapped by additional context is still transient",
+			err:  fmt.Errorf("running preflight checks: %w", newTransientError(fmt.Errorf("cluster upgrade is in progress"))),
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isTransientError(tt.err); got != tt.want {
+				t.Fatalf("isTransientError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestTransientErrorUnwrap(t *testing.T) {
+	inner := fmt.Errorf("cluster operators are not healthy")
+	transient := newTransientError(inner)
+
+	if transient.Error() != inner.Error() {
+		t.Fatalf("transient.Error() = %q, want %q", transient.Error(), inner.Error())
+	}
+	if !errors.Is(transient, inner) {
+		t.Fatalf("errors.Is(transient, inner) = false, want true")
+	}
+}
+
+func TestReasonForError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want string
+	}{
+		{
+			name: "transient error retries",
+			err:  newTransientError(fmt.Errorf("cluster operators are not healthy")),
+			want: migrationv1alpha1.ReasonRetrying,
+		},
+		{
+			name: "plain error fails",
+			err:  fmt.Errorf("spec.failureDomains must not be empty"),
+			want: migrationv1alpha1.ReasonFailed,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := reasonForError(tt.err); got != tt.want {
+				t.Fatalf("reasonForError(%v) = %q, want %q", tt.err, got, tt.want)
+			}
+		})
+	}
+}

internal/controller/preflight.go

@@ -93,7 +93,7 @@ func (r *VmwareCloudFoundationMigrationReconciler) runPreflightChecks(ctx contex
 	}
 	r.setCondition(migration, condType, metav1.ConditionFalse, migrationv1alpha1.ReasonProgressing, "Validating cluster readiness")
 	if support.UpgradeInProgress {
-		return "", fmt.Errorf("cluster upgrade is in progress; wait for ClusterVersion/version Progressing=False before starting migration")
+		return "", newTransientError(fmt.Errorf("cluster upgrade is in progress; wait for ClusterVersion/version Progressing=False before starting migration"))
 	}
 
 	opMgr := openshift.NewOperatorManager(r.ConfigClient)
@@ -102,7 +102,7 @@ func (r *VmwareCloudFoundationMigrationReconciler) runPreflightChecks(ctx contex
 		return "", fmt.Errorf("checking cluster operator health: %w", err)
 	}
 	if !healthy {
-		return "", fmt.Errorf("cluster operators are not healthy; wait for operators to recover before starting migration: %s", strings.Join(unhealthyOperators, ", "))
+		return "", newTransientError(fmt.Errorf("cluster operators are not healthy; wait for operators to recover before starting migration: %s", strings.Join(unhealthyOperators, ", ")))
 	}
 
 	if err := checkNoVSphereCSIPersistentVolumes(ctx, r.KubeClient); err != nil {

internal/controller/preflight_test.go

@@ -481,6 +481,7 @@ func TestRunPreflightChecks(t *testing.T) {
 		mutateMigration           func(*migrationv1alpha1.VmwareCloudFoundationMigration)
 		wantMessageContains       string
 		wantErrContains           string
+		wantTransient             bool
 		wantTargetSecretReadCount int
 	}{
 		{
@@ -551,6 +552,7 @@ func TestRunPreflightChecks(t *testing.T) {
 			gateEnabled:               true,
 			progressing:               true,
 			wantErrContains:           "cluster upgrade is in progress",
+			wantTransient:             true,
 			wantTargetSecretReadCount: 1,
 		},
 		{
@@ -569,6 +571,7 @@ func TestRunPreflightChecks(t *testing.T) {
 				},
 			},
 			wantErrContains:           "cluster operators are not healthy",
+			wantTransient:             true,
 			wantTargetSecretReadCount: 1,
 		},
 		{
@@ -668,6 +671,9 @@ func TestRunPreflightChecks(t *testing.T) {
 				if !strings.Contains(err.Error(), tt.wantErrContains) {
 					t.Fatalf("runPreflightChecks error = %q, want substring %q", err.Error(), tt.wantErrContains)
 				}
+				if got := isTransientError(err); got != tt.wantTransient {
+					t.Fatalf("isTransientError(err) = %v, want %v", got, tt.wantTransient)
+				}
 			} else {
 				if err != nil {
 					t.Fatalf("runPreflightChecks: %v", err)

internal/controller/vmwarecloudfoundationmigration_controller.go

@@ -241,7 +241,7 @@ func (r *VmwareCloudFoundationMigrationReconciler) Reconcile(ctx context.Context
 		log.V(1).Info("processing condition", "condition", condType)
 		result, err := handler(ctx, migration)
 		if err != nil {
-			r.setCondition(migration, condType, metav1.ConditionFalse, migrationv1alpha1.ReasonFailed, err.Error())
+			r.setCondition(migration, condType, metav1.ConditionFalse, reasonForError(err), err.Error())
 			r.Recorder.Eventf(migration, "Warning", "ConditionFailed", "Condition %s failed: %v", condType, err)
 		}