Repository navigation
feat(platform): implement backup strategy for mariadb - #2605
Conversation
github-actions
Bot
added
area/platform
dosubot
Bot
added
the
area/database
8c2afee to
cbaf8ed
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a native backup and restore strategy for MariaDB by integrating with the k8s.mariadb.com operator. It introduces the necessary API definitions, controller logic, and RBAC permissions to manage MariaDB backups and restores effectively. Furthermore, it updates existing documentation and configuration to transition users away from the legacy backup method toward this new, operator-native approach. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds operator-native MariaDB backup/restore support: strategy CR types, internal app/operator types, a strategy controller that orchestrates k8s.mariadb.com Backup/Restore CRs for BackupJob/RestoreJob reconciliation, controller scheme wiring, examples/tests, RBAC changes, and deprecation notes for the legacy restic-based chart backup. ChangesMariaDB Backup/Restore Strategy
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs:
Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a native MariaDB backup and restore strategy that integrates with the mariadb-operator. It adds the necessary API types, controller logic for BackupJob and RestoreJob reconciliation, and updates the MariaDB package to mark existing restic-based backups as legacy. Feedback identifies a potential nil pointer dereference when handling backup retention durations and recommends using status patches instead of updates to avoid resource conflicts. A minor optimization for the RestoreJob reconciliation loop was also suggested to prevent unnecessary requeues.
| Databases: append([]string(nil), rendered.Databases...), | ||
| Compression: rendered.Compression, | ||
| LogLevel: rendered.LogLevel, | ||
| MaxRetention: rendered.MaxRetention.DeepCopy(), |
There was a problem hiding this comment.
The MaxRetention field is a pointer (*metav1.Duration). Calling .DeepCopy() directly on a nil pointer will cause a panic. Please add a nil check before copying.
MaxRetention: func() *metav1.Duration {
if rendered.MaxRetention == nil {
return nil
}
return rendered.MaxRetention.DeepCopy()
}(),There was a problem hiding this comment.
metav1.Duration.DeepCopy is generated nil-safe — apimachinery's zz_generated.deepcopy.go opens with if in == nil { return nil } before any dereference. Calling .DeepCopy() on a nil *metav1.Duration returns nil rather than panicking, so the existing call site is safe and matches the pattern used elsewhere in the codebase.
| if err := r.Status().Update(ctx, j); err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
Using Update for status updates can lead to optimistic locking conflicts (409 Conflict) if the object was modified elsewhere. It is generally safer and more robust to use Patch with a MergeFrom or StrategicMergePatch in Kubernetes controllers.
| if err := r.Status().Update(ctx, j); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| if err := r.Status().Patch(ctx, j, client.MergeFrom(base)); err != nil { | |
| return ctrl.Result{}, err | |
| } |
| if err := r.Status().Update(ctx, j); err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, prefer using Patch over Update for status changes to avoid potential conflict errors during reconciliation.
| if err := r.Status().Update(ctx, j); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| if err := r.Status().Patch(ctx, j, client.MergeFrom(base)); err != nil { | |
| return ctrl.Result{}, err | |
| } |
| if err := r.Status().Update(ctx, restoreJob); err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
Prefer using Patch for status updates to ensure consistency and avoid conflict errors.
| if err := r.Status().Update(ctx, restoreJob); err != nil { | |
| return ctrl.Result{}, err | |
| } | |
| if err := r.Status().Patch(ctx, restoreJob, client.MergeFrom(base)); err != nil { | |
| return ctrl.Result{}, err | |
| } |
| } | ||
| if fresh.Status.StartedAt != nil { | ||
| restoreJob.Status.StartedAt = fresh.Status.StartedAt | ||
| return ctrl.Result{RequeueAfter: mariadbPollInterval}, nil |
There was a problem hiding this comment.
If StartedAt is already set in the API server, there is no need to requeue the reconciliation. You can simply update the local object and continue with the rest of the logic, similar to how it's handled in the BackupJob path (lines 127-129).
| return ctrl.Result{RequeueAfter: mariadbPollInterval}, nil | |
| restoreJob.Status.StartedAt = fresh.Status.StartedAt |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps/mariadb/README.md (1)
112-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI failing: regenerate the
## Parametersblock viamake generate.The Pre-Commit pipeline reports that running
make generaterewrites theBackup parameterssection heading and descriptions to include a[LEGACY]marker, but this README is checked in without that change. Per repo conventions, the## Parametersblock is auto-rewritten bycozyvalues-genfromvalues.yamlannotations, so don't edit it by hand — instead, update the chart'svalues.yaml(and/or its#arthur.xia (@Schema)/`# `@paramannotations) for thebackup.*fields to convey the legacy status, then regenerate.The hand-written prose at lines 36–50 (the new "How to back up and restore a MariaDB application" section, including the
[LEGACY]restic walkthrough) is fine to keep above## Parameterssince the generator does not touch that area.Based on learnings: "In readme files within packages that are auto-generated by cozyvalues-gen ... do not edit them manually" and "cozyvalues-gen only rewrites the
## Parametersblock in packageREADME.mdfiles when runningmake generate."🤖 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 `@packages/apps/mariadb/README.md` around lines 112 - 124, The README's `Backup parameters` table was modified manually but is auto-generated by cozyvalues-gen; run `make generate` after updating the chart's values annotations instead of editing README.md: open the chart's values.yaml and its `# `@schema``/`# `@param`` annotations for the backup.* fields (e.g., backup.enabled, backup.s3Region, backup.s3Bucket, backup.schedule, backup.cleanupStrategy, backup.s3AccessKey, backup.s3SecretKey, backup.resticPassword), add the `[LEGACY]` wording or adjust descriptions there to reflect legacy status, then run `make generate` (which invokes cozyvalues-gen) to regenerate the `## Parameters` block so the README matches CI expectations.
🧹 Nitpick comments (4)
internal/backupcontroller/mariadbapp/zz_generated.deepcopy.go (1)
1-12: 💤 Low valueFilename convention contradicts "hand-written" status.
The
zz_generated.deepcopy.gofilename is the universal controller-gen output marker; future contributors (or amake generaterun that ever enables deepcopy-gen on this internal package) are likely to overwrite this file. Consider either renaming todeepcopy.goto match the hand-written intent, or adding the standard+k8s:deepcopy-gen=packagemarkers and generating it.🤖 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/mariadbapp/zz_generated.deepcopy.go` around lines 1 - 12, The filename zz_generated.deepcopy.go signals generated output but the file is handwritten; either rename the file to deepcopy.go (or another non-generated name) to avoid being overwritten, or add the canonical k8s deepcopy marker (e.g. +k8s:deepcopy-gen=package) and place the appropriate autogenerated header so it can be managed by code generation; update package mariadbapp's comments accordingly and ensure any CI/make targets reflect the chosen approach to prevent accidental overwrites.api/backups/strategy/v1alpha1/mariadb_types.go (2)
161-169: 💤 Low valueAdd
MinLength=1toNameandKeyonMariaDBSecretKeySelector.Both fields are required to reference a real Secret/key. Today an empty string passes admission and only fails when the driver dereferences the Secret, producing a late and unhelpful error.
♻️ Proposed diff
type MariaDBSecretKeySelector struct { // Name is the Secret name. Templating is supported. + // +kubebuilder:validation:MinLength=1 Name string `json:"name"` // Key is the key within the Secret holding the value. + // +kubebuilder:validation:MinLength=1 Key string `json:"key"` }🤖 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 `@api/backups/strategy/v1alpha1/mariadb_types.go` around lines 161 - 169, The MariaDBSecretKeySelector struct's Name and Key allow empty strings; add Kubernetes validation to require non-empty values by adding a kubebuilder MinLength=1 validation marker for both fields. Locate the struct MariaDBSecretKeySelector and add the comment tag "+kubebuilder:validation:MinLength=1" immediately above the Name field and likewise above the Key field so the CRD enforces at least one character for Name and Key.
99-117: ⚡ Quick winEnforce the "exactly one of" storage constraint via CEL XValidation.
The doc comment states "Exactly one of S3, PersistentVolumeClaim or Volume must be set", but each field is just
+optionaland nothing rejects a CR with zero or multiple set at admission time. Misconfigured strategies will only surface at driver-render time, which is late and noisy for tenants.♻️ Proposed XValidation rule
// MariaDBStorageTemplate mirrors k8s.mariadb.com/v1alpha1 Backup.spec.storage. // Exactly one of S3, PersistentVolumeClaim or Volume must be set. +// +kubebuilder:validation:XValidation:rule="[has(self.s3), has(self.persistentVolumeClaim), has(self.volume)].filter(x, x).size() == 1",message="exactly one of s3, persistentVolumeClaim or volume must be set" type MariaDBStorageTemplate struct {controller-gen kubebuilder XValidation oneOf field set CEL syntax🤖 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 `@api/backups/strategy/v1alpha1/mariadb_types.go` around lines 99 - 117, The struct MariaDBStorageTemplate allows zero or multiple of S3, PersistentVolumeClaim, and Volume to be set but must enforce "exactly one" at admission; add a kubebuilder XValidation (CEL) rule on MariaDBStorageTemplate that validates exactly one of the three fields is non-null (e.g. a oneOf expression using nonEmpty checks on .s3, .persistentVolumeClaim, .volume or equivalent CEL boolean arithmetic) so the API server rejects CRs with none or more than one storage target set; attach the XValidation marker to the MariaDBStorageTemplate type so validation runs at admission time.internal/backupcontroller/mariadbstrategy_controller.go (1)
699-740: 💤 Low valueValidate exactly-one of S3/PVC/Volume rather than first-match.
The error message on line 738 says the template "requires exactly one of s3, persistentVolumeClaim or volume", but the
switchonly enforces "at least one". If a tenant authors a strategy template with boths3andpersistentVolumeClaimset, the case order silently picks S3 and discards the PVC branch without any indication. The operator-side OpenAPI schema will eventually reject mixed branches on the operatorBackupCR for the cases it covers, but surfacing the conflict here as a strategy validation error gives the tenant a much clearer message and avoids ever creating the operator Backup with the wrong shape.♻️ Proposed validation
func buildMariaDBBackupStorage(in strategyv1alpha1.MariaDBStorageTemplate) (mariadbtypes.BackupStorage, error) { + set := 0 + if in.S3 != nil { + set++ + } + if in.PersistentVolumeClaim != nil { + set++ + } + if in.Volume != nil { + set++ + } + if set > 1 { + return mariadbtypes.BackupStorage{}, fmt.Errorf("strategy.spec.template.storage must set exactly one of s3, persistentVolumeClaim or volume; %d branches set", set) + } switch { case in.S3 != nil:🤖 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/mariadbstrategy_controller.go` around lines 699 - 740, The buildMariaDBBackupStorage function currently uses a switch to pick the first non-nil storage (S3, PersistentVolumeClaim, Volume) which allows multiple fields to be set silently; change it to explicitly validate that exactly one of in.S3, in.PersistentVolumeClaim, in.Volume is non-nil (e.g., count non-nil fields) and return an error if the count != 1, then proceed to construct the corresponding mariadbtypes.BackupStorage for the single selected branch (retain existing logic that builds S3 with TLS and SessionTokenSecretKeyRef and uses DeepCopy for PersistentVolumeClaim and Volume); include the same error message used now when count != 1.
🤖 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 `@internal/backupcontroller/backupjob_controller.go`:
- Line 91: The logged "supported" slice in the default branch omits
strategyv1alpha1.AltinityStrategyKind even though the switch dispatches to
r.reconcileAltinity for that kind; update the supported slice to include
strategyv1alpha1.AltinityStrategyKind so the log accurately reflects all handled
kinds (i.e., add AltinityStrategyKind alongside JobStrategyKind,
VeleroStrategyKind, CNPGStrategyKind, MariaDBStrategyKind in the supported list
emitted in the default branch).
---
Outside diff comments:
In `@packages/apps/mariadb/README.md`:
- Around line 112-124: The README's `Backup parameters` table was modified
manually but is auto-generated by cozyvalues-gen; run `make generate` after
updating the chart's values annotations instead of editing README.md: open the
chart's values.yaml and its `# `@schema``/`# `@param`` annotations for the backup.*
fields (e.g., backup.enabled, backup.s3Region, backup.s3Bucket, backup.schedule,
backup.cleanupStrategy, backup.s3AccessKey, backup.s3SecretKey,
backup.resticPassword), add the `[LEGACY]` wording or adjust descriptions there
to reflect legacy status, then run `make generate` (which invokes
cozyvalues-gen) to regenerate the `## Parameters` block so the README matches CI
expectations.
---
Nitpick comments:
In `@api/backups/strategy/v1alpha1/mariadb_types.go`:
- Around line 161-169: The MariaDBSecretKeySelector struct's Name and Key allow
empty strings; add Kubernetes validation to require non-empty values by adding a
kubebuilder MinLength=1 validation marker for both fields. Locate the struct
MariaDBSecretKeySelector and add the comment tag
"+kubebuilder:validation:MinLength=1" immediately above the Name field and
likewise above the Key field so the CRD enforces at least one character for Name
and Key.
- Around line 99-117: The struct MariaDBStorageTemplate allows zero or multiple
of S3, PersistentVolumeClaim, and Volume to be set but must enforce "exactly
one" at admission; add a kubebuilder XValidation (CEL) rule on
MariaDBStorageTemplate that validates exactly one of the three fields is
non-null (e.g. a oneOf expression using nonEmpty checks on .s3,
.persistentVolumeClaim, .volume or equivalent CEL boolean arithmetic) so the API
server rejects CRs with none or more than one storage target set; attach the
XValidation marker to the MariaDBStorageTemplate type so validation runs at
admission time.
In `@internal/backupcontroller/mariadbapp/zz_generated.deepcopy.go`:
- Around line 1-12: The filename zz_generated.deepcopy.go signals generated
output but the file is handwritten; either rename the file to deepcopy.go (or
another non-generated name) to avoid being overwritten, or add the canonical k8s
deepcopy marker (e.g. +k8s:deepcopy-gen=package) and place the appropriate
autogenerated header so it can be managed by code generation; update package
mariadbapp's comments accordingly and ensure any CI/make targets reflect the
chosen approach to prevent accidental overwrites.
In `@internal/backupcontroller/mariadbstrategy_controller.go`:
- Around line 699-740: The buildMariaDBBackupStorage function currently uses a
switch to pick the first non-nil storage (S3, PersistentVolumeClaim, Volume)
which allows multiple fields to be set silently; change it to explicitly
validate that exactly one of in.S3, in.PersistentVolumeClaim, in.Volume is
non-nil (e.g., count non-nil fields) and return an error if the count != 1, then
proceed to construct the corresponding mariadbtypes.BackupStorage for the single
selected branch (retain existing logic that builds S3 with TLS and
SessionTokenSecretKeyRef and uses DeepCopy for PersistentVolumeClaim and
Volume); include the same error message used now when count != 1.
🪄 Autofix (Beta)
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
Run ID: 2d4622fd-bf50-4405-bd75-276fe123705f
📒 Files selected for processing (16)
api/backups/strategy/v1alpha1/mariadb_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gocmd/backupstrategy-controller/main.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/mariadbapp/types.gointernal/backupcontroller/mariadbapp/zz_generated.deepcopy.gointernal/backupcontroller/mariadbstrategy_controller.gointernal/backupcontroller/mariadbstrategy_controller_test.gointernal/backupcontroller/mariadbtypes/types.gointernal/backupcontroller/mariadbtypes/zz_generated.deepcopy.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/restorejob_controller_test.gopackages/apps/mariadb/README.mdpackages/apps/mariadb/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mariadbs.yamlpackages/system/backupstrategy-controller/templates/rbac.yaml
f5978a1 to
7a94a9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/backupcontroller/mariadbstrategy_controller.go (1)
455-474: 💤 Low valueRestore
StartedAtbootstrap requeues even when stale cache resolves — asymmetric with the backup path.When the cache returned
StartedAt == nilbut a freshGetfinds it already set, the restore branch syncs the value into the in-memory object and immediately returnsRequeueAfter(line 462). The backup-side bootstrap at lines 118-138 instead absorbs the value and falls through to continue the reconcile in the same loop. The restore behaviour is correct but costs an extra reconcile cycle on stale-cache hits and diverges from the documented "same idempotency pattern as the CNPG driver" comment at line 122. Worth aligning for consistency.♻️ Proposed alignment
if fresh.Status.StartedAt != nil { restoreJob.Status.StartedAt = fresh.Status.StartedAt - return ctrl.Result{RequeueAfter: mariadbPollInterval}, nil + } else { + base := fresh.DeepCopy() + now := metav1.Now() + fresh.Status.StartedAt = &now + fresh.Status.Phase = backupsv1alpha1.RestoreJobPhaseRunning + if err := r.Status().Patch(ctx, fresh, client.MergeFrom(base)); err != nil { + return ctrl.Result{}, err + } + restoreJob.Status.StartedAt = fresh.Status.StartedAt + restoreJob.Status.Phase = fresh.Status.Phase + return ctrl.Result{RequeueAfter: mariadbPollInterval}, nil } - base := fresh.DeepCopy() - now := metav1.Now() - fresh.Status.StartedAt = &now - fresh.Status.Phase = backupsv1alpha1.RestoreJobPhaseRunning - if err := r.Status().Patch(ctx, fresh, client.MergeFrom(base)); err != nil { - return ctrl.Result{}, err - } - restoreJob.Status.StartedAt = fresh.Status.StartedAt - restoreJob.Status.Phase = fresh.Status.Phase - return ctrl.Result{RequeueAfter: mariadbPollInterval}, nil }
🤖 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 `@internal/backupcontroller/mariadbstrategy_controller.go`:
- Line 31: Remove the unused constant mariadbFieldManager from this file — it is
never referenced and golangci-lint flags it as unused; locate the declaration
"mariadbFieldManager = \"cozystack-mariadb-backup-driver\"" and delete it, since
all status writes in this file use client.MergeFrom(base) and
r.Status().Update(...) (no client.FieldOwner usage). If you later migrate to
server-side apply, reintroduce the constant and pass it as
client.FieldOwner(mariadbFieldManager) on the Patch calls instead.
🪄 Autofix (Beta)
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
Run ID: 38e319a0-8ca2-4f88-a809-0d794ce1afbe
📒 Files selected for processing (8)
api/backups/strategy/v1alpha1/mariadb_types.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/backupjob_controller_test.gointernal/backupcontroller/mariadbstrategy_controller.gointernal/backupcontroller/mariadbstrategy_controller_test.gopackages/system/backup-controller/values.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/backupstrategy-controller/values.yaml
✅ Files skipped from review due to trivial changes (2)
- packages/system/backup-controller/values.yaml
- packages/system/backupstrategy-controller/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- api/backups/strategy/v1alpha1/mariadb_types.go
7a94a9e to
0618661
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/backupcontroller/mariadbstrategy_controller.go`:
- Around line 755-795: The function buildMariaDBBackupStorage currently picks
the first non-nil storage option; change it to validate that exactly one of
in.S3, in.PersistentVolumeClaim, and in.Volume is set: compute a count of
non-nil options at the start of buildMariaDBBackupStorage (and return an error
if count != 1), and only then proceed with the existing per-branch construction
(S3 -> mariadbtypes.S3Storage, PersistentVolumeClaim -> DeepCopy(), Volume ->
DeepCopy()); keep the existing error message text for the invalid case. Ensure
you reference and update buildMariaDBBackupStorage and its handling of in.S3,
in.PersistentVolumeClaim, and in.Volume.
- Around line 268-304: ensureMariaDBBackup (and the sibling
ensureMariaDBRestore) must stop using GenerateName and instead set a
deterministic Name based on the BackupJob (e.g. fmt.Sprintf("%s-mariadb-backup",
j.Name) or similar) so creates are idempotent; when calling r.Create(ctx, obj)
handle apierrors.IsAlreadyExists(err) by doing a r.Get(ctx,
types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name}, existing) and
returning the existing object, and only return an error for other create
failures; keep the same labels/spec fields (Databases, Storage, etc.) and ensure
the name generation logic matches the pattern used at line 401 so the Get lookup
will find the same resource on concurrent reconciles.
🪄 Autofix (Beta)
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
Run ID: c3bdc772-73f5-435e-804d-ddc05a9364a7
📒 Files selected for processing (22)
api/apps/v1alpha1/mariadb/types.goapi/backups/strategy/v1alpha1/mariadb_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gocmd/backupstrategy-controller/main.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/backupjob_controller_test.gointernal/backupcontroller/mariadbapp/types.gointernal/backupcontroller/mariadbapp/zz_generated.deepcopy.gointernal/backupcontroller/mariadbstrategy_controller.gointernal/backupcontroller/mariadbstrategy_controller_test.gointernal/backupcontroller/mariadbtypes/types.gointernal/backupcontroller/mariadbtypes/zz_generated.deepcopy.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/restorejob_controller_test.gopackages/apps/mariadb/README.mdpackages/apps/mariadb/values.schema.jsonpackages/apps/mariadb/values.yamlpackages/system/backup-controller/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mariadbs.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/backupstrategy-controller/values.yamlpackages/system/mariadb-rd/cozyrds/mariadb.yaml
✅ Files skipped from review due to trivial changes (7)
- packages/apps/mariadb/README.md
- packages/system/mariadb-rd/cozyrds/mariadb.yaml
- packages/apps/mariadb/values.schema.json
- api/backups/strategy/v1alpha1/zz_generated.deepcopy.go
- api/apps/v1alpha1/mariadb/types.go
- internal/backupcontroller/restorejob_controller_test.go
- internal/backupcontroller/mariadbtypes/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/backupcontroller/restorejob_controller.go
- internal/backupcontroller/backupjob_controller.go
- internal/backupcontroller/mariadbapp/zz_generated.deepcopy.go
- cmd/backupstrategy-controller/main.go
- packages/system/backupstrategy-controller/values.yaml
- packages/apps/mariadb/values.yaml
- internal/backupcontroller/backupjob_controller_test.go
- api/backups/strategy/v1alpha1/mariadb_types.go
- packages/system/backup-controller/values.yaml
- internal/backupcontroller/mariadbapp/types.go
- internal/backupcontroller/mariadbtypes/types.go
| func (r *BackupJobReconciler) ensureMariaDBBackup(ctx context.Context, j *backupsv1alpha1.BackupJob, rendered *strategyv1alpha1.MariaDBTemplate) (*mariadbtypes.Backup, error) { | ||
| existing, err := r.findMariaDBBackupForJob(ctx, j) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if existing != nil { | ||
| return existing, nil | ||
| } | ||
|
|
||
| storage, err := buildMariaDBBackupStorage(rendered.Storage) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| obj := &mariadbtypes.Backup{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: j.Namespace, | ||
| GenerateName: fmt.Sprintf("%s-", j.Name), | ||
| Labels: map[string]string{ | ||
| backupsv1alpha1.OwningJobNameLabel: j.Name, | ||
| backupsv1alpha1.OwningJobNamespaceLabel: j.Namespace, | ||
| }, | ||
| }, | ||
| Spec: mariadbtypes.BackupSpec{ | ||
| MariaDBRef: mariadbtypes.MariaDBObjectRef{Name: mariadbNameForApp(j.Spec.ApplicationRef.Name)}, | ||
| Storage: storage, | ||
| Databases: append([]string(nil), rendered.Databases...), | ||
| Compression: rendered.Compression, | ||
| LogLevel: rendered.LogLevel, | ||
| MaxRetention: rendered.MaxRetention.DeepCopy(), | ||
| }, | ||
| } | ||
|
|
||
| if err := r.Create(ctx, obj); err != nil { | ||
| return nil, err | ||
| } | ||
| return obj, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MariaDB helper implementations =="
sed -n '265,305p;598,637p' internal/backupcontroller/mariadbstrategy_controller.go
echo
echo "== Relevant controller-runtime usage in this file =="
rg -n --type=go 'GenerateName|MatchingLabels|List\(|Create\(' internal/backupcontroller/mariadbstrategy_controller.goRepository: cozystack/cozystack
Length of output: 3188
🏁 Script executed:
# Find findMariaDBBackupForJob implementation
rg -n "func.*findMariaDBBackupForJob" internal/backupcontroller/mariadbstrategy_controller.go -A 15
# Check what happens with Create errors (check context around lines 301, 633, 401)
sed -n '298,310p' internal/backupcontroller/mariadbstrategy_controller.go
sed -n '630,645p' internal/backupcontroller/mariadbstrategy_controller.go
sed -n '398,410p' internal/backupcontroller/mariadbstrategy_controller.goRepository: cozystack/cozystack
Length of output: 1794
🏁 Script executed:
# Find which function contains the AlreadyExists handling around line 401
sed -n '380,415p' internal/backupcontroller/mariadbstrategy_controller.go
# Also check if there are any other uses of AlreadyExists in the file
rg -n "IsAlreadyExists" internal/backupcontroller/mariadbstrategy_controller.go -B 3 -A 3Repository: cozystack/cozystack
Length of output: 1410
🏁 Script executed:
# Get the function signature for the good pattern around line 380-415
sed -n '350,390p' internal/backupcontroller/mariadbstrategy_controller.go | head -50
# Find the buildMariaDBBackupStorage function to check storage validation
rg -n "func.*buildMariaDBBackupStorage" internal/backupcontroller/mariadbstrategy_controller.go -A 20Repository: cozystack/cozystack
Length of output: 2537
🏁 Script executed:
# Verify GenerateName behavior - check if it produces deterministic or random output
# Also verify the storage validation concern by checking the full function
sed -n '755,800p' internal/backupcontroller/mariadbstrategy_controller.go
# Check what the function comments say about constraints
sed -n '740,760p' internal/backupcontroller/mariadbstrategy_controller.goRepository: cozystack/cozystack
Length of output: 2860
🏁 Script executed:
# Check the MariaDBStorageTemplate type definition to see if OpenAPI validation prevents multiple backends
rg -n "type MariaDBStorageTemplate" --type go -A 20
# Also verify if there's validation that ensures mutual exclusion
rg -n "MariaDBStorageTemplate" internal/backupcontroller/mariadbstrategy_controller.go | head -5Repository: cozystack/cozystack
Length of output: 2212
Fix idempotency race: use deterministic names with AlreadyExists error handling.
The helpers ensureMariaDBBackup and ensureMariaDBRestore use GenerateName (which produces a random suffix on each call) combined with label-based List() lookup for idempotency. With controller-runtime's split client, if a follow-up reconcile runs before the cache catches up, it will not find the just-created object in the cached list, execute the same code path again, and create a duplicate Backup/Restore CR. This is especially dangerous for restore operations, where the same dump can be replayed twice against the live MariaDB.
The file already demonstrates the correct pattern (line 401): use a deterministic object Name derived from the job name, then handle AlreadyExists errors with an explicit Get. Apply this pattern to both MariaDB helpers to achieve true idempotency per controller-runtime best practices.
🤖 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/mariadbstrategy_controller.go` around lines 268 -
304, ensureMariaDBBackup (and the sibling ensureMariaDBRestore) must stop using
GenerateName and instead set a deterministic Name based on the BackupJob (e.g.
fmt.Sprintf("%s-mariadb-backup", j.Name) or similar) so creates are idempotent;
when calling r.Create(ctx, obj) handle apierrors.IsAlreadyExists(err) by doing a
r.Get(ctx, types.NamespacedName{Namespace: obj.Namespace, Name: obj.Name},
existing) and returning the existing object, and only return an error for other
create failures; keep the same labels/spec fields (Databases, Storage, etc.) and
ensure the name generation logic matches the pattern used at line 401 so the Get
lookup will find the same resource on concurrent reconciles.
0618661 to
3103510
Compare
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Review saved to: /tmp/branch-review-feat-mariadb-backups-1778535640.md
NOT LGTM
Reviewing against target branch: main (PR #2605: feat(platform): implement backup strategy for mariadb). Branch is feat/mariadb-backups, merge base 86f5267dd. go build ./... and go test ./internal/backupcontroller/... both pass.
Overall the implementation is well-structured and mirrors the CNPG/Altinity strategy drivers closely. The single blocker is documentation drift that the PR introduces.
Blocking
1. README points at examples/backups/mariadb/, which this PR does not create
packages/apps/mariadb/README.md:36-43 adds a new "How to back up and restore a MariaDB application" section that tells users to "See examples/backups/mariadb for a numbered, end-to-end walkthrough (bucket, source, strategy, BackupClass, Plan, ad-hoc BackupJob, and both in-place + to-copy RestoreJob fixtures)."
That directory does not exist on this branch:
$ ls examples/backups/
clickhouse
postgres
vmi
For comparison, the analogous postgres PR (packages/apps/postgres/README.md:44, 60, 117, 119) ships matching manifests at examples/backups/postgres/00-bucket.yaml, 05-postgres-src.yaml, 10-cnpg-strategy.yaml, 15-backupclass.yaml, 20-plan.yaml, 25-backupjob-adhoc.yaml, 30-postgres-target.yaml, 35-restorejob-in-place.yaml, 40-restorejob-to-copy.yaml. The clickhouse PR ships an even richer set (numbered shell scripts plus scenario markdown). The mariadb PR ships zero example fixtures.
Per the project rules: stale/misleading documentation about the area being changed is a blocking issue. This is more than stale — the README explicitly promises an "end-to-end walkthrough" that a tenant clicking the relative link gets a 404 on. New users following the README to try the new operator-native flow will hit a dead end immediately.
- Fix: add the
examples/backups/mariadb/directory with manifests matching the structure used byexamples/backups/postgres/— at minimum a bucket, MariaDB source app,strategy.backups.cozystack.io/MariaDBexample,BackupClass,Plan, ad-hocBackupJob, target MariaDB app, in-placeRestoreJob, to-copyRestoreJob. The strategy and storage shapes need to exercise S3-on-seaweedfs since the strategy template (api/backups/strategy/v1alpha1/mariadb_types.go:124-161) calls that out as the canonical target. Or, drop the README pointer if examples aren't ready for this PR (but then drop the entire "end-to-end walkthrough" paragraph; do not leave a broken link). - Test that fails without the fix: an e2e test that lints/applies the manifests under
examples/backups/mariadb/(the postgres flow is exercised this way — see theexamples/backups/postgres/shape). A unit test that grep/parses the README to assert the path resolves on disk is the wrong shape per the project's own "Don't write tests that grep / parse source files" rule; the correct guard is the e2e fixture applying the files. If e2e for mariadb isn't ready yet, amake example-lint-style CI hook that fails when a relative link inpackages/apps/*/README.mdpoints at a non-existent path inside the repo is the next-best line of defense.
Recommended to fix (not blocking)
A. Dead code in mariadbtypes
internal/backupcontroller/mariadbtypes/types.go declares MariaDBSpec.BootstrapFrom, the BootstrapFrom struct, and the BackupContentTypeLogical constant. None of these are referenced anywhere in the controller, command, or tests:
$ grep -rn "BootstrapFrom\|BackupContentTypeLogical\b" internal/ cmd/ api/
internal/backupcontroller/mariadbtypes/types.go: (declarations only)
internal/backupcontroller/mariadbtypes/zz_generated.deepcopy.go: (codegen for the dead type)
The strategy doc comment in mariadbstrategy_controller.go:39 is explicit that the driver does NOT touch MariaDB.spec.bootstrapFrom — the restore path goes through a separate k8s.mariadb.com/Restore CR. So MariaDBSpec ends up with one field, that field is unused, and a generated DeepCopyInto exists for a struct nothing reads. Drop the field + struct + constant from mariadbtypes/types.go and regenerate deepcopy via the project codegen target. (Leave BackupReference — it's reused for RestoreSpec.BackupRef.)
B. marshalMariaDBBackupSnapshot first parameter is unused
internal/backupcontroller/mariadbstrategy_controller.go:689 declares marshalMariaDBBackupSnapshot(_ *mariadbapp.MariaDB, ...) — the source app is accepted and ignored. The CNPG analogue (marshalCNPGBackupSnapshot, line 937 of cnpgstrategy_controller.go) reads the app to snapshot spec.databases / spec.users. For MariaDB the app spec is currently empty so there's nothing to snapshot, but the function signature implies otherwise. The call site (mariadbstrategy_controller.go:369) also passes the live app — both are misleading.
Either:
- Drop the parameter (caller no longer threads
sourceAppthroughcreateMariaDBBackupArtifact), or - Add a one-liner doc-comment on
marshalMariaDBBackupSnapshotexplaining why the app argument is reserved (mirroring themariadbapp.MariaDBSpec"intentionally empty" note).
Option 1 is cleaner; option 2 is the smaller diff. Either way the underscore-as-name in a public-ish helper is reviewer-bait.
C. Snapshot is persisted but never consumed
marshalMariaDBBackupSnapshot writes a structured payload into Backup.status.underlyingResources with the rendered storage descriptor and parameters. The doc comment (lines 678-687) claims this exists "so a future RestoreJob can reproduce them exactly when the operator-side Backup CR has been pruned." But the restore path at mariadbstrategy_controller.go:512-520 terminally fails if the operator-side k8s.mariadb.com/Backup CR is missing — it never falls back to the snapshot. So the rationale in the doc comment is aspirational at best.
This is either a) genuinely forward-looking and OK to keep, or b) drift that should match reality. If you're keeping the snapshot for forward-compat, tighten the doc comment to say "reserved for a future fallback path that re-creates the operator Backup from the snapshot when the original has been GC'd" instead of claiming the current restore path uses it.
D. CRD does not enforce "exactly one of S3 / PVC / Volume"
packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mariadbs.yaml declares storage as required but allows zero, one, two, or three of its branches to be populated. The Go code (buildMariaDBBackupStorage, line 755) catches the zero case but silently picks S3 first when multiple are set. The user-facing failure mode is "operator picks an unexpected branch."
controller-gen doesn't easily express oneOf, but a // +kubebuilder:validation:XValidation:rule="..." on MariaDBStorageTemplate would surface the conflict at admission time. Optional polish; matches the gap in CNPG so it's consistent with the rest of the codebase.
Notes on what's correct (no action required)
- The
OwningJob{Name,Namespace}idempotency on both backup and restore CR creation is sound, andfindMariaDBBackupForJob's deterministic-pick fallback for the duplicate-race case is tested (TestFindMariaDBBackupForJob_DuplicateMatches). - The wall-clock deadline gate (
mariadbBackupDeadlineExceeded) correctly returns false on nilStartedAtto avoid tripping on the very first reconcile; tested. - The
validateMariaDBApplicationRefacceptsapiGroup=""(legacy refs) but rejectsapiGroup="other.example.com"— same shape as CNPG; tested. - The transient-missing-target handling in
reconcileMariaDBRestoremirrors the backup path and is explicitly tested (TestReconcileMariaDBRestore_TargetMariaDBNotFoundIsTransient). - The
Status().Patch+ RequeueAfter pattern when writingStartedAtis correct (avoids stale ResourceVersion on a subsequentStatus().Updatein the same reconcile, matching the CNPG implementation note). - RBAC additions in
packages/system/backupstrategy-controller/templates/rbac.yamlare minimal: read onmariadbs, create+list+get+watch onbackups/restores. No patch/delete, consistent with the driver's behavior. Sound. cmd/backupstrategy-controller/main.goregisters bothmariadbtypesandmariadbappwith the scheme — needed so controller-runtime's cache can serve typed Get/List for both API groups.supportedBackupStrategyKinds()centralization is a nice refactor;TestSupportedBackupStrategyKindsMatchesDispatchlocks the invariant.- The deprecation marking of the chart's
backup.*block inpackages/apps/mariadb/values.yaml,values.schema.json,README.md, and the renderedmariadb-rd/cozyrds/mariadb.yamlopenAPISchema is consistent across all four surfaces. - No leaked secrets in the diff.
Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR — no exceptions.
Every issue listed above must have a corresponding test. No matter the severity — bug, logic error, security issue, error handling gap, documentation drift — each one must be covered by a test that fails without the fix and passes with it. Untested fixes are not fixes.
3103510 to
55af5ec
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/backups/mariadb/05-mariadb-src.yaml (1)
27-29: ⚡ Quick winAdd an explicit “no plaintext secrets” warning next to the sample password.
This example is likely to be copied; a short warning here helps prevent accidental plaintext credential commits.
Proposed diff
users: app: + # Example placeholder only. Do not commit real credentials in Git. + # Replace via your secret-management flow before apply. password: REPLACE_WITH_PASSWORD maxUserConnections: 100🤖 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/mariadb/05-mariadb-src.yaml` around lines 27 - 29, Replace the plaintext sample password in the YAML by either changing app.password to a non-secret placeholder (e.g. REPLACE_WITH_SECRET or <REDACTED>) and add an inline YAML comment warning not to store plaintext credentials, or keep the current placeholder and append a comment like "# WARNING: do not commit plaintext secrets — use env or secret store"; target the app.password key in the file (near app.maxUserConnections) so the warning is clearly visible where users copy the example.
🤖 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.
Nitpick comments:
In `@examples/backups/mariadb/05-mariadb-src.yaml`:
- Around line 27-29: Replace the plaintext sample password in the YAML by either
changing app.password to a non-secret placeholder (e.g. REPLACE_WITH_SECRET or
<REDACTED>) and add an inline YAML comment warning not to store plaintext
credentials, or keep the current placeholder and append a comment like "#
WARNING: do not commit plaintext secrets — use env or secret store"; target the
app.password key in the file (near app.maxUserConnections) so the warning is
clearly visible where users copy the example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1cb3c7cc-4474-40f0-a41b-0a664ae88819
📒 Files selected for processing (30)
api/apps/v1alpha1/mariadb/types.goapi/backups/strategy/v1alpha1/mariadb_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gocmd/backupstrategy-controller/main.goexamples/backups/mariadb/00-bucket.yamlexamples/backups/mariadb/05-mariadb-src.yamlexamples/backups/mariadb/10-mariadb-strategy.yamlexamples/backups/mariadb/15-backupclass.yamlexamples/backups/mariadb/20-plan.yamlexamples/backups/mariadb/25-backupjob-adhoc.yamlexamples/backups/mariadb/30-mariadb-target.yamlexamples/backups/mariadb/35-restorejob-in-place.yamlexamples/backups/mariadb/40-restorejob-to-copy.yamlexamples/backups/mariadb/README.mdinternal/backupcontroller/backupjob_controller.gointernal/backupcontroller/backupjob_controller_test.gointernal/backupcontroller/mariadbapp/types.gointernal/backupcontroller/mariadbapp/zz_generated.deepcopy.gointernal/backupcontroller/mariadbstrategy_controller.gointernal/backupcontroller/mariadbstrategy_controller_test.gointernal/backupcontroller/mariadbtypes/types.gointernal/backupcontroller/mariadbtypes/zz_generated.deepcopy.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/restorejob_controller_test.gopackages/apps/mariadb/README.mdpackages/apps/mariadb/values.schema.jsonpackages/apps/mariadb/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mariadbs.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/mariadb-rd/cozyrds/mariadb.yaml
✅ Files skipped from review due to trivial changes (11)
- examples/backups/mariadb/00-bucket.yaml
- examples/backups/mariadb/10-mariadb-strategy.yaml
- examples/backups/mariadb/30-mariadb-target.yaml
- examples/backups/mariadb/25-backupjob-adhoc.yaml
- examples/backups/mariadb/README.md
- packages/apps/mariadb/README.md
- packages/apps/mariadb/values.schema.json
- api/apps/v1alpha1/mariadb/types.go
- packages/system/mariadb-rd/cozyrds/mariadb.yaml
- api/backups/strategy/v1alpha1/zz_generated.deepcopy.go
- internal/backupcontroller/backupjob_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/backupcontroller/restorejob_controller_test.go
- cmd/backupstrategy-controller/main.go
- internal/backupcontroller/mariadbapp/types.go
- packages/apps/mariadb/values.yaml
- internal/backupcontroller/backupjob_controller.go
- internal/backupcontroller/mariadbtypes/types.go
- internal/backupcontroller/mariadbapp/zz_generated.deepcopy.go
- internal/backupcontroller/restorejob_controller.go
- api/backups/strategy/v1alpha1/mariadb_types.go
- internal/backupcontroller/mariadbtypes/zz_generated.deepcopy.go
- internal/backupcontroller/mariadbstrategy_controller.go
- internal/backupcontroller/mariadbstrategy_controller_test.go
55af5ec to
9d0724f
Compare
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Review saved to: /tmp/branch-review-feat-mariadb-backups-1747055000.md
NOT LGTM
Reviewing against target branch: main (PR #2605 #2605).
The PR is overall well-built: a new strategy.backups.cozystack.io/v1alpha1 MariaDB strategy that delegates to mariadb-operator, with typed partial schemas, XValidation on storage, idempotent backup/restore Create paths, wall-clock deadlines, snapshot persistence in Backup.status.underlyingResources, and substantive table-driven tests. Two blocking issues plus several documentation / consistency drifts must be fixed before merge.
Blocker 1 — BackupReconciler.cleanupOnDelete is missing the MariaDBStrategyKind branch
internal/backupcontroller/backup_controller.go:96-123 dispatches per-strategy cleanup. The new strategy adds MariaDBStrategyKind to:
BackupJobReconciler.Reconciledispatch (backupjob_controller.go:85-86)RestoreJobReconciler.Reconciledispatch (restorejob_controller.go:117-118)RestoreJobReconciler.cleanupOnDelete(restorejob_controller.go:211)supportedBackupStrategyKinds()(backupjob_controller.go:100-108)
but NOT to BackupReconciler.cleanupOnDelete. A MariaDB-strategy Cozystack Backup therefore falls into the default: branch which calls cleanupVeleroBackup. Today that's a happy no-op because the MariaDB driver never sets veleroBackupNameMetadataKey. However:
- The Altinity strategy faced the identical situation and was guarded with an explicit
case strategyv1alpha1.AltinityStrategyKind: return nilprecisely because falling into the Velero default is a maintenance trap. The fix even has a dedicated regression test,TestBackupCleanup_Altinity_DoesNotFallThroughToVelero(backup_controller_test.go:122-171), whose docstring spells out the exact concern: "with the old default-fallback dispatch, this would synthesise a DeleteBackupRequest. The Altinity branch must short-circuit before that happens." The new MariaDB strategy has reintroduced the same hole that test was written to guard against. - If a tenant ever stamps
velero.io/backup-nameonto a MariaDB Backup'sdriverMetadata(e.g. during a migration, or by a future driver refactor that reuses the key), the default branch will synthesise aDeleteBackupRequestagainstcozy-velero. Cozystack does not own the mariadb-operator-sidek8s.mariadb.com/BackupCR (rbac.yaml:117-122 grants onlyget/list/watch/create); the deletion contract for that archive lives elsewhere. The dispatcher must encode that explicitly, not by accident of which metadata key the driver happens not to set today.
Fix: add to backup_controller.go:96:
case strategyv1alpha1.MariaDBStrategyKind:
// Cozystack Backup deletion does NOT delete the operator-side
// k8s.mariadb.com/Backup CR. Lifecycle of that CR (and the
// backing S3 / PVC archive) is owned by mariadb-operator's own
// MaxRetention and by tenants / explicit teardown flows. This
// mirrors the Altinity branch's "we do not own S3" contract.
return nilRequired test (mirrors TestBackupCleanup_Altinity_DoesNotFallThroughToVelero): a MariaDB-strategy Cozystack Backup with veleroBackupNameMetadataKey + veleroBackupNamespaceMetadataKey stamped into DriverMetadata, plus a pre-seeded velero.io/Backup named after that key — cleanupOnDelete must complete without creating any DeleteBackupRequest and the Velero Backup must survive. Without the case, this test fails because the default branch fires cleanupVeleroBackup, which sees the metadata key, finds the Velero Backup, and creates a DBR.
Also extend TestStrategyKindForBackup (backup_controller_test.go:175-202) with a {"mariadb", strategyv1alpha1.MariaDBStrategyKind, strategyv1alpha1.MariaDBStrategyKind} row for symmetry with the other strategies — this is a small but cheap drift guard for the dispatcher's lookup helper.
Blocker 2 — Documentation references an e2e test that does not exist
Two files in this PR promise a CI e2e harness:
examples/backups/mariadb/README.md:31-33: "Same-namespace flows are covered by the e2e (hack/e2e-apps/backup-mariadb.bats); cross-tenant restores are blocked by the per-tenant Cilium egress policy and stay a manual flow."examples/backups/mariadb/30-mariadb-target.yaml:15-18: "The CI e2e (hack/e2e-apps/backup-mariadb.bats) deploys this manifest in the same namespace as mariadb-src, exercising the same-namespace to-copy happy path."examples/backups/mariadb/40-restorejob-to-copy.yaml:7-9: "Driven by the CI e2e (hack/e2e-apps/backup-mariadb.bats) for the same-namespace happy path."
hack/e2e-apps/backup-mariadb.bats does not exist on this branch (find hack -name 'backup-mariadb*' returns nothing; hack/e2e-apps/ contains mariadb.bats and a disabled backup.bats.disabled that has zero MariaDB references). A reader following the README to "see how the CI exercises this" hits a dead end. Per the rubric, documentation that contradicts the code is a blocking issue.
Fix: either land the bats file in the same PR (preferred — there is matching prior art for the CNPG/Altinity strategies; the file should exercise apply -f on the numbered manifests, write a sentinel row via the app user, kick the ad-hoc BackupJob, wait for BackupRef, run 35-restorejob-in-place.yaml, and verify the sentinel survives), or remove the three references and replace them with neutral phrasing ("smoke-testable manually with kubectl apply -f"). Do not leave references to a missing file.
Required test (for the "remove references" path): grep-style smoke verification is brittle (see project rule against parsing-style tests in ~/.claude/CLAUDE.md), but the artifact itself is exercised by find hack/e2e-apps -name backup-mariadb.bats in CI's existing hack/ discovery if such a check exists — preferred path is just to land the bats file and use the harness's own existence as the test.
Documentation drift — mariadbapp/types.go contradicts the actual driver behaviour
internal/backupcontroller/mariadbapp/types.go:55-64:
// MariaDBSpec is intentionally empty: the driver does not currently read or
// patch any apps.cozystack.io/MariaDB spec fields. It targets the downstream
// k8s.mariadb.com/MariaDB CR (rendered by the chart's HelmRelease) for both
// bootstrapFrom patching and existence checks.The driver does NOT patch bootstrapFrom. The strategy-level docstring at api/backups/strategy/v1alpha1/mariadb_types.go:36-39 (and the rendered CRD at definitions/strategy.backups.cozystack.io_mariadbs.yaml:27-28) explicitly says "the driver does not touch MariaDB.spec.bootstrapFrom." The RBAC at packages/system/backupstrategy-controller/templates/rbac.yaml:117-119 confirms it: verbs: ["get", "list", "watch"] on k8s.mariadb.com/mariadbs. The mariadbapp/types.go comment is straight-up wrong.
This is documentation drift inside the diff. Per the rubric "if the PR changes behaviour but existing documentation still describes the old behaviour — blocking", and this file is part of the PR.
Fix: replace lines 56-59 to drop the bootstrapFrom claim. Suggested replacement:
// MariaDBSpec is intentionally empty: the driver does not currently read or
// patch any apps.cozystack.io/MariaDB spec fields. The driver only reads the
// downstream k8s.mariadb.com/MariaDB CR (rendered by the chart's HelmRelease)
// for existence checks - restores create k8s.mariadb.com/Restore CRs whose
// mariaDbRef points at the live MariaDB; the operator replays the dump into
// the running instance, so the driver does NOT touch
// MariaDB.spec.bootstrapFrom or any other field on the operator CR.Required test: a code-level assertion that the driver does not write to mariadbtypes.MariaDB would catch a regression of the comment becoming true again, but per the project rule against grep/parse tests the right place to anchor this is the existing absence of patch verbs in the rbac.yaml and the fact that the only usage of mariadbtypes.MariaDB{} in the controller is the two r.Get calls (lines 168, 531) — no test exercises a Patch because no Patch call exists. That's the de facto guard.
Documentation drift — MariaDBSecretKeySelector.Key claims no templating, but template.Template walks every string
api/backups/strategy/v1alpha1/mariadb_types.go:170-175:
type MariaDBSecretKeySelector struct {
// Name is the Secret name. Templating is supported.
Name string `json:"name"`
// Key is the key within the Secret holding the value.
Key string `json:"key"`
}The docstring on Name says templating is supported; the docstring on Key is silent, which a reader will interpret as "Key is NOT templated." But internal/template/template.go:41-58 walks every string in the JSON-marshalled object and substitutes it through the templating function, so Key is in fact templated. Same caveat applies to MariaDBS3Template.Bucket, Endpoint, Prefix, Region (those are documented correctly), and to MariaDBS3TLS.Enabled (which is a bool — not affected) and CASecretKeyRef.Key (silent — same drift).
This is borderline. The honest options are:
- Update the comments to say templating is supported on every string field of the template.
- Add an explicit "Templating is NOT supported on Key" guard in
renderMariaDBTemplatethat errors out if Key contains{{.
Pick one. Right now the doc says one thing and the code does the other — a tenant who templates Key against .Application.metadata.name to vary per-app keys will get behaviour the docs claim is unsupported.
Required test: if you go with option 1 (documentation only) — no test, just truth in advertising. If you go with option 2 (enforcement) — a unit test that feeds Key: "{{ .Application.metadata.name }}" and asserts renderMariaDBTemplate returns a non-nil error.
Recommendations (non-blocking)
MariaDBSpecinapi/backups/strategy/v1alpha1/mariadb_types.go:57-68documents that Parameters MUST NOT carry credentials. No enforcement exists. If a tenant setsparameters.secretAccessKey: hunter2on aBackupClassStrategy, the value gets templated in, persisted intoBackup.status.underlyingResourcesviamarshalMariaDBBackupSnapshot, and is now world-readable in cluster RBAC scope. Not a regression introduced by this PR (the CNPG strategy has the same shape), but it would be nice to have a future-proof comment atmarshalMariaDBBackupSnapshotreiterating that the snapshot is plaintext-readable and Parameters values land there verbatim.mariadbBackupURI(mariadbstrategy_controller.go:417-427) handles the empty-prefix case correctly (pinned byTestMariaDBBackupURI_EmptyPrefix). One omission: whenprefix == "/"exactly, the helper producess3://bkt//<name>(double slash). Not a bug — that's a valid S3 key — but worth a follow-up "normalise leading-slash prefix" if the operator ever rejects double-slash prefixes. Not blocking.findMariaDBBackupForJob(mariadbstrategy_controller.go:318-341) returnslist.Items[0]on duplicate matches. The fake client lists by name; the real apiserver lists byresourceVersion(ETCd order). The testTestFindMariaDBBackupForJob_DuplicateMatchesaccepts either name on a single call but asserts a second call returns the same one — that's a fake-client artefact, not a property of the real apiserver. Real apiserver list order is stable per cache snapshot but can change between watches. Picking a differentItems[0]between reconciles would result in driving two operator Backups in lockstep (the orphaned one stays Running and gets reaped by the deadline). Acceptable; the V(1) breadcrumb is the right place to surface that we hit the race.mariadbBackupSnapshot.Storagecarries the rendered secret-name fields (e.g.mariadb-src-mariadb-backup-creds). These are pointers — not the secret values — so the snapshot is safe. Re-emphasise this in the snapshot godoc to head off future readers wondering why secret refs are persisted plaintext.
Reminder: "Pre-existing" is not a thing. If a problem is flagged in this review, it must be fixed in this PR — no exceptions.
Every issue listed above must have a corresponding test. No matter the severity — bug, logic error, security issue, error handling gap, documentation drift — each one must be covered by a test that fails without the fix and passes with it. Untested fixes are not fixes.
2a4612c to
9408f84
Compare
Signed-off-by: Andrey Kolkov <[email protected]>
9408f84 to
62605a1
Compare
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
LGTM
Reviewed the single new commit on feat/mariadb-backups (62605a198 feat(platform): implement backup strategy for mariadb) against main upstream. The branch carries other commits already merged upstream as part of PR #2607 (instance-type resource presets); the local main ref is stale and shows them as branch-only, but they are not part of this PR's review surface.
Scope of changes reviewed:
- New
strategy.backups.cozystack.io/v1alpha1.MariaDBCRD (api/backups/strategy/v1alpha1/mariadb_types.go+ zz_generated, CRD yaml inpackages/system/backupstrategy-controller/definitions/) - New driver
internal/backupcontroller/mariadbstrategy_controller.go(~866 LOC) implementing the BackupJob + RestoreJob reconcile paths againstk8s.mariadb.com/v1alpha1Backup/Restore CRs - Local partial mirror of
k8s.mariadb.com/v1alpha1shapes ininternal/backupcontroller/mariadbtypes/and the apps-side handle ininternal/backupcontroller/mariadbapp/ - Dispatch wiring in
backup_controller.go,backupjob_controller.go,restorejob_controller.goandcmd/backupstrategy-controller/main.go - RBAC additions in
packages/system/backupstrategy-controller/templates/rbac.yaml - End-to-end fixture set under
examples/backups/mariadb/ - DEPRECATED-tagging of the legacy chart-side restic CronJob fields (
packages/apps/mariadb/{values.yaml,values.schema.json,README.md}plus the openAPI schema inpackages/system/mariadb-rd/cozyrds/mariadb.yaml)
Build + tests are clean: go build ./..., go vet ./internal/backupcontroller/..., and go test ./internal/backupcontroller/... -count=1 all pass.
Things I checked and confirmed are fine
- Strategy template only accepts Secret REFERENCES (Name+Key) for S3 credentials. No code path persists raw credential values;
marshalMariaDBBackupSnapshotsnapshot is plaintext-readable but only carries Secret refs, not Secret bytes, with the contract documented at the type. RBAC carries no cluster-scoped Secret read verb, which is the right enforcement. XValidationrule onMariaDBStorageTemplateplus the in-Go defence-in-depth check inbuildMariaDBBackupStoragetogether pin the "exactly one of s3 / pvc / volume" invariant at admission AND at runtime; the testTestBuildMariaDBBackupStorage_RejectsMultipleexists specifically to prove the runtime guard fires even if a strategy CR predates admission.- Idempotency on both ensure paths is by
OwningJob{Name,Namespace}label match, withfindMariaDBBackupForJobdeterministic on duplicate hits (TestFindMariaDBBackupForJob_DuplicateMatches). - Stale-cache pattern for
StartedAtmirrors the CNPG path exactly: refetch fresh, patch Status, requeue. markBackupJobFailed/markRestoreJobFailedusemeta.SetStatusCondition, notappend— pinned byTestMarkBackupJobFailed_DoesNotAppendDuplicateReady.- Validation hoist on the RestoreJob path: Kind/APIGroup check runs before
srcBackupGet so a malformed TargetApplicationRef does not waste an apiserver round-trip — pinned byTestReconcileMariaDBRestore_KindCheckedBeforeSrcBackupGet. - The RestoreJob target-MariaDB-not-found branch IS deadline-gated via
options.effectiveRestoreDeadline()— pinned byTestReconcileMariaDBRestore_TargetMariaDBNotFoundIsTransientand (terminal path) byTestReconcileMariaDBRestore_StartedAtPersistedContinuesInline. - Operator-side Backup CR not found on the RestoreJob path correctly maps to a terminal failure (the operator-side artifact has been reaped) — covered by
TestReconcileMariaDBRestore_StartedAtPersistedContinuesInline. mariadbBackupURIcorrectly handles empty Prefix (no leading double slash) — pinned byTestMariaDBBackupURI_EmptyPrefix.- PVC/Volume storage correctly leaves
Status.Artifact = nil(no URI{URI:""} confusion) — pinned byTestCreateMariaDBBackupArtifact_ArtifactShape. - Cozystack Backup deletion explicitly routes MariaDB to a no-op cleanup (not Velero fall-through) — pinned by
TestBackupCleanup_MariaDB_DoesNotFallThroughToVeleroand supported by the RBAC explicitly NOT having adeleteverb onk8s.mariadb.com/backups. - The unsupported-strategy diagnostic and the dispatch switch can no longer drift apart — pinned by
TestSupportedBackupStrategyKindsMatchesDispatch. - The cozystack mariadb HelmRelease prefix (
mariadb-) matchesmariadbNameForApp, verified directly againstpackages/system/mariadb-rd/cozyrds/mariadb.yamland the chart'stemplates/mariadb.yaml(which uses.Release.Name). - Backup CRD does NOT have
+kubebuilder:subresource:status(confirmed inapi/backups/v1alpha1/backup_types.go), soCreatewithStatuspopulated is the documented pattern and matches CNPG/Altinity drivers. - Legacy
backup.*block on the chart is correctly disabled by default (backup.enabled=false) and templates are gated on it, so the DEPRECATED tagging does not change runtime behaviour for existing tenants.
Follow-up backlog (non-blocking — NOT a condition of LGTM)
Surfacing these explicitly so they don't get lost in the merge. Pick up or discard as you see fit; none of them gate this PR.
FU-1 — Align BackupJob "operator-side MariaDB CR not found" with its RestoreJob counterpart
The BackupJob path's "operator-side MariaDB CR not found" branch (internal/backupcontroller/mariadbstrategy_controller.go:169-181) returns a transient Ready=False/MariaDBNotReady + requeue with no deadline gate, while the symmetric RestoreJob branch DOES deadline-gate that case via options.effectiveRestoreDeadline().
- Symptom: If a tenant fires a BackupJob against an
applicationRefwhose chart never produces the operator-side MariaDB CR, the BackupJob is stuckRunninguntil either the CR appears or the user manually deletes the BackupJob. - Eventual safety net: once the MariaDB CR does materialise and
ensureMariaDBBackupcreates an operator-side Backup, the existingmariadbBackupDeadlineExceededgate kicks in 30m later. So the worst case is "no early failure, but eventual failure once the operator CR exists." - Why it's not blocking: the CNPG driver ships the exact same shape today and the trade-off is deliberate there. Aligning the MariaDB driver internally is the smaller of the two follow-ups; aligning all drivers is the bigger one.
- Suggested test when picked up: a unit test asserting that a BackupJob whose
applicationRefpoints at a MariaDB app with no operator-sidek8s.mariadb.com/MariaDBCR transitions toFailedafter theeffectiveBackupDeadline()window, mirroringTestReconcileMariaDBRestore_TargetMariaDBNotFoundIsTransient.
FU-2 — Template engine swallows malformed templates (cross-driver)
internal/template/template.go silently returns the original input on template-evaluation error (if err != nil { return in }).
- Symptom: A typo like
{{ .Application.metadata.name(missing closing brace) in a strategy template yields a literal"{{ .Application.metadata.name"in the rendered operator-side Backup CR. The mariadb-operator then rejects the resulting Backup with its own error message, which is the only breadcrumb the tenant gets. - Not new in this PR: CNPG and Velero drivers share the same engine and the same trade-off. Out of scope for the MariaDB PR.
- Suggested fix when picked up: log the template-evaluation error at
V(0)with the strategy ref attached, OR surface as aReady=False/TemplateRenderFailedcondition on the BackupJob/RestoreJob. Either is a cross-driver change that should live in its own PR.
FU-3 — Automate the COSI BucketInfo → mariadb-operator credential Secret extraction
The example fixture under examples/backups/mariadb/ documents that tenants must hand-roll a per-app <app>-mariadb-backup-creds Secret extracted from the COSI BucketInfo blob, because mariadb-operator expects separate AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY keys, while the COSI BucketClaim materialises a single BucketInfo JSON blob.
- Symptom: known UX gap. The README acknowledges it and defers automation to a follow-up bats e2e harness.
- Why it's not blocking: it is explicitly called out in the example README and is a UX rough edge, not a regression — the existing manual flow works.
- Suggested shape: either a small webhook/controller that watches
BucketInfoSecrets matching a label and projects out the two operator-shaped keys, or build the projection into the BackupClass / Plan reconcile path. The bats e2e harness will need this either way.
Ticket Compliance: not applicable (no --ticket was provided).
Andrei Kvapil (kvaps)
added
the
kind/backport
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-1.3
git worktree add -d .worktree/backport-2605-to-release-1.3 origin/release-1.3
cd .worktree/backport-2605-to-release-1.3
git switch --create backport-2605-to-release-1.3
git cherry-pick -x 62605a198bfc0d8f3d834d01bd6c044a64c2ad8b |
Andrei Kvapil (kvaps)
removed
the
kind/backport
3 participants
What this PR does
Screenshots
Release note
Summary by CodeRabbit
New Features
Documentation
Tests
Chores