Repository navigation
feat(backups): add a native MongoDB (psmdb) backup-strategy driver - #3562
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a native MongoDB backup strategy backed by Percona resources. The change adds API types, backup and restore reconciliation with PITR, default strategy wiring, S3 TLS configuration, operational examples, unit tests, and Chainsaw end-to-end coverage. ChangesMongoDB native backup strategy
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant BackupJobController
participant MongoDBStrategyController
participant PerconaOperator
participant BackupArtifact
BackupJobController->>MongoDBStrategyController: Reconcile MongoDB BackupJob
MongoDBStrategyController->>PerconaOperator: Create or reuse PerconaServerMongoDBBackup
PerconaOperator-->>MongoDBStrategyController: Return backup status and destination
MongoDBStrategyController->>BackupArtifact: Persist backup metadata and snapshot
MongoDBStrategyController->>PerconaOperator: Create or reuse PerconaServerMongoDBRestore
PerconaOperator-->>MongoDBStrategyController: Return restore status
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/backupcontroller/psmdbstrategy_controller_test.go (1)
577-619: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding reconcile-loop coverage.
The helpers build a fake client that already registers
psmdbtypes,mongodbapp, and the status subresources. That is everythingreconcileMongoDBandreconcileMongoDBRestoreneed.Neither reconcile function is exercised. The untested paths include the
StartedAtpersistence and refetch at Lines 145-165, the precondition requeue at Lines 205-217, the deadline gates at Lines 200-204, 272-279, and 607-616, and the terminal-state transitions.Add table-driven tests that seed a
PerconaServerMongoDBand aPerconaServerMongoDBBackupin each operator state, then assert the resultingBackupJobphase and requeue result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/backupcontroller/psmdbstrategy_controller_test.go` around lines 577 - 619, Add table-driven reconcile-loop tests using newMongoDBStrategyTestClient, seeding PerconaServerMongoDB and PerconaServerMongoDBBackup resources across each relevant operator state. Exercise reconcileMongoDB and reconcileMongoDBRestore, asserting BackupJob phase, StartedAt persistence/refetch behavior, deadline and precondition handling, terminal transitions, and expected requeue results.internal/backupcontroller/restorejob_controller.go (1)
332-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the no-op rationale for MongoDB.
The comment states these drivers "don't materialise namespaced artifacts that outlive the RestoreJob". The MongoDB driver does create a
PerconaServerMongoDBRestoreCR in the tenant namespace, and that CR outlives theRestoreJob. The same applies to the MariaDB driver.The no-op behaviour is intentional and matches the MongoDB branch in
backup_controller.go. Only the stated reason is inaccurate. Update the comment so a future maintainer does not read it as "no CR exists".♻️ Proposed comment update
case strategyv1alpha1.CNPGStrategyKind, strategyv1alpha1.JobStrategyKind, strategyv1alpha1.AltinityStrategyKind, strategyv1alpha1.MariaDBStrategyKind, strategyv1alpha1.MongoDBStrategyKind, strategyv1alpha1.FoundationDBStrategyKind, strategyv1alpha1.EtcdStrategyKind: - // Nothing to clean up: these drivers don't materialise namespaced - // artifacts that outlive the RestoreJob. (Etcd: the operator-side + // Nothing to clean up: these drivers own no namespaced artifacts + // whose lifecycle belongs to this controller. Where an operator-side + // CR does outlive the RestoreJob (MariaDB Restore, + // PerconaServerMongoDBRestore), the owning operator's retention + // governs it, and this controller has no delete verb on it. + // (Etcd: the operator-side // EtcdCluster is owned by the source HelmRelease, and the // EtcdClusterSpecCaptured / TargetPurged conditions live on the // RestoreJob itself - all gone with the parent.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/backupcontroller/restorejob_controller.go` around lines 332 - 337, Update the no-op comment in the strategy switch containing CNPGStrategyKind and MongoDBStrategyKind so it does not claim these drivers lack namespaced artifacts that outlive the RestoreJob. Explain that cleanup is intentionally omitted because any driver-created artifacts, including MongoDB and MariaDB restore CRs, are handled or retained according to their existing controller behavior, consistent with the MongoDB branch in backup_controller.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/backups/mongodb/README.md`:
- Around line 3-10: Reflow each prose paragraph in the MongoDB README to a
single physical line, including the paragraphs in the referenced ranges.
Preserve all headings, lists, tables, fenced code blocks, wording, and paragraph
boundaries unchanged.
In `@internal/backupcontroller/mongodbapp/zz_generated.deepcopy.go`:
- Around line 3-5: Move the hand-written DeepCopy methods from
zz_generated.deepcopy.go into a non-generated file such as deepcopy.go,
preserving their existing behavior and package placement. Remove the
hand-written implementation and explanatory comment from the generated-pattern
file so zz_generated.*.go is no longer edited directly.
In `@internal/backupcontroller/psmdbstrategy_controller.go`:
- Around line 426-460: After the successful r.Create call in
createMongoDBBackupArtifact, persist the populated Backup status through
r.Status().Update or an equivalent status patch, including Phase, Artifact, and
UnderlyingResources. Return any status-write error, while preserving the
existing AlreadyExists retrieval path.
In `@internal/backupcontroller/psmdbtypes/zz_generated.deepcopy.go`:
- Around line 3-6: Rename the manually maintained deepcopy implementation file
from the generated-file naming pattern to a non-generated name such as
deepcopy.go, preserving all existing methods and their behavior. Update any
references or generation configuration that depends on the old filename, while
keeping the package API unchanged.
---
Nitpick comments:
In `@internal/backupcontroller/psmdbstrategy_controller_test.go`:
- Around line 577-619: Add table-driven reconcile-loop tests using
newMongoDBStrategyTestClient, seeding PerconaServerMongoDB and
PerconaServerMongoDBBackup resources across each relevant operator state.
Exercise reconcileMongoDB and reconcileMongoDBRestore, asserting BackupJob
phase, StartedAt persistence/refetch behavior, deadline and precondition
handling, terminal transitions, and expected requeue results.
In `@internal/backupcontroller/restorejob_controller.go`:
- Around line 332-337: Update the no-op comment in the strategy switch
containing CNPGStrategyKind and MongoDBStrategyKind so it does not claim these
drivers lack namespaced artifacts that outlive the RestoreJob. Explain that
cleanup is intentionally omitted because any driver-created artifacts, including
MongoDB and MariaDB restore CRs, are handled or retained according to their
existing controller behavior, consistent with the MongoDB branch in
backup_controller.go.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fce4cf55-7cc6-459a-802d-e75db42d3ab0
📒 Files selected for processing (39)
api/apps/v1alpha1/mongodb/types.goapi/backups/strategy/v1alpha1/mongodb_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gocmd/backupstrategy-controller/main.godocs/operations/backup-classes.mdexamples/backups/mongodb/00-bucket.yamlexamples/backups/mongodb/00-helpers.shexamples/backups/mongodb/05-mongodb-src.yamlexamples/backups/mongodb/10-backupjob-adhoc.yamlexamples/backups/mongodb/15-plan.yamlexamples/backups/mongodb/20-mongodb-target.yamlexamples/backups/mongodb/25-restorejob-in-place.yamlexamples/backups/mongodb/30-restorejob-to-copy.yamlexamples/backups/mongodb/README.mdexamples/backups/mongodb/cleanup.shexamples/backups/mongodb/run-all.shhack/e2e-chainsaw/mongodb/chainsaw-test.yamlinternal/backupcontroller/backup_controller.gointernal/backupcontroller/backup_controller_test.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/backupjob_controller_test.gointernal/backupcontroller/mongodbapp/types.gointernal/backupcontroller/mongodbapp/zz_generated.deepcopy.gointernal/backupcontroller/psmdbstrategy_controller.gointernal/backupcontroller/psmdbstrategy_controller_test.gointernal/backupcontroller/psmdbtypes/types.gointernal/backupcontroller/psmdbtypes/zz_generated.deepcopy.gointernal/backupcontroller/restorejob_controller.gopackages/apps/mongodb/README.mdpackages/apps/mongodb/templates/mongodb.yamlpackages/apps/mongodb/templates/restore.yamlpackages/apps/mongodb/values.schema.jsonpackages/apps/mongodb/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mongodbs.yamlpackages/system/backupstrategy-controller/templates/backupclass-default.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/backupstrategy-controller/templates/strategy-mongodb-default.yamlpackages/system/backupstrategy-controller/tests/rendering_test.yamlpackages/system/mongodb-rd/cozyrds/mongodb.yaml
| End-to-end example for backing up and restoring a Cozystack-managed MongoDB | ||
| application via the `strategy.backups.cozystack.io/v1alpha1` `MongoDB` strategy | ||
| driver. The driver delegates execution to the | ||
| [Percona Server for MongoDB operator](https://docs.percona.com/percona-operator-for-mongodb/) | ||
| (`psmdb.percona.com` CRDs, already shipped with Cozystack): backups materialise | ||
| as `PerconaServerMongoDBBackup` CRs and restores as `PerconaServerMongoDBRestore` | ||
| CRs in the application's namespace; the operator handles the actual | ||
| percona-backup-mongodb (pbm) logical dump / restore. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep each prose paragraph on one physical line.
The changed prose wraps individual paragraphs over multiple lines. Reflow each paragraph without changing headings, lists, tables, or fenced code blocks.
As per coding guidelines, Markdown prose paragraphs must use one continuous line per paragraph.
Also applies to: 26-39, 43-51, 55-57, 65-67, 78-86, 90-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/backups/mongodb/README.md` around lines 3 - 10, Reflow each prose
paragraph in the MongoDB README to a single physical line, including the
paragraphs in the referenced ranges. Preserve all headings, lists, tables,
fenced code blocks, wording, and paragraph boundaries unchanged.
Source: Coding guidelines
| // Hand-written DeepCopy methods. The package opted not to wire deepcopy-gen | ||
| // (this is an internal partial-schema mirror, not a generated public API); | ||
| // the surface area is small enough to maintain by hand. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move hand-written code out of zz_generated.deepcopy.go.
Lines 3-5 state that this file is hand-written. Its path matches the generated-file pattern. Generate this file, or move the hand-written methods to a non-generated filename such as deepcopy.go.
As per coding guidelines, **/zz_generated.*.go: “Do not edit generated Go files matching zz_generated.*.go directly.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/backupcontroller/mongodbapp/zz_generated.deepcopy.go` around lines 3
- 5, Move the hand-written DeepCopy methods from zz_generated.deepcopy.go into a
non-generated file such as deepcopy.go, preserving their existing behavior and
package placement. Remove the hand-written implementation and explanatory
comment from the generated-pattern file so zz_generated.*.go is no longer edited
directly.
Source: Coding guidelines
| status := backupsv1alpha1.BackupStatus{ | ||
| Phase: backupsv1alpha1.BackupPhaseReady, | ||
| UnderlyingResources: underlyingResources, | ||
| } | ||
| if mdbBackup.Status.Destination != "" { | ||
| status.Artifact = &backupsv1alpha1.BackupArtifact{URI: mdbBackup.Status.Destination} | ||
| } | ||
|
|
||
| backup := &backupsv1alpha1.Backup{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: j.Name, | ||
| Namespace: j.Namespace, | ||
| }, | ||
| Spec: backupsv1alpha1.BackupSpec{ | ||
| ApplicationRef: j.Spec.ApplicationRef, | ||
| StrategyRef: resolved.StrategyRef, | ||
| TakenAt: takenAt, | ||
| DriverMetadata: driverMD, | ||
| }, | ||
| Status: status, | ||
| } | ||
| if j.Spec.PlanRef != nil { | ||
| backup.Spec.PlanRef = j.Spec.PlanRef | ||
| } | ||
| if err := r.Create(ctx, backup); err != nil { | ||
| if !apierrors.IsAlreadyExists(err) { | ||
| return nil, err | ||
| } | ||
| existing := &backupsv1alpha1.Backup{} | ||
| if getErr := r.Get(ctx, types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}, existing); getErr != nil { | ||
| return nil, getErr | ||
| } | ||
| return existing, nil | ||
| } | ||
| return backup, nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the Backup CRD declares a status subresource, and how sibling drivers persist artifact status.
set -euo pipefail
echo "=== Backup CRD definitions ==="
fd -t f -e yaml -e json | xargs rg -l 'kind: CustomResourceDefinition' 2>/dev/null | while read -r f; do
if rg -q 'backups\.cozystack\.io' "$f"; then
echo "--- $f ---"
rg -n 'names:|kind: Backup|subresources|status: \{\}|scope:' "$f"
fi
done
echo "=== kubebuilder subresource markers on Backup type ==="
rg -n 'subresource:status' api/backups/v1alpha1/
echo "=== sibling drivers: do they call Status().Update after creating the Backup artifact? ==="
rg -nP -C 6 'func \(r \*BackupJobReconciler\) create\w+BackupArtifact' internal/backupcontroller/ | head -100
rg -nP -C 4 'Status\(\)\.Update\(ctx, backup\)' internal/backupcontroller/Repository: cozystack/cozystack
Length of output: 12947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Backup artifact creation/status update implementations ==="
for f in internal/backupcontroller/*strategy_controller.go; do
if rg -q 'create[A-Za-z]*BackupArtifact' "$f"; then
echo "--- $f ---"
sed -n '/func .*create[A-Za-z]*BackupArtifact/,/return .*Backup/p' "$f" | sed -n '1,180p'
fi
done
echo "=== Status().Update occurrences in backup controller ==="
rg -n -C 5 'Status\(\)\.Update|\.Update\(ctx, .*Backup\)' internal/backupcontroller/
echo "=== Backup CRD manifest only ==="
fd -t f 'backups\.cozystack\.io_backups\.yaml' 'backups\.cozystack\.io_backups\.json' packages/api packages/system/backup-controller internal/backupcontroller 2>/dev/null | while read -r f; do
echo "--- $f ---"
rg -n 'kind: CustomResourceDefinition|names:\s*\{[^}]*?\}|subresources|status: \{\}|scope:|apiVersion:' "$f"
done
echo "=== Backup type definition ==="
fd -t f 'backup_types.go|backupobject_types.go' . | while read -r f; do
echo "--- $f ---"
sed -n '1,260p' "$f"
doneRepository: cozystack/cozystack
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Backup type status fields and marker ==="
sed -n '1,220p' api/backups/v1alpha1/backup_types.go
echo "=== Backup CRD status subresource definition ==="
sed -n '/kind: Backup/,/subresources:/p' packages/system/backup-controller/definitions/backups.cozystack.io_backups.yaml | tail -80
echo "=== Backups artifact functions body outlines ==="
for f in internal/backupcontroller/*strategy_controller.go; do
if rg -q 'func .*create[A-Za-z]*BackupArtifact' "$f"; then
echo "--- $f ---"
rg -n 'func .*create[A-Za-z]*BackupArtifact|err := r\.Status\(\)\|Status\(\)\.Update|Status\(\)\.Patch|UnderlyingResources|Artifact' "$f"
fi
done
echo "=== Backup artifact tests with artifact status assertions ==="
rg -n -C 4 'Test[A-Za-z]*BackupArtifact|UnderlyingResources|Artifact != nil|artifact\.Status|Status\.UnderlyingResources' internal/backupcontroller/*_test.goRepository: cozystack/cozystack
Length of output: 38477
Persist Backup.status after creating the artifact.
Backup has the status subresource, so r.Create drops status fields. createMongoDBBackupArtifact uses Backup.status.phase, status.artifact.uri, and status.underlyingResources, but none of these is written through r.Status().Update/Status().Patch. Add a post-create status write for the populated artifact/status fields after r.Create succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/backupcontroller/psmdbstrategy_controller.go` around lines 426 -
460, After the successful r.Create call in createMongoDBBackupArtifact, persist
the populated Backup status through r.Status().Update or an equivalent status
patch, including Phase, Artifact, and UnderlyingResources. Return any
status-write error, while preserving the existing AlreadyExists retrieval path.
| // Hand-written DeepCopy methods. As with mariadbtypes, this package opted not | ||
| // to take the full percona-server-mongodb-operator Go API as a dependency, so | ||
| // deepcopy-gen is not wired up; the surface area is small enough to maintain | ||
| // by hand. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the hand-written methods out of this generated-file path.
This file states that its methods are hand-written. zz_generated.deepcopy.go is reserved for generated code. Rename this file to a non-generated path, such as deepcopy.go, and keep the manual implementations there. This prevents a generation step from overwriting maintained code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/backupcontroller/psmdbtypes/zz_generated.deepcopy.go` around lines 3
- 6, Rename the manually maintained deepcopy implementation file from the
generated-file naming pattern to a non-generated name such as deepcopy.go,
preserving all existing methods and their behavior. Update any references or
generation configuration that depends on the old filename, while keeping the
package API unchanged.
Source: Coding guidelines
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Faithful mirror of the MariaDB/CNPG driver shape (dispatch, cleanup-on-delete without Velero fall-through, supported-kinds, scheme registration, least-privilege RBAC), with green go build/vet/test, helm unittest, and clean renders of every schema-valid chart corner. Two blocking issues in the restore path, plus minor/operational notes.
Findings
[MAJOR] packages/apps/mongodb/templates/backup-secret.yaml:6 + internal/backupcontroller/psmdbstrategy_controller.go:641-684 — restore after the source app is deleted cannot authenticate to S3.
backup-secret.yaml creates {{ .Release.Name }}-s3-creds with no helm.sh/resource-policy: keep, so it is deleted when the source app's Helm release is removed. resolveMongoDBBackupSource builds the restore backupSource.s3.credentialsSecret from the SOURCE cluster's storage config (mongodb-<src>-s3-creds). Scenario: mongodb-src is deleted (the disaster-recovery case backups exist for); the tenant deploys a fresh mongodb-target with backup.enabled=true (which has its own valid mongodb-target-s3-creds for the same bucket) and files a RestoreJob against the surviving Backup artifact. The driver references the now-deleted source Secret, the operator restore cannot authenticate, and the RestoreJob rides the 30m deadline to Failed. The target cluster's own credentials Secret is never used, and nothing warns at backup time. This makes the advertised "restore into a differently-named instance" unreliable for real DR. Fix: derive the restore credentials from the TARGET cluster's storage (the restore replays into the target, which already carries valid creds), or persist/re-project the credentials, or at minimum document that the source release's -s3-creds Secret must outlive any restore.
[MAJOR] internal/backupcontroller/psmdbstrategy_controller.go:515-524,899-912 — a non-string recoveryTime silently degrades a PITR request to a plain snapshot restore and reports Succeeded.
The comment at :515-518 states a malformed recoveryTime fails terminally because "a silent wrong-point restore is worse than failing." That guarantee only holds when spec.options is a JSON object with a string recoveryTime. Since spec.options is runtime.RawExtension (free-form JSON), options: {"recoveryTime": 20260805} (a number) passes admission; json.Unmarshal into the typed struct errors; parseMongoDBRestoreOptions returns zero options plus an error; the caller emits a MalformedOptions Warning and proceeds with defaults; pitrSpec() returns nil; the restore runs to the snapshot, not the requested point, and the RestoreJob reports Succeeded. This is exactly the silent wrong-point restore the code claims to prevent. Fix: when the raw blob parses as an object and the recoveryTime key is present but the typed decode failed, fail terminally instead of falling back to defaults.
[MINOR] internal/backupcontroller/psmdbtypes/types.go:217-226 — partial BackupStorageS3 mirror drops upstream psmdb S3 fields on the read-then-create restore path.
Both restore paths (live live.Status.S3.DeepCopy() at :648-653; snapshot at :681) decode the operator status into this partial type and Create a new PerconaServerMongoDBRestore.spec.backupSource from it. Fields not mirrored (serverSideEncryption, retryer, uploadPartSize, maxUploadParts) are silently dropped, and non-s3 storages (azure/gcs/minio/filesystem) leave backupSource.s3: nil. Not reachable via the default chart (it renders only an s3 storage), so MINOR, but a custom storage would fail restore. The package comment about server-side field preservation on merge does not apply here — this is a Create from partial data, not a patch.
[MINOR] internal/backupcontroller/psmdbstrategy_controller.go:330-364,451-459 — re-creating a BackupJob with the same name reports Succeeded without taking a new backup.
The operator PerconaServerMongoDBBackup CR and the Cozystack Backup artifact (named j.Name) have no OwnerReference to the BackupJob and outlive it. findMongoDBBackupForJob matches the stale CR by OwningJob labels, sees state: ready, and the AlreadyExists path returns the stale artifact, so a re-applied same-name ad-hoc BackupJob flips to Succeeded pointing at an old (possibly reaped) destination. Inherited from the MariaDB driver but data-protection-relevant. Consider owner-referencing the artifact/CR to the BackupJob or keying artifacts on a unique suffix.
[MINOR] examples/backups/mongodb/20-mongodb-target.yaml:17 — the to-copy target writes into the source's S3 prefix.
The target sets destinationPath: s3://<bucket>/mongodb-src/ (the source's prefix) with backup.enabled=true, which enables PITR oplog slicing and the daily backup task (packages/apps/mongodb/templates/mongodb.yaml:165-175). Both clusters use replset rs0, so the target's oplog/backup metadata interleave with the source's under the same keys. Anyone copying this example into a long-lived setup risks a source PITR resolving oplog chunks written by the target. The restore reads via backupSource and does not need the target to write there — give the target its own prefix.
[MINOR] examples/backups/mongodb/00-helpers.sh:128-133 — wait_psmdb_ready treats status.state=error as terminal.
A PerconaServerMongoDB's state is recomputed each operator reconcile and can pass through error transiently during replset init before converging to ready. Passing error as the fail-fast value makes one transient blip fail run-all.sh, failing the mongodb-2-backup-roundtrip Chainsaw suite intermittently. Poll only for the positive terminal value with a timeout.
[MINOR] internal/backupcontroller/psmdbstrategy_controller.go:178 vs :198 — app-CR NotFound fails terminally while psmdb-CR NotFound gets a 30m grace.
A GitOps apply creating the MongoDB app and the BackupJob in one commit can have the BackupJob reconcile before the app CR is visible, hitting an immediate terminal failure at :178, whereas the very next gate tolerates NotFound for 30m. Any transient apiserver error on List/Create likewise fails the job terminally. Consider tolerating app-CR NotFound with the same bounded grace.
Claim mismatches
[PARTIAL] "restore into a differently-named instance ... survives the source Backup CR being reaped." True while the source app release still exists; the restore breaks once the source release (and its -s3-creds Secret) is deleted — see Finding 1.
[UNVERIFIABLE] "Verified end-to-end on a live dev cluster." This review is static; the live run cannot be reproduced here. Plausible only if the dev MongoDB app was configured with backup.enabled=true and a full backup block, and if the source app was still present at restore time.
Operational risks
- Same-name BackupJob re-creation reporting a stale success (Finding above): a scheduled/GitOps flow that re-applies a fixed BackupJob name never produces a new backup yet reports healthy.
Caveats
cozy-defaultfor MongoDB is app-owns-the-storage (bucket/creds live on the MongoDB app CR, populated from.Values.backup.*into a per-release Secret), asymmetric with CNPG/MariaDB which use platform-projectedcozy-backups-creds. Selecting thecozy-defaultBackupClass is not self-service the way the name implies; the app must first be deployed withbackup.enabled=trueand a fullbackupblock. Documented and degrades legibly (Ready=False/PerconaServerMongoDBBackupsDisabled), so a design choice — worth explicit maintainer sign-off.- Upgrade path reasoned, not executed: the only change to an already-installed chart is
apps/mongodbgainingbackup.insecureSkipTLSVerify(default false), byte-identical to the prior hardcoded false, on an operator-owned field; no migration; new CRD has no stored objects. Route an in-place upgrade of a MongoDB app withbackup.enabled=truethrough cozystack-pr-test if a live signal is wanted.
Recommended follow-ups
- Persist strategy-CR
status.conditions(theMongoDBStatusgodoc promises diagnostics that no code writes) or drop the promise. examples/backups/mongodb/run-all.shsed-based substitution corrupts manifests if a COSI-issued S3 key contains|,&, or\;cleanup.shprunes only three hard-coded owner names, leaving Plan-stamped CRs behind (re-arms the stale-reuse trap).
… review Address the two blocking findings in PR #3562: - Restore into a differently-named target now authenticates with the TARGET cluster's own S3 credentials instead of the source's. The backupSource carried the source release's -s3-creds Secret, which is deleted together with the source app — the disaster-recovery case backups exist for — leaving a to-copy restore unable to reach S3. The restore replays into the target (gated on backup.enabled just above), whose own -s3-creds Secret is present and grants access to the same bucket; only the credential reference is swapped, bucket/prefix/endpoint still come from the source archive. Falls back to the source reference when the target declares no discoverable s3 credentials (in-place restore resolves to the same Secret, so it is a no-op there). - A non-string recoveryTime (spec.options is free-form runtime.RawExtension, so {"recoveryTime": 20260805} passes admission) no longer silently degrades a PITR request to a plain snapshot restore reported Succeeded. When the typed decode fails and the blob carries a recoveryTime key, the RestoreJob now fails terminally — the same guarantee pitrSpec already makes for a malformed RFC3339 string. Minor findings also addressed: - BackupJob tolerates a not-yet-present MongoDB app CR with the same bounded grace the psmdb-CR existence gate uses, instead of failing terminally on a GitOps single-commit ordering race. - examples: the to-copy target writes into its OWN S3 prefix (mongodb-target/) rather than the source's, so the two clusters' oplog and backup metadata no longer interleave under shared keys (both run replset rs0). - examples: wait_psmdb_ready no longer treats a transient state=error as terminal — the cluster state is recomputed each reconcile and can blip through error during replset init before converging to ready. Unit tests cover the target-credential selector (preferred/default/sole storage) and the load-bearing recoveryTime-key detection. Signed-off-by: Andrey Kolkov <[email protected]>
… review Address the two blocking findings in PR #3562: - Restore into a differently-named target now authenticates with the TARGET cluster's own S3 credentials instead of the source's. The backupSource carried the source release's -s3-creds Secret, which is deleted together with the source app — the disaster-recovery case backups exist for — leaving a to-copy restore unable to reach S3. The restore replays into the target (gated on backup.enabled just above), whose own -s3-creds Secret is present and grants access to the same bucket; only the credential reference is swapped, bucket/prefix/endpoint still come from the source archive. Falls back to the source reference when the target declares no discoverable s3 credentials (in-place restore resolves to the same Secret, so it is a no-op there). - A non-string recoveryTime (spec.options is free-form runtime.RawExtension, so {"recoveryTime": 20260805} passes admission) no longer silently degrades a PITR request to a plain snapshot restore reported Succeeded. When the typed decode fails and the blob carries a recoveryTime key, the RestoreJob now fails terminally — the same guarantee pitrSpec already makes for a malformed RFC3339 string. Minor findings also addressed: - BackupJob tolerates a not-yet-present MongoDB app CR with the same bounded grace the psmdb-CR existence gate uses, instead of failing terminally on a GitOps single-commit ordering race. - examples: the to-copy target writes into its OWN S3 prefix (mongodb-target/) rather than the source's, so the two clusters' oplog and backup metadata no longer interleave under shared keys (both run replset rs0). - examples: wait_psmdb_ready no longer treats a transient state=error as terminal — the cluster state is recomputed each reconcile and can blip through error during replset init before converging to ready. Unit tests cover the target-credential selector (preferred/default/sole storage) and the load-bearing recoveryTime-key detection. Signed-off-by: Andrey Kolkov <[email protected]>
da383c7 to
b9efc6a
Compare
VerdictLGTM with non-blocking notes The driver is additive, backward-compatible, follows the established per-DB strategy idiom, and its decision branches are covered by non-vacuous tests ( Findings[MINOR]
Novelty pass — second independent reviewer (findings verified against the code)A second reviewer, deliberately given no regression-class checklist, raised the items below. Each was re-verified against the driver source and cross-checked against the already-shipped MariaDB driver before inclusion; none is blocking. [MINOR] Stale-CR adoption — recreating a fixed-name [MINOR] to-copy restore across a different S3 bucket — [NIT] Swallowed template errors — Claim mismatches[DROP / false-positive] cozy_invariant [UNVERIFIABLE] "Verified end-to-end on a live dev cluster …" — out of scope for this hermetic static review (no live cluster). Plausible; not independently confirmed. See Caveats. Caveats
Recommended follow-ups
|
Adds a MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so `apps.cozystack.io/MongoDB` reaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore-to-differently-named instances, and point-in-time recovery through the unified BackupJob/RestoreJob/BackupClass/Plan interface (closes #3545). The driver orchestrates the operator's PerconaServerMongoDBBackup / PerconaServerMongoDBRestore CRDs: - BackupJob: creates a one-shot PerconaServerMongoDBBackup, polls status.state, and persists the S3 destination + storage descriptor (by-reference, no credentials) onto the Cozystack Backup artifact. - RestoreJob: drives PerconaServerMongoDBRestore via backupSource (read back from the source backup status, snapshot fallback) so the same artifact restores in-place or into a differently-named instance; PITR via spec.options.pitr (date/latest). Because psmdb resolves the backup storage by name against the cluster's spec.backup.storages, the driver names the chart's storage (s3-storage) via the cozy-default-mongodb Strategy rather than injecting it, and surfaces a clear Ready=False precondition when a target has backups disabled. Wiring: - api/backups/strategy/v1alpha1: MongoDB strategy type + generated CRD/deepcopy. - internal/backupcontroller: psmdbstrategy_controller + psmdbtypes/mongodbapp partial-schema mirrors; dispatch, supported-kinds and scheme registration. - backupstrategy-controller: cozy-default MongoDB route, cozy-default-mongodb Strategy CR, and psmdb.percona.com RBAC. - apps/mongodb: backup.insecureSkipTLSVerify knob (psmdb s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed seaweedfs. Tests + e2e: - unit tests for the driver (validation, ensure-idempotency, precondition, backupSource resolution, snapshot round-trip, PITR option parsing). - examples/backups/mongodb + a mongodb-2-backup-roundtrip Chainsaw suite that drives run-all.sh end-to-end (sentinel document round-trips through S3 into a restored copy while the source stays untouched), per docs/agents/e2e-testing.md. - docs/operations/backup-classes.md updated. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
…rd test Address branch-review recommendation: give the MongoDB strategy an explicit case in BackupReconciler.cleanupOnDelete, matching CNPG / Altinity / MariaDB / FoundationDB. A deleted MongoDB Backup artifact previously fell through to the Velero default (a safe no-op — MongoDB driverMetadata never carries velero.io/backup-name), but the explicit branch documents the "we do not own the psmdb S3 archive" contract and guards the seam against a future shared helper incidentally stamping velero.io/backup-name onto MongoDB metadata. Ships with TestBackupCleanup_MongoDB_DoesNotFallThroughToVelero, mirroring the one-test-per-branch convention its three siblings follow: it seeds the velero metadata keys + a matching velero.io/Backup and asserts no DeleteBackupRequest is synthesised and the Velero Backup survives — so deleting the branch makes the test fail, which is what makes the guard self-verifying. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
Address branch-review blocker: add TestCreateMongoDBBackupArtifact_ArtifactShape, mirroring the MariaDB sibling. createMongoDBBackupArtifact writes the driver-metadata keys (backup-name/namespace/destination), the Status.Artifact URI, and the persisted snapshot that the restore path reads back — the producer half of a seam previously exercised only end-to-end. A key-name drift between this producer and resolveMongoDBBackupSource would otherwise pass every unit test and surface only in e2e. The test asserts the metadata keys, phase, artifact URI, and that the snapshot decodes to a usable by-reference backupSource; a second case pins the empty-destination path (nil Artifact, no destination key). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Andrey Kolkov <[email protected]>
…on unknown restore options
PITR: replace the bespoke spec.options.pitr{type,date} blob with a single
spec.options.recoveryTime RFC3339 string, matching the CNPG strategy's option
exactly (CNPGRestoreOptions.RecoveryTime). The driver converts recoveryTime to
the psmdb Restore's pitr{type:date,date} in the operator's "YYYY-MM-DD HH:MM:SS"
UTC format. An empty recoveryTime restores the snapshot as-taken (documented
difference from CNPG, whose empty value means latest WAL — a psmdb logical
backup is already consistent). A non-RFC3339 recoveryTime fails terminally.
Unknown options: parseMongoDBRestoreOptions now also returns the set of
spec.options keys the driver does not recognise; reconcileMongoDBRestore emits a
UnknownRestoreOption Warning event per unknown key (and logs it) instead of
silently ignoring it — so a "recoverytime" typo no longer silently degrades a
PITR restore to a snapshot restore. (BackupJob carries no spec.options blob, so
there is no backup-path equivalent; the only backup-time input is the free-form
BackupClass parameters used as template vars.)
Docs + examples updated to recoveryTime; tests cover the RFC3339→UTC mapping,
empty/invalid recoveryTime, and unknown-key reporting.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Signed-off-by: Andrey Kolkov <[email protected]>
… review Address the two blocking findings in PR #3562: - Restore into a differently-named target now authenticates with the TARGET cluster's own S3 credentials instead of the source's. The backupSource carried the source release's -s3-creds Secret, which is deleted together with the source app — the disaster-recovery case backups exist for — leaving a to-copy restore unable to reach S3. The restore replays into the target (gated on backup.enabled just above), whose own -s3-creds Secret is present and grants access to the same bucket; only the credential reference is swapped, bucket/prefix/endpoint still come from the source archive. Falls back to the source reference when the target declares no discoverable s3 credentials (in-place restore resolves to the same Secret, so it is a no-op there). - A non-string recoveryTime (spec.options is free-form runtime.RawExtension, so {"recoveryTime": 20260805} passes admission) no longer silently degrades a PITR request to a plain snapshot restore reported Succeeded. When the typed decode fails and the blob carries a recoveryTime key, the RestoreJob now fails terminally — the same guarantee pitrSpec already makes for a malformed RFC3339 string. Minor findings also addressed: - BackupJob tolerates a not-yet-present MongoDB app CR with the same bounded grace the psmdb-CR existence gate uses, instead of failing terminally on a GitOps single-commit ordering race. - examples: the to-copy target writes into its OWN S3 prefix (mongodb-target/) rather than the source's, so the two clusters' oplog and backup metadata no longer interleave under shared keys (both run replset rs0). - examples: wait_psmdb_ready no longer treats a transient state=error as terminal — the cluster state is recomputed each reconcile and can blip through error during replset init before converging to ready. Unit tests cover the target-credential selector (preferred/default/sole storage) and the load-bearing recoveryTime-key detection. Signed-off-by: Andrey Kolkov <[email protected]>
…rror
resolveMongoDBBackupSource folded two failure classes into one error return:
a transient API read error on the live PerconaServerMongoDBBackup CR (cache
not yet synced, apiserver hiccup) and the terminal no-restorable-destination
case. The RestoreJob caller turned every error into markRestoreJobFailed, so a
retryable read error failed a restore that would have succeeded on the next
reconcile. The backup path already treats the symmetric live-CR read error as
transient (return ctrl.Result{}, err).
Wrap the transient read error in a new errTransientRestoreSource sentinel and
requeue on it at the caller; only a genuinely unresolvable source stays
terminal. Adds a non-vacuous interceptor-driven test.
Addresses the MINOR robustness note from PR review.
Signed-off-by: Andrey Kolkov <[email protected]>
…an read the source bucket A to-copy restore reads the dump from the source backup's bucket/endpoint but authenticates with the target cluster's own S3 credentials (the source release's Secret dies with it in a DR scenario). Document that the target's credentials must therefore be able to read the source bucket - the common same-bucket cozy-backups case - and that a cross-account/bucket target fails loudly with AccessDenied rather than silently restoring nothing. Addresses the MINOR credential-scoping note from PR review. Signed-off-by: Andrey Kolkov <[email protected]>
752bea3 to
b496fb1
Compare
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Approving the API gate, but please get this approved with the relevant section's code owner.
…bootstrap (#3815) ## What this PR does Closes #3814. Unblocks a fresh install of `main`. Since #3562 the `backupstrategy-controller` chart renders one default Strategy CR ungated, and that makes the whole release fail on a cluster where its CRDs are not yet applied: ``` Helm install failed for release cozy-backup-controller/backupstrategy-controller resource mapping not found for name: "cozy-default-mongodb" no matches for kind "MongoDB" in version "strategy.backups.cozystack.io/v1alpha1" ``` The chart ships its own eight `strategy.backups.cozystack.io` CRDs as ordinary templates, since `templates/crds.yaml` globs `definitions/*.yaml`, and Helm pre-applies only a `crds/` directory. Helm then resolves every document in the release manifest through the cluster's RESTMapper in `Build()` before it applies any of them, so an ungated Strategy CR has no mapping on the first install, `Build()` fails, and the release applies **zero** objects, including the CRDs that would have made the mapping resolve. Nothing recovers from that. Every retry renders the same manifest, so it is permanent rather than a race, and Helm's kind sorter cannot help because it orders the apply and the failure is before the apply. The install log shows `Warning InstallFailed 43s (x36 over 20m)` and then `Install failed (no retry)`. ### Why the seven siblings never hit this Each of them gates on a resolved `$bucketName`, which is empty until the COSI BucketClaim status is reconciled, so they are simply absent from the first render. By the time `DefaultObjectsGate` forces a second revision the CRDs from revision 1 exist and the mapping resolves. That gate is documented as "the CR needs the bucket name", and it silently doubles as the CRD-ordering guard. MongoDB genuinely does not need the bucket name, the template argues that correctly, so the gate was left off and the accidental protection went with it. This gates it too, for the ordering reason rather than the value, and records that reason in the template so the next bucket-independent strategy keeps it. Convergence is unchanged. `DefaultObjectsGate` already routes `apps.cozystack.io/MongoDB` to `cozy-default-mongodb` and forces a revision once any routed object is missing, which is exactly how the seven siblings materialise today. ### Why this was invisible `tests/rendering_test.yaml` asserted `count: 1` for the MongoDB template under a case named "bootstrap window (no resolvable bucket name)", so the defect was codified as the expected behaviour and shipped green. helm-unittest has no RESTMapper and cannot see the consequence. That assertion is flipped here, with the ordering reason written next to it. It was also masked in CI until yesterday by #3806. `StreamVisitor.Visit` returns immediately on a validation error but only continues on a mapping error, so the OpenAPI defect aborted the manifest stream at the first document and the mapping check was never reached. #3808 fixed that and this surfaced underneath it. ### Screenshots Not applicable. ### Downstream repositories - [x] No downstream repository is affected by this change ### Testing Verified by rendering both windows, since the whole bug is which documents appear in the first render and a cluster adds nothing to that. Before, bootstrap window: one Strategy document, `MongoDB`, alongside the eight not-yet-applied CRDs. After: none. With `backupStorage.bucketNameOverride=b`, which stands in for a resolved BucketClaim: all eight, MongoDB included. ``` helm template bsc packages/system/backupstrategy-controller | grep -c '^kind: MongoDB$' ``` `helm unittest packages/system/backupstrategy-controller` passes, 4 suites, 18 tests. ### Follow-up, not in this PR The durable fix is to stop shipping these CRDs as templates and split `definitions/` into a `backupstrategy-controller-crds` package with a `dependsOn` edge, which is the convention six other packages already follow. That cannot be a hotfix: none of the eight definitions carries `helm.sh/resource-policy: keep`, so the upgrade that removes them from this chart's manifest would have Helm delete the eight CRDs and cascade-delete every live Strategy CR on existing clusters. It needs the `keep` annotation shipped a release ahead. ### Release note ```release-note fix(backupstrategy-controller): gate the default MongoDB backup strategy on the bootstrap window, so a fresh platform install no longer fails with "no matches for kind MongoDB" and applies nothing ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented the default MongoDB backup strategy from rendering before the required S3 storage configuration is available. * Improved first-install reliability by avoiding premature strategy resource creation. * **Tests** * Updated bootstrap validation to confirm the MongoDB strategy is omitted until a bucket name is resolved. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Adds a native MongoDB (Percona Server for MongoDB / psmdb) backup-strategy driver so
apps.cozystack.io/MongoDBreaches feature parity with the CNPG, MariaDB and ClickHouse native drivers — logical backups, restore into a differently-named instance, and point-in-time recovery — all through the unifiedBackupJob/RestoreJob/BackupClass/Planinterface (closes #3545).The driver orchestrates the operator's
PerconaServerMongoDBBackup/PerconaServerMongoDBRestoreCRDs:PerconaServerMongoDBBackup, pollsstatus.state, and persists the S3 destination + storage descriptor (by reference — Secret name only, never a raw credential) onto the CozystackBackupartifact.PerconaServerMongoDBRestoreviabackupSource(read back from the source backup status, with a snapshot fallback for a reaped operator CR) so the same artifact restores in-place or into a differently-named instance. PITR is exposed asspec.options.recoveryTime(RFC3339), matching the CNPG driver's option exactly; unrecognisedspec.optionskeys are surfaced as aUnknownRestoreOptionWarning event rather than silently dropped.Because psmdb resolves the backup storage by name against the cluster's
spec.backup.storages, the driver names the chart's storage (s3-storage) via thecozy-default-mongodbStrategy rather than injecting it, and surfaces a clearReady=Falseprecondition when a target has backups disabled instead of hanging until the deadline.Wiring
api/backups/strategy/v1alpha1:MongoDBstrategy type + generated CRD/deepcopy.internal/backupcontroller:psmdbstrategy_controller+psmdbtypes/mongodbapppartial-schema mirrors; BackupJob/RestoreJob dispatch, supported-kinds, cleanup-on-delete branch, and scheme registration.backupstrategy-controller:cozy-defaultMongoDB route,cozy-default-mongodbStrategy CR, andpsmdb.percona.comRBAC (get/list/watch, +create on backups/restores, no delete — the driver does not own the S3 archive).apps/mongodb: abackup.insecureSkipTLSVerifyvalue (psmdb's s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed in-cluster seaweedfs. Defaultfalse, fully backward-compatible; regenerated schema/README/RD.Tests + e2e
backupSourceresolution (live-preferred / snapshot fallback / no-destination), snapshot round-trip, the producer-seam (createMongoDBBackupArtifact) driver-metadata, the cleanup-on-delete no-Velero-fall-through guard, andrecoveryTime→ psmdb-UTC mapping + unknown-option reporting.examples/backups/mongodb/+ amongodb-2-backup-roundtripChainsaw suite that drivesrun-all.shend-to-end (a sentinel document round-trips through S3 into a restored copy while the source stays untouched), perdocs/agents/e2e-testing.md.docs/operations/backup-classes.mdupdated (new route, the app-owns-the-storage model, and therecoveryTimePITR usage).Verified end-to-end on a live dev cluster against the platform-default
cozy-backupsbucket: backup viacozy-defaultreached the bucket and the CozystackBackupwentReady, and a to-copyRestoreJobrestored the sentinel into a differently-named instance with the source left intact.Screenshots
N/A — no UI changes (controller, chart values, CRD, docs and tests only).
Downstream repositories
Walked the trigger map in
docs/agents/contributing.mdfile by file against the diff:cozystack/website — the only app-facing change is
packages/apps/mongodb(a newbackup.insecureSkipTLSVerifyvalue + regeneratedREADME.md). The managed-app reference page is regenerated from the packageREADME.mdby the release bot for apps already in the websiteMakefile(mongodb is one), so it refreshes automatically on the next tag — no manual follow-up.docs/operations/backup-classes.mdis not a website-mirrored doc in the trigger map.cozystack/terraform-provider-cozystack — the one candidate trigger ("a field added to an app's
values.schema.json") lands inside the mongodbbackupblock, which the provider'sinternal/provider/mongodb_schema.goexplicitly states it does not manage ("The deprecated backup block, the bootstrap restore block, and detailed shardingConfig are not managed"). No enum, default,release.prefix, Secret/Service name, or hand-typed CRD (Plan,RestoreJob) changed; the newstrategy.backups.cozystack.io/MongoDBCRD is opaque to the provider. Not affected.All other repositories — no matching trigger (no
hack/layout or make-target change, no namespace rename, noApplicationDefinitionsemantics change, nopackages/core/platformvalues, no telemetry metric orcozy-proxylabel rename).No downstream repository is affected by this change
cozystack/website - follow-up:
cozystack/terraform-provider-cozystack - follow-up:
cozystack/ansible-cozystack - follow-up:
cozystack/ccp - follow-up:
cozystack/talm - follow-up:
cozystack/cozyhr - follow-up:
cozystack/cozy-proxy - follow-up:
cozystack/cozystack-telemetry-server - follow-up:
cozystack/external-apps-example - follow-up:
cozystack/examples - follow-up:
Release note
Summary by CodeRabbit
New Features
Documentation
Tests