Skip to content

feat(backups): add a native MongoDB (psmdb) backup-strategy driver - #3562

Merged
IvanHunters merged 7 commits into
mainfrom
feat/3545-mongodb-backup-driver
Aug 13, 2026
Merged

IvanHunters merged 7 commits into
mainfrom
feat/3545-mongodb-backup-driver

Conversation

@androndo

@androndo Andrey Kolkov (androndo) commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

Adds a native 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 into a differently-named instance, and point-in-time recovery — all 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 — Secret name only, never a raw credential) onto the Cozystack Backup artifact.
  • RestoreJob: drives PerconaServerMongoDBRestore via backupSource (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 as spec.options.recoveryTime (RFC3339), matching the CNPG driver's option exactly; unrecognised spec.options keys are surfaced as a UnknownRestoreOption Warning 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 the cozy-default-mongodb Strategy rather than injecting it, and surfaces a clear Ready=False precondition when a target has backups disabled instead of hanging until the deadline.

Wiring

  • api/backups/strategy/v1alpha1: MongoDB strategy type + generated CRD/deepcopy.
  • internal/backupcontroller: psmdbstrategy_controller + psmdbtypes / mongodbapp partial-schema mirrors; BackupJob/RestoreJob dispatch, supported-kinds, cleanup-on-delete branch, and scheme registration.
  • backupstrategy-controller: cozy-default MongoDB route, cozy-default-mongodb Strategy CR, and psmdb.percona.com RBAC (get/list/watch, +create on backups/restores, no delete — the driver does not own the S3 archive).
  • apps/mongodb: a backup.insecureSkipTLSVerify value (psmdb's s3 storage has no CA-bundle field) so backups can target Cozystack's self-signed in-cluster seaweedfs. Default false, fully backward-compatible; regenerated schema/README/RD.

Tests + e2e

  • Unit tests for the driver: applicationRef validation, ensure-idempotency (backup + restore), the backups-enabled/storage precondition gate, backupSource resolution (live-preferred / snapshot fallback / no-destination), snapshot round-trip, the producer-seam (createMongoDBBackupArtifact) driver-metadata, the cleanup-on-delete no-Velero-fall-through guard, and recoveryTime → psmdb-UTC mapping + unknown-option reporting.
  • examples/backups/mongodb/ + a mongodb-2-backup-roundtrip Chainsaw suite that drives run-all.sh end-to-end (a 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 (new route, the app-owns-the-storage model, and the recoveryTime PITR usage).

Verified end-to-end on a live dev cluster against the platform-default cozy-backups bucket: backup via cozy-default reached the bucket and the Cozystack Backup went Ready, and a to-copy RestoreJob restored 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.md file by file against the diff:

  • cozystack/website — the only app-facing change is packages/apps/mongodb (a new backup.insecureSkipTLSVerify value + regenerated README.md). The managed-app reference page is regenerated from the package README.md by the release bot for apps already in the website Makefile (mongodb is one), so it refreshes automatically on the next tag — no manual follow-up. docs/operations/backup-classes.md is 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 mongodb backup block, which the provider's internal/provider/mongodb_schema.go explicitly 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 new strategy.backups.cozystack.io/MongoDB CRD is opaque to the provider. Not affected.

  • All other repositories — no matching trigger (no hack/ layout or make-target change, no namespace rename, no ApplicationDefinition semantics change, no packages/core/platform values, no telemetry metric or cozy-proxy label 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

feat(backups): add a native MongoDB (psmdb) backup-strategy driver — logical backup to the cozy-default BackupClass, restore into a differently-named instance, and point-in-time recovery via spec.options.recoveryTime

Summary by CodeRabbit

  • New Features

    • Added MongoDB backup and restore support, including scheduled and on-demand backups, restores to new or existing clusters, and point-in-time recovery.
    • Added configurable S3 TLS certificate verification for MongoDB backups, defaulting to secure verification.
    • Added MongoDB support to the default backup configuration.
  • Documentation

    • Added MongoDB backup and restore examples, workflows, and operational guidance.
  • Tests

    • Added automated coverage for MongoDB backup, restore, cleanup, and end-to-end scenarios.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: eff3b2dc-3613-4d21-a807-ef7da5a14e76

📥 Commits

Reviewing files that changed from the base of the PR and between b9efc6a and 752bea3.

📒 Files selected for processing (3)
  • docs/operations/backup-classes.md
  • internal/backupcontroller/psmdbstrategy_controller.go
  • internal/backupcontroller/psmdbstrategy_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/operations/backup-classes.md
  • internal/backupcontroller/psmdbstrategy_controller_test.go
  • internal/backupcontroller/psmdbstrategy_controller.go

📝 Walkthrough

Walkthrough

Adds 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.

Changes

MongoDB native backup strategy

Layer / File(s) Summary
API and operator contracts
api/backups/strategy/v1alpha1/*, internal/backupcontroller/mongodbapp/*, internal/backupcontroller/psmdbtypes/*, packages/system/backupstrategy-controller/definitions/*
Defines MongoDB strategy resources and typed Percona backup and restore resources. Registers schemes and adds deep-copy implementations. Adds the MongoDB strategy CRD.
Strategy wiring and defaults
cmd/backupstrategy-controller/main.go, packages/system/backupstrategy-controller/templates/*, packages/system/backupstrategy-controller/tests/rendering_test.yaml, packages/apps/mongodb/*, packages/system/mongodb-rd/cozyrds/mongodb.yaml, docs/operations/backup-classes.md
Routes MongoDB through cozy-default-mongodb, adds RBAC and rendering coverage, registers runtime types, and exposes configurable S3 TLS verification with a default of false.
Backup and restore controller flow
internal/backupcontroller/psmdbstrategy_controller.go, internal/backupcontroller/backupjob_controller.go, internal/backupcontroller/restorejob_controller.go, internal/backupcontroller/backup_controller.go
Reconciles Percona backup and restore resources, persists backup artifacts, supports restore targets and PITR, applies deadlines, and skips Velero cleanup for MongoDB resources.
Controller validation coverage
internal/backupcontroller/psmdbstrategy_controller_test.go, internal/backupcontroller/backupjob_controller_test.go, internal/backupcontroller/backup_controller_test.go
Tests validation, defaults, preconditions, idempotency, snapshot fallback, restore options, artifact metadata, dispatch, and cleanup behavior.
Backup and restore examples
examples/backups/mongodb/*, hack/e2e-chainsaw/mongodb/chainsaw-test.yaml
Adds MongoDB source and target resources, helper and lifecycle scripts, operational documentation, a complete round-trip runner, cleanup logic, and Chainsaw coverage.

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
Loading

Suggested labels: area/database, area/testing

Suggested reviewers: lllamnyp

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.31% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the primary change: adding a native MongoDB PSMDB backup-strategy driver.
Linked Issues check ✅ Passed The changes add the MongoDB strategy, PSMDB orchestration, dispatch, BackupClass routing, restore support, PITR, tests, and documentation required by issue [#3545].
Out of Scope Changes check ✅ Passed The API, application configuration, examples, documentation, tests, RBAC, and cleanup changes directly support the MongoDB backup-strategy driver objectives.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/3545-mongodb-backup-driver

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/feature Categorizes issue or PR as related to a new feature size/XXL This PR changes 1000+ lines, ignoring generated files labels Aug 5, 2026
@androndo
Andrey Kolkov (androndo) marked this pull request as ready for review August 6, 2026 05:30

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
internal/backupcontroller/psmdbstrategy_controller_test.go (1)

577-619: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consider adding reconcile-loop coverage.

The helpers build a fake client that already registers psmdbtypes, mongodbapp, and the status subresources. That is everything reconcileMongoDB and reconcileMongoDBRestore need.

Neither reconcile function is exercised. The untested paths include the StartedAt persistence 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 PerconaServerMongoDB and a PerconaServerMongoDBBackup in each operator state, then assert the resulting BackupJob phase 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 value

Clarify 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 PerconaServerMongoDBRestore CR in the tenant namespace, and that CR outlives the RestoreJob. 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

📥 Commits

Reviewing files that changed from the base of the PR and between df157da and 9eb8f09.

📒 Files selected for processing (39)
  • api/apps/v1alpha1/mongodb/types.go
  • api/backups/strategy/v1alpha1/mongodb_types.go
  • api/backups/strategy/v1alpha1/zz_generated.deepcopy.go
  • cmd/backupstrategy-controller/main.go
  • docs/operations/backup-classes.md
  • examples/backups/mongodb/00-bucket.yaml
  • examples/backups/mongodb/00-helpers.sh
  • examples/backups/mongodb/05-mongodb-src.yaml
  • examples/backups/mongodb/10-backupjob-adhoc.yaml
  • examples/backups/mongodb/15-plan.yaml
  • examples/backups/mongodb/20-mongodb-target.yaml
  • examples/backups/mongodb/25-restorejob-in-place.yaml
  • examples/backups/mongodb/30-restorejob-to-copy.yaml
  • examples/backups/mongodb/README.md
  • examples/backups/mongodb/cleanup.sh
  • examples/backups/mongodb/run-all.sh
  • hack/e2e-chainsaw/mongodb/chainsaw-test.yaml
  • internal/backupcontroller/backup_controller.go
  • internal/backupcontroller/backup_controller_test.go
  • internal/backupcontroller/backupjob_controller.go
  • internal/backupcontroller/backupjob_controller_test.go
  • internal/backupcontroller/mongodbapp/types.go
  • internal/backupcontroller/mongodbapp/zz_generated.deepcopy.go
  • internal/backupcontroller/psmdbstrategy_controller.go
  • internal/backupcontroller/psmdbstrategy_controller_test.go
  • internal/backupcontroller/psmdbtypes/types.go
  • internal/backupcontroller/psmdbtypes/zz_generated.deepcopy.go
  • internal/backupcontroller/restorejob_controller.go
  • packages/apps/mongodb/README.md
  • packages/apps/mongodb/templates/mongodb.yaml
  • packages/apps/mongodb/templates/restore.yaml
  • packages/apps/mongodb/values.schema.json
  • packages/apps/mongodb/values.yaml
  • packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mongodbs.yaml
  • packages/system/backupstrategy-controller/templates/backupclass-default.yaml
  • packages/system/backupstrategy-controller/templates/rbac.yaml
  • packages/system/backupstrategy-controller/templates/strategy-mongodb-default.yaml
  • packages/system/backupstrategy-controller/tests/rendering_test.yaml
  • packages/system/mongodb-rd/cozyrds/mongodb.yaml

Comment on lines +3 to +10
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

Comment on lines +3 to +5
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

Comment on lines +426 to +460
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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"
done

Repository: 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.go

Repository: 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.

Comment on lines +3 to +6
// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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 IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-default for 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-projected cozy-backups-creds. Selecting the cozy-default BackupClass is not self-service the way the name implies; the app must first be deployed with backup.enabled=true and a full backup block. 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/mongodb gaining backup.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 with backup.enabled=true through cozystack-pr-test if a live signal is wanted.

Recommended follow-ups

  • Persist strategy-CR status.conditions (the MongoDBStatus godoc promises diagnostics that no code writes) or drop the promise.
  • examples/backups/mongodb/run-all.sh sed-based substitution corrupts manifests if a COSI-issued S3 key contains |, &, or \; cleanup.sh prunes only three hard-coded owner names, leaving Plan-stamped CRs behind (re-arms the stale-reuse trap).

Andrey Kolkov (androndo) added a commit that referenced this pull request Aug 6, 2026
… 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]>
Andrey Kolkov (androndo) added a commit that referenced this pull request Aug 11, 2026
… 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]>
@androndo
Andrey Kolkov (androndo) force-pushed the feat/3545-mongodb-backup-driver branch from da383c7 to b9efc6a Compare August 11, 2026 09:20
@IvanHunters

Copy link
Copy Markdown
Collaborator

Verdict

LGTM 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 (go build/go vet clean, go test ./internal/backupcontroller/... green with a non-vacuous mutation check, helm template/helm unittest corners admittable). I found one MINOR robustness asymmetry plus a few items surfaced by a second independent reviewer, none blocking.

Findings

[MINOR] internal/backupcontroller/psmdbstrategy_controller.go:748-750 (via caller :614-617) — a transient error resolving the live source backup terminally fails the RestoreJob.

resolveMongoDBBackupSource returns an error on any non-NotFound error from r.Get of the live PerconaServerMongoDBBackup, and the caller turns that into markRestoreJobFailed (terminal). The backup path treats the symmetric case as transient (return ctrl.Result{}, err, i.e. requeue). So a transient read error (cache not yet synced, apiserver hiccup) on the source-backup lookup fails a RestoreJob that would have succeeded on retry, instead of requeuing. Window is narrow (controller-runtime cache reads), hence MINOR. Suggested shape: on !IsNotFound, return ctrl.Result{}, err (requeue) rather than folding it into a terminal failure; only a genuinely absent live CR should fall through to the snapshot path. If this deliberately mirrors a MariaDB/CNPG convention where source-CR read errors are terminal, downgrade to a non-issue.

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 BackupJob/RestoreJob can report Succeeded without taking a fresh dump. Confirmed mechanism: operator CRs are created with labels only and no OwnerReferences (psmdbstrategy_controller.go:398-413), the RBAC grant is create-only (get,list,watch,create, no delete/patch in packages/system/backupstrategy-controller/templates/rbac.yaml), findMongoDBBackupForJob returns &list.Items[0] (:430-453) and the ready branch fires immediately, while createMongoDBBackupArtifact names the artifact after the job and returns the OLD one on AlreadyExists (:495-519). So deleting and re-creating a BackupJob under the exact same name adopts the leftover ready CR and settles Succeeded in seconds with no new backup. This is inherited verbatim from the shipped MariaDB driver (mariadbstrategy_controller.go:289-306, identical create-only RBAC) and is not a regression this PR introduces; scheduled Plan-created jobs are immune (unique <plan>-<unixMinute> names + Plan ownerRef). Best tracked as a separate platform-wide hardening issue (add a generation/creationTimestamp freshness guard, or ownerRef+GC), not a blocker here.

[MINOR] to-copy restore across a different S3 bucket — psmdbTargetCredentialsSecret (:336-355, new logic in this driver) swaps in the target cluster's S3 credentials while keeping the source bucket/endpoint (:662-668, gated on cred != "" && cred != source). This is the correct default for the documented same-bucket DR case, but a to-copy target whose credentials are scoped to a different S3 account/bucket cannot read the source bucket, so the restore fails with a loud AccessDenied → RestoreJob Failed (:691-696). New-feature edge case, fails loudly (no silent corruption). Worth a one-line doc note stating the target's creds must be able to read the source bucket. (Both reviewers independently flagged this credential re-pointing behaviour — convergence, so it is the main operator-facing sharp edge.)

[NIT] Swallowed template errors — internal/template/template.go:21-27 returns the literal template string when execution errors. A cluster-scoped (admin-only) Strategy referencing {{ .Application.spec.* }} (MongoDBSpec is intentionally an empty struct, internal/backupcontroller/mongodbapp/types.go:66) yields a garbled precondition message (storage "{{...}}" not declared) rather than a clear error. Admin-only, fails loudly, shared pre-existing helper across all drivers — cosmetic.

Claim mismatches

[DROP / false-positive] cozy_invariant template-comment-bloat @ packages/system/backupstrategy-controller/templates/strategy-mongodb-default.yaml:1-15. Verified via helm template that this is a {{/* ... */}} Go-template comment (stripped before render — grep of the rendered output returns 0), not a # YAML comment. The invariant's operational cost does not apply; it is a legitimate design-rationale comment. Not a finding.

[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

  • Upgrade (N-1): additive only. No migration is required and none is present (repo invariants clean, no packages/core/platform change). The one existing-object surface — the PerconaServerMongoDB CR rendered by packages/apps/mongodb — is byte-identical after the change: insecureSkipTLSVerify now renders from a values default of false, confirmed by render to equal the previously hardcoded false, so there is no SSA/immutable-field drift on existing MongoDB apps. The new strategy.backups.cozystack.io/MongoDB CRD, the cozy-default-mongodb Strategy CR, and the cozy-default BackupClass MongoDB binding are all new/additive.
  • Fresh install: the mongodbs CRD ships in the same chart as its unconditionally-rendered default MongoDB Strategy CR. This CRD+CR-in-one-apply ordering is the pre-existing pattern shared by all sibling strategies in this chart, not introduced here.
  • Live-cluster verification of the upstream psmdb PerconaServerMongoDBBackup/Restore admission and the actual round-trip was not executed here (hermetic review). Residual risk is low given the additive, no-upgrade-state-rewrite nature of the change.

Recommended follow-ups

  • Optional cozystack-pr-test on a dev cluster to exercise the BackupJob → Backup → RestoreJob(to-copy) round-trip through the real psmdb operator, confirming the partial psmdbtypes Create shapes are accepted by admission (the one class a static render cannot prove), and to smoke-test the PITR (recoveryTime) path, which is documented but not exercised by the unit tests or the chainsaw suite.
  • Consider a separate hardening issue for the platform-wide stale-CR-adoption pattern noted above (affects MariaDB and MongoDB drivers equally).

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]>
@androndo
Andrey Kolkov (androndo) force-pushed the feat/3545-mongodb-backup-driver branch from 752bea3 to b496fb1 Compare August 12, 2026 08:01

@lllamnyp Timofei Larkin (lllamnyp) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the API gate, but please get this approved with the relevant section's code owner.

@IvanHunters
IvanHunters merged commit 09dc7f4 into main Aug 13, 2026
41 of 44 checks passed
@IvanHunters
IvanHunters deleted the feat/3545-mongodb-backup-driver branch August 13, 2026 08:18
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Aug 14, 2026
…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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/feature Categorizes issue or PR as related to a new feature size/XXL This PR changes 1000+ lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(backups): add a native MongoDB (psmdb) backup-strategy driver

3 participants