From bb7ba33c7d0479cd0cbd0b936a6294bdfbd3335f Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 20 Aug 2026 11:35:49 -0700 Subject: [PATCH 1/2] fix(sidecar): restore the seictl compatibility name in the image MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #504 shipped this shim in its first commit and removed it in its third, before either half of its own stated removal condition was met. The condition was: remove only after the controller stops rendering Command AND every cell has rolled past that controller. The first half happened at 186d65a. The second has not happened anywhere — no cell has adopted this image at all. Without the shim the two working combinations are diagonal: controller renders ["seictl","serve"] + old seictl image -> works (today) controller renders ["seictl","serve"] + this image -> exec seictl: not found controller renders nothing + this image -> works (target) controller renders nothing + old seictl image -> bare seictl, exits 0 There is no ordering between those two that passes through a working state, so image and controller have to flip together in every cell at once, and the rollback wedges: every mode's plan runs config-patch (a polled sidecar task) before replace-pod, so reverting the controller submits to a sidecar that is already dead. The submit fails, replace-pod is never reached, and the broken pod is never replaced — recovery is a manual pod delete per node. With this name present there is never a dead sidecar to wedge on, and reverting the controller alone is sufficient. Restoring it makes THIS image valid under both controller versions, which decomposes the cutover into three independently reversible steps: adopt the image, then bump the controller, then delete the shim. Note the asymmetry — the image becomes version-agnostic, the controller does not. Old image plus new controller is still broken, so the safe rollback is "revert the controller, leave the image", never the reverse. A second COPY rather than a symlink: the final stage is distroless and has no shell to run `ln`. Costs one duplicated static binary in the layer. Verified argv[0] independence empirically rather than by reading: built the binary, copied it to `seictl`, and ran both names. `seictl serve` with SEI_HOME unset reaches serve's required-flag check; with a relative SEI_HOME it hits validateHome with the same message; output is identical under either name. The root command's Name is a hardcoded literal and nothing in sidecar/ or sidecarapi/ reads os.Args[0], so dispatch keys only on argv[1]. Dockerfile-only change; no Go behaviour is affected. The image build is not verified locally (no Docker daemon available) — CI's publish-sidecar job covers it. Co-Authored-By: Claude Opus 5 (1M context) --- sidecar/Dockerfile | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/sidecar/Dockerfile b/sidecar/Dockerfile index 7657796d..d64a3f98 100644 --- a/sidecar/Dockerfile +++ b/sidecar/Dockerfile @@ -45,4 +45,34 @@ COPY --from=build /go/bin/sei-sidecar /usr/bin/sei-sidecar # restartPolicy Always is a container that loops forever while reporting # success. Harder to notice than a crash, and the seid container beside it would # sit blocked on /v0/healthz behind a StartupProbe with FailureThreshold 86400. + +# Compatibility shim, deliberately temporary. A pod spec's `command` overrides +# ENTRYPOINT outright, so this second name is what lets ONE image satisfy both +# controller versions: +# +# controller at 92850d5 and earlier renders Command ["seictl","serve"], +# which overrides the ENTRYPOINT below and resolves through this name. +# +# controller at 186d65a and later renders no Command, so the ENTRYPOINT runs. +# +# Without it the two working combinations are diagonal — old controller needs +# `seictl`, new controller needs the ENTRYPOINT — so there is no ordering that +# passes through a working state, and image and controller must flip together in +# every cell at once. Worse, the rollback wedges: every mode's plan runs +# config-patch (a polled sidecar task) before replace-pod, so reverting the +# controller submits to a sidecar that is already dead, the submit fails, +# replace-pod is never reached, and the broken pod is never replaced. With this +# name present there is never a dead sidecar to wedge on. +# +# A second COPY rather than a symlink because the final stage is distroless and +# has no shell to run `ln`. The cost is one duplicated static binary in the +# image layer, taken deliberately over being clever across build stages. +# +# REMOVAL CONDITION (unchanged from when this was first written): remove only +# after the controller stops rendering Command — it already has, at 186d65a — +# AND every cell has rolled past that controller. As of this commit no cell has +# adopted this image at all, so the second half is not met anywhere. Deleting it +# before then is what turns three reversible steps into one simultaneous flip. +COPY --from=build /go/bin/sei-sidecar /usr/bin/seictl + ENTRYPOINT ["/usr/bin/sei-sidecar", "serve"] From 80a678fed167c74d775d79740b2bdd0577d1d213 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 20 Aug 2026 11:53:25 -0700 Subject: [PATCH 2/2] docs+test: name the ordered deploy and pin the no-Command contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the shim, three parts. Dockerfile comment. Two claims in it were wrong and one was missing. - The symlink alternative is not impossible: the BUILD stage has a shell, so `RUN ln -s` there plus a COPY of the link is the ordinary way to get a symlink into a distroless image, and it would save the duplicated binary. Untested here (no daemon), so the comment now gives the real reason — a transitional cost preferred over being clever across stages — instead of asserting a constraint that does not exist. - The wedge trigger was wrong. Reverting the controller changes no image, so sidecarImageDrifted is false and no update plan is built at all; the pod just sits there under OnDelete. The wedge belongs to any LATER image-drift plan once a sidecar is already dead: that one does drift, and dies at config-patch before reaching replace-pod. Same conclusion, correct mechanism. - The required order was never named. It is image first, controller second, per cell, and the safe rollback is "revert the controller, leave the image". Removal condition now names checkable signals — no rendered pod spec naming seictl at either render site, and no cell's config still on ghcr — rather than a controller version, because OnDelete lets a pod outlive the bump. The block also moved above the ENTRYPOINT comment, which it had pushed 29 lines away from the ENTRYPOINT it documents. CLAUDE.md. It still told the reader this was a coordinated deploy where image, config and controller ship together per cell — the thing the shim exists to stop being true. Rewritten as the three ordered steps, with the rollback direction and the manual-delete recovery, because nobody reads a Dockerfile before planning a deploy. Tests. Nothing pinned the invariant the whole cutover rests on: neither render site asserted the sidecar container renders no Command and no Args. Re-adding either breaks every pod in every cell quietly — exits 0, restarts, seid blocked behind a ~5-day StartupProbe. Both assertions mutation-checked: adding Command back fails each one at the expected line, and removing it passes again. Verified: gofmt and go vet clean; both new tests pass and fail under mutation. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 8 ++- internal/noderesource/noderesource_test.go | 17 +++++ internal/task/bootstrap_resources_test.go | 33 ++++++++++ sidecar/Dockerfile | 77 ++++++++++++++-------- 4 files changed, 105 insertions(+), 30 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 15e1c682..98c49085 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,7 +28,13 @@ Two checks keep the controller tidyable, both in `make ci`. The `depguard` rule `sidecar/main.go` → `sei-sidecar`, published to ECR as `sei/sei-sidecar`. The controller renders **no `Command`** for the sidecar container (`internal/noderesource/`, `internal/task/bootstrap_resources.go`), so the image's ENTRYPOINT — `sei-sidecar serve` — is what runs. The image owns its entrypoint; renaming the binary is an image-only change. -**Changing the sidecar image is a coordinated deploy.** The image, `images.sidecar` in the platform app-config, and the controller ship together per cell. `images.sidecar` is read once at startup, so a config edit alone does nothing until the controller restarts — and the two halves failing apart is silent, not loud. A controller that renders no `Command` against an image whose entrypoint lacks the subcommand gets a container that prints help and exits 0, restarting forever under `restartPolicy: Always`, while seid blocks on a shell loop polling `/v0/healthz` behind a StartupProbe with `FailureThreshold: 86400` at 5s — about five days before Kubernetes calls it failed. +**Changing the sidecar image is an ordered deploy, not a simultaneous one.** The image carries a `seictl` compatibility name alongside `sei-sidecar` (`sidecar/Dockerfile`), so it runs under both the controller that renders `Command: ["seictl","serve"]` and the one that renders none. That makes the image version-agnostic; it does **not** make the controller version-agnostic. Per cell, in this order: + +1. point `images.sidecar` at the ECR image and restart the controller — the restart is what reads it, and it rolls the cell through `replace-pod` on the *old* controller, via the `seictl` name; +2. bump the controller — changes no image, so it drifts nothing and rolls nothing; +3. delete the shim, once no cell is left behind. + +**The safe rollback is "revert the controller, leave the image" — never the reverse.** The new controller against an *old* image is the one pair the shim cannot save: a bare `seictl` prints help and exits 0, restarting forever under `restartPolicy: Always` while seid blocks on a shell loop polling `/v0/healthz` behind a StartupProbe with `FailureThreshold: 86400` at 5s — about five days before Kubernetes calls it failed. Recovery from a dead sidecar is a manual `kubectl delete pod` per node: StatefulSets are `OnDelete`, so nothing re-rolls on its own, and any later image-drift plan wedges at `config-patch`, which every mode orders before `replace-pod` and which submits to the dead sidecar. The rollout is controller-driven, not Kubernetes-driven: StatefulSets use `UpdateStrategy: OnDelete`, so a template change never touches a live pod. Sidecar-image drift builds a NodeUpdate plan whose `replace-pod` task deletes pods at the old revision, for **every** node whose `status.currentSidecarImage` is set. Expect the whole cell to roll. diff --git a/internal/noderesource/noderesource_test.go b/internal/noderesource/noderesource_test.go index 8e6be5e7..3a8e8a30 100644 --- a/internal/noderesource/noderesource_test.go +++ b/internal/noderesource/noderesource_test.go @@ -382,6 +382,23 @@ func TestSidecarContainer_DefaultImage(t *testing.T) { g.Expect(sc.Image).To(Equal(platformtest.Config().SidecarImage)) } +// The sidecar container must render NO Command and NO Args: the image's +// ENTRYPOINT is the command. Re-adding either silently breaks every pod in every +// cell — the container exits 0 and restarts while seid blocks on /v0/healthz +// behind a StartupProbe tolerating roughly five days, so nothing crashes and +// nothing alerts. Nothing else in the suite pins this, and the `seictl` +// compatibility name in sidecar/Dockerfile cannot be removed safely until it is. +func TestSidecarContainer_RendersNoCommandOrArgs(t *testing.T) { + g := NewWithT(t) + node := newSnapshotNode("sc-0", "default") + + sts := mustGenerateStatefulSet(t, node, platformtest.Config()) + sc := findInitContainer(sts.Spec.Template.Spec.InitContainers, "sei-sidecar") + + g.Expect(sc.Command).To(BeEmpty()) + g.Expect(sc.Args).To(BeEmpty()) +} + func TestSidecarContainer_CustomImage(t *testing.T) { g := NewWithT(t) node := newSnapshotNode("sc-0", "default") diff --git a/internal/task/bootstrap_resources_test.go b/internal/task/bootstrap_resources_test.go index 58832e02..db8a37c2 100644 --- a/internal/task/bootstrap_resources_test.go +++ b/internal/task/bootstrap_resources_test.go @@ -248,6 +248,39 @@ func bootstrapEnv(c *corev1.Container, name string) string { // (platform.HomeDir), never the data dir itself. A regression here — HOME left // equal to the data dir, or a lingering `--home "$HOME"` — reintroduces the // nesting bug and, mid-migration, risks a validator-data wipe. +// Second render site for the sidecar container, and it must agree with the +// StatefulSet one: no Command, no Args, so the image's ENTRYPOINT is the +// command. See TestSidecarContainer_RendersNoCommandOrArgs in +// internal/noderesource — the bootstrap Job is the site a steady-state cell +// roll never exercises, so only a new snapshot-bootstrapped node would find a +// regression here. +func TestBootstrapJob_SidecarRendersNoCommandOrArgs(t *testing.T) { + node := &seiv1alpha1.SeiNode{ + ObjectMeta: metav1.ObjectMeta{Name: "v-0", Namespace: testReplaceNs}, + Spec: seiv1alpha1.SeiNodeSpec{ + ChainID: "sei-test", + Image: "ghcr.io/sei-protocol/seid:latest", + }, + } + snap := &seiv1alpha1.SnapshotSource{S3: &seiv1alpha1.S3SnapshotSource{TargetHeight: 12345}} + + job, err := GenerateBootstrapJob(node, snap, platformtest.Config()) + if err != nil { + t.Fatalf("GenerateBootstrapJob error: %v", err) + } + + sc := findBootstrapContainer(job.Spec.Template.Spec, bootstrapTestSidecarContainer) + if sc == nil { + t.Fatalf("%s container not found", bootstrapTestSidecarContainer) + } + if len(sc.Command) != 0 { + t.Errorf("sidecar Command = %v, want empty (the image ENTRYPOINT is the command)", sc.Command) + } + if len(sc.Args) != 0 { + t.Errorf("sidecar Args = %v, want empty", sc.Args) + } +} + func TestBootstrapJob_HomeAndDataDir(t *testing.T) { node := &seiv1alpha1.SeiNode{ ObjectMeta: metav1.ObjectMeta{Name: "v-0", Namespace: testReplaceNs}, diff --git a/sidecar/Dockerfile b/sidecar/Dockerfile index d64a3f98..42af24d9 100644 --- a/sidecar/Dockerfile +++ b/sidecar/Dockerfile @@ -36,6 +36,54 @@ RUN cd sidecar && CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} \ FROM gcr.io/distroless/static-debian12 COPY --from=build /go/bin/sei-sidecar /usr/bin/sei-sidecar +# Compatibility shim, deliberately temporary. A pod spec's `command` REPLACES +# the image ENTRYPOINT (and `args` replaces CMD); neither render site sets Args, +# so argv is exactly what the controller renders. This second name is therefore +# what lets ONE image satisfy both controller versions: +# +# controller <= 92850d5 renders Command ["seictl","serve"], which replaces the +# ENTRYPOINT below and resolves through this name via PATH (/usr/bin is on the +# distroless PATH). +# +# controller >= 186d65a renders no Command, so the ENTRYPOINT below runs. +# +# The asymmetry matters: this makes the IMAGE valid under both controllers. It +# does not make the new controller valid with the OLD image, and cannot — that +# pair runs a bare `seictl`, which prints help and exits 0, looping under +# restartPolicy Always while seid sits blocked on /v0/healthz. So the cutover has +# a required order, per cell: adopt this image FIRST, bump the controller SECOND. +# The safe rollback is "revert the controller, leave the image", never the +# reverse. +# +# Because images.sidecar is read once at controller startup, adopting the image +# means restarting the old controller, and that restart trips sidecarImageDrifted +# and rolls the cell through replace-pod — on the old controller, through this +# name. The later controller bump changes no image, so it drifts nothing and +# rolls nothing. StatefulSets are OnDelete, so live pods keep the old template's +# Command while any recreated pod carries none; this name is what makes both +# revisions runnable in one cell at one time. +# +# Why it must not be removed early: once a node's sidecar is dead, any LATER +# image-drift plan wedges at config-patch, which every mode orders before +# replace-pod and which submits to that dead sidecar. The pod is then never +# replaced and recovery is a manual delete. Keeping this name means the revert +# leaves a live sidecar behind, so that state is not reachable by rolling back. +# +# A second COPY rather than a symlink: the build stage could `ln -s` and this +# stage could COPY the link, saving a duplicated binary, but that is untested +# here and the duplicate is a transitional cost we would rather pay than be +# clever across stages for a shim scheduled for deletion. +# +# REMOVAL CONDITION — check the pods, not the controller version, because +# OnDelete lets a pod created by the old controller outlive the controller bump: +# 1. no rendered pod spec names `seictl` — StatefulSet template AND bootstrap +# Job, both render sites; and +# 2. no cell's controller-config still points images.sidecar at ghcr. +# Per-node confirmation that a roll landed is .status.currentSidecarImage. +# Deleting this makes a controller rollback below 186d65a a one-way door unless +# the image is rolled back with it. +COPY --from=build /go/bin/sei-sidecar /usr/bin/seictl + # The subcommand belongs here, not in the pod spec. The controller renders no # `Command` for the sidecar container, so this ENTRYPOINT is what runs, and the # image owns its own binary name — renaming it later is an image-only change. @@ -46,33 +94,4 @@ COPY --from=build /go/bin/sei-sidecar /usr/bin/sei-sidecar # success. Harder to notice than a crash, and the seid container beside it would # sit blocked on /v0/healthz behind a StartupProbe with FailureThreshold 86400. -# Compatibility shim, deliberately temporary. A pod spec's `command` overrides -# ENTRYPOINT outright, so this second name is what lets ONE image satisfy both -# controller versions: -# -# controller at 92850d5 and earlier renders Command ["seictl","serve"], -# which overrides the ENTRYPOINT below and resolves through this name. -# -# controller at 186d65a and later renders no Command, so the ENTRYPOINT runs. -# -# Without it the two working combinations are diagonal — old controller needs -# `seictl`, new controller needs the ENTRYPOINT — so there is no ordering that -# passes through a working state, and image and controller must flip together in -# every cell at once. Worse, the rollback wedges: every mode's plan runs -# config-patch (a polled sidecar task) before replace-pod, so reverting the -# controller submits to a sidecar that is already dead, the submit fails, -# replace-pod is never reached, and the broken pod is never replaced. With this -# name present there is never a dead sidecar to wedge on. -# -# A second COPY rather than a symlink because the final stage is distroless and -# has no shell to run `ln`. The cost is one duplicated static binary in the -# image layer, taken deliberately over being clever across build stages. -# -# REMOVAL CONDITION (unchanged from when this was first written): remove only -# after the controller stops rendering Command — it already has, at 186d65a — -# AND every cell has rolled past that controller. As of this commit no cell has -# adopted this image at all, so the second half is not met anywhere. Deleting it -# before then is what turns three reversible steps into one simultaneous flip. -COPY --from=build /go/bin/sei-sidecar /usr/bin/seictl - ENTRYPOINT ["/usr/bin/sei-sidecar", "serve"]