diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 447735dee..e28e6fac7 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -187,6 +187,54 @@ jobs: - name: Build, vet, and test the Go modules under tools/ run: tools/ci/check-go-tools + stack-upgrade-policy: + name: stack upgrade policy + # Pull requests only. The check weighs the migrations a candidate + # introduces against the version bump its commits imply, which needs two + # sides: a baseline to measure from, and a proposal to measure. A pull + # request has both -- origin/main is the baseline and the branch is the + # proposal. A push to main has already merged, so origin/main..HEAD is + # empty and there is no proposal left to judge. + # + # Concretely: a branch that adds + # migrations/cassandra/keyspaces/nvct_api/05_drop_health_info.up.sql and + # carries only `fix:` commits fails this job, because dropping a column + # cannot be skipped past and so needs a major, while `fix` asks for a + # patch. Once those same commits are on main, running here would compare + # main against itself and find nothing to report. + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + stack: + - nvcf-self-managed-stack + - nvcf-compute-plane-stack + - nvcf-observability-stack + steps: + - uses: actions/checkout@v4 + with: + persist-credentials: false + # The check diffs against the stack's last published release tag, so + # it needs the tags and the history behind them. + fetch-depth: 0 + fetch-tags: true + + - uses: actions/setup-go@v5 + with: + go-version-file: tools/go-toolchain/go.mod + + # Interpolations go through env, never into the script body: a ref name + # expanded inline is a shell injection. + - name: Check migrations against the proposed version bump + env: + BASE_REF: ${{ github.base_ref }} + STACK: ${{ matrix.stack }} + run: | + bump="$(tools/ci/release-bump-type for-branch "origin/${BASE_REF}")" + echo "proposed bump: ${bump}" + tools/ci/check-stack-upgrade-policy --stack "${STACK}" --bump "${bump}" + github-release-helper: name: GitHub release helper runs-on: ubuntu-latest diff --git a/deploy/stacks/self-managed/Makefile b/deploy/stacks/self-managed/Makefile index ed014a117..c3ae1f41a 100644 --- a/deploy/stacks/self-managed/Makefile +++ b/deploy/stacks/self-managed/Makefile @@ -15,6 +15,7 @@ test: @tests/grpc-proxy-nats-endpoint.sh @tests/llm-pki-openbao-migration.sh @tests/api-keys-startup-probe.sh + @tests/upgrade-receipt-wiring.sh @tests/cassandra-openbao-credential-wiring.sh @tests/llm-pki-release.sh @tests/check-llm-pki-issuer.sh diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/Chart.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/Chart.yaml new file mode 100644 index 000000000..9abbe3869 --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/Chart.yaml @@ -0,0 +1,8 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: v2 +name: nvcf-upgrade-receipt +description: Records the installed NVCF stack version in-cluster so an upgrade can tell where it is starting from. +type: application +version: 0.1.0 +appVersion: "0.1.0" diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/job.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/job.yaml new file mode 100644 index 000000000..3b2e6848b --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/job.yaml @@ -0,0 +1,56 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +{{- $version := required "stackVersion is required: a receipt that names the wrong version is worse than no receipt" .Values.stackVersion }} +apiVersion: batch/v1 +kind: Job +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + # post-* rather than pre-*: the receipt states what the cluster is running, + # so it must not be written until the release it describes has been applied. + # + # Both install and upgrade, because the first cluster to receive this chart + # has no prior release of it, and Helm runs post-install there rather than + # post-upgrade. Omitting post-install would leave exactly the clusters this + # exists for without a receipt. + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "0" + "helm.sh/hook-delete-policy": before-hook-creation +spec: + backoffLimit: 3 + ttlSecondsAfterFinished: 600 + template: + metadata: + name: {{ .Release.Name }} + spec: + restartPolicy: Never + serviceAccountName: {{ .Release.Name }} + {{- with .Values.imagePullSecrets }} + imagePullSecrets: +{{ toYaml . | indent 8 }} + {{- end }} + containers: + - name: receipt + image: "{{ with .Values.image.registry }}{{ . }}/{{ end }}{{ .Values.image.repository }}:{{ .Values.image.tag }}" + imagePullPolicy: {{ .Values.image.pullPolicy }} + env: + - name: RECEIPT_CONFIGMAP + value: {{ .Values.configMapName | quote }} + - name: INSTALLED_STACK_VERSION + value: {{ $version | quote }} + command: + - /bin/sh + - -c + - | + set -eu + # Rendered through apply rather than create so that the first + # install and every later upgrade take the same path. A receipt + # that only appears on a fresh install would be absent from + # exactly the clusters that are upgrading. + kubectl create configmap "${RECEIPT_CONFIGMAP}" \ + --from-literal=installed_stack_version="${INSTALLED_STACK_VERSION}" \ + --from-literal=recorded_at="$(date -u +%Y-%m-%dT%H:%M:%SZ)" \ + --dry-run=client -o yaml \ + | kubectl apply -f - + echo "recorded installed_stack_version=${INSTALLED_STACK_VERSION} in ${RECEIPT_CONFIGMAP}" diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml new file mode 100644 index 000000000..469367eea --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/role.yaml @@ -0,0 +1,26 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "-5" + "helm.sh/hook-delete-policy": before-hook-creation +rules: + # Scoped to the receipt itself. get and patch cover the upgrade case where a + # receipt already exists, and naming the resource keeps this identity from + # reaching any other ConfigMap in the namespace. + - apiGroups: [""] + resources: ["configmaps"] + resourceNames: [{{ .Values.configMapName | quote }}] + verbs: ["get", "patch"] + # create cannot be scoped: RBAC matches resourceNames against an object that + # does not exist yet, so a create rule naming one is never satisfied. It is + # kept in its own rule so the unscoped verb is visible rather than buried + # alongside the scoped ones, and it is only reachable on a first install. + - apiGroups: [""] + resources: ["configmaps"] + verbs: ["create"] diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/rolebinding.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/rolebinding.yaml new file mode 100644 index 000000000..4139a6c87 --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/rolebinding.yaml @@ -0,0 +1,19 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "-5" + "helm.sh/hook-delete-policy": before-hook-creation +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: {{ .Release.Name }} +subjects: + - kind: ServiceAccount + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/serviceaccount.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/serviceaccount.yaml new file mode 100644 index 000000000..dcdf2d5ef --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/templates/serviceaccount.yaml @@ -0,0 +1,17 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ .Release.Name }} + namespace: {{ .Release.Namespace }} + annotations: + # Must exist before the Job that uses it, and survive long enough to be + # bound. A lower weight than the Job is what orders them. + "helm.sh/hook": post-install,post-upgrade + "helm.sh/hook-weight": "-5" + "helm.sh/hook-delete-policy": before-hook-creation +{{- with .Values.imagePullSecrets }} +imagePullSecrets: +{{ toYaml . | indent 2 }} +{{- end }} diff --git a/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/values.yaml b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/values.yaml new file mode 100644 index 000000000..87537031e --- /dev/null +++ b/deploy/stacks/self-managed/charts/nvcf-upgrade-receipt/values.yaml @@ -0,0 +1,19 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +# The stack version this bundle installs. The Helmfile supplies it from the +# bundle's own VERSION file; there is no sensible default, so rendering fails +# rather than recording a version the cluster is not running. +stackVersion: "" + +image: + registry: "" + repository: alpine-k8s + tag: "1.33.1" + pullPolicy: IfNotPresent + +imagePullSecrets: [] + +# Name of the ConfigMap holding the receipt. An upgrade reads this to decide +# whether the jump it has been asked to make is one it can make safely. +configMapName: nvcf-upgrade-receipt diff --git a/deploy/stacks/self-managed/helmfile.d/04-upgrade-receipt.yaml.gotmpl b/deploy/stacks/self-managed/helmfile.d/04-upgrade-receipt.yaml.gotmpl new file mode 100644 index 000000000..a42e8f9fd --- /dev/null +++ b/deploy/stacks/self-managed/helmfile.d/04-upgrade-receipt.yaml.gotmpl @@ -0,0 +1,30 @@ +environments: + default: + values: + - ../environments/base.yaml + - ../environments/{{ requiredEnv "HELMFILE_ENV" }}.yaml + +--- + +{{- /* + A stage of its own, and the last one, so the receipt is written only after + every other release has been applied. Ordering is a stage boundary rather + than a needs: edge on purpose: under the helmfile version this stack pins, + needs: places a release in a later DAG layer where it waits on every peer in + the previous one, and a single slow or failed peer silently skips it. See the + admin-issuer-proxy comment in 02-core.yaml.gotmpl. +*/}} + +releases: + - name: upgrade-receipt + chart: ../charts/nvcf-upgrade-receipt + namespace: nvcf + values: + - stackVersion: {{ readFile "../VERSION" | trim | quote }} + image: + registry: {{ .Values.global.image.registry | quote }} + repository: {{ .Values.global.image.repository }}/alpine-k8s + {{- with .Values.global.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 10 }} + {{- end }} diff --git a/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh b/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh new file mode 100755 index 000000000..f59ec3bc8 --- /dev/null +++ b/deploy/stacks/self-managed/tests/upgrade-receipt-wiring.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# Test that the stack records the version it installed. +# +# An upgrade has to know where it is starting from, and nothing else in a +# cluster carries that: Helm tracks chart versions per release, and helmfile has +# no concept of the bundle's own version. Without this receipt every cluster +# looks identical to every other one at upgrade time. +# +# The assertions that matter are the hook kinds and the recorded version. A +# pre-* hook would claim a version before it was applied, and a post-upgrade +# hook alone would skip the first install of this chart, which is precisely the +# set of clusters that need a receipt written. +set -euo pipefail + +stack_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +expected_version="$(tr -d '[:space:]' < "$stack_dir/VERSION")" + +rendered="$(cd "$stack_dir" && HELMFILE_ENV=base helmfile \ + --file helmfile.d/04-upgrade-receipt.yaml.gotmpl template)" + +fail() { echo "FAIL: $1" >&2; exit 1; } + +grep -q 'kind: Job' <<<"$rendered" || fail "no Job rendered" +grep -q '"helm.sh/hook": post-install,post-upgrade' <<<"$rendered" \ + || fail "receipt must run on both install and upgrade, after the release it describes" +grep -q "value: \"${expected_version}\"" <<<"$rendered" \ + || fail "recorded version does not match VERSION (${expected_version})" +# The ConfigMap is created by the Job at run time, not rendered, so its name +# reaches the cluster as the env var the script reads. +grep -q 'value: "nvcf-upgrade-receipt"' <<<"$rendered" \ + || fail "receipt ConfigMap name is not the one an upgrade will read" + +for kind in ServiceAccount Role RoleBinding; do + grep -q "kind: ${kind}" <<<"$rendered" || fail "missing ${kind}; the Job cannot write the ConfigMap without it" +done +# get and patch are scoped to the receipt by name so this identity cannot +# reach any other ConfigMap. create cannot be scoped -- RBAC matches +# resourceNames against an object that does not exist yet. +grep -qE '^\s+resourceNames: \["nvcf-upgrade-receipt"\]' <<<"$rendered" \ + || fail "get/patch are not scoped to the receipt ConfigMap by name" +grep -qE '^\s+verbs: \["get", "patch"\]' <<<"$rendered" \ + || fail "scoped rule should carry only get and patch" +grep -qE '^\s+verbs: \["create"\]' <<<"$rendered" \ + || fail "create must remain, in its own rule, for the first install" + +# The stage number is the ordering guarantee. needs: is deliberately not used +# here; see the comment in the stage file. +last_stage="$(ls "$stack_dir"/helmfile.d/*.gotmpl | sort | tail -1)" +[[ "$(basename "$last_stage")" == "04-upgrade-receipt.yaml.gotmpl" ]] \ + || fail "receipt is not the last stage; it would record a version before the stack finished applying" + +echo "PASS: upgrade-receipt-wiring" diff --git a/tools/ci/check-stack-upgrade-policy b/tools/ci/check-stack-upgrade-policy new file mode 100755 index 000000000..4c50b0af9 --- /dev/null +++ b/tools/ci/check-stack-upgrade-policy @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +# SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Stable CI entrypoint for the Go tool in tools/stack-upgrade-policy. +# +# The wrapper exists for the same two reasons tools/ci/chart-service-edge does. +# +# The repository root. `go run -C ` leaves the process running with that +# directory as its working directory, so the tool cannot find the release +# metadata or the git history on its own. Resolving the root from this script's +# own location means callers do not have to pass it. +# +# The exit code. `go run` does NOT propagate the program's status: it prints +# "exit status N" and exits 1. This tool distinguishes 1 (policy violation or +# error) from 2 (bad invocation), so collapsing them would be a trap. +# +# Run the tests with: go test -C tools/stack-upgrade-policy ./... +set -euo pipefail + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +bin_dir="$(mktemp -d)" +trap 'rm -rf "${bin_dir}"' EXIT + +go build -C "${repo_root}/tools/stack-upgrade-policy" -o "${bin_dir}/stack-upgrade-policy" . + +# Not exec, so the trap above still runs, and not under errexit, so the exit +# code reaches the caller rather than aborting the shell first. +set +e +"${bin_dir}/stack-upgrade-policy" --root "${repo_root}" "$@" +status=$? +set -e +exit "${status}" diff --git a/tools/ci/github-release-subprojects.json b/tools/ci/github-release-subprojects.json index 19a647bf2..1e88d7fe9 100644 --- a/tools/ci/github-release-subprojects.json +++ b/tools/ci/github-release-subprojects.json @@ -42,6 +42,7 @@ { "id": "nvcf-self-managed-stack", "path": "deploy/stacks/self-managed", + "migration_paths": ["migrations/cassandra", "migrations/openbao"], "service_name": "nvcf-self-managed-stack", "tag_format": "deploy/stacks/self-managed/v${version}", "version_file": "VERSION", diff --git a/tools/stack-upgrade-policy/.gitignore b/tools/stack-upgrade-policy/.gitignore new file mode 100644 index 000000000..920c6a71a --- /dev/null +++ b/tools/stack-upgrade-policy/.gitignore @@ -0,0 +1,2 @@ +# go build ./... drops the binary here; it must never be committed. +/stack-upgrade-policy diff --git a/tools/stack-upgrade-policy/classify.go b/tools/stack-upgrade-policy/classify.go new file mode 100644 index 000000000..a4029536d --- /dev/null +++ b/tools/stack-upgrade-policy/classify.go @@ -0,0 +1,96 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "regexp" + "strings" +) + +// Class is what a migration does to data that already exists. +type Class int + +const ( + // Additive migrations only add schema. A cluster that skipped every stack + // version between two points still arrives at the right schema, because + // golang-migrate applies the ordered set regardless of how far behind the + // cluster was. + Additive Class = iota + // Destructive migrations remove a table, type, or column, or delete rows. + // Skipping versions is still safe for the schema itself, but the drop is + // unrecoverable without a restore, and it constrains deployment order: + // the migration must land before the services that read what it removes. + Destructive +) + +func (c Class) String() string { + if c == Destructive { + return "Destructive" + } + return "Additive" +} + +var destructive = regexp.MustCompile(`(?i)\b(DROP|TRUNCATE|DELETE)\b`) + +// Classify reports what a CQL migration does. Comments are stripped first: +// migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql documents a +// "high-churn write/delete workload" above a CREATE TABLE, and a classifier +// that reads raw text calls that destructive. +func Classify(sql string) Class { + if destructive.MatchString(stripComments(sql)) { + return Destructive + } + return Additive +} + +// stripComments blanks out everything CQL does not execute: line comments, +// block comments, and single-quoted strings. Keyword matching runs on what is +// left, so a DROP mentioned in a comment or a table's comment option is not +// mistaken for one the database will perform. +// +// Stripped regions become a space rather than nothing, so that removing a +// comment between two tokens cannot weld them into a third. +func stripComments(sql string) string { + var b strings.Builder + for i := 0; i < len(sql); { + switch { + case strings.HasPrefix(sql[i:], "--"): + end := strings.IndexByte(sql[i:], '\n') + if end < 0 { + i = len(sql) + } else { + i += end + } + b.WriteByte(' ') + case strings.HasPrefix(sql[i:], "/*"): + end := strings.Index(sql[i+2:], "*/") + if end < 0 { + i = len(sql) + } else { + i += 2 + end + 2 + } + b.WriteByte(' ') + case sql[i] == '\'': + i++ + for i < len(sql) { + if sql[i] != '\'' { + i++ + continue + } + // '' is an escaped quote inside the string, not its end. + if i+1 < len(sql) && sql[i+1] == '\'' { + i += 2 + continue + } + i++ + break + } + b.WriteByte(' ') + default: + b.WriteByte(sql[i]) + i++ + } + } + return b.String() +} diff --git a/tools/stack-upgrade-policy/classify_test.go b/tools/stack-upgrade-policy/classify_test.go new file mode 100644 index 000000000..429d99a2e --- /dev/null +++ b/tools/stack-upgrade-policy/classify_test.go @@ -0,0 +1,76 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "testing" + +func TestClassifyAddColumnIsAdditive(t *testing.T) { + sql := "ALTER TABLE nvcf_api.functions_v3 ADD llm_config frozen;" + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} + +func TestClassifyDropTableIsDestructive(t *testing.T) { + sql := "DROP TABLE IF EXISTS nvcf_autoscaler.recently_invoked_functions_history;" + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} + +func TestClassifyAlterTableDropIsDestructive(t *testing.T) { + sql := "ALTER TABLE IF EXISTS nvcf_api.functions_deployment_v2 DROP IF EXISTS gpu_specs;" + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} + +func TestClassifyDropTypeIsDestructive(t *testing.T) { + sql := "DROP TYPE IF EXISTS nvct_api.health_udt;" + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} + +// migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql carries the +// comment "Tuned for high-churn write/delete workload: UCS + short gc_grace." +// A classifier that greps the raw text calls that table creation destructive. +func TestClassifyIgnoresKeywordsInComments(t *testing.T) { + sql := `-- Tuned for high-churn write/delete workload: UCS + short gc_grace. +-- We may DROP this table later. +CREATE TABLE IF NOT EXISTS nvcf_api.functions_v3 (id uuid PRIMARY KEY);` + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} + +// CodeRabbit on #1983: stripComments only handled `--`, so a block comment +// containing DROP made an additive migration look destructive and would have +// failed a legitimate non-major release. +func TestClassifyIgnoresKeywordsInBlockComments(t *testing.T) { + sql := `/* DROP TABLE old_data; superseded, see NVCF-1234 */ +CREATE TABLE IF NOT EXISTS nvcf_api.new_data (id uuid PRIMARY KEY);` + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} + +// CQL carries table options as string literals, and a table whose comment +// mentions dropping rows is not a destructive migration. +func TestClassifyIgnoresKeywordsInStringLiterals(t *testing.T) { + sql := "CREATE TABLE nvcf_api.t (id uuid PRIMARY KEY) WITH comment = 'we never delete or drop here';" + if got := Classify(sql); got != Additive { + t.Fatalf("Classify() = %v, want Additive", got) + } +} + +// The doubled-quote escape must not leave the scanner stuck inside a string, +// or every statement after one would be skipped and a real DROP missed. +func TestClassifyHandlesEscapedQuotesAndStillSeesRealDrops(t *testing.T) { + sql := `CREATE TABLE nvcf_api.t (id uuid PRIMARY KEY) WITH comment = 'it''s fine'; +DROP TABLE IF EXISTS nvcf_api.old;` + if got := Classify(sql); got != Destructive { + t.Fatalf("Classify() = %v, want Destructive", got) + } +} diff --git a/tools/stack-upgrade-policy/config.go b/tools/stack-upgrade-policy/config.go new file mode 100644 index 000000000..84b7a1748 --- /dev/null +++ b/tools/stack-upgrade-policy/config.go @@ -0,0 +1,54 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" +) + +// MetadataPath is the release metadata every subproject is already registered +// in. The migration paths live here rather than in a new per-stack file so +// that a stack has one place describing how it releases, not two. +const MetadataPath = "tools/ci/github-release-subprojects.json" + +const stackPrefix = "deploy/stacks/" + +// Stack is one released stack bundle and the paths whose migrations ship with +// it. A stack that declares no migration paths is a stack that ships no +// schema, and the check is a no-op for it. +type Stack struct { + ID string `json:"id"` + Path string `json:"path"` + MigrationPaths []string `json:"migration_paths"` +} + +type metadata struct { + Services []Stack `json:"services"` +} + +// LoadStack finds a stack by its release-metadata id. +func LoadStack(root, id string) (Stack, error) { + raw, err := os.ReadFile(filepath.Join(root, MetadataPath)) + if err != nil { + return Stack{}, err + } + var meta metadata + if err := json.Unmarshal(raw, &meta); err != nil { + return Stack{}, fmt.Errorf("%s: %w", MetadataPath, err) + } + for _, s := range meta.Services { + if s.ID != id { + continue + } + if !strings.HasPrefix(s.Path, stackPrefix) { + return Stack{}, fmt.Errorf("%q is not a stack: its path %q is not under %s", id, s.Path, stackPrefix) + } + return s, nil + } + return Stack{}, fmt.Errorf("no subproject %q in %s", id, MetadataPath) +} diff --git a/tools/stack-upgrade-policy/config_test.go b/tools/stack-upgrade-policy/config_test.go new file mode 100644 index 000000000..ee9d95282 --- /dev/null +++ b/tools/stack-upgrade-policy/config_test.go @@ -0,0 +1,74 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +const metadataFixture = `{ + "version": 1, + "services": [ + {"id": "nvcf-cli", "path": "src/clis/nvcf-cli"}, + {"id": "nvcf-self-managed-stack", "path": "deploy/stacks/self-managed", + "migration_paths": ["migrations/cassandra", "migrations/openbao"]}, + {"id": "nvcf-observability-stack", "path": "deploy/stacks/observability"} + ] +}` + +func metadataDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + p := filepath.Join(dir, MetadataPath) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(metadataFixture), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +func TestLoadStackReadsPathAndMigrationPaths(t *testing.T) { + s, err := LoadStack(metadataDir(t), "nvcf-self-managed-stack") + if err != nil { + t.Fatal(err) + } + if s.Path != "deploy/stacks/self-managed" { + t.Errorf("Path = %q", s.Path) + } + if len(s.MigrationPaths) != 2 { + t.Errorf("MigrationPaths = %v, want two", s.MigrationPaths) + } +} + +// A stack that declares no migration paths is not an error: the compute-plane +// and observability stacks ship no schema, so the check is a no-op for them +// until they need it. +func TestLoadStackAllowsNoMigrationPaths(t *testing.T) { + s, err := LoadStack(metadataDir(t), "nvcf-observability-stack") + if err != nil { + t.Fatal(err) + } + if len(s.MigrationPaths) != 0 { + t.Errorf("MigrationPaths = %v, want none", s.MigrationPaths) + } +} + +func TestLoadStackRejectsUnknownID(t *testing.T) { + if _, err := LoadStack(metadataDir(t), "no-such-stack"); err == nil { + t.Fatal("err = nil, want an error naming the unknown stack") + } +} + +// Guards against pointing the check at a service that is not a stack, which +// would silently compare against a tag series that has nothing to do with a +// published bundle. +func TestLoadStackRejectsANonStackService(t *testing.T) { + if _, err := LoadStack(metadataDir(t), "nvcf-cli"); err == nil { + t.Fatal("err = nil, want an error for a non-stack service") + } +} diff --git a/tools/stack-upgrade-policy/decide.go b/tools/stack-upgrade-policy/decide.go new file mode 100644 index 000000000..c9e2233ae --- /dev/null +++ b/tools/stack-upgrade-policy/decide.go @@ -0,0 +1,112 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "strings" +) + +// Status is what happened to a migration file between two points in history. +type Status int + +const ( + Added Status = iota + Modified + Deleted +) + +func (s Status) String() string { + switch s { + case Added: + return "added" + case Modified: + return "modified" + default: + return "deleted" + } +} + +// Change is one migration file and what happened to it. +type Change struct { + Path string + Status Status + Class Class +} + +// Evidence is every migration change a release candidate introduces since the +// stack's last published release. +type Evidence struct { + Changes []Change +} + +// Bump is the semver step a release candidate is proposing. +type Bump int + +const ( + NoBump Bump = iota + Patch + Minor + Major +) + +func (b Bump) String() string { + switch b { + case Major: + return "major" + case Minor: + return "minor" + case Patch: + return "patch" + default: + return "none" + } +} + +// Decision is the outcome for one release candidate. +type Decision struct { + // Qualifying reports whether the candidate contains a change a customer + // cannot safely skip past. + Qualifying bool + OK bool + Reason string +} + +// Decide compares migration evidence against the proposed version bump. +// +// Additive migrations never qualify. golang-migrate applies the ordered set +// per keyspace, so a cluster many versions behind still arrives at the right +// schema; nothing about skipping versions breaks it. +// +// A destructive migration or a deleted migration file does qualify. A drop +// cannot be undone without a restore, and a deleted migration is how a +// compatibility bridge stops shipping — once it is gone, a cluster arriving +// later has no way to run it. Both need the major boundary that tells a +// customer not to skip this release. +func Decide(ev Evidence, bump Bump) Decision { + var qualifying []Change + for _, c := range ev.Changes { + if c.Status == Deleted || c.Class == Destructive { + qualifying = append(qualifying, c) + } + } + if len(qualifying) == 0 { + return Decision{OK: true} + } + if bump == Major { + return Decision{Qualifying: true, OK: true} + } + + var b strings.Builder + fmt.Fprintf(&b, "proposed bump is %s, but this candidate contains %d change(s) a customer cannot skip past:\n", bump, len(qualifying)) + for _, c := range qualifying { + if c.Status == Deleted { + fmt.Fprintf(&b, " %s (deleted: a cluster arriving later cannot run it)\n", c.Path) + continue + } + fmt.Fprintf(&b, " %s (%s, destructive)\n", c.Path, c.Status) + } + b.WriteString("\nEither cut this as a major release, or keep the removed migration in place until the next major boundary.") + return Decision{Qualifying: true, Reason: b.String()} +} diff --git a/tools/stack-upgrade-policy/decide_test.go b/tools/stack-upgrade-policy/decide_test.go new file mode 100644 index 000000000..2d2d74bdb --- /dev/null +++ b/tools/stack-upgrade-policy/decide_test.go @@ -0,0 +1,84 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import "testing" + +func TestDecideNoMigrationChangesPasses(t *testing.T) { + d := Decide(Evidence{}, Minor) + if !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } + if d.Qualifying { + t.Fatal("Qualifying = true, want false") + } +} + +func TestDecideAdditiveOnlyPassesOnMinor(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/sis_api/09_add_reservation_backup_disabled.up.sql", Status: Added, Class: Additive}, + }} + d := Decide(ev, Minor) + if !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } + if d.Qualifying { + t.Fatal("Qualifying = true, want false") + } +} + +func TestDecideDestructiveWithoutMajorFails(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", Status: Added, Class: Destructive}, + }} + d := Decide(ev, Minor) + if d.OK { + t.Fatal("OK = true, want false") + } + if !d.Qualifying { + t.Fatal("Qualifying = false, want true") + } + if d.Reason == "" { + t.Fatal("Reason is empty, want an explanation of what to do") + } +} + +func TestDecideDestructiveWithMajorPasses(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", Status: Added, Class: Destructive}, + }} + d := Decide(ev, Major) + if !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } + if !d.Qualifying { + t.Fatal("Qualifying = false, want true") + } +} + +// A deleted migration is how the compatibility bridge disappears: the backfill +// task stops shipping, so a cluster arriving later has no way to run it. That +// is the change that forces a customer stop, and it is invisible to a +// classifier that only reads added files. +func TestDecideDeletedMigrationWithoutMajorFails(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/05_add_model_specs.up.sql", Status: Deleted}, + }} + d := Decide(ev, Patch) + if d.OK { + t.Fatal("OK = true, want false") + } + if !d.Qualifying { + t.Fatal("Qualifying = false, want true") + } +} + +func TestDecideDeletedMigrationWithMajorPasses(t *testing.T) { + ev := Evidence{Changes: []Change{ + {Path: "keyspaces/nvcf_api/05_add_model_specs.up.sql", Status: Deleted}, + }} + if d := Decide(ev, Major); !d.OK { + t.Fatalf("OK = false, want true (reason: %s)", d.Reason) + } +} diff --git a/tools/stack-upgrade-policy/evidence.go b/tools/stack-upgrade-policy/evidence.go new file mode 100644 index 000000000..eda783045 --- /dev/null +++ b/tools/stack-upgrade-policy/evidence.go @@ -0,0 +1,113 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "fmt" + "os/exec" + "strings" +) + +func runGit(root string, args ...string) (string, error) { + cmd := exec.Command("git", args...) + cmd.Dir = root + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + return string(out), nil +} + +// LatestReleaseTag returns the highest published tag for a stack, for example +// deploy/stacks/self-managed/v1.0.0. Prereleases are skipped: they are not a +// version any customer was told to install, so they are not the baseline a +// release candidate is measured against. +func LatestReleaseTag(root, stackPath string) (string, error) { + out, err := runGit(root, "tag", "--list", stackPath+"/v*", "--sort=-v:refname") + if err != nil { + return "", err + } + for _, tag := range strings.Split(out, "\n") { + tag = strings.TrimSpace(tag) + if tag == "" { + continue + } + if strings.Contains(tag[strings.LastIndex(tag, "/")+1:], "-") { + continue + } + return tag, nil + } + return "", fmt.Errorf("no published release tag under %s", stackPath) +} + +// GatherEvidence reports every migration file that changed between a stack's +// last published release and HEAD. +// +// The comparison is against the release tag rather than against the pinned +// migrations image, because the stack does not pin that image: the +// cassandra.migrations.image block in global.yaml.gotmpl only emits a tag when +// an operator supplies one. Until it does, "what landed on main since the last +// stack release" is the closest available answer to what the bundle ships. +func GatherEvidence(root, baseRef string, paths []string) (Evidence, error) { + if len(paths) == 0 { + return Evidence{}, nil + } + args := append([]string{"diff", "--name-status", baseRef + "..HEAD", "--"}, paths...) + out, err := runGit(root, args...) + if err != nil { + return Evidence{}, err + } + + var ev Evidence + add := func(path string, status Status) error { + if !strings.HasSuffix(path, ".sql") { + return nil + } + if status == Deleted { + ev.Changes = append(ev.Changes, Change{Path: path, Status: Deleted}) + return nil + } + sql, err := runGit(root, "show", "HEAD:"+path) + if err != nil { + return err + } + ev.Changes = append(ev.Changes, Change{Path: path, Status: status, Class: Classify(sql)}) + return nil + } + + for _, line := range strings.Split(out, "\n") { + fields := strings.Split(strings.TrimSpace(line), "\t") + if len(fields) < 2 || fields[0] == "" { + continue + } + var err error + switch fields[0][0] { + case 'D': + err = add(fields[1], Deleted) + case 'R': + // A rename stops shipping the old path. Recording only the new one + // would hide a migration being renamed away. + if len(fields) < 3 { + continue + } + if err = add(fields[1], Deleted); err == nil { + err = add(fields[2], Modified) + } + case 'C': + // A copy leaves its source in place, so nothing is deleted. + if len(fields) < 3 { + continue + } + err = add(fields[2], Added) + case 'A': + err = add(fields[1], Added) + case 'M': + err = add(fields[1], Modified) + } + if err != nil { + return Evidence{}, err + } + } + return ev, nil +} diff --git a/tools/stack-upgrade-policy/evidence_test.go b/tools/stack-upgrade-policy/evidence_test.go new file mode 100644 index 000000000..33dc8c804 --- /dev/null +++ b/tools/stack-upgrade-policy/evidence_test.go @@ -0,0 +1,176 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "os/exec" + "path/filepath" + "testing" +) + +func git(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.com", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.com", + ) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return string(out) +} + +func write(t *testing.T, dir, rel, content string) { + t.Helper() + p := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +// fixture builds a repo with one released stack tag and returns its root. +func fixture(t *testing.T) string { + t.Helper() + dir := t.TempDir() + git(t, dir, "init", "-q", "-b", "main") + write(t, dir, "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql", + "CREATE TABLE IF NOT EXISTS nvcf_api.functions_v3 (id uuid PRIMARY KEY);") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "base") + git(t, dir, "tag", "deploy/stacks/self-managed/v1.0.0") + return dir +} + +func TestLatestReleaseTagPicksHighestSemver(t *testing.T) { + dir := fixture(t) + git(t, dir, "tag", "deploy/stacks/self-managed/v0.20.7") + git(t, dir, "tag", "deploy/stacks/self-managed/v1.0.0-rc.1") + + got, err := LatestReleaseTag(dir, "deploy/stacks/self-managed") + if err != nil { + t.Fatal(err) + } + if want := "deploy/stacks/self-managed/v1.0.0"; got != want { + t.Fatalf("LatestReleaseTag() = %q, want %q", got, want) + } +} + +func TestGatherEvidenceReportsNothingWhenNoMigrationsChanged(t *testing.T) { + dir := fixture(t) + write(t, dir, "src/foo/main.go", "package main") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "unrelated") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 0 { + t.Fatalf("Changes = %v, want none", ev.Changes) + } +} + +func TestGatherEvidenceClassifiesAnAddedDestructiveMigration(t *testing.T) { + dir := fixture(t) + write(t, dir, "migrations/cassandra/keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", + "ALTER TABLE IF EXISTS nvcf_api.functions_deployment_v2 DROP IF EXISTS gpu_specs;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "drop") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 1 { + t.Fatalf("Changes = %v, want exactly one", ev.Changes) + } + c := ev.Changes[0] + if c.Status != Added { + t.Errorf("Status = %v, want Added", c.Status) + } + if c.Class != Destructive { + t.Errorf("Class = %v, want Destructive", c.Class) + } +} + +func TestGatherEvidenceReportsADeletedMigration(t *testing.T) { + dir := fixture(t) + if err := os.Remove(filepath.Join(dir, "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql")); err != nil { + t.Fatal(err) + } + git(t, dir, "add", "-A") + git(t, dir, "commit", "-qm", "remove the bridge") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 1 { + t.Fatalf("Changes = %v, want exactly one", ev.Changes) + } + if ev.Changes[0].Status != Deleted { + t.Fatalf("Status = %v, want Deleted", ev.Changes[0].Status) + } +} + +// An additive migration added on top of a released tag must not be reported as +// qualifying, or every ordinary schema addition becomes a customer stop. +func TestGatherEvidenceClassifiesAnAddedAdditiveMigration(t *testing.T) { + dir := fixture(t) + write(t, dir, "migrations/cassandra/keyspaces/sis_api/09_add_reservation_backup_disabled.up.sql", + "ALTER TABLE sis_api.reservations ADD backup_disabled boolean;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "add column") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + if len(ev.Changes) != 1 { + t.Fatalf("Changes = %v, want exactly one", ev.Changes) + } + if ev.Changes[0].Class != Additive { + t.Fatalf("Class = %v, want Additive", ev.Changes[0].Class) + } +} + +// CodeRabbit on #1983: a rename was recorded only as Modified on the new path, +// so renaming a migration away produced no Deleted change and Decide would +// permit a non-major release even though the old migration no longer ships. +func TestGatherEvidenceTreatsARenameAsDeletingTheOldPath(t *testing.T) { + dir := fixture(t) + git(t, dir, "mv", + "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql", + "migrations/cassandra/keyspaces/nvcf_api/03_init_tables_renamed.up.sql") + git(t, dir, "commit", "-qm", "rename the migration") + + ev, err := GatherEvidence(dir, "deploy/stacks/self-managed/v1.0.0", []string{"migrations/cassandra"}) + if err != nil { + t.Fatal(err) + } + var deleted, present int + for _, c := range ev.Changes { + if c.Status == Deleted { + deleted++ + if c.Path != "migrations/cassandra/keyspaces/nvcf_api/03_init_tables.up.sql" { + t.Errorf("deleted path = %q, want the original name", c.Path) + } + continue + } + present++ + } + if deleted != 1 { + t.Fatalf("deleted count = %d, want 1 (changes: %+v)", deleted, ev.Changes) + } + if present != 1 { + t.Fatalf("non-deleted count = %d, want 1 for the new path (changes: %+v)", present, ev.Changes) + } +} diff --git a/tools/stack-upgrade-policy/go.mod b/tools/stack-upgrade-policy/go.mod new file mode 100644 index 000000000..db4edb34d --- /dev/null +++ b/tools/stack-upgrade-policy/go.mod @@ -0,0 +1,3 @@ +module stack-upgrade-policy + +go 1.26 diff --git a/tools/stack-upgrade-policy/main.go b/tools/stack-upgrade-policy/main.go new file mode 100644 index 000000000..9979bfa8b --- /dev/null +++ b/tools/stack-upgrade-policy/main.go @@ -0,0 +1,157 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Command stack-upgrade-policy reports the schema migrations a stack release +// candidate introduces, and fails when they do not match the version bump the +// candidate is proposing. +// +// stack-upgrade-policy --stack --bump +// +// The upgrade contract this enforces is that a customer installs each stack +// major version in order. That rule is derived from the version numbers +// themselves, so nothing has to publish a catalog or maintain a floor field +// per release. What the rule needs in exchange is that a major boundary is +// actually cut whenever the candidate contains something a customer cannot +// safely skip past. +// +// Deciding that from a stack-pin diff is not possible by eye: the diff is a +// list of chart versions moving, and nothing in "1.5.3 -> 2.0.0" says a +// migration landed. In a monorepo the evidence is computable instead, which +// also means it cannot be forgotten the way a declaration written in a pull +// request two months earlier can. +// +// Additive migrations never qualify. golang-migrate applies the ordered set +// per keyspace, so a cluster many versions behind still arrives at the right +// schema. Destructive migrations and deleted migration files do qualify: a +// drop is unrecoverable without a restore, and a deleted migration is how a +// compatibility bridge stops shipping, leaving a cluster that arrives later +// with no way to run it. +package main + +import ( + "encoding/json" + "flag" + "fmt" + "io" + "os" + "strings" +) + +func main() { + stack := flag.String("stack", "", "release-metadata id of the stack, for example nvcf-self-managed-stack") + bump := flag.String("bump", "", "proposed bump as a conventional-commit type, as reported by tools/ci/release-bump-type") + root := flag.String("root", ".", "repository root") + asJSON := flag.Bool("json", false, "emit the evidence report as JSON") + flag.Parse() + + if *stack == "" || *bump == "" { + flag.Usage() + os.Exit(2) + } + code, err := run(*root, *stack, *bump, *asJSON, os.Stdout, os.Stderr) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + os.Exit(code) +} + +// ParseBump maps a conventional-commit type onto the semver step it causes. +// The input is what tools/ci/release-bump-type reports, so that this and +// semantic-release agree on what the candidate is proposing rather than +// deriving it twice from different places. +func ParseBump(commitType string) (Bump, error) { + switch { + case strings.HasSuffix(commitType, "!"): + return Major, nil + case commitType == "feat": + return Minor, nil + case commitType == "fix": + return Patch, nil + default: + return NoBump, fmt.Errorf("unrecognized commit type %q: expected feat!, feat, or fix from release-bump-type", commitType) + } +} + +type reportChange struct { + Path string `json:"path"` + Status string `json:"status"` + Class string `json:"class"` +} + +type report struct { + Stack string `json:"stack"` + BaseTag string `json:"base_tag"` + Bump string `json:"bump"` + Qualifying bool `json:"qualifying"` + OK bool `json:"ok"` + Checked bool `json:"checked"` + Changes []reportChange `json:"changes"` +} + +func run(root, stackID, bumpType string, asJSON bool, out, errOut io.Writer) (int, error) { + stack, err := LoadStack(root, stackID) + if err != nil { + return 1, err + } + bump, err := ParseBump(bumpType) + if err != nil { + return 1, err + } + if len(stack.MigrationPaths) == 0 { + emit(out, asJSON, report{Stack: stack.ID, Bump: bump.String(), OK: true}) + return 0, nil + } + + baseTag, err := LatestReleaseTag(root, stack.Path) + if err != nil { + return 1, err + } + ev, err := GatherEvidence(root, baseTag, stack.MigrationPaths) + if err != nil { + return 1, err + } + decision := Decide(ev, bump) + + rep := report{Stack: stack.ID, BaseTag: baseTag, Bump: bump.String(), Qualifying: decision.Qualifying, OK: decision.OK, Checked: true} + for _, c := range ev.Changes { + rc := reportChange{Path: c.Path, Status: c.Status.String(), Class: c.Class.String()} + if c.Status == Deleted { + rc.Class = "" + } + rep.Changes = append(rep.Changes, rc) + } + + emit(out, asJSON, rep) + if !decision.OK { + fmt.Fprintln(errOut, decision.Reason) + return 1, nil + } + return 0, nil +} + +func emit(out io.Writer, asJSON bool, rep report) { + if asJSON { + enc := json.NewEncoder(out) + enc.SetIndent("", " ") + _ = enc.Encode(rep) + return + } + writeText(out, rep) +} + +func writeText(out io.Writer, rep report) { + if !rep.Checked { + fmt.Fprintf(out, "%s declares no migration paths; nothing to check.\n", rep.Stack) + return + } + fmt.Fprintf(out, "%s: %d migration change(s) since %s (proposed bump: %s)\n", + rep.Stack, len(rep.Changes), rep.BaseTag, rep.Bump) + for _, c := range rep.Changes { + if c.Class == "" { + fmt.Fprintf(out, " %-9s %s\n", c.Status, c.Path) + continue + } + fmt.Fprintf(out, " %-9s %-12s %s\n", c.Status, strings.ToLower(c.Class), c.Path) + } +} diff --git a/tools/stack-upgrade-policy/main_test.go b/tools/stack-upgrade-policy/main_test.go new file mode 100644 index 000000000..6ab69f38b --- /dev/null +++ b/tools/stack-upgrade-policy/main_test.go @@ -0,0 +1,171 @@ +// SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "encoding/json" + "strings" + "testing" +) + +// release-bump-type reports the conventional-commit type, not a semver word, +// so that is what this has to accept. +func TestParseBumpAcceptsReleaseBumpTypeOutput(t *testing.T) { + for in, want := range map[string]Bump{"feat!": Major, "fix!": Major, "feat": Minor, "fix": Patch} { + got, err := ParseBump(in) + if err != nil { + t.Fatalf("ParseBump(%q): %v", in, err) + } + if got != want { + t.Errorf("ParseBump(%q) = %v, want %v", in, got, want) + } + } +} + +func TestParseBumpRejectsGarbage(t *testing.T) { + if _, err := ParseBump("probably-fine"); err == nil { + t.Fatal("err = nil, want an error rather than a silent Patch") + } +} + +// fullFixture is a repo carrying both the release metadata and a published +// stack tag, so run() can be exercised end to end. +func fullFixture(t *testing.T) string { + t.Helper() + dir := fixture(t) + write(t, dir, MetadataPath, `{"version":1,"services":[ + {"id":"nvcf-self-managed-stack","path":"deploy/stacks/self-managed", + "migration_paths":["migrations/cassandra"]}]}`) + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "metadata") + return dir +} + +func TestRunFailsOnADestructiveMigrationWithoutAMajorBump(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/nvct_api/05_drop_health_info.up.sql", + "ALTER TABLE nvct_api.tasks_v2 DROP IF EXISTS health_info;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "drop health_info") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-self-managed-stack", "feat", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 1 { + t.Fatalf("exit = %d, want 1\nstdout:\n%s\nstderr:\n%s", code, out.String(), errOut.String()) + } + if !strings.Contains(errOut.String(), "05_drop_health_info.up.sql") { + t.Fatalf("stderr does not name the offending file:\n%s", errOut.String()) + } +} + +func TestRunPassesTheSameChangeWithAMajorBump(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/nvct_api/05_drop_health_info.up.sql", + "ALTER TABLE nvct_api.tasks_v2 DROP IF EXISTS health_info;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "drop health_info") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-self-managed-stack", "feat!", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 0 { + t.Fatalf("exit = %d, want 0\nstderr:\n%s", code, errOut.String()) + } +} + +// The report is the reason this runs on every candidate, not only failing +// ones: a release cutter staring at a list of chart version bumps otherwise +// has no way to know a migration landed. +func TestRunReportsAdditiveMigrationsAndStillPasses(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/sis_api/09_add_reservation_backup_disabled.up.sql", + "ALTER TABLE sis_api.reservations ADD backup_disabled boolean;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "add column") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-self-managed-stack", "fix", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 0 { + t.Fatalf("exit = %d, want 0", code) + } + if !strings.Contains(out.String(), "09_add_reservation_backup_disabled.up.sql") { + t.Fatalf("report omits the additive migration:\n%s", out.String()) + } +} + +func TestRunIsANoOpForAStackWithNoMigrationPaths(t *testing.T) { + dir := fullFixture(t) + write(t, dir, MetadataPath, `{"version":1,"services":[ + {"id":"nvcf-observability-stack","path":"deploy/stacks/observability"}]}`) + write(t, dir, "migrations/cassandra/keyspaces/nvcf_api/07_drop_gpu_spec.up.sql", + "DROP TYPE IF EXISTS nvcf_api.gpu_spec_udt;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "unrelated drop") + git(t, dir, "tag", "deploy/stacks/observability/v0.3.0") + + var out, errOut bytes.Buffer + code, err := run(dir, "nvcf-observability-stack", "fix", false, &out, &errOut) + if err != nil { + t.Fatal(err) + } + if code != 0 { + t.Fatalf("exit = %d, want 0\nstderr:\n%s", code, errOut.String()) + } +} + +func TestRunEmitsJSON(t *testing.T) { + dir := fullFixture(t) + write(t, dir, "migrations/cassandra/keyspaces/sis_api/09_add.up.sql", + "ALTER TABLE sis_api.reservations ADD backup_disabled boolean;") + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "add column") + + var out, errOut bytes.Buffer + if _, err := run(dir, "nvcf-self-managed-stack", "fix", true, &out, &errOut); err != nil { + t.Fatal(err) + } + if !strings.Contains(out.String(), `"class": "Additive"`) { + t.Fatalf("json output missing class field:\n%s", out.String()) + } +} + +func TestRunRejectsAnUnknownStack(t *testing.T) { + dir := fullFixture(t) + var out, errOut bytes.Buffer + if _, err := run(dir, "not-a-stack", "fix", false, &out, &errOut); err == nil { + t.Fatal("err = nil, want an error naming the unknown stack") + } +} + +// CodeRabbit on #1983: the no-migration-paths early return printed text before +// the JSON branch, so --json emitted non-JSON for the compute-plane and +// observability stacks and broke any consumer parsing it. +func TestRunEmitsJSONForAStackWithNoMigrationPaths(t *testing.T) { + dir := fullFixture(t) + write(t, dir, MetadataPath, `{"version":1,"services":[ + {"id":"nvcf-observability-stack","path":"deploy/stacks/observability"}]}`) + git(t, dir, "add", ".") + git(t, dir, "commit", "-qm", "observability only") + + var out, errOut bytes.Buffer + if _, err := run(dir, "nvcf-observability-stack", "fix", true, &out, &errOut); err != nil { + t.Fatal(err) + } + var parsed map[string]any + if err := json.Unmarshal(out.Bytes(), &parsed); err != nil { + t.Fatalf("--json did not emit JSON: %v\ngot: %s", err, out.String()) + } + if parsed["stack"] != "nvcf-observability-stack" { + t.Errorf("stack = %v", parsed["stack"]) + } +}