fix(controllers): switch the PDB from maxUnavailable to minAvailable

README.md

@@ -21,7 +21,7 @@ The full design rationale is in [docs/concepts.md](docs/concepts.md).
 - **Pod restart / node failure**: data PVC is preserved, the new Pod reads the existing WAL and rejoins with the same member ID.
 - **Memory-backed storage (opt-in)**: `spec.storage.medium: Memory` switches each member's data dir to a tmpfs `emptyDir` whose lifetime is bound to the Pod. Members that lose their Pod (eviction, node failure) lose their data; the operator detects this, removes the member from etcd, and replaces it via the existing scale-up path. Suits scenarios where the etcd state is reconstructable and replication absorbs single-member losses. For production, set `spec.affinity` and `spec.resources.limits.memory` explicitly — neither is defaulted ([#16](https://github.com/lllamnyp/etcd-operator/issues/16)); see [docs/concepts.md](docs/concepts.md#storage).
 - **Apiserver-enforced validation**: CEL rules on the CRD (k8s 1.29+) reject `replicas: 0` with `storage.medium: Memory`, `storage.size: 0` with `storage.medium: Memory`, `storage.medium` changes after creation, and `storage.size` shrinks. No webhook / cert-manager dependency.
-- **PodDisruptionBudget**: per-cluster PDB selects voting members only (`role=voter`); `maxUnavailable = (voters-1)/2` so `kubectl drain` cannot voluntarily push the cluster below quorum.
+- **PodDisruptionBudget**: per-cluster PDB selects voting members only (`role=voter`); `minAvailable` is the quorum of whichever is larger, the live voter count or the intended cluster size, so `kubectl drain` cannot voluntarily push the cluster below quorum — even while node churn shrinks live membership.
 - **TLS (BYO Secrets or cert-manager)**: `spec.tls.client` / `spec.tls.peer` enable TLS on each surface independently. Material comes from either user-provided Secrets (`serverSecretRef` / `operatorClientSecretRef` / `secretRef`) or operator-emitted `cert-manager.io/v1` Certificates (`certManager.{serverIssuerRef,operatorClientIssuerRef,issuerRef}`) — mutually exclusive per subtree, enforced by CEL. mTLS is the implicit mode when an operator-client source is supplied; server-TLS-only when it isn't. The whole `tls` subtree is CEL-locked immutable post-create. cert-manager-emitted certs auto-renew via cert-manager; Pod-side rotation is a manual one-at-a-time `kubectl delete pod` either way. See [docs/concepts.md](docs/concepts.md#tls).
 - **Resource sizing**: `spec.resources` (a `corev1.ResourceRequirements`) sets the etcd container's CPU/memory requests and limits. Unset uses a conservative 100m/128Mi-request default. Updates take effect on newly-created members; pair with a `VerticalPodAutoscaler` targeting the cluster for live recommendation/rollout.
 - **Scheduling & extra metadata**: `spec.affinity` and `spec.topologySpreadConstraints` pass through to every member Pod (anti-affinity is not defaulted — set it for production); `spec.additionalMetadata` merges user labels/annotations onto every object the operator creates (member Pods, data PVCs, Services, PDB, `EtcdMember` CRs), with operator-owned keys winning on collision. All three apply on object creation and are latched like the rest of the spec. See [docs/concepts.md](docs/concepts.md#pod-scheduling-and-additional-metadata).

api/v1alpha2/etcdmember_types.go

@@ -143,11 +143,10 @@ type EtcdMemberSpec struct {
 	// affect already-running members.
 	ClusterToken string `json:"clusterToken"`
 
-	// Replicas exists only because the PodDisruptionBudget controller
-	// traverses Pods' controllerRef looking for /scale on the parent and
-	// fails closed ("does not implement the scale subresource") if it
-	// isn't there. Each EtcdMember represents exactly one Pod; this field
-	// is locked to 1 by validation and cannot be tuned.
+	// Replicas backs the /scale subresource. The operator's own PDB
+	// (integer minAvailable) never resolves scale; kept because
+	// maxUnavailable or percentage budgets over member Pods fail
+	// without it. Locked to 1: an EtcdMember is exactly one Pod.
 	// +kubebuilder:default=1
 	// +kubebuilder:validation:Minimum=1
 	// +kubebuilder:validation:Maximum=1
@@ -214,14 +213,13 @@ type EtcdMemberStatus struct {
 	IsVoter bool `json:"isVoter,omitempty"`
 
 	// Replicas exposes via /scale "this EtcdMember owns 1 Pod if it has
-	// a PodName, 0 otherwise". Required by the PodDisruptionBudget
-	// controller to derive expectedPods for the cluster's PDB — without
-	// /scale on the Pod controller-ref it sets the PDB to SyncFailed.
+	// a PodName, 0 otherwise". Unused by the operator's own PDB;
+	// scale-resolving budgets go SyncFailed without it.
 	// +optional
 	Replicas int32 `json:"replicas,omitempty"`
 
-	// Selector exposes the label-selector that matches this member's Pod
-	// via /scale (consumed by the PDB controller; not user-facing).
+	// Selector exposes the label-selector matching this member's Pod via
+	// /scale (for scale-resolving disruption budgets; not user-facing).
 	// +optional
 	Selector string `json:"selector,omitempty"`
 

charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdmembers.yaml

@@ -1100,11 +1100,10 @@ spec:
               replicas:
                 default: 1
                 description: |-
-                  Replicas exists only because the PodDisruptionBudget controller
-                  traverses Pods' controllerRef looking for /scale on the parent and
-                  fails closed ("does not implement the scale subresource") if it
-                  isn't there. Each EtcdMember represents exactly one Pod; this field
-                  is locked to 1 by validation and cannot be tuned.
+                  Replicas backs the /scale subresource. The operator's own PDB
+                  (integer minAvailable) never resolves scale; kept because
+                  maxUnavailable or percentage budgets over member Pods fail
+                  without it. Locked to 1: an EtcdMember is exactly one Pod.
                 format: int32
                 maximum: 1
                 minimum: 1
@@ -1676,15 +1675,14 @@ spec:
               replicas:
                 description: |-
                   Replicas exposes via /scale "this EtcdMember owns 1 Pod if it has
-                  a PodName, 0 otherwise". Required by the PodDisruptionBudget
-                  controller to derive expectedPods for the cluster's PDB — without
-                  /scale on the Pod controller-ref it sets the PDB to SyncFailed.
+                  a PodName, 0 otherwise". Unused by the operator's own PDB;
+                  scale-resolving budgets go SyncFailed without it.
                 format: int32
                 type: integer
               selector:
                 description: |-
-                  Selector exposes the label-selector that matches this member's Pod
-                  via /scale (consumed by the PDB controller; not user-facing).
+                  Selector exposes the label-selector matching this member's Pod via
+                  /scale (for scale-resolving disruption budgets; not user-facing).
                 type: string
               version:
                 description: |-

controllers/etcdcluster_controller.go

@@ -1492,16 +1492,16 @@ func (r *EtcdClusterReconciler) updateStatus(
 	return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
 }
 
-// pdbMaxUnavailable returns the disruption budget for a cluster with
-// the given voting-member count. The formula is (voters-1)/2, Go
-// integer-divided so the result auto-floors. Equivalently: at most
-// fewer-than-quorum voters may be unavailable. For 1 voter the budget
-// is 0 (any disruption is quorum loss); for 3 → 1, 4 → 1, 5 → 2.
-func pdbMaxUnavailable(voterCount int32) int32 {
-	if voterCount < 1 {
+// pdbMinAvailable returns the eviction floor: quorum (n/2+1) of
+// max(live voters, latched target). The target keeps the floor from
+// re-basing during churn; the live count covers scale-down. Full
+// analysis in docs/concepts.md#poddisruptionbudget.
+func pdbMinAvailable(voterCount, targetReplicas int32) int32 {
+	anchor := max(voterCount, targetReplicas)
+	if anchor < 1 {
 		return 0
 	}
-	return (voterCount - 1) / 2
+	return anchor/2 + 1
 }
 
 // reconcilePDB ensures a per-cluster PodDisruptionBudget exists that
@@ -1512,7 +1512,7 @@ func pdbMaxUnavailable(voterCount int32) int32 {
 // When voterCount is 0 (pre-bootstrap, paused, or wedged), the PDB is
 // deleted: there are no Pods to protect and a stale PDB selector
 // against missing labels is at best confusing, at worst (with a
-// non-zero MaxUnavailable from a prior state) misleading.
+// stale MinAvailable from a prior state) misleading.
 func (r *EtcdClusterReconciler) reconcilePDB(
 	ctx context.Context,
 	cluster *lll.EtcdCluster,
@@ -1532,7 +1532,11 @@ func (r *EtcdClusterReconciler) reconcilePDB(
 		return r.Delete(ctx, pdb)
 	}
 
-	max := intstr.FromInt32(pdbMaxUnavailable(voterCount))
+	targetReplicas := int32(0)
+	if cluster.Status.Observed != nil {
+		targetReplicas = cluster.Status.Observed.Replicas
+	}
+	minAvail := intstr.FromInt32(pdbMinAvailable(voterCount, targetReplicas))
 	wantSelector := &metav1.LabelSelector{
 		MatchLabels: map[string]string{
 			LabelCluster: cluster.Name,
@@ -1550,8 +1554,8 @@ func (r *EtcdClusterReconciler) reconcilePDB(
 				Annotations: pdbAnnotations,
 			},
 			Spec: policyv1.PodDisruptionBudgetSpec{
-				MaxUnavailable: &max,
-				Selector:       wantSelector,
+				MinAvailable: &minAvail,
+				Selector:     wantSelector,
 			},
 		}
 		if err := controllerutil.SetControllerReference(cluster, fresh, r.Scheme); err != nil {
@@ -1563,19 +1567,20 @@ func (r *EtcdClusterReconciler) reconcilePDB(
 		return getErr
 	}
 
-	// Patch only if maxUnavailable diverged. Selector is invariant by
-	// construction; if a future change wanted a different selector,
-	// PDB Selector is immutable on the apiserver side anyway, so the
-	// correct path would be Delete+Create rather than Patch.
-	currentMax := int32(-1)
-	if pdb.Spec.MaxUnavailable != nil {
-		currentMax = int32(pdb.Spec.MaxUnavailable.IntValue())
+	// Patch if minAvailable diverged or a pre-migration maxUnavailable
+	// remains (setting both is invalid). Selector is invariant by
+	// construction; it's immutable server-side anyway, so changing it
+	// would take Delete+Create rather than Patch.
+	currentMin := int32(-1)
+	if pdb.Spec.MaxUnavailable == nil && pdb.Spec.MinAvailable != nil {
+		currentMin = int32(pdb.Spec.MinAvailable.IntValue())
 	}
-	if currentMax == pdbMaxUnavailable(voterCount) {
+	if currentMin == minAvail.IntVal {
 		return nil
 	}
 	orig := pdb.DeepCopy()
-	pdb.Spec.MaxUnavailable = &max
+	pdb.Spec.MaxUnavailable = nil
+	pdb.Spec.MinAvailable = &minAvail
 	return r.Patch(ctx, pdb, client.MergeFrom(orig))
 }
 

controllers/etcdcluster_controller_test.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -3164,27 +3165,45 @@ func TestUpdateStatus_SetsScaleSelector(t *testing.T) {
 	}
 }
 
-// TestPDBMaxUnavailable pins the disruption budget formula. The
-// PodDisruptionBudget protects voting members; this is the single
-// place where the (n-1)/2 floor is computed.
-func TestPDBMaxUnavailable(t *testing.T) {
+// TestPDBMinAvailable pins the formula: quorum of max(voters, target).
+func TestPDBMinAvailable(t *testing.T) {
 	cases := []struct {
 		voters int32
+		target int32
 		want   int32
 	}{
-		{voters: 0, want: 0},
-		{voters: 1, want: 0},
-		{voters: 2, want: 0},
-		{voters: 3, want: 1},
-		{voters: 4, want: 1},
-		{voters: 5, want: 2},
-		{voters: 6, want: 2},
-		{voters: 7, want: 3},
+		// Steady state: quorum(n) = n/2+1.
+		{voters: 1, target: 1, want: 1},
+		{voters: 2, target: 2, want: 2},
+		{voters: 3, target: 3, want: 2},
+		{voters: 4, target: 4, want: 3},
+		{voters: 5, target: 5, want: 3},
+		{voters: 7, target: 7, want: 4},
+		// No latched target (Observed nil): anchor to live voters.
+		{voters: 3, target: 0, want: 2},
+		{voters: 5, target: 0, want: 3},
+		// Churn: live voters shrink, target holds the floor.
+		{voters: 3, target: 5, want: 3},
+		{voters: 2, target: 5, want: 3},
+		// Below target the floor holds but the budget only reaches zero
+		// once healthy voters fall to it. Allowed = healthy - want, so a
+		// one-voter shortfall still permits disruptions on targets > 3:
+		{voters: 4, target: 5, want: 3}, // 4-3 = 1 allowed
+		{voters: 6, target: 7, want: 4}, // 6-4 = 2 allowed
+		{voters: 5, target: 7, want: 4}, // 5-4 = 1 allowed
+		{voters: 4, target: 7, want: 4}, // 4-4 = 0, blocked
+		// Scale-down 5→3: live count dominates until members go.
+		{voters: 5, target: 3, want: 3},
+		{voters: 4, target: 3, want: 3},
+		// Bootstrap 1→3: target dominates from the start.
+		{voters: 1, target: 3, want: 2},
+		{voters: 2, target: 3, want: 2},
+		{voters: 0, target: 0, want: 0},
 	}
 	for _, tc := range cases {
-		got := pdbMaxUnavailable(tc.voters)
+		got := pdbMinAvailable(tc.voters, tc.target)
 		if got != tc.want {
-			t.Fatalf("pdbMaxUnavailable(%d) = %d, want %d", tc.voters, got, tc.want)
+			t.Fatalf("pdbMinAvailable(%d, %d) = %d, want %d", tc.voters, tc.target, got, tc.want)
 		}
 	}
 }
@@ -3210,8 +3229,11 @@ func TestReconcilePDB_CreatesWithVoterSelector(t *testing.T) {
 	if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test"}, pdb); err != nil {
 		t.Fatalf("Get PDB: %v", err)
 	}
-	if pdb.Spec.MaxUnavailable == nil || pdb.Spec.MaxUnavailable.IntValue() != 1 {
-		t.Fatalf("MaxUnavailable = %v, want 1", pdb.Spec.MaxUnavailable)
+	if pdb.Spec.MinAvailable == nil || pdb.Spec.MinAvailable.IntValue() != 2 {
+		t.Fatalf("MinAvailable = %v, want 2", pdb.Spec.MinAvailable)
+	}
+	if pdb.Spec.MaxUnavailable != nil {
+		t.Fatalf("MaxUnavailable = %v, want nil", pdb.Spec.MaxUnavailable)
 	}
 	want := map[string]string{LabelCluster: "test", LabelRole: RoleVoter}
 	if !reflect.DeepEqual(pdb.Spec.Selector.MatchLabels, want) {
@@ -3273,7 +3295,7 @@ func TestAdditionalMetadata_AppliedToServiceAndPDB(t *testing.T) {
 }
 
 // TestReconcilePDB_UpdatesOnVoterCountChange verifies the in-place
-// Patch path: existing PDB's MaxUnavailable is updated rather than the
+// Patch path: existing PDB's MinAvailable is updated rather than the
 // PDB being deleted+recreated. PDB Selector is immutable on the
 // apiserver side; the test also implicitly guards against an
 // accidental Selector change attempting that path.
@@ -3296,16 +3318,80 @@ func TestReconcilePDB_UpdatesOnVoterCountChange(t *testing.T) {
 	if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test"}, pdb); err != nil {
 		t.Fatalf("Get PDB: %v", err)
 	}
-	if pdb.Spec.MaxUnavailable.IntValue() != 2 {
-		t.Fatalf("MaxUnavailable after 3→5 transition = %d, want 2", pdb.Spec.MaxUnavailable.IntValue())
+	if pdb.Spec.MinAvailable.IntValue() != 3 {
+		t.Fatalf("MinAvailable after 3→5 transition = %d, want 3", pdb.Spec.MinAvailable.IntValue())
+	}
+}
+
+// A pre-upgrade maxUnavailable PDB is converted; both fields set is invalid.
+func TestReconcilePDB_MigratesMaxUnavailable(t *testing.T) {
+	ctx := context.Background()
+	cluster := &lll.EtcdCluster{
+		ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"},
+	}
+	oldMax := intstr.FromInt32(1)
+	oldPDB := &policyv1.PodDisruptionBudget{
+		ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"},
+		Spec: policyv1.PodDisruptionBudgetSpec{
+			MaxUnavailable: &oldMax,
+			Selector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{LabelCluster: "test", LabelRole: RoleVoter},
+			},
+		},
+	}
+	c, _ := newTestClient(t, cluster, oldPDB)
+	r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)}
+
+	if err := r.reconcilePDB(ctx, cluster, 3); err != nil {
+		t.Fatalf("reconcilePDB: %v", err)
+	}
+
+	pdb := &policyv1.PodDisruptionBudget{}
+	if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test"}, pdb); err != nil {
+		t.Fatalf("Get PDB: %v", err)
+	}
+	if pdb.Spec.MaxUnavailable != nil {
+		t.Fatalf("MaxUnavailable = %v, want cleared", pdb.Spec.MaxUnavailable)
+	}
+	if pdb.Spec.MinAvailable == nil || pdb.Spec.MinAvailable.IntValue() != 2 {
+		t.Fatalf("MinAvailable = %v, want 2", pdb.Spec.MinAvailable)
+	}
+}
+
+// Voters shrink during churn; the floor stays at the target's quorum.
+func TestReconcilePDB_HoldsFloorDuringChurn(t *testing.T) {
+	ctx := context.Background()
+	cluster := &lll.EtcdCluster{
+		ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"},
+		Status: lll.EtcdClusterStatus{
+			Observed: &lll.ObservedClusterSpec{Replicas: 5},
+		},
+	}
+	c, _ := newTestClient(t, cluster)
+	r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)}
+
+	if err := r.reconcilePDB(ctx, cluster, 5); err != nil {
+		t.Fatalf("reconcilePDB(5): %v", err)
+	}
+	for _, voters := range []int32{4, 3, 2} {
+		if err := r.reconcilePDB(ctx, cluster, voters); err != nil {
+			t.Fatalf("reconcilePDB(%d): %v", voters, err)
+		}
+		pdb := &policyv1.PodDisruptionBudget{}
+		if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test"}, pdb); err != nil {
+			t.Fatalf("Get PDB: %v", err)
+		}
+		if pdb.Spec.MinAvailable.IntValue() != 3 {
+			t.Fatalf("MinAvailable at %d live voters = %d, want 3", voters, pdb.Spec.MinAvailable.IntValue())
+		}
 	}
 }
 
 // TestReconcilePDB_DeletesWhenNoVoters covers pre-bootstrap, paused,
 // and wedged states: a PDB with zero matching Pods (because the
 // label-bearing voters don't exist) would be inert anyway, but its
-// staleness — particularly with a non-zero MaxUnavailable from a
-// prior state — would mislead operators reading kubectl get pdb.
+// staleness — particularly with a stale MinAvailable from a prior
+// state — would mislead operators reading kubectl get pdb.
 func TestReconcilePDB_DeletesWhenNoVoters(t *testing.T) {
 	ctx := context.Background()
 	cluster := &lll.EtcdCluster{

controllers/etcdmember_controller.go

@@ -1020,8 +1020,8 @@ func (r *EtcdMemberReconciler) updateStatus(ctx context.Context, member *lll.Etc
 	}
 
 	// /scale fields: Replicas reflects "Pod exists" (1) or not (0).
-	// Selector matches this member's single Pod. Consumed by the PDB
-	// controller during expectedPods derivation; not user-facing.
+	// Selector matches this member's single Pod. Read only by
+	// scale-resolving disruption budgets; not user-facing.
 	if member.Status.Replicas != 1 {
 		member.Status.Replicas = 1
 		changed = true