From 57d21b547b761a81063c96a6fb6e9c37039a0e4c Mon Sep 17 00:00:00 2001 From: Timofei Larkin Date: Mon, 3 Aug 2026 18:15:43 +0300 Subject: [PATCH] fix(controllers): derive --initial-cluster-state from phase, not from the seed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --initial-cluster-state was rendered from `spec.bootstrap`, a field set once at seed creation and never cleared. The seed's Pod was therefore built with `=new` for the entire life of the cluster rather than only for its first boot, while its --initial-cluster stayed frozen at the bootstrap value naming only itself. etcd honours the flag only on an empty data dir, so this was inert on ordinary restarts and inert on a corrupt data dir — that fails to boot either way, which is the crash-loop path self-heal already covers. It was not inert when the seed's data dir came back *empty* with the PVC binding intact: a re-provisioned volume, a blank PV restore, node-local storage lost on reimage. `=new` against a self-only --initial-cluster is a complete, internally consistent bootstrap instruction, so etcd did not error. It formed a fresh one-member cluster on the empty dir and reported healthy. That failure is quieter than the crash-loop it resembles. The Pod goes Ready, so neither self-heal trigger can see it — one needs a lost Pod, the other a not-ready container — and the -client Service selects every member Pod with no role filter, so a share of client traffic reaches a member serving an empty keyspace while writes routed there stay invisible to the real cluster. Since etcd derives cluster and member IDs from the initial peer-URL set plus the cluster token, none of which change here, such a member returns under the same cluster ID rather than being rejected on a mismatch. Emit `=new` only while nothing yet says the cluster exists: the member has never appeared in etcd's member list (status.memberID empty) and the cluster has not latched a status.clusterID. Requiring both is strictly safer than either alone and cannot misfire — either signal being set proves the cluster formed, which makes an empty data dir data loss rather than a pending bootstrap, and `=existing` then fails loudly into the self-heal path instead of silently forking the cluster. spec.bootstrap keeps its identity role: it still anchors seed discovery before clusterID is latched, and now records which member bootstrapped without steering any ongoing behaviour. No spec writes and no Pod restarts — ensurePod never re-templates, so running seeds keep their current argv and converge to `=existing` whenever their Pod is next recreated. The restore path is unaffected: the agent's snapshot.Restore writes a complete data dir with a WAL, so etcd ignores the flag there entirely. Adds unit coverage for a flag that previously had none, and an e2e that wipes (rather than corrupts) the seed's data dir and asserts it never returns serving an empty keyspace, that it is replaced, and that data written before the wipe survives on every member. Signed-off-by: Timofei Larkin --- controllers/etcdmember_controller.go | 40 ++- controllers/etcdmember_controller_test.go | 144 +++++++++-- controllers/restore_initcontainer_test.go | 8 +- docs/concepts.md | 31 ++- test/e2e/member_selfheal_test.go | 19 +- test/e2e/seed_rebootstrap_test.go | 281 ++++++++++++++++++++++ 6 files changed, 475 insertions(+), 48 deletions(-) create mode 100644 test/e2e/seed_rebootstrap_test.go diff --git a/controllers/etcdmember_controller.go b/controllers/etcdmember_controller.go index f9b0ef70..c94157eb 100644 --- a/controllers/etcdmember_controller.go +++ b/controllers/etcdmember_controller.go @@ -478,7 +478,15 @@ func (r *EtcdMemberReconciler) ensurePod(ctx context.Context, member *lll.EtcdMe "(set --operator-image / OPERATOR_IMAGE); refusing to create a seed Pod with an empty restore image", member.Name) } - pod = r.buildPod(member) + // A missing/unreadable parent is treated as "not formed": that can only + // happen before the cluster exists or while it is being deleted, and the + // member's own MemberID still guards the seed case independently. + clusterFormed := false + if cluster, cErr := r.clusterFor(ctx, member); cErr == nil { + clusterFormed = cluster.Status.ClusterID != "" + } + + pod = r.buildPod(member, clusterFormed) if err := controllerutil.SetControllerReference(member, pod, r.Scheme); err != nil { return err } @@ -617,10 +625,32 @@ func optionFlags(o *lll.EtcdOptions) []string { return flags } -func (r *EtcdMemberReconciler) buildPod(member *lll.EtcdMember) *corev1.Pod { - clusterState := "new" - if !member.Spec.Bootstrap { - clusterState = "existing" +// buildPod renders a member's Pod. clusterFormed reports whether the parent +// cluster has latched status.clusterID; see the --initial-cluster-state +// derivation below for why it is a parameter rather than read from the member. +func (r *EtcdMemberReconciler) buildPod(member *lll.EtcdMember, clusterFormed bool) *corev1.Pod { + // --initial-cluster-state=new is a bootstrap instruction, not a property of + // the seed. etcd honours it only on an empty data dir, so emitting it for + // life means a seed that comes back with an *empty* dir (re-provisioned + // volume, blank PV restore) silently forms a fresh one-member cluster on + // its frozen self-only --initial-cluster and reports healthy — invisible to + // self-heal, which needs a not-ready container. =existing fails loudly + // instead, which is recoverable. + // + // Three conditions, and the first is a hard gate rather than a phase + // signal: only the seed may ever be told to bootstrap. Keep it even though + // the phase signals usually imply it — clusterFormed falls back to false + // when the parent cluster cannot be read, and a scale-up member's MemberID + // is empty until its Pod is Ready, so without spec.bootstrap a transient + // Get failure would hand a scale-up member =new. + // + // The other two are the phase signals: the member has never been seen in + // etcd's member list, and the cluster has not latched a clusterID. Either + // one being set proves the cluster formed, which makes an empty data dir + // data loss rather than a pending bootstrap. + clusterState := "existing" + if member.Spec.Bootstrap && member.Status.MemberID == "" && !clusterFormed { + clusterState = "new" } clientScheme := memberClientScheme(member) diff --git a/controllers/etcdmember_controller_test.go b/controllers/etcdmember_controller_test.go index 106758c9..2568dcb3 100644 --- a/controllers/etcdmember_controller_test.go +++ b/controllers/etcdmember_controller_test.go @@ -1270,7 +1270,7 @@ func TestBuildPod_LivenessIsNotQuorumAware(t *testing.T) { pod := r.buildPod(&lll.EtcdMember{ ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns"}, Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17"}, - }) + }, false) lp := pod.Spec.Containers[0].LivenessProbe if lp == nil { t.Fatalf("missing liveness probe entirely") @@ -1295,7 +1295,7 @@ func TestBuildPod_ImageRepoAndPullSecrets(t *testing.T) { pod := r.buildPod(&lll.EtcdMember{ ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns"}, Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.6.11"}, - }) + }, false) if got := pod.Spec.Containers[0].Image; got != "registry.internal/mirror/etcd:v3.6.11" { t.Errorf("image = %q, want operator-default mirror", got) } @@ -1310,7 +1310,7 @@ func TestBuildPod_ImageRepoAndPullSecrets(t *testing.T) { Version: "3.6.11", ImagePullSecrets: []corev1.LocalObjectReference{{Name: "regcreds"}}, }, - }) + }, false) if len(pod.Spec.ImagePullSecrets) != 1 || pod.Spec.ImagePullSecrets[0].Name != "regcreds" { t.Errorf("pod.imagePullSecrets = %+v, want [regcreds]", pod.Spec.ImagePullSecrets) } @@ -1352,7 +1352,7 @@ func TestBuildPod_AppliesSchedulingAndMetadata(t *testing.T) { Annotations: map[string]string{"example.com/note": "bar"}, }, }, - }) + }, false) if !equality.Semantic.DeepEqual(pod.Spec.Affinity, aff) { t.Errorf("pod affinity = %+v, want %+v", pod.Spec.Affinity, aff) @@ -1379,7 +1379,7 @@ func TestBuildPod_NoAdditionalMetadataLeavesAnnotationsNil(t *testing.T) { pod := r.buildPod(&lll.EtcdMember{ ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns"}, Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17"}, - }) + }, false) if pod.Annotations != nil { t.Errorf("expected nil annotations, got %+v", pod.Annotations) } @@ -2021,7 +2021,7 @@ func TestBuildPod_MemoryMediumUsesEmptyDir(t *testing.T) { Version: "3.5.17", Storage: lll.StorageSpec{Size: storage, Medium: lll.StorageMediumMemory}, }, - }) + }, false) if len(pod.Spec.Volumes) != 1 { t.Fatalf("expected one Volume; got %d", len(pod.Spec.Volumes)) @@ -2055,7 +2055,7 @@ func TestBuildPod_DefaultMediumUsesPVC(t *testing.T) { Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, // storage.medium left empty. }, - }) + }, false) v := pod.Spec.Volumes[0] if v.EmptyDir != nil { t.Fatalf("default member must not have an EmptyDir volume source; got %+v", v.EmptyDir) @@ -2466,7 +2466,7 @@ func TestBuildPod_RoleLabelAtCreateForVoter(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "m-1", Namespace: "ns"}, Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17"}, Status: lll.EtcdMemberStatus{IsVoter: true}, - }) + }, false) if pod.Labels[LabelRole] != RoleVoter { t.Fatalf("buildPod with IsVoter=true must emit %s=%q; got %q", LabelRole, RoleVoter, pod.Labels[LabelRole]) } @@ -2477,7 +2477,7 @@ func TestBuildPod_RoleLabelAtCreateForVoter(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "m-2", Namespace: "ns"}, Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17"}, Status: lll.EtcdMemberStatus{IsVoter: false}, - }) + }, false) if _, present := pod2.Labels[LabelRole]; present { t.Fatalf("buildPod with IsVoter=false must not set %s; got %q", LabelRole, pod2.Labels[LabelRole]) } @@ -2520,7 +2520,7 @@ func TestBuildPod_PlaintextHasNoTLSFlags(t *testing.T) { pod := r.buildPod(&lll.EtcdMember{ ObjectMeta: metav1.ObjectMeta{Name: "m", Namespace: "ns"}, Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}}, - }) + }, false) cmd := pod.Spec.Containers[0].Command if !cmdContains(cmd, "--listen-peer-urls=http://0.0.0.0:2380") { t.Fatalf("plaintext peer listen URL missing: %v", cmd) @@ -2558,7 +2558,7 @@ func TestBuildPod_ClientTLSOnlyAddsServerCertButNoClientAuth(t *testing.T) { ClientMTLS: false, }, }, - }) + }, false) cmd := pod.Spec.Containers[0].Command if !cmdContains(cmd, "--listen-client-urls=https://0.0.0.0:2379") { t.Fatalf("client listen URL not https: %v", cmd) @@ -2600,7 +2600,7 @@ func TestBuildPod_ClientMTLSAddsTrustedCAAndClientCertAuth(t *testing.T) { ClientMTLS: true, }, }, - }) + }, false) cmd := pod.Spec.Containers[0].Command if !cmdContains(cmd, "--client-cert-auth=true") { t.Fatalf("mTLS pod must set --client-cert-auth=true: %v", cmd) @@ -2624,7 +2624,7 @@ func TestBuildPod_PeerTLSAlwaysMTLS(t *testing.T) { PeerSecretRef: &corev1.LocalObjectReference{Name: "peer"}, }, }, - }) + }, false) cmd := pod.Spec.Containers[0].Command if !cmdContains(cmd, "--listen-peer-urls=https://0.0.0.0:2380") { t.Fatalf("peer listen URL not https: %v", cmd) @@ -2655,7 +2655,7 @@ func TestBuildPod_PeerAutoTLS(t *testing.T) { ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, TLS: &lll.EtcdMemberTLS{PeerAutoTLS: true}, }, - }) + }, false) cmd := pod.Spec.Containers[0].Command if !cmdContains(cmd, "--listen-peer-urls=https://0.0.0.0:2380") { t.Fatalf("peer listen URL not https: %v", cmd) @@ -2708,7 +2708,7 @@ func TestBuildPod_AlwaysExposesMetricsPort(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - pod := r.buildPod(tc.member) + pod := r.buildPod(tc.member, false) var foundPort *corev1.ContainerPort for i, p := range pod.Spec.Containers[0].Ports { if p.Name == "metrics" { @@ -2752,7 +2752,7 @@ func TestBuildPod_UsesSpecResources(t *testing.T) { Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, Resources: want, }, - }) + }, false) got := pod.Spec.Containers[0].Resources if got.Requests.Cpu().Cmp(*want.Requests.Cpu()) != 0 || got.Requests.Memory().Cmp(*want.Requests.Memory()) != 0 || @@ -2781,7 +2781,7 @@ func TestBuildPod_ClaimsOnlyResourcesNotDroppedToDefault(t *testing.T) { Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, Resources: in, }, - }) + }, false) got := pod.Spec.Containers[0].Resources if len(got.Claims) != 1 || got.Claims[0].Name != "gpu" { t.Fatalf("Claims dropped on the floor; got %+v", got.Claims) @@ -2805,7 +2805,7 @@ func TestBuildPod_DefaultsResourcesWhenUnset(t *testing.T) { Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, // Resources intentionally zero. }, - }) + }, false) got := pod.Spec.Containers[0].Resources if got.Requests.Cpu().Cmp(resource.MustParse("100m")) != 0 { t.Fatalf("default CPU request = %v; want 100m", got.Requests.Cpu()) @@ -2839,7 +2839,7 @@ func TestBuildPod_AppliesEtcdOptions(t *testing.T) { SnapshotCount: &snapCount, }, }, - }) + }, false) cmd := pod.Spec.Containers[0].Command for _, want := range []string{ "--quota-backend-bytes=10200547328", @@ -2859,7 +2859,7 @@ func TestBuildPod_AppliesEtcdOptions(t *testing.T) { ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, - }) + }, false) for _, arg := range pod.Spec.Containers[0].Command { for _, prefix := range []string{"--quota-backend-bytes", "--auto-compaction", "--snapshot-count"} { if strings.HasPrefix(arg, prefix) { @@ -2893,7 +2893,7 @@ func TestBuildPod_AdoptionAnnotations(t *testing.T) { ClusterName: "etcd", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, - }) + }, false) if pod.Spec.Subdomain != "etcd-headless" { t.Errorf("subdomain = %q; want the annotation's headless service name", pod.Spec.Subdomain) } @@ -2916,7 +2916,7 @@ func TestBuildPod_AdoptionAnnotations(t *testing.T) { ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, - }) + }, false) if pod.Spec.Subdomain != "test" { t.Errorf("default subdomain = %q; want cluster name", pod.Spec.Subdomain) } @@ -2950,7 +2950,7 @@ func TestBuildPod_DataDirSubPathFailsClosed(t *testing.T) { ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, - }) + }, false) if !cmdContains(pod.Spec.Containers[0].Command, "--data-dir=/var/lib/etcd") { t.Errorf("subpath %q: --data-dir not fail-closed to volume root; got %v", bad, pod.Spec.Containers[0].Command) } @@ -2971,7 +2971,7 @@ func TestBuildPod_DataDirSubPathFailsClosed(t *testing.T) { ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, - }) + }, false) if !cmdContains(pod.Spec.Containers[0].Command, "--data-dir=/var/lib/etcd/default.etcd") { t.Errorf("valid subpath rejected: %v", pod.Spec.Containers[0].Command) } @@ -3155,3 +3155,99 @@ func TestEnsurePod_BlocksOnMissingTLSSecret(t *testing.T) { t.Fatalf("Pod should not exist when referenced TLS secret is missing") } } + +// podClusterState extracts the --initial-cluster-state flag from a built Pod. +func podClusterState(t *testing.T, pod *corev1.Pod) string { + t.Helper() + for _, arg := range pod.Spec.Containers[0].Command { + if v, ok := strings.CutPrefix(arg, "--initial-cluster-state="); ok { + return v + } + } + t.Fatalf("no --initial-cluster-state flag in %v", pod.Spec.Containers[0].Command) + return "" +} + +// TestBuildPod_InitialClusterState pins the one bootstrap instruction etcd acts +// on. `new` is only ever correct while the cluster has demonstrably not formed: +// on an empty data dir it makes etcd bootstrap a fresh cluster instead of +// failing, so a seed that keeps `new` for life turns data-dir loss into a +// silent one-member cluster serving an empty keyspace. Either signal proving +// the cluster exists must therefore force `existing`. +func TestBuildPod_InitialClusterState(t *testing.T) { + member := func(bootstrap bool, memberID string) *lll.EtcdMember { + return &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns"}, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", Bootstrap: bootstrap, + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test", + }, + Status: lll.EtcdMemberStatus{MemberID: memberID}, + } + } + + cases := []struct { + name string + member *lll.EtcdMember + clusterFormed bool + want string + }{ + {"seed mid-bootstrap: nothing says the cluster exists", member(true, ""), false, "new"}, + {"seed already in etcd's member list", member(true, "abc"), false, "existing"}, + {"seed whose cluster latched a clusterID", member(true, ""), true, "existing"}, + {"seed with both signals set", member(true, "abc"), true, "existing"}, + // Load-bearing: this is the row that pins the spec.bootstrap conjunct. + // clusterFormed falls back to false when the parent cluster cannot be + // read, and a scale-up member's MemberID is empty until its Pod is + // Ready, so the phase signals alone would yield "new" here. + {"scale-up member is never bootstrapping", member(false, ""), false, "existing"}, + {"adopted member (no seed, clusterID pre-latched)", member(false, "abc"), true, "existing"}, + } + + r := &EtcdMemberReconciler{Scheme: testScheme(t)} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := podClusterState(t, r.buildPod(tc.member, tc.clusterFormed)) + if got != tc.want { + t.Fatalf("--initial-cluster-state = %q, want %q", got, tc.want) + } + }) + } +} + +// TestEnsurePod_FormedClusterGivesSeedExistingState is the integration half: +// ensurePod must actually read the parent cluster's clusterID, not just accept +// a bool. A seed Pod re-created after the cluster formed gets `existing`. +func TestEnsurePod_FormedClusterGivesSeedExistingState(t *testing.T) { + ctx := context.Background() + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + Spec: lll.EtcdClusterSpec{Replicas: ptrInt32(3)}, + } + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", Labels: memberLabels("test", "test-0")}, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Bootstrap: true, Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test", + }, + } + c, _ := newTestClient(t, cluster, member) + got := mustGet(t, c, "test", "ns", &lll.EtcdCluster{}) + got.Status.ClusterID = "deadbeef" + if err := c.Status().Update(ctx, got); err != nil { + t.Fatalf("latch clusterID: %v", err) + } + + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + if err := r.ensurePod(ctx, member); err != nil { + t.Fatalf("ensurePod: %v", err) + } + + pod := &corev1.Pod{} + if err := c.Get(ctx, types.NamespacedName{Namespace: "ns", Name: "test-0"}, pod); err != nil { + t.Fatalf("get pod: %v", err) + } + if state := podClusterState(t, pod); state != "existing" { + t.Fatalf("seed Pod re-created after the cluster formed must get --initial-cluster-state=existing, got %q", state) + } +} diff --git a/controllers/restore_initcontainer_test.go b/controllers/restore_initcontainer_test.go index 93936eae..fa9caaeb 100644 --- a/controllers/restore_initcontainer_test.go +++ b/controllers/restore_initcontainer_test.go @@ -47,7 +47,7 @@ func findInitContainer(pod *corev1.Pod, name string) (corev1.Container, bool) { func TestBuildPod_NoRestoreInitContainerWithoutSpec(t *testing.T) { r := &EtcdMemberReconciler{Scheme: testScheme(t), OperatorImage: "operator:latest"} - pod := r.buildPod(seedMember(nil)) + pod := r.buildPod(seedMember(nil), false) if _, ok := findInitContainer(pod, "restore"); ok { t.Error("restore initContainer present though no restore spec was set") } @@ -64,7 +64,7 @@ func TestBuildPod_RestoreInitContainerS3(t *testing.T) { }, }} r := &EtcdMemberReconciler{Scheme: testScheme(t), OperatorImage: "operator:latest"} - pod := r.buildPod(seedMember(restore)) + pod := r.buildPod(seedMember(restore), false) ic, ok := findInitContainer(pod, "restore") if !ok { @@ -128,7 +128,7 @@ func TestBuildPod_RestoreInitContainerS3(t *testing.T) { // auto-mount a ServiceAccount token. func TestBuildPod_NoServiceAccountToken(t *testing.T) { r := &EtcdMemberReconciler{Scheme: testScheme(t), OperatorImage: "operator:latest"} - pod := r.buildPod(seedMember(nil)) + pod := r.buildPod(seedMember(nil), false) if pod.Spec.AutomountServiceAccountToken == nil || *pod.Spec.AutomountServiceAccountToken { t.Errorf("AutomountServiceAccountToken = %v, want explicit false", pod.Spec.AutomountServiceAccountToken) } @@ -161,7 +161,7 @@ func TestBuildPod_RestoreInitContainerPVC(t *testing.T) { PVC: &lll.PVCSnapshotLocation{ClaimName: "snap-pvc", SubPath: "b1.db"}, }} r := &EtcdMemberReconciler{Scheme: testScheme(t), OperatorImage: "operator:latest"} - pod := r.buildPod(seedMember(restore)) + pod := r.buildPod(seedMember(restore), false) ic, ok := findInitContainer(pod, "restore") if !ok { diff --git a/docs/concepts.md b/docs/concepts.md index e114aa40..4abc8d81 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -90,7 +90,7 @@ The cluster forms from a single seed member. Multi-seed bootstrap (multiple memb The seed is identified by `spec.bootstrap=true`. Member names being random precludes a name-based lookup, and trusting list order (`members[0]`) silently anchors discovery to the wrong member when scale-up CRs land in front of the seed. Once `clusterID` is set, no *membership* decision re-reads `spec.bootstrap` — the seed is, from that point on, just a regular member. It is not scheduled differently, not weighted in quorum, and not exempt from [crash-loop self-heal](#crash-loop-self-heal). A cluster running with no `spec.bootstrap=true` member at all is a normal, supported state: it is what every cluster adopted by `cmd/etcd-migrate` starts out as, and what any cluster becomes once its seed is replaced. -One reader does survive past latch: `buildPod` still derives `--initial-cluster-state` from `spec.bootstrap`, so a *re-created* seed Pod is handed `=new` rather than `=existing` for the life of the cluster. etcd consults that flag only when the data dir is empty, so it is inert on every ordinary restart. It is not inert if the seed's data dir is ever lost while the PVC binding survives — see [Known gap: a seed that re-bootstraps](#known-gap-a-seed-that-re-bootstraps). +`spec.bootstrap` is never cleared, so it stays a permanent record of which member formed the cluster. One derivation still consults it: `--initial-cluster-state` uses it to gate *which* member may ever be told to bootstrap, and then narrows that by cluster phase — see [Bootstrap state is a phase, not a member](#bootstrap-state-is-a-phase-not-a-member). If the seed's pod hasn't been created yet (between Create and Pod-up), the controller surfaces `Progressing=True/WaitingForSeed` rather than dialing a nonexistent endpoint and burning the reconcile budget. @@ -169,22 +169,33 @@ The member controller detects this and replaces the member: - **Replacement.** Deleting the `EtcdMember` runs the finalizer's clean `MemberRemove`, any member-owned PVC is GC'd (discarding the corrupt data dir; memory members have none), and the cluster controller gap-fills a fresh `GenerateName` member with a current `--initial-cluster` and a **new** etcd member ID — not a same-ID rejoin. - **Latency.** `CrashLoopBackOff` caps its backoff at 5 minutes, so reaching 5 restarts takes on the order of **tens of minutes**, not the ~5s of the memory Pod-loss path. A deliberately-deleted-and-replaced member during this window is expected operator behavior, not a fault. A slow restore or slow learner join on the *replacement* can itself trip the threshold and be replaced again; this is quorum-gated and self-limiting, but expect it on a struggling cluster. -### Known gap: a seed that re-bootstraps +### Bootstrap state is a phase, not a member -Crash-loop self-heal keys on a Pod that will not become ready. There is one seed-specific data-loss shape it therefore cannot see, and it is worth knowing about because it is *quieter* than the crash-loop it resembles. +`--initial-cluster-state` is the one flag that decides whether etcd *forms* a cluster or *joins* one. etcd reads it only when the data dir is empty, which makes it invisible on every ordinary restart and decisive in exactly one situation: a member coming back with nothing on disk. -`buildPod` sets `--initial-cluster-state=new` for `spec.bootstrap=true` members and `=existing` for everyone else, and `spec.bootstrap` is never cleared — so the seed's Pod is built with `=new` for the whole life of the cluster, not just for its first boot. Its `--initial-cluster` is likewise frozen at the value written during bootstrap, which lists only itself. +That makes it a statement about where the cluster is in its life, not merely about which member is which. Identity remains a precondition — only the seed can ever bootstrap — but it is no longer sufficient on its own. `new` requires **all three** of: -etcd reads both flags only when the data dir is empty. So on an ordinary restart, or on a restart onto a *corrupt* data dir, nothing changes: a corrupt dir makes etcd fail to boot, the Pod crash-loops, and self-heal replaces the member as described above. The gap is the case where the seed's data dir comes back **empty** rather than corrupt, with the PVC binding intact — a re-provisioned or wiped volume, a PV restored blank, node-local storage lost on reimage. Then: +- `spec.bootstrap=true`, so a scale-up member is never a candidate under any circumstances; +- the member has never been seen in etcd's member list (`status.memberID` is empty); and +- the cluster has not latched a `status.clusterID`. -- a non-seed member gets `=existing` against a stale `--initial-cluster` and fails loudly (`member count is unequal`), crash-loops, and is self-healed; -- the **seed** gets `=new` against an `--initial-cluster` naming only itself, which is a complete and internally consistent bootstrap instruction. etcd does not error. It forms a fresh one-member cluster on the empty dir and reports healthy. +The first is a hard identity gate that never expires. The other two are the phase signals, and either of them being set proves the cluster already formed — which makes an empty data dir data loss rather than a pending bootstrap. `=existing` is then correct: etcd fails to start against the stale `--initial-cluster`, the Pod crash-loops, and [self-heal](#crash-loop-self-heal) replaces the member. -The Pod then becomes *ready*, so neither self-heal trigger fires — not the `Status.PodUID` check (the Pod was never lost) and not the crash-loop check (`etcdContainerStuck` requires not-ready). Meanwhile `-client` selects every member Pod with no role filter, so a share of client traffic lands on a member serving an empty keyspace, and writes routed there are invisible to the real cluster. +Keeping the identity gate matters for a reason the phase signals alone do not cover: the cluster-formed signal is read from the parent `EtcdCluster` and falls back to "not formed" when that read fails, while a scale-up member's `status.memberID` stays empty until its Pod is Ready. Without `spec.bootstrap` a transient API error during that window would hand a scale-up member `=new`. -Note also that etcd derives both the cluster ID and the member ID from the initial peer-URL set plus `--initial-cluster-token`, all of which are unchanged here — so the re-bootstrapped seed is expected to come back up under the *same* cluster ID as the cluster it lost, rather than being rejected on an ID mismatch. That reasoning is from etcd's ID derivation, not from an observed incident. +Deriving it from `spec.bootstrap` instead — a field set once and never cleared — meant the seed carried `new` for the life of the cluster. That is inert on a *corrupt* data dir (etcd fails to boot either way) but not on an **empty** one: `new` against the seed's frozen self-only `--initial-cluster` is a complete, internally consistent bootstrap instruction, so etcd would not error. It would form a fresh one-member cluster on the empty dir, elect itself leader of it, and pass its `/health` readiness probe. -Closing this means decoupling "which member bootstrapped the cluster" from "which member should boot with `=new`" — the same phase-versus-identity split that removed the self-heal exemption. It is tracked separately from that fix. +Two consequences follow, and it is worth separating what is certain from what is not. + +**Certain: the member goes Ready on an empty data dir.** The readiness probe is `/health`, which a healthy one-member cluster answers 200. `-client` selects every member Pod with no role filter, so the member immediately takes a share of client traffic — serving reads from an empty keyspace, and accepting writes into a log the rest of the cluster knows nothing about. Neither self-heal trigger can see any of this: one needs a lost Pod, the other a not-ready container. + +**Certain: etcd's own guard against strangers does not fire.** `EtcdServer.Process` rejects an incoming raft message whose `m.To` is not the local member ID (`cannot process message to mismatch member`) — the check that would normally fence off a member belonging to a different cluster. Because the member ID is derived deterministically and nothing about this member changed, the re-booted member's ID is *identical* to the one the surviving members still have on file, so their messages address it correctly and are admitted straight into raft. The same holds one level up for the cluster ID, so the peer transport's `X-Etcd-Cluster-ID` check passes too. The collision is what removes the loud failure. + +**Not established: how long the divergence lasts.** Once those messages reach raft, the surviving leader's higher term and mismatched log should drive the member back into line — most likely via an `InstallSnapshot` that overwrites its empty state and restores the real membership and data. If so, the window is short. That is the expected behaviour rather than something observed here, and it is *not* what makes the flag wrong: a brief window is still wrong reads served to clients, and any write acknowledged in that window is silently discarded when the snapshot lands — an acknowledged-write loss, which is worse than a stale read. + +The argument for `=existing` does not rest on the divergence being durable. It rests on not issuing a bootstrap instruction to a member that is not bootstrapping: `=existing` fails immediately and deterministically into a designed, tested recovery path, instead of relying on raft to repair a state the operator should never have created. + +The `--` token derivation does not prevent the collision, and it is worth being precise about why, because it protects against a neighbouring failure. Its randomness lives in the `EtcdCluster`'s UID, so it varies **across incarnations** — delete and recreate a same-named cluster and the token differs, which is what stops a stale PVC from the previous incarnation rejoining ([member naming](#member-naming)). A member rebooting inside *one* incarnation has the same object, hence the same UID, hence the same token; its peer URL is unchanged because it keeps its name, and its `--initial-cluster` is frozen at the bootstrap value. The token defends the boundary between clusters, not between boots. ### What is missing from memory clusters today diff --git a/test/e2e/member_selfheal_test.go b/test/e2e/member_selfheal_test.go index 72c18ea5..72a4859d 100644 --- a/test/e2e/member_selfheal_test.go +++ b/test/e2e/member_selfheal_test.go @@ -147,11 +147,11 @@ func TestPVCMemberCrashLoopSelfHeal(t *testing.T) { } // readyMembersIs returns a waitFor condition that the cluster reports `want` -// ready members. -func readyMembersIs(name string, want int32) func(context.Context) error { +// ready members. Namespace-scoped variant for suites with their own namespace. +func readyMembersIsIn(namespace, name string, want int32) func(context.Context) error { return func(ctx context.Context) error { ec := &etcdv1alpha2.EtcdCluster{} - if err := kube.Get(ctx, client.ObjectKey{Namespace: selfHealNamespace, Name: name}, ec); err != nil { + if err := kube.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, ec); err != nil { return err } if ec.Status.ReadyMembers != want { @@ -161,6 +161,10 @@ func readyMembersIs(name string, want int32) func(context.Context) error { } } +func readyMembersIs(name string, want int32) func(context.Context) error { + return readyMembersIsIn(selfHealNamespace, name, want) +} + // selfHealMembers returns the cluster's EtcdMember names, failing the test on // a list error. func selfHealMembers(ctx context.Context, t *testing.T) []string { @@ -183,10 +187,15 @@ func selfHealMembers(ctx context.Context, t *testing.T) []string { // one), so a wrongly-shaped cluster fails loudly here rather than silently // selecting some other member. func selfHealSeedMember(ctx context.Context, t *testing.T) string { + t.Helper() + return seedMemberIn(ctx, t, selfHealNamespace, selfHealCluster) +} + +func seedMemberIn(ctx context.Context, t *testing.T, namespace, cluster string) string { t.Helper() list := &etcdv1alpha2.EtcdMemberList{} - if err := kube.List(ctx, list, client.InNamespace(selfHealNamespace), - client.MatchingLabels{"etcd-operator.cozystack.io/cluster": selfHealCluster}); err != nil { + if err := kube.List(ctx, list, client.InNamespace(namespace), + client.MatchingLabels{"etcd-operator.cozystack.io/cluster": cluster}); err != nil { t.Fatalf("list members: %v", err) } var seeds []string diff --git a/test/e2e/seed_rebootstrap_test.go b/test/e2e/seed_rebootstrap_test.go new file mode 100644 index 00000000..a00c1d11 --- /dev/null +++ b/test/e2e/seed_rebootstrap_test.go @@ -0,0 +1,281 @@ +//go:build e2e + +package e2e + +import ( + "context" + "fmt" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + etcdv1alpha2 "github.com/cozystack/etcd-operator/api/v1alpha2" +) + +const ( + rebootstrapNamespace = "seed-rebootstrap-e2e" + rebootstrapCluster = "etcd" + rebootstrapKey = "/e2e/pre-wipe-sentinel" + rebootstrapValue = "written-before-the-wipe" +) + +// TestSeedDataDirLossDoesNotRebootstrap covers the one data-loss shape that a +// crash-loop cannot express, and that the seed used to be uniquely vulnerable +// to. +// +// A member Pod is built with --initial-cluster-state, which etcd honours only +// when the data dir is empty. For a non-seed member that flag is `existing`, so +// an empty data dir fails loudly against a stale --initial-cluster and the +// member crash-loops into self-heal. The seed used to carry `new` for the life +// of the cluster, and `new` against its frozen self-only --initial-cluster is a +// complete bootstrap instruction: etcd does not error, it forms a *fresh* +// one-member cluster on the empty dir and reports healthy. The Pod goes Ready, +// so no self-heal trigger can see it, while the client Service keeps routing a +// share of traffic to a member serving an empty keyspace. +// +// Corruption is not a substitute: corrupt files make etcd fail to boot, which +// is the crash-loop path TestPVCMemberCrashLoopSelfHeal already covers. The +// divergence is specifically empty-vs-corrupt, so this test wipes the dir. +// +// The wiped seed must therefore never come back serving an empty keyspace; it +// must fail to start and be replaced, with the pre-wipe data intact throughout. +func TestSeedDataDirLossDoesNotRebootstrap(t *testing.T) { + ctx := context.Background() + + ns := &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Namespace"}, + ObjectMeta: metav1.ObjectMeta{Name: rebootstrapNamespace}, + } + if err := kube.Patch(ctx, ns, client.Apply, fieldOwner, client.ForceOwnership); err != nil { + t.Fatalf("create namespace %s: %v", rebootstrapNamespace, err) + } + t.Cleanup(func() { + _ = kube.Delete(context.Background(), &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: rebootstrapNamespace}}) + }) + + three := int32(3) + ec := &etcdv1alpha2.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: rebootstrapCluster, Namespace: rebootstrapNamespace}, + Spec: etcdv1alpha2.EtcdClusterSpec{ + Replicas: &three, + Version: "3.6.11", + Storage: etcdv1alpha2.StorageSpec{Size: resource.MustParse("1Gi")}, + }, + } + if err := kube.Create(ctx, ec); err != nil { + t.Fatalf("create EtcdCluster: %v", err) + } + + waitFor(ctx, t, 5*time.Minute, "cluster Available", etcdClusterAvailable(rebootstrapNamespace, rebootstrapCluster)) + waitFor(ctx, t, 2*time.Minute, "3 members ready", readyMembersIsIn(rebootstrapNamespace, rebootstrapCluster, 3)) + + // Write through the client Service so the value is committed cluster-wide. + // Its presence afterwards is what distinguishes "the cluster survived" from + // "a member is answering out of a brand-new empty store". + putSentinel(ctx, t) + + seed := seedMemberIn(ctx, t, rebootstrapNamespace, rebootstrapCluster) + t.Logf("wiping data dir of seed member %q", seed) + wipeMemberDataDir(ctx, t, seed) + + // Restart onto the now-empty volume. This is the moment + // --initial-cluster-state is consulted. + if err := kube.Delete(ctx, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: seed, Namespace: rebootstrapNamespace}}); err != nil { + t.Fatalf("delete seed pod %q: %v", seed, err) + } + + waitFor(ctx, t, 15*time.Minute, fmt.Sprintf("wiped seed %q replaced rather than re-bootstrapped", seed), + func(ctx context.Context) error { + // Opportunistic fast-fail, not the primary assertion. If we catch + // the wiped seed serving without the sentinel, it re-bootstrapped + // and we can say so precisely instead of burning the full budget. + // Missing it proves nothing: the surviving leader may already have + // snapshotted the member back into consistency, which restores the + // sentinel. The load-bearing assertion is the one below — a seed + // that re-bootstrapped stays Ready and is therefore never replaced, + // so this wait times out either way. + // + // It cannot produce a false failure: it fires only on a successful + // read that returns something other than the value we wrote. + if pod := readyPod(ctx, rebootstrapNamespace, seed); pod != nil { + stdout, _, err := podExec(ctx, rebootstrapNamespace, seed, "etcd", []string{ + "etcdctl", "--endpoints=http://localhost:2379", "get", rebootstrapKey, "--print-value-only", + }) + if err == nil && trimSpace(stdout) != rebootstrapValue { + t.Fatalf("seed %q came up Ready serving an empty keyspace — it re-bootstrapped a fresh "+ + "one-member cluster on the wiped data dir instead of failing to start. "+ + "Clients reaching this member through the %s-client Service see no data. "+ + "(--initial-cluster-state must not be `new` once the cluster has formed)", + seed, rebootstrapCluster) + } + } + err := kube.Get(ctx, client.ObjectKey{Namespace: rebootstrapNamespace, Name: seed}, &etcdv1alpha2.EtcdMember{}) + if apierrors.IsNotFound(err) { + return nil + } + if err != nil { + return err + } + return fmt.Errorf("seed %q still present; it should be crash-looping on the empty data dir "+ + "and then replaced by self-heal", seed) + }) + + waitFor(ctx, t, 10*time.Minute, "cluster back to 3 ready members", + readyMembersIsIn(rebootstrapNamespace, rebootstrapCluster, 3)) + + // The decisive assertion: the data written before the wipe is still there. + // A re-bootstrapped seed would have taken its share of client traffic into + // an empty keyspace. + assertSentinelIntact(ctx, t) + t.Log("wiped seed failed to start, was replaced, and no data was lost") +} + +// putSentinel writes the pre-wipe key via the cluster's client Service. +func putSentinel(ctx context.Context, t *testing.T) { + t.Helper() + pod := anyReadyMemberPod(ctx, t) + endpoint := fmt.Sprintf("http://%s-client.%s.svc:2379", rebootstrapCluster, rebootstrapNamespace) + if _, stderr, err := podExec(ctx, rebootstrapNamespace, pod, "etcd", []string{ + "etcdctl", "--endpoints=" + endpoint, "put", rebootstrapKey, rebootstrapValue, + }); err != nil { + t.Fatalf("etcdctl put sentinel: %v (stderr: %s)", err, stderr) + } +} + +// assertSentinelIntact reads the pre-wipe key back from every ready member, so +// a single member answering out of an empty store is caught rather than being +// averaged away by whichever endpoint the Service happened to pick. +func assertSentinelIntact(ctx context.Context, t *testing.T) { + t.Helper() + pods := &corev1.PodList{} + if err := kube.List(ctx, pods, client.InNamespace(rebootstrapNamespace), + client.MatchingLabels{"etcd-operator.cozystack.io/cluster": rebootstrapCluster}); err != nil { + t.Fatalf("list member pods: %v", err) + } + checked := 0 + for i := range pods.Items { + p := &pods.Items[i] + if !podIsReady(p) { + continue + } + stdout, stderr, err := podExec(ctx, rebootstrapNamespace, p.Name, "etcd", []string{ + "etcdctl", "--endpoints=http://localhost:2379", "get", rebootstrapKey, "--print-value-only", + }) + if err != nil { + t.Fatalf("etcdctl get on %s: %v (stderr: %s)", p.Name, err, stderr) + } + if got := trimSpace(stdout); got != rebootstrapValue { + t.Fatalf("member %s serves %q for the pre-wipe key, want %q — data was lost", p.Name, got, rebootstrapValue) + } + checked++ + } + if checked != 3 { + t.Fatalf("expected to verify the sentinel on 3 ready members, checked %d", checked) + } +} + +// wipeMemberDataDir empties the member's data dir, leaving the volume mounted +// and writable — the "volume came back blank" shape (re-provisioned PV, blank +// restore, node-local storage lost on reimage), as opposed to the corrupt-files +// shape exercised elsewhere. +func wipeMemberDataDir(ctx context.Context, t *testing.T, member string) { + t.Helper() + pod, err := clientset.CoreV1().Pods(rebootstrapNamespace).Get(ctx, member, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get seed pod %q: %v", member, err) + } + pod.Spec.EphemeralContainers = append(pod.Spec.EphemeralContainers, corev1.EphemeralContainer{ + EphemeralContainerCommon: corev1.EphemeralContainerCommon{ + Name: "wipe-data", + Image: "busybox:1.36", + // The emptiness test is the last statement on purpose: it is what + // the container's exit code reports. A bare `ls -A` succeeds + // whether or not the directory is empty, so it would mask a partial + // wipe (say a leftover file this container's UID cannot remove) and + // the test would then fail much later, with the confusing symptom + // of a seed that starts fine, instead of failing here. + // Note the `||` on the listing itself: an unreadable data dir makes + // `ls` fail with empty stdout, which a bare emptiness test would + // read as a successful wipe. + Command: []string{"sh", "-c", + "rm -rf /var/lib/etcd/* /var/lib/etcd/.[!.]* 2>/dev/null; sync; " + + `left=$(ls -A /var/lib/etcd) || { echo "cannot read data dir after wipe" >&2; exit 1; }; ` + + `if [ -n "$left" ]; then echo "wipe incomplete, still present: $left" >&2; exit 1; fi`}, + VolumeMounts: []corev1.VolumeMount{ + {Name: "data", MountPath: "/var/lib/etcd"}, + }, + }, + }) + if _, err := clientset.CoreV1().Pods(rebootstrapNamespace).UpdateEphemeralContainers( + ctx, member, pod, metav1.UpdateOptions{}); err != nil { + t.Fatalf("add wipe-data ephemeral container to %q: %v", member, err) + } + waitFor(ctx, t, 3*time.Minute, "data-dir wipe container finished", func(ctx context.Context) error { + p, err := clientset.CoreV1().Pods(rebootstrapNamespace).Get(ctx, member, metav1.GetOptions{}) + if err != nil { + return err + } + for _, cs := range p.Status.EphemeralContainerStatuses { + if cs.Name != "wipe-data" { + continue + } + if cs.State.Terminated != nil { + if cs.State.Terminated.ExitCode != 0 { + t.Fatalf("wipe-data exited %d (%s); its container log names what survived the wipe", + cs.State.Terminated.ExitCode, cs.State.Terminated.Reason) + } + return nil + } + return fmt.Errorf("wipe-data not finished: %+v", cs.State) + } + return fmt.Errorf("wipe-data status not reported yet") + }) +} + +// anyReadyMemberPod returns the name of a member Pod with a ready etcd +// container, failing the test if there is none. +func anyReadyMemberPod(ctx context.Context, t *testing.T) string { + t.Helper() + pods := &corev1.PodList{} + if err := kube.List(ctx, pods, client.InNamespace(rebootstrapNamespace), + client.MatchingLabels{"etcd-operator.cozystack.io/cluster": rebootstrapCluster}); err != nil { + t.Fatalf("list member pods: %v", err) + } + for i := range pods.Items { + if podIsReady(&pods.Items[i]) { + return pods.Items[i].Name + } + } + t.Fatalf("no ready etcd member pod to probe") + return "" +} + +// readyPod returns the named Pod when its etcd container is ready, else nil. +func readyPod(ctx context.Context, namespace, name string) *corev1.Pod { + pod := &corev1.Pod{} + if err := kube.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, pod); err != nil { + return nil + } + if !podIsReady(pod) { + return nil + } + return pod +} + +func podIsReady(p *corev1.Pod) bool { + if p.Status.Phase != corev1.PodRunning { + return false + } + for _, cs := range p.Status.ContainerStatuses { + if cs.Name == "etcd" && cs.Ready { + return true + } + } + return false +}