diff --git a/pkg/controller/chk/controller.go b/pkg/controller/chk/controller.go index a00184ec0..abf25d791 100644 --- a/pkg/controller/chk/controller.go +++ b/pkg/controller/chk/controller.go @@ -118,6 +118,19 @@ func (c *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, nil } + // A staged joiner whose Pod never started has already exhausted the + // configured startup timeout and persisted Aborted. Child events and + // status updates must not restart the same failed generation. A spec + // edit (e.g. fixing the broken image) increments metadata.Generation + // and permits a new attempt. + if shouldHoldRaftStagedStartupAbort(new) { + log.V(1).M(new).F().Info( + "Keeper staged-joiner startup Aborted for generation %d; wait for CHK spec change", + new.GetGeneration(), + ) + return ctrl.Result{}, nil + } + return c.reconcileResult(new, w.reconcileCR(ctx, nil, new)) } @@ -131,6 +144,12 @@ func (c *Controller) reconcileResult(cr *apiChk.ClickHouseKeeperInstallation, er switch { case err == nil: return ctrl.Result{}, nil + case errors.Is(err, errRaftStagedJoinerStartupTimeout): + // Status was persisted as Aborted by reconcileCR. Do not return an + // error (controller-runtime retries it), and do not explicitly requeue. + // The generation guard above also suppresses status/child-event retries. + log.V(1).M(cr).F().Info("Keeper staged-joiner startup timed out; wait for spec edit") + return ctrl.Result{}, nil case errors.Is(err, common.ErrCRUDDeferred): // Soft defer: quorum headroom not available yet. Status already // records [RaftQuorumUnsafe]; retry on a fixed interval instead of diff --git a/pkg/controller/chk/controller_test.go b/pkg/controller/chk/controller_test.go index d65968087..6679c384f 100644 --- a/pkg/controller/chk/controller_test.go +++ b/pkg/controller/chk/controller_test.go @@ -71,6 +71,15 @@ func TestReconcileResult(t *testing.T) { require.Zero(t, res.RequeueAfter) }) + t.Run("terminal staged startup abort is not requeued", func(t *testing.T) { + err := fmt.Errorf("%w: %w", + common.ErrCRUDAbort, errRaftStagedJoinerStartupTimeout) + res, outErr := c.reconcileResult(cr, err) + require.NoError(t, outErr, + "terminal startup abort was already persisted as Aborted, so runtime must not retry it") + require.Equal(t, ctrl.Result{}, res) + }) + t.Run("arbitrary error is returned verbatim", func(t *testing.T) { boom := errors.New("boom") res, err := c.reconcileResult(cr, boom) @@ -84,3 +93,49 @@ func TestReconcileResult(t *testing.T) { require.Equal(t, raftQuorumDeferredRequeueAfter, res.RequeueAfter) }) } + +// TestHoldRaftStagedStartupAbort ensures status and child-object watch events +// cannot turn a persisted terminal abort back into InProgress. Only a spec +// edit (new Kubernetes generation) should resume an aborted image pull. +func TestHoldRaftStagedStartupAbort(t *testing.T) { + target := &apiChk.ClickHouseKeeperInstallation{ + ObjectMeta: meta.ObjectMeta{Generation: 7}, + } + cr := &apiChk.ClickHouseKeeperInstallation{ + ObjectMeta: meta.ObjectMeta{Namespace: "test", Name: "keeper", Generation: 7}, + Status: &apiChk.Status{ + Status: apiChk.StatusAborted, + NormalizedCR: target, + Errors: []string{raftStagedJoinerStartupTimeoutStatusMarker + " failed to start"}, + }, + } + + t.Run("same spec generation stays aborted after resync", func(t *testing.T) { + require.True(t, shouldHoldRaftStagedStartupAbort(cr)) + }) + t.Run("spec edit permits image recovery", func(t *testing.T) { + updated := cr.DeepCopy() + updated.Generation = 8 + require.False(t, shouldHoldRaftStagedStartupAbort(updated)) + }) + t.Run("unrelated abort retains legacy behavior", func(t *testing.T) { + updated := cr.DeepCopy() + updated.Status.Errors = []string{"unrelated error"} + require.False(t, shouldHoldRaftStagedStartupAbort(updated)) + }) + t.Run("in progress is never held", func(t *testing.T) { + updated := cr.DeepCopy() + updated.Status.Status = apiChk.StatusInProgress + require.False(t, shouldHoldRaftStagedStartupAbort(updated)) + }) + t.Run("missing last-attempt generation is not a permanent lock", func(t *testing.T) { + updated := cr.DeepCopy() + updated.Status.NormalizedCR = nil + require.False(t, shouldHoldRaftStagedStartupAbort(updated)) + }) + t.Run("legacy zero generation is not a permanent lock", func(t *testing.T) { + updated := cr.DeepCopy() + updated.Status.NormalizedCR.Generation = 0 + require.False(t, shouldHoldRaftStagedStartupAbort(updated)) + }) +} diff --git a/pkg/controller/chk/raftplan/admission.go b/pkg/controller/chk/raftplan/admission.go new file mode 100644 index 000000000..6980fc972 --- /dev/null +++ b/pkg/controller/chk/raftplan/admission.go @@ -0,0 +1,171 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package raftplan + +// PlanAdmission determines which Keeper hosts may be published into +// raft_configuration and which missing StatefulSets may be created. +// +// It preserves the legacy staged-admission policy for established ensembles +// with fewer than three ancestor members. Existing published members are +// never removed merely because their StatefulSet has disappeared. Deletion +// and scale-down are outside this policy and require a separate safety gate. +// +// The caller supplies DesiredHosts in stable server-ID order and must +// provide an independently obtained, live snapshot. The function never +// modifies any of its inputs. +func PlanAdmission(s Snapshot) AdmissionPlan { + plan := AdmissionPlan{ + MayCreate: make(map[HostID]bool), + Reason: AdmissionAllowed, + } + + // Preservation is the safe default. An unreadable ConfigMap cannot be + // interpreted as an empty member set. + included := unique(s.PublishedHosts) + if !s.PublishedKnown { + plan.Reason = ObservationUnknown + plan.IncludedHosts = included + return plan + } + + // A stopped CHK may reconcile existing resources but must not create a + // new member. Preserve precisely what was already published. + if s.Stopped { + plan.IncludedHosts = included + plan.Reason = CHKStopped + return plan + } + + // Unknown StatefulSet existence for any requested member must not be + // converted into an admission or a membership removal. + for _, id := range unique(s.DesiredHosts) { + host, ok := s.Hosts[id] + if !ok || host.StatefulSet == PresenceUnknown { + plan.IncludedHosts = included + plan.Reason = ObservationUnknown + return plan + } + } + + // Existing voters and members with live StatefulSets are preserved, + // including after an interrupted creation and when normalizedCompleted + // hasn't yet caught up with the current live topology. + included = appendUnique(included, s.AncestorHosts...) + for _, id := range unique(s.DesiredHosts) { + if s.Hosts[id].StatefulSet == PresencePresent && + (!s.Established || len(unique(s.AncestorHosts)) >= 3 || + (s.Hosts[id].PodStarted && s.Hosts[id].StartedKnown) || + (s.Hosts[id].Ready && s.Hosts[id].ReadyKnown)) { + included = appendUnique(included, id) + } + } + + // Fresh bootstrap and 3->N intentionally retain the existing fan-out + // policy. The safe serial join policy applies only to an established + // ensemble with fewer than three ancestor members. + if !s.Established || len(unique(s.AncestorHosts)) >= 3 { + for _, id := range unique(s.DesiredHosts) { + included = appendUnique(included, id) + if s.Hosts[id].StatefulSet == PresenceAbsent { + plan.MayCreate[id] = true + } + } + return finalizeAdmission(plan, included, s.PublishedHosts) + } + + ancestor := make(map[HostID]bool, len(s.AncestorHosts)) + for _, id := range s.AncestorHosts { + ancestor[id] = true + } + + // A previous joiner with an existing unready StatefulSet blocks the + // next absent joiner. The existing hosts remain eligible for safe + // reconciliation so they can adopt the membership configuration. + for _, id := range unique(s.DesiredHosts) { + if ancestor[id] { + continue + } + h := s.Hosts[id] + if h.StatefulSet == PresencePresent && (!h.ReadyKnown || !h.Ready) { + plan.BlockedBy = id + plan.Reason = WaitingForJoiner + return finalizeAdmission(plan, included, s.PublishedHosts) + } + } + + // If a previously published joiner has no StatefulSet, repair that + // member before admitting a different one. Do not retract membership on + // an ambiguous create response or skip over a stranded admission. + for _, id := range unique(s.DesiredHosts) { + if ancestor[id] { + continue + } + if s.Hosts[id].StatefulSet == PresenceAbsent && contains(included, id) { + plan.MayCreate[id] = true + return finalizeAdmission(plan, included, s.PublishedHosts) + } + } + + // Admit at most one new absent member in stable ID order. + for _, id := range unique(s.DesiredHosts) { + if ancestor[id] { + continue + } + if s.Hosts[id].StatefulSet == PresenceAbsent { + // Create with a host-scoped bootstrap config first; publishing + // now could exhaust Keeper's retries while the Pod is offline. + plan.MayCreate[id] = true + break + } + } + return finalizeAdmission(plan, included, s.PublishedHosts) +} + +func finalizeAdmission(plan AdmissionPlan, included, published []HostID) AdmissionPlan { + plan.IncludedHosts = unique(included) + plan.ShouldPublish = !equalMembers(plan.IncludedHosts, published) + return plan +} + +func contains(ids []HostID, want HostID) bool { + for _, id := range ids { + if id == want { + return true + } + } + return false +} + +func unique(ids []HostID) []HostID { + seen := make(map[HostID]bool, len(ids)) + result := make([]HostID, 0, len(ids)) + for _, id := range ids { + if !seen[id] { + seen[id] = true + result = append(result, id) + } + } + return result +} + +func appendUnique(ids []HostID, extra ...HostID) []HostID { + for _, id := range extra { + if !contains(ids, id) { + ids = append(ids, id) + } + } + return ids +} + +func equalMembers(left, right []HostID) bool { + left, right = unique(left), unique(right) + if len(left) != len(right) { + return false + } + for _, id := range left { + if !contains(right, id) { + return false + } + } + return true +} diff --git a/pkg/controller/chk/raftplan/admission_test.go b/pkg/controller/chk/raftplan/admission_test.go new file mode 100644 index 000000000..5e6160d54 --- /dev/null +++ b/pkg/controller/chk/raftplan/admission_test.go @@ -0,0 +1,169 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package raftplan + +import ( + "reflect" + "testing" +) + +func baseline(desired, ancestor []HostID) Snapshot { + s := Snapshot{ + DesiredHosts: append([]HostID(nil), desired...), + AncestorHosts: append([]HostID(nil), ancestor...), + Established: len(ancestor) != 0, + PublishedHosts: append([]HostID(nil), ancestor...), + PublishedKnown: true, + Hosts: make(map[HostID]HostState), + } + for _, id := range desired { + s.Hosts[id] = HostState{ID: id, InAncestor: contains(ancestor, id), StatefulSet: PresenceAbsent, ReadyKnown: true} + } + for _, id := range ancestor { + s.Hosts[id] = HostState{ID: id, InAncestor: true, StatefulSet: PresencePresent, ReadyKnown: true, Ready: true} + } + return s +} + +func setHost(s *Snapshot, id HostID, presence Presence, ready, readyKnown bool) { + h := s.Hosts[id] + h.StatefulSet, h.Ready, h.ReadyKnown = presence, ready, readyKnown + s.Hosts[id] = h +} + +func TestPlanAdmission(t *testing.T) { + ids := []HostID{"h0", "h1", "h2"} + tests := []struct { + name string + input func() Snapshot + include []HostID + create []HostID + blocked HostID + reason AdmissionReason + publish bool + }{ + {"fresh bootstrap", func() Snapshot { return baseline(ids, nil) }, ids, []HostID{"h0", "h1", "h2"}, "", AdmissionAllowed, true}, + {"1 to 3 start", func() Snapshot { return baseline(ids, ids[:1]) }, ids[:1], []HostID{"h1"}, "", AdmissionAllowed, false}, + {"1 to 3 pending", func() Snapshot { + s := baseline(ids, ids[:1]) + s.PublishedHosts = ids[:2] + setHost(&s, "h1", PresencePresent, false, true) + return s + }, ids[:2], nil, "h1", WaitingForJoiner, false}, + {"1 to 3 pending with unknown readiness", func() Snapshot { + s := baseline(ids, ids[:1]) + s.PublishedHosts = ids[:2] + setHost(&s, "h1", PresencePresent, false, false) + return s + }, ids[:2], nil, "h1", WaitingForJoiner, false}, + {"1 to 3 next admit", func() Snapshot { + s := baseline(ids, ids[:1]) + s.PublishedHosts = ids[:2] + setHost(&s, "h1", PresencePresent, true, true) + return s + }, ids[:2], []HostID{"h2"}, "", AdmissionAllowed, false}, + {"2 to 3", func() Snapshot { return baseline(ids, ids[:2]) }, ids[:2], []HostID{"h2"}, "", AdmissionAllowed, false}, + {"missing published joiner is repaired first", func() Snapshot { + s := baseline(ids, ids[:1]) + s.PublishedHosts = ids[:2] + return s + }, ids[:2], []HostID{"h1"}, "", AdmissionAllowed, false}, + {"unknown sts blocks admission", func() Snapshot { + s := baseline(ids, ids[:1]) + setHost(&s, "h1", PresenceUnknown, false, false) + return s + }, ids[:1], nil, "", ObservationUnknown, false}, + {"unreadable publication blocks admission", func() Snapshot { + s := baseline(ids, ids[:1]) + s.PublishedKnown = false + return s + }, ids[:1], nil, "", ObservationUnknown, false}, + {"stop retains existing publication", func() Snapshot { + s := baseline(ids, ids[:1]) + s.Stopped = true + s.PublishedHosts = ids[:2] + return s + }, ids[:2], nil, "", CHKStopped, false}, + {"3 to N remains fanout", func() Snapshot { + all := []HostID{"h0", "h1", "h2", "h3", "h4"} + return baseline(all, all[:3]) + }, []HostID{"h0", "h1", "h2", "h3", "h4"}, []HostID{"h3", "h4"}, "", AdmissionAllowed, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s := tc.input() + before := cloneSnapshot(s) + p := PlanAdmission(s) + if !reflect.DeepEqual(p.IncludedHosts, tc.include) { + t.Errorf("IncludedHosts=%v, want %v", p.IncludedHosts, tc.include) + } + for id := range p.MayCreate { + if !contains(tc.create, id) { + t.Errorf("unexpected creation allowance: %s", id) + } + } + for _, id := range tc.create { + if !p.MayCreate[id] { + t.Errorf("missing creation allowance: %s", id) + } + } + if p.BlockedBy != tc.blocked || p.Reason != tc.reason || p.ShouldPublish != tc.publish { + t.Errorf("plan=%+v, want blocked=%q reason=%q publish=%v", p, tc.blocked, tc.reason, tc.publish) + } + if !reflect.DeepEqual(s, before) { + t.Fatal("planner mutated its input") + } + if !reflect.DeepEqual(p, PlanAdmission(s)) { + t.Fatal("planner output changed without new observations") + } + }) + } +} + +func TestPlanAdmissionDefersPublicationUntilPodStarted(t *testing.T) { + s := baseline([]HostID{"h0", "h1", "h2"}, []HostID{"h0"}) + setHost(&s, "h1", PresencePresent, false, true) + notStarted := PlanAdmission(s) + if contains(notStarted.IncludedHosts, "h1") || notStarted.ShouldPublish || notStarted.MayCreate["h2"] { + t.Fatalf("existing but unstarted joiner admitted: %+v", notStarted) + } + h := s.Hosts["h1"] + h.StartedKnown, h.PodStarted = true, true + s.Hosts["h1"] = h + started := PlanAdmission(s) + if !contains(started.IncludedHosts, "h1") || !started.ShouldPublish || started.MayCreate["h2"] { + t.Fatalf("started joiner not published or next voter admitted early: %+v", started) + } +} + +func TestPlanAdmissionPreservesOutOfDesiredPublishedMember(t *testing.T) { + s := baseline([]HostID{"h0", "h1"}, []HostID{"h0"}) + s.PublishedHosts = []HostID{"h0", "h3"} // scale-down must not be implicit + plan := PlanAdmission(s) + if !contains(plan.IncludedHosts, "h3") { + t.Fatalf("existing published voter dropped: %v", plan.IncludedHosts) + } +} + +func TestPlanAdmissionOneToFiveSerial(t *testing.T) { + ids := []HostID{"h0", "h1", "h2", "h3", "h4"} + s := baseline(ids, ids[:1]) + s.PublishedHosts = ids[:2] + setHost(&s, "h1", PresencePresent, false, true) + p := PlanAdmission(s) + if !reflect.DeepEqual(p.IncludedHosts, ids[:2]) || len(p.MayCreate) != 0 || p.BlockedBy != "h1" { + t.Fatalf("unexpected additional joiner while h1 blocked: %+v", p) + } +} + +func cloneSnapshot(s Snapshot) Snapshot { + out := s + out.DesiredHosts = append([]HostID(nil), s.DesiredHosts...) + out.AncestorHosts = append([]HostID(nil), s.AncestorHosts...) + out.PublishedHosts = append([]HostID(nil), s.PublishedHosts...) + out.Hosts = make(map[HostID]HostState, len(s.Hosts)) + for id, state := range s.Hosts { + out.Hosts[id] = state + } + return out +} diff --git a/pkg/controller/chk/raftplan/disruption.go b/pkg/controller/chk/raftplan/disruption.go new file mode 100644 index 000000000..54dd97dbf --- /dev/null +++ b/pkg/controller/chk/raftplan/disruption.go @@ -0,0 +1,33 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package raftplan + +// CheckDisruption evaluates an already characterized proposed host mutation. +// Rolling classification and MemberCount/ReadyCount intentionally follow the +// legacy quorumSizingEnsemble policy in this initial, behavior-neutral phase. +// It does not acquire a lock: the worker must serialize disruptions and +// reobserve immediately before applying the decision. +func CheckDisruption(s Snapshot, hostID HostID) DisruptionDecision { + h, ok := s.Hosts[hostID] + if !ok { + return DisruptionDecision{Reason: DisruptionStateUnknown} + } + if !h.WouldDisrupt || s.Stopped { + return DisruptionDecision{Allowed: true, Reason: DisruptionNotNeeded} + } + if !s.CountsKnown || !h.ReadyKnown || s.MemberCount < 0 || s.ReadyCount < 0 || s.ReadyCount > s.MemberCount { + return DisruptionDecision{Reason: DisruptionStateUnknown} + } + + // These exceptions intentionally mirror the existing 3+ member gate. + // Protecting a one- or two-member ensemble from any disruption requires + // an explicit availability policy rather than changing it in this refactor. + if !s.Rolling || s.MemberCount < 3 || !h.Ready { + return DisruptionDecision{Allowed: true, Reason: DisruptionSafe} + } + + if s.ReadyCount-1 < s.MemberCount/2+1 { + return DisruptionDecision{Reason: DisruptionQuorumUnsafe} + } + return DisruptionDecision{Allowed: true, Reason: DisruptionSafe} +} diff --git a/pkg/controller/chk/raftplan/disruption_test.go b/pkg/controller/chk/raftplan/disruption_test.go new file mode 100644 index 000000000..e4f49fa1c --- /dev/null +++ b/pkg/controller/chk/raftplan/disruption_test.go @@ -0,0 +1,56 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package raftplan + +import "testing" + +func TestCheckDisruption(t *testing.T) { + tests := []struct { + name string + memberCount int + readyCount int + ready bool + readyKnown bool + countsKnown bool + rolling bool + wouldDisrupt bool + stopped bool + wantAllowed bool + wantReason DisruptionReason + }{ + {"all ready 3 -> allow one", 3, 3, true, true, true, true, true, false, true, DisruptionSafe}, + {"two ready 3 -> refuse", 3, 2, true, true, true, true, true, false, false, DisruptionQuorumUnsafe}, + {"all ready 5 -> allow one", 5, 5, true, true, true, true, true, false, true, DisruptionSafe}, + {"three ready 5 -> refuse", 5, 3, true, true, true, true, true, false, false, DisruptionQuorumUnsafe}, + {"two ready 5, already unready target", 5, 2, false, true, true, true, true, false, true, DisruptionSafe}, + {"existing same", 3, 2, true, true, true, true, false, false, true, DisruptionNotNeeded}, + {"stopped", 3, 2, true, true, true, true, true, true, true, DisruptionNotNeeded}, + {"two-member legacy exception", 2, 2, true, true, true, true, true, false, true, DisruptionSafe}, + {"nonrolling bootstrap", 3, 1, true, true, true, false, true, false, true, DisruptionSafe}, + {"quorum count unknown", 3, 0, true, true, false, true, true, false, false, DisruptionStateUnknown}, + {"target readiness unknown", 3, 3, false, false, true, true, true, false, false, DisruptionStateUnknown}, + {"inconsistent counts unknown", 3, 4, true, true, true, true, true, false, false, DisruptionStateUnknown}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s := Snapshot{ + MemberCount: tc.memberCount, ReadyCount: tc.readyCount, + CountsKnown: tc.countsKnown, Rolling: tc.rolling, Stopped: tc.stopped, + Hosts: map[HostID]HostState{ + "h0": {ID: "h0", Ready: tc.ready, ReadyKnown: tc.readyKnown, WouldDisrupt: tc.wouldDisrupt}, + }, + } + got := CheckDisruption(s, "h0") + if got.Allowed != tc.wantAllowed || got.Reason != tc.wantReason { + t.Errorf("CheckDisruption=%+v, want allow=%v reason=%s", got, tc.wantAllowed, tc.wantReason) + } + }) + } +} + +func TestCheckDisruptionMissingHost(t *testing.T) { + got := CheckDisruption(Snapshot{}, "missing") + if got.Allowed || got.Reason != DisruptionStateUnknown { + t.Fatalf("missing host incorrectly allowed: %+v", got) + } +} diff --git a/pkg/controller/chk/raftplan/join.go b/pkg/controller/chk/raftplan/join.go new file mode 100644 index 000000000..7a8846b7d --- /dev/null +++ b/pkg/controller/chk/raftplan/join.go @@ -0,0 +1,68 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package raftplan + +// JoinPhase is reconstructed from live observations after every reconcile +// or controller restart. It is never persisted in memory or the CHK status. +type JoinPhase string + +const ( + JoinUnknown JoinPhase = "Unknown" + JoinPending JoinPhase = "Pending" + JoinStarting JoinPhase = "Starting" + JoinPublishing JoinPhase = "Publishing" + JoinJoining JoinPhase = "Joining" + JoinReady JoinPhase = "Ready" +) + +type JoinAction string + +const ( + JoinWait JoinAction = "Wait" + JoinCreate JoinAction = "CreateStatefulSet" + JoinPublish JoinAction = "PublishMembership" + JoinVerify JoinAction = "VerifyMembership" + JoinAdmitNext JoinAction = "AdmitNext" +) + +type JoinObservation struct { + StatefulSet Presence + StartedKnown bool + Started bool + PublishedKnown bool + Published bool + CommittedKnown bool + Committed bool + ReadyKnown bool + Ready bool +} + +type JoinDecision struct { + Phase JoinPhase + Action JoinAction +} + +// PlanJoin permits publication only after Pod Started, and admission of the +// next voter only after committed membership and readiness are observed. +func PlanJoin(s JoinObservation) JoinDecision { + switch { + case s.StatefulSet == PresenceUnknown: + return JoinDecision{JoinUnknown, JoinWait} + case s.StatefulSet == PresenceAbsent: + return JoinDecision{JoinPending, JoinCreate} + case !s.StartedKnown: + return JoinDecision{JoinUnknown, JoinWait} + case !s.Started: + return JoinDecision{JoinStarting, JoinWait} + case !s.PublishedKnown: + return JoinDecision{JoinUnknown, JoinWait} + case !s.Published: + return JoinDecision{JoinPublishing, JoinPublish} + case !s.CommittedKnown || !s.Committed: + return JoinDecision{JoinJoining, JoinVerify} + case !s.ReadyKnown || !s.Ready: + return JoinDecision{JoinJoining, JoinWait} + default: + return JoinDecision{JoinReady, JoinAdmitNext} + } +} diff --git a/pkg/controller/chk/raftplan/join_test.go b/pkg/controller/chk/raftplan/join_test.go new file mode 100644 index 000000000..69a51d58a --- /dev/null +++ b/pkg/controller/chk/raftplan/join_test.go @@ -0,0 +1,32 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package raftplan + +import "testing" + +func TestPlanJoinTransitionsAndRecovery(t *testing.T) { + cases := []struct { + name string + observed JoinObservation + want JoinDecision + }{ + {"unknown STS", JoinObservation{}, JoinDecision{JoinUnknown, JoinWait}}, + {"pending", JoinObservation{StatefulSet: PresenceAbsent}, JoinDecision{JoinPending, JoinCreate}}, + {"unknown startup", JoinObservation{StatefulSet: PresencePresent}, JoinDecision{JoinUnknown, JoinWait}}, + {"image pull", JoinObservation{StatefulSet: PresencePresent, StartedKnown: true}, JoinDecision{JoinStarting, JoinWait}}, + {"unknown publication", JoinObservation{StatefulSet: PresencePresent, StartedKnown: true, Started: true}, JoinDecision{JoinUnknown, JoinWait}}, + {"started, unpublished", JoinObservation{StatefulSet: PresencePresent, StartedKnown: true, Started: true, PublishedKnown: true}, JoinDecision{JoinPublishing, JoinPublish}}, + {"published, not committed", JoinObservation{StatefulSet: PresencePresent, StartedKnown: true, Started: true, PublishedKnown: true, Published: true, CommittedKnown: true}, JoinDecision{JoinJoining, JoinVerify}}, + {"live membership unknown", JoinObservation{StatefulSet: PresencePresent, StartedKnown: true, Started: true, PublishedKnown: true, Published: true}, JoinDecision{JoinJoining, JoinVerify}}, + {"committed, syncing", JoinObservation{StatefulSet: PresencePresent, StartedKnown: true, Started: true, PublishedKnown: true, Published: true, CommittedKnown: true, Committed: true, ReadyKnown: true}, JoinDecision{JoinJoining, JoinWait}}, + {"ready", JoinObservation{StatefulSet: PresencePresent, StartedKnown: true, Started: true, PublishedKnown: true, Published: true, CommittedKnown: true, Committed: true, ReadyKnown: true, Ready: true}, JoinDecision{JoinReady, JoinAdmitNext}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := PlanJoin(tc.observed) + if got != tc.want || PlanJoin(tc.observed) != got { + t.Fatalf("PlanJoin(%+v)=%+v, want %+v", tc.observed, got, tc.want) + } + }) + } +} diff --git a/pkg/controller/chk/raftplan/published.go b/pkg/controller/chk/raftplan/published.go new file mode 100644 index 000000000..c8dea6fd4 --- /dev/null +++ b/pkg/controller/chk/raftplan/published.go @@ -0,0 +1,99 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package raftplan + +import ( + "encoding/xml" + "fmt" + "regexp" + "sort" + "strings" +) + +// raftBlock isolates the Raft section because a common ConfigMap may also +// contain unrelated configuration files that do not parse as XML. +var raftBlock = regexp.MustCompile(`(?s)]*)?\s*>.*?`) + +type raftSection struct { + Servers []struct { + ID string `xml:"id"` + Hostname string `xml:"hostname"` + } `xml:"server"` +} + +// PublishedHostnames extracts the Raft hostnames from a live common ConfigMap. +// known is false if no Raft section was found, which must never be interpreted +// as an empty published membership. A confirmed ConfigMap NotFound is handled +// by the caller, not here. Inputs are treated as read-only. +func PublishedHostnames(data map[string]string) (hostnames []string, known bool, err error) { + if len(data) == 0 { + return nil, false, nil + } + keys := make([]string, 0, len(data)) + for key := range data { + keys = append(keys, key) + } + sort.Strings(keys) + var reference []string + for _, key := range keys { + blocks := raftBlock.FindAllString(data[key], -1) + for _, block := range blocks { + var section raftSection + if err := xml.Unmarshal([]byte(block), §ion); err != nil { + return nil, false, fmt.Errorf("ConfigMap %s Raft XML: %w", key, err) + } + // User settings can render a partial raft_configuration, e.g. + // keeper_server/raft_configuration/server/port. That produces + // 9444 in chop-generated-settings.xml; + // it is not a published member and must not be parsed as one. + // Still inspect settings sections with member identifiers so a + // conflicting or malformed membership cannot be silently ignored. + if key == "chop-generated-settings.xml" { + hasMemberIdentifier := false + for _, server := range section.Servers { + if strings.TrimSpace(server.ID) != "" || strings.TrimSpace(server.Hostname) != "" { + hasMemberIdentifier = true + break + } + } + if !hasMemberIdentifier { + continue + } + } + names := make([]string, 0, len(section.Servers)) + seen := map[string]bool{} + for _, server := range section.Servers { + name := strings.TrimSpace(server.Hostname) + if name == "" || seen[name] { + return nil, false, fmt.Errorf("ConfigMap %s contains blank or duplicate Raft hostname %q", key, name) + } + seen[name] = true + names = append(names, name) + } + if known && !equalStrings(reference, names) { + return nil, false, fmt.Errorf("conflicting raft_configuration sections in ConfigMap") + } + reference, known = names, true + } + } + if known && reference == nil { + reference = []string{} + } + return reference, known, nil +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + counts := make(map[string]bool, len(a)) + for _, v := range a { + counts[v] = true + } + for _, v := range b { + if !counts[v] { + return false + } + } + return true +} diff --git a/pkg/controller/chk/raftplan/published_test.go b/pkg/controller/chk/raftplan/published_test.go new file mode 100644 index 000000000..f65328ed1 --- /dev/null +++ b/pkg/controller/chk/raftplan/published_test.go @@ -0,0 +1,67 @@ +package raftplan + +import ( + "reflect" + "testing" +) + +func TestPublishedHostnames(t *testing.T) { + tests := []struct { + name string + data map[string]string + want []string + known bool + fails bool + }{ + {name: "no config map data"}, + {name: "no Raft section", data: map[string]string{"other.xml": ""}}, + {name: "valid Raft section", data: map[string]string{ + "keeper.xml": "1h0.ns.svch1.ns.svc", + "users.xml": "not xml <", + }, want: []string{"h0.ns.svc", "h1.ns.svc"}, known: true}, + {name: "empty valid section", data: map[string]string{"raft.xml": ""}, want: []string{}, known: true}, + {name: "duplicate hostname", data: map[string]string{"raft.xml": "h0h0"}, fails: true}, + {name: "empty hostname", data: map[string]string{"raft.xml": "2"}, fails: true}, + {name: "settings port-only overlay before actual published membership", data: map[string]string{ + "chop-generated-settings.xml": "9444", + "chop-generated-raft.xml": "0h0.ns.svc9444", + }, want: []string{"h0.ns.svc"}, known: true}, + {name: "settings port-only overlay without published membership is unknown", data: map[string]string{ + "chop-generated-settings.xml": "9444", + }}, + {name: "settings overlay does not hide invalid generated membership", data: map[string]string{ + "chop-generated-settings.xml": "9444", + "chop-generated-raft.xml": "0", + }, fails: true}, + {name: "settings overlay with a hostname still checks conflicting membership", data: map[string]string{ + "chop-generated-settings.xml": "unexpected", + "chop-generated-raft.xml": "h0.ns.svc", + }, fails: true}, + {name: "malformed XML", data: map[string]string{"raft.xml": ""}, fails: true}, + {name: "conflicting blocks", data: map[string]string{ + "a.xml": "h0", + "b.xml": "h1", + }, fails: true}, + {name: "identical blocks", data: map[string]string{ + "a.xml": "h0", + "b.xml": "h0", + }, want: []string{"h0"}, known: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, known, err := PublishedHostnames(tt.data) + if (err != nil) != tt.fails { + t.Fatalf("err = %v, want failure %v", err, tt.fails) + } + if tt.fails { + return + } + if known != tt.known { + t.Errorf("known=%v, want %v", known, tt.known) + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("hosts=%v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/controller/chk/raftplan/types.go b/pkg/controller/chk/raftplan/types.go new file mode 100644 index 000000000..05b1aba64 --- /dev/null +++ b/pkg/controller/chk/raftplan/types.go @@ -0,0 +1,96 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package raftplan contains deterministic, side-effect-free CHK Raft safety decisions. +// +// Inputs are immutable observations supplied by the CHK worker. The package +// intentionally does not import Kubernetes or the CHK API. In the first +// migration step it is evaluated in shadow mode: existing reconciliation +// remains authoritative until integration parity tests have passed. +package raftplan + +type HostID string + +type Presence uint8 + +const ( + PresenceUnknown Presence = iota + PresenceAbsent + PresencePresent +) + +type HostState struct { + ID HostID + + // InAncestor means present in the last successfully completed topology. + InAncestor bool + + StatefulSet Presence + Ready bool + ReadyKnown bool + PodStarted bool + StartedKnown bool + + // WouldDisrupt is computed by the adapter from the actual proposed STS + // mutation, including force-recreate and force-restart options. + WouldDisrupt bool +} + +type Snapshot struct { + // Host IDs in stable Keeper server-ID order, with no duplicates. + DesiredHosts []HostID + AncestorHosts []HostID + Established bool + Stopped bool + + Hosts map[HostID]HostState + + // PublishedHosts is what the current common ConfigMap contains. These + // fields let the planner avoid silently dropping a previously published + // member when a StatefulSet create result is ambiguous. If the ConfigMap + // could not be read, new membership publication must not be attempted. + PublishedHosts []HostID + PublishedKnown bool + + // Same ensemble denominator as quorumSizingEnsemble in the legacy gate. + Rolling bool + MemberCount int + ReadyCount int + CountsKnown bool +} + +type AdmissionReason string + +const ( + AdmissionAllowed AdmissionReason = "Allowed" + WaitingForJoiner AdmissionReason = "JoinerNotReady" + CHKStopped AdmissionReason = "Stopped" + ObservationUnknown AdmissionReason = "ObservationUnknown" +) + +type AdmissionPlan struct { + // IncludedHosts is the desired published membership, in stable order. + IncludedHosts []HostID + // MayCreate controls newly absent StatefulSets, not existing STS repairs. + MayCreate map[HostID]bool + BlockedBy HostID + Reason AdmissionReason + // ShouldPublish is true only when the existing publication is known and + // differs from IncludedHosts. A false value with ObservationUnknown means + // do not touch publication, not that it already matches. + ShouldPublish bool +} + +type DisruptionReason string + +const ( + DisruptionSafe DisruptionReason = "Safe" + DisruptionNotNeeded DisruptionReason = "NotNeeded" + DisruptionQuorumUnsafe DisruptionReason = "QuorumUnsafe" + DisruptionStateUnknown DisruptionReason = "StateUnknown" +) + +type DisruptionDecision struct { + Allowed bool + Reason DisruptionReason +} diff --git a/pkg/controller/chk/worker-raft-safety.go b/pkg/controller/chk/worker-raft-safety.go index 5f49884e8..200772f39 100644 --- a/pkg/controller/chk/worker-raft-safety.go +++ b/pkg/controller/chk/worker-raft-safety.go @@ -28,7 +28,9 @@ import ( "github.com/altinity/clickhouse-operator/pkg/apis/common/types" "github.com/altinity/clickhouse-operator/pkg/controller/common" a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" "github.com/altinity/clickhouse-operator/pkg/controller/common/statefulset" + "github.com/altinity/clickhouse-operator/pkg/model/k8s" "github.com/altinity/clickhouse-operator/pkg/interfaces" "github.com/altinity/clickhouse-operator/pkg/util" ) @@ -103,9 +105,9 @@ func crHasEstablishedCluster(cr api.ICustomResource) bool { // stageUncommittedScaleUpHosts keeps not-yet-created members out of // raft_configuration on 1→N / 2→3, so the published XML lists at most one -// extra server while the previous join is still coming up. Only hosts whose -// StatefulSet is confirmed absent are tagged: an interrupted pass must not -// re-exclude a member that is already running. A Get error other than NotFound +// extra server while the previous join is still coming up. Hosts without a +// StatefulSet, or with a StatefulSet whose Pod has not Started, remain staged. +// Previously published members are never re-excluded on an interrupted pass. A Get error other than NotFound // fails closed — nothing is staged and the caller must requeue rather than // publish an expanded membership. Fresh bootstrap and 3→N (ancestor already // has quorum headroom) are left alone. @@ -117,20 +119,60 @@ func (w *worker) stageUncommittedScaleUpHosts(ctx context.Context, cr *apiChk.Cl return nil } + // Read the actual common ConfigMap before deciding whether an existing + // StatefulSet is still in the bootstrap phase. Never retract an already + // published voter, including a previously interrupted creation. + published := map[raftplan.HostID]bool{} + publishedKnown := false + if w.c.APIReader != nil && w.c.namer != nil && w.task != nil { + observed, err := w.observeRaftPlanAdmission(ctx, cr) + if err != nil { + return err + } + publishedKnown = observed.PublishedKnown + for _, id := range observed.PublishedHosts { + published[id] = true + } + } + var toStage []*api.Host var getErr error _ = cr.WalkHosts(func(host *api.Host) error { if host == nil || !host.GetReconcileAttributes().GetStatus().Is(types.ObjectStatusRequested) { return nil } - // Same predicate as rollbackStagedRaftAdmit: absent means NotFound. sts, err := w.stagedJoinerStatefulSet(ctx, host) if err != nil { getErr = err return err } + if publishedKnown && published[raftPlanHostID(host)] { + return nil + } if sts == nil { toStage = append(toStage, host) + return nil + } + // On spec repair, the failed joiner's STS already exists but its Pod + // may never have started. Preserve the bootstrap-only membership. + if !publishedKnown || w.c.kube.Pod() == nil { + return nil // test-only adapters without Pod/CM observations + } + pod, err := w.c.kube.Pod().Get(ctx, host) + if apiErrors.IsNotFound(err) { + toStage = append(toStage, host) + return nil + } + if err != nil { + getErr = err + return err + } + if pod == nil { + getErr = fmt.Errorf("nil staged Keeper Pod for %s", host.GetName()) + return getErr + } + if !k8s.PodHasAllContainersStarted(pod) { + toStage = append(toStage, host) } return nil }) @@ -152,10 +194,8 @@ func (w *worker) stageUncommittedScaleUpHosts(ctx context.Context, cr *apiChk.Cl // ancestor without headroom, and this host is absent from that ancestor — not // ObjectStatusRequested. PrepareHostStatefulSetWithStatus overwrites Requested // to Same/Modified once the joiner STS exists, so a retry must not treat that -// host as still waiting to be admitted. deferUnreadyStagedAdmit uses this to -// postpone the next not-yet-created joiner until this one's live ReadyReplicas -// is positive. Ready is not committed Raft membership (see -// verifyHostEnsembleMembership). 3→N is not staged. +// host as absent. The admission gate defers the next joiner until this one +// becomes Ready; the join verifier also checks Raft membership. 3→N is not staged. func hostIsStagedScaleUpJoin(host *api.Host) bool { if host == nil || host.GetCR() == nil { return false @@ -169,8 +209,8 @@ func hostIsStagedScaleUpJoin(host *api.Host) bool { return !ensembleHasQuorumHeadroom(host.GetCR().GetAncestor().HostsCount()) } -// deferUnreadyStagedAdmit postpones a not-yet-created 1→N / 2→3 joiner while -// another staged joiner already has a StatefulSet that is not Ready. +// deferUnreadyStagedAdmit is the optional legacy admission fallback for +// 1→N / 2→3. The authoritative path uses raftplan.PlanAdmission. // // The check is live ReadyReplicas, not ObjectStatus and not WaitUntilReady. // After an interrupted pass the previous joiner's StatefulSet exists, so @@ -416,7 +456,11 @@ func (w *worker) ensureQuorumSafeToDisruptHost( if snap == nil || !snap.rolling || !ensembleHasQuorumHeadroom(snap.members) { return nil } - if !w.hostDisruptionWouldBreakQuorum(ctx, host, opts, *snap) { + blocked, err := w.raftPlanDisruptionBlocked(ctx, host, opts, *snap) + if err != nil { + return err + } + if !blocked { return nil } @@ -465,7 +509,11 @@ func (w *worker) ensureQuorumSafeToDisruptHost( if err := w.refreshQuorumSnapshotCounts(ctx, host, snap); err != nil { return err } - if !w.hostDisruptionWouldBreakQuorum(ctx, host, opts, *snap) { + blocked, err = w.raftPlanDisruptionBlocked(ctx, host, opts, *snap) + if err != nil { + return err + } + if !blocked { w.a.V(1).M(host).F().Info( "Raft quorum headroom available — proceeding with host %s disruption (ready=%d)", host.GetName(), snap.readyCount, @@ -524,12 +572,10 @@ func (w *worker) isHostHealthyForReconcile(ctx context.Context, host *api.Host) // // /ready on a newly created joiner is not a committed-membership barrier, and it // is not how 1→N / 2→3 serializes creates. A new peer cannot reach /ready until -// existing members reload or roll onto the published file, and recovery-first -// reconciles that peer before they do, so SetWaitUntilReady there expires into -// ErrCRUDAbort. deferUnreadyStagedAdmit postpones the next missing joiner until -// the previous joiner's live ReadyReplicas is positive, and that postponement -// still lets this pass roll the existing members. verifyHostEnsembleMembership -// is the extension point for /keeper/config + mntr. +// existing members reload the published file, and recovery-first reconciles +// that peer before they do, so SetWaitUntilReady there expires into ErrCRUDAbort. +// The admission gate postpones the next missing joiner until the previous one +// is Ready; the post-publication gate checks live membership and Pod readiness. // // Sizing growth on the desired set inflates the denominator against a membership that does not // exist yet, and the damage is in the unsafe direction: a 3->5 with one member already down @@ -609,13 +655,46 @@ func quorumDisruptDeferMessage(host *api.Host, snap hostEnsembleSnapshot) string ) } -// verifyHostEnsembleMembership is the extension point for Raft membership -// verification after a host joins in rolling mode. Currently a no-op: STS Ready -// wait already ran. Implement committed-config / leader sync barriers here when -// adopting a fuller rescale design. +// verifyHostEnsembleMembership advances only after the joiner's live +// StatefulSet AND Pod report Ready. Active Raft membership is a necessary +// intermediate state, never proof that the new Pod has finished joining. +// Each reconcile re-observes Kubernetes; no phase or readiness is cached. func (w *worker) verifyHostEnsembleMembership(ctx context.Context, host *api.Host) error { - _ = ctx - _ = host + if !hostIsStagedScaleUpJoin(host) || host.IsStopped() { + return nil + } + ready, err := w.readStagedJoinerReady(ctx, host) + if err != nil { + return err // unknown Kubernetes readiness fails closed + } + + // Ready on this joiner implies a successful join and also works for + // TLS-only Keepers where this plaintext ZooKeeper probe is unavailable. + committed := ready + if !ready { + committed, err = w.readStagedJoinerMembership(ctx, host) + if err != nil { + if ctx.Err() != nil { + return ctx.Err() + } + w.a.V(1).M(host).F().Warning("Keeper Raft membership not observable for %s: %v", host.GetName(), err) + return common.ErrCRUDDeferred + } + } + decision := raftplan.PlanJoin(raftplan.JoinObservation{ + StatefulSet: raftplan.PresencePresent, + StartedKnown: true, Started: true, + PublishedKnown: true, Published: true, + CommittedKnown: true, Committed: committed, + ReadyKnown: true, Ready: ready, + }) + if decision.Action != raftplan.JoinAdmitNext { + w.a.V(1).M(host).F().Info( + "Keeper %s still joining: phase=%s action=%s raftCommitted=%t podReady=%t", + host.GetName(), decision.Phase, decision.Action, committed, ready, + ) + return common.ErrCRUDDeferred + } return nil } @@ -667,6 +746,15 @@ func (w *worker) prepareStsReconcileOptsWaitSection( w.a.V(1).M(host).F().Warning("Setting option SetWaitUntilStarted") } + // A staged joiner's StatefulSet may already exist on a retry even though + // its image has never started. The standard Same shortcut otherwise skips + // the startup poll forever while admission defers the next joiner. Recheck + // Pod STARTED with the usual bounded timeout; do not require Pod READY, + // since readiness can depend on existing members adopting the new config. + if hostIsStagedScaleUpJoin(host) && !host.IsStopped() && !hostContributesReady(host) { + opts = opts.SetWaitUntilStartedOnSame() + } + switch { case rolling && joinedEnsemble && !probes.GetReadiness().IsFalse(): opts = opts.SetWaitUntilReady() diff --git a/pkg/controller/chk/worker-raft-safety_test.go b/pkg/controller/chk/worker-raft-safety_test.go index 963ed216a..5ba834bac 100644 --- a/pkg/controller/chk/worker-raft-safety_test.go +++ b/pkg/controller/chk/worker-raft-safety_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/require" apps "k8s.io/api/apps/v1" + core "k8s.io/api/core/v1" apiErrors "k8s.io/apimachinery/pkg/api/errors" meta "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -34,6 +35,7 @@ import ( api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/apis/common/types" "github.com/altinity/clickhouse-operator/pkg/chop" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" "github.com/altinity/clickhouse-operator/pkg/controller/common" a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" "github.com/altinity/clickhouse-operator/pkg/controller/common/statefulset" @@ -670,13 +672,14 @@ func (f *raftFakeSTS) List(ctx context.Context, namespace string, opts meta.List type raftFakeKube struct { interfaces.IKube sts interfaces.IKubeSTS + pod interfaces.IKubePod } func (k *raftFakeKube) STS() interfaces.IKubeSTS { return k.sts } -// Pod and Storage are wired into the StatefulSet reconciler by newTask but never called on -// the paths under test; nil is enough to let construction succeed. -func (k *raftFakeKube) Pod() interfaces.IKubePod { return nil } +// Most raftFakeKube users do not need a Pod reader; tests for an expired +// staged admission supply one to exercise the real executor deadline path. +func (k *raftFakeKube) Pod() interfaces.IKubePod { return k.pod } func (k *raftFakeKube) Storage() interfaces.IKubeStoragePVC { return nil } func (k *raftFakeKube) CR() interfaces.IKubeCR { return raftFakeKubeCR{} } @@ -690,6 +693,18 @@ func (raftFakeKubeCR) StatusUpdate(ctx context.Context, cr api.ICustomResource, return nil } +// raftFakeStartupPod exposes an actual Pod startup state to the planner's +// post-admission executor without requiring a Kubernetes cluster. +type raftFakeStartupPod struct { + interfaces.IKubePod + pod *core.Pod + err error +} + +func (f *raftFakeStartupPod) Get(ctx context.Context, params ...any) (*core.Pod, error) { + return f.pod, f.err +} + func raftWorkerWithSTS(sts interfaces.IKubeSTS) *worker { return &worker{c: &Controller{kube: &raftFakeKube{sts: sts}}} } @@ -1453,11 +1468,27 @@ func TestStagedAdmitDefersWhilePreviousJoinerNotReady(t *testing.T) { return cr, fake, cr.Spec.Configuration.Clusters[0].Layout.Shards[0] } + // The host-loop fixture has no ConfigMap/task machinery. Supply only the + // membership that pass 1 published (h0 and h1), then let the real + // observeRaftPlanHosts read the fake live StatefulSets and run the planner. + newStagedAdmitWorker := func(fake *raftFakeSTS) *worker { + w := &worker{a: a.NewAnnouncer(nil, nil), c: &Controller{kube: &raftFakeKube{sts: fake}}} + w.readPublishedRaftMembersFn = func( + _ context.Context, cr *apiChk.ClickHouseKeeperInstallation, + ) ([]raftplan.HostID, bool, error) { + return []raftplan.HostID{ + raftPlanHostID(hostAtOnCR(cr, 0)), + raftPlanHostID(hostAtOnCR(cr, 1)), + }, true, nil + } + return w + } + t.Run("unready previous joiner blocks the next admit and still reconciles the existing member", func(t *testing.T) { cr, fake, shard := newScaleUp(0) h2 := hostAtOnCR(cr, 2) var visited []string - w := &worker{a: a.NewAnnouncer(nil, nil), c: &Controller{kube: &raftFakeKube{sts: fake}}} + w := newStagedAdmitWorker(fake) w.reconcileHostFn = func(_ context.Context, host *api.Host) error { visited = append(visited, host.GetName()) return nil @@ -1470,10 +1501,39 @@ func TestStagedAdmitDefersWhilePreviousJoinerNotReady(t *testing.T) { require.Zero(t, fake.mutationCount()) }) + t.Run("expired image pull aborts instead of deferring forever", func(t *testing.T) { + cr, fake, shard := newScaleUp(0) + var visited []string + broken := &core.Pod{ + ObjectMeta: meta.ObjectMeta{ + CreationTimestamp: meta.NewTime(time.Now().Add(-72 * time.Hour)), + }, + Spec: core.PodSpec{Containers: []core.Container{{Name: "keeper"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{{ + Name: "keeper", + State: core.ContainerState{ + Waiting: &core.ContainerStateWaiting{Reason: "ImagePullBackOff"}, + }, + }}}, + } + w := newStagedAdmitWorker(fake) + w.c.kube = &raftFakeKube{sts: fake, pod: &raftFakeStartupPod{pod: broken}} + w.reconcileHostFn = func(_ context.Context, host *api.Host) error { + visited = append(visited, host.GetName()) + return nil + } + err := w.reconcileShardWithHosts(ctx, shard) + require.ErrorIs(t, err, common.ErrCRUDAbort, + "a never-started published joiner must abort instead of softly deferring its successor") + require.Equal(t, []string{"h1"}, visited, "h2 and the other hosts must not be touched after abort") + require.Zero(t, fake.mutationCount()) + require.True(t, hostAtOnCR(cr, 2).GetReconcileAttributes().IsExclude()) + }) + t.Run("ready previous joiner admits the next host", func(t *testing.T) { _, fake, shard := newScaleUp(1) var visited []string - w := &worker{a: a.NewAnnouncer(nil, nil), c: &Controller{kube: &raftFakeKube{sts: fake}}} + w := newStagedAdmitWorker(fake) w.reconcileHostFn = func(_ context.Context, host *api.Host) error { visited = append(visited, host.GetName()) return nil @@ -1487,7 +1547,7 @@ func TestStagedAdmitDefersWhilePreviousJoinerNotReady(t *testing.T) { _, fake, shard := newScaleUp(0) fake.err = errors.New("apiserver unavailable") var visited []string - w := &worker{a: a.NewAnnouncer(nil, nil), c: &Controller{kube: &raftFakeKube{sts: fake}}} + w := newStagedAdmitWorker(fake) w.reconcileHostFn = func(_ context.Context, host *api.Host) error { visited = append(visited, host.GetName()) return nil @@ -1504,7 +1564,7 @@ func TestStagedAdmitDefersWhilePreviousJoinerNotReady(t *testing.T) { cr.Spec.Stop = types.NewStringBool(true) h2 := hostAtOnCR(cr, 2) var visited []string - w := &worker{a: a.NewAnnouncer(nil, nil), c: &Controller{kube: &raftFakeKube{sts: fake}}} + w := newStagedAdmitWorker(fake) w.reconcileHostFn = func(_ context.Context, host *api.Host) error { visited = append(visited, host.GetName()) return nil @@ -1552,121 +1612,63 @@ func TestIncludeHostIntoRaftClusterRestoresOnlyStagedExclude(t *testing.T) { }) } -func TestRollbackStagedRaftAdmitRepublishesExclude(t *testing.T) { - ctx := context.Background() - - t.Run("missing StatefulSet drops the host from raft XML", func(t *testing.T) { - cr := chkWithHosts(2) - cr.Spec.Defaults = &api.Defaults{} - cr.SetAncestor(chkWithHosts(1)) - hostAtOnCR(cr, 0) - host := hostAtOnCR(cr, 1) - fake := newRaftFakeSTS() - var published string - w := &worker{ - a: a.NewAnnouncer(nil, nil), - c: &Controller{kube: &raftFakeKube{sts: fake}}, - } - w.publishRaftFn = func(context.Context, *api.Host) error { - published = renderedRaft(t, w, cr) - return nil - } - - w.rollbackStagedRaftAdmit(ctx, host) - require.True(t, host.GetReconcileAttributes().IsExclude()) - require.NotContains(t, published, raftHostname(host), - "rollback must republish raft_configuration without the host that has no Pod") - require.Zero(t, fake.mutationCount()) - }) - - t.Run("existing StatefulSet stays published", func(t *testing.T) { - cr := chkWithHosts(2) - cr.SetAncestor(chkWithHosts(1)) - host := hostAtOnCR(cr, 1) - fake := newRaftFakeSTS() - fake.setReady(host, 0) - published := false - w := &worker{ - a: a.NewAnnouncer(nil, nil), - c: &Controller{kube: &raftFakeKube{sts: fake}}, - publishRaftFn: func(context.Context, *api.Host) error { - published = true - return nil - }, - } - - w.rollbackStagedRaftAdmit(ctx, host) - require.False(t, published, "a start-wait failure must not unpublish a StatefulSet that exists") - require.False(t, host.GetReconcileAttributes().IsExclude()) - require.Zero(t, fake.mutationCount()) - }) -} - -// TestStagedAdmitPublishesRaftWithoutTheHostSeam renders the raft XML the -// ConfigMap would get. reconcileHostFn is not installed, so a publish has to -// come from publishStagedAdmit, which is what reconcileHostMain calls. -func TestStagedAdmitPublishesRaftWithoutTheHostSeam(t *testing.T) { +// TestStagedRaftPublicationFollowsStartedPod checks the active 1->N +// protocol with rendered Raft XML. No joiner is published before Pod Started, +// and starting the first joiner does not admit the other missing joiners. +func TestStagedRaftPublicationFollowsStartedPod(t *testing.T) { ctx := context.Background() cr := chkWithHosts(5) cr.Name = "scale" cr.Spec.Defaults = &api.Defaults{} cr.SetAncestor(chkWithHosts(1)) - fake := newRaftFakeSTS() - for i := 0; i < 5; i++ { - host := hostAtOnCR(cr, i) - switch i { - case 0: - host.Runtime.CurStatefulSet = &apps.StatefulSet{} - host.Runtime.CurStatefulSet.Status.ReadyReplicas = 1 - fake.setReady(host, 1) - case 1: - host.Runtime.CurStatefulSet = &apps.StatefulSet{} - host.GetReconcileAttributes().SetStatus(types.ObjectStatusSame) - fake.setReady(host, 0) - default: - host.GetReconcileAttributes().SetStatus(types.ObjectStatusRequested) - host.GetReconcileAttributes().SetExclude() - } + // Every host visited by the Raft renderer needs a CR back-reference; + // the first voter is not part of the staged-joiner loop below. + hostAtOnCR(cr, 0) + for i := 1; i < 5; i++ { + joiner := hostAtOnCR(cr, i) + joiner.GetReconcileAttributes().SetExclude() + joiner.GetReconcileAttributes().SetStatus(types.ObjectStatusRequested) + } + first := hostAtOnCR(cr, 1) + fakeSTS := newRaftFakeSTS().setReady(first, 0) + fakePod := &raftFakeStartupPod{ + pod: &core.Pod{Spec: core.PodSpec{ + Containers: []core.Container{{Name: "clickhouse-keeper"}}, + }}, } - var published []string w := &worker{ a: a.NewAnnouncer(nil, nil), - c: &Controller{kube: &raftFakeKube{sts: fake}}, + c: &Controller{kube: &raftFakeKube{sts: fakeSTS, pod: fakePod}}, } w.publishRaftFn = func(context.Context, *api.Host) error { published = append(published, renderedRaft(t, w, cr)) return nil } - deferred := false - for _, host := range shardHostsRecoveryFirst(cr.Spec.Configuration.Clusters[0].Layout.Shards[0], func(h *api.Host) bool { - return w.isHostHealthyForReconcile(ctx, h) - }) { - err := w.deferUnreadyStagedAdmit(ctx, host) - if errors.Is(err, errSkipStagedAdmit) { - continue - } - if errors.Is(err, common.ErrCRUDDeferred) { - deferred = true - continue - } - require.NoError(t, err) - _, err = w.publishStagedAdmit(ctx, host) - require.NoError(t, err) + require.ErrorIs(t, w.publishStartedStagedJoiner(ctx, first), common.ErrCRUDDeferred, + "Pod not Started must not publish Raft membership") + require.Empty(t, published) + require.True(t, first.GetReconcileAttributes().IsExclude()) + + started := true + fakePod.pod = &core.Pod{ + Spec: core.PodSpec{Containers: []core.Container{{Name: "clickhouse-keeper"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{{ + Name: "clickhouse-keeper", Started: &started, + }}}, } - - require.True(t, deferred, "h2 must be postponed while h1 is not Ready") - require.Empty(t, published, "no raft_configuration publish may admit h2, h3, or h4") - xml := renderedRaft(t, w, cr) - require.Contains(t, xml, raftHostname(hostAtOnCR(cr, 0))) - require.Contains(t, xml, raftHostname(hostAtOnCR(cr, 1))) + require.NoError(t, w.publishStartedStagedJoiner(ctx, first)) + require.Len(t, published, 1) + require.Contains(t, published[0], raftHostname(hostAtOnCR(cr, 0))) + require.Contains(t, published[0], raftHostname(first)) + require.False(t, first.GetReconcileAttributes().IsExclude()) for i := 2; i < 5; i++ { - require.NotContains(t, xml, raftHostname(hostAtOnCR(cr, i)), - "1→5 must keep every not-yet-created member out of raft_configuration") + require.NotContains(t, published[0], raftHostname(hostAtOnCR(cr, i)), + "only the started joiner may enter shared Raft configuration") require.True(t, hostAtOnCR(cr, i).GetReconcileAttributes().IsExclude()) } - require.Zero(t, fake.mutationCount(), "admitting into raft_configuration must not create or update a StatefulSet") + require.Zero(t, fakeSTS.mutationCount(), "publication must not mutate a StatefulSet") } func renderedRaft(t *testing.T, w *worker, cr *apiChk.ClickHouseKeeperInstallation) string { @@ -2000,3 +2002,169 @@ func TestQuorumWaitReturnsContextErrorOnCancellation(t *testing.T) { "a cancelled reconcile must report cancellation, not a downstream API error") } } + +func TestStagedJoinerRetryWaitsUntilPodStarted(t *testing.T) { + cr := chkWithHosts(3) + cr.SetAncestor(chkWithHosts(1)) + joiner := hostAtOnCR(cr, 1) + joiner.Runtime.CurStatefulSet = &apps.StatefulSet{} + joiner.GetReconcileAttributes().SetStatus(types.ObjectStatusSame) + w := &worker{a: a.NewAnnouncer(nil, nil)} + + opts := w.prepareStsReconcileOptsWaitSection(joiner, nil, false) + require.True(t, opts.WaitUntilStarted(), "new joiner must wait to start") + require.True(t, opts.WaitUntilStartedOnSame(), "a matching STS may still have an unstarted Pod") + require.False(t, opts.WaitUntilReady(), "joiner Ready can depend on existing members updating Raft config") + + joiner.Runtime.CurStatefulSet.Status.ReadyReplicas = 1 + opts = w.prepareStsReconcileOptsWaitSection(joiner, nil, false) + require.False(t, opts.WaitUntilStartedOnSame(), "already Ready joiners do not need a redundant startup check") + + existing := hostAtOnCR(cr, 0) + opts = w.prepareStsReconcileOptsWaitSection(existing, nil, false) + require.False(t, opts.WaitUntilStartedOnSame(), "do not apply retry startup wait to established voters") + + joiner.Runtime.CurStatefulSet.Status.ReadyReplicas = 0 + cr.Spec.Stop = types.NewStringBool(true) + opts = w.prepareStsReconcileOptsWaitSection(joiner, nil, false) + require.False(t, opts.WaitUntilStartedOnSame(), "stopped CHK must never block on pod startup") +} + +// TestPublishStartedStagedJoiner pins the broken-image recovery transition: +// an existing STS must not be published until kubelet reports Pod Started. +// Publishing does not require Ready and never mutates the existing member. +func TestPublishStartedStagedJoiner(t *testing.T) { + ctx := context.Background() + cr := chkWithHosts(3) + cr.SetAncestor(chkWithHosts(1)) + host := hostAtOnCR(cr, 1) + host.GetReconcileAttributes().SetExclude() + host.GetReconcileAttributes().SetStatus(types.ObjectStatusRequested) + fakeSTS := newRaftFakeSTS().setReady(host, 0) + pod := &core.Pod{Spec: core.PodSpec{ + Containers: []core.Container{{Name: "clickhouse-keeper"}}, + }} + fakePod := &raftFakeStartupPod{pod: pod} + w := &worker{ + a: a.NewAnnouncer(nil, nil), + c: &Controller{kube: &raftFakeKube{sts: fakeSTS, pod: fakePod}}, + } + published := 0 + w.publishRaftFn = func(context.Context, *api.Host) error { + published++ + return nil + } + require.ErrorIs(t, w.publishStartedStagedJoiner(ctx, host), common.ErrCRUDDeferred) + require.True(t, host.GetReconcileAttributes().IsExclude()) + require.Zero(t, published) + + started := true + fakePod.pod = &core.Pod{ + Spec: core.PodSpec{Containers: []core.Container{{Name: "clickhouse-keeper"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{{ + Name: "clickhouse-keeper", Started: &started, + }}}, + } + require.NoError(t, w.publishStartedStagedJoiner(ctx, host)) + require.False(t, host.GetReconcileAttributes().IsExclude()) + require.Equal(t, 1, published) + require.Zero(t, fakeSTS.mutationCount(), "must not restart an existing Keeper") + + // Operator restart / subsequent pass: no duplicate publication. + require.NoError(t, w.publishStartedStagedJoiner(ctx, host)) + require.Equal(t, 1, published) +} + +// A new Keeper starts with its own ID even though the shared Raft config +// still excludes it. A second not-yet-created joiner is never in that overlay. +func TestStagedJoinerBootstrapRaftConfig(t *testing.T) { + cr := chkWithHosts(3) + cr.Name = "scale" + cr.Spec.Defaults = &api.Defaults{} + cr.SetAncestor(chkWithHosts(1)) + h0, h1, h2 := hostAtOnCR(cr, 0), hostAtOnCR(cr, 1), hostAtOnCR(cr, 2) + h1.GetReconcileAttributes().SetExclude() + h2.GetReconcileAttributes().SetExclude() + w := &worker{a: a.NewAnnouncer(nil, nil)} + opts := w.options().SetHost(h1).SetBootstrapRaft(true) + sections := map[string]string{} + chkConfig.NewFilesGeneratorDomain(chkConfig.NewGenerator(cr, chkNamer.New(), nil)). + CreateConfigFilesGroupHost(sections, opts) + xml := sections[chkConfig.BootstrapRaftHostConfigKey] + require.Contains(t, xml, `raft_configuration replace="replace"`) + require.Contains(t, xml, raftHostname(h0)) + require.Contains(t, xml, raftHostname(h1)) + require.NotContains(t, xml, raftHostname(h2)) +} + + +// TestStagedJoinerReadyBarrier verifies the final JoinJoining -> JoinReady +// transition. Published/committed membership must not complete a CHK while +// Kubernetes still reports the last joining Pod as unready. Every observation +// is re-read after a retry; old CurStatefulSet snapshots are not trusted. +func TestStagedJoinerReadyBarrier(t *testing.T) { + ctx := context.Background() + cr := chkWithHosts(3) + cr.SetAncestor(chkWithHosts(1)) + host := hostAtOnCR(cr, 2) + sts := newRaftFakeSTS().setReady(host, 0) + + readyPod := func(status core.ConditionStatus) *core.Pod { + return &core.Pod{ + Status: core.PodStatus{ + Phase: core.PodRunning, + Conditions: []core.PodCondition{{ + Type: core.PodReady, Status: status, + }}, + }, + } + } + pods := &raftFakeStartupPod{pod: readyPod(core.ConditionFalse)} + committed := true + membershipChecks := 0 + w := &worker{ + a: a.NewAnnouncer(nil, nil), + c: &Controller{kube: &raftFakeKube{sts: sts, pod: pods}}, + readStagedJoinerMembershipFn: func(context.Context, *api.Host) (bool, error) { + membershipChecks++ + return committed, nil + }, + } + + // The reconcile-time cache can contain an earlier ready result. The + // source of truth must be a new API read, not this stale snapshot. + host.Runtime.CurStatefulSet = &apps.StatefulSet{ + Status: apps.StatefulSetStatus{ReadyReplicas: 1}, + } + require.ErrorIs(t, w.verifyHostEnsembleMembership(ctx, host), common.ErrCRUDDeferred, + "committed membership cannot complete while live STS has 0 ReadyReplicas") + require.Equal(t, 1, membershipChecks) + require.Positive(t, sts.calls()) + + // A ready STS count alone can be stale during a Pod replacement; the + // actual Pod must also have Ready=True. + sts.setReady(host, 1) + require.ErrorIs(t, w.verifyHostEnsembleMembership(ctx, host), common.ErrCRUDDeferred, + "live Pod Ready=False must hold the final transition") + require.Equal(t, 2, membershipChecks) + pods.pod = nil + pods.err = apiErrors.NewNotFound(schema.GroupResource{Resource: "pods"}, host.GetName()) + require.ErrorIs(t, w.verifyHostEnsembleMembership(ctx, host), common.ErrCRUDDeferred, + "disappearing Pod must never be considered ready") + pods.err = nil + pods.pod = readyPod(core.ConditionTrue) + + // As soon as both the StatefulSet and Pod are Ready, the next observed + // pass can complete. No wait before membership publication is involved. + require.NoError(t, w.verifyHostEnsembleMembership(ctx, host)) + require.Zero(t, sts.mutationCount(), "readiness checking must not restart Keepers") + require.Equal(t, 3, membershipChecks, + "Ready on the joiner is sufficient, including for TLS-only Keepers") + + // Subsequent flapping is never masked by the prior successful probe. + sts.setReady(host, 0) + require.ErrorIs(t, w.verifyHostEnsembleMembership(ctx, host), common.ErrCRUDDeferred) + committed = false + require.ErrorIs(t, w.verifyHostEnsembleMembership(ctx, host), common.ErrCRUDDeferred, + "an uncommitted/unready joiner is not admitted") +} diff --git a/pkg/controller/chk/worker-raftplan-enforce.go b/pkg/controller/chk/worker-raftplan-enforce.go new file mode 100644 index 000000000..5901b5292 --- /dev/null +++ b/pkg/controller/chk/worker-raftplan-enforce.go @@ -0,0 +1,236 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package chk + +import ( + "context" + "errors" + "fmt" + "os" + "strings" + "time" + + core "k8s.io/api/core/v1" + apiErrors "k8s.io/apimachinery/pkg/api/errors" + + "github.com/altinity/clickhouse-operator/pkg/controller/common/poller" + "github.com/altinity/clickhouse-operator/pkg/model/k8s" + + apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" + "github.com/altinity/clickhouse-operator/pkg/controller/common" + a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" + "github.com/altinity/clickhouse-operator/pkg/controller/common/statefulset" +) + +// A startup timeout is terminal for the *current CHK specification*, unlike +// ordinary Raft admission deferral. Keep this identity separate from +// ErrCRUDAbort so controller-runtime does not retry a known-broken image. +var errRaftStagedJoinerStartupTimeout = errors.New("staged Keeper joiner startup timed out") + +const raftStagedJoinerStartupTimeoutStatusMarker = "[RaftJoinerStartupTimeout]" + +// shouldHoldRaftStagedStartupAbort guards status/child-object resyncs after a +// terminal startup timeout. Kubernetes metadata.Generation changes on a spec +// edit (such as repairing an image), but not on a status update. NormalizedCR +// is the last attempted spec stored in CHK status, so no new CRD field is needed. +// Fail open for old/unmarked aborts and when recorded generation is unknown. +func shouldHoldRaftStagedStartupAbort(cr *apiChk.ClickHouseKeeperInstallation) bool { + if cr == nil || cr.Status == nil || cr.Status.GetStatus() != apiChk.StatusAborted { + return false + } + target := cr.Status.GetNormalizedCR() + if target == nil || target.GetGeneration() <= 0 || + target.GetGeneration() != cr.GetGeneration() { + return false + } + for _, message := range cr.Status.GetErrors() { + if strings.Contains(message, raftStagedJoinerStartupTimeoutStatusMarker) { + return true + } + } + return false +} + +// raftPlanEnforceEnabled runs the Raft planner by default in this test +// branch, so the standard e2e operator install needs no environment changes. +// CHOP_CHK_RAFTPLAN_ENFORCE=0 restores the legacy admission decision only; +// the safe publish-after-Pod-Started sequence remains active in both modes. +func raftPlanEnforceEnabled() bool { + return os.Getenv("CHOP_CHK_RAFTPLAN_ENFORCE") != "0" +} + +// raftPlanAdmissionVerdict is the pure-to-worker boundary. An unknown +// membership observation cannot authorize creation of another Keeper. +func raftPlanAdmissionVerdict(snapshot raftplan.Snapshot, id raftplan.HostID) (raftplan.AdmissionPlan, error) { + plan := raftplan.PlanAdmission(snapshot) + if plan.Reason == raftplan.ObservationUnknown { + return plan, fmt.Errorf("Raft admission state unknown for %s: %s", id, plan.Reason) + } + if !plan.MayCreate[id] { + return plan, common.ErrCRUDDeferred + } + // The admission planner authorizes a bootstrap StatefulSet here, but + // deliberately does not publish its voter until Pod Started. + return plan, nil +} + +// enforceRaftPlanAdmission replaces deferUnreadyStagedAdmit for staged +// 1->N/2->3 joins only. Existing members still reconcile while the next +// joiner is held so they can adopt the already published Raft config. +func (w *worker) enforceRaftPlanAdmission(ctx context.Context, host *api.Host) error { + if !hostIsStagedScaleUpJoin(host) || w.c == nil { + return nil + } + // An existing STS is handled by the Started -> publish -> Ready path + // in host reconciliation. Never create it again after an interruption. + own, err := w.stagedJoinerStatefulSet(ctx, host) + if err != nil { + return err + } + if own != nil { + return nil + } + if host.IsStopped() { + return errSkipStagedAdmit + } + cr, ok := host.GetCR().(*apiChk.ClickHouseKeeperInstallation) + if !ok { + return fmt.Errorf("Raft admission host %s has no CHK", host.GetName()) + } + observed, err := w.observeRaftPlanAdmission(ctx, cr) + if err != nil { + return fmt.Errorf("observe Raft admission for %s: %w", host.GetName(), err) + } + id := raftPlanHostID(host) + state, exists := observed.Hosts[id] + if !exists || state.StatefulSet == raftplan.PresenceUnknown { + return fmt.Errorf("Raft admission observation missing StatefulSet state for %s", id) + } + if state.StatefulSet == raftplan.PresencePresent { + return nil // another observation saw the StatefulSet appear + } + plan, err := raftPlanAdmissionVerdict(observed, id) + if err == common.ErrCRUDDeferred { + // "Not Ready" covers two very different situations: a Pod that has + // started but is waiting for Raft quorum (keep deferring), and one + // that has never started (e.g. ImagePullBackOff). In the latter case + // a repeated soft deferral can mask the StatefulSet startup timeout. + // The deadline is based on live Kubernetes creation timestamps so + // it survives controller restarts and repeated reconcile passes. + if plan.Reason == raftplan.WaitingForJoiner && plan.BlockedBy != "" { + expired, checkErr := w.stagedJoinerStartupExpired(ctx, cr, plan.BlockedBy) + if checkErr != nil { + return checkErr + } + if expired { + w.a.M(host).F().Warning( + "Raft staged joiner %s never started within StatefulSet timeout; aborting rather than deferring %s", + plan.BlockedBy, id, + ) + return fmt.Errorf("%w: %w (joiner %s blocked host %s)", + common.ErrCRUDAbort, errRaftStagedJoinerStartupTimeout, plan.BlockedBy, id) + } + } + w.a.WithEvent(host.GetCR(), a.EventActionReconcile, a.EventReasonReconcileInProgress). + WithAction(host.GetCR()).M(host).F().Info( + "Raft planner defers admitting %s: reason=%s blockedBy=%s", + id, plan.Reason, plan.BlockedBy, + ) + } + return err +} + +// raftPlanDisruptionBlocked supplies the existing quorum wait with its +// authoritative predicate. A fresh Ready count is obtained by the caller +// before each re-evaluation. Unknown or inconsistent state fails closed. +// Without opt-in this invokes the exact legacy quorum predicate. +func (w *worker) raftPlanDisruptionBlocked( + ctx context.Context, host *api.Host, + opts *statefulset.ReconcileOptions, snap hostEnsembleSnapshot, +) (bool, error) { + if !raftPlanEnforceEnabled() { + return w.hostDisruptionWouldBreakQuorum(ctx, host, opts, snap), nil + } + if host == nil { + return true, fmt.Errorf("Raft disruption host is nil") + } + observed := w.raftPlanDisruptionSnapshot(ctx, host, opts, snap) + decision := raftplan.CheckDisruption(observed, raftPlanHostID(host)) + if decision.Reason == raftplan.DisruptionStateUnknown { + return true, fmt.Errorf( + "Raft planner cannot determine disruption safety for %s (ready=%d members=%d rolling=%t)", + host.GetName(), snap.readyCount, snap.members, snap.rolling, + ) + } + return !decision.Allowed, nil +} + +// stagedJoinerStartupDeadlineExceeded is an executor-side timeout, not Raft +// policy. A started-but-unready Keeper can legitimately need its existing +// peers to load the new membership, so only an unstarted Pod can time out. +// Use the Pod's creation time (not a per-reconcile timer) to avoid resetting +// the deadline on every soft deferral. For a missing Pod, use the StatefulSet +// creation time if it is available. +func stagedJoinerStartupDeadlineExceeded( + pod *core.Pod, stsCreated, now time.Time, timeout time.Duration, +) bool { + if timeout <= 0 { + return false + } + var startedAt time.Time + if pod != nil { + if k8s.PodHasAllContainersStarted(pod) { + return false + } + startedAt = pod.CreationTimestamp.Time + } else { + startedAt = stsCreated + } + return !startedAt.IsZero() && now.Sub(startedAt) >= timeout +} + +// stagedJoinerStartupExpired checks only a joiner that the pure planner +// identified as blocking an absent member. The existing StatefulSet create +// wait remains the first timeout path. This is a cross-reconcile backstop +// when the joiner's STS already exists and repeated admission deferrals would +// otherwise leave the CHK indefinitely InProgress. +func (w *worker) stagedJoinerStartupExpired( + ctx context.Context, cr *apiChk.ClickHouseKeeperInstallation, id raftplan.HostID, +) (bool, error) { + // Legacy lightweight host-loop tests don't construct Pod clients; the + // planner and real StatefulSet observation are still exercised there. + if w == nil || w.c == nil || w.c.kube == nil || w.c.kube.Pod() == nil { + return false, nil + } + var blocker *api.Host + _ = cr.WalkHosts(func(peer *api.Host) error { + if peer != nil && raftPlanHostID(peer) == id { + blocker = peer + } + return nil + }) + if blocker == nil { + return false, fmt.Errorf("Raft planner blocked by unknown joiner %s", id) + } + sts, err := w.stagedJoinerStatefulSet(ctx, blocker) + if err != nil { + return false, fmt.Errorf("read blocked joiner StatefulSet %s: %w", id, err) + } + if sts == nil { + return false, nil // lost STS: admission observation must be repeated + } + pod, err := w.c.kube.Pod().Get(ctx, blocker) + if apiErrors.IsNotFound(err) { + pod = nil + } else if err != nil { + return false, fmt.Errorf("read blocked joiner Pod %s: %w", id, err) + } else if pod == nil { + return false, fmt.Errorf("nil Pod with successful Get for blocked joiner %s", id) + } + timeout := poller.NewOptionsFromConfig().Timeout + return stagedJoinerStartupDeadlineExceeded( + pod, sts.CreationTimestamp.Time, time.Now(), timeout, + ), nil +} diff --git a/pkg/controller/chk/worker-raftplan-enforce_test.go b/pkg/controller/chk/worker-raftplan-enforce_test.go new file mode 100644 index 000000000..1631a64f3 --- /dev/null +++ b/pkg/controller/chk/worker-raftplan-enforce_test.go @@ -0,0 +1,200 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package chk + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/require" + apps "k8s.io/api/apps/v1" + core "k8s.io/api/core/v1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" + "github.com/altinity/clickhouse-operator/pkg/controller/common" + a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" + "github.com/altinity/clickhouse-operator/pkg/controller/common/statefulset" +) + +func TestRaftPlanEnforcementDefaultForE2E(t *testing.T) { + // E2E deploys the operator without configuring this environment variable. + t.Setenv("CHOP_CHK_RAFTPLAN_ENFORCE", "") + require.True(t, raftPlanEnforceEnabled(), "planner must be active without e2e installation changes") + t.Setenv("CHOP_CHK_RAFTPLAN_ENFORCE", "1") + require.True(t, raftPlanEnforceEnabled()) + // Preserve an explicit rollback for manual test deployments. + t.Setenv("CHOP_CHK_RAFTPLAN_ENFORCE", "0") + require.False(t, raftPlanEnforceEnabled()) +} + +func TestRaftPlanEnforcedAdmissionVerdicts(t *testing.T) { + ids := []raftplan.HostID{"h0", "h1", "h2"} + makeSnap := func() raftplan.Snapshot { + return raftplan.Snapshot{ + Established: true, PublishedKnown: true, + DesiredHosts: ids, AncestorHosts: ids[:1], PublishedHosts: ids[:1], + Hosts: map[raftplan.HostID]raftplan.HostState{ + "h0": {ID: "h0", InAncestor: true, StatefulSet: raftplan.PresencePresent, Ready: true, ReadyKnown: true}, + "h1": {ID: "h1", StatefulSet: raftplan.PresenceAbsent, ReadyKnown: true}, + "h2": {ID: "h2", StatefulSet: raftplan.PresenceAbsent, ReadyKnown: true}, + }, + } + } + t.Run("first joiner admitted, second held", func(t *testing.T) { + s := makeSnap() + plan, err := raftPlanAdmissionVerdict(s, "h1") + require.NoError(t, err) + require.True(t, plan.MayCreate["h1"]) + _, err = raftPlanAdmissionVerdict(s, "h2") + require.ErrorIs(t, err, common.ErrCRUDDeferred) + }) + t.Run("unready joiner blocks next", func(t *testing.T) { + s := makeSnap() + s.PublishedHosts = ids[:2] + s.Hosts["h1"] = raftplan.HostState{ID: "h1", StatefulSet: raftplan.PresencePresent, ReadyKnown: true} + plan, err := raftPlanAdmissionVerdict(s, "h2") + require.ErrorIs(t, err, common.ErrCRUDDeferred) + require.Equal(t, raftplan.HostID("h1"), plan.BlockedBy) + }) + t.Run("ready joiner unblocks next", func(t *testing.T) { + s := makeSnap() + s.PublishedHosts = ids[:2] + s.Hosts["h1"] = raftplan.HostState{ID: "h1", StatefulSet: raftplan.PresencePresent, ReadyKnown: true, Ready: true} + _, err := raftPlanAdmissionVerdict(s, "h2") + require.NoError(t, err) + }) + t.Run("unknown publication fails closed", func(t *testing.T) { + s := makeSnap() + s.PublishedKnown = false + _, err := raftPlanAdmissionVerdict(s, "h1") + require.Error(t, err) + require.False(t, errors.Is(err, common.ErrCRUDDeferred)) + }) + t.Run("unknown StatefulSet fails closed", func(t *testing.T) { + s := makeSnap() + s.Hosts["h1"] = raftplan.HostState{ID: "h1", StatefulSet: raftplan.PresenceUnknown} + _, err := raftPlanAdmissionVerdict(s, "h1") + require.Error(t, err) + }) + t.Run("stopped defers missing member", func(t *testing.T) { + s := makeSnap() + s.Stopped = true + _, err := raftPlanAdmissionVerdict(s, "h1") + require.ErrorIs(t, err, common.ErrCRUDDeferred) + }) +} + +func TestRaftPlanEnforcedDisruptionGate(t *testing.T) { + // Exercise the same no-environment default used by the e2e operator install. + t.Setenv("CHOP_CHK_RAFTPLAN_ENFORCE", "") + ctx := context.Background() + for _, tc := range []struct { + name string + ready int + status types.ObjectStatus + forceRecreate bool + blocked bool + }{ + {"degraded", 2, types.ObjectStatusModified, false, true}, + {"healthy", 3, types.ObjectStatusModified, false, false}, + {"unchanged", 2, types.ObjectStatusSame, false, false}, + {"force recreate", 2, types.ObjectStatusSame, true, true}, + {"new", 2, types.ObjectStatusRequested, false, false}, + } { + t.Run(tc.name, func(t *testing.T) { + host := hostOnCR(chkWithHosts(3)) + host.Runtime.CurStatefulSet = &apps.StatefulSet{} + host.Runtime.CurStatefulSet.Status.ReadyReplicas = 1 + host.GetReconcileAttributes().SetStatus(tc.status) + w := &worker{a: a.NewAnnouncer(nil, nil)} + snap := hostEnsembleSnapshot{rolling: true, members: 3, readyCount: tc.ready} + var opts *statefulset.ReconcileOptions + if tc.forceRecreate { + opts = statefulset.NewReconcileStatefulSetOptions().SetForceRecreate() + } + blocked, err := w.raftPlanDisruptionBlocked(ctx, host, opts, snap) + require.NoError(t, err) + require.Equal(t, tc.blocked, blocked) + }) + } + t.Run("inconsistent quorum counts fail closed", func(t *testing.T) { + host := hostOnCR(chkWithHosts(3)) + host.Runtime.CurStatefulSet = &apps.StatefulSet{} + host.Runtime.CurStatefulSet.Status.ReadyReplicas = 1 + host.GetReconcileAttributes().SetStatus(types.ObjectStatusModified) + _, err := (&worker{a: a.NewAnnouncer(nil, nil)}).raftPlanDisruptionBlocked(ctx, host, nil, + hostEnsembleSnapshot{rolling: true, members: 3, readyCount: 4}) + require.Error(t, err) + }) + t.Run("existing quorum gate defers", func(t *testing.T) { + host := hostOnCR(chkWithHosts(3)) + host.Runtime.CurStatefulSet = &apps.StatefulSet{} + host.Runtime.CurStatefulSet.Status.ReadyReplicas = 1 + host.GetReconcileAttributes().SetStatus(types.ObjectStatusModified) + w := &worker{ + a: a.NewAnnouncer(nil, nil), + countReadyEnsembleMembersFn: func(context.Context, api.ICustomResource) int { return 2 }, + quorumDisruptPollOverride: 2 * time.Millisecond, + quorumDisruptWaitOverride: 10 * time.Millisecond, + } + snap := hostEnsembleSnapshot{rolling: true, members: 3, readyCount: 2} + err := w.ensureQuorumSafeToDisruptHost(ctx, host, nil, &snap) + require.ErrorIs(t, err, common.ErrCRUDDeferred) + }) +} + +func TestStagedJoinerStartupDeadlineExceeded(t *testing.T) { + now := time.Date(2026, time.October, 23, 9, 0, 0, 0, time.UTC) + timeout := time.Minute + started := true + neverStarted := false + old := meta.NewTime(now.Add(-2 * timeout)) + recent := meta.NewTime(now.Add(-timeout / 2)) + pendingPod := func(creation meta.Time, started *bool) *core.Pod { + return &core.Pod{ + ObjectMeta: meta.ObjectMeta{CreationTimestamp: creation}, + Spec: core.PodSpec{Containers: []core.Container{{Name: "keeper"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{{ + Name: "keeper", Started: started, + }}}, + } + } + t.Run("broken image after timeout", func(t *testing.T) { + require.True(t, stagedJoinerStartupDeadlineExceeded( + pendingPod(old, &neverStarted), old.Time, now, timeout, + )) + }) + t.Run("pod without container status after timeout", func(t *testing.T) { + pod := pendingPod(old, nil) + pod.Status.ContainerStatuses = nil + require.True(t, stagedJoinerStartupDeadlineExceeded(pod, old.Time, now, timeout)) + }) + t.Run("unstarted pod inside timeout", func(t *testing.T) { + require.False(t, stagedJoinerStartupDeadlineExceeded( + pendingPod(recent, &neverStarted), old.Time, now, timeout, + )) + }) + t.Run("started but unready must defer, not abort", func(t *testing.T) { + require.False(t, stagedJoinerStartupDeadlineExceeded( + pendingPod(old, &started), old.Time, now, timeout, + )) + }) + t.Run("no pod but old StatefulSet", func(t *testing.T) { + require.True(t, stagedJoinerStartupDeadlineExceeded(nil, old.Time, now, timeout)) + }) + t.Run("unknown creation age is not a timeout", func(t *testing.T) { + require.False(t, stagedJoinerStartupDeadlineExceeded( + pendingPod(meta.Time{}, &neverStarted), old.Time, now, timeout, + )) + }) + t.Run("zero timeout disables deadline", func(t *testing.T) { + require.False(t, stagedJoinerStartupDeadlineExceeded( + pendingPod(old, &neverStarted), old.Time, now, 0, + )) + }) +} diff --git a/pkg/controller/chk/worker-raftplan-live.go b/pkg/controller/chk/worker-raftplan-live.go new file mode 100644 index 000000000..50adec80f --- /dev/null +++ b/pkg/controller/chk/worker-raftplan-live.go @@ -0,0 +1,137 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package chk + +import ( + "context" + "errors" + "fmt" + "strings" + "time" + + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" + "github.com/altinity/clickhouse-operator/pkg/controller/common" + "github.com/altinity/clickhouse-operator/pkg/interfaces" + "github.com/altinity/clickhouse-operator/pkg/model/k8s" + "github.com/go-zookeeper/zk" + core "k8s.io/api/core/v1" + apiErrors "k8s.io/apimachinery/pkg/api/errors" +) + +// publishStartedStagedJoiner is the only transition from bootstrap to +// published membership in the serial 1->N/2->3 admission path. Its inputs +// are live Kubernetes facts, so a retry after operator restart resumes here. +func (w *worker) publishStartedStagedJoiner(ctx context.Context, host *api.Host) error { + if !hostIsStagedScaleUpJoin(host) || !host.GetReconcileAttributes().IsExclude() || host.IsStopped() { + return nil + } + sts, err := w.stagedJoinerStatefulSet(ctx, host) + if err != nil { + return err + } + if sts == nil { + return common.ErrCRUDDeferred + } + if w.c == nil || w.c.kube == nil || w.c.kube.Pod() == nil { + return fmt.Errorf("Keeper joiner Pod observation unavailable: %s", host.GetName()) + } + pod, err := w.c.kube.Pod().Get(ctx, host) + if apiErrors.IsNotFound(err) { + return common.ErrCRUDDeferred + } + if err != nil { + return err + } + if pod == nil { + return fmt.Errorf("nil Keeper joiner Pod: %s", host.GetName()) + } + decision := raftplan.PlanJoin(raftplan.JoinObservation{ + StatefulSet: raftplan.PresencePresent, + StartedKnown: true, + Started: k8s.PodHasAllContainersStarted(pod), + PublishedKnown: true, + Published: false, + }) + if decision.Action != raftplan.JoinPublish { + return common.ErrCRUDDeferred + } + return w.publishStagedAdmit(ctx, host) +} + +// readStagedJoinerReady checks both live Kubernetes readiness indicators. +// A cached CurStatefulSet (captured before the join) or Raft membership alone +// cannot authorize completion. Checking the Pod as well avoids using an STS +// ready count from a previous Pod during a replacement/restart. +func (w *worker) readStagedJoinerReady(ctx context.Context, host *api.Host) (bool, error) { + sts, err := w.stagedJoinerStatefulSet(ctx, host) + if err != nil { + return false, fmt.Errorf("read staged Keeper StatefulSet %s: %w", host.GetName(), err) + } + if sts == nil || sts.Status.ReadyReplicas < 1 { + return false, nil + } + if w.c == nil || w.c.kube == nil || w.c.kube.Pod() == nil { + return false, fmt.Errorf("Keeper Pod observation unavailable for %s", host.GetName()) + } + pod, err := w.c.kube.Pod().Get(ctx, host) + if apiErrors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("read staged Keeper Pod %s: %w", host.GetName(), err) + } + if pod == nil { + return false, fmt.Errorf("nil staged Keeper Pod %s on successful Get", host.GetName()) + } + if pod.DeletionTimestamp != nil || pod.Status.Phase != core.PodRunning { + return false, nil + } + for _, condition := range pod.Status.Conditions { + if condition.Type == core.PodReady { + return condition.Status == core.ConditionTrue, nil + } + } + return false, nil +} + +// readStagedJoinerMembership probes an established member, not the joiner: +// /keeper/config on a new candidate is unavailable before it joins quorum. +// The probe is read-only and its failure never permits the next voter. +func (w *worker) readStagedJoinerMembership(ctx context.Context, host *api.Host) (bool, error) { + if err := ctx.Err(); err != nil { + return false, err + } + if w.readStagedJoinerMembershipFn != nil { + return w.readStagedJoinerMembershipFn(ctx, host) + } + var peer *api.Host + _ = host.GetCR().WalkHosts(func(h *api.Host) error { + if peer == nil && h != nil && h.HasAncestor() { + peer = h + } + return nil + }) + if peer == nil { + return false, errors.New("no established Keeper to query for Raft membership") + } + ns := peer.GetRuntime().GetAddress().GetNamespace() + address := fmt.Sprintf("%s.%s.svc:2181", w.c.namer.Name(interfaces.NameInstanceHostname, peer), ns) + conn, _, err := zk.Connect([]string{address}, 4*time.Second) + if err != nil { + return false, err + } + defer conn.Close() + data, _, err := conn.Get("/keeper/config") + if err != nil { + return false, err + } + want := fmt.Sprintf("server.%d=%s:%d;", host.GetRuntime().GetAddress().GetReplicaIndex(), + w.c.namer.Name(interfaces.NameInstanceHostname, host), host.RaftPort.Value()) + for _, line := range strings.Split(string(data), "\n") { + if strings.HasPrefix(strings.TrimSpace(line), want) { + return true, nil + } + } + return false, nil +} diff --git a/pkg/controller/chk/worker-raftplan-observer.go b/pkg/controller/chk/worker-raftplan-observer.go new file mode 100644 index 000000000..d03d722d5 --- /dev/null +++ b/pkg/controller/chk/worker-raftplan-observer.go @@ -0,0 +1,213 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package chk + +import ( + "context" + "errors" + "fmt" + + core "k8s.io/api/core/v1" + apiErrors "k8s.io/apimachinery/pkg/api/errors" + kubeTypes "k8s.io/apimachinery/pkg/types" + + apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" + "github.com/altinity/clickhouse-operator/pkg/controller/common/statefulset" + "github.com/altinity/clickhouse-operator/pkg/interfaces" + "github.com/altinity/clickhouse-operator/pkg/model/k8s" +) + +func raftPlanHostID(host *api.Host) raftplan.HostID { + return raftplan.HostID(host.GetName()) +} + +// observeRaftPlanHosts returns values captured from live StatefulSet Gets. +// Unlike a cached CurStatefulSet, an API error is explicitly Unknown; callers +// cannot interpret an unreadable member as absent or unready. +// The authoritative admission gate, preliminary staging, and optional shadow +// diagnostics all use this read-only adapter. No resources are mutated. +func (w *worker) observeRaftPlanHosts( + ctx context.Context, + cr *apiChk.ClickHouseKeeperInstallation, + published []raftplan.HostID, + publishedKnown bool, +) (raftplan.Snapshot, error) { + if w == nil || w.c == nil || w.c.kube == nil || cr == nil { + return raftplan.Snapshot{}, fmt.Errorf("raftplan observation requires worker, kube adapter, and CHK") + } + snap := raftplan.Snapshot{ + Established: crHasEstablishedCluster(cr), + Stopped: cr.IsStopped(), + Hosts: map[raftplan.HostID]raftplan.HostState{}, + PublishedHosts: append([]raftplan.HostID(nil), published...), + PublishedKnown: publishedKnown, + } + // Preserve the same order that the existing host walker uses to admit + // joiners. Do not sort by StatefulSet creation timestamp. + if ancestor := cr.GetAncestor(); ancestor != nil { + _ = ancestor.WalkHosts(func(host *api.Host) error { + if host != nil { + snap.AncestorHosts = append(snap.AncestorHosts, raftPlanHostID(host)) + } + return nil + }) + } + existing := make(map[raftplan.HostID]bool, len(snap.AncestorHosts)) + for _, id := range snap.AncestorHosts { + existing[id] = true + } + + walkErrors := cr.WalkHosts(func(host *api.Host) error { + if host == nil { + return nil + } + id := raftPlanHostID(host) + if _, duplicate := snap.Hosts[id]; duplicate { + return fmt.Errorf("duplicate Raft planner host ID %q", id) + } + snap.DesiredHosts = append(snap.DesiredHosts, id) + state := raftplan.HostState{ + ID: id, + InAncestor: existing[id], + StatefulSet: raftplan.PresenceUnknown, + } + sts, getErr := w.c.kube.STS().Get(ctx, host) + switch { + case apiErrors.IsNotFound(getErr): + state.StatefulSet = raftplan.PresenceAbsent + state.ReadyKnown = true + case getErr != nil: + snap.Hosts[id] = state + return fmt.Errorf("read Keeper StatefulSet for %s: %w", id, getErr) + case sts == nil: + snap.Hosts[id] = state + return fmt.Errorf("nil StatefulSet with no error for %s", id) + default: + state.StatefulSet = raftplan.PresencePresent + state.ReadyKnown = true + state.Ready = sts.Status.ReadyReplicas > 0 + if state.Ready { + state.PodStarted, state.StartedKnown = true, true + } else if w.c.kube.Pod() != nil && hostIsStagedScaleUpJoin(host) { + // A StatefulSet exists before Keeper starts. Observation must + // distinguish it from a joiner eligible for publication. + pod, podErr := w.c.kube.Pod().Get(ctx, host) + switch { + case apiErrors.IsNotFound(podErr): + state.StartedKnown = true + case podErr != nil: + snap.Hosts[id] = state + return fmt.Errorf("read Keeper joiner Pod %s: %w", id, podErr) + case pod == nil: + snap.Hosts[id] = state + return fmt.Errorf("nil Keeper Pod %s on successful Get", id) + default: + state.StartedKnown = true + state.PodStarted = k8s.PodHasAllContainersStarted(pod) + } + } + } + snap.Hosts[id] = state + return nil + }) + // WalkHosts aggregates callback errors as []error, not error. + return snap, errors.Join(walkErrors...) +} + +// observeRaftPlanAdmission reads actual published membership from the common +// ConfigMap. It is deliberately separate from the live STS reads above so +// integration tests can compare policy decisions without constructing a CM. +func (w *worker) observeRaftPlanAdmission( + ctx context.Context, cr *apiChk.ClickHouseKeeperInstallation, +) (raftplan.Snapshot, error) { + // Test-only seam for published membership. This does not bypass the + // actual live StatefulSet observer or the planner's safety decisions. + if w != nil && w.readPublishedRaftMembersFn != nil { + published, known, err := w.readPublishedRaftMembersFn(ctx, cr) + if err != nil { + return raftplan.Snapshot{}, fmt.Errorf("read published Raft membership: %w", err) + } + return w.observeRaftPlanHosts(ctx, cr, published, known) + } + if w == nil || w.c == nil || w.c.APIReader == nil || w.c.namer == nil || w.task == nil || cr == nil { + return raftplan.Snapshot{}, fmt.Errorf("raftplan admission observation is unavailable") + } + // Use the exact common ConfigMap name produced by the production creator. + desiredCM := w.task.Creator().CreateConfigMap(interfaces.ConfigMapCommon, w.options()) + current := &core.ConfigMap{} + key := kubeTypes.NamespacedName{Namespace: desiredCM.Namespace, Name: desiredCM.Name} + err := w.c.APIReader.Get(ctx, key, current) + var published []raftplan.HostID + publishedKnown := false + switch { + case apiErrors.IsNotFound(err): + // A confirmed missing ConfigMap has no published members. + publishedKnown = true + case err != nil: + return raftplan.Snapshot{}, fmt.Errorf("read live Keeper common ConfigMap: %w", err) + default: + hostnames, known, parseErr := raftplan.PublishedHostnames(current.Data) + if parseErr != nil { + return raftplan.Snapshot{}, parseErr + } + publishedKnown = known + if known { + // The Raft XML uses instance hostnames, while policy keys are + // Host.GetName(). Unknown names remain in the published set and + // therefore cannot be silently dropped by the planner. + byHostname := make(map[string]raftplan.HostID) + visit := func(host *api.Host) error { + if host != nil { + name := w.c.namer.Name(interfaces.NameInstanceHostname, host) + byHostname[name] = raftPlanHostID(host) + } + return nil + } + _ = cr.WalkHosts(visit) + if ancestor := cr.GetAncestor(); ancestor != nil { + _ = ancestor.WalkHosts(visit) + } + for _, name := range hostnames { + id, ok := byHostname[name] + if !ok { + return raftplan.Snapshot{}, fmt.Errorf("published Raft hostname %q has no known CHK host (observation uncertain)", name) + } + published = append(published, id) + } + } + } + return w.observeRaftPlanHosts(ctx, cr, published, publishedKnown) +} + +// raftPlanDisruptionSnapshot adapts the same prepared STS status and frozen +// quorum denominator as the existing quorum gate. It performs no extra API +// reads: the caller must invoke it immediately after its existing fresh +// snapshotHostEnsemble/PrepareHostStatefulSetWithStatus sequence. +func (w *worker) raftPlanDisruptionSnapshot( + ctx context.Context, host *api.Host, + opts *statefulset.ReconcileOptions, snap hostEnsembleSnapshot, +) raftplan.Snapshot { + id := raftPlanHostID(host) + status := host.GetReconcileAttributes().GetStatus() + willDisrupt := !host.IsStopped() && !status.Is(types.ObjectStatusRequested) && + (!status.Is(types.ObjectStatusSame) || w.shouldForceRestartHost(ctx, host) || + (opts != nil && opts.ForceRecreate())) + return raftplan.Snapshot{ + Stopped: host.IsStopped(), + Rolling: snap.rolling, + MemberCount: snap.members, + ReadyCount: snap.readyCount, + CountsKnown: true, + Hosts: map[raftplan.HostID]raftplan.HostState{ + id: { + ID: id, + Ready: hostContributesReady(host), + ReadyKnown: true, + WouldDisrupt: willDisrupt, + }, + }, + } +} diff --git a/pkg/controller/chk/worker-raftplan-shadow.go b/pkg/controller/chk/worker-raftplan-shadow.go new file mode 100644 index 000000000..4e6a258de --- /dev/null +++ b/pkg/controller/chk/worker-raftplan-shadow.go @@ -0,0 +1,81 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package chk + +import ( + "context" + "errors" + "os" + + apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" + "github.com/altinity/clickhouse-operator/pkg/controller/common" + "github.com/altinity/clickhouse-operator/pkg/controller/common/statefulset" +) + +// This env toggle is opt-in. Shadow mode never replaces or modifies a legacy +// decision, status condition, ConfigMap or StatefulSet. It performs extra live +// reads and should only be enabled on a controlled test installation. +func raftPlanShadowEnabled() bool { + return os.Getenv("CHOP_CHK_RAFTPLAN_SHADOW") == "1" +} + +// shadowRaftAdmission compares the *per-host* legacy gate with the new planner. +// Preliminary exclusion is intentionally not compared directly: joiners +// are staged before creation and published only after Pod Started. Shadow +// admission compares only whether the next StatefulSet may be created. +func (w *worker) shadowRaftAdmission(ctx context.Context, host *api.Host, legacyErr error) { + if !raftPlanShadowEnabled() || host == nil || host.GetCR() == nil || !hostIsStagedScaleUpJoin(host) { + return + } + cr, ok := host.GetCR().(*apiChk.ClickHouseKeeperInstallation) + if !ok { + return + } + snapshot, err := w.observeRaftPlanAdmission(ctx, cr) + if err != nil { + w.a.V(1).M(host).F().Warning("CHK raftplan shadow admission observation failed: %v", err) + return + } + id := raftPlanHostID(host) + state, ok := snapshot.Hosts[id] + if !ok || state.StatefulSet != raftplan.PresenceAbsent { + return // existing STS is not subject to the legacy admission gate + } + plan := raftplan.PlanAdmission(snapshot) + if plan.Reason == raftplan.ObservationUnknown { + return // not enough information to compare decisions + } + legacyAllowed := legacyErr == nil + if legacyErr != nil && !errors.Is(legacyErr, common.ErrCRUDDeferred) && + !errors.Is(legacyErr, errSkipStagedAdmit) { + return // unrelated hard failure, not a safety-gate decision + } + if plan.MayCreate[id] != legacyAllowed { + w.a.M(host).F().Warning( + "CHK raftplan shadow ADMISSION MISMATCH host=%s legacyAllowed=%t plannerAllowed=%t plannerReason=%s blockedBy=%s", + id, legacyAllowed, plan.MayCreate[id], plan.Reason, plan.BlockedBy) + } +} + +// shadowRaftDisruption compares policy decisions just after the live quorum +// snapshot and STS mutation options have been prepared, before the authoritative +// legacy gate starts polling. It must never cause a disruptive mutation. +func (w *worker) shadowRaftDisruption( + ctx context.Context, host *api.Host, opts *statefulset.ReconcileOptions, + snap hostEnsembleSnapshot, +) { + if !raftPlanShadowEnabled() || host == nil { + return + } + policySnapshot := w.raftPlanDisruptionSnapshot(ctx, host, opts, snap) + result := raftplan.CheckDisruption(policySnapshot, raftPlanHostID(host)) + legacyAllowed := !w.hostDisruptionWouldBreakQuorum(ctx, host, opts, snap) + if result.Allowed != legacyAllowed { + w.a.M(host).F().Warning( + "CHK raftplan shadow DISRUPTION MISMATCH host=%s legacyAllowed=%t plannerAllowed=%t reason=%s ready=%d members=%d rolling=%t", + host.GetName(), legacyAllowed, result.Allowed, result.Reason, + snap.readyCount, snap.members, snap.rolling) + } +} diff --git a/pkg/controller/chk/worker-raftplan-shadow_test.go b/pkg/controller/chk/worker-raftplan-shadow_test.go new file mode 100644 index 000000000..864bfefca --- /dev/null +++ b/pkg/controller/chk/worker-raftplan-shadow_test.go @@ -0,0 +1,123 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +package chk + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" + "github.com/altinity/clickhouse-operator/pkg/controller/common" + a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" + "github.com/altinity/clickhouse-operator/pkg/controller/common/statefulset" + apps "k8s.io/api/apps/v1" +) + +// Uses the real legacy deferUnreadyStagedAdmit on the same fake live StatefulSets +// that feed the new observer. The preliminary exclusion tags are deliberately +// not the oracle: all not-yet-created members are excluded before the host loop, +// while admission occurs immediately before a specific STS create. +func TestRaftPlanAdmissionParityWithLegacyGate(t *testing.T) { + ctx := context.Background() + tests := []struct { + name string + desired int + ancestor int + readyJoiner bool + hasJoiner bool + target int + published []int + stopped bool + expectAllowed bool + }{ + {name: "1 to 3 first joiner", desired: 3, ancestor: 1, target: 1, published: []int{0}, expectAllowed: true}, + {name: "1 to 3 blocked by unready joiner", desired: 3, ancestor: 1, hasJoiner: true, target: 2, published: []int{0, 1}}, + {name: "1 to 3 advances after Ready", desired: 3, ancestor: 1, hasJoiner: true, readyJoiner: true, target: 2, published: []int{0, 1}, expectAllowed: true}, + {name: "2 to 3 final joiner", desired: 3, ancestor: 2, target: 2, published: []int{0, 1}, expectAllowed: true}, + {name: "missing published joiner is repaired", desired: 3, ancestor: 1, target: 1, published: []int{0, 1}, expectAllowed: true}, + {name: "3 to 5 preserves full fan-out", desired: 5, ancestor: 3, target: 3, published: []int{0, 1, 2}, expectAllowed: true}, + {name: "stop prevents admission", desired: 3, ancestor: 1, target: 2, published: []int{0, 1}, stopped: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cr := chkWithHosts(tc.desired) + cr.SetAncestor(chkWithHosts(tc.ancestor)) + if tc.stopped { + cr.Spec.Stop = types.NewStringBool(true) + } + fake := newRaftFakeSTS() + for i := 0; i < tc.ancestor; i++ { + fake.setReady(hostAtOnCR(cr, i), 1) + } + if tc.hasJoiner { + ready := int32(0) + if tc.readyJoiner { + ready = 1 + } + fake.setReady(hostAtOnCR(cr, 1), ready) + } + w := raftWorkerWithSTS(fake) + w.a = a.NewAnnouncer(nil, nil) + pub := make([]raftplan.HostID, 0, len(tc.published)) + for _, i := range tc.published { + pub = append(pub, raftPlanHostID(hostAtOnCR(cr, i))) + } + snapshot, err := w.observeRaftPlanHosts(ctx, cr, pub, true) + require.NoError(t, err) + plan := raftplan.PlanAdmission(snapshot) + target := hostAtOnCR(cr, tc.target) + require.Equal(t, raftplan.PresenceAbsent, snapshot.Hosts[raftPlanHostID(target)].StatefulSet) + legacyErr := w.deferUnreadyStagedAdmit(ctx, target) + legacyAllowed := legacyErr == nil + if legacyErr != nil { + require.True(t, errors.Is(legacyErr, common.ErrCRUDDeferred) || errors.Is(legacyErr, errSkipStagedAdmit), "unexpected legacy error: %v", legacyErr) + } + require.Equal(t, tc.expectAllowed, legacyAllowed, "legacy fixture invalid") + require.Equal(t, legacyAllowed, plan.MayCreate[raftPlanHostID(target)], "planner disagrees with legacy host gate") + }) + } +} + +func TestRaftPlanDisruptionParityWithLegacyGate(t *testing.T) { + ctx := context.Background() + tests := []struct { + name string + memberCount int + readyCount int + rolling bool + targetReady bool + status types.ObjectStatus + opts *statefulset.ReconcileOptions + wantAllowed bool + }{ + {name: "degraded 3 members", memberCount: 3, readyCount: 2, rolling: true, targetReady: true, status: types.ObjectStatusModified}, + {name: "healthy 3 members", memberCount: 3, readyCount: 3, rolling: true, targetReady: true, status: types.ObjectStatusModified, wantAllowed: true}, + {name: "unchanged STS", memberCount: 3, readyCount: 2, rolling: true, targetReady: true, status: types.ObjectStatusSame, wantAllowed: true}, + {name: "new STS", memberCount: 3, readyCount: 2, rolling: true, targetReady: true, status: types.ObjectStatusRequested, wantAllowed: true}, + {name: "force recreate", memberCount: 3, readyCount: 2, rolling: true, targetReady: true, status: types.ObjectStatusSame, opts: statefulset.NewReconcileStatefulSetOptions().SetForceRecreate()}, + {name: "bootstrap 3 members", memberCount: 3, readyCount: 1, rolling: false, targetReady: true, status: types.ObjectStatusModified, wantAllowed: true}, + {name: "two members compatibility", memberCount: 2, readyCount: 2, rolling: true, targetReady: true, status: types.ObjectStatusModified, wantAllowed: true}, + {name: "unready target", memberCount: 3, readyCount: 2, rolling: true, targetReady: false, status: types.ObjectStatusModified, wantAllowed: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + host := hostOnCR(chkWithHosts(tc.memberCount)) + host.GetReconcileAttributes().SetStatus(tc.status) + host.Runtime.CurStatefulSet = &apps.StatefulSet{} + if tc.targetReady { + host.Runtime.CurStatefulSet.Status.ReadyReplicas = 1 + } + w := &worker{} + snap := hostEnsembleSnapshot{rolling: tc.rolling, members: tc.memberCount, readyCount: tc.readyCount} + newSnapshot := w.raftPlanDisruptionSnapshot(ctx, host, tc.opts, snap) + got := raftplan.CheckDisruption(newSnapshot, raftPlanHostID(host)) + legacyAllowed := !w.hostDisruptionWouldBreakQuorum(ctx, host, tc.opts, snap) + require.Equal(t, tc.wantAllowed, legacyAllowed, "legacy fixture invalid") + require.Equal(t, legacyAllowed, got.Allowed, "planner diverged from legacy quorum gate") + }) + } +} diff --git a/pkg/controller/chk/worker-reconciler-chk.go b/pkg/controller/chk/worker-reconciler-chk.go index 2cde120e4..f699e64f1 100644 --- a/pkg/controller/chk/worker-reconciler-chk.go +++ b/pkg/controller/chk/worker-reconciler-chk.go @@ -148,12 +148,25 @@ func (w *worker) reconcileCR(ctx context.Context, old, new *apiChk.ClickHouseKee WithError(new). M(new).F(). Error("FAILED to reconcile CR %s, err: %v", util.NamespaceNameString(new), err) - err = common.ErrCRUDAbort - w.markReconcileCompletedUnsuccessfully(ctx, new, err) - if errors.Is(err, common.ErrCRUDAbort) { - metrics.CRReconcilesAborted(ctx, new) + // Only an expired staged-joiner startup is terminal for this spec + // generation. Other hard errors keep their historical retry behavior. + startupTimedOut := errors.Is(err, errRaftStagedJoinerStartupTimeout) + if startupTimedOut { + new.EnsureStatus().PushError( + raftStagedJoinerStartupTimeoutStatusMarker + + " Keeper joiner failed to start; edit CHK spec to retry", + ) } - return err + abortErr := common.ErrCRUDAbort + if startupTimedOut { + abortErr = fmt.Errorf("%w: %w", common.ErrCRUDAbort, err) + } + if persistErr := w.markReconcileCompletedUnsuccessfully(ctx, new, abortErr); persistErr != nil { + // Do not acknowledge a terminal error before Aborted is durable. + return fmt.Errorf("persist CHK Aborted status: %w", persistErr) + } + metrics.CRReconcilesAborted(ctx, new) + return abortErr } // Reconcile successful @@ -367,14 +380,9 @@ func (w *worker) reconcileCRAuxObjectsFinal(ctx context.Context, cr *apiChk.Clic err = w.reconcileConfigMapCommon(ctx, cr) cr.GetRuntime().UnlockCommonConfig() - w.includeAllHostsIntoCluster(ctx, cr) return err } -func (w *worker) includeAllHostsIntoCluster(ctx context.Context, cr *apiChk.ClickHouseKeeperInstallation) { - // Not applicable -} - // reconcileConfigMapCommon reconciles common ConfigMap func (w *worker) reconcileConfigMapCommon( ctx context.Context, @@ -416,7 +424,10 @@ func (w *worker) reconcileConfigMapCommonUsers(ctx context.Context, cr api.ICust // reconcileConfigMapHost reconciles host's personal ConfigMap func (w *worker) reconcileConfigMapHost(ctx context.Context, host *api.Host) error { // ConfigMap for a host - configMap := w.task.Creator().CreateConfigMap(interfaces.ConfigMapHost, host) + opts := w.options().SetHost(host).SetBootstrapRaft( + hostIsStagedScaleUpJoin(host) && host.GetReconcileAttributes().IsExclude(), + ) + configMap := w.task.Creator().CreateConfigMap(interfaces.ConfigMapHost, host, opts) err := w.reconcileConfigMap(ctx, host.GetCR(), configMap) if err == nil { w.task.RegistryReconciled().RegisterConfigMap(configMap.GetObjectMeta()) @@ -442,6 +453,7 @@ func (w *worker) reconcileHostStatefulSet( w.stsReconciler.PrepareHostStatefulSetWithStatus(ctx, host, host.IsStopped()) opts = w.prepareStsReconcileOptsWaitSection(host, opts, snap.rolling) + w.shadowRaftDisruption(ctx, host, opts, snap) if err := w.ensureQuorumSafeToDisruptHost(ctx, host, opts, &snap); err != nil { return err } @@ -468,6 +480,14 @@ func (w *worker) reconcileHostStatefulSet( // We are in place, where we can reconcile StatefulSet to desired configuration. w.a.V(1).M(host).F().Info("Reconcile host STS: %s. Reconcile StatefulSet", host.GetName()) err := w.stsReconciler.ReconcileStatefulSet(ctx, host, true, opts) + // An unchanged staged joiner's *startup* timeout is terminal for this + // CHK spec, not another reason for controller-runtime to re-run the same + // broken-image reconcile. The Same+opt-in path only waits for Pod Started. + if errors.Is(err, common.ErrCRUDAbort) && + hostIsStagedScaleUpJoin(host) && opts.WaitUntilStartedOnSame() && + host.GetReconcileAttributes().GetStatus().Is(types.ObjectStatusSame) { + err = fmt.Errorf("%w: %w", errRaftStagedJoinerStartupTimeout, err) + } if err == nil { w.task.RegistryReconciled().RegisterStatefulSet(host.Runtime.DesiredStatefulSet.GetObjectMeta()) } else { @@ -721,7 +741,16 @@ func (w *worker) reconcileHost(ctx context.Context, host *api.Host) error { // for the previous joiner's Ready wait (ObjectStatusSame returns first). // The next missing joiner has to be postponed here, while the loop continues // so existing members can still roll. - if err := w.deferUnreadyStagedAdmit(ctx, host); err != nil { + var admitErr error + if raftPlanEnforceEnabled() { + // Opt-in: the pure planner decides serial joiner admission. + // Existing staging, publication, and rollback remain unchanged. + admitErr = w.enforceRaftPlanAdmission(ctx, host) + } else { + admitErr = w.deferUnreadyStagedAdmit(ctx, host) + w.shadowRaftAdmission(ctx, host, admitErr) + } + if err := admitErr; err != nil { if errors.Is(err, errSkipStagedAdmit) { return nil } @@ -792,9 +821,9 @@ func (w *worker) reconcileHost(ctx context.Context, host *api.Host) error { // reconcileHostPrepare reconciles specified ClickHouse host func (w *worker) reconcileHostPrepare(ctx context.Context, host *api.Host) error { if host.GetReconcileAttributes().IsExclude() { - // Stay out of raft_configuration until immediately before STS create. - // Publishing here would leave the server in XML if ConfigMapHost / PVC - // abort before the StatefulSet exists. + // Keep staged joiners out of shared raft_configuration until their Pod + // has Started. The host-specific bootstrap config lets them start + // without first changing the established members' Raft configuration. w.a.V(1). M(host).F(). Info("Skip raft include in prepare; host is staged. Host/shard/cluster: %d/%d/%s", @@ -874,27 +903,21 @@ func (w *worker) reconcileHostMain(ctx context.Context, host *api.Host) error { Warning("Reconcile Host Main - unable to reconcile Service. Host: %s Err: %v", host.GetName(), err) } - // Admit this host into raft_configuration before creating its StatefulSet, - // so the new pod starts with itself already in the published membership. - // Remaining staged hosts stay excluded. - admitted, admitErr := w.publishStagedAdmit(ctx, host) - if admitErr != nil { - metrics.HostReconcilesErrors(ctx, host.GetCR()) - w.a.V(1). - M(host).F(). - Warning("Reconcile Host Main - unable to publish host into raft_configuration. Host: %s Err: %v", host.GetName(), admitErr) - return admitErr - } - + // A new staged voter must start before its membership is published to + // existing Keepers. Its host ConfigMap carries a temporary bootstrap Raft + // overlay; the shared configuration still contains only live members. // Snapshot the ensemble and reconcile the StatefulSet against that same snapshot. snap, err := w.reconcileHostStatefulSetWithEnsembleSnapshot(ctx, host, stsReconcileOpts) if err != nil { metrics.HostReconcilesErrors(ctx, host.GetCR()) - // Publication already cleared exclude. Put the host back and republish - // so a 1+1 scale-up does not keep a 2-of-2 quorum with no Pod. - if admitted { - w.rollbackStagedRaftAdmit(ctx, host) - } + return err + } + + // WaitHostPodStarted runs inside the StatefulSet reconciler, including + // unchanged StatefulSets on a spec-repair retry. Only now may the new + // voter enter the established ensemble's shared Raft configuration. + if err := w.publishStartedStagedJoiner(ctx, host); err != nil { + metrics.HostReconcilesErrors(ctx, host.GetCR()) return err } @@ -961,14 +984,14 @@ func (w *worker) reconcileHostPVCs(ctx context.Context, host *api.Host) storage. } func (w *worker) reconcileHostMainDomain(ctx context.Context, host *api.Host, snap hostEnsembleSnapshot) error { - if !host.GetReconcileAttributes().GetStatus().Is(types.ObjectStatusRequested) { - return nil - } - - // Staged 1→N joins are serialized by deferUnreadyStagedAdmit, not by this sleep. + // Even ObjectStatusSame on resume must pass the live Raft/Ready barrier. + // Stopped staged joiners return immediately from the verifier. if hostIsStagedScaleUpJoin(host) { return w.verifyHostEnsembleMembership(ctx, host) } + if !host.GetReconcileAttributes().GetStatus().Is(types.ObjectStatusRequested) { + return nil + } if !snap.rolling { // Fresh bootstrap / resume-from-stopped / recovery: peers start together; diff --git a/pkg/controller/chk/worker-reconciler-chk_test.go b/pkg/controller/chk/worker-reconciler-chk_test.go index e11e35c90..df6f09798 100644 --- a/pkg/controller/chk/worker-reconciler-chk_test.go +++ b/pkg/controller/chk/worker-reconciler-chk_test.go @@ -19,13 +19,17 @@ import ( "errors" "testing" + "github.com/stretchr/testify/require" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/apis/common/types" a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" + "github.com/altinity/clickhouse-operator/pkg/controller/common" "github.com/altinity/clickhouse-operator/pkg/interfaces" + "github.com/altinity/clickhouse-operator/pkg/model/chk/normalizer" ) type fakeCRStatusWriter struct { @@ -125,3 +129,45 @@ func TestMarkReconcileStartReturnsStatusUpdateError(t *testing.T) { t.Fatalf("status = %q, want %q before persistence attempt", got, api.StatusInProgress) } } + +func TestAbortStatusPersistenceIsRequiredBeforeTerminalAcknowledge(t *testing.T) { + cr := &apiChk.ClickHouseKeeperInstallation{ + ObjectMeta: meta.ObjectMeta{Namespace: "test", Name: "keeper"}, + Status: &apiChk.Status{ + TaskID: "stalled-joiner", + Errors: []string{raftStagedJoinerStartupTimeoutStatusMarker + " image unavailable"}, + }, + } + statusWriter := &fakeCRStatusWriter{err: errors.New("status unavailable")} + w := newStatusTestWorker(statusWriter) + + err := w.markReconcileCompletedUnsuccessfully(context.Background(), cr, + errors.Join(errRaftStagedJoinerStartupTimeout, errors.New("startup timeout"), common.ErrCRUDAbort)) + require.ErrorIs(t, err, statusWriter.err, + "failed status persistence must be returned so controller-runtime can retry writing Aborted") + require.Equal(t, apiChk.StatusAborted, cr.Status.GetStatus()) + require.Contains(t, cr.Status.GetErrors()[0], raftStagedJoinerStartupTimeoutStatusMarker) + + statusWriter.err = nil + require.NoError(t, w.markReconcileCompletedUnsuccessfully(context.Background(), cr, common.ErrCRUDAbort)) + require.Equal(t, apiChk.StatusAborted, cr.Status.GetStatus()) + require.Same(t, cr, statusWriter.updated) +} + +// Normalization copies the Kubernetes generation into status.normalized, +// which is the durable last-attempt identity used by the terminal-abort guard. +func TestNormalizedCHKPreservesAttemptedGenerationForTerminalAbort(t *testing.T) { + cr := chkWithHosts(1) + cr.SetGeneration(17) + cr.Status = &apiChk.Status{ + Status: apiChk.StatusAborted, + Errors: []string{raftStagedJoinerStartupTimeoutStatusMarker + " test"}, + } + w := &worker{normalizer: normalizer.New()} + normalized := w.createTemplated(cr) + require.Equal(t, int64(17), normalized.GetGeneration()) + require.NotNil(t, normalized.GetTarget()) + require.Equal(t, int64(17), normalized.GetTarget().GetGeneration()) + require.NotEqual(t, apiChk.StatusAborted, normalized.Status.GetStatus(), + "a changed spec must not inherit an old terminal abort as a new normalization failure") +} diff --git a/pkg/controller/chk/worker-wait-exclude-include-restart.go b/pkg/controller/chk/worker-wait-exclude-include-restart.go index 8a1eddd25..ab0bfbb9a 100644 --- a/pkg/controller/chk/worker-wait-exclude-include-restart.go +++ b/pkg/controller/chk/worker-wait-exclude-include-restart.go @@ -21,7 +21,6 @@ import ( log "github.com/altinity/clickhouse-operator/pkg/announcer" apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" - a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" "github.com/altinity/clickhouse-operator/pkg/util" ) @@ -103,8 +102,8 @@ func (w *worker) includeHost(ctx context.Context, host *api.Host) error { } // includeHostIntoRaftCluster publishes the host into raft_configuration. -// On ConfigMap failure a host that was excluded is excluded again, so the next -// pass retries publication instead of creating a StatefulSet against stale XML. +// On ConfigMap failure a host that was excluded is excluded again, so the +// started joiner can retry publication without claiming membership succeeded. // A host that was not excluded stays that way: SetExclude here would drop an // existing voter from raft_configuration after a conflict. func (w *worker) includeHostIntoRaftCluster(ctx context.Context, host *api.Host) error { @@ -124,47 +123,13 @@ func (w *worker) includeHostIntoRaftCluster(ctx context.Context, host *api.Host) return err } -// rollbackStagedRaftAdmit puts a host back out of raft_configuration after its -// StatefulSet create failed, and republishes so the ConfigMap does not list a -// server that has no Pod. A live StatefulSet stays published: rolling it back -// and letting the next pass's preliminary ConfigMap put it back flaps membership. -// Absence is stagedJoinerStatefulSet returning nil, the same NotFound staging uses. -func (w *worker) rollbackStagedRaftAdmit(ctx context.Context, host *api.Host) { - sts, err := w.stagedJoinerStatefulSet(ctx, host) - if err != nil { - w.a.V(1). - M(host).F(). - Warning("Reconcile Host Main - unable to tell whether %s still has a StatefulSet after admit failed: %v", host.GetName(), err) - return - } - if sts != nil { - w.a.WithEvent(host.GetCR(), a.EventActionReconcile, a.EventReasonReconcileInProgress). - WithAction(host.GetCR()). - M(host).F(). - Info("Keep %s in raft_configuration; its StatefulSet exists", host.GetName()) - return - } - host.GetCR().GetRuntime().LockCommonConfig() - host.GetReconcileAttributes().SetExclude() - err = w.publishRaftConfiguration(ctx, host) - host.GetCR().GetRuntime().UnlockCommonConfig() - if err != nil { - w.a.V(1). - M(host).F(). - Warning("Reconcile Host Main - unable to drop host from raft_configuration after StatefulSet create failed. Host: %s Err: %v", host.GetName(), err) - } -} - -// publishStagedAdmit publishes a host that is still excluded. admitted is false -// when the host was not excluded, so a later error must not roll membership back. -func (w *worker) publishStagedAdmit(ctx context.Context, host *api.Host) (admitted bool, err error) { +// publishStagedAdmit publishes only after the joiner's Pod has Started. +// There is no pre-create publication to roll back after failed startup. +func (w *worker) publishStagedAdmit(ctx context.Context, host *api.Host) error { if host == nil || !host.GetReconcileAttributes().IsExclude() { - return false, nil - } - if err := w.includeHostIntoRaftCluster(ctx, host); err != nil { - return false, err + return nil } - return true, nil + return w.includeHostIntoRaftCluster(ctx, host) } // publishRaftConfiguration writes raft_configuration for the host's CR. diff --git a/pkg/controller/chk/worker.go b/pkg/controller/chk/worker.go index 0b44be583..adfd442ad 100644 --- a/pkg/controller/chk/worker.go +++ b/pkg/controller/chk/worker.go @@ -25,6 +25,7 @@ import ( apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/controller/chk/raftplan" "github.com/altinity/clickhouse-operator/pkg/controller/common" a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" "github.com/altinity/clickhouse-operator/pkg/controller/common/poller/domain" @@ -72,6 +73,17 @@ type worker struct { // publishRaftFn overrides raft ConfigMap publication (tests only). publishRaftFn func(ctx context.Context, host *api.Host) error + // readPublishedRaftMembersFn replaces ONLY the common-ConfigMap membership + // read in tests. Live StatefulSet observation and the admission planner + // still execute normally; production always reads the actual ConfigMap. + readPublishedRaftMembersFn func( + ctx context.Context, cr *apiChk.ClickHouseKeeperInstallation, + ) (published []raftplan.HostID, known bool, err error) + + // readStagedJoinerMembershipFn overrides the live /keeper/config probe + // in unit tests only; production uses the real Keeper connection. + readStagedJoinerMembershipFn func(ctx context.Context, host *api.Host) (bool, error) + // quorumWaitSpent is the wait the Raft disrupt gate has already burned in THIS pass. The // budget is per pass, not per host: a permanently unhealthy peer would otherwise cost the // full timeout on every remaining host, and with ReconcileCHKsThreadsNumber defaulting to 1 @@ -340,10 +352,10 @@ func (w *worker) finalizeReconcileAndMarkCompleted(ctx context.Context, _cr *api return nil } -func (w *worker) markReconcileCompletedUnsuccessfully(ctx context.Context, cr *apiChk.ClickHouseKeeperInstallation, err error) { +func (w *worker) markReconcileCompletedUnsuccessfully(ctx context.Context, cr *apiChk.ClickHouseKeeperInstallation, err error) error { if util.IsContextDone(ctx) { log.V(1).Info("Reconcile is aborted. cr: %s ", cr.GetName()) - return + return ctx.Err() } switch { @@ -352,13 +364,15 @@ func (w *worker) markReconcileCompletedUnsuccessfully(ctx context.Context, cr *a case errors.Is(err, common.ErrCRUDAbort): cr.EnsureStatus().ReconcileAbort() } - w.c.updateCRObjectStatus(ctx, cr, types.UpdateStatusOptions{ + if statusErr := w.c.updateCRObjectStatus(ctx, cr, types.UpdateStatusOptions{ CopyStatusOptions: types.CopyStatusOptions{ CopyStatusFieldGroup: types.CopyStatusFieldGroup{ FieldGroupMain: true, }, }, - }) + }); statusErr != nil { + return statusErr + } w.a.V(1). WithEvent(cr, a.EventActionReconcile, a.EventReasonReconcileFailed). @@ -366,6 +380,7 @@ func (w *worker) markReconcileCompletedUnsuccessfully(ctx context.Context, cr *a WithActions(cr). M(cr).F(). Warning("reconcile completed UNSUCCESSFULLY, task id: %s", cr.GetSpecT().GetTaskID()) + return nil } func (w *worker) setHostStatusesPreliminary(ctx context.Context, cr *apiChk.ClickHouseKeeperInstallation) { diff --git a/pkg/controller/common/statefulset/statefulset-reconciler-options.go b/pkg/controller/common/statefulset/statefulset-reconciler-options.go index 134c46885..bd6eb0ce9 100644 --- a/pkg/controller/common/statefulset/statefulset-reconciler-options.go +++ b/pkg/controller/common/statefulset/statefulset-reconciler-options.go @@ -15,9 +15,10 @@ package statefulset type ReconcileOptions struct { - forceRecreate bool - waitUntilStarted bool - waitUntilReady bool + forceRecreate bool + waitUntilStarted bool + waitUntilReady bool + waitStartedOnSame bool } func NewReconcileStatefulSetOptions() *ReconcileOptions { @@ -44,6 +45,23 @@ func (o *ReconcileOptions) WaitUntilStarted() bool { return o.waitUntilStarted } +// SetWaitUntilStartedOnSame rechecks startup even for an existing, unchanged +// StatefulSet. Used by CHK staged joins on a retry after an interrupted create: +// the STS can be Same while its Pod never started (e.g. a broken image). +// This is deliberately opt-in and does not impose a Ready wait on the joiner. +func (o *ReconcileOptions) SetWaitUntilStartedOnSame() *ReconcileOptions { + o = o.Ensure() + o.waitStartedOnSame = true + return o +} + +func (o *ReconcileOptions) WaitUntilStartedOnSame() bool { + if o == nil { + return false + } + return o.waitStartedOnSame +} + func (o *ReconcileOptions) SetWaitUntilReady() *ReconcileOptions { o = o.Ensure() o.waitUntilReady = true diff --git a/pkg/controller/common/statefulset/statefulset-reconciler.go b/pkg/controller/common/statefulset/statefulset-reconciler.go index 823cfa42a..1e6002172 100644 --- a/pkg/controller/common/statefulset/statefulset-reconciler.go +++ b/pkg/controller/common/statefulset/statefulset-reconciler.go @@ -150,6 +150,19 @@ func (r *Reconciler) ReconcileStatefulSet( defer r.a.V(2).M(host).E().Info(util.NamespaceNameString(newStatefulSet)) if host.GetReconcileAttributes().GetStatus().Is(types.ObjectStatusSame) { + // An unchanged StatefulSet does not imply its Pod ever started. + // CHK serial admission can be interrupted after creating a joiner's + // StatefulSet but before kubelet starts its image. The next pass + // must still enforce the bounded startup wait (without waiting for + // Raft Ready, which may require the existing members to roll). + if opts.WaitUntilStartedOnSame() { + if err := r.hostObjectsPoller.WaitHostPodStarted(ctx, host); err != nil { + r.a.V(1).M(host).F().Warning( + "Existing StatefulSet pod did not start before timeout: %s, err: %v", + util.NamespaceNameString(newStatefulSet), err) + return r.fallback.OnStatefulSetCreateFailed(ctx, host) + } + } r.a.V(2).M(host).F().Info("No need to reconcile THE SAME StatefulSet: %s", util.NamespaceNameString(newStatefulSet)) if register { host.GetCR().IEnsureStatus().HostUnchanged() diff --git a/pkg/controller/common/statefulset/statefulset-reconciler_test.go b/pkg/controller/common/statefulset/statefulset-reconciler_test.go index 96e8b1784..5bd9193ad 100644 --- a/pkg/controller/common/statefulset/statefulset-reconciler_test.go +++ b/pkg/controller/common/statefulset/statefulset-reconciler_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/apis/common/types" "github.com/altinity/clickhouse-operator/pkg/controller/common" announcer "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" "github.com/altinity/clickhouse-operator/pkg/controller/common/poller" @@ -120,8 +121,10 @@ func (n *fakeNamer) Names(what interfaces.NameType, params ...any) []string { // fakePoller is an IHostObjectsPoller test double. The default zero value is a // successful wait; tests that need a scale-to-0 timeout set waitReadyErr. type fakePoller struct { - waitReadyCalls int - waitReadyErr error + waitReadyCalls int + waitReadyErr error + waitStartedCalls int + waitStartedErr error } func (p *fakePoller) WaitHostStatefulSetReady(ctx context.Context, host *api.Host) error { @@ -129,7 +132,8 @@ func (p *fakePoller) WaitHostStatefulSetReady(ctx context.Context, host *api.Hos return p.waitReadyErr } func (p *fakePoller) WaitHostPodStarted(ctx context.Context, host *api.Host) error { - return nil + p.waitStartedCalls++ + return p.waitStartedErr } // fakePod is a minimal IKubePod test double. Only Delete is exercised by the @@ -609,3 +613,43 @@ func (f *cancelOnGetSTS) Get(_ context.Context, _ ...any) (*apps.StatefulSet, er f.cancel() return nil, context.Canceled } + +type testAbortStartupFallback struct{} + +func (*testAbortStartupFallback) OnStatefulSetCreateFailed(context.Context, *api.Host) common.ErrorCRUD { + return common.ErrCRUDAbort +} +func (*testAbortStartupFallback) OnStatefulSetUpdateFailed( + context.Context, *apps.StatefulSet, *api.Host, interfaces.IKubeSTS, +) common.ErrorCRUD { + return common.ErrCRUDAbort +} + +// An interrupted Keeper scale-up can leave an unchanged StatefulSet whose +// image never started. Opted-in reconciliation must still time out and abort +// rather than treating the matching desired STS as a successful launch. +func TestReconcileStatefulSetSameWaitsForStagedStartup(t *testing.T) { + ctx := context.Background() + p := &fakePoller{waitStartedErr: poller.ErrTimeout} + r := newReconciler(&fakeSTS{}, "keeper-0") + r.hostObjectsPoller = p + r.fallback = &testAbortStartupFallback{} + h := hostWithCR("ns", "keeper") + h.Runtime.DesiredStatefulSet = &apps.StatefulSet{ + ObjectMeta: meta.ObjectMeta{Namespace: "ns", Name: "keeper-0"}, + } + h.GetReconcileAttributes().SetStatus(types.ObjectStatusSame) + + require.NoError(t, r.ReconcileStatefulSet(ctx, h, false, NewReconcileStatefulSetOptions())) + require.Zero(t, p.waitStartedCalls, "ordinary unchanged StatefulSets should retain the fast path") + + opts := NewReconcileStatefulSetOptions().SetWaitUntilStartedOnSame() + require.ErrorIs(t, r.ReconcileStatefulSet(ctx, h, false, opts), common.ErrCRUDAbort, + "a failed startup wait must escalate to the configured Keeper create fallback") + require.Equal(t, 1, p.waitStartedCalls) + + p.waitStartedErr = nil + require.NoError(t, r.ReconcileStatefulSet(ctx, h, false, opts), + "a Pod that has started may continue even if Keeper is not Ready") + require.Equal(t, 2, p.waitStartedCalls) +} diff --git a/pkg/model/chk/config/const.go b/pkg/model/chk/config/const.go index 4017bbe61..7ca229a7d 100644 --- a/pkg/model/chk/config/const.go +++ b/pkg/model/chk/config/const.go @@ -82,3 +82,13 @@ const ( // Generator.getPlaintextListenerRemoval. configListenersCommon = "common-listeners" ) + +const ( + // BootstrapRaftHostConfigKey is intentionally not an .xml file: Keeper must + // not load the host bootstrap Raft overlay from conf.d before the shared Raft + // configuration in keeper_config.d. The projected volume renames it to XML. + BootstrapRaftHostConfigKey = "chop-generated-bootstrap-raft.fragment" + // BootstrapRaftProjectedFile sorts after chop-generated-raft.xml, so the + // host-local replace="replace" applies last in keeper_config.d. + BootstrapRaftProjectedFile = "zz-chop-generated-bootstrap-raft.xml" +) diff --git a/pkg/model/chk/config/files_generator_domain.go b/pkg/model/chk/config/files_generator_domain.go index da578e9b3..29cc32bee 100644 --- a/pkg/model/chk/config/files_generator_domain.go +++ b/pkg/model/chk/config/files_generator_domain.go @@ -40,6 +40,10 @@ func (c *FilesGeneratorDomain) CreateConfigFilesGroupUsers(configSections map[st } func (c *FilesGeneratorDomain) CreateConfigFilesGroupHost(configSections map[string]string, options *FilesGeneratorOptions) { + if options != nil && options.BootstrapRaft { + util.IncludeNonEmpty(configSections, BootstrapRaftHostConfigKey, + c.configGenerator.getBootstrapRaftConfig(options.GetHost(), options.GetRaftOptions())) + } util.IncludeNonEmpty(configSections, createConfigSectionFilename(configServerId), c.configGenerator.getHostServerId(options.GetHost())) util.IncludeNonEmpty(configSections, createConfigSectionFilename(configListeners), c.configGenerator.getHostListenersOverride(options.GetHost())) } diff --git a/pkg/model/chk/config/files_generator_options.go b/pkg/model/chk/config/files_generator_options.go index ef879aef0..70d0c09c8 100644 --- a/pkg/model/chk/config/files_generator_options.go +++ b/pkg/model/chk/config/files_generator_options.go @@ -21,8 +21,9 @@ import ( // FilesGeneratorOptions specifies options for configuration files generator type FilesGeneratorOptions struct { - host *api.Host - RaftOptions *config.HostSelector + host *api.Host + RaftOptions *config.HostSelector + BootstrapRaft bool } // defaultFilesGeneratorOptions creates new default options for files generator @@ -35,6 +36,14 @@ func NewFilesGeneratorOptions() *FilesGeneratorOptions { return &FilesGeneratorOptions{} } +// SetBootstrapRaft enables a temporary per-host Raft configuration overlay. +func (o *FilesGeneratorOptions) SetBootstrapRaft(enabled bool) *FilesGeneratorOptions { + if o != nil { + o.BootstrapRaft = enabled + } + return o +} + func (o *FilesGeneratorOptions) GetHost() *api.Host { if o == nil { return nil diff --git a/pkg/model/chk/config/generator.go b/pkg/model/chk/config/generator.go index ecc2e4863..dc0d2f2f6 100644 --- a/pkg/model/chk/config/generator.go +++ b/pkg/model/chk/config/generator.go @@ -17,6 +17,7 @@ package config import ( "bytes" "fmt" + "strings" log "github.com/altinity/clickhouse-operator/pkg/announcer" chi "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" @@ -101,6 +102,24 @@ func (c *Generator) getRaftConfig(selector *config.HostSelector) string { return chi.NewSettings().Set("keeper_server/raft_configuration", chi.MustNewSettingScalarFromAny(raft).SetEmbed()).ClickHouseConfig() } +// getBootstrapRaftConfig gives a staged joiner its own server ID while the +// established ensemble's shared configuration still excludes that joiner. +// The host-specific overlay replaces the shared raft_configuration. +func (c *Generator) getBootstrapRaftConfig(host *chi.Host, selector *config.HostSelector) string { + if host == nil { + return "" + } + bootstrap := config.NewHostSelector() + _ = c.cr.WalkHosts(func(peer *chi.Host) error { + if peer != host && selector != nil && selector.Exclude(peer) { + bootstrap.ExcludeHost(peer) + } + return nil + }) + xml := c.getRaftConfig(bootstrap) + return strings.Replace(xml, "", "", 1) +} + // getHostServerId builds server id config for the host func (c *Generator) getHostServerId(host *chi.Host) string { return chi.NewSettings().Set("keeper_server/server_id", chi.MustNewSettingScalarFromAny(getServerId(host))).ClickHouseConfig() diff --git a/pkg/model/chk/creator/config_map.go b/pkg/model/chk/creator/config_map.go index 830027af5..2957fe5ce 100644 --- a/pkg/model/chk/creator/config_map.go +++ b/pkg/model/chk/creator/config_map.go @@ -61,6 +61,9 @@ func (m *ConfigMapManager) CreateConfigMap(what interfaces.ConfigMapType, params if len(params) > 0 { host = params[0].(*api.Host) options = config.NewFilesGeneratorOptions().SetHost(host) + if len(params) > 1 { + options = params[1].(*config.FilesGeneratorOptions).SetHost(host) + } return m.createConfigMapHost(host, options) } } diff --git a/pkg/model/chk/volume/volume.go b/pkg/model/chk/volume/volume.go index 73342b8aa..3d3ade32c 100644 --- a/pkg/model/chk/volume/volume.go +++ b/pkg/model/chk/volume/volume.go @@ -16,6 +16,7 @@ package volume import ( apps "k8s.io/api/apps/v1" + core "k8s.io/api/core/v1" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/interfaces" @@ -57,10 +58,10 @@ func (m *Manager) stsSetupVolumesForConfigMaps(statefulSet *apps.StatefulSet, ho configMapCommonUsersName := m.namer.Name(interfaces.NameConfigMapCommonUsers, m.cr) configMapHostName := m.namer.Name(interfaces.NameConfigMapHost, host) - // Add all ConfigMap objects as Volume objects of type ConfigMap + // Mount shared and host configuration, projecting the late bootstrap overlay k8s.StatefulSetAppendVolumes( statefulSet, - k8s.CreateVolumeForConfigMap(configMapCommonName), + createProjectedCommonConfigVolume(configMapCommonName, configMapHostName), k8s.CreateVolumeForConfigMap(configMapCommonUsersName), k8s.CreateVolumeForConfigMap(configMapHostName), ) @@ -75,6 +76,39 @@ func (m *Manager) stsSetupVolumesForConfigMaps(statefulSet *apps.StatefulSet, ho ) } +// createProjectedCommonConfigVolume combines the shared Raft configuration +// with an optional, host-specific bootstrap replacement in keeper_config.d. +// Keeper loads conf.d first, so the replacement must sort *after* the shared +// chop-generated-raft.xml in keeper_config.d. +// +// A projected volume (not subPath) keeps ConfigMap changes live: when the host +// no longer needs bootstrap Raft, the overlay file is removed automatically. +func createProjectedCommonConfigVolume(commonName, hostName string) core.Volume { + var defaultMode int32 = 0644 + optional := true + return core.Volume{ + Name: commonName, + VolumeSource: core.VolumeSource{ + Projected: &core.ProjectedVolumeSource{ + DefaultMode: &defaultMode, + Sources: []core.VolumeProjection{ + {ConfigMap: &core.ConfigMapProjection{ + LocalObjectReference: core.LocalObjectReference{Name: commonName}, + }}, + {ConfigMap: &core.ConfigMapProjection{ + LocalObjectReference: core.LocalObjectReference{Name: hostName}, + Items: []core.KeyToPath{{ + Key: config.BootstrapRaftHostConfigKey, + Path: config.BootstrapRaftProjectedFile, + }}, + Optional: &optional, + }}, + }, + }, + }, + } +} + // stsSetupVolumesUserDataWithFixedPaths // appends VolumeMounts for Data and Log VolumeClaimTemplates on all containers. // Creates VolumeMounts for Data and Log volumes in case these volume templates are specified in `templates`. diff --git a/pkg/model/chk/volume/volume_test.go b/pkg/model/chk/volume/volume_test.go new file mode 100644 index 000000000..03cfba745 --- /dev/null +++ b/pkg/model/chk/volume/volume_test.go @@ -0,0 +1,46 @@ +package volume + +import ( + "strings" + "testing" + + "github.com/altinity/clickhouse-operator/pkg/model/chk/config" +) + +// Keeper loads conf.d before keeper_config.d. The bootstrap replacement must +// be projected into keeper_config.d after the shared Raft configuration, and +// the projected ConfigMap must remain live-updatable (no subPath mounts). +func TestProjectedCommonConfigVolumeBootstrapOrdering(t *testing.T) { + v := createProjectedCommonConfigVolume("common-cm", "host-cm") + if v.Name != "common-cm" || v.Projected == nil || v.ConfigMap != nil { + t.Fatalf("expected projected common config volume, got %#v", v) + } + if v.Projected.DefaultMode == nil || *v.Projected.DefaultMode != 0644 { + t.Fatalf("unexpected projected file permissions: %#v", v.Projected.DefaultMode) + } + sources := v.Projected.Sources + if len(sources) != 2 { + t.Fatalf("expected shared and host ConfigMap sources, got %d", len(sources)) + } + if sources[0].ConfigMap == nil || sources[0].ConfigMap.Name != "common-cm" || + len(sources[0].ConfigMap.Items) != 0 || sources[0].ConfigMap.Optional != nil { + t.Fatalf("expected all common ConfigMap keys to be mandatory: %#v", sources[0]) + } + host := sources[1].ConfigMap + if host == nil || host.Name != "host-cm" || host.Optional == nil || !*host.Optional { + t.Fatalf("expected optional host bootstrap source: %#v", sources[1]) + } + if len(host.Items) != 1 { + t.Fatalf("expected one projected bootstrap key, got %#v", host.Items) + } + item := host.Items[0] + if item.Key != config.BootstrapRaftHostConfigKey || item.Path != config.BootstrapRaftProjectedFile { + t.Fatalf("unexpected bootstrap projection: %#v", item) + } + if strings.HasSuffix(item.Key, ".xml") { + t.Fatalf("host bootstrap source must not be loaded from conf.d: %q", item.Key) + } + if item.Path <= "chop-generated-raft.xml" || !strings.HasSuffix(item.Path, ".xml") { + t.Fatalf("bootstrap must override shared Raft XML last: %q", item.Path) + } +} diff --git a/pkg/model/k8s/pod.go b/pkg/model/k8s/pod.go index 6b6f8e9fa..97fabcb29 100644 --- a/pkg/model/k8s/pod.go +++ b/pkg/model/k8s/pod.go @@ -70,24 +70,30 @@ func PodHasNotReadyContainers(pod *core.Pod) bool { } func PodHasAllContainersStarted(pod *core.Pod) bool { - allContainersStarted := true - for _, containerStatus := range pod.Status.ContainerStatuses { - // Started indicates whether the container has finished its postStart lifecycle hook - // and passed its startup probe. - // Initialized as false, becomes true after startupProbe is considered - // successful. Resets to false when the container is restarted, or if kubelet - // loses state temporarily. In both cases, startup probes will run again. - // Is always true when no startupProbe is defined and container is running and - // has passed the postStart lifecycle hook. The null value must be treated the - // same as false. - if (containerStatus.Started != nil) && (*containerStatus.Started) { - // Current container is started. no changes in all status - } else { - // Current container is NOT started - allContainersStarted = false + // Kubelet may not have populated containerStatuses yet. In particular, an + // ImagePullBackOff pod briefly has no statuses after StatefulSet creation. + // Treating an empty slice as vacuously true lets the startup waiter succeed + // without a single container ever starting. + if pod == nil || len(pod.Spec.Containers) == 0 || + len(pod.Status.ContainerStatuses) != len(pod.Spec.Containers) { + return false + } + for _, container := range pod.Spec.Containers { + started := false + for _, status := range pod.Status.ContainerStatuses { + // Started is only true after the postStart hook / startup probe. + // Nil means unknown, not started. Match by name so a missing + // sidecar cannot be masked by an unrelated status entry. + if status.Name == container.Name && status.Started != nil && *status.Started { + started = true + break + } + } + if !started { + return false } } - return allContainersStarted + return true } func PodHasNotStartedContainers(pod *core.Pod) bool { diff --git a/pkg/model/k8s/pod_test.go b/pkg/model/k8s/pod_test.go index 0b0b47405..3ed670a16 100644 --- a/pkg/model/k8s/pod_test.go +++ b/pkg/model/k8s/pod_test.go @@ -47,3 +47,54 @@ func TestPodContainersRestartCountsGet(t *testing.T) { require.Nil(t, PodContainersRestartCountsGet(nil, "clickhouse")) require.Nil(t, PodContainersRestartCountsGet(&core.Pod{})) } + +func TestPodHasAllContainersStartedRequiresObservedStatuses(t *testing.T) { + started := true + notStarted := false + t.Run("nil pod", func(t *testing.T) { + require.False(t, PodHasAllContainersStarted(nil)) + }) + t.Run("status not yet reported for image pull", func(t *testing.T) { + pod := &core.Pod{Spec: core.PodSpec{ + Containers: []core.Container{{Name: "keeper"}}, + }} + require.False(t, PodHasAllContainersStarted(pod)) + require.True(t, PodHasNotStartedContainers(pod)) + }) + t.Run("broken image reports waiting", func(t *testing.T) { + pod := &core.Pod{ + Spec: core.PodSpec{Containers: []core.Container{{Name: "keeper"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{{ + Name: "keeper", Started: ¬Started, + State: core.ContainerState{Waiting: &core.ContainerStateWaiting{Reason: "ImagePullBackOff"}}, + }}}, + } + require.False(t, PodHasAllContainersStarted(pod)) + }) + t.Run("one missing sidecar status", func(t *testing.T) { + pod := &core.Pod{ + Spec: core.PodSpec{Containers: []core.Container{{Name: "keeper"}, {Name: "sidecar"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{ + {Name: "keeper", Started: &started}, + }}, + } + require.False(t, PodHasAllContainersStarted(pod)) + }) + t.Run("all containers started but not necessarily ready", func(t *testing.T) { + pod := &core.Pod{ + Spec: core.PodSpec{Containers: []core.Container{{Name: "keeper"}, {Name: "sidecar"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{ + {Name: "sidecar", Started: &started}, + {Name: "keeper", Started: &started}, + }}, + } + require.True(t, PodHasAllContainersStarted(pod)) + }) + t.Run("a nil Started status cannot count as started", func(t *testing.T) { + pod := &core.Pod{ + Spec: core.PodSpec{Containers: []core.Container{{Name: "keeper"}}}, + Status: core.PodStatus{ContainerStatuses: []core.ContainerStatus{{Name: "keeper"}}}, + } + require.False(t, PodHasAllContainersStarted(pod)) + }) +} diff --git a/tests/e2e/steps.py b/tests/e2e/steps.py index cbd033faf..8dc272520 100644 --- a/tests/e2e/steps.py +++ b/tests/e2e/steps.py @@ -44,7 +44,7 @@ def _dump_failed_test_namespace(test, ns, shell): operator_ns = getattr(test.context, "operator_namespace", None) or ns print(f"\n=== DEBUG DUMP (test failed) namespace={ns} ===") - for kind in ("pods", "chi", "chk"): + for kind in ("pods", "statefulsets", "chi", "chk"): try: print(f"\n--- {kind.upper()} ---") items = kubectl.launch(f"get {kind}", ns=ns, ok_to_fail=True, shell=shell) @@ -52,6 +52,16 @@ def _dump_failed_test_namespace(test, ns, shell): except Exception as exc: print(f"failed to list {kind}: {exc}") + try: + print("\n--- Kubernetes events (last 30 lines) ---") + events = kubectl.launch( + "get events --sort-by=.lastTimestamp", + ns=ns, ok_to_fail=True, timeout=15, shell=shell, + ) + print("\n".join((events or "").splitlines()[-30:]) or "(no events)") + except Exception as exc: + print(f"failed to list events: {exc}") + try: print(f"\n--- ClickHouse log (last {_dump_log_lines} lines) ---") chis = kubectl.get_obj_names("", obj_type = "chi", kind = "", ns=ns, shell=shell) @@ -76,6 +86,35 @@ def _dump_failed_test_namespace(test, ns, shell): except Exception as exc: print(f"failed to fetch CHI action plan: {exc}") + # Keeper-specific failure context: published vs live Raft membership. + try: + chks = kubectl.get_obj_names("", obj_type="chk", kind="", ns=ns, shell=shell) + for chk in chks: + print(f"\n--- Keeper Raft ({chk}) ---") + published = kubectl.launch( + f"get cm chk-{chk}-common-configd " + "-o go-template='{{index .data \"chop-generated-raft.xml\"}}'", + ns=ns, ok_to_fail=True, timeout=15, shell=shell, + ) + print(f"Published raft_configuration:\n{published or '(unavailable)'}") + for pod in sorted(kubectl.get_chk_pod_names(chk, ns=ns, shell=shell))[:2]: + print(f"\n# {pod}: live /keeper/config") + live = kubectl.launch( + f"exec {pod} -c clickhouse-keeper -- " + 'clickhouse-keeper-client --history-file=/dev/null ' + '-h 127.0.0.1 -p 2181 -q "get \'/keeper/config\'"', + ns=ns, ok_to_fail=True, timeout=15, shell=shell, + ) + print(live or "(unavailable)") + print(f"# {pod}: Keeper log (last {_dump_log_lines} lines)") + logs = kubectl.launch( + f"logs {pod} -c clickhouse-keeper --tail={_dump_log_lines}", + ns=ns, ok_to_fail=True, timeout=15, shell=shell, + ) + print(logs or "(unavailable)") + except Exception as exc: + print(f"failed to fetch Keeper Raft diagnostics: {exc}") + try: print(f"\n--- Operator log (last {_dump_log_lines} lines) ---") operator_pod = kubectl.get_operator_pod(ns=operator_ns, shell=shell) diff --git a/tests/e2e/test_operator.py b/tests/e2e/test_operator.py index a49019836..2d4fea186 100644 --- a/tests/e2e/test_operator.py +++ b/tests/e2e/test_operator.py @@ -6802,14 +6802,35 @@ def test_010064(self): # satisfy the not-restarted asserts below without ever having read a timestamp. A missing # pod raises IndexError inside the helper instead, which fails loudly on its own. assert start_time != "", error("could not read the ClickHouse container start time") - connected_time = "" with And("CHI is connected to Keeper"): connected_time = clickhouse.query(chi, "SELECT connected_time from system.zookeeper_connection") assert connected_time != "" + # A taskID-only CHK update does not change the resolved endpoint list. + # The watcher must record a skip, not reconfigure the CHI. Connected_time + # is not a valid proxy for CHI reconfiguration: an otherwise healthy + # ClickHouse can create a new ZooKeeper session independently. + zk_config_name = f"chi-{chi}-deploy-confd-default-0-0" + zk_config_before = kubectl.get("configmap", zk_config_name)["data"]["chop-generated-zookeeper.xml"] + with When("Trigger CHK reconcile by patching taskID"): kubectl.force_chk_reconcile(chk, "keeper-watch-test") + with Then("Watcher skips CHI reconcile when Keeper endpoints are unchanged"): + skipped = False + for i in range(1, 16): + events = kubectl.get("events", "").get("items", []) + skipped = any( + event.get("reason") == "KeeperUpdateNoEndpointChange" + and event.get("involvedObject", {}).get("kind") == "ClickHouseInstallation" + and event.get("involvedObject", {}).get("name") == chi + for event in events + ) + if skipped: + break + retry_sleep(i, 5, "Waiting for CHK watcher skip event") + assert skipped, error("CHK watcher did not report skipping unchanged Keeper endpoints") + with Then("Confirm CHI is complete"): kubectl.wait_chi_status(chi, "Completed") @@ -6817,9 +6838,9 @@ def test_010064(self): new_start_time = kubectl.get_clickhouse_start(chi) assert new_start_time == start_time, error("CHI has been restarted") - with Then("CHI does not reconnect to Keeper"): - new_connected_time = clickhouse.query(chi, "SELECT connected_time from system.zookeeper_connection") - assert new_connected_time == connected_time, error("ClickHouse reconnected to Keeper") + with Then("CHI ZooKeeper configuration has not changed"): + zk_config_after = kubectl.get("configmap", zk_config_name)["data"]["chop-generated-zookeeper.xml"] + assert zk_config_after == zk_config_before, error("CHI ZooKeeper configuration changed without Keeper endpoint changes") with When("Rescale Keeper to 3 nodes"): kubectl.create_and_check(