Skip to content

Commit 3d342ce

Browse files
committed
fix(mongodb): keep retentionPolicy a genuine no-op, and cancel a timed-out backup so it cannot orphan an archive
Every prose surface (the useSystemBucket field, the retentionPolicy field, and backup-classes.md) tells a tenant that schedule and retentionPolicy are ignored on this flow, and cozyvalues-gen copies that into the schema, README and the dashboard. A render-time reject of a non-default retentionPolicy contradicted all of them: a tenant who followed the documented opt-in with a retentionPolicy still set got a failed render and a HelmRelease that stopped converging, pointed at the very doc that says the field does nothing. Drop the reject so retentionPolicy is inert exactly as documented, handled the same way as schedule (gated out, no error). The absence of retention on this flow is real but belongs to a follow-up, so the docs now point at the BackupRetentionPolicy design (#4192) and the teardown-GC issue (#3966), and recommend an S3 lifecycle policy until they land. Cancel a timed-out operator backup instead of only failing the BackupJob. The deadline fires for a backup the operator has not started; but `waiting` is the state pbm reports for a backup queued behind another, and the operator starts it once the slot frees — after the BackupJob is Failed and never reconciled again, writing an archive into the shared bucket that no Backup object represents. The deadline branch now deletes the operator CR before failing, which prevents that late run and, on the useSystemBucket flow, lets its delete-backup finalizer take anything partially written; the delete is best-effort so it never blocks the failure. Assisted-By: LLM Signed-off-by: Andrey Kolkov <[email protected]>
1 parent 3206628 commit 3d342ce

5 files changed

Lines changed: 78 additions & 37 deletions

File tree

‎docs/operations/backup-classes.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ When `useSystemBucket: true`:
119119

120120
Because the chart drops `spec.backup.tasks` and `spec.backup.pitr`, the psmdb operator's own scheduled backups and oplog PITR do **not** run on this flow — `backup.schedule` and `backup.retentionPolicy` stay in the schema for the default flow but have no effect here, and the `recoveryTime` PITR restores described below are not available. Migrate scheduled backups to a `backups.cozystack.io/Plan` against `cozy-default` instead. **Flipping this flag on a running app** removes the storage, tasks and pitr the app had, so a release that was taking scheduled backups + PITR before the flip stops until a `Plan` takes over.
121121

122-
Dropping `spec.backup.tasks` also drops the psmdb `keep` retention it carried, so on this flow **the driver owns the archive** the way the Rabbitmq/Redis drivers own theirs (an on-demand `PerconaServerMongoDBBackup` is not covered by psmdb task retention). The driver stamps the operator's `percona.com/delete-backup` finalizer onto each backup it mints here, and deleting a Cozystack `Backup` (a retention-pruned `Plan`, or a manual delete) deletes that `PerconaServerMongoDBBackup` CR — whose finalizer removes the pbm object from the shared bucket before the `Backup` is released, so a Plan does not accumulate objects in `cozy-backups`. A legacy backup writes to the tenant's own bucket and keeps the pre-existing hands-off contract (its lifecycle stays the tenant's / operator's; nothing is deleted on `Backup` removal). Note this prunes only when a `Backup` is deleted: unlike CNPG and MariaDB, which prune engine-side on their own schedule, MongoDB has no engine-side scheduled trim on this flow, so keeping `cozy-backups` bounded depends on a `Plan`'s retention actually deleting old `Backup` objects (scheduled retention at the `Plan` level is tracked as future work). Two cases release the `Backup` without waiting for the object to be pruned, so a wedged prune never pins a namespace in `Terminating`: a terminating namespace (where the operator's delete-backup step cannot run) and the `backups.cozystack.io/skip-artifact-cleanup: "true"` annotation. In both the driver strips its own finalizer and leaves the object for a bucket lifecycle policy to reclaim. A down operator or unreachable storage in a live namespace is NOT released automatically — the `Backup` stays `Terminating` as a visible signal until the operator recovers or you set that annotation to force the release.
122+
Dropping `spec.backup.tasks` also drops the psmdb `keep` retention it carried, so on this flow **the driver owns the archive** the way the Rabbitmq/Redis drivers own theirs (an on-demand `PerconaServerMongoDBBackup` is not covered by psmdb task retention). The driver stamps the operator's `percona.com/delete-backup` finalizer onto each backup it mints here, and deleting a Cozystack `Backup` (a retention-pruned `Plan`, or a manual delete) deletes that `PerconaServerMongoDBBackup` CR — whose finalizer removes the pbm object from the shared bucket before the `Backup` is released, so a Plan does not accumulate objects in `cozy-backups`. A legacy backup writes to the tenant's own bucket and keeps the pre-existing hands-off contract (its lifecycle stays the tenant's / operator's; nothing is deleted on `Backup` removal). Note this prunes only when a `Backup` is deleted: unlike CNPG and MariaDB, which prune engine-side on their own schedule, MongoDB has no engine-side scheduled trim on this flow, so keeping `cozy-backups` bounded depends on a `Plan`'s retention actually deleting old `Backup` objects. **A `Plan` has no retention field yet** (`PlanSpec` carries only `applicationRef`, `backupClassName` and `schedule`), so today a nightly `useSystemBucket` Plan grows the shared bucket unbounded — `schedule` and `retentionPolicy` on the app are inert here (see [MongoDB: opt-in to the system bucket](#mongodb-backup-storage)), and platform-level retention is tracked in the [`BackupRetentionPolicy` design (#4192)](https://github.com/cozystack/cozystack/pull/4192). Teardown GC of orphaned `Backup`s / S3 data is [#3966](https://github.com/cozystack/cozystack/issues/3966). Until those land, bound `cozy-backups` with an S3 lifecycle policy on the object store. Two cases release the `Backup` without waiting for the object to be pruned, so a wedged prune never pins a namespace in `Terminating`: a terminating namespace (where the operator's delete-backup step cannot run) and the `backups.cozystack.io/skip-artifact-cleanup: "true"` annotation. In both the driver strips its own finalizer and leaves the object for a bucket lifecycle policy to reclaim. A down operator or unreachable storage in a live namespace is NOT released automatically — the `Backup` stays `Terminating` as a visible signal until the operator recovers or you set that annotation to force the release.
123123

124124
**Opting back out** (`useSystemBucket` true→false) is not automatic in reverse: the driver stops injecting, but the `s3-storage` entry it already applied stays on the live `PerconaServerMongoDB` under the driver's own field manager (`cozystack-psmdb-backup-driver`) — the chart does not own it and so never prunes it. It is inert once `backup.enabled=false`; to fully return a release to the legacy flow, delete that storage entry from the `PerconaServerMongoDB` by hand before re-declaring the tenant's own `backup.*` S3 values.
125125

‎internal/backupcontroller/psmdbstrategy_controller.go‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,22 @@ func (r *BackupJobReconciler) reconcileMongoDB(ctx context.Context, j *backupsv1
348348
// BackupJob Running forever), but a running dump is left to finish rather
349349
// than stranding its archive — see psmdbBackupTimedOut.
350350
if psmdbBackupTimedOut(state, j.Status.StartedAt) {
351+
// Cancel the operator CR before failing. A `waiting` backup is queued
352+
// behind another and the operator starts it once the slot frees — which
353+
// would be after this BackupJob is Failed and never reconciled again,
354+
// writing an archive into the shared bucket that no Backup object
355+
// represents. Deleting the CR now prevents that late run; on the
356+
// useSystemBucket flow its delete-backup finalizer also takes anything
357+
// partially written. Best-effort: a delete failure must not stop the
358+
// job from failing (a leftover CR is the pre-existing behaviour, not a
359+
// regression). The terminal Phase=Failed makes reconcileMongoDB return
360+
// early next time, so ensureMongoDBBackup never re-creates it.
361+
if mdbBackup.DeletionTimestamp.IsZero() {
362+
if derr := r.Delete(ctx, mdbBackup); derr != nil && !apierrors.IsNotFound(derr) {
363+
getLogger(ctx).Debug("could not cancel the timed-out operator backup before failing",
364+
"backupjob", j.Name, "sourceBackup", mdbBackup.Name, "error", derr)
365+
}
366+
}
351367
detail := "no state observed"
352368
if state != "" {
353369
detail = fmt.Sprintf("state=%s", state)
@@ -575,9 +591,11 @@ func psmdbBackupDeadlineExceeded(startedAt *metav1.Time) bool {
575591
// (the operator CR carries no ownerRef either). A real dataset routinely outruns
576592
// 30m, and a genuinely broken dump terminates through the operator's own
577593
// error/rejected state, so a running backup is left to finish. The deadline
578-
// still bounds the case it was written for — the operator never starts the
579-
// backup (state stays "", requested or waiting), where nothing has been written
580-
// and failing strands nothing.
594+
// still bounds the case it was written for — the operator has not started the
595+
// backup (state stays "", requested or waiting), where nothing has been written.
596+
// A `waiting` backup (queued behind another) could still be started later, so
597+
// the caller cancels the operator CR when this trips, rather than leaving it to
598+
// run into the shared bucket after the BackupJob is already Failed.
581599
func psmdbBackupTimedOut(state string, startedAt *metav1.Time) bool {
582600
if state == psmdbtypes.StateRunning {
583601
return false

‎internal/backupcontroller/psmdbstrategy_controller_test.go‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1234,6 +1234,54 @@ func mongodbInjectFixture(enabled bool) (*backupsv1alpha1.BackupJob, *strategyv1
12341234
return job, strategy, app, cluster, resolved
12351235
}
12361236

1237+
// When a backup the operator never started (state=waiting, queued behind
1238+
// another) trips the deadline, the driver must cancel the operator CR before
1239+
// failing the BackupJob — otherwise the operator starts it later and writes an
1240+
// archive into the shared bucket that no Backup object represents.
1241+
func TestReconcileMongoDB_TimedOutBackupIsCancelled(t *testing.T) {
1242+
job, strategy, app, cluster, resolved := mongodbInjectFixture(true)
1243+
// The BackupJob started well past the deadline.
1244+
old := &metav1.Time{Time: time.Now().Add(-2 * psmdbDefaultBackupDeadline)}
1245+
job.Status.StartedAt = old
1246+
// An operator backup already exists for this job, queued (state=waiting), with
1247+
// the delete-backup finalizer the useSystemBucket flow stamps.
1248+
opBackup := &psmdbtypes.PerconaServerMongoDBBackup{
1249+
ObjectMeta: metav1.ObjectMeta{
1250+
Namespace: "tenant",
1251+
Name: "op-waiting",
1252+
Finalizers: []string{psmdbDeleteBackupFinalizer},
1253+
Labels: map[string]string{
1254+
backupsv1alpha1.OwningJobNameLabel: job.Name,
1255+
backupsv1alpha1.OwningJobNamespaceLabel: job.Namespace,
1256+
},
1257+
},
1258+
Status: psmdbtypes.PerconaServerMongoDBBackupStatus{State: psmdbtypes.StateWaiting},
1259+
}
1260+
1261+
c := newMongoDBStrategyTestClient(t, job, strategy, app, cluster, opBackup)
1262+
r := &BackupJobReconciler{Client: c, Scheme: c.Scheme()}
1263+
1264+
if _, err := r.reconcileMongoDB(context.Background(), job.DeepCopy(), resolved); err != nil {
1265+
t.Fatalf("reconcileMongoDB: %v", err)
1266+
}
1267+
1268+
// The operator CR must be cancelled (deleted): with the finalizer it stays
1269+
// Terminating, so assert a DeletionTimestamp; without the fix it is untouched.
1270+
got := &psmdbtypes.PerconaServerMongoDBBackup{}
1271+
err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: "op-waiting"}, got)
1272+
if err == nil && got.DeletionTimestamp.IsZero() {
1273+
t.Errorf("timed-out operator backup must be cancelled (deleted) so it cannot run later; it was left untouched")
1274+
}
1275+
// And the BackupJob is failed.
1276+
persisted := &backupsv1alpha1.BackupJob{}
1277+
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "tenant", Name: job.Name}, persisted); err != nil {
1278+
t.Fatalf("get job: %v", err)
1279+
}
1280+
if persisted.Status.Phase != backupsv1alpha1.BackupJobPhaseFailed {
1281+
t.Errorf("expected the BackupJob to be Failed, got phase=%q", persisted.Status.Phase)
1282+
}
1283+
}
1284+
12371285
// A cluster that can never service a backup (backup.enabled=false) must not be
12381286
// mutated: injection is gated on Enabled so the disabled cluster fails the
12391287
// precondition on the enabled check without the driver having written storage

‎packages/apps/mongodb/templates/mongodb.yaml‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -217,18 +217,6 @@ spec:
217217
{{- end }}
218218
{{- end }}
219219

220-
{{- /*
221-
useSystemBucket cannot honour backup.retentionPolicy: the tasks[].keep block
222-
below (the chart's only retention) is gated out on this flow, and no
223-
Plan-level retention exists yet, so a retentionPolicy set here would silently
224-
grow the shared cozy-backups bucket. Reject a value the tenant changed from
225-
the chart default rather than ignore it — an untouched useSystemBucket install
226-
keeps rendering. $defaultRetentionPolicy MUST track the values.yaml default.
227-
*/}}
228-
{{- $defaultRetentionPolicy := "30d" }}
229-
{{- if and .Values.backup.useSystemBucket .Values.backup.retentionPolicy (ne .Values.backup.retentionPolicy $defaultRetentionPolicy) }}
230-
{{- fail (printf "backup.retentionPolicy=%q cannot be honoured when backup.useSystemBucket=true: the platform flow has no engine-side retention and Plan-level retention is not yet available (see docs/operations/backup-classes.md). Remove backup.retentionPolicy, or drive scheduled backups and retention through a backups.cozystack.io/Plan." .Values.backup.retentionPolicy) }}
231-
{{- end }}
232220
backup:
233221
enabled: {{ .Values.backup.enabled | default false }}
234222
image: percona/percona-backup-mongodb:2.11.0

‎packages/apps/mongodb/tests/mongodb_test.yaml‎

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,10 @@ tests:
9393
path: spec.backup.pitr
9494
documentIndex: 0
9595

96-
- it: rejects a non-default retentionPolicy on useSystemBucket
97-
# The platform flow cannot honour retentionPolicy (no engine-side trim, no
98-
# Plan retention yet), so a value the tenant changed from the default is a
99-
# hard error rather than a silent no-op that grows the shared bucket.
96+
- it: ignores retentionPolicy and schedule on useSystemBucket (renders, no tasks)
97+
# On this flow retentionPolicy and schedule are inert (documented as ignored),
98+
# handled the same way: a custom value renders cleanly without emitting the
99+
# tasks block, never a hard render error.
100100
release:
101101
name: test-mongodb
102102
namespace: tenant-test
@@ -107,28 +107,15 @@ tests:
107107
enabled: true
108108
useSystemBucket: true
109109
retentionPolicy: 7d
110-
asserts:
111-
- failedTemplate:
112-
errorMessage: 'backup.retentionPolicy="7d" cannot be honoured when backup.useSystemBucket=true: the platform flow has no engine-side retention and Plan-level retention is not yet available (see docs/operations/backup-classes.md). Remove backup.retentionPolicy, or drive scheduled backups and retention through a backups.cozystack.io/Plan.'
113-
114-
- it: allows the default retentionPolicy on useSystemBucket
115-
# The untouched default must still render, so the guard only fires on a
116-
# tenant-changed value (this pins the discriminator, not just the reject).
117-
release:
118-
name: test-mongodb
119-
namespace: tenant-test
120-
set:
121-
_cluster:
122-
cluster-domain: cozy.local
123-
backup:
124-
enabled: true
125-
useSystemBucket: true
126-
retentionPolicy: 30d
110+
schedule: "0 5 * * *"
127111
asserts:
128112
- equal:
129113
path: spec.backup.enabled
130114
value: true
131115
documentIndex: 0
116+
- notExists:
117+
path: spec.backup.tasks
118+
documentIndex: 0
132119

133120
##################
134121
# Finalizers #

0 commit comments

Comments
 (0)