feat(platform): implement backup strategy for postgres#2552
Conversation
androndo
requested review from
IvanHunters,
kvaps,
lexfrei,
lllamnyp,
myasnikovdaniil and
sircthulhu
as code owners
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a robust backup and restore strategy for Postgres applications using the CloudNativePG (CNPG) operator. By leveraging the CNPG operator's native capabilities, this change allows for more reliable backup management and flexible recovery options, including in-place and to-copy restores. The implementation emphasizes security by ensuring that S3 credentials are handled via references rather than direct inclusion in resource specifications, and it includes necessary controller logic, API definitions, and end-to-end tests to ensure stability. 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-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
|
github-actions
Bot
added
area/platform
|
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 CNPG backup/restore strategy support and optional referencing of pre-existing S3 credential Secrets. Changes include new CNPG strategy API types/CRD, controller reconcile paths for CNPG backup/restore and cleanup dispatch, minimal CNPG/Postgres typed objects, Helm chart wiring to prefer external credential Secrets, example manifests, deepcopy updates, RBAC expansion, and tests. ChangesCNPG backup strategy + S3 credential secret support
Sequence Diagram(s)sequenceDiagram
participant User
participant BackupController
participant CNPGStrategy
participant PostgresApp
participant CNPGCluster
participant SecretStore
User->>BackupController: Create BackupJob (strategyRef: CNPG)
BackupController->>CNPGStrategy: Fetch strategy/template
BackupController->>PostgresApp: Fetch source Postgres app
BackupController->>CNPGCluster: SSA-apply patch -> spec.backup.barmanObjectStore
CNPGCluster-->>BackupController: Patch accepted / Cluster present?
BackupController->>CNPGCluster: Create cnpg.io/Backup (one-shot)
CNPGCluster->>BackupController: Update Backup status (running -> completed/failed)
BackupController->>SecretStore: Use referenced Secret name (if template provides)
BackupController->>BackupController: Create Cozystack Backup artifact (driver metadata + snapshot)
BackupController->>User: Update BackupJob status (Succeeded / Failed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
dosubot
Bot
added
area/database
![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 CloudNativePG (CNPG) backup and restore strategy for Postgres applications, including the necessary API types, controller logic, and Helm chart updates to support external S3 credential secrets. The review feedback highlights a critical issue with Server-Side Apply (SSA) patches missing TypeMeta, a potential data loss scenario during to-copy restores if the source application is missing, and opportunities to improve performance and race condition handling during resource purging.
| func newCNPGClusterPatch(namespace, name string) *cnpgtypes.Cluster { | ||
| return &cnpgtypes.Cluster{ | ||
| ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}, | ||
| } | ||
| } |
There was a problem hiding this comment.
The newCNPGClusterPatch helper creates a Cluster object for Server-Side Apply (SSA) but fails to initialize TypeMeta. SSA requires Kind and APIVersion to be set on the object to correctly identify the resource on the server side. Without these, the r.Patch call at line 254 may fail or behave unexpectedly.
func newCNPGClusterPatch(namespace, name string) *cnpgtypes.Cluster {
return &cnpgtypes.Cluster{
TypeMeta: metav1.TypeMeta{
APIVersion: cnpgtypes.GroupVersion.String(),
Kind: "Cluster",
},
ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name},
}
}| for i := range pvcList.Items { | ||
| pvc := &pvcList.Items[i] | ||
| if err := r.Delete(ctx, pvc); err != nil && !apierrors.IsNotFound(err) { | ||
| return fmt.Errorf("delete PVC %s/%s: %w", pvc.Namespace, pvc.Name, err) | ||
| } | ||
| } | ||
| logger.Debug("deleted cnpg cluster PVCs", "count", len(pvcList.Items)) |
There was a problem hiding this comment.
When purging the existing cluster, PVCs are deleted immediately after the Cluster object. Since r.Delete is asynchronous, the pods belonging to the cluster might still be running and holding the PVCs. While Kubernetes handles this via finalizers, it might be safer to ensure pods are terminating before attempting to wipe the PVCs to avoid race conditions where the operator might try to re-attach them if the deletion is slow.
| func toJSONMap(obj interface{}) (map[string]interface{}, error) { | ||
| raw, err := json.Marshal(obj) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| out := map[string]interface{}{} | ||
| if err := json.Unmarshal(raw, &out); err != nil { | ||
| return nil, err | ||
| } | ||
| return out, nil | ||
| } |
There was a problem hiding this comment.
The toJSONMap function performs a full JSON marshal and unmarshal on every reconcile loop where templating is required. While functional for providing a clean map for Go templates, this is computationally expensive. If performance becomes an issue, consider using a more direct reflection-based approach or caching the result if the application object hasn't changed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
internal/backupcontroller/cnpgtypes/zz_generated.deepcopy.go (1)
3-5: ⚡ Quick winAdd a deepcopy regression test to guard manual drift.
Since these methods are intentionally hand-maintained (Line 3-5), add a small aliasing test that mutates copied nested fields and asserts the original is unchanged. This will catch future misses when structs in
types.goevolve.As per coding guidelines, "Use controller-runtime patterns and kubebuilder style for Go code in the Cozystack project."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/backupcontroller/cnpgtypes/zz_generated.deepcopy.go` around lines 3 - 5, Add a regression unit test in the internal/backupcontroller/cnpgtypes package (following controller-runtime/kubebuilder test patterns) that verifies the hand-written DeepCopy methods do not alias nested fields: create an instance of a representative struct from types.go, call its DeepCopy/DeepCopyInto methods (e.g., MyType.DeepCopy/DeepCopyInto or other exported types present), mutate nested slices/maps/struct fields on the copy, and assert the original value is unchanged; use the same test harness style as other controller tests (testing.T, table-driven if needed) and include clear failure messages to catch future manual-drift in the zz_generated.deepcopy implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/backupcontroller/cnpg_rbac_test.go`:
- Around line 37-44: The test currently only flags rules where an APIGroups
entry equals "" and a Resources entry equals "secrets"; update the guardrail to
also treat wildcard entries as matching core/secrets. Specifically, in the loop
over rule.APIGroups and rule.Resources (variables rule.APIGroups,
rule.Resources, role.Name, i, rule.Verbs), consider group == "*" as covering the
core group and res == "*" as covering "secrets" (i.e., treat group != "" &&
group != "*" as the continue condition, and flag if res == "secrets" || res ==
"*"); keep the existing error message and parameters.
In `@internal/backupcontroller/cnpgstrategy_controller.go`:
- Around line 611-619: The current readSourceDatabasesAndUsers in
RestoreJobReconciler returns (nil, nil, nil) when getPostgresApp returns
IsNotFound, which allows restores to proceed without source DB/User info; change
this to fail the restore explicitly: in readSourceDatabasesAndUsers (and where
it calls getPostgresApp) return a clear error (e.g., wrap or return
fmt.Errorf("source PostgresApp %s/%s not found", namespace, name) or a sentinel
ErrSourcePostgresAppNotFound) instead of nil,nil,nil, and ensure the caller of
readSourceDatabasesAndUsers aborts the restore path on that error (update error
handling in the reconcile path that invokes readSourceDatabasesAndUsers).
Alternatively, if you prefer the persistence approach, implement logic in
readSourceDatabasesAndUsers to load databases/users from the Backup artifact
when getPostgresApp IsNotFound (fetch the Backup/CR and populate
src.Spec.Databases and src.Spec.Users) and return those maps so the restore can
safely proceed; choose one approach and implement consistent error handling or
backup-reading accordingly.
- Around line 588-597: The deep copy retains cleartext S3 keys on the Postgres
CR; when switching to using S3 credentials secret you must scrub any inline keys
from patched.Spec.Backup to avoid leaving s3AccessKey/s3SecretKey in etcd/audit
logs—after you set patched.Spec.Backup.S3CredentialsSecret (or whenever credsRef
!= nil) clear patched.Spec.Backup.S3AccessKey and
patched.Spec.Backup.S3SecretKey (and any equivalent inline fields) so only the
secret-ref remains; ensure this sanitization happens in the same block that
assigns patched.Spec.Backup.S3CredentialsSecret and also when switching from
secret to inline to cover both transitions.
- Around line 98-125: The controller currently only checks
j.Spec.ApplicationRef.Kind but not API group, allowing non-Cozystack Postgres
refs to be looked up by getPostgresApp; update the validation before calling
getPostgresApp to also verify j.Spec.ApplicationRef.APIVersion (or group)
matches the Cozystack Postgres API group (apps.cozystack.io/v1alpha1) and return
via markBackupJobFailed with a clear message if it doesn't match; apply the
identical API group check in the restore path so both the backup reconcile flow
(before calling getPostgresApp) and the restore flow reject refs that are not
the expected Cozystack Postgres CRD.
- Around line 337-360: When r.Create(ctx, backup) returns an AlreadyExists
error, treat that as success by fetching the existing Backup resource and
returning it instead of propagating the error; specifically, detect
apierrors.IsAlreadyExists(err) after the r.Create call, do a r.Get(ctx,
types.NamespacedName{Name: backup.Name, Namespace: backup.Namespace},
existingBackup) (or equivalent) to load the current Backup object, and return
that existingBackup so a concurrent Status().Update on the BackupJob does not
flip a completed run into Failed; otherwise return the original error as before.
- Around line 581-604: The code using app.DeepCopy() unintentionally preserves
prior restore settings (recoveryTime, endpointURL, s3CredentialsSecret,
databases, users) when the current restore intends them to be empty; modify the
patch logic in cnpgstrategy_controller.go to explicitly clear those fields on
the copied object when the corresponding inputs are empty before applying new
values — e.g., reset patched.Spec.Bootstrap.RecoveryTime to empty when
recoveryTime == "", clear patched.Spec.Backup.EndpointURL when sourceEndpointURL
== "", set patched.Spec.Backup.S3CredentialsSecret to an empty/zero value when
credsRef is nil or credsRef.SecretRef.Name == "", and assign nil/empty slices to
patched.Spec.Databases and patched.Spec.Users when sourceDatabases/sourceUsers
are empty — while still setting Bootstrap.Enabled, OldName and ServerName as
currently done.
In `@internal/backupcontroller/restorejob_controller_test.go`:
- Around line 97-119: The test currently only seeds a Velero Restore owned by a
different job, so it passes even if cleanupVeleroRestore runs; update the test
to also seed a Velero Restore that is labeled as owned by the same RestoreJob
(use rj.Name and rj.Namespace labels) via newRestoreJobTestClient and include
that object alongside stray, then after calling
RestoreJobReconciler.cleanupOnDelete(ctx, rj) assert that this owner-labeled
Velero Restore still exists (Get into a got object) to verify the reconciler
does not touch Velero restores.
In `@packages/apps/postgres/templates/db.yaml`:
- Around line 16-18: The template currently assigns $credAccessKey and
$credSecretKey defaults even when no external secret name is provided, causing
CNPG to look for keys that don't exist; modify the db.yaml template so the
derived variables ($credAccessKey, $credSecretKey) are only set/overridden when
the external secret name ($credCfg.name) is present (i.e., wrap the defaulting
of $credAccessKey and $credSecretKey in the same conditional that detects
external-secret mode), leaving them unset otherwise so the existing
backup-secret.yaml uses its fixed AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY keys;
apply the same guard to the other duplicate blocks referenced (the blocks
currently at ~lines 26–30 and ~52–56).
---
Nitpick comments:
In `@internal/backupcontroller/cnpgtypes/zz_generated.deepcopy.go`:
- Around line 3-5: Add a regression unit test in the
internal/backupcontroller/cnpgtypes package (following
controller-runtime/kubebuilder test patterns) that verifies the hand-written
DeepCopy methods do not alias nested fields: create an instance of a
representative struct from types.go, call its DeepCopy/DeepCopyInto methods
(e.g., MyType.DeepCopy/DeepCopyInto or other exported types present), mutate
nested slices/maps/struct fields on the copy, and assert the original value is
unchanged; use the same test harness style as other controller tests (testing.T,
table-driven if needed) and include clear failure messages to catch future
manual-drift in the zz_generated.deepcopy implementations.
🪄 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: 7edf8314-83a7-4d9e-ad88-de0174355752
📒 Files selected for processing (33)
api/apps/v1alpha1/postgresql/types.goapi/apps/v1alpha1/postgresql/zz_generated.deepcopy.goapi/backups/strategy/v1alpha1/cnpg_types.goapi/backups/strategy/v1alpha1/zz_generated.deepcopy.gocmd/backupstrategy-controller/main.goexamples/backups/postgres/00-bucket.yamlexamples/backups/postgres/05-postgres-src.yamlexamples/backups/postgres/07-s3-credentials.yamlexamples/backups/postgres/10-cnpg-strategy.yamlexamples/backups/postgres/15-backupclass.yamlexamples/backups/postgres/20-plan.yamlexamples/backups/postgres/25-backupjob-adhoc.yamlexamples/backups/postgres/30-postgres-target.yamlexamples/backups/postgres/35-restorejob-in-place.yamlexamples/backups/postgres/40-restorejob-to-copy.yamlhack/e2e-apps/backup-postgres.batsinternal/backupcontroller/backupjob_controller.gointernal/backupcontroller/cnpg_rbac_test.gointernal/backupcontroller/cnpgstrategy_controller.gointernal/backupcontroller/cnpgstrategy_controller_test.gointernal/backupcontroller/cnpgtypes/types.gointernal/backupcontroller/cnpgtypes/zz_generated.deepcopy.gointernal/backupcontroller/postgresapp/types.gointernal/backupcontroller/postgresapp/zz_generated.deepcopy.gointernal/backupcontroller/restorejob_controller.gointernal/backupcontroller/restorejob_controller_test.gopackages/apps/postgres/README.mdpackages/apps/postgres/templates/backup-secret.yamlpackages/apps/postgres/templates/db.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_cnpgs.yamlpackages/system/backupstrategy-controller/templates/rbac.yaml
dosubot
Bot
added
size:XXL
and removed
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/apps/postgres/templates/db.yaml`:
- Around line 8-14: The multiline Helm block comments using the {{/* ... */}}
form (surrounding the s3Credentials explanatory text and again at the 17-25
region) are breaking YAML linting; replace those Helm block comments with
YAML-safe comments (use standard YAML # comments outside of Helm's block-comment
syntax or use single-line Helm comment tokens) so the template remains valid
YAML while preserving the same explanatory text for s3Credentials and the
backup/restore behavior.
🪄 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: db6dfedc-5811-426e-9bc2-7d6b7fab6c54
📒 Files selected for processing (11)
examples/backups/postgres/05-postgres-src.yamlinternal/backupcontroller/backup_controller.gointernal/backupcontroller/backup_controller_test.gointernal/backupcontroller/cnpg_rbac_test.gointernal/backupcontroller/cnpgstrategy_controller.gointernal/backupcontroller/cnpgstrategy_controller_test.gointernal/backupcontroller/cozyrd_drift_test.gointernal/backupcontroller/restorejob_controller_test.gopackages/apps/postgres/README.mdpackages/apps/postgres/templates/db.yamlpackages/system/postgres-rd/cozyrds/postgres.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/backups/postgres/05-postgres-src.yaml
- internal/backupcontroller/cnpg_rbac_test.go
- internal/backupcontroller/restorejob_controller_test.go
- packages/apps/postgres/README.md
- internal/backupcontroller/cnpgstrategy_controller.go
| {{/* | ||
| s3Credentials points at the operator-supplied Secret when | ||
| backup.s3CredentialsSecret.name is set; otherwise it falls back to the | ||
| chart-managed <release>-s3-creds Secret materialised from s3AccessKey/ | ||
| s3SecretKey. The override path keeps credentials out of the Postgres | ||
| CR .spec — the CNPG backup driver uses it on restore. | ||
| */}} |
There was a problem hiding this comment.
Replace multiline Helm block comments with YAML-safe comments to avoid lint breakage.
Static analysis flags a YAML parse error at Line 14. The multiline {{/* ... */}} blocks in this position are the likely trigger for YAMLlint.
Suggested fix
- {{/*
- s3Credentials points at the operator-supplied Secret when
- backup.s3CredentialsSecret.name is set; otherwise it falls back to the
- chart-managed <release>-s3-creds Secret materialised from s3AccessKey/
- s3SecretKey. The override path keeps credentials out of the Postgres
- CR .spec — the CNPG backup driver uses it on restore.
- */}}
+ # s3Credentials points at the operator-supplied Secret when
+ # backup.s3CredentialsSecret.name is set; otherwise it falls back to the
+ # chart-managed <release>-s3-creds Secret materialised from s3AccessKey/
+ # s3SecretKey. The override path keeps credentials out of the Postgres
+ # CR .spec — the CNPG backup driver uses it on restore.
{{- $credCfg := default (dict) .Values.backup.s3CredentialsSecret }}
{{- $credSecretName := $credCfg.name | default (printf "%s-s3-creds" .Release.Name) }}
- {{/*
- accessKeyIDKey/secretAccessKeyKey only apply when an external Secret was
- supplied via backup.s3CredentialsSecret.name. The chart-managed
- <release>-s3-creds Secret materialised by backup-secret.yaml hard-codes
- AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY, so applying overrides in
- that mode would point CNPG at non-existent keys and break credential
- reads. Default both names unconditionally and let the override take
- effect only in external-Secret mode.
- */}}
+ # accessKeyIDKey/secretAccessKeyKey only apply when an external Secret was
+ # supplied via backup.s3CredentialsSecret.name. The chart-managed
+ # <release>-s3-creds Secret in backup-secret.yaml uses AWS_ACCESS_KEY_ID /
+ # AWS_SECRET_ACCESS_KEY, so overrides should apply only in external-Secret mode.Also applies to: 17-25
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 14-14: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps/postgres/templates/db.yaml` around lines 8 - 14, The multiline
Helm block comments using the {{/* ... */}} form (surrounding the s3Credentials
explanatory text and again at the 17-25 region) are breaking YAML linting;
replace those Helm block comments with YAML-safe comments (use standard YAML #
comments outside of Helm's block-comment syntax or use single-line Helm comment
tokens) so the template remains valid YAML while preserving the same explanatory
text for s3Credentials and the backup/restore behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/pre-commit.yml (1)
42-46: ⚡ Quick winHarden
code-generatorversion extraction to avoid brittle failures.The current regex only matches
v[0-9.]+. If the pinned tag format changes (e.g.,-rcsuffix), this step will fail in a non-obvious way. A slightly broader parse plus an explicit empty-check makes failures clearer.Suggested patch
- version=$(grep -oP 'code-generator@\Kv[0-9.]+' hack/update-codegen.sh) + version=$(sed -nE 's|.*code-generator@(v[^"[:space:]}]+).*|\1|p' hack/update-codegen.sh | head -n1) + if [ -z "$version" ]; then + echo "::error::Failed to extract k8s.io/code-generator version from hack/update-codegen.sh" + exit 1 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pre-commit.yml around lines 42 - 46, The version extraction in hack/update-codegen.sh currently sets version=$(grep -oP 'code-generator@\Kv[0-9.]+' ...) which is brittle; update the grep to capture a broader semver-like string (e.g., allow alphanumerics, dashes and dots such as v1.2.3-rc1) and then add an explicit check that the variable version is non-empty before proceeding; modify the assignment to use the new regex (still assigning to version) and add a guard after that which logs a clear error and exits if version is empty so the go get "k8s.io/code-generator@${version}" step cannot run with an invalid 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 @.github/workflows/pre-commit.yml:
- Around line 42-46: The version extraction in hack/update-codegen.sh currently
sets version=$(grep -oP 'code-generator@\Kv[0-9.]+' ...) which is brittle;
update the grep to capture a broader semver-like string (e.g., allow
alphanumerics, dashes and dots such as v1.2.3-rc1) and then add an explicit
check that the variable version is non-empty before proceeding; modify the
assignment to use the new regex (still assigning to version) and add a guard
after that which logs a clear error and exits if version is empty so the go get
"k8s.io/code-generator@${version}" step cannot run with an invalid value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 203f7f6a-35f6-426c-8ba3-ca4e73e4229e
📒 Files selected for processing (1)
.github/workflows/pre-commit.yml
dosubot
Bot
added
the
size:XXL
github-actions
Bot
added
the
size/XXL
dosubot
Bot
added
the
size:XXL
github-actions
Bot
removed
the
size:XXL
dosubot
Bot
added
the
size:XXL
github-actions
Bot
added
the
size/XXL
dosubot
Bot
added
size:XXL
github-actions
Bot
removed
the
size:XXL
dosubot
Bot
added
the
size:XXL
github-actions
Bot
added
the
size/XXL
github-actions
Bot
removed
the
size:XXL
|
NOT LGTM Reviewing against target branch: Blocking issues:
The backup artifact already persists only a few rendered fields at Add a test with a CNPG strategy whose restore-relevant fields use
Verification run:
|
Branch ReviewNOT LGTM The CNPG backup strategy is well-architected and the code paths I read are mostly tight: SSA field-ownership on the live Cluster, the WAL-archive gate before destructive purge, the StartedAt refetch-before-write, the That said, several things must change before merge: Blockers1. Documentation/code drift in
|
androndo
changed the title
Branch ReviewLGTM (with non-blocking recommendations) Reviewing branch Scope of the feature:
Why LGTM despite the scopeThe controller code is unusually careful about the lifecycle hazards specific to CNPG bootstrap (admission immutability, archive-lag races, helm-controller rollback wiping SSA-applied fields, terminating-cluster vs fresh-recovery cluster ambiguity) and every one of those hazards is documented in a long comment AND covered by a unit test. Tests exist for: WAL archive gate ( Documentation in Recommended-to-fix (non-blocking)
Tests already cover all of the above where it matters: the Recommended notes are either cleanup (1, 5), policy/style (2, 4), or a slow-path retry that the existing test suite doesn't notice and probably should not (3). Each could be addressed in this PR or a fast follow-up; none block the merge.
|
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
2 participants
What this PR does
Screenshots
Release note
Summary by CodeRabbit
New Features
Examples
Security/RBAC
Documentation
Tests