docs(platform): add guides related managed apps backups#536
Conversation
✅ Deploy Preview for cozystack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request adds comprehensive documentation for Cozystack's backup and recovery framework. It introduces two new guides—one for tenants and one for cluster administrators—covering the BackupClass-driven workflow for data-only backups across Postgres, MariaDB, ClickHouse, and FoundationDB. The changes also update existing application docs with deprecation warnings for legacy chart-level backup configurations. ChangesBackup and Recovery Framework Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 new data-only backup and recovery framework for managed databases (Postgres, MariaDB, ClickHouse, and FoundationDB) in Cozystack, transitioning from legacy chart-level configurations to a centralized system using BackupClass and strategy-based resources. It provides detailed documentation for both tenants and administrators. Reviewers suggested several improvements, including standardizing FoundationDB account names for better multi-tenancy, enhancing security by using non-root users in backup pods, and clarifying S3 endpoint terminology to prevent configuration errors.
| ENDPOINT_FULL=$(jq -r .spec.secretS3.endpoint /tmp/bucket.json) | ||
| ENDPOINT_HOSTPORT=${ENDPOINT_FULL#http://} | ||
| ENDPOINT_HOSTPORT=${ENDPOINT_HOSTPORT#https://} | ||
| ACCOUNT_NAME="${ACCESS_KEY}@${ENDPOINT_HOSTPORT}" |
There was a problem hiding this comment.
Using the dynamic $ACCESS_KEY as part of the ACCOUNT_NAME makes it difficult for administrators to pre-configure a cluster-scoped BackupClass, as they would need to know each tenant's access key in advance.
It is recommended to use a fixed, descriptive account name (e.g., fdb-backup) and ensure it matches the accountName parameter defined by the administrator in the BackupClass. This allows the same BackupClass to be used by multiple tenants with their own credentials.
| ACCOUNT_NAME="${ACCESS_KEY}@${ENDPOINT_HOSTPORT}" | |
| ACCOUNT_NAME="fdb-backup@${ENDPOINT_HOSTPORT}" |
| compression: gzip | ||
| ``` | ||
|
|
||
| The `endpoint` is **path-style without scheme** (e.g. `seaweedfs-s3.<seaweedfs-namespace>.svc:8333` for the default in-cluster SeaweedFS — substitute the namespace where SeaweedFS is deployed in your environment). Drop the `tls` block entirely when the endpoint serves a publicly-trusted certificate. |
There was a problem hiding this comment.
The term "path-style" in the context of S3 usually refers to the addressing mode (e.g., s3.amazonaws.com/bucket vs bucket.s3.amazonaws.com). Here, it seems to be used to describe the endpoint format (host and port without scheme). To avoid confusion, it might be clearer to explicitly state that the endpoint should contain only the host and port.
| The `endpoint` is **path-style without scheme** (e.g. `seaweedfs-s3.<seaweedfs-namespace>.svc:8333` for the default in-cluster SeaweedFS — substitute the namespace where SeaweedFS is deployed in your environment). Drop the `tls` block entirely when the endpoint serves a publicly-trusted certificate. | |
| The `endpoint` should be provided as **host:port without scheme** (e.g. `seaweedfs-s3.<seaweedfs-namespace>.svc:8333` for the default in-cluster SeaweedFS — substitute the namespace where SeaweedFS is deployed in your environment). Drop the `tls` block entirely when the endpoint serves a publicly-trusted certificate. |
| cpu: 200m | ||
| memory: 256Mi | ||
| securityContext: | ||
| runAsUser: 0 |
There was a problem hiding this comment.
Running as root (runAsUser: 0) is generally discouraged for security reasons and should not be necessary for the FoundationDB backup agent. It is recommended to use the default FoundationDB user ID 4059 to adhere to the principle of least privilege.
| runAsUser: 0 | |
| runAsUser: 4059 |
| kind: FoundationDB | ||
| name: foundationdb-data-strategy | ||
| parameters: | ||
| accountName: "<api_key>@<endpoint-host>:<port>" |
There was a problem hiding this comment.
Instead of using a placeholder that implies a dynamic per-tenant value (which is hard to manage in a cluster-scoped resource), it is better to use a fixed account name that matches the instructions provided to tenants in their guide.
| accountName: "<api_key>@<endpoint-host>:<port>" | |
| accountName: "fdb-backup@<endpoint-host>:<port>" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
content/en/docs/next/applications/backup-and-recovery.md (1)
36-43: ⚡ Quick winConsider adding a language identifier to the fenced code block.
The output example is missing a language identifier. Adding
textorconsolewould satisfy linters and improve rendering:-``` +```text NAME AGE postgres-data-backup 14m🤖 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 `@content/en/docs/next/applications/backup-and-recovery.md` around lines 36 - 43, The fenced code block in the backup-and-recovery.md example lacks a language identifier; update the triple-backtick fence around the output listing (the block showing NAME / AGE entries like "postgres-data-backup" and "velero") to include a language such as "text" or "console" (e.g., change ``` to ```text) so linters/renderers recognize it as plain output.
🤖 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 `@content/en/docs/next/applications/backup-and-recovery.md`:
- Around line 36-43: The fenced code block in the backup-and-recovery.md example
lacks a language identifier; update the triple-backtick fence around the output
listing (the block showing NAME / AGE entries like "postgres-data-backup" and
"velero") to include a language such as "text" or "console" (e.g., change ``` to
```text) so linters/renderers recognize it as plain output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f871a89b-ab72-46ba-b44a-b1b590bb3620
📒 Files selected for processing (8)
content/en/docs/next/applications/backup-and-recovery.mdcontent/en/docs/next/applications/clickhouse.mdcontent/en/docs/next/applications/foundationdb.mdcontent/en/docs/next/applications/mariadb.mdcontent/en/docs/next/applications/postgres.mdcontent/en/docs/next/operations/services/managed-app-backup-configuration.mdcontent/en/docs/next/operations/services/velero-backup-configuration.mdcontent/en/docs/next/virtualization/backup-and-recovery.md
androndo
force-pushed
the
feat/apps-backups-guides
branch
from
4efa0e7 to
0b9ead0
Compare
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
androndo
force-pushed
the
feat/apps-backups-guides
branch
from
0b9ead0 to
237d627
Compare
|
NOT LGTM Reviewing against Primary blocker — tenant guide is unrunnable as a tenant
Neither is permitted by the aggregated tenant roles. Verified against
Meanwhile the COSI flow in So the tenant guide is broken at step one for every driver except ClickHouse (which routes through chart values, not a hand-rolled Secret). A user following this guide on a real cluster sees 403 on the very first This is not a wording fix. The doc as written exposes a missing piece in the implementation: there is no tenant-visible path from a VerificationEnd-to-end test should run the tenant guide as a tenant ServiceAccount (one of `cozy:tenant:admin` / `cozy:tenant:use`), impersonated via `--as`, against a real cluster, and complete a Postgres / MariaDB / FoundationDB backup and restore without ever using a cluster-admin kubeconfig. A reproducible CI variant lives in `cozystack/cozystack/examples/backups//run-all.sh`; those scripts currently assume cluster-admin and would need a parallel `run-all-as-tenant.sh` variant under the chosen ServiceAccount. Secondary issues (correctness)These are real, but each one sits inside text that will move or vanish when the primary blocker is addressed. Worth folding into the rewrite rather than spot-patching now.
Out of scope / fine as-is
|
2 participants
Summary by CodeRabbit