K8SPG-1114: Automate deploying logical replicas - #1747
Open
egegunes wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds automated PostgreSQL 17 logical replicas, including bootstrap, lifecycle management, restore safety, configuration, and testing.
Changes:
- Adds logical-replica APIs, reconciliation, status, RBAC, and generated manifests.
- Supports pgBackRest and
pg_basebackupseeding. - Adds unit, envtest, and end-to-end coverage.
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adjusts generation order. |
build/crd/percona/generated/pgv2.percona.com_perconapgclusters.yaml |
Adds generated CRD schema. |
config/crd/bases/pgv2.percona.com_perconapgclusters.yaml |
Adds Percona API schema. |
config/crd/bases/upstream.pgv2.percona.com_postgresclusters.yaml |
Adds upstream projection schema. |
config/rbac/cluster/role.yaml |
Updates cluster RBAC. |
config/rbac/namespace/role.yaml |
Updates namespace RBAC. |
deploy/bundle.yaml |
Regenerates deployment bundle. |
deploy/cr.yaml |
Adds configuration examples. |
deploy/crd.yaml |
Regenerates deployed CRDs. |
deploy/cw-bundle.yaml |
Regenerates CW bundle. |
deploy/cw-rbac.yaml |
Updates CW RBAC. |
deploy/rbac.yaml |
Updates deployed RBAC. |
e2e-tests/run-minikube.csv |
Enables the E2E suite. |
e2e-tests/run-pr.csv |
Enables PR testing. |
e2e-tests/run-release.csv |
Enables release testing. |
e2e-tests/tests/logical-replica/00-assert.yaml |
Verifies operator deployment. |
e2e-tests/tests/logical-replica/00-deploy-operator.yaml |
Deploys test prerequisites. |
e2e-tests/tests/logical-replica/01-assert.yaml |
Waits for cluster readiness. |
e2e-tests/tests/logical-replica/01-create-cluster.yaml |
Creates the test cluster. |
e2e-tests/tests/logical-replica/02-write-data.yaml |
Seeds source data. |
e2e-tests/tests/logical-replica/03-add-logical-replica.yaml |
Creates both replica types. |
e2e-tests/tests/logical-replica/03-assert.yaml |
Verifies replica readiness. |
e2e-tests/tests/logical-replica/04-verify-seeded-data.yaml |
Verifies physical seeding. |
e2e-tests/tests/logical-replica/05-stream-new-data.yaml |
Verifies logical replication. |
e2e-tests/tests/logical-replica/06-slot-survives-patroni.yaml |
Tests slot preservation. |
e2e-tests/tests/logical-replica/07-replica-is-read-only.yaml |
Tests read-only behavior. |
e2e-tests/tests/logical-replica/08-assert.yaml |
Verifies backup completion. |
e2e-tests/tests/logical-replica/08-create-backup.yaml |
Creates a full backup. |
e2e-tests/tests/logical-replica/09-write-after-backup.yaml |
Adds post-backup data. |
e2e-tests/tests/logical-replica/10-restore.yaml |
Starts an in-place restore. |
e2e-tests/tests/logical-replica/11-assert.yaml |
Verifies invalidation after restore. |
e2e-tests/tests/logical-replica/12-remove-logical-replica.yaml |
Tests cleanup. |
e2e-tests/tests/logical-replica/13-assert.yaml |
Verifies reseeding. |
e2e-tests/tests/logical-replica/13-reseed-logical-replica.yaml |
Recreates a replica. |
e2e-tests/tests/logical-replica/14-missing-database.yaml |
Tests missing databases. |
e2e-tests/tests/logical-replica/99-remove-cluster-gracefully.yaml |
Cleans up the suite. |
e2e-tests/tests/logical-replica/conf/logical-replica.yaml |
Defines test configuration. |
internal/controller/postgrescluster/controller.go |
Adds logical-replication HBA rules. |
internal/logicalreplica/postgres.go |
Implements PostgreSQL helpers. |
internal/logicalreplica/postgres_test.go |
Tests PostgreSQL helpers. |
internal/patroni/config.go |
Preserves logical slots. |
internal/patroni/config_test.go |
Tests Patroni configuration. |
internal/pgbackrest/reconcile.go |
Exposes the config mount helper. |
percona/controller/pgcluster/controller.go |
Registers watches, RBAC, and reconciliation. |
percona/controller/pgcluster/logicalreplica.go |
Implements replica lifecycle management. |
percona/controller/pgcluster/logicalreplica_envtest_test.go |
Tests API validation and status. |
percona/controller/pgcluster/logicalreplica_restore.go |
Handles source restores. |
percona/controller/pgcluster/logicalreplica_restore_test.go |
Tests restore handling. |
percona/controller/pgcluster/logicalreplica_test.go |
Tests replica reconciliation. |
percona/controller/pgcluster/status.go |
Owns the readiness condition. |
percona/controller/pgcluster/status_test.go |
Tests condition ownership. |
percona/naming/conditions.go |
Defines the readiness condition. |
percona/naming/label.go |
Defines replica labels. |
pkg/apis/pgv2.percona.com/v2/logicalreplica_types_test.go |
Tests API helpers and projection. |
pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go |
Adds public API and status types. |
pkg/apis/pgv2.percona.com/v2/zz_generated.deepcopy.go |
Adds generated deep copies. |
pkg/apis/upstream.pgv2.percona.com/v1beta1/postgrescluster_types.go |
Adds upstream replica projection. |
pkg/apis/upstream.pgv2.percona.com/v1beta1/zz_generated.deepcopy.go |
Adds generated deep copies. |
Suppressed comments (3)
pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go:696
-
- Problem: This projects logical replicas even when
LogicalReplicasEnabled()is false forcrVersion < 3.1.0. 2. Why it matters: The upstream layer then renders HBA and Patroni configuration for a feature that the Percona reconciler and replication-user injection deliberately disable. 3. Fix: Gate this projection withLogicalReplicasEnabled()and clear the upstream field otherwise.
percona/controller/pgcluster/logicalreplica.go:1323
- Problem: This projects logical replicas even when
-
- Problem: User metadata is merged last, allowing it to overwrite the cluster and logical-replica labels used by the immutable StatefulSet selector and Service/pod lookup. 2. Why it matters: A valid metadata label can leave the StatefulSet pod unselected and make the replica permanently unavailable. 3. Fix: Merge user labels first and operator-owned labels last, matching the established instance-label pattern.
percona/controller/pgcluster/logicalreplica.go:1397
- Problem: User metadata is merged last, allowing it to overwrite the cluster and logical-replica labels used by the immutable StatefulSet selector and Service/pod lookup. 2. Why it matters: A valid metadata label can leave the StatefulSet pod unselected and make the replica permanently unavailable. 3. Fix: Merge user labels first and operator-owned labels last, matching the established instance-label pattern.
-
- Problem:
Exposedoes not applyNodePortorLoadBalancerClass, and removingExposedoes not reset an existing Service's type, source ranges, or annotations. 2. Why it matters: Requested exposure settings are silently ignored, and removing external exposure can leave the logical replica publicly reachable. 3. Fix: Reconcile everyServiceExposefield into a fresh desired Service state (includingServicePort.NodePortandSpec.LoadBalancerClass) and explicitly restore ClusterIP/default metadata whenExposeis nil.
- Problem:
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+84
to
+90
| switch pgRestore.Status.State { | ||
| case v2.RestoreRunning: | ||
| restore.InFlight = true | ||
| restore.DataReplaced = true | ||
| case v2.RestoreStarting: | ||
| restore.InFlight = true | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I don't think this is possible, we set DataReplaced=true as soon as crunchy cr has postgrescluster.ConditionPGBackRestRestoreProgressing condition which is when instances are terminated
Comment on lines
+650
to
+653
| if user.Name == UserLogicalReplication { | ||
| log.Info(UserLogicalReplication + " user is reserved, it'll be ignored.") | ||
| continue | ||
| } |
Contributor
Author
There was a problem hiding this comment.
i think we need to disallow this user even when logical replication is disabled because it might be enabled in any time during cluster lifecycle. it's a low probability for a cluster to have this user with exact same username and also even if it's ignored it's not dropped from the database
Member
There was a problem hiding this comment.
I think it might still be worth addressing this, although unrelated to your PR:
when PMM is enabled line 658 rebuilds users from cr.Spec.Users
Collaborator
commit: 7a4b4d3 |
egegunes
marked this pull request as ready for review
August 13, 2026 06:15
egegunes
requested review from
DhruthiKV,
eleo007,
gkech,
hors,
jvpasinatto,
mayankshah1607,
nmarukovich,
oksana-grishchenko,
pooknull and
valmiranogueira
as code owners
August 13, 2026 06:15
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 58 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
percona/controller/pgcluster/logicalreplica.go:199
-
- Problem: A
readyreplica is treated as settled, so reconciliation stops polling it; this controller does not watch replica Pods/StatefulSets, and an apply-worker failure does not change either object anyway. 2. Why it matters:status.logicalReplicascan remainreadyindefinitely after replication stops, contrary to the status contract. 3. Fix: Keep a lower-frequency health-check requeue for active replicas (separate from the 5-second bootstrap retry), or add equivalent periodic monitoring.
percona/controller/pgcluster/logicalreplica.go:1144
- Problem: A
-
- Problem: The logical replica serves the cluster TLS certificate, but that certificate is generated only for the primary and replica Service DNS names (
internal/controller/postgrescluster/pki.go:402-413), not the new logical-replica Service name. 2. Why it matters: Clients connecting to this Service with hostname verification (sslmode=verify-full) will reject the certificate. 3. Fix: Add logical-replica Service DNS names to managed certificates, or generate a certificate per logical replica; document the required SANs for user-provided certificates.
percona/controller/pgcluster/logicalreplica.go:1352
- Problem: The logical replica serves the cluster TLS certificate, but that certificate is generated only for the primary and replica Service DNS names (
-
- Problem:
ServiceExpose.NodePortandLoadBalancerClassare never copied to the Service, and removingexposeleaves the previously fetched Service type/ranges/annotations intact. 2. Why it matters: Valid API settings are silently ignored and exposure cannot be reliably reverted. 3. Fix: Reconcile allServiceExposefields and explicitly restore ClusterIP/default metadata when exposure is removed, accounting for Kubernetes Service field immutability.
percona/controller/pgcluster/logicalreplica.go:679
- Problem:
-
- Problem: Capacity is checked independently immediately before each Job, so multiple replicas created in one reconcile can all observe the same free slots/senders before any Job consumes them. 2. Why it matters: The aggregate demand can exceed PostgreSQL limits after every check passed, causing a one-shot bootstrap to fail and require a full reseed. 3. Fix: Reserve/check aggregate capacity before launching Jobs, or serialize logical-replica bootstraps.
Comment on lines
+1281
to
+1282
| sts.Spec.Template.Labels = naming.Merge( | ||
| logicalReplicaLabels(cr, spec.Name), spec.Metadata.GetLabelsOrNil()) |
| LogicalReplicas LogicalReplicas `json:"logicalReplicas,omitempty"` | ||
| } | ||
|
|
||
| type LogicalReplicas []LogicalReplicaSpec |
Member
There was a problem hiding this comment.
This will enforce uniqueness without additional code
Suggested change
| type LogicalReplicas []LogicalReplicaSpec | |
| // +listType=map | |
| // +listMapKey=name | |
| type LogicalReplicas []LogicalReplicaSpec |
| // Databases to replicate. When empty, every database in the cluster except | ||
| // the templates and "postgres" is replicated. | ||
| // +optional | ||
| Databases []crunchyv1beta1.PostgresIdentifier `json:"databases,omitempty"` |
Member
There was a problem hiding this comment.
This will enforce uniqueness without additional code
Suggested change
| Databases []crunchyv1beta1.PostgresIdentifier `json:"databases,omitempty"` | |
| // +listType=set | |
| Databases []crunchyv1beta1.PostgresIdentifier `json:"databases,omitempty"` |
Comment on lines
+499
to
+500
| // ValidateLogicalReplicas checks the invariants of spec.logicalReplicas that | ||
| // cannot be expressed with kubebuilder markers. |
Member
There was a problem hiding this comment.
I already left 2 comments above for how we can enforce uniqueness for logical replica name and database name. The instanceName/logicalReplicaName collision can also be handled with CEL:
you will have to set something like this on PerconaPGCluster:
// +kubebuilder:validation:XValidation:rule="!has(self.logicalReplicas) || self.logicalReplicas.all(r, !(r.name in self.instances.map(i, i.name)))",message="spec.logicalReplicas names must not collide with spec.instances names"but you will get the estimated rule cost exceeds budget by factor of more than 100x error, so you need to set +kubebuilder:validation:MaxItems=N on PGInstanceSets and LogicalReplicas type.
Then we don't need this function at all
|
|
||
| // checkPrimaryCapacity verifies the primary can host the replication slots and | ||
| // WAL senders this replica needs. | ||
| func (r *PGClusterReconciler) checkPrimaryCapacity( |
Member
There was a problem hiding this comment.
In case we have multiple logical replicas, do we need to account for pending bootstrap jobs which once completed will also consume the capacity? Or maybe start logical replicas one by one
oksana-grishchenko
approved these changes
Aug 13, 2026
oksana-grishchenko
left a comment
oksana-grishchenko
left a comment
Contributor
There was a problem hiding this comment.
Overall looks good to me, it's a great work.
Few non-blocking suggestions
Comment on lines
+115
to
+117
| if cr.CompareVersion("3.1.0") < 0 { | ||
| return false, nil | ||
| } |
Contributor
There was a problem hiding this comment.
what happens if someone updated the operator but didn't bump the CR version? I know it's their problems and they should have the CR version updated, but we could make it a bit easier to understand the issue - if the version is less than 3.1.0 but there are cr.Spec.LogicalReplicas configured, we could have a log saying that in order to use the feature they have to have crVersion >= 3.1.0
Comment on lines
+381
to
+382
| # databases: | ||
| # - cluster1 |
Contributor
There was a problem hiding this comment.
I'm not sure which case appears more often - to replicate a specific DB or to replicate them all. If replicating all appears more often then instead of a specific meaningless name we could have
# databases: [] # empty = all non-template databases except "postgres"
this will also document the behaviour nicely.
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
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.
CHANGE DESCRIPTION
This PR adds support for deploying logical replicas.
pg_createsubscriber, so this feature is PG17+ only.logicalreplwith optionsSUPERUSER REPLICATIONif a logical replica is defined in cr.yaml.logicalreplis now a reserved user name.use_slotsis false by default but now it's overridable, user can define slots in patroni configuration to ensure they survive failover. If logical replication is broken due to failover, it's reflected in the status.status.logicalReplicas[].state: bootstrapping|ready|broken|suspended, plus reason, message, databases, seededAt, invalidatedAt.CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability