K8SPG-1057: Allow using etcd as patroni DCS - #1647
Conversation
|
|
|
I will wait for the jira ticket to open the PR in the helm charts repo |
|
and another note - this is my first OSS contribute so be gentel 😄 |
egegunes
left a comment
egegunes
left a comment
There was a problem hiding this comment.
@yoav-katz the implementation looks good to me in general. but we definitely need an e2e test that deploys etcd and configures PerconaPGCluster to use it.
|
would love to see this merged! |
…erator into etcd-dcs
|
@egegunes Im not sure if the tests are failuing because a change I did. |
|
@yoav-katz I reran the failing tests, let's wait for their results to see if they're broken. For the linter, please add |
|
Hey @yoav-katz please resolve the conflicts and the linting errors |
|
@mayankshah1607 conflict resolved, thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 49 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
internal/patroni/config.go:219
-
- Problem: The closing braces in this map literal are misindented, indicating the surrounding block wasn’t
gofmt’d.
- Problem: The closing braces in this map literal are misindented, indicating the surrounding block wasn’t
- Why it matters: Consistent formatting keeps the generated YAML logic easy to read/maintain and avoids CI formatting failures.
- Fix: Align the braces/indentation with
gofmtoutput.
pkg/apis/upstream.pgv2.percona.com/v1beta1/patroni_types.go:106
-
- Problem: The immutability CEL rule allows changing the DCS type when the existing object has no stored
typefield (e.g., clusters created before this field existed), because!has(oldSelf.type)short-circuits the check.
- Problem: The immutability CEL rule allows changing the DCS type when the existing object has no stored
- Why it matters: This permits switching an existing cluster from the implicit Kubernetes DCS to etcd after creation, which contradicts the stated “immutable after cluster creation” contract and could lead to unsafe/unsupported transitions.
- Fix: Treat a missing
oldSelf.typeas the default (kubernetes) and only allowself.typeto remainkubernetesin that case.
internal/patroni/config.go:214
-
- Problem: This block has stray whitespace and incorrect indentation (spaces instead of tabs), so the file is not
gofmt-formatted.
- Problem: This block has stray whitespace and incorrect indentation (spaces instead of tabs), so the file is not
- Why it matters: The repository’s Go code is expected to be gofmt’d; leaving formatting drift makes reviews harder and can fail formatting/lint checks.
- Fix: Reformat this conditional with
gofmt(remove the whitespace-only line and align braces/tabs).
This issue also appears on line 215 of the same file.
internal/controller/postgrescluster/patroni.go:78
-
- Problem: Under external DCS, errors querying Patroni’s REST API for
pending_restartare silently swallowed (err == nil && restart).
- Problem: Under external DCS, errors querying Patroni’s REST API for
- Why it matters: If the REST call fails (TLS/SAN mismatch, transient network issue, curl missing, etc.), the operator will skip required restarts without any log signal, making issues hard to diagnose.
- Fix: Log the error (and keep returning false) so failures are visible in operator logs.
|
fixed all problems co-pilot suggested:
|
commit: b46ab19 |
|
Hey @yoav-katz ! Firstly, thank you for all your work on this PR so far, and sorry for the delay with getting this reviewed. This is a big change that touches several parts of the codebase so we want to be careful that the design is right moving forward. While discussing this internally, we wondered whether we could improve the code organization a bit. Most of the operator was originally written with Kubernetes as the only DCS backend in mind, so a lot of the current logic is tightly coupled to that assumption. As a result we now have this One possible direction would be to introduce an interface that abstracts the DCS, for example type DCSBackend interface {
GetClusterConfig(cluster *v1beta.PostgresCluster) (map[string]string, error)
GetInstanceConfig(cluster *v1beta.PostgresCluster) (map[string]string, error)
GetSystemIdentifier(cluster *v1beta.PostgresCluster) (string, error)
CheckIsPendingRestart(cluster *v1beta.PostgresCluster) (bool, error)
ConfigureInstancePodEnv(cluster *v1beta.PostgresCluster, podTpl *corev1.PodTemplateCheckIsPendingRestart) (bool, error)
GetPermissions(cluster *v1beta.PostgresCluster) ([]rbacv1.PolicyRule, error)
// .. not a complete list, maybe we need more methods
}The idea here is that each backend implements this interface, the implementation is initialised once at the start of Reconcile, and each of the sub-reconcile functions would not need to know which backend is being used, and be left completely untouched. What do you feel about such an approach? We are also open to other ideas or improve on top of this if you see a better way to structure it. 😄 |
|
We were discussing this PR internally and were concerned about the upcoming maintenance cost after merging this PR. What Mayank proposed above would make this much easier to reason about and maintain, also potentially allow adding more backends in the future. This is a big change request, I understand if you're not willing to do it. If that's the case, we'll take this as a feature request and implement it in the upcoming releases. If you're willing to do it, we'll assist you all the way of course, but I don't think this will make it into v3.1.0. |
|
Ha, yeah -for what it's worth, I did consider a DCSBackend-style abstraction early on, and held off mainly to keep the diff reviewable. But I agree it's the right long-term shape, so I'll fold it into this PR rather than punt it. |
|
@egegunes |
@yoav-katz a design doc would be great to prevent back and forth and wasting your efforts |
|
@mayankshah1607 @egegunes I would like your feedback about this implementation - ProposalIntroduce a new package: This package becomes the single place that knows about specific DCS implementations. It exposes a backend interface with two implementations: The backend is selected once per reconcile: backend := dcs.For(cluster)After that, callers interact only with backend capabilities and do not know which DCS is active. Examples of backend-owned behavior:
Design rules
Import directionThe dependency direction remains:
The controller wires the two together by passing backend-provided data into existing Patroni helpers. The rule of thumb:
|
|
@yoav-katz The design looks good to me, pretty much also what I had in mind. Lets go ahead and create a separate draft PR. Since these changes will be huge, I propose we do it in 2 stages:
|
|
Just double checking I understood correctly - you want me to draft this PR and create two other PR stacked on each other |
|
Yes correct, in the first PR lets add the interface and refactor the existing code behind a |
CHANGE DESCRIPTION
Problem:
Patroni supports multiple DCS backends, but the operator hardcodes Kubernetes Endpoints as the only option. This blocks clusters on managed Kubernetes platforms where workloads cannot reach the control plane API.
Cause:
The kubernetes: stanza was hardcoded in the generated Patroni config with no mechanism to select a different backend.
Several other pieces of the operator also assumed k8s DCS: RBAC rules unconditionally granted Endpoints permissions, the primary service routed through Patroni-managed Endpoints objects, and pod role labels/annotations were expected to be set by Patroni itself (which only happens with k8s DCS).
Solution:
Add a spec.patroni.dcs field (type: kubernetes default, type: etcd alternative). The field is immutable after cluster creation, enforced by a CEL validation rule on the CRD.
When type: etcd, the operator:
Reconciliation model (event-driven → periodic):
With k8s DCS the operator is event-driven: Patroni writes leader election, bootstrap (initialize), and pending-restart state into Endpoints objects and pod annotations, and the operator's watches turn those writes into reconciles. Under etcd DCS none of those k8s writes happen, so those watch events never fire. The operator compensates in three ways:
In-place restore DCS cleanup:
An in-place restore must clear stale Patroni state before re-bootstrapping. Under k8s DCS this is done by deleting the Patroni Endpoints objects. Those don't exist under etcd, so instead a one-shot Job runs patronictl remove against etcd to clear the leader lock, member keys, and initialize key. It runs only after Patroni is confirmed stopped (so no live process re-registers itself), and the operator gates the restore state machine on the Job (create → wait → retry-on-failure). The Job reuses the restore's affinity, tolerations, resources, and priorityClassName.
The Kubernetes DCS path is unchanged.
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
Config/Logging/Testability