feat(backups): RabbitMQ definitions backup via a dedicated strategy driver - #3990

sequenceDiagram
  participant Operator
  participant BackupJobReconciler
  participant RabbitmqCluster
  participant BackupJob
  participant S3
  Operator->>BackupJobReconciler: Create BackupJob
  BackupJobReconciler->>RabbitmqCluster: Check AllReplicasReady
  RabbitmqCluster-->>BackupJobReconciler: Ready
  BackupJobReconciler->>BackupJob: Create export Job
  BackupJob->>S3: PUT definitions.json
  BackupJobReconciler->>Operator: Mark BackupJob succeeded

sequenceDiagram
  participant Operator
  participant RestoreJobReconciler
  participant RabbitmqCluster
  participant RestoreJob
  participant S3
  Operator->>RestoreJobReconciler: Create RestoreJob
  RestoreJobReconciler->>RabbitmqCluster: Check target readiness
  RabbitmqCluster-->>RestoreJobReconciler: Ready
  RestoreJobReconciler->>RestoreJob: Create import Job
  RestoreJob->>S3: GET definitions.json
  RestoreJob->>RabbitmqCluster: POST definitions
  RestoreJobReconciler->>Operator: Mark RestoreJob succeeded

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 @coderabbitai help to get the list of available commands.}

Thanks for the thorough review — this was exactly the right set of things to catch. Addressed in 534e20bd2:

Blockers

• Restore now reads the recorded URI, not a reconstruction. The artifact URI is the single source of truth: the driver renders artifactURITemplate once, injects it into the backup Job (which writes exactly that object) and records it on status.artifact.uri; restore reads that value back and fetches the same object. A strategy that stores backups must now set artifactURITemplate (the backup fails loud otherwise), and a restore of a Backup with no recorded URI fails with a clear message instead of 404-ing on a reconstructed key — matching the Etcd driver's "the recorded URI is authoritative" stance you pointed at.
• Restore-is-a-merge is now documented where operators look. docs/operations/backup-classes.md (the Rabbitmq tuning entry) and the CRD field docs (rabbitmq_types.go) now both state that POST /api/definitions creates or updates and never deletes, so objects created after the backup survive and the broker is not returned to its exact backup-time state, plus the to-copy user-import note. (The example README already had it.)

Non-blocking

• forcePathStyle is now plumbed through (S3_FORCE_PATH_STYLE); the script builds a virtual-hosted-style URL when it is false, path-style otherwise.
• The object key is no longer authored twice: the shell no longer rebuilds a key at all — it splits ARTIFACT_URI — so the Go template is the only place the layout lives, and the divergence you flagged can't happen.
• A same-named Backup is adopted only when its applicationRef matches; otherwise the backup fails rather than reporting success against a Backup describing a different application.
• The credentials-on-argv and basic-auth-over-plain-HTTP nits I left as-is for now since they're platform-wide (Altinity does the same) — glad to take them in a follow-up if you'd prefer they land here.

Separate issue

Filed the RabbitMQ 4.2 t1.nano OOM as #4003 — a default RabbitMQ install never starts on current main.

New unit tests cover the recorded-URI restore render, the no-recorded-URI and no-artifactURITemplate failures, and the distinct-key guard; go test + helm unittest are green.

The driver itself looks solid — the readiness gate fails closed, credentials are env-from-secret only (nothing lands in the Job spec or logs), the to-copy restore correctly pairs the target's credentials with the source's recorded object key, and the recorded-URI contract plus the Backup-adoption guard are improvements over the Altinity template this follows. A few things I'd like addressed before merge, none structural:

1. S3 object lifecycle. cleanupOnDelete in internal/backupcontroller/backup_controller.go has an explicit branch per strategy kind documenting each archive-ownership contract; Rabbitmq is missing and falls into the Velero-fallback default. Unlike the engines with documented no-ops, nothing anywhere ever deletes the definitions.json objects this driver writes — a daily Plan with retention prunes Backup CRs forever while the bucket accumulates one object per run, unboundedly. At minimum add the explicit branch plus a line in backup-classes.md; ideally delete the object at status.artifact.uri, since this is the one driver that unambiguously owns its artifact and has the exact key recorded.

2. Tenant-facing scope disclosure. The definitions-only boundary is stated well in the CRD godoc, backup-classes.md, and the example README — but packages/apps/rabbitmq/README.md, the doc a tenant actually reads, says nothing about backups at all (compare ClickHouse's "Backup orchestration" section). A tenant creating a Plan against cozy-default has no visible signal that message payloads aren't included. A short "Backups" section stating definitions-only / no messages / restore-is-merge would close it.

3. Unit-test env contradicts the real CRD on the load-bearing property. newRabbitmqTestEnv registers Backup in WithStatusSubresource(...), but the real Backup CRD has no status subresource — that absence is why persist-on-Create works. TestCreateRabbitmqBackupArtifact_ArtifactURI then asserts on the returned struct rather than a re-Get, so it passes even though the fake API server drops the status it claims to verify. If a status subresource is ever added to Backup, every restore breaks and no unit test notices. Drop Backup from WithStatusSubresource and re-Get in that test.

4. Nit: hack/image-pin-consistency.bats pins .migrations.image and .chBackupClientImage to the exact same string; rabbitmqBackupClientImage is stamped in lockstep but not covered by that test. Same rationale (the tag string is what an operator reads off the rendered pod spec) — one line to extend.

And to confirm the merge gate already implied in the description: a green rabbitmq-2-backup-roundtrip run (PR e2e or a posted dev-cluster transcript). The roundtrip itself is honest — sentinel definition seeded, dropped, restored in place and to-copy with a source-untouched assertion — so a green run is meaningful.

Thanks — all four addressed (217f92976).

1. S3 object lifecycle. Added an explicit Rabbitmq branch to cleanupOnDelete. Since this driver uniquely owns its object and has the exact key on status.artifact.uri, it now deletes it — not just documents a no-op. The controller has no S3 client, so the delete runs as a one-shot Job through the strategy's curl image in a new cleanup mode (s3 -X DELETE, exiting before any management-API access so a gone broker doesn't matter; the broker secretKeyRefs are now optional). The Job is ownerless (the Backup finalizer is removed as soon as cleanup returns, so a Backup-owned Job would be GC'd before running) with a short activeDeadlineSeconds + ttlSecondsAfterFinished. It is deliberately fire-and-forget: every error path returns nil, so it never blocks Backup deletion — a failed delete leaks the object rather than wedging the finalizer, no worse than the operator-backed drivers. backup-classes.md now states the ownership/lifecycle contract, and there are unit tests for the spawn (ownerless, TTL, --mode=cleanup) and the no-artifact no-op.
2. Tenant-facing scope. Added a "## Backups" section to packages/apps/rabbitmq/README.md (definitions-only / no messages / restore-is-merge), placed like ClickHouse's before the generated Parameters block.
3. Test env vs real CRD. Dropped Backup from WithStatusSubresource in newRabbitmqTestEnv (the real Backup CRD has none — that's why persist-on-Create works), and TestCreateRabbitmqBackupArtifact_ArtifactURI now re-Gets and asserts the persisted status.artifact.uri, so a status subresource ever added to Backup would fail it.
4. Pin consistency. Extended hack/image-pin-consistency.bats to cover rabbitmqBackupClientImage (a == b == c); passes 2/2.

On the merge gate: I ran the rabbitmq-2-backup-roundtrip flow on a dev cluster earlier — backup + in-place + to-copy, with the source-untouched assertion — green; happy to post the transcript if useful while the PR e2e settles.

Revised the object cleanup from fire-and-forget to waiting (3581c8ea0): the Backup's deletion now blocks until the delete Job actually succeeds, so no object is orphaned. cleanupOnDelete returns a ctrl.Result; the Rabbitmq branch requeues while the delete Job runs and only releases the finalizer once it completes. The delete script no longer swallows failures — it inspects the HTTP status, treats a missing object as success (idempotent), and exits non-zero on any real error, so a genuinely failing delete is retried and the Backup stays Terminating (a visible signal) rather than silently leaking the object. Updated backup-classes.md and the unit tests (create → requeue → complete → reaped) accordingly.

Verdict

NOT LGTM

The Backup finalizer path now has to CREATE objects in the application namespace, which cannot succeed while that namespace is terminating, and every failure there is treated as retryable, so a namespace holding a Rabbitmq Backup can no longer finish deleting.

Findings

[MAJOR] Backup cleanup creates objects in the namespace, so namespace teardown deadlocks

internal/backupcontroller/rabbitmqstrategy_controller.go:733,746

An operator deletes a tenant (or any namespace) that holds at least one Rabbitmq Backup. The namespace goes Terminating, the Backup gets a deletionTimestamp but keeps backups.cozystack.io/cleanup, and cleanupRabbitmqBackup takes the IsNotFound branch for its cleanup Job:

if perr := ProjectBackupCredentials(ctx, r.Client, r.CredentialsConfig, backup.Namespace); perr != nil {
    return ctrl.Result{}, perr
}
...
if cerr := r.Create(ctx, buildRabbitmqCleanupJob(backup.Namespace, jobName, rendered)); cerr != nil && !apierrors.IsAlreadyExists(cerr) {
    return ctrl.Result{}, cerr
}

ProjectBackupCredentials does a CreateOrUpdate on a Secret (credentials_projector.go:134) and then the Job is created outright. The NamespaceLifecycle admission plugin rejects CREATE in a terminating namespace, and Forbidden is neither NotFound nor AlreadyExists, so the error propagates to backup_controller.go:64-67, which logs it and returns it. controller-runtime then retries with backoff forever, the finalizer is never removed, and the namespace stays Terminating with NamespaceContentRemaining: backups.backups.cozystack.io.

This is a behaviour change on a path that works today. The Velero and CNPG cleanup branches only ever DELETE, and DELETE is permitted in a terminating namespace.

This repository already knows the mechanism and guards against it elsewhere. internal/controller/wildcardsecret/reconciler.go:341-344 skips namespaces carrying a DeletionTimestamp, and the test at internal/controller/wildcardsecret/reconciler_test.go:608-613 spells out why: "the API server forbids creating a replica in a Terminating namespace ... without the skip every tenant teardown would produce forbidden-error requeue churn".

Three other inputs reach the same wedge through the same "any error is retryable" treatment: the platform credentials Secret being gone makes ProjectBackupCredentials return ReasonSourceMissing rather than a skip; a pre-existing unlabelled cozy-backups-creds trips the projector's ownership guard; and a Backup name over 55 characters makes <name>-cleanup exceed the 63-character Job name limit, so the Create is rejected as Invalid. BackupJob names carry no length cap in backups.cozystack.io_backupjobs.yaml.

The backups.cozystack.io/skip-artifact-cleanup escape hatch does clear it, since UPDATE is still allowed on a terminating namespace, but it has to be applied per Backup by an operator who already knows to look, and docs/operations/backup-classes.md:247 only presents it as the remedy for unreachable object storage.

Fix: classify the failure instead of retrying it. Read the target namespace (or match apierrors.IsForbidden / IsInvalid on the Create) and treat "cannot create the cleanup Job here" as terminal: log it, emit a Warning Event naming the object key that is being left behind, and return a zero ctrl.Result so the finalizer is released. Test that is red today and green after: a BackupReconciler.Reconcile case where the namespace carries a DeletionTimestamp (or a fake client whose Job Create returns Forbidden), asserting the finalizer is gone and the Event was recorded.

[MAJOR] The RabbitmqCluster readiness gate never becomes terminal

internal/backupcontroller/rabbitmqstrategy_controller.go:255

Both the backup and the restore path defer on cluster readiness with nothing that converts the wait into a failure:

// :255 (backup), mirrored at :615 (restore)
if msg := rabbitmqNotReadyMessage(cluster); msg != "" {
    return r.requeueRabbitmqBackupWaiting(ctx, j, "RabbitMQClusterNotReady", msg)
}

requeueRabbitmqBackupWaiting (:330) sets Ready=False and returns RequeueAfter: 5s, unconditionally. A broker that never reaches AllReplicasReady (PVC stuck Pending, OOMKilled, an undersized preset) leaves the BackupJob in phase=Running indefinitely. PlanReconciler has no concurrency policy: plan_controller.go:84 creates a fresh BackupJob on every cron fire regardless of what is still in flight, so each tick adds another job that also never terminates. An operator alerting on status.phase == Failed sees nothing, and a backup that is not happening is indistinguishable from one in progress.

Every sibling driver with a precondition gate bounds it. psmdbstrategy_controller.go:66 defines psmdbDefaultBackupDeadline = 30 * time.Minute and applies it at :185, :213 and :224; cnpgstrategy_controller.go:152 does the same; the generic StrategyNotReadyDeadline at backupjob_controller.go:305 covers the missing-strategy wait for both job types. The comment at :328-329 says this helper "Mirrors requeueMongoDBBackupWaiting", but the mirror dropped the deadline check that makes the MongoDB version terminal.

Fix: add a rabbitmqBackupDeadlineExceeded(j.Status.StartedAt) in the shape of psmdbBackupDeadlineExceeded and call markBackupJobFailed past it at both call sites, with the not-ready message carried into the failure. Test: reconcile with Status.StartedAt set 31 minutes back and a cluster reporting AllReplicasReady=False, asserting phase=Failed.

What would change my mind: an outer deadline on BackupJob that I missed. I grepped Deadline|deadline across internal/backupcontroller/ and the only bounds are the per-driver constants above plus StrategyNotReadyDeadline, none of which covers this branch.

[MINOR] Rabbitmq is missing from the RestoreJob cleanup no-op list

internal/backupcontroller/restorejob_controller.go:334

The dispatch at :169 was added, but the companion enumeration in cleanupOnDelete was not:

case strategyv1alpha1.CNPGStrategyKind, strategyv1alpha1.JobStrategyKind, strategyv1alpha1.AltinityStrategyKind, strategyv1alpha1.MariaDBStrategyKind, strategyv1alpha1.MongoDBStrategyKind, strategyv1alpha1.FoundationDBStrategyKind, strategyv1alpha1.EtcdStrategyKind:

Deleting a Rabbitmq RestoreJob therefore lands in default: and runs cleanupStrayVeleroRestore. Nothing foreign gets deleted, since the List is scoped by OwningJobName/OwningJobNamespace labels, but it costs a Velero List per delete on clusters that may not have Velero installed and logs "RestoreJob has unrecognised strategy kind". The surrounding comments describe this branch as a deliberate seam guard against a future refactor, and Rabbitmq is now outside it. Add strategyv1alpha1.RabbitmqStrategyKind to the case list.

[MINOR] The deletion-blocking wiring has no test

internal/backupcontroller/backup_controller.go:68

The most consequential new semantic in this PR is that a Rabbitmq Backup's removal now waits on a Job. I removed the block that implements it and re-ran the package:

$ # deleted lines 68-74 (the `if res.Requeue || res.RequeueAfter > 0 { return res, nil }` block)
$ go test ./internal/backupcontroller/... -count=1
ok  	github.com/cozystack/cozystack/internal/backupcontroller	1.303s
ok  	github.com/cozystack/cozystack/internal/backupcontroller/factory	1.203s

TestCleanupRabbitmqBackup_SpawnsDeleteJob asserts what cleanupRabbitmqBackup returns but never that Reconcile acts on it, so the finalizer-retention behaviour is unpinned. For comparison, the readiness gate is pinned: deleting the rabbitmqNotReadyMessage check at :255 turns TestReconcileRabbitmq_PreconditionGate/cluster_not_ready_defers_without_creating_a_Job red. Add a Reconcile-level case: a Backup with a deletionTimestamp, the finalizer, and an unfinished cleanup Job, asserting the finalizer survives, plus the completed-Job counterpart asserting it is removed. That test is also the one that would catch the teardown wedge above.

[MINOR] rabbitmq_has_sentinel reads a failed exec as "absent"

examples/backups/rabbitmq/00-helpers.sh:156

rabbitmqctl_exec "$cr" list_vhosts name 2>/dev/null | grep -qxF "$token"

A failing kubectl exec (pod not yet up, container restarting, a transient API error) produces no stdout with its stderr discarded, so grep -q returns 1 and the helper answers "the sentinel is not there". Used as a positive assertion that is fine, but run-all.sh:83-84 uses it negatively to establish that the target does not already hold the sentinel before the to-copy restore, which is the precondition that makes that leg meaningful, and :67 uses it to confirm the drop. Capture the exec separately and fail closed when the exec itself failed, rather than folding it into "absent".

Claim mismatches

[MISSING] "The Chainsaw roundtrip is being validated on a dev cluster; results will be added here." The Chainsaw suite is the only end-to-end evidence for the driver, and by the PR's own account it has not produced a result yet. Everything I could reproduce hermetically is green (build, vet, go test, helm unittest 18/18, hack/image-pin-consistency.bats, four rendered config corners, kubeconform), but none of that exercises a real S3 round-trip or a real management-API call.

[PARTIAL] docs/operations/backup-classes.md:247, "deletes it from the bucket when its Backup is deleted, via a one-shot Job that the Backup's removal waits on, so no object is orphaned". Three branches release the Backup without deleting the object: the strategy CR being absent (rabbitmqstrategy_controller.go:716-722, "Give up rather than wedge the Backup forever; the object may be left behind"), a render error (:741-745), and the skip annotation (:700-707). The first is reachable in normal operation, because cozy-default-rabbitmq is gated on backupstrategy-controller.bucketName and stops rendering whenever that lookup stops resolving. Reword to say the delete is best-effort with named give-up conditions.

Caveats

• Existing-customer upgrade, verified no breakage: no migration is required (nothing under packages/core/platform/images/migrations/ changed); no values key was removed or renamed; the only RBAC delta is an additive get,list,watch on rabbitmq.com/rabbitmqclusters; the controller Deployment podspec is untouched, since rabbitmqBackupClientImage is read only by the strategy template, so no rollout and no SSA field-ownership surface; the cozy-default BackupClass gains a route rather than changing one, and Plan requires an explicit spec.applicationRef (backups.cozystack.io_plans.yaml:93), so no existing RabbitMQ instance starts being backed up implicitly on upgrade.
• Fresh install, verified no breakage: the new CRD ships through templates/crds.yaml alongside its Rabbitmq CR in one release, but the CR is gated on a resolved $bucketName, which is empty on a cold install, so the CR only appears on the forced upgrade the DefaultObjectsGate issues once the BucketClaim status is populated, by which time the CRD is established. tests/rendering_test.yaml pins both halves. The gate's own missingObjects check now includes cozy-default-rabbitmq, which is what makes the new CR converge. The image is the already-mirrored platform-migrations digest, so air-gapped pullability is unchanged.
• Config corners rendered on this run: provisionBucket=false, provisionBucket=true + bucketNameOverride=ovr, forcePathStyle=false, and endpoint supplied without a scheme. All four render, kubeconform -strict -ignore-missing-schemas reports 0 invalid, and the extracted pod script passes sh -n and shellcheck -s sh clean.
• Mechanical sweep run on the diff. Nine || true / 2>/dev/null hits, eight refuted: the DELETE at strategy-rabbitmq-default.yaml:142 is fail-closed, because curl -w '%{http_code}' emits 000 on a transport failure and the case falls to *) exit 1; the rest are kubectl polls inside timeout-bounded loops in demo scripts. The ninth became the MINOR above. No mutating or wildcard verb appears in the changed RBAC.
• Not executed, needs a live cluster: the teardown deadlock in the first finding is derived from the code path plus documented NamespaceLifecycle admission behaviour and the in-tree precedent at wildcardsecret/reconciler.go:341, not from an observed reproduction. Also unverified here: whether curl --aws-sigv4 in the pinned platform-migrations image signs correctly against SeaweedFS in path-style mode, and whether a tenant-namespace pod can actually reach the external S3 ingress.
• No test in this PR exercises the driver from a normal tenant namespace. hack/e2e-chainsaw/.chainsaw.yaml records that the e2e tenant-test tenant's Cilium egress allowlist blocks its Pods from reaching tenant-root's SeaweedFS, and the new rabbitmq-2-backup-roundtrip Test overrides to tenant-root for that reason. That matches what the postgres-2-, mariadb-2- and clickhouse-2- suites already do, so it is a pre-existing coverage boundary shared by every backup driver rather than something this PR introduces, but it does mean the primary product path stays unexercised.
• Neither Chainsaw Test deletes a namespace (rabbitmq runs in the pre-existing tenant-test, rabbitmq-2-backup-roundtrip sets skipDelete: true), so CI cannot surface the teardown finding.

Recommended follow-ups

• Run an end-to-end pass on a disposable dev cluster with the teardown case explicitly in scope: create a RabbitMQ app, take a backup, then delete the whole namespace and watch whether it reaches Terminated. The same run should confirm the --aws-sigv4 path-style upload against SeaweedFS and the management-API export from inside a real tenant.
• Retention pruning on a Plan deletes several Backups at once, and each one now spawns a Job running the 349 MB platform-migrations image. The slim curl + ca-certificates image already TODO'd in values.yaml matters more for this driver than for Altinity, because the cleanup path fires per deleted Backup rather than per backup run.
• The PR body notes that RabbitMQ 4.2 is OOMKilled on the nano preset, and both the example and the Chainsaw fixture work around it with c1.small. packages/apps/rabbitmq/values.yaml:65,86 still ship resourcesPreset: t1.nano with version: v4.2 as the defaults, so the default configuration is the one nothing here covers. I did not verify the OOM myself, and it is out of scope for this PR, but it warrants its own issue rather than living in a PR description.

Thanks — the teardown deadlock especially, that was a real regression from the waiting-cleanup change. All addressed in ba32df69c.

Blockers

• Teardown deadlock. cleanupRabbitmqBackup now classifies "the delete Job cannot be created here" as terminal instead of retrying. It checks the namespace's DeletionTimestamp before any CREATE, and on the projection / Job-create paths it releases on IsForbidden / IsInvalid (and on a projection failure or an absent/unrenderable strategy). "Release" means log + a Warning Event (ArtifactNotDeleted) naming the object left behind + a zero Result, so the finalizer is dropped and the namespace finishes deleting. That covers all four inputs you listed — terminating namespace, source Secret gone, ownership-guard trip, and the >55-char name → <name>-cleanup Invalid. Reconcile-level tests pin both the finalizer retention (unfinished Job → finalizer kept; completed → removed) and the terminating-namespace release (finalizer gone + Event, no Job created).
• Readiness never terminal. Added rabbitmqBackupDeadline = 30 * time.Minute + rabbitmqBackupDeadlineExceeded, applied at both the not-found and not-ready branches on the backup and restore paths; past it, markBackupJobFailed / markRestoreJobFailed carrying the not-ready message. Test: StartedAt 31 minutes back with AllReplicasReady=False → phase=Failed.

Minors

• Added strategyv1alpha1.RabbitmqStrategyKind to the RestoreJob cleanupOnDelete no-op case list.
• The deletion-blocking wiring now has the Reconcile-level test you described — deleting the requeue block in backup_controller.go turns it red.
• rabbitmq_has_sentinel now captures the exec separately and aborts on a failed exec instead of folding it into "absent", so the negative pre-checks in run-all.sh fail closed.

Claim mismatches

• Reworded backup-classes.md: the delete is best-effort with the named give-up conditions, not "no object is orphaned".
• Updated the PR body: the backup + in-place + to-copy roundtrip was verified end-to-end on a dev cluster (real management-API export, real S3 --aws-sigv4 upload/download against SeaweedFS); the cleanup / teardown / deadline paths added during review are unit-tested but not yet cluster-exercised, and the in-CI Chainsaw run remains the outstanding automated evidence.

On the follow-ups: the slim curl image and the nano-default OOM (filed as #4003) are tracked, and a dev-cluster teardown pass is on my list.