diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 73c777d..182b107 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -66,6 +66,8 @@ jobs: context: . file: backend/Dockerfile push: true + build-args: | + VERSION=${{ steps.meta.outputs.version }} tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} diff --git a/.goreleaser.yml b/.goreleaser.yml index a61b08f..fe18031 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -38,6 +38,7 @@ builds: - arm64 ldflags: - -s -w + - -X main.buildVersion={{.Version}} archives: - id: skillsctl diff --git a/backend/Dockerfile b/backend/Dockerfile index 617a9fa..1a4a83d 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -8,7 +8,12 @@ RUN go mod download # Build COPY . . -RUN CGO_ENABLED=0 GOOS=linux go build -ldflags="-s -w" -o /skillsctl-server ./backend/cmd/server +# VERSION stamps main.buildVersion so the server only seeds skills under the +# release it was actually built from. +ARG VERSION=dev +RUN CGO_ENABLED=0 GOOS=linux go build \ + -ldflags="-s -w -X main.buildVersion=${VERSION}" \ + -o /skillsctl-server ./backend/cmd/server # Final image FROM scratch diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index 46dfef9..827ae9e 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -2,6 +2,7 @@ package main import ( "context" + "fmt" "log" "net/http" "os" @@ -20,6 +21,11 @@ import ( "github.com/nebari-dev/skillsctl/skills" ) +// buildVersion is the release this binary was built from, injected at build +// time with -ldflags "-X main.buildVersion=". Local builds leave it +// at "dev". +var buildVersion = "dev" + func main() { port := envOr("PORT", "8080") dbPath := envOr("DB_PATH", "skillsctl.db") @@ -63,12 +69,14 @@ func main() { repo := sqlitestore.New(db) // Seed default skills (idempotent - skips if version already exists) - appVersion := envOr("APP_VERSION", "0.0.0") skillsctlUsage, err := skills.FS.ReadFile("skillsctl-usage.md") if err != nil { log.Fatalf("read embedded skill: %v", err) } - if err := seed.Run(context.Background(), repo, appVersion, seed.DefaultSkills(skillsctlUsage)); err != nil { + seedVersion, err := resolveSeedVersion(buildVersion, os.Getenv("APP_VERSION")) + if err != nil { + log.Printf("seed: refusing to seed (%v)", err) + } else if err := seed.Run(context.Background(), repo, seedVersion, seed.DefaultSkills(skillsctlUsage)); err != nil { log.Fatalf("seed skills: %v", err) } @@ -120,6 +128,31 @@ func envOr(key, fallback string) string { return fallback } +// resolveSeedVersion decides which version the embedded skills are published +// under. Seeded content comes from this binary, so the version must identify +// this binary: publishing under a version built from different content makes +// that version permanently wrong, because published versions are immutable. +// +// A stamped binary therefore refuses to seed when APP_VERSION disagrees with +// its own build version. That disagreement happens whenever a pod from an +// older image reads a configmap that a newer release already updated, which is +// exactly the window a rolling update opens. +// +// Unstamped builds (local development, "dev") have no version to compare +// against and fall back to APP_VERSION. +func resolveSeedVersion(build, appVersion string) (string, error) { + if build == "" || build == "dev" { + if appVersion == "" { + return "0.0.0", nil + } + return appVersion, nil + } + if appVersion != "" && appVersion != build { + return "", fmt.Errorf("APP_VERSION %q does not match build version %q; this binary must not publish skills as %s", appVersion, build, appVersion) + } + return build, nil +} + func isDevMode() bool { v := strings.ToLower(os.Getenv("DEV_MODE")) return v == "1" || v == "true" || v == "yes" diff --git a/backend/cmd/server/main_test.go b/backend/cmd/server/main_test.go index a03c55d..2c26efc 100644 --- a/backend/cmd/server/main_test.go +++ b/backend/cmd/server/main_test.go @@ -30,3 +30,43 @@ func TestIsDevMode(t *testing.T) { }) } } + +func TestResolveSeedVersion(t *testing.T) { + tests := []struct { + name string + build string + appVersion string + want string + wantErr bool + }{ + {name: "stamped build, matching APP_VERSION", build: "0.2.2", appVersion: "0.2.2", want: "0.2.2"}, + {name: "stamped build, no APP_VERSION", build: "0.2.2", appVersion: "", want: "0.2.2"}, + {name: "stamped build, APP_VERSION ahead", build: "0.2.0", appVersion: "0.2.1", wantErr: true}, + {name: "stamped build, APP_VERSION behind", build: "0.2.2", appVersion: "0.2.1", wantErr: true}, + {name: "dev build falls back to APP_VERSION", build: "dev", appVersion: "9.9.9", want: "9.9.9"}, + {name: "dev build with no APP_VERSION", build: "dev", appVersion: "", want: "0.0.0"}, + {name: "empty build treated as unstamped", build: "", appVersion: "1.2.3", want: "1.2.3"}, + {name: "empty build and no APP_VERSION", build: "", appVersion: "", want: "0.0.0"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolveSeedVersion(tt.build, tt.appVersion) + if tt.wantErr { + if err == nil { + t.Fatalf("resolveSeedVersion(%q, %q) = %q, want error", tt.build, tt.appVersion, got) + } + if got != "" { + t.Errorf("resolveSeedVersion(%q, %q) returned version %q alongside error, want empty", tt.build, tt.appVersion, got) + } + return + } + if err != nil { + t.Fatalf("resolveSeedVersion(%q, %q) unexpected error: %v", tt.build, tt.appVersion, err) + } + if got != tt.want { + t.Errorf("resolveSeedVersion(%q, %q) = %q, want %q", tt.build, tt.appVersion, got, tt.want) + } + }) + } +} diff --git a/backend/internal/seed/seed.go b/backend/internal/seed/seed.go index ab3e048..1603c2f 100644 --- a/backend/internal/seed/seed.go +++ b/backend/internal/seed/seed.go @@ -44,6 +44,7 @@ func Run(ctx context.Context, repo store.Repository, version string, skills []Sk err := repo.CreateSkillVersion(ctx, skill, ver, s.Content) if err != nil { if errors.Is(err, store.ErrAlreadyExists) { + log.Printf("seed: %s@%s already present, skipping", s.Name, version) continue } if errors.Is(err, store.ErrPermissionDenied) { diff --git a/chart/templates/deployment.yaml b/chart/templates/deployment.yaml index b052fac..e61c684 100644 --- a/chart/templates/deployment.yaml +++ b/chart/templates/deployment.yaml @@ -6,6 +6,16 @@ metadata: {{- include "skillsctl.labels" . | nindent 4 }} spec: replicas: {{ .Values.replicaCount }} + {{- if and .Values.persistence.enabled (ne .Values.persistence.accessMode "ReadWriteMany") }} + # Any access mode short of ReadWriteMany binds the volume to one node, so the + # default RollingUpdate deadlocks: it holds the old pod until the new one is + # Ready, and the new pod cannot attach until the old one releases the claim. + # Recreate also avoids running two image generations at once, which would let + # an older pod seed skills under the newer release's version. Inverted on + # purpose, so an unset or unexpected accessMode still gets the safe strategy. + strategy: + type: Recreate + {{- end }} selector: matchLabels: {{- include "skillsctl.selectorLabels" . | nindent 6 }} diff --git a/chart/values.yaml b/chart/values.yaml index d26c35e..8d1b3f1 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -27,7 +27,10 @@ service: persistence: enabled: true storageClassName: "" # uses cluster default if empty - accessMode: ReadWriteOnce # set to ReadWriteMany for HA + # ReadWriteOnce forces the Deployment to strategy: Recreate, since an RWO + # volume cannot attach to the new pod while the old one still holds it. + # Upgrades therefore have a brief gap. Set to ReadWriteMany for HA. + accessMode: ReadWriteOnce size: 1Gi mountPath: /data