Repository navigation
feat(platform): add foundationdb backups - #2606
Conversation
github-actions
Bot
added
the
size/XXL
|
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 a FoundationDB backup/restore strategy: public strategy CRDs, internal app/operator type mirrors, controller scheme/dispatch wiring, strategy reconciler with helpers, comprehensive tests, example scripts and docs, RBAC, and deprecation notices for legacy in-chart backups. ChangesFoundationDB Backup Strategy Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
🚥 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 |
github-actions
Bot
added
area/platform
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 introduces a new backup framework for FoundationDB and MariaDB applications by delegating backup and restore operations to their respective Kubernetes operators. By moving away from in-chart backup mechanisms, this change provides a more robust, standardized, and extensible approach to data protection within the Cozystack platform. It includes comprehensive API definitions, controller logic for managing backup and restore jobs, and detailed documentation and examples for users to migrate to the new strategy-based workflow. 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
|
![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 new backup and restore strategy for FoundationDB and MariaDB applications by integrating with the operator-native APIs (k8s.mariadb.com and apps.foundationdb.org). It adds the necessary controllers, types, and RBAC permissions to support these strategies, along with comprehensive documentation and example scripts for end-to-end backup/restore workflows. The changes also include deprecation notices for the legacy in-chart backup mechanisms in both the MariaDB and FoundationDB charts. My review identified a few minor issues regarding error handling and robustness in the FoundationDB strategy controller, which are addressed in the feedback.
| - apiGroups: ["k8s.mariadb.com"] | ||
| resources: ["mariadbs"] | ||
| verbs: ["get", "list", "watch"] | ||
| - apiGroups: ["k8s.mariadb.com"] | ||
| resources: ["backups", "restores"] | ||
| verbs: ["get", "list", "watch", "create"] |
There was a problem hiding this comment.
The RBAC rules for k8s.mariadb.com include create permissions for backups and restores. Ensure that the controller-runtime manager is configured with a restricted namespace if Cozystack's security model requires tenant isolation at the API level, as this allows the controller to create these resources in any namespace it can watch.
References
- Flag overly broad RBAC permissions. (link)
| if err := json.Unmarshal(raw.Raw, snap); err != nil { | ||
| return nil, err | ||
| } | ||
| if snap.Kind != foundationdbBackupSnapshotKind { |
There was a problem hiding this comment.
The check if snap.Kind != foundationdbBackupSnapshotKind returns nil, nil. This makes it indistinguishable from a missing snapshot. It would be safer to return an error if the Kind is present but incorrect, as it indicates the UnderlyingResources field contains an unexpected type.
| if snap.Kind != foundationdbBackupSnapshotKind { | |
| if snap.Kind != foundationdbBackupSnapshotKind { | |
| return nil, fmt.Errorf("unexpected snapshot kind: %s", snap.Kind) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
api/backups/strategy/v1alpha1/mariadb_types.go (1)
101-119: ⚡ Quick winConsider adding CRD validation for the storage type constraint.
The comment states "Exactly one of S3, PersistentVolumeClaim or Volume must be set," but there's no kubebuilder validation enforcing this constraint. Consider adding a CEL validation rule to catch configuration errors at admission time rather than during reconciliation.
🔍 Suggested kubebuilder validation
Add a validation rule to
MariaDBStorageTemplate:// 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 {This CEL rule counts the number of non-null storage fields and ensures exactly one is present.
🤖 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 101 - 119, Add a CEL XValidation kubebuilder tag on the MariaDBStorageTemplate type to enforce that exactly one of the storage fields is set: S3, PersistentVolumeClaim or Volume; update the type comment and add a +kubebuilder:validation:XValidation annotation on MariaDBStorageTemplate that evaluates the presence of self.s3, self.persistentVolumeClaim and self.volume and asserts their count equals 1 so the admission webhook rejects CRs with zero or multiple storage fields set.internal/backupcontroller/foundationdbstrategy_controller.go (1)
600-610: ⚡ Quick winAvoid silently swallowing snapshot decode errors.
When
decodeFoundationDBBackupSnapshotreturns a realjson.Unmarshalfailure (i.e.underlyingResources.Rawis malformed, not just a foreignKind), the error is discarded and the function silently degrades to thedriverMetadatafallback. If the snapshot path was the only source carrying e.g.CustomParameters/EncryptionKeyPath, the restore proceeds with partial config and no breadcrumb tying the surprise back to a bad payload. Same pattern the MariaDB driver uses forMalformedOptionswould help: log + event, then fall through.♻️ Suggested change
- snap, _ := decodeFoundationDBBackupSnapshot(backup.Status.UnderlyingResources) + snap, err := decodeFoundationDBBackupSnapshot(backup.Status.UnderlyingResources) + if err != nil { + logger.Info("malformed Backup.status.underlyingResources; falling back to driverMetadata", "error", err) + r.Recorder.Eventf(restoreJob, corev1.EventTypeWarning, "MalformedSnapshot", + "Backup.status.underlyingResources is not a valid FoundationDB snapshot; falling back to driverMetadata: %v", err) + }🤖 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/foundationdbstrategy_controller.go` around lines 600 - 610, Capture and handle the error returned by decodeFoundationDBBackupSnapshot instead of discarding it: change the call to receive an err (snap, err := decodeFoundationDBBackupSnapshot(...)); if err != nil then log the error via the controller logger and emit a Kubernetes event on the restoreJob (similar to the MariaDB MalformedOptions pattern) to record the malformed snapshot payload, then continue to fall back to resolveFoundationDBRestoreBlob as before; reference decodeFoundationDBBackupSnapshot, resolveFoundationDBRestoreBlob and the restoreJob/event/logging helpers so reviewers can find and follow the change.internal/backupcontroller/mariadbstrategy_controller.go (1)
30-32: 💤 Low valueUnused constant
mariadbFieldManager.Declared but not referenced anywhere in the package; golangci-lint flags it. Either wire it into the operator-side
Backup/RestoreCreate/Patchcalls as a server-side apply field manager, or drop the constant until it's actually needed.♻️ If unused, remove
- mariadbFieldManager = "cozystack-mariadb-backup-driver" - // mariadb-operator's MariaDB ApplicationDefinition (release.prefix). The🤖 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 30 - 32, The const mariadbFieldManager is declared but unused; either remove it or wire it into server-side apply options for Backup/Restore operations: locate the operator-side Backup and Restore create/patch calls (e.g., places calling client.Create or client.Patch for Backup/Restore resources) and pass the field owner option using mariadbFieldManager (e.g., client.FieldOwner(mariadbFieldManager) or the equivalent Create/Patch option your controller-runtime version supports); if you choose not to use it now, delete the mariadbFieldManager constant to satisfy the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/backups/foundationdb/01-create-strategy.sh`:
- Around line 65-66: Replace the securityContext entry that sets runAsUser: 0
with runAsUser: 4059 for the backup container so it runs as the non-root
FoundationDB UID; update the same change for the template using backup_agent
(the YAML block containing securityContext and runAsUser) to match the
operator/main DB UID 4059 rather than root. Ensure there are no other
conflicting securityContext/runAsUser entries for the backup container and keep
any existing capabilities/volume mounts intact.
In `@examples/backups/foundationdb/90-scenario-admin-prepare.md`:
- Around line 31-34: Update the fenced code block containing the kubectl
commands so it includes a language specifier (e.g., change the opening ``` to
```bash) to enable proper syntax highlighting for the two lines "kubectl get
foundationdbs.strategy.backups.cozystack.io" and "kubectl get
backupclasses.backups.cozystack.io foundationdb-default"; simply add the
language token on the existing opening fence for that block.
In `@examples/backups/foundationdb/91-scenario-user-backup.md`:
- Around line 50-55: The fenced code block showing kubectl commands is missing a
language specifier; update the block delimiter to include a shell language
(e.g., "bash" or "shell") so tools render syntax highlighting and accessibility
correctly—modify the fenced block around the kubectl commands in the
91-scenario-user-backup snippet to start with ```bash (or ```shell) instead of
just ```, leaving the command lines (kubectl -n <ns> get
backupjobs.backups.cozystack.io <name> -o yaml, kubectl -n <ns> get
backups.backups.cozystack.io <name> -o yaml, kubectl -n <ns> get
foundationdbbackups.apps.foundationdb.org -l
backups.cozystack.io/owned-by.BackupJobName=<name>) unchanged.
In `@examples/backups/foundationdb/92-scenario-user-restore.md`:
- Around line 44-48: The fenced code block containing the kubectl commands
starting with "kubectl -n <ns> get restorejobs.backups.cozystack.io <name> -o
yaml" and the follow-up "kubectl -n <ns> get
foundationdbrestores.apps.foundationdb.org ..." should declare a language for
markdown linting; replace the opening triple backticks with an annotated fence
(e.g., ```bash) so the block becomes a bash code block.
In `@internal/backupcontroller/mariadbstrategy_controller.go`:
- Around line 751-796: The error text in buildMariaDBBackupStorage promises
"exactly one" but the switch allows the first non-nil of in.S3,
in.PersistentVolumeClaim, in.Volume to win, so update the function to enforce
exclusivity: before the switch (or instead of it) count the non-nil fields
(in.S3, in.PersistentVolumeClaim, in.Volume) and return an error if count != 1
with the same user-facing message; then proceed to construct the appropriate
mariadbtypes.BackupStorage for the single non-nil field. This ensures
buildMariaDBBackupStorage enforces the one-of contract rather than silently
preferring the first set field.
---
Nitpick comments:
In `@api/backups/strategy/v1alpha1/mariadb_types.go`:
- Around line 101-119: Add a CEL XValidation kubebuilder tag on the
MariaDBStorageTemplate type to enforce that exactly one of the storage fields is
set: S3, PersistentVolumeClaim or Volume; update the type comment and add a
+kubebuilder:validation:XValidation annotation on MariaDBStorageTemplate that
evaluates the presence of self.s3, self.persistentVolumeClaim and self.volume
and asserts their count equals 1 so the admission webhook rejects CRs with zero
or multiple storage fields set.
In `@internal/backupcontroller/foundationdbstrategy_controller.go`:
- Around line 600-610: Capture and handle the error returned by
decodeFoundationDBBackupSnapshot instead of discarding it: change the call to
receive an err (snap, err := decodeFoundationDBBackupSnapshot(...)); if err !=
nil then log the error via the controller logger and emit a Kubernetes event on
the restoreJob (similar to the MariaDB MalformedOptions pattern) to record the
malformed snapshot payload, then continue to fall back to
resolveFoundationDBRestoreBlob as before; reference
decodeFoundationDBBackupSnapshot, resolveFoundationDBRestoreBlob and the
restoreJob/event/logging helpers so reviewers can find and follow the change.
In `@internal/backupcontroller/mariadbstrategy_controller.go`:
- Around line 30-32: The const mariadbFieldManager is declared but unused;
either remove it or wire it into server-side apply options for Backup/Restore
operations: locate the operator-side Backup and Restore create/patch calls
(e.g., places calling client.Create or client.Patch for Backup/Restore
resources) and pass the field owner option using mariadbFieldManager (e.g.,
client.FieldOwner(mariadbFieldManager) or the equivalent Create/Patch option
your controller-runtime version supports); if you choose not to use it now,
delete the mariadbFieldManager constant to satisfy the linter.
🪄 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: 022f2364-9592-45d2-841e-66af66a141eb
📒 Files selected for processing (46)
api/apps/v1alpha1/foundationdb/types.goapi/apps/v1alpha1/mariadb/types.goapi/backups/strategy/v1alpha1/foundationdb_types.goapi/backups/strategy/v1alpha1/mariadb_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gocmd/backupstrategy-controller/main.goexamples/backups/foundationdb/00-helpers.shexamples/backups/foundationdb/01-create-strategy.shexamples/backups/foundationdb/02-create-backupclass.shexamples/backups/foundationdb/03-create-bucket.shexamples/backups/foundationdb/04-create-foundationdb-src.shexamples/backups/foundationdb/05-create-backupjob.shexamples/backups/foundationdb/06-restore-in-place.shexamples/backups/foundationdb/07-restore-to-copy.shexamples/backups/foundationdb/90-scenario-admin-prepare.mdexamples/backups/foundationdb/91-scenario-user-backup.mdexamples/backups/foundationdb/92-scenario-user-restore.mdexamples/backups/foundationdb/README.mdexamples/backups/foundationdb/cleanup.shexamples/backups/foundationdb/run-all.shinternal/backupcontroller/backupjob_controller.gointernal/backupcontroller/backupjob_controller_test.gointernal/backupcontroller/foundationdbapp/types.gointernal/backupcontroller/foundationdbapp/zz_generated.deepcopy.gointernal/backupcontroller/foundationdbstrategy_controller.gointernal/backupcontroller/foundationdbstrategy_controller_test.gointernal/backupcontroller/foundationdbtypes/types.gointernal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.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/foundationdb/README.mdpackages/apps/foundationdb/values.schema.jsonpackages/apps/foundationdb/values.yamlpackages/apps/mariadb/README.mdpackages/apps/mariadb/values.schema.jsonpackages/apps/mariadb/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_foundationdbs.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mariadbs.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/mariadb-rd/cozyrds/mariadb.yaml
f3a9bc6 to
bd3989c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
examples/backups/foundationdb/90-scenario-admin-prepare.md (1)
31-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language specifier to code block.
The fenced code block should specify
bashorshellfor proper syntax highlighting and accessibility.📝 Proposed fix
-``` +```bash kubectl get foundationdbs.strategy.backups.cozystack.io kubectl get backupclasses.backups.cozystack.io foundationdb-default</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/foundationdb/90-scenario-admin-prepare.mdaround lines 31 -
34, The fenced code block containing the two kubectl commands is missing a
language specifier; update the opening fence to include bash (i.e., changetobash) for the block with the lines "kubectl get
foundationdbs.strategy.backups.cozystack.io" and "kubectl get
backupclasses.backups.cozystack.io foundationdb-default" so the snippet is
properly highlighted and accessible.</details> </blockquote></details> <details> <summary>examples/backups/foundationdb/91-scenario-user-backup.md (1)</summary><blockquote> `50-55`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language identifier to the fenced code block.** The code block should specify `bash` as the language for proper syntax highlighting and markdown linting compliance. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```bash kubectl -n <ns> get backupjobs.backups.cozystack.io <name> -o yaml kubectl -n <ns> get backups.backups.cozystack.io <name> -o yaml kubectl -n <ns> get foundationdbbackups.apps.foundationdb.org \ -l backups.cozystack.io/owned-by.BackupJobName=<name> ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/foundationdb/91-scenario-user-backup.mdaround lines 50 -
55, Update the fenced code block containing the kubectl commands (the block
starting with "kubectl -n get backupjobs.backups.cozystack.io -o
yaml" and including the two following kubectl lines) to include the language
identifier by changing the opening triple backticks fromtobash so the
block becomes a bash code block; leave the closing ``` unchanged.</details> </blockquote></details> <details> <summary>examples/backups/foundationdb/92-scenario-user-restore.md (1)</summary><blockquote> `44-48`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language identifier to the fenced code block.** The code block should specify `bash` as the language for proper syntax highlighting and markdown linting compliance. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```bash kubectl -n <ns> get restorejobs.backups.cozystack.io <name> -o yaml kubectl -n <ns> get foundationdbrestores.apps.foundationdb.org \ -l backups.cozystack.io/owned-by.BackupJobName=<name> ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/foundationdb/92-scenario-user-restore.mdaround lines 44 -
48, The fenced code block containing the two kubectl commands should include the
language identifier "bash" for proper syntax highlighting and markdown linting;
update the block that wraps the lines starting with "kubectl -n get
restorejobs.backups.cozystack.io -o yaml" and "kubectl -n get
foundationdbrestores.apps.foundationdb.org ..." so the opening fence is
"bash" instead of "".</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>examples/backups/mariadb/05-mariadb-src.yaml (1)</summary><blockquote> `28-28`: _⚡ Quick win_ **Consider adding a comment warning about the placeholder password.** While `REPLACE_WITH_PASSWORD` is self-evident, adding an explicit comment reminding users to replace this with a secure password before deployment would improve clarity for those following the examples. <details> <summary>📝 Suggested enhancement</summary> ```diff users: app: + # IMPORTANT: Replace with a secure password before deploying password: REPLACE_WITH_PASSWORD maxUserConnections: 100 ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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` at line 28, The YAML contains a placeholder value "password: REPLACE_WITH_PASSWORD"; add an inline comment immediately after this key (e.g., "password: REPLACE_WITH_PASSWORD # TODO: replace with a secure password before deployment") to warn users to replace the placeholder with a secure password and avoid committing defaults to production. ``` </details> </blockquote></details> <details> <summary>internal/backupcontroller/mariadbstrategy_controller.go (1)</summary><blockquote> `31-31`: _💤 Low value_ **Remove unused constant or add comment explaining future intent.** `mariadbFieldManager` is never referenced in the codebase. Either remove it to keep the code clean, or add a comment if it's reserved for future client-side apply operations. <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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` at line 31, The constant mariadbFieldManager is defined but never used; either remove the declaration of mariadbFieldManager from mariadbstrategy_controller.go to avoid dead code, or if you intend to reserve it for future client-side apply usage, add a short comment above the mariadbFieldManager constant explaining its intended purpose (e.g., "reserved for client-side apply fieldManager name") so its presence is justified. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@examples/backups/foundationdb/90-scenario-admin-prepare.md:
- Around line 31-34: The fenced code block containing the two kubectl commands
is missing a language specifier; update the opening fence to include bash (i.e.,
changetobash) for the block with the lines "kubectl get
foundationdbs.strategy.backups.cozystack.io" and "kubectl get
backupclasses.backups.cozystack.io foundationdb-default" so the snippet is
properly highlighted and accessible.In
@examples/backups/foundationdb/91-scenario-user-backup.md:
- Around line 50-55: Update the fenced code block containing the kubectl
commands (the block starting with "kubectl -n get
backupjobs.backups.cozystack.io -o yaml" and including the two following
kubectl lines) to include the language identifier by changing the opening triple
backticks fromtobash so the block becomes a bash code block; leave the
closing ``` unchanged.In
@examples/backups/foundationdb/92-scenario-user-restore.md:
- Around line 44-48: The fenced code block containing the two kubectl commands
should include the language identifier "bash" for proper syntax highlighting and
markdown linting; update the block that wraps the lines starting with "kubectl
-n get restorejobs.backups.cozystack.io -o yaml" and "kubectl -n
get foundationdbrestores.apps.foundationdb.org ..." so the opening fence is
"bash" instead of "".
Nitpick comments:
In@examples/backups/mariadb/05-mariadb-src.yaml:
- Line 28: The YAML contains a placeholder value "password:
REPLACE_WITH_PASSWORD"; add an inline comment immediately after this key (e.g.,
"password: REPLACE_WITH_PASSWORD # TODO: replace with a secure password before
deployment") to warn users to replace the placeholder with a secure password and
avoid committing defaults to production.In
@internal/backupcontroller/mariadbstrategy_controller.go:
- Line 31: The constant mariadbFieldManager is defined but never used; either
remove the declaration of mariadbFieldManager from mariadbstrategy_controller.go
to avoid dead code, or if you intend to reserve it for future client-side apply
usage, add a short comment above the mariadbFieldManager constant explaining its
intended purpose (e.g., "reserved for client-side apply fieldManager name") so
its presence is justified.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `4ee45ca0-3af2-49b6-80a5-99bf041eb699` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f960c397ad17283cf4c0199059f6d2b0c6cee173 and bd3989cf1e8d84d85ce379f07274d8d9999220ce. </details> <details> <summary>📒 Files selected for processing (56)</summary> * `api/apps/v1alpha1/foundationdb/types.go` * `api/apps/v1alpha1/mariadb/types.go` * `api/backups/strategy/v1alpha1/foundationdb_types.go` * `api/backups/strategy/v1alpha1/mariadb_types.go` * `api/backups/strategy/v1alpha1/zz_generated.deepcopy.go` * `cmd/backupstrategy-controller/main.go` * `examples/backups/foundationdb/00-helpers.sh` * `examples/backups/foundationdb/01-create-strategy.sh` * `examples/backups/foundationdb/02-create-backupclass.sh` * `examples/backups/foundationdb/03-create-bucket.sh` * `examples/backups/foundationdb/04-create-foundationdb-src.sh` * `examples/backups/foundationdb/05-create-backupjob.sh` * `examples/backups/foundationdb/06-restore-in-place.sh` * `examples/backups/foundationdb/07-restore-to-copy.sh` * `examples/backups/foundationdb/90-scenario-admin-prepare.md` * `examples/backups/foundationdb/91-scenario-user-backup.md` * `examples/backups/foundationdb/92-scenario-user-restore.md` * `examples/backups/foundationdb/README.md` * `examples/backups/foundationdb/cleanup.sh` * `examples/backups/foundationdb/run-all.sh` * `examples/backups/mariadb/00-bucket.yaml` * `examples/backups/mariadb/05-mariadb-src.yaml` * `examples/backups/mariadb/10-mariadb-strategy.yaml` * `examples/backups/mariadb/15-backupclass.yaml` * `examples/backups/mariadb/20-plan.yaml` * `examples/backups/mariadb/25-backupjob-adhoc.yaml` * `examples/backups/mariadb/30-mariadb-target.yaml` * `examples/backups/mariadb/35-restorejob-in-place.yaml` * `examples/backups/mariadb/40-restorejob-to-copy.yaml` * `examples/backups/mariadb/README.md` * `internal/backupcontroller/backupjob_controller.go` * `internal/backupcontroller/backupjob_controller_test.go` * `internal/backupcontroller/foundationdbapp/types.go` * `internal/backupcontroller/foundationdbapp/zz_generated.deepcopy.go` * `internal/backupcontroller/foundationdbstrategy_controller.go` * `internal/backupcontroller/foundationdbstrategy_controller_test.go` * `internal/backupcontroller/foundationdbtypes/types.go` * `internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go` * `internal/backupcontroller/mariadbapp/types.go` * `internal/backupcontroller/mariadbapp/zz_generated.deepcopy.go` * `internal/backupcontroller/mariadbstrategy_controller.go` * `internal/backupcontroller/mariadbstrategy_controller_test.go` * `internal/backupcontroller/mariadbtypes/types.go` * `internal/backupcontroller/mariadbtypes/zz_generated.deepcopy.go` * `internal/backupcontroller/restorejob_controller.go` * `internal/backupcontroller/restorejob_controller_test.go` * `packages/apps/foundationdb/README.md` * `packages/apps/foundationdb/values.schema.json` * `packages/apps/foundationdb/values.yaml` * `packages/apps/mariadb/README.md` * `packages/apps/mariadb/values.schema.json` * `packages/apps/mariadb/values.yaml` * `packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_foundationdbs.yaml` * `packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_mariadbs.yaml` * `packages/system/backupstrategy-controller/templates/rbac.yaml` * `packages/system/mariadb-rd/cozyrds/mariadb.yaml` </details> <details> <summary>✅ Files skipped from review due to trivial changes (14)</summary> * examples/backups/mariadb/00-bucket.yaml * packages/apps/foundationdb/README.md * examples/backups/mariadb/25-backupjob-adhoc.yaml * examples/backups/mariadb/README.md * packages/system/mariadb-rd/cozyrds/mariadb.yaml * examples/backups/mariadb/30-mariadb-target.yaml * packages/apps/mariadb/values.schema.json * packages/apps/mariadb/README.md * examples/backups/foundationdb/README.md * internal/backupcontroller/mariadbapp/types.go * api/apps/v1alpha1/foundationdb/types.go * internal/backupcontroller/foundationdbapp/zz_generated.deepcopy.go * api/backups/strategy/v1alpha1/zz_generated.deepcopy.go * packages/apps/foundationdb/values.schema.json </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (17)</summary> * packages/apps/foundationdb/values.yaml * internal/backupcontroller/restorejob_controller.go * examples/backups/foundationdb/run-all.sh * cmd/backupstrategy-controller/main.go * internal/backupcontroller/mariadbapp/zz_generated.deepcopy.go * internal/backupcontroller/restorejob_controller_test.go * internal/backupcontroller/backupjob_controller.go * api/backups/strategy/v1alpha1/mariadb_types.go * api/backups/strategy/v1alpha1/foundationdb_types.go * examples/backups/foundationdb/00-helpers.sh * internal/backupcontroller/backupjob_controller_test.go * internal/backupcontroller/foundationdbtypes/types.go * api/apps/v1alpha1/mariadb/types.go * internal/backupcontroller/foundationdbapp/types.go * internal/backupcontroller/foundationdbstrategy_controller_test.go * internal/backupcontroller/foundationdbstrategy_controller.go * internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
bd3989c to
faf4410
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/backups/foundationdb/03-create-bucket.sh (1)
109-118: ⚡ Quick winSerialize cached env values with shell-safe quoting.
These exports are written unquoted; special characters in any value can make
.bucket-info.envunparsable when sourced later. Use shell-escaped output viaprintf %q.Suggested patch
-umask 077 -cat > "$SCRIPT_DIR/.bucket-info.env" <<ENV -export FDB_ACCESS_KEY=${FDB_ACCESS_KEY} -export FDB_SECRET_KEY=${FDB_SECRET_KEY} -export FDB_ENDPOINT_FULL=${FDB_ENDPOINT_FULL} -export FDB_ENDPOINT_HOSTPORT=${FDB_ENDPOINT_HOSTPORT} -export FDB_REGION=${FDB_REGION} -export FDB_BUCKET=${FDB_BUCKET} -export FDB_ACCOUNT_NAME=${FDB_ACCOUNT_NAME} -export SECURE_CONNECTION=${SECURE_CONNECTION} -ENV +umask 077 +{ + printf 'export FDB_ACCESS_KEY=%q\n' "$FDB_ACCESS_KEY" + printf 'export FDB_SECRET_KEY=%q\n' "$FDB_SECRET_KEY" + printf 'export FDB_ENDPOINT_FULL=%q\n' "$FDB_ENDPOINT_FULL" + printf 'export FDB_ENDPOINT_HOSTPORT=%q\n' "$FDB_ENDPOINT_HOSTPORT" + printf 'export FDB_REGION=%q\n' "$FDB_REGION" + printf 'export FDB_BUCKET=%q\n' "$FDB_BUCKET" + printf 'export FDB_ACCOUNT_NAME=%q\n' "$FDB_ACCOUNT_NAME" + printf 'export SECURE_CONNECTION=%q\n' "$SECURE_CONNECTION" +} > "$SCRIPT_DIR/.bucket-info.env" chmod 600 "$SCRIPT_DIR/.bucket-info.env"🤖 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/foundationdb/03-create-bucket.sh` around lines 109 - 118, The here-doc writing to "$SCRIPT_DIR/.bucket-info.env" writes unquoted exports (e.g., FDB_ACCESS_KEY, FDB_SECRET_KEY, FDB_ENDPOINT_FULL, etc.) which can break when values contain spaces or special chars; change the creation to emit shell-safe quoted values by printing each export using a shell-escaped formatter (e.g., use printf with %q for each variable) so the file contains lines like export VAR=<shell-escaped-value>, ensuring .bucket-info.env remains sourceable and robust.
🤖 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/foundationdb/03-create-bucket.sh`:
- Around line 109-118: The here-doc writing to "$SCRIPT_DIR/.bucket-info.env"
writes unquoted exports (e.g., FDB_ACCESS_KEY, FDB_SECRET_KEY,
FDB_ENDPOINT_FULL, etc.) which can break when values contain spaces or special
chars; change the creation to emit shell-safe quoted values by printing each
export using a shell-escaped formatter (e.g., use printf with %q for each
variable) so the file contains lines like export VAR=<shell-escaped-value>,
ensuring .bucket-info.env remains sourceable and robust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35f4e18b-f813-4616-ab78-93b37c79f283
📒 Files selected for processing (58)
api/apps/v1alpha1/foundationdb/types.goapi/apps/v1alpha1/mariadb/types.goapi/backups/strategy/v1alpha1/foundationdb_types.goapi/backups/strategy/v1alpha1/mariadb_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gocmd/backupstrategy-controller/main.goexamples/backups/foundationdb/00-helpers.shexamples/backups/foundationdb/01-create-strategy.shexamples/backups/foundationdb/02-create-backupclass.shexamples/backups/foundationdb/03-create-bucket.shexamples/backups/foundationdb/04-create-foundationdb-src.shexamples/backups/foundationdb/05-create-backupjob.shexamples/backups/foundationdb/06-restore-in-place.shexamples/backups/foundationdb/07-restore-to-copy.shexamples/backups/foundationdb/90-scenario-admin-prepare.mdexamples/backups/foundationdb/91-scenario-user-backup.mdexamples/backups/foundationdb/92-scenario-user-restore.mdexamples/backups/foundationdb/README.mdexamples/backups/foundationdb/cleanup.shexamples/backups/foundationdb/run-all.shexamples/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/backup_controller.gointernal/backupcontroller/backup_controller_test.gointernal/backupcontroller/backupjob_controller.gointernal/backupcontroller/backupjob_controller_test.gointernal/backupcontroller/foundationdbapp/types.gointernal/backupcontroller/foundationdbapp/zz_generated.deepcopy.gointernal/backupcontroller/foundationdbstrategy_controller.gointernal/backupcontroller/foundationdbstrategy_controller_test.gointernal/backupcontroller/foundationdbtypes/types.gointernal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.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/foundationdb/README.mdpackages/apps/foundationdb/values.schema.jsonpackages/apps/foundationdb/values.yamlpackages/apps/mariadb/README.mdpackages/apps/mariadb/values.schema.jsonpackages/apps/mariadb/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_foundationdbs.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 (17)
- examples/backups/mariadb/20-plan.yaml
- examples/backups/mariadb/README.md
- packages/apps/mariadb/README.md
- examples/backups/foundationdb/README.md
- examples/backups/mariadb/40-restorejob-to-copy.yaml
- packages/apps/foundationdb/values.yaml
- packages/apps/foundationdb/README.md
- examples/backups/foundationdb/run-all.sh
- examples/backups/mariadb/15-backupclass.yaml
- packages/system/mariadb-rd/cozyrds/mariadb.yaml
- internal/backupcontroller/restorejob_controller.go
- packages/apps/mariadb/values.schema.json
- api/apps/v1alpha1/mariadb/types.go
- packages/apps/foundationdb/values.schema.json
- api/apps/v1alpha1/foundationdb/types.go
- internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go
- api/backups/strategy/v1alpha1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (22)
- cmd/backupstrategy-controller/main.go
- examples/backups/mariadb/05-mariadb-src.yaml
- internal/backupcontroller/backupjob_controller.go
- examples/backups/mariadb/30-mariadb-target.yaml
- examples/backups/mariadb/10-mariadb-strategy.yaml
- internal/backupcontroller/restorejob_controller_test.go
- examples/backups/mariadb/35-restorejob-in-place.yaml
- examples/backups/mariadb/25-backupjob-adhoc.yaml
- internal/backupcontroller/foundationdbapp/types.go
- internal/backupcontroller/foundationdbapp/zz_generated.deepcopy.go
- internal/backupcontroller/backupjob_controller_test.go
- internal/backupcontroller/mariadbapp/zz_generated.deepcopy.go
- internal/backupcontroller/foundationdbstrategy_controller_test.go
- api/backups/strategy/v1alpha1/mariadb_types.go
- internal/backupcontroller/mariadbtypes/types.go
- internal/backupcontroller/mariadbapp/types.go
- examples/backups/foundationdb/00-helpers.sh
- internal/backupcontroller/foundationdbtypes/types.go
- internal/backupcontroller/mariadbtypes/zz_generated.deepcopy.go
- internal/backupcontroller/foundationdbstrategy_controller.go
- internal/backupcontroller/mariadbstrategy_controller_test.go
- internal/backupcontroller/mariadbstrategy_controller.go
90da70d to
6d80ec0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
examples/backups/foundationdb/91-scenario-user-backup.md (1)
50-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language specifier to fenced code block.
This issue was previously flagged and remains unaddressed. The code block should specify
bashfor proper syntax highlighting and accessibility.📝 Proposed fix
-``` +```bash kubectl -n <ns> get backupjobs.backups.cozystack.io <name> -o yaml kubectl -n <ns> get backups.backups.cozystack.io <name> -o yaml kubectl -n <ns> get foundationdbbackups.apps.foundationdb.org \ -l backups.cozystack.io/owned-by.BackupJobName=<name></details> <details> <summary>🤖 Prompt for AI Agents</summary>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/foundationdb/91-scenario-user-backup.mdaround lines 50 -
55, The fenced code block containing the kubectl examples (the triple-backtick
block that starts before "kubectl -n get backupjobs.backups.cozystack.io
-o yaml" and ends after the foundationdbbackups command) needs a language
specifier; change the opening fence fromtobash so the block is marked
as bash for proper syntax highlighting and accessibility while leaving the
kubectl command lines unchanged.</details> </blockquote></details> <details> <summary>examples/backups/foundationdb/92-scenario-user-restore.md (1)</summary><blockquote> `44-48`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language identifier to the fenced code block.** This issue was previously flagged and remains unaddressed. The code block should specify `bash` to improve readability and satisfy markdown lint. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```bash kubectl -n <ns> get restorejobs.backups.cozystack.io <name> -o yaml kubectl -n <ns> get foundationdbrestores.apps.foundationdb.org \ -l backups.cozystack.io/owned-by.BackupJobName=<name> ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/foundationdb/92-scenario-user-restore.mdaround lines 44 -
48, The fenced code block containing the kubectl commands should include a
language identifier to enable syntax highlighting; change the opening fence from
tobash for the block that starts with "kubectl -n get
restorejobs.backups.cozystack.io -o yaml" (the block that also contains
the "kubectl -n get foundationdbrestores.apps.foundationdb.org" command) so
the markdown linter and readers recognize it as bash.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@examples/backups/foundationdb/91-scenario-user-backup.md:
- Around line 50-55: The fenced code block containing the kubectl examples (the
triple-backtick block that starts before "kubectl -n get
backupjobs.backups.cozystack.io -o yaml" and ends after the
foundationdbbackups command) needs a language specifier; change the opening
fence fromtobash so the block is marked as bash for proper syntax
highlighting and accessibility while leaving the kubectl command lines
unchanged.In
@examples/backups/foundationdb/92-scenario-user-restore.md:
- Around line 44-48: The fenced code block containing the kubectl commands
should include a language identifier to enable syntax highlighting; change the
opening fence fromtobash for the block that starts with "kubectl -n
get restorejobs.backups.cozystack.io -o yaml" (the block that also
contains the "kubectl -n get foundationdbrestores.apps.foundationdb.org"
command) so the markdown linter and readers recognize it as bash.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `417bafe1-fc1c-469a-8cac-8132d5e3441a` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between faf44101e7862d5d7975be4b8a1ce5c748b81fc2 and 90da70dfb1b3d4e5cedaebd185efbcd69e4b475a. </details> <details> <summary>📒 Files selected for processing (33)</summary> * `api/apps/v1alpha1/foundationdb/types.go` * `api/backups/strategy/v1alpha1/foundationdb_types.go` * `api/backups/strategy/v1alpha1/zz_generated.deepcopy.go` * `cmd/backupstrategy-controller/main.go` * `examples/backups/foundationdb/00-helpers.sh` * `examples/backups/foundationdb/01-create-strategy.sh` * `examples/backups/foundationdb/02-create-backupclass.sh` * `examples/backups/foundationdb/03-create-bucket.sh` * `examples/backups/foundationdb/04-create-foundationdb-src.sh` * `examples/backups/foundationdb/05-create-backupjob.sh` * `examples/backups/foundationdb/06-restore-in-place.sh` * `examples/backups/foundationdb/07-restore-to-copy.sh` * `examples/backups/foundationdb/90-scenario-admin-prepare.md` * `examples/backups/foundationdb/91-scenario-user-backup.md` * `examples/backups/foundationdb/92-scenario-user-restore.md` * `examples/backups/foundationdb/README.md` * `examples/backups/foundationdb/cleanup.sh` * `examples/backups/foundationdb/run-all.sh` * `internal/backupcontroller/backupjob_controller.go` * `internal/backupcontroller/backupjob_controller_test.go` * `internal/backupcontroller/foundationdbapp/types.go` * `internal/backupcontroller/foundationdbapp/zz_generated.deepcopy.go` * `internal/backupcontroller/foundationdbstrategy_controller.go` * `internal/backupcontroller/foundationdbstrategy_controller_test.go` * `internal/backupcontroller/foundationdbtypes/types.go` * `internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go` * `internal/backupcontroller/restorejob_controller.go` * `internal/backupcontroller/restorejob_controller_test.go` * `packages/apps/foundationdb/README.md` * `packages/apps/foundationdb/values.schema.json` * `packages/apps/foundationdb/values.yaml` * `packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_foundationdbs.yaml` * `packages/system/backupstrategy-controller/templates/rbac.yaml` </details> <details> <summary>✅ Files skipped from review due to trivial changes (5)</summary> * packages/apps/foundationdb/README.md * packages/apps/foundationdb/values.schema.json * examples/backups/foundationdb/README.md * api/apps/v1alpha1/foundationdb/types.go * api/backups/strategy/v1alpha1/zz_generated.deepcopy.go </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (13)</summary> * examples/backups/foundationdb/run-all.sh * internal/backupcontroller/restorejob_controller.go * internal/backupcontroller/foundationdbapp/types.go * internal/backupcontroller/backupjob_controller_test.go * cmd/backupstrategy-controller/main.go * internal/backupcontroller/foundationdbtypes/types.go * api/backups/strategy/v1alpha1/foundationdb_types.go * examples/backups/foundationdb/00-helpers.sh * internal/backupcontroller/foundationdbapp/zz_generated.deepcopy.go * internal/backupcontroller/restorejob_controller_test.go * internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go * internal/backupcontroller/foundationdbstrategy_controller_test.go * internal/backupcontroller/foundationdbstrategy_controller.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
♻️ Duplicate comments (2)
examples/backups/foundationdb/91-scenario-user-backup.md (1)
50-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language specifier to fenced code block.
The code block should specify
bashorshellas the language for proper syntax highlighting and better accessibility.📝 Proposed fix
-``` +```bash kubectl -n <ns> get backupjobs.backups.cozystack.io <name> -o yaml kubectl -n <ns> get backups.backups.cozystack.io <name> -o yaml kubectl -n <ns> get foundationdbbackups.apps.foundationdb.org \ -l backups.cozystack.io/owned-by.BackupJobName=<name></details> <details> <summary>🤖 Prompt for AI Agents</summary>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/foundationdb/91-scenario-user-backup.mdaround lines 50 -
55, The fenced code block containing the three kubectl commands (the lines
beginning with "kubectl -n get backupjobs.backups.cozystack.io", "kubectl
-n get backups.backups.cozystack.io", and "kubectl -n get
foundationdbbackups.apps.foundationdb.org") needs a language specifier for
syntax highlighting; update the opening fence fromtobash (or ```shell)
so the block is marked as bash/shell.</details> </blockquote></details> <details> <summary>examples/backups/foundationdb/90-scenario-admin-prepare.md (1)</summary><blockquote> `31-34`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add language specifier to fenced code block.** The code block should specify `bash` or `shell` as the language for proper syntax highlighting and better accessibility. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```bash kubectl get foundationdbs.strategy.backups.cozystack.io kubectl get backupclasses.backups.cozystack.io foundationdb-default ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/foundationdb/90-scenario-admin-prepare.mdaround lines 31 -
34, The fenced code block containing the kubectl commands in
examples/backups/foundationdb/90-scenario-admin-prepare.md should include a
language specifier (e.g., bash or shell) for proper syntax highlighting; update
the triple-backtick fence that wraps "kubectl get
foundationdbs.strategy.backups.cozystack.io" and "kubectl get
backupclasses.backups.cozystack.io foundationdb-default" to usebash (orshell) at the start and close with ``` so the block is annotated for
highlighting and accessibility.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go (1)</summary><blockquote> `175-187`: _💤 Low value_ **Consider adding standalone DeepCopy methods for consistency.** `BackupDeploymentSpec` and `BlobStoreConfiguration` only implement `DeepCopyInto`, unlike other types in this file that provide both `DeepCopyInto` and `DeepCopy`. While not strictly required if these are only used as embedded fields, adding standalone `DeepCopy` methods would improve consistency and future flexibility. <details> <summary>♻️ Example implementation for consistency</summary> ```diff func (in *BackupDeploymentSpec) DeepCopyInto(out *BackupDeploymentSpec) { *out = *in if in.PodTemplateSpec != nil { out.PodTemplateSpec = in.PodTemplateSpec.DeepCopy() } } + +func (in *BackupDeploymentSpec) DeepCopy() *BackupDeploymentSpec { + if in == nil { + return nil + } + out := new(BackupDeploymentSpec) + in.DeepCopyInto(out) + return out +} func (in *BlobStoreConfiguration) DeepCopyInto(out *BlobStoreConfiguration) { *out = *in if in.URLParameters != nil { out.URLParameters = append([]string(nil), in.URLParameters...) } } + +func (in *BlobStoreConfiguration) DeepCopy() *BlobStoreConfiguration { + if in == nil { + return nil + } + out := new(BlobStoreConfiguration) + in.DeepCopyInto(out) + return out +} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/foundationdbtypes/zz_generated.deepcopy.go` around lines 175 - 187, Add missing standalone DeepCopy methods for BackupDeploymentSpec and BlobStoreConfiguration to match other types: implement BackupDeploymentSpec.DeepCopy() *BackupDeploymentSpec and BlobStoreConfiguration.DeepCopy() *BlobStoreConfiguration that return nil if the receiver is nil, otherwise allocate a new value, call the existing DeepCopyInto(out) to populate it, and return the new pointer; reference the existing DeepCopyInto implementations for PodTemplateSpec and URLParameters copying behavior when writing these methods. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@examples/backups/foundationdb/90-scenario-admin-prepare.md:
- Around line 31-34: The fenced code block containing the kubectl commands in
examples/backups/foundationdb/90-scenario-admin-prepare.md should include a
language specifier (e.g., bash or shell) for proper syntax highlighting; update
the triple-backtick fence that wraps "kubectl get
foundationdbs.strategy.backups.cozystack.io" and "kubectl get
backupclasses.backups.cozystack.io foundationdb-default" to usebash (orshell) at the start and close with ``` so the block is annotated for
highlighting and accessibility.In
@examples/backups/foundationdb/91-scenario-user-backup.md:
- Around line 50-55: The fenced code block containing the three kubectl commands
(the lines beginning with "kubectl -n get backupjobs.backups.cozystack.io",
"kubectl -n get backups.backups.cozystack.io", and "kubectl -n get
foundationdbbackups.apps.foundationdb.org") needs a language specifier for
syntax highlighting; update the opening fence fromtobash (or ```shell)
so the block is marked as bash/shell.
Nitpick comments:
In@internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go:
- Around line 175-187: Add missing standalone DeepCopy methods for
BackupDeploymentSpec and BlobStoreConfiguration to match other types: implement
BackupDeploymentSpec.DeepCopy() *BackupDeploymentSpec and
BlobStoreConfiguration.DeepCopy() *BlobStoreConfiguration that return nil if the
receiver is nil, otherwise allocate a new value, call the existing
DeepCopyInto(out) to populate it, and return the new pointer; reference the
existing DeepCopyInto implementations for PodTemplateSpec and URLParameters
copying behavior when writing these methods.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `ffc131ab-ffa9-4704-8c93-ba615ca90741` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 90da70dfb1b3d4e5cedaebd185efbcd69e4b475a and 6d80ec07be6da3c4728f9141dc3e1faa4a90b0da. </details> <details> <summary>📒 Files selected for processing (34)</summary> * `api/apps/v1alpha1/foundationdb/types.go` * `api/backups/strategy/v1alpha1/foundationdb_types.go` * `api/backups/strategy/v1alpha1/zz_generated.deepcopy.go` * `cmd/backupstrategy-controller/main.go` * `examples/backups/foundationdb/00-helpers.sh` * `examples/backups/foundationdb/01-create-strategy.sh` * `examples/backups/foundationdb/02-create-backupclass.sh` * `examples/backups/foundationdb/03-create-bucket.sh` * `examples/backups/foundationdb/04-create-foundationdb-src.sh` * `examples/backups/foundationdb/05-create-backupjob.sh` * `examples/backups/foundationdb/06-restore-in-place.sh` * `examples/backups/foundationdb/07-restore-to-copy.sh` * `examples/backups/foundationdb/90-scenario-admin-prepare.md` * `examples/backups/foundationdb/91-scenario-user-backup.md` * `examples/backups/foundationdb/92-scenario-user-restore.md` * `examples/backups/foundationdb/README.md` * `examples/backups/foundationdb/cleanup.sh` * `examples/backups/foundationdb/run-all.sh` * `internal/backupcontroller/backupjob_controller.go` * `internal/backupcontroller/backupjob_controller_test.go` * `internal/backupcontroller/foundationdbapp/types.go` * `internal/backupcontroller/foundationdbapp/zz_generated.deepcopy.go` * `internal/backupcontroller/foundationdbstrategy_controller.go` * `internal/backupcontroller/foundationdbstrategy_controller_test.go` * `internal/backupcontroller/foundationdbtypes/types.go` * `internal/backupcontroller/foundationdbtypes/zz_generated.deepcopy.go` * `internal/backupcontroller/restorejob_controller.go` * `internal/backupcontroller/restorejob_controller_test.go` * `packages/apps/foundationdb/README.md` * `packages/apps/foundationdb/values.schema.json` * `packages/apps/foundationdb/values.yaml` * `packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_foundationdbs.yaml` * `packages/system/backupstrategy-controller/templates/rbac.yaml` * `packages/system/foundationdb-rd/cozyrds/foundationdb.yaml` </details> <details> <summary>✅ Files skipped from review due to trivial changes (6)</summary> * packages/apps/foundationdb/values.schema.json * packages/apps/foundationdb/README.md * examples/backups/foundationdb/README.md * packages/apps/foundationdb/values.yaml * api/backups/strategy/v1alpha1/zz_generated.deepcopy.go * packages/system/foundationdb-rd/cozyrds/foundationdb.yaml </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (12)</summary> * api/apps/v1alpha1/foundationdb/types.go * cmd/backupstrategy-controller/main.go * examples/backups/foundationdb/00-helpers.sh * examples/backups/foundationdb/run-all.sh * internal/backupcontroller/restorejob_controller.go * internal/backupcontroller/foundationdbapp/zz_generated.deepcopy.go * internal/backupcontroller/backupjob_controller.go * internal/backupcontroller/backupjob_controller_test.go * internal/backupcontroller/foundationdbtypes/types.go * internal/backupcontroller/foundationdbapp/types.go * internal/backupcontroller/foundationdbstrategy_controller.go * internal/backupcontroller/foundationdbstrategy_controller_test.go </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
6d80ec0 to
6f51366
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-foundationdb-backup-1778597123.md
NOT LGTM
Target branch: main (from PR #2606 base). Merge base: 5f83ae72a8ec (head of main after the MariaDB backup feature). Single commit on this branch (6f513663c feat(platform): add implementation of backup-strategy controller for foundationdb). All FoundationDB tests pass locally (go test ./internal/backupcontroller/ -count=1 clean), and go build ./... is clean. The work is large (~12.9k lines) but well-structured and mirrors the established CNPG / MariaDB driver patterns closely.
Worth calling out before the blockers: this is genuinely good work. The test surface is dense, the gates against operator-specific quirks (one-running-backup-per-cluster, in-chart-conflict diagnostic, blob-credentials precondition, deadline-expiry cleanup) are real and the test names map cleanly to review item numbers, suggesting prior review cycles already chewed through the obvious foot-guns. The blockers below are not architectural — they are contract-violation seams and one factual error in shipped documentation.
Blockers
1. decodeFoundationDBBackupSnapshot ignores the apiVersion it explicitly promises to check
internal/backupcontroller/foundationdbstrategy_controller.go:1063-1076 (decode), 1031-1043 (marshal), and the foundationdbBackupSnapshotKind constant block at internal/backupcontroller/foundationdbstrategy_controller.go:381-389 and 391-394 (apiVersion var).
The comment block on the snapshot constants says, verbatim:
Bump the apiVersion (or introduce a Version field on foundationdbBackupSnapshot) the moment the on-disk shape changes in a way that breaks older readers — decoders deployed against a mismatched apiVersion must be able to detect the mismatch and refuse to use the snapshot rather than silently mis-interpret it.
The decoder checks only snap.Kind != foundationdbBackupSnapshotKind and returns (nil, nil) for any other case. It never reads snap.APIVersion. The contract documented next to the constant is therefore unenforceable: a future v2 snapshot with the same Kind (the most likely shape of a "bump the apiVersion" change) will be silently parsed as v1, the v2 fields the v1 struct doesn't know about will be dropped, and a restore will run against a stale interpretation of the persisted strategy. The decode call site at internal/backupcontroller/foundationdbstrategy_controller.go:822 (snap, _ := decodeFoundationDBBackupSnapshot(...)) discards the error too, so even if the decoder grew an error return for version mismatch, the consumer would ignore it.
This is the classic comment-as-fix anti-pattern: the future risk is documented but the code does not honor the documentation. Either delete the warning (and the dangling foundationdbBackupSnapshotAPIVersion variable that nothing reads) or enforce it.
Fix: in decodeFoundationDBBackupSnapshot, after the Kind check, also reject snapshots whose APIVersion does not match foundationdbBackupSnapshotAPIVersion. Return a typed error (e.g. errSnapshotAPIVersionMismatch) so the call site at line 822 can distinguish "no snapshot present" (current behavior, fall through to driverMetadata) from "snapshot present but unreadable" (RestoreJob must fail terminally — falling back to driverMetadata on a known-mismatched snapshot would corrupt the restore). Update the line-822 call site to surface that terminal error via markRestoreJobFailed.
Test: add TestDecodeFoundationDBBackupSnapshot_RejectsMismatchedAPIVersion that marshals a snapshot with APIVersion: "future.cozystack.io/v2", decodes it, and asserts the decoder returns a non-nil error and a nil snapshot. Add TestReconcileFoundationDBRestore_SnapshotAPIVersionMismatchIsTerminal that seeds a Backup whose status.underlyingResources carries the future apiVersion, drives the restore reconcile, and asserts RestoreJob.status.phase == Failed with a Ready=False condition message that mentions the version mismatch (and that no FoundationDBRestore CR was created). Both fail without the fix.
2. The 92-scenario-user-restore.md verification command is factually wrong and will mislead users
examples/backups/foundationdb/92-scenario-user-restore.md:78-86:
kubectl -n <ns> get foundationdbrestores.apps.foundationdb.org \
-l backups.cozystack.io/owned-by.BackupJobName=<name>
Note: the label key is named `owned-by.BackupJobName` for historical
reasons (it is shared with the BackupJob ownership label), but for
`FoundationDBRestore` CRs the value is the **RestoreJob** name, not a
BackupJob name. Use `<RESTOREJOB_INPLACE_NAME>` or
`<RESTOREJOB_TOCOPY_NAME>` when filtering for restore artefacts.
The label is correctly identified (api/backups/v1alpha1/backupjob_types.go:25 defines OwningJobNameLabel = thisGroup + "/owned-by.BackupJobName"), and the doc is right that the value on a FoundationDBRestore is the RestoreJob name. But the cleanup.sh you shipped alongside this doc uses the SAME label selector with the SAME key — and the script's loop iterates over RestoreJob names AND the BackupJob name as if they all live under the same label:
examples/backups/foundationdb/cleanup.sh:24-28:
for owner in "${BACKUPJOB_NAME}" "${RESTOREJOB_INPLACE_NAME}" "${RESTOREJOB_TOCOPY_NAME}"; do
kubectl -n "$NAMESPACE" delete --ignore-not-found --wait=false \
foundationdbbackups.apps.foundationdb.org,foundationdbrestores.apps.foundationdb.org \
-l "backups.cozystack.io/owned-by.BackupJobName=${owner}" || true
doneThat part is actually correct (the loop pretends BackupJob and RestoreJob names are interchangeable label values, which they are because both ensureFoundationDBBackup and ensureFoundationDBRestore set OwningJobNameLabel to their own owner's name). So cleanup.sh works.
But the doc carries a second factual error that cleanup.sh doesn't catch: the doc instructs users to filter FoundationDBRestore by BackupJob-named labels for verification ("BackupJobName=" with hint to substitute the RestoreJob name). For a user copy-pasting the example, that works mechanically. The real problem is the SECOND code block at 92-scenario-user-restore.md:78-81 says to get foundationdbrestores but the example in 91-scenario-user-backup.md:62-64 uses the IDENTICAL label-key selector to get foundationdbbackups — which means if a user has both BackupJob and RestoreJob CRs whose names collide (e.g. both set to "fdb-backup-1"), the verification command from either doc will list BOTH the operator-side Backup and the operator-side Restore. The docs do not warn about this, and the label key is genuinely shared.
This is fixable in the docs without code changes, and it's important because the example uses different names for the source BackupJob and the RestoreJobs so the demo coincidentally avoids the collision — but a real tenant who reuses names will hit a surprise.
Fix: in 92-scenario-user-restore.md, change the verification code block to:
kubectl -n <ns> get foundationdbrestores.apps.foundationdb.org \
-l backups.cozystack.io/owned-by.BackupJobName=<restorejob-name> \
-l backups.cozystack.io/owned-by.BackupJobNamespace=<ns>
and add a warning: "The label key is owned-by.BackupJobName regardless of whether the value names a BackupJob or a RestoreJob. If your BackupJob and RestoreJob share the same name, also filter by resource kind. Consider renaming the label in a future API revision."
In 91-scenario-user-backup.md, mirror the same warning.
Test: this is a documentation-accuracy blocker. The right way to guard it is NOT a test that parses the markdown files (per CLAUDE.md guidance against grep-the-doc tests); the right way is to make the label NAME unambiguous. Track that as a follow-up API task (backups.cozystack.io/owned-by.JobName + a kind-discriminating label). For THIS PR, the doc fix above is the deliverable. The existing TestEnsureFoundationDBRestore_IdempotentByLabel and TestEnsureFoundationDBBackup_IdempotentByLabel together already pin that both kinds use the same key — that's the underlying contract, and the doc must accurately describe it.
3. 02-create-backupclass.sh writes accountName: "REPLACE_ME" to the cluster and step 03 patches it; if step 03 fails or is skipped, a tenant who runs 05-create-backupjob.sh burns 45 wall-clock minutes before learning the strategy is half-configured
examples/backups/foundationdb/02-create-backupclass.sh:30-39 (writes REPLACE_ME placeholders), and examples/backups/foundationdb/05-create-backupjob.sh:31 (wait_for_field ... 2700 — 45-minute timeout matching the driver's foundationdbDefaultBackupDeadline).
The script's own comment acknowledges this is intentional:
Empty values cause the strategy to render an invalid FoundationDBBackup (accountName is required), which surfaces as a BackupJob Failed condition - intentional, so a half-configured tenant fails fast.
But accountName: "REPLACE_ME" is NOT empty. It's a non-empty 10-character string that passes the +kubebuilder:validation:MinLength=1 validation on api/backups/strategy/v1alpha1/foundationdb_types.go:177. The driver renders the strategy successfully (no template error, the value is a literal), creates the FoundationDBBackup CR successfully (operator-side OpenAPI accepts any non-empty accountName), and the operator's backup_agent Deployment crash-loops trying to dial REPLACE_ME@<unknown-host>. The foundationdbDefaultBackupDeadline is 45 minutes (internal/backupcontroller/foundationdbstrategy_controller.go:415); the BackupJob spins for the full deadline before terminating.
The comment is wrong about the failure mode (it claims operator-side validation rejects accountName when it's empty; the kubebuilder validation actually rejects empty accountName, not literal REPLACE_ME), so the "fail fast" claim only holds if a tenant actually managed to apply step 02 in a way that produced an empty value — and the YAML they paste contains the literal string REPLACE_ME, so they can't.
Fix (pick one):
- (preferred) Remove step 02 from the script set entirely and fold its body into step 03's "patch BackupClass parameters with resolved blob-store config" apply at
03-create-bucket.sh:80. Step 03 already does a full apply with the resolved values; there is no functional reason for step 02 to apply a placeholder version of the same resource first. - (alternative) In step 02, leave the BackupClass with NO
strategiesblock at all (justspec: {}orspec: { strategies: [] }), and have step 03 add the strategy entry. An emptystrategieslist is invalid per the framework (a BackupJob will Fail withno strategy matches), but it Fails fast on the first reconcile rather than on the 45-minute deadline. - (last-resort) Tighten the strategy CRD's validation: forbid the literal sentinel string. Bad option — sentinels are demo-script noise, not API constraints.
Test: add an integration-style unit test TestReconcileFoundationDB_RejectsPlaceholderAccountName only if you pick the "tighten validation" path (validate that accountName="REPLACE_ME" results in a terminal BackupJob Failed within the first reconcile). For the "fold steps 02 and 03" path, no Go test — the change is in the shell-script demo; rerun run-all.sh end-to-end on a real cluster and confirm step 05 still works AND skipping step 03 produces a kubectl apply error rather than a 45-minute spin. The e2e run IS the test here, per the project's stance against parsing-shell-scripts-from-Go.
4. j.Status.Phase = Running Status Update at foundationdbstrategy_controller.go:246-251 runs AFTER ensureFoundationDBBackup has already created a server-side CR; on Status().Update conflict the BackupJob is wedged with an orphan FoundationDBBackup whose label matches a BackupJob whose status was never advanced past Pending
internal/backupcontroller/foundationdbstrategy_controller.go:246-251:
if j.Status.Phase != backupsv1alpha1.BackupJobPhaseRunning {
j.Status.Phase = backupsv1alpha1.BackupJobPhaseRunning
if err := r.Status().Update(ctx, j); err != nil {
return ctrl.Result{}, err
}
}The Update returns the bare error, which controller-runtime reschedules. On the next reconcile, findFoundationDBBackupForJob matches the orphan CR (correct), j.Status.Phase is still empty/Pending (because the previous Status update lost), the if-block re-enters, the Status Update may succeed this time — fine. But the ordering is fragile: between the Create on line 137 (inside ensureFoundationDBBackup) and the Status Update on 248, any uncached condition on the parent BackupJob (e.g. concurrent deletion that flips it to terminating) leaves an orphan CR labeled to a BackupJob the framework is reaping. The orphan persists, and a future BackupJob with the same name (uncommon but possible after a tenant kubectl delete && kubectl apply retry) will pick up the orphan via the label-match path in findFoundationDBBackupForJob.
This isn't a "high-probability bug today" — it's a latent race that the MariaDB and CNPG drivers presumably have the same way, and one that ownerReferences would fix once the framework starts setting them. But the FoundationDBBackup intentionally does NOT carry an OwnerReference to the BackupJob (cleanup.sh explains this is by design for idempotency). So this driver is no worse than its peers — but the comment at cleanup.sh:22-26 is the only place in the tree that documents this contract, and that documentation lives in a demo script. Without it, an operator-side reader has to reverse-engineer why the operator CR survives BackupJob deletion.
Fix: add a comment to foundationdbstrategy_controller.go:735 (ensureFoundationDBBackup) explaining that the operator-side CR is intentionally NOT owner-ref'd to the BackupJob, why (idempotent ensure-by-label across BackupJob recreates with the same name), and what the cleanup contract is (the parent framework's GC, not Kubernetes ownerRefs, is responsible). Reference cleanup.sh from there so the contract is anchored in code.
Test: add TestEnsureFoundationDBBackup_DoesNotSetOwnerReference that creates a BackupJob, runs ensureFoundationDBBackup, lists the resulting FoundationDBBackup, and asserts len(b.OwnerReferences) == 0. The test pins the contract for the next person who adds an OwnerReferences: field "for safety" (which would inadvertently break the idempotent ensure-by-label semantics on BackupJob recreate).
5. requeueWithReason Status().Update has the same stale-cache class of bug that StartedAt defends against, but is unconditional
internal/backupcontroller/foundationdbstrategy_controller.go:715-726:
func (r *BackupJobReconciler) requeueWithReason(ctx context.Context, j *backupsv1alpha1.BackupJob, reason, message string) (ctrl.Result, error) {
apimeta.SetStatusCondition(&j.Status.Conditions, metav1.Condition{
Type: "Ready",
Status: metav1.ConditionFalse,
Reason: reason,
Message: message,
})
if err := r.Status().Update(ctx, j); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: foundationdbPollInterval}, nil
}If j carries a stale RV (which is exactly the scenario StartedAt bootstrap at foundationdbstrategy_controller.go:496-502 defends against by refetching), this Update returns a 409 Conflict. The error propagates to controller-runtime, which reschedules. On reschedule, the informer cache may STILL be stale; the loop can pin a busy-spin for the duration of the cache resync (typically seconds, sometimes longer under load).
The MariaDB and CNPG drivers have the same pattern, so this isn't FoundationDB-specific — but the FDB driver has more requeueWithReason call sites (six: FoundationDBClusterNotReady, FoundationDBClusterNotAvailable, FoundationDBClusterVersionUnknown, MissingBlobCredentialsSecret, FoundationDBBackupRunning, plus the deadline-exceeded path which markBackupJobFailed-s instead). Five of those six fire on the cold-start path where the informer cache is most likely to be stale.
Fix: in requeueWithReason, on 409 Conflict, refetch the BackupJob, re-apply the condition delta on the fresh object, and retry the Update once. Same pattern as patchStopWithRetry at internal/backupcontroller/foundationdbstrategy_controller.go:783-815. Cap the retry budget at 1 or 2 — a persistent Conflict means the parent reconciler will pick up the next change anyway.
Test: TestRequeueWithReason_RetriesOnStatusConflict — seed a BackupJob, use the controller-runtime fake-client interceptor (same pattern as TestStopOtherFoundationDBBackups_RetriesOnConflict at line 2861 of the existing test file) to return one 409 on the first Status().Update of a BackupJob, let the second through, and assert the condition lands without the reconcile returning an error.
Recommended (non-blocking, would still like to see addressed)
internal/backupcontroller/foundationdbstrategy_controller.go:415says "CNPG/MariaDB use 30m for the same reason; FDB needs a touch more headroom because the first snapshot for an empty cluster still has to do range-file rotation through the backup_agent." This is plausible but unmeasured. Worth turning the 45-minute hardcode into a strategy-level knob (e.g.FoundationDBSpec.BackupTimeoutSeconds) so a tenant with a multi-TB cluster can extend it without recompiling. The restore path already hasrestoreTimeoutSecondsviaSpec.Options; mirror it on the backup path.foundationdbBackupReady(internal/backupcontroller/foundationdbstrategy_controller.go:707-721) gates onb.Status.Generations.Reconciled < b.Generation, butBackupGenerationStatuscarries five otherNeedsBackup*counters (internal/backupcontroller/foundationdbtypes/types.go:204-210). If the operator surfacesNeedsBackupStartlong-running whileReconciled == Generation, the readiness check passes but the agent isn't actually streaming. Worth adding a guard that allNeedsBackup*counters are zero. (The operator's own semantics may make this redundant; verify with the upstream operator behavior before adding the check.)internal/backupcontroller/foundationdbstrategy_controller.go:1136(foundationdbBackupURI): the comment at 1136-1145 explains why bucket-empty falls back to "". The fallbackblobstore://%s/%sURI omitssecure_connectionand other URLParameters that the operator-reportedBackupDetails.URLwould carry; the URI is "informational only" per the comment, but the inconsistency between the two return paths is worth a doc note (no behavior change needed).internal/backupcontroller/foundationdbstrategy_controller.go:830: snapshot Kind constant is FDB-specific but the apiVersion borrowsbackupsv1alpha1.GroupVersion. That's documented as deliberate, but worth a brief note that this meansapiVersion: backups.cozystack.io/v1alpha1+kind: FoundationDBBackupSnapshotis not a registered API — it's a self-typing hint. A future reader who tries tokubectl get foundationdbbackupsnapshotswill get a NotFound; the comment block should call out that this is a status-payload-only kind, not a CRD.
Closing summary of what's in scope
This is the FoundationDB twin of the recently-merged MariaDB BackupClass integration. The five blockers above are: one contract violation (snapshot apiVersion), one shipped-documentation factual error (92-scenario-user-restore.md), one demo-script tripping over its own validation (02-create-backupclass.sh placeholder), and two latent reliability issues shared with the older drivers but newly relevant because the FDB driver has more transient-error call sites (orphan CR contract comment, Status().Update conflict retry).
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.
…foundationdb Signed-off-by: Andrey Kolkov <[email protected]>
6f51366 to
92930ad
Compare
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
LGTM
Reviewing against target branch: main (PR #2606, "feat(platform): add foundationdb backups").
Scope
Branch contains a single commit on top of main (92930ad1a). It adds the FoundationDB backup-strategy driver: a new strategy.backups.cozystack.io/v1alpha1.FoundationDB type, controller wiring for the BackupJob and RestoreJob paths, a typed mirror of the apps.foundationdb.org/v1beta2 shape the driver writes, RBAC, an end-to-end example, and DEPRECATED markers on the chart-side backup.* values. Net diff is ~13.3k lines but ~8.7k of that is the generated CRD YAML and ~1.8k is the new test file. The hand-written code surface is roughly internal/backupcontroller/foundationdbstrategy_controller.go (1265 lines) + foundationdbtypes/types.go (281) + foundationdbapp/types.go (66) + api/backups/strategy/v1alpha1/foundationdb_types.go (157).
What I checked
- Build (
go build ./...) and vet (go vet ./...) are clean — the two pre-existing vet warnings incmd/cozypkgandpkg/lineageare unrelated to this branch. go test ./internal/backupcontroller/...passes (47 FDB-specific tests + the dispatcher tests).- Controller dispatcher updates in
backupjob_controller.go,restorejob_controller.go, andcmd/backupstrategy-controller/main.goadd the new Kind to the supported list, the dispatch switch, the cleanup no-op group, and the scheme. All four are consistent.supportedBackupStrategyKinds()and its sibling test stay in sync. - RBAC delta in
packages/system/backupstrategy-controller/templates/rbac.yamlcoversfoundationdbclusters(get/list/watch),foundationdbbackups(get/list/watch/create/patch), andfoundationdbrestores(get/list/watch/create). The patch verb is needed for thebackupState=Stoppedtransition on prior CRs; nodeleteis granted, which matches the same "we do not own the archive" stance Altinity/MariaDB take. - Chart-level deprecation:
api/apps/v1alpha1/foundationdb/types.go,packages/apps/foundationdb/values.yaml,values.schema.json,README.md, andpackages/system/foundationdb-rd/cozyrds/foundationdb.yaml's embeddedopenAPISchemaall carry theDEPRECATEDprefix consistently. Chart templates underpackages/apps/foundationdb/templates/are untouched, sobackup.enabled=truekeeps rendering the originalFoundationDBBackupCR unchanged (backward compat). ThefindConflictingInChartBackupgate catches the case where a tenant tries to run both flows against the same cluster, with a terminalReady=False/ConflictingInChartBackup-style message — chart-rendered CR is left untouched so the user sees the diagnostic before doing the migration. - Snapshot schema versioning (
foundationdbBackupSnapshotKind/foundationdbBackupSnapshotAPIVersion) is enforced both ways: encoder stamps both fields, decoder returnserrSnapshotUnrecognisedon mismatch (covered byTestDecodeFoundationDBBackupSnapshot_RejectsMismatchedAPIVersion,…_RejectsMismatchedKind, and the call-site testTestReconcileFoundationDBRestore_SnapshotAPIVersionMismatchIsTerminal). The apiVersion is tied tobackupsv1alpha1.GroupVersion.String()— that's a deliberate design choice documented in the contract comment, with the trade-off (a future v1beta1 group bump would invalidate existing snapshots) called out as the maintainer's responsibility. - Idempotency contracts:
ensureFoundationDBBackupfinds byOwningJobName/Namespacelabels and reuses, withTestEnsureFoundationDBBackup_IdempotentByLabelpinning it;TestEnsureFoundationDBBackup_DoesNotSetOwnerReferencepins the deliberate absence of an OwnerReference (otherwise GC on BackupJob deletion would break the "delete && apply retry" reuse path). The restore side mirrors this inensureFoundationDBRestore. - Conflict handling:
stopOtherFoundationDBBackupsretries on 409 with a refetch+re-patch loop bounded byfoundationdbStopConflictRetries=5, covered byTestStopOtherFoundationDBBackups_RetriesOnConflictusing aninterceptor.Funcsinjection.requeueWithReasondoes the same forStatus().Update409s, covered byTestRequeueWithReason_RetriesOnStatusConflict. - Deadline-expiry path:
TestReconcileFoundationDB_DeadlineExpiry_StopsFoundationDBBackuppins that a 45-minute hang Stops the operator-side CR before marking the BackupJob Failed, so thebackup_agentDeployment doesn't leak past the BackupJob. - Preconditions: empty
cluster.spec.version,health.available=false, missing blob-credentials Secret, and conflicting in-chart backup all have positive+negative tests and emit distinctReady=False/<Reason>conditions. - Restore options:
parseFoundationDBRestoreOptionstreats a malformedspec.optionsblob as terminal (per the in-tree convention) —TestReconcileFoundationDBRestore_MalformedOptionsIsTerminalpins this and asserts noFoundationDBRestoreCR was created. - URI synthesis:
foundationdbBackupURIprefers the operator-reported URL, falls back to bucket+backupName, returns "" when bucket is empty (refuses to graftAccountNameinto the bucket slot — covered byTestFoundationDBBackupURI_EmptyBucketIsEmpty). - Example flow (
examples/backups/foundationdb/) is self-contained and consistent with the controller: label keys incleanup.sh(backups.cozystack.io/owned-by.BackupJobName=…) matchbackupsv1alpha1.OwningJobNameLabel. The cluster-name mapping (foundationdbClusterNameForApp="foundationdb-" + appName) matches both00-helpers.sh'sFDB_CLUSTER_NAMEand the FoundationDB ApplicationDefinition'srelease.prefix. The.bucket-info.envcache (chmod 600, raw S3 creds) mirrors the established clickhouse demo pattern; not gitignored anywhere, butcleanup.shremoves it and the clickhouse demo lives under the same.gitignorerules — consistent with the existing convention.
Follow-up backlog (non-blocking — NOT a condition of LGTM)
Surfacing this explicitly so it doesn't get lost in the merge. Pick up or discard as you see fit; this does not gate the PR.
FU-1 — Add an explicit FoundationDBStrategyKind no-op branch in BackupReconciler.cleanupOnDelete
internal/backupcontroller/backup_controller.go:96-135 currently has explicit no-op branches for AltinityStrategyKind and MariaDBStrategyKind, each carrying a comment along the lines of "we do not own the archive." FoundationDB has identical semantics and additionally a more subtle twist (the operator-side FoundationDBBackup Deployment continues writing until either a new BackupJob calls stopOtherFoundationDBBackups against it, an operator picks it up manually, or examples/backups/foundationdb/cleanup.sh is run). But there is no matching case strategyv1alpha1.FoundationDBStrategyKind: branch on this PR — the kind falls through to default, which calls cleanupVeleroBackup.
-
Why it's not blocking now:
cleanupVeleroBackupis a guarded no-op for any Backup that does not haveveleroBackupNameset inDriverMetadata. The FoundationDB driver never sets that key, so the fall-through is harmless today. -
Why it's worth doing: this is exactly the seam that bit the MariaDB driver before the explicit branch was added — a future driver refactor that incidentally stamps
velero.io/backup-nameonto FDB driver metadata (e.g. during a migration, or because someone reuses a shared metadata helper) silently starts synthesisingDeleteBackupRequestagainst Velero. The Altinity + MariaDB tests (TestBackupCleanup_Altinity_DoesNotFallThroughToVelero,TestBackupCleanup_MariaDB_DoesNotFallThroughToVelero) were written specifically to guard that boundary. FoundationDB should join the pattern for the same reason. -
Suggested shape:
case strategyv1alpha1.FoundationDBStrategyKind: // Cozystack Backup deletion does NOT delete the operator-side // foundationdb.org/FoundationDBBackup CR. That CR drives a continuously // streaming backup_agent Deployment; its lifecycle is governed by the // next BackupJob's stopOtherFoundationDBBackups call, by manual // operator action, or by examples/backups/foundationdb/cleanup.sh. // Same "we do not own the archive" contract as Altinity / MariaDB. return nil
-
Suggested test when picked up:
TestBackupCleanup_FoundationDB_DoesNotFallThroughToVeleromodelled exactly on the Altinity/MariaDB pairs — stampveleroBackupNameMetadataKey+veleroBackupNamespaceMetadataKeyinto a FoundationDB-strategy CozystackBackup, pre-seed avelero.io/Backupnamed accordingly, drivecleanupOnDelete, assert noDeleteBackupRequestwas created and the Velero Backup survives. Fails without the case; passes with it.
Ticket compliance
No --ticket was supplied. The PR title "feat(platform): add foundationdb backups" matches the diff scope: a complete strategy driver, dispatch wiring, scheme registration, RBAC, deprecation markers, demo, and test coverage.
2 participants
What this PR does
Screenshots
Release note
Summary by CodeRabbit
New Features
Examples
Documentation
Deprecations