feat(postgres): add serverName parameter for backup recovery#2362
feat(postgres): add serverName parameter for backup recovery#2362IvanHunters wants to merge 2 commits intomainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdded an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a serverName field to the PostgreSQL bootstrap configuration, enabling recovery from backups where the Barman server name does not match the Kubernetes cluster name. The changes encompass Go API types, Helm templates, documentation, and validation schemas. Review feedback highlights a critical issue where the groupName was changed to values.helm.io, which would break API compatibility for existing manifests. Additionally, it was noted that the +kubebuilder:object:root=true marker was moved to an incorrect location, which may interfere with CRD generation tools.
| // +kubebuilder:object:generate=true | ||
| // +groupName=apps.cozystack.io | ||
| // +kubebuilder:object:root=true | ||
| // +groupName=values.helm.io |
There was a problem hiding this comment.
The groupName has been changed from apps.cozystack.io to values.helm.io. This appears to be an unintended change, likely caused by a tool update or misconfiguration during code generation. Changing the group name will break API compatibility for the Postgres resource, as existing manifests and tests (e.g., postgres.bats) reference apps.cozystack.io.
| // +groupName=values.helm.io | |
| // +groupName=apps.cozystack.io |
| // Code generated by values-gen. DO NOT EDIT. | ||
| // +kubebuilder:object:generate=true | ||
| // +groupName=apps.cozystack.io | ||
| // +kubebuilder:object:root=true |
There was a problem hiding this comment.
The +kubebuilder:object:root=true marker has been moved to the package level. In standard Kubebuilder usage, this marker should be placed directly above the struct that represents the root of the CRD (in this case, the Config struct). Moving it to the top of the file may cause issues with CRD generation tools.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/v1alpha1/postgresql/types.go`:
- Around line 7-8: The generated Postgres artifacts under package postgresql
(types.go and related types/schema/CRD files) are out of sync; rerun the
project's generation flow (the values-gen/tooling used to create these files) to
regenerate the Postgres types/schema/CRD artifacts, verify there are no local
diffs, and commit the updated generated files so the generated code (including
the file that begins with "Code generated by values-gen. DO NOT EDIT." in
package postgresql) is in sync with the repository.
In `@packages/apps/postgres/values.yaml`:
- Around line 156-157: The description for oldName is now misleading: update the
comment for the field oldName to reflect that it represents the previous
Kubernetes cluster name (not the Barman backup.info serverName) and that
serverName should be used when the backup.info serverName differs; specifically,
change the oldName docstring to state it is the previous cluster name used for
recovery lookup and clarify that serverName is an explicit override for the
Barman serverName from backup.info when names differ (referencing the oldName
and serverName fields in the values.yaml).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d7a4a66-7b37-4b2c-a99b-99e8cb878276
📒 Files selected for processing (6)
api/apps/v1alpha1/postgresql/types.gopackages/apps/postgres/README.mdpackages/apps/postgres/templates/db.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/system/postgres-rd/cozyrds/postgres.yaml
cf948ae to
24439e7
Compare
24439e7 to
454c956
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/apps/v1alpha1/postgresql/types.go`:
- Around line 91-93: The comment for the OldName field is misleading; update the
docstring for OldName in types.go (the OldName string `json:"oldName"` field) to
state that it is the previous cluster name before deletion and that it may
differ from the serverName recorded in backup.info (do not claim they "match");
keep it concise and clarify its purpose (e.g., to correlate backups when cluster
names change).
In `@api/apps/v1alpha1/tcpbalancer/types.go`:
- Line 39: The Whitelist field on the TCPBalancer spec is marked with
`omitempty` which can remove the key and cause undefined ACLs in the HAProxy
template; remove `omitempty` from the struct tag on the `Whitelist []string`
field in the TCPBalancer types (the `Whitelist` field in
api/apps/v1alpha1/tcpbalancer/types.go) so the JSON always includes an empty
array when unset, ensuring the templates referencing `whitelist` always receive
a defined value.
In `@packages/apps/vm-disk/values.schema.json`:
- Around line 41-42: The schema lost the object type for the source.upload node
so validation is weakened; update the source generator or source values.yaml to
declare source.upload as an object (matching how source.http and source.image
are defined), then run the cozyvalues-gen regeneration (make generate) to emit
updated values.schema.json artifacts; specifically modify the values.yaml entry
for source.upload to include type: object (and any required properties) or
update the cozyvalues-gen mapping that builds source.upload before regenerating
the schema files.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3d7f951-4668-41e3-94fd-f89d0f36a86c
📒 Files selected for processing (58)
api/apps/v1alpha1/bucket/types.goapi/apps/v1alpha1/clickhouse/types.goapi/apps/v1alpha1/foundationdb/types.goapi/apps/v1alpha1/harbor/types.goapi/apps/v1alpha1/httpcache/types.goapi/apps/v1alpha1/kafka/types.goapi/apps/v1alpha1/mongodb/types.goapi/apps/v1alpha1/nats/types.goapi/apps/v1alpha1/openbao/types.goapi/apps/v1alpha1/opensearch/types.goapi/apps/v1alpha1/postgresql/types.goapi/apps/v1alpha1/qdrant/types.goapi/apps/v1alpha1/redis/types.goapi/apps/v1alpha1/tcpbalancer/types.goapi/apps/v1alpha1/tenant/types.goapi/apps/v1alpha1/vmdisk/types.goapi/apps/v1alpha1/vminstance/types.goapi/apps/v1alpha1/vpc/types.goapi/apps/v1alpha1/vpn/types.gopackages/apps/bucket/values.schema.jsonpackages/apps/clickhouse/values.schema.jsonpackages/apps/foundationdb/values.schema.jsonpackages/apps/harbor/values.schema.jsonpackages/apps/http-cache/values.schema.jsonpackages/apps/kafka/values.schema.jsonpackages/apps/mongodb/values.schema.jsonpackages/apps/nats/values.schema.jsonpackages/apps/openbao/values.schema.jsonpackages/apps/opensearch/values.schema.jsonpackages/apps/qdrant/values.schema.jsonpackages/apps/redis/values.schema.jsonpackages/apps/tcp-balancer/values.schema.jsonpackages/apps/tenant/values.schema.jsonpackages/apps/vm-disk/values.schema.jsonpackages/apps/vm-instance/values.schema.jsonpackages/apps/vpc/values.schema.jsonpackages/apps/vpn/values.schema.jsonpackages/system/clickhouse-rd/cozyrds/clickhouse.yamlpackages/system/harbor-rd/cozyrds/harbor.yamlpackages/system/harbor/templates/database.yamlpackages/system/http-cache-rd/cozyrds/http-cache.yamlpackages/system/kafka-rd/cozyrds/kafka.yamlpackages/system/keycloak/templates/db.yamlpackages/system/mongodb-rd/cozyrds/mongodb.yamlpackages/system/monitoring/templates/alerta/alerta-db.yamlpackages/system/monitoring/templates/grafana/db.yamlpackages/system/nats-rd/cozyrds/nats.yamlpackages/system/openbao-rd/cozyrds/openbao.yamlpackages/system/opensearch-rd/cozyrds/opensearch.yamlpackages/system/qdrant-rd/cozyrds/qdrant.yamlpackages/system/redis-rd/cozyrds/redis.yamlpackages/system/seaweedfs/templates/database.yamlpackages/system/tcp-balancer-rd/cozyrds/tcp-balancer.yamlpackages/system/tenant-rd/cozyrds/tenant.yamlpackages/system/virtualprivatecloud-rd/cozyrds/virtualprivatecloud.yamlpackages/system/vm-disk-rd/cozyrds/vm-disk.yamlpackages/system/vm-instance-rd/cozyrds/vm-instance.yamlpackages/system/vpn-rd/cozyrds/vpn.yaml
✅ Files skipped from review due to trivial changes (38)
- packages/apps/bucket/values.schema.json
- packages/system/harbor/templates/database.yaml
- packages/apps/nats/values.schema.json
- packages/system/monitoring/templates/alerta/alerta-db.yaml
- packages/apps/foundationdb/values.schema.json
- api/apps/v1alpha1/bucket/types.go
- packages/apps/tcp-balancer/values.schema.json
- packages/apps/openbao/values.schema.json
- packages/apps/vpc/values.schema.json
- packages/system/tenant-rd/cozyrds/tenant.yaml
- packages/apps/http-cache/values.schema.json
- packages/apps/vpn/values.schema.json
- packages/apps/qdrant/values.schema.json
- packages/system/vpn-rd/cozyrds/vpn.yaml
- packages/system/mongodb-rd/cozyrds/mongodb.yaml
- packages/system/redis-rd/cozyrds/redis.yaml
- packages/system/openbao-rd/cozyrds/openbao.yaml
- packages/system/seaweedfs/templates/database.yaml
- packages/system/opensearch-rd/cozyrds/opensearch.yaml
- packages/apps/tenant/values.schema.json
- packages/system/http-cache-rd/cozyrds/http-cache.yaml
- packages/apps/clickhouse/values.schema.json
- packages/system/qdrant-rd/cozyrds/qdrant.yaml
- packages/system/monitoring/templates/grafana/db.yaml
- packages/system/kafka-rd/cozyrds/kafka.yaml
- packages/apps/redis/values.schema.json
- packages/system/nats-rd/cozyrds/nats.yaml
- packages/system/vm-instance-rd/cozyrds/vm-instance.yaml
- packages/apps/opensearch/values.schema.json
- packages/apps/vm-instance/values.schema.json
- api/apps/v1alpha1/vpc/types.go
- packages/system/tcp-balancer-rd/cozyrds/tcp-balancer.yaml
- api/apps/v1alpha1/kafka/types.go
- api/apps/v1alpha1/opensearch/types.go
- packages/system/clickhouse-rd/cozyrds/clickhouse.yaml
- packages/system/virtualprivatecloud-rd/cozyrds/virtualprivatecloud.yaml
- api/apps/v1alpha1/harbor/types.go
- packages/system/keycloak/templates/db.yaml
| // Previous cluster name before deletion (matches serverName in backup.info). | ||
| // +kubebuilder:default:="" | ||
| OldName string `json:"oldName"` |
There was a problem hiding this comment.
Fix misleading OldName docstring.
At Line 91, the comment says old cluster name “matches serverName in backup.info”, but the whole feature exists because these can differ. Please reword to avoid operator confusion.
✏️ Suggested wording
- // Previous cluster name before deletion (matches serverName in backup.info).
+ // Previous Kubernetes cluster name used in backup path naming.
+ // This may differ from backup.info serverName.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Previous cluster name before deletion (matches serverName in backup.info). | |
| // +kubebuilder:default:="" | |
| OldName string `json:"oldName"` | |
| // Previous Kubernetes cluster name used in backup path naming. | |
| // This may differ from backup.info serverName. | |
| // +kubebuilder:default:="" | |
| OldName string `json:"oldName"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/v1alpha1/postgresql/types.go` around lines 91 - 93, The comment for
the OldName field is misleading; update the docstring for OldName in types.go
(the OldName string `json:"oldName"` field) to state that it is the previous
cluster name before deletion and that it may differ from the serverName recorded
in backup.info (do not claim they "match"); keep it concise and clarify its
purpose (e.g., to correlate backups when cluster names change).
| // List of allowed client networks. | ||
| // +kubebuilder:default:={} | ||
| HttpAndHttps HttpAndHttps `json:"httpAndHttps"` | ||
| Whitelist []string `json:"whitelist,omitempty"` |
There was a problem hiding this comment.
Revert omitempty on whitelist to avoid undefined ACL generation.
At Line 39, making whitelist optional can omit the key entirely. In packages/apps/tcp-balancer/templates/configmap.yaml (Lines 42-49), ACL whitelist is defined conditionally but referenced unconditionally, which can break rendered HAProxy config when whitelistHTTP=true and list is absent.
💡 Proposed fix
- Whitelist []string `json:"whitelist,omitempty"`
+ Whitelist []string `json:"whitelist"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Whitelist []string `json:"whitelist,omitempty"` | |
| Whitelist []string `json:"whitelist"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/apps/v1alpha1/tcpbalancer/types.go` at line 39, The Whitelist field on
the TCPBalancer spec is marked with `omitempty` which can remove the key and
cause undefined ACLs in the HAProxy template; remove `omitempty` from the struct
tag on the `Whitelist []string` field in the TCPBalancer types (the `Whitelist`
field in api/apps/v1alpha1/tcpbalancer/types.go) so the JSON always includes an
empty array when unset, ensuring the templates referencing `whitelist` always
receive a defined value.
| "upload": { | ||
| "description": "Upload local image.", | ||
| "type": "object" | ||
| "description": "Upload local image." |
There was a problem hiding this comment.
Restore the source.upload object type.
Unlike http and image, upload is now just a description node with no declared object type. That makes this branch effectively untyped in the generated schema and weakens validation for a user-facing input path. Please fix this in the source/generator and regenerate both schema artifacts.
Based on learnings: values.schema.json files in package directories are auto-generated from values.yaml by the cozyvalues-gen tool and regenerated on make generate, so manual edits here will be overwritten.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps/vm-disk/values.schema.json` around lines 41 - 42, The schema
lost the object type for the source.upload node so validation is weakened;
update the source generator or source values.yaml to declare source.upload as an
object (matching how source.http and source.image are defined), then run the
cozyvalues-gen regeneration (make generate) to emit updated values.schema.json
artifacts; specifically modify the values.yaml entry for source.upload to
include type: object (and any required properties) or update the cozyvalues-gen
mapping that builds source.upload before regenerating the schema files.
721ada5 to
0fcaef5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/system/postgres-rd/cozyrds/postgres.yaml (1)
11-11:⚠️ Potential issue | 🟡 MinorSame misleading
oldNamedescription in OpenAPI schema.The embedded JSON schema contains
"oldName":{"description":"Previous cluster name before deletion (matches serverName in backup.info)."...}which perpetuates the same misleading text. This should be updated to clarify thatoldNameis the K8s cluster name and may differ fromserverNamein backup.info.The
serverNamefield definition and its description are correct.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/postgres-rd/cozyrds/postgres.yaml` at line 11, Update the OpenAPI description for the bootstrap.oldName property: change the misleading text that says it "matches serverName in backup.info" to clearly state that oldName is the Kubernetes cluster name from the previous cluster and may differ from bootstrap.serverName (which is the Barman/server name stored in backup.info); edit the "oldName" description in the bootstrap properties to reflect this distinction.api/apps/v1alpha1/postgresql/types.go (1)
89-91:⚠️ Potential issue | 🟡 Minor
OldNamedocstring is still misleading.The comment on line 89 states
(matches serverName in backup.info), but the entire purpose of this PR is to handle cases where they differ. This contradicts the PR's intent and will confuse operators. The past review flagged this exact issue but it remains unaddressed.Since this is an auto-generated file, please update the source (likely in
values.yamlor schema definition) and regenerate.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/v1alpha1/postgresql/types.go` around lines 89 - 91, Update the misleading docstring for the OldName field: change the comment to state that OldName is the previous cluster name before deletion and may differ from serverName in backup.info (i.e., it records the original name used prior to deletion/restore). Make this change in the source schema (e.g., the values.yaml or the CRD/schema definition that generates api/apps/v1alpha1/postgresql/types.go) rather than editing the generated file directly, then regenerate the API/types.go to apply the corrected comment for the OldName field.packages/apps/postgres/values.yaml (1)
156-157:⚠️ Potential issue | 🟡 Minor
oldNamedescription remains misleading despite being marked as addressed.Line 156 still contains
(matches serverName in backup.info)which contradicts the purpose of the newserverNamefield. The past review flagged this and it was marked as addressed in commit 0fcaef5, but the text is unchanged.Suggested fix
-## `@field` {string} oldName - Previous cluster name before deletion (matches serverName in backup.info). +## `@field` {string} oldName - Previous Kubernetes cluster name before deletion. May differ from serverName in backup.info.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/postgres/values.yaml` around lines 156 - 157, The current description for the values.yaml field oldName is still incorrect — it claims "(matches serverName in backup.info)" which conflicts with the new serverName field; update the oldName description to reflect that it is the previous Kubernetes cluster/cluster resource name (e.g., "Previous Kubernetes cluster name before deletion") and remove the parenthetical about backup.info, and ensure the serverName field description remains the place to document the Barman server name from backup.info; change only the text for oldName in values.yaml (referencing the oldName and serverName fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/apps/v1alpha1/postgresql/types.go`:
- Around line 89-91: Update the misleading docstring for the OldName field:
change the comment to state that OldName is the previous cluster name before
deletion and may differ from serverName in backup.info (i.e., it records the
original name used prior to deletion/restore). Make this change in the source
schema (e.g., the values.yaml or the CRD/schema definition that generates
api/apps/v1alpha1/postgresql/types.go) rather than editing the generated file
directly, then regenerate the API/types.go to apply the corrected comment for
the OldName field.
In `@packages/apps/postgres/values.yaml`:
- Around line 156-157: The current description for the values.yaml field oldName
is still incorrect — it claims "(matches serverName in backup.info)" which
conflicts with the new serverName field; update the oldName description to
reflect that it is the previous Kubernetes cluster/cluster resource name (e.g.,
"Previous Kubernetes cluster name before deletion") and remove the parenthetical
about backup.info, and ensure the serverName field description remains the place
to document the Barman server name from backup.info; change only the text for
oldName in values.yaml (referencing the oldName and serverName fields).
In `@packages/system/postgres-rd/cozyrds/postgres.yaml`:
- Line 11: Update the OpenAPI description for the bootstrap.oldName property:
change the misleading text that says it "matches serverName in backup.info" to
clearly state that oldName is the Kubernetes cluster name from the previous
cluster and may differ from bootstrap.serverName (which is the Barman/server
name stored in backup.info); edit the "oldName" description in the bootstrap
properties to reflect this distinction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da8ec22b-aed1-4db8-915b-92a7bcde7693
📒 Files selected for processing (6)
api/apps/v1alpha1/postgresql/types.gopackages/apps/postgres/README.mdpackages/apps/postgres/templates/db.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/system/postgres-rd/cozyrds/postgres.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/postgres/templates/db.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/postgres/README.md
- packages/apps/postgres/values.schema.json
0fcaef5 to
8b3e346
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/apps/v1alpha1/postgresql/types.go (1)
89-97:⚠️ Potential issue | 🟡 MinorCorrect
OldNamesemantics to avoid misconfiguration.At Line 89, describing
oldNameas matchingbackup.info.serverNameconflicts with the newly addedserverNamefield.oldNameshould represent the previous Kubernetes cluster name/path identifier, whileserverNameis the Barman value frombackup.info.✏️ Proposed wording fix
- // Previous cluster name before deletion (matches serverName in backup.info). + // Previous Kubernetes cluster name used for backup path naming. + // This may differ from backup.info serverName.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/apps/v1alpha1/postgresql/types.go` around lines 89 - 97, The comment for OldName is incorrect and must be rewritten to state that OldName is the previous Kubernetes cluster name/path identifier (the cluster's prior k8s resource name), not the Barman backup.info.serverName; update the OldName field comment on the Postgresql spec to reflect that semantic, and ensure the ServerName field comment (ServerName) clearly states it is the Barman server name taken from backup.info for use when the original cluster used a different serverName than its Kubernetes cluster name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/apps/v1alpha1/postgresql/types.go`:
- Around line 89-97: The comment for OldName is incorrect and must be rewritten
to state that OldName is the previous Kubernetes cluster name/path identifier
(the cluster's prior k8s resource name), not the Barman backup.info.serverName;
update the OldName field comment on the Postgresql spec to reflect that
semantic, and ensure the ServerName field comment (ServerName) clearly states it
is the Barman server name taken from backup.info for use when the original
cluster used a different serverName than its Kubernetes cluster name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3564c423-b0ce-4f58-8a8a-a2285dfd020e
📒 Files selected for processing (6)
api/apps/v1alpha1/postgresql/types.gopackages/apps/postgres/README.mdpackages/apps/postgres/templates/db.yamlpackages/apps/postgres/values.schema.jsonpackages/apps/postgres/values.yamlpackages/system/postgres-rd/cozyrds/postgres.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/postgres/templates/db.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/apps/postgres/values.yaml
- packages/apps/postgres/README.md
There was a problem hiding this comment.
Review + E2E Test Results
E2E Testing
Tested on a dev cluster:
Positive test (PASS): Deployed a postgres cluster with backups, inserted test data (3 rows), triggered backup. Then deployed a new cluster with oldName=pg-source (intentionally mismatched) and serverName=pg-test-restore (the actual S3 path). Recovery completed successfully — all 3 rows present. The serverName parameter correctly directs CNPG to the right S3 path.
Negative test (PASS): Same setup but without serverName. Recovery pods crash-loop with "no target backup found" — confirming this PR fixes a real issue.
Code Review
Template, schema, CRD — all correct. The conditional in db.yaml works as expected, serverName is correctly not in the required array, keysOrder is updated, all 6 files are consistent. See inline comments for the one description issue found.
| ## @field {bool} enabled - Whether to restore from a backup. | ||
| ## @field {string} [recoveryTime] - Timestamp (RFC3339) for point-in-time recovery; empty means latest. | ||
| ## @field {string} oldName - Previous cluster name before deletion. | ||
| ## @field {string} oldName - Previous cluster name before deletion (matches serverName in backup.info). |
There was a problem hiding this comment.
This parenthetical is misleading. oldName maps to externalClusters[].name — a local reference label that wires bootstrap.recovery.source to the external cluster entry. It does not necessarily match backup.info's server_name.
The whole point of this PR is to handle the case where they don't match, so stating they match here contradicts the PR's own purpose.
Suggestion: revert to the original description:
## @field {string} oldName - Previous cluster name before deletion.
Then re-run make generate to propagate.
| ## @field {string} [recoveryTime] - Timestamp (RFC3339) for point-in-time recovery; empty means latest. | ||
| ## @field {string} oldName - Previous cluster name before deletion. | ||
| ## @field {string} oldName - Previous cluster name before deletion (matches serverName in backup.info). | ||
| ## @field {string} [serverName] - Barman server name from the old cluster's backup.info. Use when the original cluster used a different serverName than its Kubernetes cluster name. |
There was a problem hiding this comment.
The description references backup.info which implies users should open that file and copy a value. Per CNPG docs, barmanObjectStore.serverName is the S3 path prefix barman uses — it defaults to the cluster's metadata.name.
Also "Kubernetes cluster name" is ambiguous (K8s resource name vs. the K8s cluster itself).
Suggested rewording:
## @field {string} [serverName] - Barman server name (S3 path prefix) used by the original cluster when writing backups. Set this only when the original cluster had an explicit barmanObjectStore.serverName that differed from its Kubernetes resource name.
| barmanObjectStore: | ||
| destinationPath: {{ .Values.backup.destinationPath }} | ||
| {{- if .Values.bootstrap.serverName }} | ||
| serverName: {{ .Values.bootstrap.serverName }} |
There was a problem hiding this comment.
Looks good — the {{- if }} guard correctly omits the field when empty, and the indentation is correct within barmanObjectStore. Verified with helm template rendering and E2E test.
Add serverName field to bootstrap configuration to explicitly specify Barman server name from backup.info. This fixes "no target backup found" errors when server_name in backup.info differs from Kubernetes cluster name. Signed-off-by: IvanHunters <[email protected]>
2de16de to
82afa31
Compare
82afa31 to
91358c2
Compare
8ec5c4b to
af988b5
Compare
af988b5 to
be0e328
Compare
Update oldName and serverName field descriptions based on code review feedback to avoid confusion about their actual roles: - oldName: Remove misleading "(matches serverName in backup.info)" text. This field represents the Kubernetes cluster resource name, not the Barman server name. - serverName: Provide clearer explanation that it's the S3 path prefix (barmanObjectStore.serverName) used by the original cluster, and should only be set when it differs from the Kubernetes resource name. Updated in: - values.yaml (source of truth for field documentation) - types.go (Go API type comments) - values.schema.json (JSON schema for validation) - postgres.yaml (CRD with embedded OpenAPI schema) Signed-off-by: IvanHunters <[email protected]>
be0e328 to
9f41dc3
Compare
| *out = new(v1.TypedLocalObjectReference) | ||
| (*in).DeepCopyInto(*out) | ||
| } | ||
| if in.Options != nil { |
There was a problem hiding this comment.
Lets remove this change for clear git history, I will bring it back with new PR and fixed precommit checks to always run make generate.
Summary
Add
serverNamefield to bootstrap configuration to support backup recovery when the Barman server name inbackup.infodiffers from the Kubernetes cluster name.This fixes "no target backup found" errors during CloudNativePG recovery operations.
Problem
CloudNativePG forms the backup path as
destinationPath + "postgres-" + oldName, but searches for backups using theserver_namefield frombackup.info. When these values don't match (e.g., cluster name isgrafanabutserver_nameiscloud), recovery fails with "no target backup found".Solution
serverNameparameter tobootstrapconfigurationoldNamewhenserverNameis not provided (backwards compatible)Changes
ServerNamefield to PostgreSQL CRD type definitionserverNameto ClusterexternalClusterstemplatevalues.yamland README.md withserverNamedocumentationvalues.schema.jsonandpostgres.yamlCRDTest plan
bootstrap.enabled=true,bootstrap.oldName=<original-name>, andbootstrap.serverName=<value-from-backup.info>Summary by CodeRabbit
New Features
Documentation