Repository navigation
feat(etcd): update operator to v0.4.3 and expose S3 backup schedules - #2428
Merged
Andrei Kvapil (kvaps) merged 16 commits intoMay 1, 2026
Merged
Conversation
Upstream release notes: https://github.com/aenix-io/etcd-operator/releases/tag/v0.4.3 Notable changes: - Add EtcdBackup CRD for declarative etcd snapshots (#307) - Add EtcdBackupSchedule CRD for scheduled backups (#307) - Expose metrics port on client service (#299) - Replace deprecated gcr.io/kubebuilder/kube-rbac-proxy with quay.io/brancz/kube-rbac-proxy:v0.18.1 (#308) - Update Kamaji example manifest Note: Helm 3 does not install CRDs on upgrade. Existing clusters require manual application of the new EtcdBackup and EtcdBackupSchedule CRDs: kubectl apply --filename packages/system/etcd-operator/charts/etcd-operator/crds/ Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Expose the EtcdBackupSchedule CRD (etcd.aenix.io/v1alpha1) through optional values so operators can declaratively configure periodic S3 backups of the platform etcd cluster. Kamaji stores tenant control-plane etcd data here, so losing this cluster means losing every tenant cluster — a backup schedule is critical. The backup is disabled by default (backup.enabled: false), keeping existing installations unchanged. When enabled, a Secret is created with AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY keys referenced by the new EtcdBackupSchedule resource. Field naming mirrors the existing postgres backup pattern (destinationPath, endpointURL, s3AccessKey, s3SecretKey, schedule) for consistency. Requires etcd-operator v0.4.3+ (shipped in the previous commit). Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Adds two bats cases: a baseline Etcd managed-app deployment and a deployment with backup.enabled=true that verifies the EtcdBackupSchedule resource and the S3 credentials Secret are materialized. The backup case uses fake S3 credentials — the operator will not successfully push backups, but the schedule resource itself is reconciled, which is what this test validates. Also hardcodes the EtcdBackupSchedule and backup Secret names to 'etcd' / 'etcd-s3-creds' to match the existing hardcoded EtcdCluster name in the chart (one etcd per tenant namespace). Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Review found three issues:
1. The guard in backup-secret.yaml only triggered on empty strings,
not on the placeholder defaults ('<your-access-key>' etc). A user
enabling backup.enabled=true without overriding the values would
produce a Secret containing the literal placeholder, and the
backup CronJob would fail at runtime with an opaque 403 far from
the source. Add explicit template guards that fail rendering when
any of the four required fields still hold their placeholder
default.
2. The backup e2e test waited on .spec.schedule of the newly-created
EtcdBackupSchedule, which is set at apply time — the wait was
tautological and did not prove the etcd-operator reconciled
anything. Replace it with a wait on the EtcdCluster's Ready
condition plus a poll for the CronJob the operator generates
from the schedule (selected via the
etcd.aenix.io/etcdbackupschedule-name label).
3. The backup e2e test also did not wait for EtcdCluster readiness
before tearing down. Add the same Ready wait as the baseline
test, and verify the Secret contains the real access key value
rather than just existing.
Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
- Mark backup.enabled as optional in the values typedef so
generated JSON schema no longer lists it in 'required'.
Previously, 'backup: {}' (relying on the enabled:false default)
failed schema validation at CRD admission.
- Validate backup.destinationPath has s3:// prefix. Without this
check, passing a path without the scheme silently used the
first path segment as the bucket name.
- Add tail newline to values.schema.json to match repo convention
(cozyvalues-gen output lacks it — worth fixing in that project
separately).
- Add bats setup() that clears tenant-test etcd resources before
each test so a failure mid-test does not leak into subsequent
tests. Hardcoded chart-level resource names ('etcd',
'etcd-s3-creds') make two Etcd apps in the same namespace
collide; setup() enforces a single owner per test.
- Add dump_diagnostics() helper that dumps operator logs + the
relevant CRD state on wait-for-ready timeouts, so CI failures
surface actionable context instead of bare exit code 124.
- Add a third e2e case 'Create Etcd with empty backup block' that
asserts backup:{} admits and produces no backup resources — a
regression test for the required:['enabled'] bug fixed here.
Known scope: the upstream EtcdCluster.spec.bootstrap restore field
(also new in v0.4.3) is not exposed through this values.yaml yet.
Restoring a cluster from an existing snapshot is a follow-up PR.
Assisted-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
- Use name='etcd' in all three e2e cases. The chart pins the Helm release name to 'etcd' via check-release-name.yaml, so any other name causes template rendering to fail before the HelmRelease can reconcile. This is why the tenant chart hardcodes HR name 'etcd' in packages/apps/tenant/templates/etcd.yaml. - Consolidate all backup validation guards into etcd-backup-schedule.yaml so Helm's alphabetical render order no longer determines which failure a user sees first. Previously, a bad destinationPath would be masked by the earlier placeholder check in backup-secret.yaml. - Use regex match '^<.*>$' for placeholder detection instead of exact string equality. The previous check silently stopped firing if anyone updated the default placeholder string in values.yaml; the regex form catches any angle-bracketed default. - Reject destinationPath strings containing '//' after the s3:// prefix. Previously 's3://bucket//subdir' silently produced a key beginning with '/', confusing S3 clients and retention policies. - Document the backup contract in README.md: what enabling it produces, which fields are required, why placeholders are rejected, and that restore is a known follow-up gap. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Fourth review round fixes. Simplify backup validation by dropping placeholder-regex guards and switching default values of s3AccessKey, s3SecretKey, destinationPath, and endpointURL from angle-bracketed hints to empty strings. A single 'required' check on each field is now the whole validation for presence; the regex guard was brittle (silently stopped matching if the hint ever changed) and dropping it makes the values.yaml defaults match the behavior contract the README describes. s3:// prefix and '//' rejection still live in etcd-backup-schedule.yaml as template-time fail branches. Add helm-unittest suites in packages/extra/etcd/tests/ covering both templates end-to-end: rendered output assertions for the happy path (bucket/key split, region passthrough, conditional rendering of optional fields) and failedTemplate assertions for every fail branch. A 'test' target is wired into the package Makefile so hack/helm-unit-tests.sh picks it up on 'make unit-tests'. Add type: Opaque to backup-secret.yaml for consistency with the mongodb template. E2E: wait for HelmRelease, Secret, and EtcdBackupSchedule deletion in setup() so the empty-backup case does not flake against lingering state from the previous test. Replace the fictitious minio-e2e hostname with no-such-endpoint.invalid and explicitly document that the e2e verifies schedule/CronJob materialization, not backup execution. Restore via EtcdCluster.spec.bootstrap and the one-shot EtcdBackup CRD are still not exposed through this chart; both are explicit follow-ups called out in the README. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
v0.4.3 introduced new CRDs (EtcdBackup, EtcdBackupSchedule). Flux defaults to Skip on upgrade, so on existing clusters those CRDs never land without a manual kubectl apply. Setting upgradeCRDs: CreateReplace on the etcd-operator component tells PackageReconciler to wire hr.Spec.Upgrade.Crds accordingly. etcd-operator's CRDs evolve additively between versions, so CreateReplace is safe here. Depends on the PackageSource.Spec.Variants[].Components[].Install.UpgradeCRDs field — must ship with a cozystack-operator release that recognizes it. Without that, this field is ignored and the HelmRelease still defaults to Skip. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
dosubot
Bot
added
the
size/L
Contributor
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 upgrades the etcd-operator to v0.4.3 and introduces support for scheduled S3 backups within the Cozystack Etcd application. It adds the necessary CRD management, configuration parameters, and validation logic to enable reliable, automated snapshotting of etcd clusters to S3-compatible storage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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
|
dosubot
Bot
added
the
kind/feature
Contributor
📝 WalkthroughWalkthroughAdds scheduled S3 backup support for Etcd: new CRDs ( Changes
Sequence DiagramsequenceDiagram
participant User as User/Client
participant Chart as Etcd Helm Chart
participant K8s as Kubernetes API
participant Op as Etcd Operator
participant CJ as CronJob Controller
participant S3 as S3 Storage
User->>Chart: Apply Etcd manifest (backup enabled)
Chart->>K8s: Create Secret `etcd-s3-creds`
Chart->>K8s: Create EtcdBackupSchedule CR
Op->>K8s: Reconcile EtcdBackupSchedule -> create CronJob
CJ->>K8s: Trigger CronJob per schedule -> create Job
Job->>S3: Upload snapshot to S3
S3-->>Job: Confirm upload
Op->>K8s: Update EtcdBackupSchedule status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces scheduled S3 backup support for the etcd package, adding templates for backup schedules and secrets, updating the etcd-operator configuration, and providing documentation and tests. The review feedback recommends improving the JSON schema by marking mandatory backup fields as required, defining resource requests and limits for the backup workload to ensure stability, and using explicit integer casting in Helm templates for job history limits to avoid type errors.
Comment on lines
5
to
61
| "backup": { | ||
| "description": "Backup configuration.", | ||
| "type": "object", | ||
| "default": {}, | ||
| "properties": { | ||
| "destinationPath": { | ||
| "description": "Destination path for backups (e.g. s3://bucket/path/).", | ||
| "type": "string", | ||
| "default": "" | ||
| }, | ||
| { | ||
| "type": "string" | ||
| "enabled": { | ||
| "description": "Enable scheduled S3 backups.", | ||
| "type": "boolean", | ||
| "default": false | ||
| }, | ||
| "endpointURL": { | ||
| "description": "S3 endpoint URL for uploads.", | ||
| "type": "string", | ||
| "default": "" | ||
| }, | ||
| "failedJobsHistoryLimit": { | ||
| "description": "Number of failed backup jobs to retain.", | ||
| "type": "integer", | ||
| "default": 1 | ||
| }, | ||
| "forcePathStyle": { | ||
| "description": "Use path-style S3 URLs (required for MinIO and most S3-compatible providers).", | ||
| "type": "boolean", | ||
| "default": true | ||
| }, | ||
| "region": { | ||
| "description": "S3 region.", | ||
| "type": "string", | ||
| "default": "" | ||
| }, | ||
| "s3AccessKey": { | ||
| "description": "Access key for S3 authentication.", | ||
| "type": "string", | ||
| "default": "" | ||
| }, | ||
| "s3SecretKey": { | ||
| "description": "Secret key for S3 authentication.", | ||
| "type": "string", | ||
| "default": "" | ||
| }, | ||
| "schedule": { | ||
| "description": "Cron schedule for automated backups.", | ||
| "type": "string", | ||
| "default": "0 2 * * *" | ||
| }, | ||
| "successfulJobsHistoryLimit": { | ||
| "description": "Number of successful backup jobs to retain.", | ||
| "type": "integer", | ||
| "default": 3 | ||
| } | ||
| ], | ||
| "x-kubernetes-int-or-string": true | ||
| }, | ||
| "storageClass": { | ||
| "description": "StorageClass used to store the data.", | ||
| "type": "string", | ||
| "default": "" | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
The backup configuration block contains several fields that are effectively required when backup.enabled is set to true. To improve the user experience in the dashboard and provide earlier validation, consider adding a required array to the backup object schema for destinationPath, endpointURL, s3AccessKey, and s3SecretKey.
Contributor
Author
There was a problem hiding this comment.
Good catch on the UX gap, but it's blocked on the generator rather than a local edit. cozyvalues-gen (which owns values.schema.json for every chart in this repo) only exposes [field]/field for unconditional optional/required and has no annotation for conditional required that tracks backup.enabled. Render-time validation via the Helm required function already produces a clear error, so I've filed the generator feature request and a tracking issue here to revisit once it lands: cozystack/cozyvalues-gen#23, #2486.
Comment on lines
+12
to
+34
| apiVersion: etcd.aenix.io/v1alpha1 | ||
| kind: EtcdBackupSchedule | ||
| metadata: | ||
| name: etcd | ||
| spec: | ||
| clusterRef: | ||
| name: etcd | ||
| schedule: {{ .Values.backup.schedule | quote }} | ||
| successfulJobsHistoryLimit: {{ .Values.backup.successfulJobsHistoryLimit }} | ||
| failedJobsHistoryLimit: {{ .Values.backup.failedJobsHistoryLimit }} | ||
| destination: | ||
| s3: | ||
| endpoint: {{ $endpointURL | quote }} | ||
| bucket: {{ $bucket | quote }} | ||
| {{- with $key }} | ||
| key: {{ . | quote }} | ||
| {{- end }} | ||
| {{- with .Values.backup.region }} | ||
| region: {{ . | quote }} | ||
| {{- end }} | ||
| forcePathStyle: {{ .Values.backup.forcePathStyle }} | ||
| credentialsSecretRef: | ||
| name: etcd-s3-creds |
Contributor
There was a problem hiding this comment.
The EtcdBackupSchedule workload currently lacks resource requests and limits. Per the project guidelines, new workloads should have resource configurations to ensure cluster stability and predictable scheduling. Consider adding a resources block to the EtcdBackupSchedule spec, potentially mapping it to a new backup.resources value.
References
- Missing resource requests/limits on new workloads should be flagged.
Contributor
Author
There was a problem hiding this comment.
The EtcdBackupSchedule CRD shipped by etcd-operator v0.4.3 has no resources or pod-template field — the backup pod spec is produced internally by the operator from a CronJob template it owns (see the CRD in packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yaml). Adding resources to this manifest would make Helm render an unknown field and the API server would reject it. Resource sizing for the backup pod needs to land upstream in etcd-operator first; I'll revisit once the CRD exposes it.
Comment on lines
+20
to
+21
| successfulJobsHistoryLimit: {{ .Values.backup.successfulJobsHistoryLimit }} | ||
| failedJobsHistoryLimit: {{ .Values.backup.failedJobsHistoryLimit }} |
Contributor
There was a problem hiding this comment.
While the schema defines these as integers, it is safer to explicitly cast them to integers in the template to avoid potential type mismatch issues if they are passed as strings via Helm's --set flag.
successfulJobsHistoryLimit: {{ .Values.backup.successfulJobsHistoryLimit | int }}
failedJobsHistoryLimit: {{ .Values.backup.failedJobsHistoryLimit | int }}
Contributor
Author
There was a problem hiding this comment.
Good call — applied in aaf9dd0. Both successfulJobsHistoryLimit and failedJobsHistoryLimit now flow through | int in packages/extra/etcd/templates/etcd-backup-schedule.yaml.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extra/etcd/values.schema.json (1)
1-122:⚠️ Potential issue | 🟡 MinorRegenerate to satisfy pre-commit.
Pre-Commit Checks reports this tracked file would be modified by
make generate. As per learnings,values.schema.jsonis auto-generated bycozyvalues-genfromvalues.yaml; please re-run the generator and commit the result rather than editing by hand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extra/etcd/values.schema.json` around lines 1 - 122, The tracked values.schema.json is auto-generated and must be regenerated from values.yaml using the cozyvalues-gen generator (run the project generator via make generate or the cozyvalues-gen command) instead of manual edits; re-run make generate (or cozyvalues-gen pointed at packages/extra/etcd/values.yaml), commit the updated values.schema.json, and ensure pre-commit passes so the file no longer appears modified by the generator.
🧹 Nitpick comments (1)
hack/e2e-apps/etcd.bats (1)
61-67: Consider a short settle window before asserting absence.After
wait hr etcd --for=condition=ready, the HelmRelease is reconciled, so withbackup.enabled=falsethe schedule/secret will indeed never be rendered — the assertions are correct. However, if a prior test left anetcd-s3-credsSecret orEtcdBackupSchedulebehind and thesetup()--for=deletetimed out silently (both use|| true), thesegetchecks could pass or fail non-deterministically on a re-run. A quickkubectl get ... --ignore-not-foundplus an explicit emptiness assert, or tighteningsetup()to not swallow timeout failures for these specific resources, would make the negative assertion more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/etcd.bats` around lines 61 - 67, The negative assertions for etcdbackupschedule.etcd.aenix.io (EtcdBackupSchedule) and secret etcd-s3-creds are flakey because prior artifacts can persist; add a short settle/check step after waiting for HelmRelease readiness in hack/e2e-apps/etcd.bats: either (A) run kubectl get secret etcd-s3-creds --ignore-not-found and kubectl get etcdbackupschedule.etcd.aenix.io etcd --ignore-not-found and explicitly assert the outputs are empty, or (B) tighten setup() so the prior --for=delete calls do not swallow timeouts for these two resources, ensuring any leftover Secret/EtcdBackupSchedule is removed before asserting absence; update the test to use one of these approaches so the negative checks against etcd-s3-creds and etcdbackupschedule.etcd.aenix.io are deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/e2e-apps/etcd.bats`:
- Around line 17-22: The diagnostic function dump_diagnostics currently uses the
wrong redirection order (e.g., "2>&1 >&3") which causes stderr to be sent to the
test's default stdout instead of fd 3; update each kubectl invocation inside
dump_diagnostics (the lines calling kubectl -n tenant-test get ..., kubectl -n
tenant-test describe ..., and kubectl -n cozy-etcd-operator logs ...) to
redirect stdout to fd 3 first and then stderr to follow (i.e., change the order
so stdout goes to >&3 and then stderr is redirected to that fd).
In `@packages/extra/etcd/README.md`:
- Line 9: The manually-added "Backups" prose in packages/extra/etcd/README.md
will be lost on regeneration; move that guidance into the chart's values.yaml
using the `@section` pattern and inline comments (follow the example in
packages/apps/postgres/values.yaml) so it persists when cozyvalues-gen runs, and
update wording to use "custom resource manifest" instead of "CRD" for EtcdBackup
and any references to EtcdCluster.spec.bootstrap; ensure the values.yaml entry
documents that restore/one-shot backup requires applying the EtcdBackup custom
resource instance and where to find the manifest/template.
In `@packages/system/etcd-rd/cozyrds/etcd.yaml`:
- Line 11: The committed openAPISchema in etcd.yaml is out of sync with the
generated schema; run the generator and commit the result: run make generate
(which uses cozyvalues-gen to regenerate the embedded openAPISchema from
packages/extra/etcd/values.yaml and values.schema.json), verify the diff updates
packages/system/etcd-rd/cozyrds/etcd.yaml, and commit the regenerated file so
the openAPISchema in etcd.yaml matches the generated output.
---
Outside diff comments:
In `@packages/extra/etcd/values.schema.json`:
- Around line 1-122: The tracked values.schema.json is auto-generated and must
be regenerated from values.yaml using the cozyvalues-gen generator (run the
project generator via make generate or the cozyvalues-gen command) instead of
manual edits; re-run make generate (or cozyvalues-gen pointed at
packages/extra/etcd/values.yaml), commit the updated values.schema.json, and
ensure pre-commit passes so the file no longer appears modified by the
generator.
---
Nitpick comments:
In `@hack/e2e-apps/etcd.bats`:
- Around line 61-67: The negative assertions for
etcdbackupschedule.etcd.aenix.io (EtcdBackupSchedule) and secret etcd-s3-creds
are flakey because prior artifacts can persist; add a short settle/check step
after waiting for HelmRelease readiness in hack/e2e-apps/etcd.bats: either (A)
run kubectl get secret etcd-s3-creds --ignore-not-found and kubectl get
etcdbackupschedule.etcd.aenix.io etcd --ignore-not-found and explicitly assert
the outputs are empty, or (B) tighten setup() so the prior --for=delete calls do
not swallow timeouts for these two resources, ensuring any leftover
Secret/EtcdBackupSchedule is removed before asserting absence; update the test
to use one of these approaches so the negative checks against etcd-s3-creds and
etcdbackupschedule.etcd.aenix.io are deterministic.
🪄 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: 3d067dff-ec2a-40f4-b59b-3bd734d94694
📒 Files selected for processing (18)
hack/e2e-apps/etcd.batspackages/core/platform/sources/etcd-operator.yamlpackages/extra/etcd/Makefilepackages/extra/etcd/README.mdpackages/extra/etcd/templates/backup-secret.yamlpackages/extra/etcd/templates/etcd-backup-schedule.yamlpackages/extra/etcd/tests/backup-secret_test.yamlpackages/extra/etcd/tests/etcd-backup-schedule_test.yamlpackages/extra/etcd/values.schema.jsonpackages/extra/etcd/values.yamlpackages/system/etcd-operator/charts/etcd-operator/Chart.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.ymlpackages/system/etcd-rd/cozyrds/etcd.yaml
Run 'make generate' under packages/extra/etcd to refresh the JSON schema and the corresponding ResourceDefinition YAML after adding backup and resources fields. Matches cozyvalues-gen v1.3.0 output used by the pre-commit CI check. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
cozytest.sh does not implement the bats 'run <cmd>' helper and '$status' variable, so the 'Create Etcd with empty backup block (disabled by default)' test died with 'run: not found' on every attempt. Replace the bats idiom with plain shell: use '!' prefix and redirect stderr to assert the resources absent-when-backup-disabled invariant directly. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
hack/e2e-apps/etcd.bats (1)
19-21:⚠️ Potential issue | 🟡 MinorKeep stderr on fd 3 for diagnostics.
This is the same redirection issue previously noted:
2>&1 >&3sends stderr to the original stdout before stdout is moved to fd 3. Use>&3 2>&1sokubectlerrors appear in the Bats diagnostic stream.🛠️ Proposed fix
- kubectl -n tenant-test get etcdcluster,etcdbackupschedule,cronjob -o wide 2>&1 >&3 || true - kubectl -n tenant-test describe etcdbackupschedule etcd 2>&1 >&3 || true - kubectl -n cozy-etcd-operator logs -l app.kubernetes.io/name=etcd-operator --tail=100 2>&1 >&3 || true + kubectl -n tenant-test get etcdcluster,etcdbackupschedule,cronjob -o wide >&3 2>&1 || true + kubectl -n tenant-test describe etcdbackupschedule etcd >&3 2>&1 || true + kubectl -n cozy-etcd-operator logs -l app.kubernetes.io/name=etcd-operator --tail=100 >&3 2>&1 || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/etcd.bats` around lines 19 - 21, The three kubectl invocations in hack/e2e-apps/etcd.bats (the "kubectl -n tenant-test get etcdcluster,etcdbackupschedule,cronjob -o wide", the "kubectl -n tenant-test describe etcdbackupschedule etcd", and the "kubectl -n cozy-etcd-operator logs -l app.kubernetes.io/name=etcd-operator --tail=100" lines) currently use the wrong redirection ordering; change the order so stdout is duplicated to file descriptor 3 first and then stderr is redirected to stdout (so that stderr ends up on fd 3) to ensure kubectl errors are captured by the Bats diagnostic stream.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/e2e-apps/etcd.bats`:
- Around line 8-15: The setup() function currently swallows cleanup errors with
"|| true" on kubectl delete/wait commands (kubectl -n tenant-test delete
etcd.apps.cozystack.io --all --ignore-not-found --timeout=2m, kubectl -n
tenant-test wait hr/etcd --for=delete ..., kubectl -n tenant-test wait
secret/etcd-s3-creds ..., kubectl -n tenant-test wait
etcdbackupschedule.etcd.aenix.io/etcd ...), which masks teardown failures;
either remove the "|| true" so failures bubble and fail the test setup, or move
these cleanup calls out of setup() into the framework/global teardown path
(ensuring the same resource names are cleaned there) so per-test setup does not
hide errors.
- Around line 83-89: Update the schedule cron expression in the etcd E2E test so
backup Jobs won't run during the ~365s test window: replace the high-frequency
schedule value for the "schedule" key (found next to destinationPath and
endpointURL in hack/e2e-apps/etcd.bats) with a low-frequency expression (e.g., a
daily or weekly cron like "0 0 * * *" or "@weekly") for both occurrences
referenced in the diff so the test only verifies CronJob rendering and does not
trigger actual backup Jobs.
- Around line 39-40: Replace the brittle "sleep 5" + "kubectl -n tenant-test
wait hr etcd --for=condition=ready" pattern with a polling helper that first
checks for the HelmRelease's existence (e.g., wait_for_hr_exists namespace name)
and only then calls "kubectl ... wait ... --for=condition=ready"; update all
occurrences where "sleep 5" and "kubectl -n tenant-test wait hr etcd
--timeout=60s --for=condition=ready || { dump_diagnostics; false; }" appear
(lines around the three occurrences) to call this new helper, ensuring the
helper uses a loop with a timeout and exits to dump_diagnostics on failure.
---
Duplicate comments:
In `@hack/e2e-apps/etcd.bats`:
- Around line 19-21: The three kubectl invocations in hack/e2e-apps/etcd.bats
(the "kubectl -n tenant-test get etcdcluster,etcdbackupschedule,cronjob -o
wide", the "kubectl -n tenant-test describe etcdbackupschedule etcd", and the
"kubectl -n cozy-etcd-operator logs -l app.kubernetes.io/name=etcd-operator
--tail=100" lines) currently use the wrong redirection ordering; change the
order so stdout is duplicated to file descriptor 3 first and then stderr is
redirected to stdout (so that stderr ends up on fd 3) to ensure kubectl errors
are captured by the Bats diagnostic stream.
🪄 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: f8150982-1e6b-4c4e-9345-0b983afc8e93
📒 Files selected for processing (1)
hack/e2e-apps/etcd.bats
Address review feedback from gemini-code-assist on packages/extra/etcd/templates/etcd-backup-schedule.yaml:20-21: explicitly coerce successfulJobsHistoryLimit and failedJobsHistoryLimit to int so string values passed via helm --set do not produce a type mismatch in the EtcdBackupSchedule spec. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
requested a review
from myasnikovdaniil
as a code owner
April 24, 2026 10:42
Address review feedback from coderabbitai on hack/e2e-apps/etcd.bats:19-21: redirect stdout to fd 3 first, then dup stderr onto the new fd 1 so failures from kubectl (NotFound, RBAC, timeouts) reach the test diagnostics stream instead of being swallowed by the bats-captured default stdout. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Address review feedback from coderabbitai on packages/extra/etcd/README.md:9: CRDs are schema definitions; what users apply to trigger a restore or one-shot backup is a custom resource instance of the EtcdBackup kind, not the CRD itself. Reword accordingly. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Address review feedback from coderabbitai on hack/e2e-apps/etcd.bats:9-14: drop the '|| true' guards in setup() so real teardown failures (RBAC errors, stuck finalizers, wait timeouts) fail the test setup instead of letting the next test run against stale HelmRelease, Secret, or EtcdBackupSchedule state. --ignore-not-found still covers the empty-namespace case on a clean run. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…diness Address review feedback from coderabbitai on hack/e2e-apps/etcd.bats:39-40: replace the sleep-then-wait pattern with a wait_etcd_hr_ready helper that polls hr/etcd until the Cozystack API reconciles the Etcd CR and only then waits for condition=ready. kubectl wait exits immediately when the resource does not yet exist, which caused spurious failures when HelmRelease creation took longer than 5s. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Address review feedback from coderabbitai on hack/e2e-apps/etcd.bats:89: the test verifies that the chart renders EtcdBackupSchedule and that the etcd-operator materializes a CronJob, not that backups fire. Use "0 0 1 1 *" so no Jobs run against the intentionally invalid endpoint during the ~6-minute test window. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hack/e2e-apps/etcd.bats (1)
69-70: Negated existence check can silently pass on transient API errors.
! kubectl get ... 2>/dev/nulltreats any non-zero exit (NotFound, but also RBAC/API errors, unreachable apiserver, missing CRD) as "resource absent" and passes the test. In this suite the HR is ready and theEtcdBackupScheduleCRD is guaranteed, so it's not an actionable bug today — just note that switching to a NotFound-specific check (e.g. grepping the error, orkubectl get ... -o name | wc -lequal to 0) would harden it against future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/etcd.bats` around lines 69 - 70, The negated checks using "! kubectl -n tenant-test get etcdbackupschedule.etcd.aenix.io etcd 2>/dev/null" and the secret check can silently pass on transient API errors; change them to explicitly detect NotFound by running the kubectl get for "etcdbackupschedule.etcd.aenix.io etcd" and for "secret etcd-s3-creds", capture stdout/stderr, and assert absence only when the error text contains "NotFound" (or use a count-based check like `kubectl ... -o name | wc -l` equals 0) so RBAC/API/unreachable errors do not falsely mark the resource as absent. Ensure you update both occurrences referring to the etcdbackupschedule resource and the etcd-s3-creds secret.packages/extra/etcd/templates/etcd-backup-schedule.yaml (1)
32-32: Optional: guardforcePathStyleagainst a nil override.
{{ .Values.backup.forcePathStyle }}emits an empty scalar (forcePathStyle:) if the value is ever explicitly overridden to null. The chart'svalues.yaml/schema default (true) keeps this safe today, but a small defensive change avoids producing invalid YAML should that default change or a user passforcePathStyle: null:Proposed defensive render
- forcePathStyle: {{ .Values.backup.forcePathStyle }} + forcePathStyle: {{ .Values.backup.forcePathStyle | default false }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/extra/etcd/templates/etcd-backup-schedule.yaml` at line 32, The template currently renders forcePathStyle directly which can emit an empty scalar when overridden to null; change the rendering to use Helm's default function so null falls back to the safe boolean (e.g. replace the current interpolation of .Values.backup.forcePathStyle with {{ default true .Values.backup.forcePathStyle }}), keeping the YAML key name forcePathStyle as-is so the template always emits a valid true/false value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/e2e-apps/etcd.bats`:
- Around line 69-70: The negated checks using "! kubectl -n tenant-test get
etcdbackupschedule.etcd.aenix.io etcd 2>/dev/null" and the secret check can
silently pass on transient API errors; change them to explicitly detect NotFound
by running the kubectl get for "etcdbackupschedule.etcd.aenix.io etcd" and for
"secret etcd-s3-creds", capture stdout/stderr, and assert absence only when the
error text contains "NotFound" (or use a count-based check like `kubectl ... -o
name | wc -l` equals 0) so RBAC/API/unreachable errors do not falsely mark the
resource as absent. Ensure you update both occurrences referring to the
etcdbackupschedule resource and the etcd-s3-creds secret.
In `@packages/extra/etcd/templates/etcd-backup-schedule.yaml`:
- Line 32: The template currently renders forcePathStyle directly which can emit
an empty scalar when overridden to null; change the rendering to use Helm's
default function so null falls back to the safe boolean (e.g. replace the
current interpolation of .Values.backup.forcePathStyle with {{ default true
.Values.backup.forcePathStyle }}), keeping the YAML key name forcePathStyle
as-is so the template always emits a valid true/false value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b11b0d3-98e4-4e5b-9e93-639c68101701
📒 Files selected for processing (3)
hack/e2e-apps/etcd.batspackages/extra/etcd/README.mdpackages/extra/etcd/templates/etcd-backup-schedule.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/extra/etcd/README.md
Aleksei Sviridkin (lexfrei)
added a commit
to cozystack/website
that referenced
this pull request
May 4, 2026
## What this PR does Bumps the pinned etcd-operator version in the v1.2 platform-stack guide from `v0.4.2` to `v0.4.3`. Accompanies cozystack/cozystack#2428, which re-vendors the chart and adds the `backup.*` API surface on the managed etcd app. The new backup parameters will land in `content/en/docs/v1.2/operations/services/etcd.md` automatically when the release-tag sync job pulls the updated `packages/extra/etcd/README.md` — no hand-edited doc is needed here. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated Kubernetes tenant etcd deployment docs to reference the operator component v0.4.3 (was v0.4.2). * Clarified wording by renaming the bullet prefix from “Operator” to “Kubernetes operator” for clearer architecture guidance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
myasnikovdaniil
pushed a commit
to tym83/cozystack-website
that referenced
this pull request
May 18, 2026
Accompanies cozystack/cozystack#2428 which updates the vendored chart and adds the S3 backup schedule API surface. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Bumps the vendored
etcd-operatorchart fromv0.4.2tov0.4.3and wires the new backup machinery through the cozystackEtcdapp API.Upstream v0.4.3 ships two new CRDs —
EtcdBackup(one-shot snapshot) andEtcdBackupSchedule(periodic snapshots) — along with theEtcdCluster.spec.bootstraprestore field, a newOPERATOR_IMAGEenv on the operator deployment, and a replacement of the deprecatedgcr.io/kubebuilder/kube-rbac-proxyimage. The chart portion of this PR is a clean re-vendor viamake update— no local patches.On top of the bump,
packages/extra/etcdexposes a new optionalbackup.*values block. Whenbackup.enabled=true, the chart renders anEtcdBackupSchedulereferencing the platformEtcdCluster/etcdand aSecret/etcd-s3-credswith the S3 credentials expected by the backup-agent. The values field names (destinationPath,endpointURL,s3AccessKey,s3SecretKey,schedule) mirror the existingpostgresbackup pattern for consistency. The chartfails at template render if any of the four required S3 fields are empty or ifdestinationPathis malformed (missings3://prefix,//segments, no bucket).Test coverage: 15
helm-unittestcases acrosstests/backup-secret_test.yamlandtests/etcd-backup-schedule_test.yaml(wired intomake test, picked up byhack/helm-unit-tests.sh), plus three bats cases inhack/e2e-apps/etcd.batscovering the baselineEtcdapp, thebackup: {}default-off path, and the fullbackup.enabled=truecase that verifies theEtcdBackupSchedulematerializes and the operator generates the downstreamCronJob.packages/core/platform/sources/etcd-operator.yamlnow setsupgradeCRDs: CreateReplaceon the etcd-operator component, so Flux applies the new CRDs on upgrade of the chart instead of silently skipping them.Depends on #2427.
Scope deferred to follow-ups
EtcdBackupCRD is vendored but not exposed through this chart — users can still hand-apply the CRD directly.EtcdCluster.spec.bootstrap(restore from an existing snapshot) is likewise vendored but not exposed. Both gaps are called out inpackages/extra/etcd/README.md.Release note
Summary by CodeRabbit
New Features
Tests
Documentation
Chores