Skip to content

Commit f004b35

Browse files
androndoandrondo
committed
fix(mongodb): record the CR's flow on the artifact and bound the two open loops
Reading a backup's flow off its CR's finalizer had reached the state switch but not the artifact: the snapshot was still written from the app flag, re-read on the completing reconcile. A flag flipped to false while a system-bucket dump streamed produced a Backup whose snapshot called a cozy-backups archive legacy, so the point-in-time refusal and the S3 backfill both switched off for it. The artifact now takes the same flow the switch does, pinned by a test that flips the flag between mint and completion. Two waits had no upper bound. A live-state read that kept failing at the deadline deferred every poll behind a debug line with no condition, so an unreadable CR pinned the job Running with nothing to act on; it is now a named wait and fails after a grace, with the CR left in place rather than deleted blind. The storage-race retry minted a fresh operator CR per poll while the previous ones terminated, for the whole deadline; it now stops after a small budget and surfaces the operator's error. On useSystemBucket the chart already guarantees backup.enabled, so a strategy without s3 coordinates failed with a remedy the tenant had in place; the wait names the missing coordinates instead. The missing-credentials message no longer blames useSystemBucket on a release that never enabled it. Assisted-by: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 5e3b8c2 commit f004b35

2 files changed

Lines changed: 222 additions & 15 deletions

File tree

‎internal/backupcontroller/psmdbstrategy_controller.go‎

Lines changed: 68 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,19 @@ const (
100100
// terminate. Tenants override via spec.options.restoreTimeoutSeconds.
101101
psmdbDefaultRestoreDeadline = 30 * time.Minute
102102

103+
// psmdbLiveReadGrace bounds how long past the backup deadline the driver keeps
104+
// deferring a cancel it cannot confirm: the live re-read of the operator CR
105+
// keeps failing (throttle, revoked verb, removed CRD). After it the job fails
106+
// and the CR is left in place rather than deleted blind.
107+
psmdbLiveReadGrace = 30 * time.Minute
108+
109+
// psmdbStorageRaceRetryBudget caps how many operator CRs the storage-race
110+
// retry may mint for one BackupJob. Each retry deletes the errored CR and
111+
// mints a fresh one; the race clears after a poll or two on a healthy
112+
// operator, so a job still hitting it after this many attempts has a
113+
// standing problem the tenant must see rather than a churn of CRs.
114+
psmdbStorageRaceRetryBudget = 3
115+
103116
// psmdbBackupSnapshotKind is the Kind stamped onto the snapshot persisted
104117
// in Backup.status.underlyingResources. It carries the S3 storage
105118
// descriptor (by reference — bucket/endpoint/credentialsSecret NAME, never
@@ -320,12 +333,20 @@ func (r *BackupJobReconciler) reconcileMongoDB(ctx context.Context, j *backupsv1
320333
}
321334
}
322335
if msg := psmdbBackupPrecondition(cluster, storageName); msg != "" {
336+
// On useSystemBucket the chart refuses backup.enabled=false at render, so
337+
// a missing storage there means the strategy carried no s3 coordinates to
338+
// inject — name that, not a remedy the tenant already has in place.
339+
reason, hint := "PerconaServerMongoDBBackupsDisabled", "set backup.enabled=true on the MongoDB application"
340+
if useSystemBucket && rendered.S3 == nil {
341+
reason, hint = "MongoDBStrategyHasNoS3", "the MongoDB strategy carries no s3 coordinates to inject; set spec.template.s3 on it"
342+
msg = fmt.Sprintf("%s (%s)", msg, hint)
343+
}
323344
if psmdbBackupDeadlineExceeded(j.Status.StartedAt) {
324345
return r.markBackupJobFailed(ctx, j, fmt.Sprintf(
325-
"psmdb.percona.com/PerconaServerMongoDB %s/%s not ready for backups within %s: %s (set backup.enabled=true on the MongoDB application)",
326-
j.Namespace, psmdbName, psmdbDefaultBackupDeadline, msg))
346+
"psmdb.percona.com/PerconaServerMongoDB %s/%s not ready for backups within %s: %s (%s)",
347+
j.Namespace, psmdbName, psmdbDefaultBackupDeadline, msg, hint))
327348
}
328-
return r.requeueMongoDBBackupWaiting(ctx, j, "PerconaServerMongoDBBackupsDisabled", msg)
349+
return r.requeueMongoDBBackupWaiting(ctx, j, reason, msg)
329350
}
330351

331352
// Opting out (useSystemBucket true→false) stops the injection but leaves the
@@ -382,7 +403,7 @@ func (r *BackupJobReconciler) reconcileMongoDB(ctx context.Context, j *backupsv1
382403
if j.Status.BackupRef != nil {
383404
return ctrl.Result{}, nil
384405
}
385-
artifact, err := r.createMongoDBBackupArtifact(ctx, j, resolved, mdbBackup, rendered, storageName, useSystemBucket)
406+
artifact, err := r.createMongoDBBackupArtifact(ctx, j, resolved, mdbBackup, rendered, storageName, flowSystemBucket)
386407
if err != nil {
387408
return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Backup artifact: %v", err))
388409
}
@@ -410,13 +431,23 @@ func (r *BackupJobReconciler) reconcileMongoDB(ctx context.Context, j *backupsv1
410431
// deadline) — delete the errored CR so a fresh one resolves against a
411432
// caught-up cache — rather than failing the BackupJob terminally.
412433
if flowSystemBucket && psmdbErrorIsUnresolvedStorage(message, storageName) && !psmdbBackupDeadlineExceeded(j.Status.StartedAt) {
413-
if mdbBackup.DeletionTimestamp.IsZero() {
414-
if derr := r.Delete(ctx, mdbBackup); derr != nil && !apierrors.IsNotFound(derr) {
415-
return ctrl.Result{}, derr
434+
// Every retry leaves the previous CR Terminating and mints another, so
435+
// count all of this job's CRs (not just the live one) against the budget;
436+
// once it is spent the error is the tenant's to see, below.
437+
minted, err := r.countMongoDBBackupsForJob(ctx, j)
438+
if err != nil {
439+
return ctrl.Result{}, err
440+
}
441+
if minted < psmdbStorageRaceRetryBudget {
442+
if mdbBackup.DeletionTimestamp.IsZero() {
443+
if derr := r.Delete(ctx, mdbBackup); derr != nil && !apierrors.IsNotFound(derr) {
444+
return ctrl.Result{}, derr
445+
}
416446
}
447+
return r.requeueMongoDBBackupWaiting(ctx, j, "PerconaServerMongoDBStorageRace",
448+
fmt.Sprintf("operator has not observed the injected storage %q yet; retrying the backup (attempt %d of %d)", storageName, minted, psmdbStorageRaceRetryBudget))
417449
}
418-
return r.requeueMongoDBBackupWaiting(ctx, j, "PerconaServerMongoDBStorageRace",
419-
fmt.Sprintf("operator has not observed the injected storage %q yet; retrying the backup", storageName))
450+
message = fmt.Sprintf("%s (the operator still could not resolve the injected storage after %d attempts)", message, psmdbStorageRaceRetryBudget)
420451
}
421452
// Terminal failure. The operator sets state=error/rejected once the pbm
422453
// backup fails or is refused, so fail the BackupJob immediately rather
@@ -482,9 +513,16 @@ func (r *BackupJobReconciler) reconcileMongoDB(ctx context.Context, j *backupsv1
482513
// error) defers rather than deletes blind.
483514
liveState, lerr := r.psmdbBackupLiveState(ctx, mdbBackup.Namespace, mdbBackup.Name)
484515
if lerr != nil {
485-
getLogger(ctx).Debug("could not re-read live backup state before cancelling; deferring rather than deleting blind",
486-
"backupjob", j.Name, "sourceBackup", mdbBackup.Name, "error", lerr)
487-
return ctrl.Result{RequeueAfter: psmdbPollInterval}, nil
516+
// Deferring is the safe default, but not forever: past the grace the
517+
// job fails with the CR left in place (never deleted blind), so an
518+
// unreadable CR cannot pin the job Running with nothing to act on.
519+
if j.Status.StartedAt != nil && time.Since(j.Status.StartedAt.Time) > psmdbDefaultBackupDeadline+psmdbLiveReadGrace {
520+
return r.markBackupJobFailed(ctx, j, fmt.Sprintf(
521+
"psmdb.percona.com PerconaServerMongoDBBackup %s did not start within %s and its live state could not be confirmed for a further %s (%v); the operator backup is left in place",
522+
mdbBackup.Name, psmdbDefaultBackupDeadline, psmdbLiveReadGrace, lerr))
523+
}
524+
return r.requeueMongoDBBackupWaiting(ctx, j, "PerconaServerMongoDBLiveStateUnknown",
525+
fmt.Sprintf("backup %s passed the start deadline but its live state could not be read (%v); not cancelling blind", mdbBackup.Name, lerr))
488526
}
489527
switch liveState {
490528
case "", psmdbtypes.StateWaiting:
@@ -850,6 +888,23 @@ func (r *BackupJobReconciler) ensureMongoDBBackup(ctx context.Context, j *backup
850888
return obj, nil
851889
}
852890

891+
// countMongoDBBackupsForJob counts every PerconaServerMongoDBBackup carrying
892+
// the BackupJob's OwningJob labels, Terminating ones included — the retry
893+
// budget is about how many CRs a job has minted, not how many are still live.
894+
func (r *BackupJobReconciler) countMongoDBBackupsForJob(ctx context.Context, j *backupsv1alpha1.BackupJob) (int, error) {
895+
list := &psmdbtypes.PerconaServerMongoDBBackupList{}
896+
if err := r.List(ctx, list,
897+
client.InNamespace(j.Namespace),
898+
client.MatchingLabels{
899+
backupsv1alpha1.OwningJobNameLabel: j.Name,
900+
backupsv1alpha1.OwningJobNamespaceLabel: j.Namespace,
901+
},
902+
); err != nil {
903+
return 0, err
904+
}
905+
return len(list.Items), nil
906+
}
907+
853908
// findMongoDBBackupForJob returns the PerconaServerMongoDBBackup labelled with
854909
// the BackupJob's OwningJob{Name,Namespace}, if any. Returns (nil, nil) when no
855910
// match is found. See the MariaDB driver's findMariaDBBackupForJob for the
@@ -1317,7 +1372,7 @@ func (r *RestoreJobReconciler) reconcileMongoDBRestore(ctx context.Context, rest
13171372
return ctrl.Result{}, err
13181373
}
13191374
msg := fmt.Sprintf(
1320-
"credentialsSecret %q for bucket %q does not exist in namespace %s (the chart prunes it when the application opts into useSystemBucket); re-create a Secret by that name holding credentials for that bucket, or restore into a target whose storage declares that bucket",
1375+
"credentialsSecret %q for bucket %q does not exist in namespace %s (a release's <release>-s3-creds goes away with the release, or when it opts into useSystemBucket); re-create a Secret by that name holding credentials for that bucket, or restore into a target whose storage declares that bucket",
13211376
source.S3.CredentialsSecret, source.S3.Bucket, target.Namespace)
13221377
deadline := options.effectiveRestoreDeadline()
13231378
if restoreJob.Status.StartedAt != nil && time.Since(restoreJob.Status.StartedAt.Time) > deadline {

‎internal/backupcontroller/psmdbstrategy_controller_test.go‎

Lines changed: 154 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2174,7 +2174,8 @@ func newPsmdbBackupDynamicClient(objs ...*unstructured.Unstructured) *dynamicfak
21742174
func TestReconcileMongoDB_TimedOutCancelUsesLiveState(t *testing.T) {
21752175
setup := func(t *testing.T, live *dynamicfake.FakeDynamicClient) (*BackupJobReconciler, client.Client, *ResolvedBackupConfig, *backupsv1alpha1.BackupJob) {
21762176
job, strategy, app, cluster, resolved := mongodbInjectFixture(true)
2177-
job.Status.StartedAt = &metav1.Time{Time: time.Now().Add(-2 * psmdbDefaultBackupDeadline)}
2177+
// Past the deadline but inside the live-read grace.
2178+
job.Status.StartedAt = &metav1.Time{Time: time.Now().Add(-psmdbDefaultBackupDeadline - psmdbLiveReadGrace/2)}
21782179
cluster.Spec.Backup.Storages = map[string]runtime.RawExtension{
21792180
"s3-storage": {Raw: []byte(`{"type":"s3","s3":{"bucket":"cozy-backups"}}`)},
21802181
}
@@ -2266,7 +2267,31 @@ func TestReconcileMongoDB_TimedOutCancelUsesLiveState(t *testing.T) {
22662267
t.Errorf("an unanswerable live read must defer, not delete blind")
22672268
}
22682269
if failed(t, c, job.Name) {
2269-
t.Errorf("an unanswerable live read must not fail the BackupJob")
2270+
t.Errorf("an unanswerable live read must not fail the BackupJob inside the grace")
2271+
}
2272+
p := &backupsv1alpha1.BackupJob{}
2273+
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: job.Name}, p); err != nil {
2274+
t.Fatalf("get job: %v", err)
2275+
}
2276+
if cond := apimeta.FindStatusCondition(p.Status.Conditions, "Ready"); cond == nil || cond.Reason != "PerconaServerMongoDBLiveStateUnknown" {
2277+
t.Errorf("the deferral must be named on the job (Ready=False PerconaServerMongoDBLiveStateUnknown), got %+v", cond)
2278+
}
2279+
})
2280+
2281+
// Deferring cannot be forever: once the grace past the deadline is spent the
2282+
// job fails, and the CR is still not deleted blind.
2283+
t.Run("live read still unanswerable past the grace → fail, CR left alone", func(t *testing.T) {
2284+
live := newPsmdbBackupDynamicClient()
2285+
r, c, resolved, job := setup(t, live)
2286+
job.Status.StartedAt = &metav1.Time{Time: time.Now().Add(-2*psmdbDefaultBackupDeadline - psmdbLiveReadGrace)}
2287+
if _, err := r.reconcileMongoDB(context.Background(), job.DeepCopy(), resolved); err != nil {
2288+
t.Fatalf("reconcileMongoDB: %v", err)
2289+
}
2290+
if deleted(t, c) {
2291+
t.Errorf("an unconfirmed CR must never be deleted blind, even when the job finally fails")
2292+
}
2293+
if !failed(t, c, job.Name) {
2294+
t.Errorf("an unreadable CR must not pin the BackupJob Running forever; expected Failed past the grace")
22702295
}
22712296
})
22722297

@@ -2803,3 +2828,130 @@ func TestRequeueMongoDBBackupWaiting_WritesStatusOnlyOnChange(t *testing.T) {
28032828
t.Errorf("an unchanged running wait must not write status again: %d writes before, %d after", first, statusWrites)
28042829
}
28052830
}
2831+
2832+
// The flow a backup ran on is fixed at mint time by its CR's finalizer, and the
2833+
// artifact must record that — not the app flag re-read on the completing
2834+
// reconcile. Flip the flag to false while the system-bucket dump completes: the
2835+
// snapshot must still say useSystemBucket, or the PITR refusal and the S3
2836+
// backfill both switch off for an archive that lives in cozy-backups.
2837+
func TestReconcileMongoDB_ArtifactRecordsTheCRsFlowNotTheFlag(t *testing.T) {
2838+
job, strategy, app, cluster, resolved := mongodbInjectFixture(true)
2839+
app.Spec.Backup.UseSystemBucket = false // flipped after the mint
2840+
job.Status.StartedAt = &metav1.Time{Time: time.Now()}
2841+
cluster.Spec.Backup.Storages = map[string]runtime.RawExtension{
2842+
"s3-storage": {Raw: []byte(`{"type":"s3","s3":{"bucket":"cozy-backups","credentialsSecret":"cozy-backups-creds"}}`)},
2843+
}
2844+
ready := &psmdbtypes.PerconaServerMongoDBBackup{
2845+
ObjectMeta: metav1.ObjectMeta{
2846+
Namespace: "tenant", Name: "op-ready",
2847+
Finalizers: []string{psmdbDeleteBackupFinalizer}, // minted on the system-bucket flow
2848+
Labels: map[string]string{
2849+
backupsv1alpha1.OwningJobNameLabel: job.Name,
2850+
backupsv1alpha1.OwningJobNamespaceLabel: job.Namespace,
2851+
},
2852+
},
2853+
Status: psmdbtypes.PerconaServerMongoDBBackupStatus{
2854+
State: psmdbtypes.StateReady,
2855+
Destination: "s3://cozy-backups/tenant/app1/2026-09-20",
2856+
S3: &psmdbtypes.BackupStorageS3{Bucket: "cozy-backups", CredentialsSecret: psmdbDefaultCredentialsSecret},
2857+
},
2858+
}
2859+
c := newMongoDBStrategyTestClient(t, job, strategy, app, cluster, ready)
2860+
r := &BackupJobReconciler{Client: c, Scheme: c.Scheme(), Recorder: record.NewFakeRecorder(10)}
2861+
if _, err := r.reconcileMongoDB(context.Background(), job.DeepCopy(), resolved); err != nil {
2862+
t.Fatalf("reconcileMongoDB: %v", err)
2863+
}
2864+
persisted := &backupsv1alpha1.BackupJob{}
2865+
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: job.Name}, persisted); err != nil {
2866+
t.Fatalf("get job: %v", err)
2867+
}
2868+
if persisted.Status.BackupRef == nil {
2869+
t.Fatalf("expected a Backup artifact, got phase=%q conditions=%+v", persisted.Status.Phase, persisted.Status.Conditions)
2870+
}
2871+
artifact := &backupsv1alpha1.Backup{}
2872+
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: persisted.Status.BackupRef.Name}, artifact); err != nil {
2873+
t.Fatalf("get artifact: %v", err)
2874+
}
2875+
snap, err := unmarshalMongoDBBackupSnapshot(artifact.Status.UnderlyingResources)
2876+
if err != nil || snap == nil {
2877+
t.Fatalf("snapshot: %v (%+v)", err, snap)
2878+
}
2879+
if !snap.UseSystemBucket {
2880+
t.Errorf("the snapshot must record the flow the CR ran on (useSystemBucket), not the app flag re-read at completion")
2881+
}
2882+
}
2883+
2884+
// On useSystemBucket the chart refuses backup.enabled=false, so a missing storage
2885+
// means the strategy carried nothing to inject; the wait must say that rather
2886+
// than send the tenant to a value they already have set.
2887+
func TestReconcileMongoDB_StrategyWithoutS3NamesTheCause(t *testing.T) {
2888+
job, strategy, app, cluster, resolved := mongodbInjectFixture(true)
2889+
strategy.Spec.Template.S3 = nil
2890+
job.Status.StartedAt = &metav1.Time{Time: time.Now()}
2891+
c := newMongoDBStrategyTestClient(t, job, strategy, app, cluster)
2892+
r := &BackupJobReconciler{Client: c, Scheme: c.Scheme(), Recorder: record.NewFakeRecorder(10)}
2893+
if _, err := r.reconcileMongoDB(context.Background(), job.DeepCopy(), resolved); err != nil {
2894+
t.Fatalf("reconcileMongoDB: %v", err)
2895+
}
2896+
persisted := &backupsv1alpha1.BackupJob{}
2897+
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: job.Name}, persisted); err != nil {
2898+
t.Fatalf("get job: %v", err)
2899+
}
2900+
cond := apimeta.FindStatusCondition(persisted.Status.Conditions, "Ready")
2901+
if cond == nil || cond.Reason != "MongoDBStrategyHasNoS3" || !strings.Contains(cond.Message, "no s3 coordinates") {
2902+
t.Errorf("expected Ready=False MongoDBStrategyHasNoS3 naming the missing coordinates, got %+v", cond)
2903+
}
2904+
}
2905+
2906+
// The storage-race retry mints a fresh operator CR per attempt; it must stop
2907+
// after the budget instead of churning CRs every poll for the whole deadline,
2908+
// and the error then reaches the tenant.
2909+
func TestReconcileMongoDB_StorageRaceRetryBudgetExhausted(t *testing.T) {
2910+
job, strategy, app, cluster, resolved := mongodbInjectFixture(true)
2911+
job.Status.StartedAt = &metav1.Time{Time: time.Now()}
2912+
cluster.Spec.Backup.Storages = map[string]runtime.RawExtension{
2913+
"s3-storage": {Raw: []byte(`{"type":"s3","s3":{"bucket":"cozy-backups","credentialsSecret":"cozy-backups-creds"}}`)},
2914+
}
2915+
mk := func(name string) *psmdbtypes.PerconaServerMongoDBBackup {
2916+
return &psmdbtypes.PerconaServerMongoDBBackup{
2917+
ObjectMeta: metav1.ObjectMeta{
2918+
Namespace: "tenant", Name: name, Finalizers: []string{psmdbDeleteBackupFinalizer},
2919+
Labels: map[string]string{
2920+
backupsv1alpha1.OwningJobNameLabel: job.Name,
2921+
backupsv1alpha1.OwningJobNamespaceLabel: job.Namespace,
2922+
},
2923+
},
2924+
Status: psmdbtypes.PerconaServerMongoDBBackupStatus{State: psmdbtypes.StateError, Error: `unable to get storage "s3-storage": not found`},
2925+
}
2926+
}
2927+
old1, old2, live := mk("op-try-1"), mk("op-try-2"), mk("op-try-3")
2928+
c := newMongoDBStrategyTestClient(t, job, strategy, app, cluster, old1, old2, live)
2929+
// Two earlier attempts already deleted (Terminating under the finalizer).
2930+
for _, o := range []*psmdbtypes.PerconaServerMongoDBBackup{old1, old2} {
2931+
if err := c.Delete(context.Background(), o); err != nil {
2932+
t.Fatalf("delete %s: %v", o.Name, err)
2933+
}
2934+
}
2935+
r := &BackupJobReconciler{Client: c, Scheme: c.Scheme(), Recorder: record.NewFakeRecorder(10)}
2936+
if _, err := r.reconcileMongoDB(context.Background(), job.DeepCopy(), resolved); err != nil {
2937+
t.Fatalf("reconcileMongoDB: %v", err)
2938+
}
2939+
persisted := &backupsv1alpha1.BackupJob{}
2940+
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: job.Name}, persisted); err != nil {
2941+
t.Fatalf("get job: %v", err)
2942+
}
2943+
if persisted.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed {
2944+
t.Fatalf("with the retry budget spent the job must fail, got phase=%q", persisted.Status.Phase)
2945+
}
2946+
got := &psmdbtypes.PerconaServerMongoDBBackup{}
2947+
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: "op-try-3"}, got); err != nil || !got.DeletionTimestamp.IsZero() {
2948+
t.Errorf("the terminal path must not delete the live errored CR, err=%v", err)
2949+
}
2950+
list := &psmdbtypes.PerconaServerMongoDBBackupList{}
2951+
if err := c.List(context.Background(), list, client.InNamespace("tenant")); err != nil {
2952+
t.Fatalf("list: %v", err)
2953+
}
2954+
if len(list.Items) != 3 {
2955+
t.Errorf("no further CR may be minted once the budget is spent, got %d", len(list.Items))
2956+
}
2957+
}

0 commit comments

Comments
 (0)