From 3d34a68d3fa933cb99e0667b9b396221e359a4d8 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 16 Jul 2026 08:21:22 -0700 Subject: [PATCH] refactor(node): read Reconcile as a step list; extract hold predicates Behavior-preserving readability refactor of SeiNodeReconciler.Reconcile so the body reads as a sequence of named steps instead of prose-annotated orchestration. The two inline hold predicates become named methods that carry their rationale as doc comments: - holdInitialSTS RHS -> shouldHoldInitialStatefulSet(node) (statefulset.go) - holdForWorkflow RHS -> adoptedWorkflowIsExecuting(node) (workflow.go) The StateSyncReady-ordering invariant is kept (trimmed) at its call site; the status-snapshot bundle and the two flush-and-return exits are left inline by design. No control-flow change; node+planner tests pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/controller/node/controller.go | 33 +++++-------------------- internal/controller/node/statefulset.go | 15 +++++++++++ internal/controller/node/workflow.go | 14 +++++++++++ 3 files changed, 35 insertions(+), 27 deletions(-) diff --git a/internal/controller/node/controller.go b/internal/controller/node/controller.go index 23d44c35..c5829d37 100644 --- a/internal/controller/node/controller.go +++ b/internal/controller/node/controller.go @@ -118,13 +118,10 @@ func (r *SeiNodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return r.Status().Patch(ctx, node, statusBase) } - // Resolve the always-present StateSyncReady condition before the Failed and - // Paused early-returns so it rides the existing flush on every path (Failed - // flush, Paused flush, and the normal end-of-reconcile patch) — no separate - // status write. Fail-closed enforcement lives in ResolvePlan, which declines - // to build a state-sync plan when this condition isn't True; that keeps - // terminal-plan cleanup and non-state-sync work running. A blocked gate - // requeues (see end of reconcile) without aborting the steps below. + // Resolved before the Failed/Paused early-returns so this always-present + // condition's mutation rides the existing flush on every exit path — no + // separate status write. A blocked gate only requeues (see steadyStateRequeue); + // fail-closed enforcement lives in ResolvePlan, so the steps below still run. stateSyncBlocked := r.reconcileStateSyncGate(node) // Failed is terminal — flush any condition updates and exit. @@ -137,26 +134,8 @@ func (r *SeiNodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, nil } - // Hold initial StatefulSet creation while the state-sync gate suppresses - // the init plan: the plan's EnsureDataPVC task is the only creator of the - // data PVC the pod mounts by claimName, so an STS created now would strand - // a Pending pod until the gate opens. Only initial creation is held - // (Status.StatefulSet == nil) — an existing STS is never touched, and - // StateSyncBlocksPlan is pre-Running-only, so a Running node's STS keeps - // syncing through transient syncer-source errors. One narrow stall is - // accepted: if the impostor branch just cleared Status.StatefulSet and the - // gate is blocked in the same window, STS re-creation waits for the gate's - // poll to re-resolve. - holdInitialSTS := planner.StateSyncBlocksPlan(node) && node.Status.StatefulSet == nil - // No roll mid-hold: a workflow ACTIVELY executing may have seid gated and its - // data directory mid-wipe. reconcileStatefulSet's unconditional SSA would - // push a spec.image template change immediately, defeating the drift - // suppression below (which only gates plan-driven replace-pod, not the direct - // apply). Skip the apply only while the adopted workflow is executing; a - // parked-Failed hold releases the skip so sidecar hotfixes and impostor-STS - // recovery aren't suspended indefinitely (the readiness gate keeps seid held, - // and a pod replacement of a parked node is in the safe interrupt class). - holdForWorkflow := node.Status.AdoptedWorkflow != nil && !adoptedWorkflowParkedFailed(node) + holdInitialSTS := shouldHoldInitialStatefulSet(node) + holdForWorkflow := adoptedWorkflowIsExecuting(node) if !holdInitialSTS && !holdForWorkflow { if err := r.reconcileStatefulSet(ctx, node); err != nil { return ctrl.Result{}, fmt.Errorf("reconciling statefulset: %w", err) diff --git a/internal/controller/node/statefulset.go b/internal/controller/node/statefulset.go index 1454389d..c48fa9e2 100644 --- a/internal/controller/node/statefulset.go +++ b/internal/controller/node/statefulset.go @@ -6,6 +6,7 @@ import ( seiv1alpha1 "github.com/sei-protocol/sei-k8s-controller/api/v1alpha1" "github.com/sei-protocol/sei-k8s-controller/internal/noderesource" + "github.com/sei-protocol/sei-k8s-controller/internal/planner" ) // reconcileStatefulSet syncs the owned StatefulSet via the typed @@ -37,3 +38,17 @@ func (r *SeiNodeReconciler) reconcileStatefulSet(ctx context.Context, node *seiv } return nil } + +// shouldHoldInitialStatefulSet reports whether initial StatefulSet creation must +// wait for the state-sync gate to open. The init plan's EnsureDataPVC task is the +// only creator of the data PVC the pod mounts by claimName, so an STS created +// while the gate suppresses that plan would strand a Pending pod until the gate +// opens. Only initial creation is held (Status.StatefulSet == nil): an existing +// STS is never touched, and StateSyncBlocksPlan is pre-Running-only, so a Running +// node's STS keeps syncing through transient syncer-source errors. One narrow +// stall is accepted: if the impostor branch just cleared Status.StatefulSet and +// the gate is blocked in the same window, STS re-creation waits for the gate's +// poll to re-resolve. +func shouldHoldInitialStatefulSet(node *seiv1alpha1.SeiNode) bool { + return planner.StateSyncBlocksPlan(node) && node.Status.StatefulSet == nil +} diff --git a/internal/controller/node/workflow.go b/internal/controller/node/workflow.go index 6844d4f4..65614030 100644 --- a/internal/controller/node/workflow.go +++ b/internal/controller/node/workflow.go @@ -724,6 +724,20 @@ func setWorkflowInProgress(node *seiv1alpha1.SeiNode, status metav1.ConditionSta }) } +// adoptedWorkflowIsExecuting reports whether an adopted workflow is actively +// occupying the node — driving execution rather than parked after failure. The +// node holds initial-STS apply off while this is true: an actively-executing +// workflow may have seid gated with its data directory mid-wipe, and +// reconcileStatefulSet's unconditional SSA would push a spec.image template +// change immediately, defeating the plan-driven drift suppression (which gates +// only replace-pod, not the direct apply). A parked-Failed hold +// (adoptedWorkflowParkedFailed) releases the skip so sidecar hotfixes and +// impostor-STS recovery aren't suspended indefinitely — the readiness gate keeps +// seid held, and replacing a parked node's pod is in the safe interrupt class. +func adoptedWorkflowIsExecuting(node *seiv1alpha1.SeiNode) bool { + return node.Status.AdoptedWorkflow != nil && !adoptedWorkflowParkedFailed(node) +} + // adoptedWorkflowParkedFailed reports whether the adopted workflow has failed // and the node is parked held (WorkflowInProgress=True/WorkflowFailedHeld) — // distinct from an actively-executing hold (reason WorkflowRunning). Read from