Skip to content

Commit 195de03

Browse files
myasnikovdaniilmyasnikovdaniil
committed
fix(mariadb): pin legacy Backups to a proven dump, refuse failed runs
A Backup taken before Backups recorded their dump gives a restore nothing to pin. Refusing such Backups would leave every existing one unrestorable, so a restore of one now makes the same ownership-proven match the backup side makes, against the operator Backup it names, and restores that dump by its timestamp. When no dump can be proven to be the run's own, or the storage cannot be read, mariadb-operator restores the newest dump under the prefix as it always did, and a BestEffortDump Warning on the RestoreJob says so and why. An operator Backup whose Job failed is Complete=True with reason JobFailed, and a BackupJob that took Complete=True for success made a Backup of it. That run left no dump of its own, and restoring it would quietly bring back another run's dump, so the RestoreJob now fails instead. The condition is read by its reason, because its status does not tell a failure from a success. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent 21d1e9f commit 195de03

2 files changed

Lines changed: 156 additions & 14 deletions

File tree

‎internal/backupcontroller/mariadb_restore_dump_test.go‎

Lines changed: 122 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -274,13 +274,128 @@ func TestReconcileMariaDBRestore_RecordedDump(t *testing.T) {
274274
})
275275
}
276276

277-
func TestReconcileMariaDBRestore_NoRecordedDump(t *testing.T) {
278-
l := &fakeS3Lister{err: errors.New("Access Denied.")}
279-
f := newMariaDBRestoreFixture(t, nil, l, s3CredsSecret("tenant", "creds"))
280-
f.reconcile(t)
281-
f.wantRestore(t, "")
282-
if l.calls != 0 {
283-
t.Errorf("S3 listed %d times for a Backup that records no dump", l.calls)
277+
// TestReconcileMariaDBRestore_LegacyBackup drives Backups that record no
278+
// dump: one proven to belong to the source run is restored by its
279+
// timestamp, and anything else restores the newest dump with a Warning.
280+
func TestReconcileMariaDBRestore_LegacyBackup(t *testing.T) {
281+
creds := s3CredsSecret("tenant", "creds")
282+
own := s3ObjectInfo{Key: mdbRestorePrefix + mdbRestoreDump, LastModified: mdbRestoreT0.Add(20 * time.Second)}
283+
newer := s3ObjectInfo{Key: mdbRestorePrefix + mdbNewerDump, LastModified: mdbRestoreT0.Add(time.Hour)}
284+
second := s3ObjectInfo{Key: mdbRestorePrefix + "backup.2026-10-21T10:00:30Z.gzip.sql", LastModified: mdbRestoreT0.Add(35 * time.Second)}
285+
286+
t.Run("a dump proven to be the run's own is restored by its timestamp", func(t *testing.T) {
287+
f := newMariaDBRestoreFixture(t, nil, &fakeS3Lister{objects: []s3ObjectInfo{own, newer}}, creds)
288+
f.reconcile(t)
289+
f.wantRestore(t, mdbRestoreTime)
290+
if ev := drainEvents(f.events); len(ev) != 0 {
291+
t.Errorf("events %q; want none for a proven dump", ev)
292+
}
293+
})
294+
295+
cases := []struct {
296+
name string
297+
lister *fakeS3Lister
298+
cause string
299+
}{
300+
{name: "two dumps in the run's lifetime", lister: &fakeS3Lister{objects: []s3ObjectInfo{own, second, newer}}, cause: "cannot be told apart"},
301+
{name: "no dump in the run's lifetime", lister: &fakeS3Lister{objects: []s3ObjectInfo{newer}}, cause: "no dump in the bucket belongs"},
302+
{name: "a storage that cannot be read", lister: &fakeS3Lister{err: errors.New("Access Denied.")}, cause: "Access Denied."},
303+
}
304+
wantBestEffort := func(t *testing.T, f *mariadbRestoreFixture, cause string) {
305+
t.Helper()
306+
f.wantRestore(t, "")
307+
ev := drainEvents(f.events)
308+
if len(ev) != 1 || !strings.HasPrefix(ev[0], "Warning BestEffortDump ") || !strings.Contains(ev[0], cause) || !strings.Contains(ev[0], "src-bk records no dump") {
309+
t.Errorf("events %q; want one BestEffortDump warning naming the Backup and %q", ev, cause)
310+
}
311+
}
312+
for _, tc := range cases {
313+
t.Run(tc.name+" restores the newest dump with a warning", func(t *testing.T) {
314+
f := newMariaDBRestoreFixture(t, nil, tc.lister, creds)
315+
f.reconcile(t)
316+
wantBestEffort(t, f, tc.cause)
317+
})
318+
}
319+
320+
t.Run("a hanging endpoint is given up at the check timeout", func(t *testing.T) {
321+
f := newMariaDBRestoreFixture(t, nil, &fakeS3Lister{}, creds)
322+
f.r.newS3ObjectLister = func(s3Connection) (s3ObjectLister, error) { return blockingLister{}, nil }
323+
var err error
324+
withShortDumpCheckTimeout(t, func() { _, err = f.r.reconcileMariaDBRestore(context.Background(), f.rj, f.backup) })
325+
if err != nil {
326+
t.Fatalf("reconcileMariaDBRestore: %v", err)
327+
}
328+
wantBestEffort(t, f, context.DeadlineExceeded.Error())
329+
})
330+
}
331+
332+
// TestReconcileMariaDBRestore_FailedBackupRun drives a Backup made of an
333+
// operator Backup whose Job failed: there is no dump of its own to
334+
// restore, whatever the Backup records.
335+
func TestReconcileMariaDBRestore_FailedBackupRun(t *testing.T) {
336+
creds := s3CredsSecret("tenant", "creds")
337+
failed := func(status metav1.ConditionStatus) []metav1.Condition {
338+
return []metav1.Condition{{
339+
Type: mariadbtypes.ConditionTypeComplete, Status: status, Reason: mariadbtypes.ConditionReasonJobFailed,
340+
Message: "Failed", LastTransitionTime: metav1.NewTime(mdbRestoreT0.Add(3 * time.Minute)),
341+
}}
342+
}
343+
listing := []s3ObjectInfo{{Key: mdbRestorePrefix + mdbRestoreDump, LastModified: mdbRestoreT0.Add(20 * time.Second)}}
344+
345+
cases := []struct {
346+
name string
347+
driverMD map[string]string
348+
status metav1.ConditionStatus
349+
}{
350+
{name: "Complete=True/JobFailed with no recorded dump", status: metav1.ConditionTrue},
351+
{name: "Complete=True/JobFailed with a recorded dump", driverMD: recordedDump(mdbRestoreDump), status: metav1.ConditionTrue},
352+
{name: "Complete=False/JobFailed", status: metav1.ConditionFalse},
353+
}
354+
for _, tc := range cases {
355+
t.Run(tc.name+" fails the RestoreJob", func(t *testing.T) {
356+
l := &fakeS3Lister{objects: listing}
357+
f := newMariaDBRestoreFixture(t, tc.driverMD, l, creds)
358+
f.src.Status.Conditions = failed(tc.status)
359+
if err := f.c.Update(context.Background(), f.src); err != nil {
360+
t.Fatalf("update operator Backup: %v", err)
361+
}
362+
f.reconcile(t)
363+
f.wantFailed(t, "k8s.mariadb.com/Backup tenant/op-bk failed", "left no dump to restore")
364+
if l.calls != 0 {
365+
t.Errorf("S3 listed %d times for a failed backup run", l.calls)
366+
}
367+
})
368+
}
369+
370+
t.Run("an operator Restore created earlier is followed", func(t *testing.T) {
371+
existing := &mariadbtypes.Restore{ObjectMeta: metav1.ObjectMeta{
372+
Namespace: "tenant", Name: "rj-abcde",
373+
Labels: map[string]string{
374+
backupsv1alpha1.OwningJobNameLabel: "rj",
375+
backupsv1alpha1.OwningJobNamespaceLabel: "tenant",
376+
},
377+
}}
378+
f := newMariaDBRestoreFixture(t, nil, &fakeS3Lister{objects: listing}, creds, existing)
379+
f.src.Status.Conditions = failed(metav1.ConditionTrue)
380+
if err := f.c.Update(context.Background(), f.src); err != nil {
381+
t.Fatalf("update operator Backup: %v", err)
382+
}
383+
f.reconcile(t)
384+
if f.rj.Status.Phase == backupsv1alpha1.RestoreJobPhaseFailed {
385+
t.Errorf("RestoreJob failed although its operator Restore was already created: %s", f.rj.Status.Message)
386+
}
387+
})
388+
}
389+
390+
func drainEvents(r *record.FakeRecorder) []string {
391+
var out []string
392+
for {
393+
select {
394+
case e := <-r.Events:
395+
out = append(out, e)
396+
default:
397+
return out
398+
}
284399
}
285400
}
286401

‎internal/backupcontroller/mariadbstrategy_controller.go‎

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -603,7 +603,7 @@ func (r *RestoreJobReconciler) reconcileMariaDBRestore(ctx context.Context, rest
603603
// The dump is chosen once, as the operator Restore is created; one that
604604
// already exists has made its choice.
605605
if mdbRestore == nil {
606-
targetTime, err := r.mariadbRestoreTargetTime(ctx, backup, srcBackup)
606+
targetTime, err := r.mariadbRestoreTargetTime(ctx, restoreJob, backup, srcBackup)
607607
var refused mariadbRestoreRefused
608608
if errors.As(err, &refused) {
609609
return r.markRestoreJobFailed(ctx, restoreJob, refused.Error())
@@ -666,19 +666,26 @@ func (e mariadbRestoreRefused) Error() string { return string(e) }
666666
// Restore for backup: the timestamp in the name of the dump backup
667667
// records. mariadb-operator restores the dump whose timestamp equals the
668668
// target and, when there is none, the nearest one, so the dump is first
669-
// checked to still exist. Empty when backup records no dump, which leaves
670-
// the operator to take the newest dump under the prefix.
671-
func (r *RestoreJobReconciler) mariadbRestoreTargetTime(ctx context.Context, backup *backupsv1alpha1.Backup, src *mariadbtypes.Backup) (string, error) {
669+
// checked to still exist. A Backup that records no dump is resolved by
670+
// legacyMariaDBRestoreTargetTime.
671+
func (r *RestoreJobReconciler) mariadbRestoreTargetTime(ctx context.Context, rj *backupsv1alpha1.RestoreJob, backup *backupsv1alpha1.Backup, src *mariadbtypes.Backup) (string, error) {
672+
// A failed Job is Complete=True too, with reason JobFailed, and a
673+
// BackupJob that took Complete=True for success made a Backup of it.
674+
if cond := apimeta.FindStatusCondition(src.Status.Conditions, mariadbtypes.ConditionTypeComplete); cond != nil && cond.Reason == mariadbtypes.ConditionReasonJobFailed {
675+
return "", mariadbRestoreRefused(fmt.Sprintf(
676+
"the backup run of k8s.mariadb.com/Backup %s/%s failed (Complete=%s/%s) and left no dump to restore; restoring would bring back another run's dump",
677+
src.Namespace, src.Name, cond.Status, cond.Reason))
678+
}
679+
ctx, cancel := context.WithTimeout(ctx, mariadbRestoreDumpCheckTimeout)
680+
defer cancel()
672681
name := backup.Spec.DriverMetadata[mariadbDumpFileKey]
673682
if name == "" {
674-
return "", nil
683+
return r.legacyMariaDBRestoreTargetTime(ctx, rj, backup, src), nil
675684
}
676685
ts, ok := parseMariaDBLogicalBackupFile(name)
677686
if !ok {
678687
return "", mariadbRestoreRefused(fmt.Sprintf("Backup driverMetadata %s=%q is not the name of a mariadb-operator dump", mariadbDumpFileKey, name))
679688
}
680-
ctx, cancel := context.WithTimeout(ctx, mariadbRestoreDumpCheckTimeout)
681-
defer cancel()
682689
exists, err := mariadbDumpExists(ctx, r.objectStore(), src, name)
683690
if err != nil {
684691
return "", fmt.Errorf("cannot check that dump %s of k8s.mariadb.com/Backup %s/%s still exists in storage: %w", name, src.Namespace, src.Name, err)
@@ -692,6 +699,26 @@ func (r *RestoreJobReconciler) mariadbRestoreTargetTime(ctx context.Context, bac
692699
return ts.UTC().Format(time.RFC3339), nil
693700
}
694701

702+
// legacyMariaDBRestoreTargetTime chooses the dump of a Backup taken before
703+
// Backups recorded theirs: the one findMariaDBDump proves belongs to src,
704+
// with the completion time the size probe was given, which takenAt keeps.
705+
// When none is proven, for any reason, it returns empty, and the operator
706+
// restores the newest dump under the prefix as such a restore always did:
707+
// refusing would leave these Backups unrestorable, and the controller may
708+
// never have been able to reach their storage. A Warning event says the
709+
// dump is a best effort.
710+
func (r *RestoreJobReconciler) legacyMariaDBRestoreTargetTime(ctx context.Context, rj *backupsv1alpha1.RestoreJob, backup *backupsv1alpha1.Backup, src *mariadbtypes.Backup) string {
711+
dump, err := findMariaDBDump(ctx, r.objectStore(), src, backup.Spec.TakenAt.Time)
712+
if err != nil {
713+
getLogger(ctx).Info("no dump proven for a MariaDB Backup that records none; restoring the newest", "restorejob", rj.Name, "backup", backup.Name, "error", err)
714+
r.Recorder.Eventf(rj, corev1.EventTypeWarning, "BestEffortDump",
715+
"Backup %s records no dump and none in storage could be proven to belong to k8s.mariadb.com/Backup %s/%s (%v); mariadb-operator restores the newest dump under its prefix as a best effort, which may be another backup run's",
716+
backup.Name, src.Namespace, src.Name, err)
717+
return ""
718+
}
719+
return dump.Timestamp.UTC().Format(time.RFC3339)
720+
}
721+
695722
// awaitMariaDBDumpCheck keeps the RestoreJob waiting, with Ready=False
696723
// saying why, while the dump it asks for cannot be checked, and fails it
697724
// with the cause once the restore deadline has passed.

0 commit comments

Comments
 (0)