diff --git a/internal/backupcontroller/mariadbstrategy_controller.go b/internal/backupcontroller/mariadbstrategy_controller.go index a5bd77094..48abd3644 100644 --- a/internal/backupcontroller/mariadbstrategy_controller.go +++ b/internal/backupcontroller/mariadbstrategy_controller.go @@ -204,7 +204,10 @@ func (r *BackupJobReconciler) reconcileMariaDB(ctx context.Context, j *backupsv1 cond := apimeta.FindStatusCondition(mdbBackup.Status.Conditions, mariadbtypes.ConditionTypeComplete) switch { - case cond != nil && cond.Status == metav1.ConditionTrue: + case cond != nil && cond.Reason == mariadbtypes.ConditionReasonJobFailed: + return r.markBackupJobFailed(ctx, j, mariadbJobFailedMessage("Backup", mdbBackup.Namespace, mdbBackup.Name, cond)) + + case cond != nil && cond.Status == metav1.ConditionTrue && cond.Reason == mariadbtypes.ConditionReasonJobComplete: // Backup completed successfully. Materialise the Cozystack artifact // and finalise the BackupJob. Idempotent: if the artifact already // exists (a previous reconcile created it and then raced on the @@ -231,22 +234,12 @@ func (r *BackupJobReconciler) reconcileMariaDB(ctx context.Context, j *backupsv1 } return ctrl.Result{}, nil - case cond != nil && cond.Status == metav1.ConditionFalse && cond.Reason == mariadbtypes.ConditionReasonJobFailed: - // Terminal failure. The operator marks Complete=False/JobFailed only - // after the underlying Job exhausts its backoff budget, so we can - // fail the BackupJob immediately rather than waiting for the - // driver-side deadline. - message := cond.Message - if message == "" { - message = "k8s.mariadb.com Backup terminal failure" - } - return r.markBackupJobFailed(ctx, j, message) - default: // Still running (Complete=False/JobRunning, Complete=False/JobNotComplete, - // JobSuspended, or no Complete condition yet). Apply a wall-clock - // deadline so a permanently-stuck Backup eventually fails the - // BackupJob instead of pinning it Running forever. + // JobSuspended, a reason this driver does not know, or no Complete + // condition yet). Apply a wall-clock deadline so a permanently-stuck + // Backup eventually fails the BackupJob instead of pinning it Running + // forever. if mariadbBackupDeadlineExceeded(j.Status.StartedAt) { detail := "no Complete condition observed" if cond != nil { @@ -259,6 +252,14 @@ func (r *BackupJobReconciler) reconcileMariaDB(ctx context.Context, j *backupsv1 } } +// mariadbJobFailedMessage names the operator object whose Job failed. The +// operator's own condition message is only "Failed"; the cause is in the +// logs of that object's Job. +func mariadbJobFailedMessage(kind, namespace, name string, cond *metav1.Condition) string { + return fmt.Sprintf("k8s.mariadb.com/%s %s/%s failed (Complete=%s/%s); the cause is in the logs of its Job", + kind, namespace, name, cond.Status, cond.Reason) +} + // mariadbBackupDeadlineExceeded reports whether enough wall-clock time has // elapsed since the BackupJob started that we should give up on a stuck // operator Backup. Returns false when StartedAt is nil so the very first @@ -570,7 +571,10 @@ func (r *RestoreJobReconciler) reconcileMariaDBRestore(ctx context.Context, rest cond := apimeta.FindStatusCondition(mdbRestore.Status.Conditions, mariadbtypes.ConditionTypeComplete) switch { - case cond != nil && cond.Status == metav1.ConditionTrue: + case cond != nil && cond.Reason == mariadbtypes.ConditionReasonJobFailed: + return r.markRestoreJobFailed(ctx, restoreJob, mariadbJobFailedMessage("Restore", mdbRestore.Namespace, mdbRestore.Name, cond)) + + case cond != nil && cond.Status == metav1.ConditionTrue && cond.Reason == mariadbtypes.ConditionReasonJobComplete: now := metav1.Now() restoreJob.Status.CompletedAt = &now restoreJob.Status.Phase = backupsv1alpha1.RestoreJobPhaseSucceeded @@ -585,13 +589,6 @@ func (r *RestoreJobReconciler) reconcileMariaDBRestore(ctx context.Context, rest } return ctrl.Result{}, nil - case cond != nil && cond.Status == metav1.ConditionFalse && cond.Reason == mariadbtypes.ConditionReasonJobFailed: - message := cond.Message - if message == "" { - message = fmt.Sprintf("k8s.mariadb.com/Restore %s/%s terminal failure", mdbRestore.Namespace, mdbRestore.Name) - } - return r.markRestoreJobFailed(ctx, restoreJob, message) - default: deadline := options.effectiveRestoreDeadline() if restoreJob.Status.StartedAt != nil && time.Since(restoreJob.Status.StartedAt.Time) > deadline { diff --git a/internal/backupcontroller/mariadbstrategy_controller_test.go b/internal/backupcontroller/mariadbstrategy_controller_test.go index 0e4f63b5e..307301223 100644 --- a/internal/backupcontroller/mariadbstrategy_controller_test.go +++ b/internal/backupcontroller/mariadbstrategy_controller_test.go @@ -867,6 +867,187 @@ func TestReconcileMariaDBRestore_KindCheckedBeforeSrcBackupGet(t *testing.T) { } } +// The operator reports a failed backing Job as Complete=True with reason +// JobFailed (SetCompleteWithJob in mariadb-operator 25.10.2), so the +// Complete status alone does not tell success from failure. +var mariadbCompleteOutcomeCases = []struct { + name string + status metav1.ConditionStatus + reason string + wantFailed bool + wantDone bool +}{ + {name: "Complete=True/JobComplete succeeds", status: metav1.ConditionTrue, reason: mariadbtypes.ConditionReasonJobComplete, wantDone: true}, + {name: "Complete=True/JobFailed fails", status: metav1.ConditionTrue, reason: mariadbtypes.ConditionReasonJobFailed, wantFailed: true}, + {name: "Complete=False/JobFailed fails", status: metav1.ConditionFalse, reason: mariadbtypes.ConditionReasonJobFailed, wantFailed: true}, + {name: "Complete=True with an unknown reason keeps waiting", status: metav1.ConditionTrue, reason: "SomethingElse"}, + {name: "Complete=False/JobComplete keeps waiting", status: metav1.ConditionFalse, reason: mariadbtypes.ConditionReasonJobComplete}, + {name: "Complete=False/JobRunning keeps waiting", status: metav1.ConditionFalse, reason: mariadbtypes.ConditionReasonJobRunning}, +} + +func mariadbCompleteCondition(status metav1.ConditionStatus, reason string) []metav1.Condition { + return []metav1.Condition{{ + Type: mariadbtypes.ConditionTypeComplete, + Status: status, + Reason: reason, + Message: "Failed", + LastTransitionTime: metav1.Now(), + }} +} + +func TestReconcileMariaDBBackup_CompleteConditionOutcome(t *testing.T) { + strategyAPIGroup := strategyv1alpha1.GroupVersion.Group + resolved := &ResolvedBackupConfig{ + StrategyRef: corev1.TypedLocalObjectReference{ + APIGroup: &strategyAPIGroup, + Kind: strategyv1alpha1.MariaDBStrategyKind, + Name: "mdb-strategy", + }, + } + + for _, tc := range mariadbCompleteOutcomeCases { + t.Run(tc.name, func(t *testing.T) { + now := metav1.Now() + seed := newMariaDBBackupJob("bj", "tenant") + seed.Status.StartedAt = &now + strategy := &strategyv1alpha1.MariaDB{ + ObjectMeta: metav1.ObjectMeta{Name: "mdb-strategy"}, + Spec: strategyv1alpha1.MariaDBSpec{Template: *newRenderedMariaDBTemplate()}, + } + operatorMariaDB := &mariadbtypes.MariaDB{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: mariadbNameForApp(seed.Spec.ApplicationRef.Name)}, + } + operatorBackup := &mariadbtypes.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant", + Name: "bj-op", + Labels: map[string]string{ + backupsv1alpha1.OwningJobNameLabel: seed.Name, + backupsv1alpha1.OwningJobNamespaceLabel: seed.Namespace, + }, + }, + Status: mariadbtypes.BackupStatus{Conditions: mariadbCompleteCondition(tc.status, tc.reason)}, + } + c := newMariaDBStrategyTestClient(t, seed, newMariaDBApp(seed.Spec.ApplicationRef.Name, "tenant"), + strategy, operatorMariaDB, operatorBackup) + r := &BackupJobReconciler{Client: c, Scheme: c.Scheme(), Recorder: record.NewFakeRecorder(10)} + + bj := &backupsv1alpha1.BackupJob{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(seed), bj); err != nil { + t.Fatalf("seed Get: %v", err) + } + res, err := r.reconcileMariaDB(context.Background(), bj, resolved) + if err != nil { + t.Fatalf("reconcileMariaDB: %v", err) + } + + artifacts := &backupsv1alpha1.BackupList{} + if err := c.List(context.Background(), artifacts, client.InNamespace("tenant")); err != nil { + t.Fatalf("list Backups: %v", err) + } + switch { + case tc.wantFailed: + if bj.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed { + t.Errorf("expected Phase=Failed, got %q", bj.Status.Phase) + } + if !strings.Contains(bj.Status.Message, "bj-op") { + t.Errorf("failure message must name the operator Backup, got %q", bj.Status.Message) + } + if bj.Status.BackupRef != nil || len(artifacts.Items) != 0 { + t.Errorf("a failed run must not produce a Backup: ref=%v artifacts=%d", bj.Status.BackupRef, len(artifacts.Items)) + } + case tc.wantDone: + if bj.Status.Phase != backupsv1alpha1.BackupJobPhaseSucceeded { + t.Errorf("expected Phase=Succeeded, got %q (message %q)", bj.Status.Phase, bj.Status.Message) + } + if bj.Status.BackupRef == nil || len(artifacts.Items) != 1 { + t.Errorf("a successful run must produce exactly one Backup: ref=%v artifacts=%d", bj.Status.BackupRef, len(artifacts.Items)) + } + default: + if bj.Status.Phase != backupsv1alpha1.BackupJobPhaseRunning { + t.Errorf("expected Phase=Running, got %q", bj.Status.Phase) + } + if res.RequeueAfter == 0 { + t.Errorf("expected a requeue while the run is in progress, got %+v", res) + } + if len(artifacts.Items) != 0 { + t.Errorf("an unfinished run must not produce a Backup, got %d", len(artifacts.Items)) + } + } + }) + } +} + +func TestReconcileMariaDBRestore_CompleteConditionOutcome(t *testing.T) { + apps := mariadbapp.GroupName + + for _, tc := range mariadbCompleteOutcomeCases { + t.Run(tc.name, func(t *testing.T) { + now := metav1.Now() + cozyBackup := &backupsv1alpha1.Backup{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "src-cozy-bk"}, + Spec: backupsv1alpha1.BackupSpec{ + ApplicationRef: corev1.TypedLocalObjectReference{Kind: "MariaDB", Name: "src", APIGroup: &apps}, + DriverMetadata: map[string]string{mariadbBackupNameKey: "src-op-bk"}, + }, + } + seed := &backupsv1alpha1.RestoreJob{ + ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "rj"}, + Spec: backupsv1alpha1.RestoreJobSpec{ + BackupRef: corev1.LocalObjectReference{Name: cozyBackup.Name}, + TargetApplicationRef: &corev1.TypedLocalObjectReference{Kind: "MariaDB", Name: "target", APIGroup: &apps}, + }, + Status: backupsv1alpha1.RestoreJobStatus{StartedAt: &now, Phase: backupsv1alpha1.RestoreJobPhaseRunning}, + } + operatorRestore := &mariadbtypes.Restore{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "tenant", + Name: "rj-op", + Labels: map[string]string{ + backupsv1alpha1.OwningJobNameLabel: seed.Name, + backupsv1alpha1.OwningJobNamespaceLabel: seed.Namespace, + }, + }, + Status: mariadbtypes.RestoreStatus{Conditions: mariadbCompleteCondition(tc.status, tc.reason)}, + } + c := newMariaDBStrategyTestClient(t, cozyBackup, seed, operatorRestore, + &mariadbtypes.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "src-op-bk"}}, + &mariadbtypes.MariaDB{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: mariadbNameForApp("target")}}) + r := &RestoreJobReconciler{Client: c, Recorder: record.NewFakeRecorder(10)} + + rj := &backupsv1alpha1.RestoreJob{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(seed), rj); err != nil { + t.Fatalf("seed Get: %v", err) + } + res, err := r.reconcileMariaDBRestore(context.Background(), rj, cozyBackup) + if err != nil { + t.Fatalf("reconcileMariaDBRestore: %v", err) + } + + switch { + case tc.wantFailed: + if rj.Status.Phase != backupsv1alpha1.RestoreJobPhaseFailed { + t.Errorf("expected Phase=Failed, got %q", rj.Status.Phase) + } + if !strings.Contains(rj.Status.Message, "rj-op") { + t.Errorf("failure message must name the operator Restore, got %q", rj.Status.Message) + } + case tc.wantDone: + if rj.Status.Phase != backupsv1alpha1.RestoreJobPhaseSucceeded { + t.Errorf("expected Phase=Succeeded, got %q (message %q)", rj.Status.Phase, rj.Status.Message) + } + default: + if rj.Status.Phase != backupsv1alpha1.RestoreJobPhaseRunning { + t.Errorf("expected Phase=Running, got %q", rj.Status.Phase) + } + if res.RequeueAfter == 0 { + t.Errorf("expected a requeue while the run is in progress, got %+v", res) + } + } + }) + } +} + // --------------------------------------------------------------------------- // Test helpers // --------------------------------------------------------------------------- diff --git a/internal/backupcontroller/mariadbtypes/types.go b/internal/backupcontroller/mariadbtypes/types.go index 7bc5b03a1..d9c09f30a 100644 --- a/internal/backupcontroller/mariadbtypes/types.go +++ b/internal/backupcontroller/mariadbtypes/types.go @@ -27,14 +27,14 @@ const ( GroupName = "k8s.mariadb.com" Version = "v1alpha1" - // Operator-side condition type set on Backup and Restore once the run - // terminates. Status=True means success; Status=False with Reason - // explains the failure or in-progress state. + // Operator-side condition type set on Backup and Restore. Status=True + // means the backing Job terminated, successfully or not; only the + // Reason tells the two apart (SetCompleteWithJob in mariadb-operator). ConditionTypeComplete = "Complete" // Reason values the upstream operator emits on the Complete condition. // JobComplete -> Status=True (run succeeded). - // JobFailed -> Status=False (terminal failure of the backing Job). + // JobFailed -> Status=True (terminal failure of the backing Job). // JobRunning -> Status=False (still in progress). // JobSuspended -> Status=False (admin-suspended via spec). ConditionReasonJobComplete = "JobComplete" @@ -183,7 +183,7 @@ type S3TLS struct { } // BackupStatus mirrors Backup.status. The operator expresses success -// via condition type "Complete" with status=True. +// via condition type "Complete" with reason JobComplete. type BackupStatus struct { Conditions []metav1.Condition `json:"conditions,omitempty"` } @@ -217,7 +217,7 @@ type RestoreSpec struct { } // RestoreStatus mirrors Restore.status. Same condition convention as -// Backup: type=Complete, Status=True on success. +// Backup: type=Complete, reason JobComplete on success. type RestoreStatus struct { Conditions []metav1.Condition `json:"conditions,omitempty"` }