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 7657796d..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. @@ -45,4 +93,5 @@ 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. + ENTRYPOINT ["/usr/bin/sei-sidecar", "serve"]