Skip to content

K8SPG-1057: Allow using etcd as patroni DCS#1647

Open
yoav-katz wants to merge 38 commits into
percona:mainfrom
yoav-katz:etcd-dcs
Open

K8SPG-1057: Allow using etcd as patroni DCS#1647
yoav-katz wants to merge 38 commits into
percona:mainfrom
yoav-katz:etcd-dcs

Conversation

@yoav-katz

@yoav-katz yoav-katz commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

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:

  • Emits an etcd3: stanza in the generated Patroni config instead of kubernetes:, with optional TLS (cacert/cert/key) and auth credentials (PATRONI_ETCD3_USERNAME/PATRONI_ETCD3_PASSWORD) sourced from referenced Secrets.
  • Injects on_start and on_role_change Patroni callbacks pointing to a new patroni-role-change.sh script. Since Patroni does not set pod role labels or the status annotation when using etcd DCS, this script patches the pod via the k8s API on every role transition, restoring the label (role=primary|replica) and annotation ({"role":"primary"}) that the rest of the operator depends on for Service routing and primary detection.
  • Creates the primary Service with a label selector (role=primary) instead of the previous headless-Endpoints-to-Patroni-leader-ClusterIP indirection, which only works with k8s DCS.
  • Skips creating the Patroni leader lease Service and distributed configuration Service, which are k8s DCS artifacts.
  • Omits the Endpoints RBAC permissions from the postgres pod ServiceAccount, since they are not needed.
  • Validates that referenced TLS and auth Secrets exist and contain the required keys, surfacing issues as Warning events on the cluster.

The Kubernetes DCS path is unchanged.

CHECKLIST

Jira

  • Is the Jira ticket created and referenced properly?
  • Does the Jira ticket have the proper statuses for documentation (Needs Doc) and QA (Needs QA)?
  • Does the Jira ticket link to the proper milestone (Fix Version field)?

Tests

  • Is an E2E test/test case added for the new feature/change?
  • Are unit tests added where appropriate?

Config/Logging/Testability

  • Are all needed new/changed options added to default YAML files?
  • Helm Chart Merge Request
  • Did we add proper logging messages for operator actions?
  • Did we ensure compatibility with the previous version or cluster upgrade process?
  • Does the change support oldest and newest supported PG version?
  • Does the change support oldest and newest supported Kubernetes version?

@it-percona-cla

it-percona-cla commented Jun 18, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@yoav-katz
yoav-katz marked this pull request as draft June 18, 2026 22:26
@yoav-katz

Copy link
Copy Markdown
Contributor Author

I will wait for the jira ticket to open the PR in the helm charts repo

@yoav-katz

Copy link
Copy Markdown
Contributor Author

and another note - this is my first OSS contribute so be gentel 😄
if there is stuff that you think should be changed becuase of style/dependency consideration I will be happy to fix!

@egegunes egegunes changed the title feat(etcd) K8SPG-1057: Allow using etcd as patroni DCS Jun 19, 2026

@egegunes egegunes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@DanBrima

Copy link
Copy Markdown

would love to see this merged!

Copilot AI review requested due to automatic review settings June 25, 2026 16:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 41 out of 42 changed files in this pull request and generated 2 comments.

Comment thread internal/patroni/reconcile.go Outdated
Comment thread internal/controller/postgrescluster/patroni.go Outdated
@egegunes egegunes added this to the v3.1.0 milestone Jun 26, 2026
egegunes
egegunes previously approved these changes Jun 26, 2026
@yoav-katz

Copy link
Copy Markdown
Contributor Author

@egegunes Im not sure what caused this failure, my commit only touched unit test files so lint will pass😅
Should I open an issue?
Is there a way to retrigger only this specific test?

@egegunes

egegunes commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

It seems to fail on cluster deletion due to timeout. I doubt it's something caused by these changes. I retriggered it.

Comment thread pkg/apis/upstream.pgv2.percona.com/v1beta1/patroni_types.go Outdated
Comment thread pkg/apis/upstream.pgv2.percona.com/v1beta1/patroni_types.go Outdated
Comment thread pkg/apis/upstream.pgv2.percona.com/v1beta1/patroni_types.go
Comment thread internal/controller/postgrescluster/patroni.go Outdated
Comment thread build/postgres-operator/patroni-role-change.sh
Comment thread build/postgres-operator/patroni-role-change.sh
@yoav-katz

yoav-katz commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@mayankshah1607
Im adding a One-shot Kubernetes Job (same pattern as the existing pgBackRest restore Job) using the Patroni image + its config/secrets, whose only command is patronictl remove <scope> (confirmation piped via stdin). observeRestoreEnv looks it up like it does restoreJob; prepareForRestore creates it once pods are gone and waits for it to complete before resetting status.

@JNKPercona

Copy link
Copy Markdown
Collaborator
Test Name Result Time
backup-enable-disable skipped 00:00:00
builtin-extensions skipped 00:00:00
cert-manager-tls skipped 00:00:00
custom-envs skipped 00:00:00
custom-tls skipped 00:00:00
database-init-sql skipped 00:00:00
demand-backup skipped 00:00:00
demand-backup-offline-snapshot skipped 00:00:00
dynamic-configuration skipped 00:00:00
finalizers skipped 00:00:00
init-deploy skipped 00:00:00
huge-pages skipped 00:00:00
major-upgrade-14-to-15 skipped 00:00:00
major-upgrade-15-to-16 skipped 00:00:00
major-upgrade-16-to-17 skipped 00:00:00
major-upgrade-17-to-18 skipped 00:00:00
ldap skipped 00:00:00
ldap-tls skipped 00:00:00
monitoring skipped 00:00:00
monitoring-pmm3 skipped 00:00:00
one-pod skipped 00:00:00
operator-self-healing skipped 00:00:00
pitr skipped 00:00:00
scaling skipped 00:00:00
scheduled-backup skipped 00:00:00
self-healing skipped 00:00:00
sidecars skipped 00:00:00
standby-pgbackrest skipped 00:00:00
standby-streaming skipped 00:00:00
start-from-backup skipped 00:00:00
tablespaces skipped 00:00:00
telemetry-transfer skipped 00:00:00
upgrade-consistency skipped 00:00:00
upgrade-minor skipped 00:00:00
users skipped 00:00:00
etcd-dcs skipped 00:00:00
migration-from-crunchy-standby skipped 00:00:00
migration-from-crunchy-pv skipped 00:00:00
migration-from-crunchy-backup-restore skipped 00:00:00
Summary Value
Tests Run 0/39
Job Duration 00:24:22
Total Test Time N/A

commit: e84a9b1
image: perconalab/percona-postgresql-operator:PR-1647-e84a9b1d6

Comment on lines +347 to +349
if cluster.UsesExternalDCS() {
return nil, nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning both nil here is going to confuse the caller

Comment on lines +442 to +448
// The pod-annotation watch that normally triggers reconciles for
// pending restarts never fires under non-Kubernetes DCS (Patroni
// doesn't write that annotation), so poll at roughly Patroni's
// own loop_wait cadence instead. See handlePatroniRestarts.
// cluster.Default() (called earlier in Reconcile) guarantees
// SyncPeriodSeconds is set.
requeue = time.Duration(*cluster.Spec.Patroni.SyncPeriodSeconds) * time.Second

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update PR description with this information that operator needs to reconcile periodically with external DCS so we don't forget documenting it

Comment on lines +93 to +95
if !requiresRestart(instance.Pods[0]) {
continue
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you tried this with more than 1 replica in an instance?

meta.Labels = labels

job.ObjectMeta = meta
job.Spec = batchv1.JobSpec{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should let users configure tolerations and affinity for this job. since this is coupled with restores, maybe we can re-use restore tolerations/affinity

@egegunes

Copy link
Copy Markdown
Contributor

@yoav-katz friendly ping, we'll release v3.1.0 soon and it'd be great to include this

@yoav-katz

Copy link
Copy Markdown
Contributor Author

Hopefully I will get to it this weekend
Sorry for the delay😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants