diff --git a/internal/backupcontroller/artifact_size.go b/internal/backupcontroller/artifact_size.go index 063e165d6..941b00323 100644 --- a/internal/backupcontroller/artifact_size.go +++ b/internal/backupcontroller/artifact_size.go @@ -60,7 +60,8 @@ type s3Connection struct { // so that the restore path, which has a reconciler of its own, can read // the same objects. type objectStore struct { - // reader must reach Secrets; the manager's client reads them uncached. + // reader reads Secrets, which the manager's client fetches uncached, + // and lists operator Backups, which it serves from the cache. reader client.Reader // newLister overrides the S3 client; nil means a real minio client. newLister func(conn s3Connection) (s3ObjectLister, error) diff --git a/internal/backupcontroller/mariadb_artifact_size.go b/internal/backupcontroller/mariadb_artifact_size.go index d10f1c4a5..6397325ca 100644 --- a/internal/backupcontroller/mariadb_artifact_size.go +++ b/internal/backupcontroller/mariadb_artifact_size.go @@ -8,6 +8,10 @@ import ( "strings" "time" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/cozystack/cozystack/internal/backupcontroller/mariadbtypes" ) @@ -60,6 +64,10 @@ func findMariaDBDump(ctx context.Context, store objectStore, mdbBackup *mariadbt if s3 == nil { return mariadbDump{}, errors.New("the operator Backup does not store to S3") } + others, err := finishedMariaDBRuns(ctx, store.reader, mdbBackup) + if err != nil { + return mariadbDump{}, fmt.Errorf("list operator Backups: %w", err) + } conn, err := mariadbS3Connection(ctx, store, mdbBackup.Namespace, s3) if err != nil { return mariadbDump{}, fmt.Errorf("resolve S3 credentials: %w", err) @@ -76,35 +84,104 @@ func findMariaDBDump(ctx context.Context, store objectStore, mdbBackup *mariadbt if err != nil { return mariadbDump{}, fmt.Errorf("list s3://%s/%s: %w", s3.Bucket, prefix, err) } - return matchMariaDBDump(objects, prefix, mdbBackup.CreationTimestamp.Time, completedAt) + own := newMariaDBRun(mdbBackup, mdbBackup.CreationTimestamp.Time, completedAt) + return matchMariaDBDump(objects, prefix, own, others) } -// matchMariaDBDump picks the dump of the run that lived from created to -// completed. The prefix is shared by every backup of the application, so -// the dump is identified by both the timestamp in its file name and its -// LastModified falling inside that lifetime: the two come from different -// clocks, so a skewed pod clock alone cannot pull a neighbouring run's dump -// into the window. -func matchMariaDBDump(objects []s3ObjectInfo, prefix string, created, completed time.Time) (mariadbDump, error) { - lower := created.Add(-mariadbSizeProbeClockSkew) - upper := completed.Add(mariadbSizeProbeClockSkew) - var matched []mariadbDump +// mariadbRun is the span in which an operator Backup can have written its +// dump: its lifetime, widened by mariadbSizeProbeClockSkew on each side. +type mariadbRun struct { + name string + lower, upper time.Time +} + +func newMariaDBRun(b *mariadbtypes.Backup, created, completed time.Time) mariadbRun { + return mariadbRun{ + name: b.Namespace + "/" + b.Name, + lower: created.Add(-mariadbSizeProbeClockSkew), + upper: completed.Add(mariadbSizeProbeClockSkew), + } +} + +func (r mariadbRun) placesOnBothClocks(d mariadbDump) bool { + return inWindow(d.Timestamp, r.lower, r.upper) && inWindow(d.LastModified, r.lower, r.upper) +} + +func (r mariadbRun) placesOnEitherClock(d mariadbDump) bool { + return inWindow(d.Timestamp, r.lower, r.upper) || inWindow(d.LastModified, r.lower, r.upper) +} + +// matchMariaDBDump picks the dump of run own among objects, which share a +// prefix with every other run of the application. A dump carries two +// clocks, the backup pod's in its file name and the object store's in its +// LastModified, and either can be skewed. A dump is taken only when it is +// the one dump either clock places in own's window, both clocks place it +// there, and neither places it in the window of another finished run: a +// dump in the window on one clock only may be this run's own, misplaced by +// a skewed clock, which leaves any other candidate unproven. A neighbour's +// dump is still taken if this run's own is missing from the listing, or +// misplaced on both clocks, and the neighbour's operator Backup is gone or +// has not completed. +func matchMariaDBDump(objects []s3ObjectInfo, prefix string, own mariadbRun, others []mariadbRun) (mariadbDump, error) { + var near []mariadbDump for _, obj := range objects { name := strings.TrimPrefix(obj.Key, prefix) ts, ok := parseMariaDBLogicalBackupFile(name) - if !ok || !inWindow(ts, lower, upper) || !inWindow(obj.LastModified, lower, upper) { + if !ok { continue } - matched = append(matched, mariadbDump{Key: obj.Key, Name: name, Timestamp: ts, Size: obj.Size, LastModified: obj.LastModified}) + d := mariadbDump{Key: obj.Key, Name: name, Timestamp: ts, Size: obj.Size, LastModified: obj.LastModified} + if own.placesOnEitherClock(d) { + near = append(near, d) + } } - switch len(matched) { - case 0: + if len(near) == 0 { return mariadbDump{}, errMariaDBDumpNotFound - case 1: - return matched[0], nil - default: - return mariadbDump{}, fmt.Errorf("%w: %d dumps fall in its lifetime", errMariaDBDumpAmbiguous, len(matched)) } + if len(near) > 1 { + return mariadbDump{}, fmt.Errorf("%w: %d dumps fall in its lifetime on at least one clock", errMariaDBDumpAmbiguous, len(near)) + } + d := near[0] + if !own.placesOnBothClocks(d) { + return mariadbDump{}, fmt.Errorf("%w: %s falls in its lifetime on one clock only", errMariaDBDumpAmbiguous, d.Name) + } + for _, other := range others { + if other.placesOnEitherClock(d) { + return mariadbDump{}, fmt.Errorf("%w: %s also falls in the lifetime of operator Backup %s", errMariaDBDumpAmbiguous, d.Name, other.name) + } + } + return d, nil +} + +// finishedMariaDBRuns returns the runs of the other operator Backups that +// write under the same bucket and prefix as b and have completed, whether +// their Job succeeded or failed. One still running is left out: its window +// has no end yet, and an operator Backup stuck short of Complete, as one +// whose Job pod never starts is, would otherwise contest every later run of +// the application for good. The endpoint is not compared, since one has +// several spellings, and a false neighbour only leaves a size unset. +func finishedMariaDBRuns(ctx context.Context, reader client.Reader, b *mariadbtypes.Backup) ([]mariadbRun, error) { + list := &mariadbtypes.BackupList{} + if err := reader.List(ctx, list); err != nil { + return nil, err + } + var runs []mariadbRun + for i := range list.Items { + other := &list.Items[i] + if other.Namespace == b.Namespace && other.Name == b.Name { + continue + } + s3 := other.Spec.Storage.S3 + if s3 == nil || s3.Bucket != b.Spec.Storage.S3.Bucket || mariadbOperatorS3Prefix(s3.Prefix) != mariadbOperatorS3Prefix(b.Spec.Storage.S3.Prefix) { + continue + } + cond := apimeta.FindStatusCondition(other.Status.Conditions, mariadbtypes.ConditionTypeComplete) + if cond == nil || cond.Status != metav1.ConditionTrue { + continue + } + runs = append(runs, newMariaDBRun(other, other.CreationTimestamp.Time, cond.LastTransitionTime.Time)) + } + return runs, nil } // mariadbS3Connection resolves the Secrets and TLS settings of the operator diff --git a/internal/backupcontroller/mariadb_artifact_size_test.go b/internal/backupcontroller/mariadb_artifact_size_test.go index 719678892..9d649c15c 100644 --- a/internal/backupcontroller/mariadb_artifact_size_test.go +++ b/internal/backupcontroller/mariadb_artifact_size_test.go @@ -239,8 +239,9 @@ func TestMatchMariaDBDump(t *testing.T) { completed := created.Add(3 * time.Minute) const prefix = "tenant/db/" own := s3ObjectInfo{Key: dumpKey(prefix, created.Add(time.Minute), "gzip"), Size: 4242, LastModified: created.Add(2 * time.Minute)} + run := newMariaDBRun(&mariadbtypes.Backup{ObjectMeta: metav1.ObjectMeta{Namespace: "tenant", Name: "op-bk"}}, created, completed) - got, err := matchMariaDBDump([]s3ObjectInfo{own}, prefix, created, completed) + got, err := matchMariaDBDump([]s3ObjectInfo{own}, prefix, run, nil) if err != nil { t.Fatalf("matchMariaDBDump: %v", err) } @@ -255,11 +256,11 @@ func TestMatchMariaDBDump(t *testing.T) { t.Errorf("matchMariaDBDump = %+v, want %+v", got, want) } - if _, err := matchMariaDBDump(nil, prefix, created, completed); !errors.Is(err, errMariaDBDumpNotFound) { + if _, err := matchMariaDBDump(nil, prefix, run, nil); !errors.Is(err, errMariaDBDumpNotFound) { t.Errorf("no dump: err = %v, want errMariaDBDumpNotFound", err) } second := s3ObjectInfo{Key: dumpKey(prefix, created.Add(2*time.Minute), "gzip"), Size: 1, LastModified: created.Add(2 * time.Minute)} - if _, err := matchMariaDBDump([]s3ObjectInfo{own, second}, prefix, created, completed); !errors.Is(err, errMariaDBDumpAmbiguous) { + if _, err := matchMariaDBDump([]s3ObjectInfo{own, second}, prefix, run, nil); !errors.Is(err, errMariaDBDumpAmbiguous) { t.Errorf("two dumps: err = %v, want errMariaDBDumpAmbiguous", err) } } @@ -303,6 +304,154 @@ func TestFindMariaDBDump(t *testing.T) { } } +// TestFindMariaDBDump_Ownership drives runs of one application that share +// a prefix. This run lives from t0 to t0+40s, so its window with the +// one-minute slack is [t0-1m, t0+1m40s]; its own dump is 123 bytes and a +// neighbour's is 666 or 777. A neighbour's dump must never be returned. +func TestFindMariaDBDump_Ownership(t *testing.T) { + t0 := time.Date(2026, 10, 21, 10, 0, 0, 0, time.UTC) + s3 := newRenderedMariaDBTemplate().Storage.S3 + s3.Prefix = "tenant/db" + const prefix = "tenant/db/" + run := func(name string, created, completed time.Time) *mariadbtypes.Backup { + b := operatorBackupStoringTo(t, s3, created) + b.Name = name + b.Status.Conditions = []metav1.Condition{mariadbBackupSucceeded(metav1.NewTime(completed))} + return b + } + dump := func(named, uploaded time.Time, size int64) s3ObjectInfo { + return s3ObjectInfo{Key: dumpKey(prefix, named, "gzip"), Size: size, LastModified: uploaded} + } + own := run("op-bk", t0, t0.Add(40*time.Second)) + find := func(t *testing.T, objects []s3ObjectInfo, operatorBackups ...client.Object) (mariadbDump, error) { + t.Helper() + store := objectStore{ + reader: newMariaDBStrategyTestClient(t, append(operatorBackups, own, s3CredsSecret("tenant", "creds"))...), + newLister: func(s3Connection) (s3ObjectLister, error) { + return &fakeS3Lister{objects: objects}, nil + }, + } + return findMariaDBDump(context.Background(), store, own, t0.Add(40*time.Second)) + } + wantAmbiguous := func(t *testing.T, got mariadbDump, err error) { + t.Helper() + if !errors.Is(err, errMariaDBDumpAmbiguous) { + t.Errorf("findMariaDBDump = %+v, %v; want errMariaDBDumpAmbiguous", got, err) + } + } + wantOwn := func(t *testing.T, got mariadbDump, err error) { + t.Helper() + if err != nil || got.Size != 123 { + t.Errorf("findMariaDBDump = %+v, %v; want this run's 123-byte dump", got, err) + } + } + + t.Run("a pod clock behind does not hand this run the previous run's dump", func(t *testing.T) { + // The previous run finished 20s before this one was created and its + // operator Backup is gone. This run's pod clock is 5m behind, so its + // own dump is named outside the window and uploaded inside it. + got, err := find(t, []s3ObjectInfo{ + dump(t0.Add(-30*time.Second), t0.Add(-20*time.Second), 666), + dump(t0.Add(5*time.Second-5*time.Minute), t0.Add(30*time.Second), 123), + }) + wantAmbiguous(t, got, err) + }) + + t.Run("a pod clock ahead does not hand this run the next run's dump", func(t *testing.T) { + // The next run started 5s after this one completed and has uploaded + // its dump by the time this one is probed; it has not completed yet. + next := operatorBackupStoringTo(t, s3, t0.Add(45*time.Second)) + next.Name = "op-next" + next.Status.Conditions = []metav1.Condition{{ + Type: mariadbtypes.ConditionTypeComplete, Status: metav1.ConditionFalse, + Reason: mariadbtypes.ConditionReasonJobRunning, Message: "Running", LastTransitionTime: metav1.NewTime(t0.Add(45 * time.Second)), + }} + got, err := find(t, []s3ObjectInfo{ + dump(t0.Add(5*time.Second+5*time.Minute), t0.Add(30*time.Second), 123), + dump(t0.Add(50*time.Second), t0.Add(80*time.Second), 777), + }, next) + wantAmbiguous(t, got, err) + }) + + t.Run("an object store clock behind does not hand this run the next run's dump", func(t *testing.T) { + // The store clock is 2m behind: this run's upload is stamped before + // the window opens, the next run's is pulled back into it. + got, err := find(t, []s3ObjectInfo{ + dump(t0.Add(5*time.Second), t0.Add(30*time.Second-2*time.Minute), 123), + dump(t0.Add(50*time.Second), t0.Add(3*time.Minute-2*time.Minute), 777), + }) + wantAmbiguous(t, got, err) + }) + + t.Run("a dump another finished run wrote is not taken when this run's own is gone", func(t *testing.T) { + // This run's dump was pruned; the next run's falls in this run's + // window on both clocks, and in its own. + next := run("op-next", t0.Add(45*time.Second), t0.Add(85*time.Second)) + got, err := find(t, []s3ObjectInfo{ + dump(t0.Add(50*time.Second), t0.Add(75*time.Second), 777), + }, next) + wantAmbiguous(t, got, err) + }) + + t.Run("a dump the window holds on one clock only is not taken", func(t *testing.T) { + // This run's dump is gone; the previous run's operator Backup is too, + // and its dump, started 2m before this run, finished uploading 20s + // before it. + got, err := find(t, []s3ObjectInfo{ + dump(t0.Add(-2*time.Minute), t0.Add(-20*time.Second), 666), + }) + if err == nil { + t.Errorf("findMariaDBDump = %+v; want no dump attributed", got) + } + }) + + t.Run("a run still in progress does not contest this run's dump", func(t *testing.T) { + // A concurrent run created 30s before this one has not uploaded yet. + running := operatorBackupStoringTo(t, s3, t0.Add(-30*time.Second)) + running.Name = "op-running" + running.Status.Conditions = []metav1.Condition{{ + Type: mariadbtypes.ConditionTypeComplete, Status: metav1.ConditionFalse, + Reason: mariadbtypes.ConditionReasonJobRunning, Message: "Running", LastTransitionTime: metav1.NewTime(t0.Add(-30 * time.Second)), + }} + got, err := find(t, []s3ObjectInfo{dump(t0.Add(5*time.Second), t0.Add(30*time.Second), 123)}, running) + wantOwn(t, got, err) + }) + + t.Run("runs minutes apart each keep their own dump", func(t *testing.T) { + previous := run("op-previous", t0.Add(-4*time.Minute), t0.Add(-3*time.Minute)) + next := run("op-next", t0.Add(3*time.Minute), t0.Add(4*time.Minute)) + got, err := find(t, []s3ObjectInfo{ + dump(t0.Add(-230*time.Second), t0.Add(-190*time.Second), 666), + dump(t0.Add(5*time.Second), t0.Add(30*time.Second), 123), + dump(t0.Add(185*time.Second), t0.Add(220*time.Second), 777), + }, previous, next) + wantOwn(t, got, err) + }) + + t.Run("a run that writes elsewhere does not contest this run's dump", func(t *testing.T) { + other := newRenderedMariaDBTemplate().Storage.S3 + other.Prefix = "tenant/other" + elsewhere := operatorBackupStoringTo(t, other, t0) + elsewhere.Name = "op-elsewhere" + elsewhere.Status.Conditions = []metav1.Condition{mariadbBackupSucceeded(metav1.NewTime(t0.Add(40 * time.Second)))} + got, err := find(t, []s3ObjectInfo{dump(t0.Add(5*time.Second), t0.Add(30*time.Second), 123)}, elsewhere) + wantOwn(t, got, err) + }) + + t.Run("a run stuck short of Complete does not contest later runs", func(t *testing.T) { + // A backup Job whose pod never starts leaves its operator Backup + // JobRunning for good. + stuck := operatorBackupStoringTo(t, s3, t0.Add(-3*time.Hour)) + stuck.Name = "op-stuck" + stuck.Status.Conditions = []metav1.Condition{{ + Type: mariadbtypes.ConditionTypeComplete, Status: metav1.ConditionFalse, + Reason: mariadbtypes.ConditionReasonJobRunning, Message: "Running", LastTransitionTime: metav1.NewTime(t0.Add(-3 * time.Hour)), + }} + got, err := find(t, []s3ObjectInfo{dump(t0.Add(5*time.Second), t0.Add(30*time.Second), 123)}, stuck) + wantOwn(t, got, err) + }) +} + // TestMariaDBArtifactSizeBytes_MinioClient drives the real minio client // against a stub ListObjectsV2 endpoint, so the Secret wiring, prefix and // response parsing are exercised together.