From 99391cf37d6563cc75ef67d0e1e6f53d0d3ff218 Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 17:46:57 +0800 Subject: [PATCH 01/12] docs(chart): retract the delivery-mode seed promise - values.yaml states the chart's declaration owns model-artifact-delivery-mode and that the worker reasserts it on every start - operations.md drops the "set the Setting to Engine before upgrading" escape and states the variable is reasserted on each worker start - the deployment template comment no longer claims a stored value always wins; the rendered output is unchanged apart from that comment - chart README and values schema regenerate from the values doc block - the spec records the template as a third document carrying the promise and names the settings table row that T5 owns Task 4 of settings-env-sync-and-default-tracking. Signed-off-by: thxCode --- deploy/gpustack-operator/chart/README.md | 2 +- .../chart/templates/worker/deployment.yaml | 3 +- .../chart/values.schema.json | 2 +- deploy/gpustack-operator/chart/values.yaml | 12 +- docs/modules/model-delivery/operations.md | 19 +- ...-settings-env-sync-and-default-tracking.md | 524 ++++++++++++++++++ 6 files changed, 544 insertions(+), 18 deletions(-) create mode 100644 specs/2026-10-19-settings-env-sync-and-default-tracking.md diff --git a/deploy/gpustack-operator/chart/README.md b/deploy/gpustack-operator/chart/README.md index b7b9d7fab..eaffa3565 100644 --- a/deploy/gpustack-operator/chart/README.md +++ b/deploy/gpustack-operator/chart/README.md @@ -169,7 +169,7 @@ Kubernetes: `>=1.23.0-0` | deviceManager.tolerations | list | `[{"operator":"Exists"}]` | Tolerations for the device-manager DaemonSets. Defaults tolerate every taint so a manager schedules on any node carrying the matching accelerator. | | deviceManager.extraArgs | list | `[]` | Extra command-line arguments appended to each device-manager container args. | | deviceManager.env | object | `{}` | Extra environment variables for the device-manager, as a name/value map. | -| modelManager.enabled | bool | `true` | Deploy the model-manager: the CSI node plugin that delivers a Hugging Face ModelArtifact's weights from each node's cache, and its CSIDriver `model.csi.gpustack.ai`. When enabled, the worker seeds the `model-artifact-delivery-mode` Setting with `Node`. A seed fills only a Setting the cluster does not have yet, so a later upgrade never overrides an administrator's choice. Upgrading from a version without that Setting rolls every ModelDeployment on a Hugging Face artifact once, to node delivery; set the Setting to `Engine` before upgrading, or upgrade with this disabled, to avoid it. | +| modelManager.enabled | bool | `true` | Deploy the model-manager: the CSI node plugin that delivers a Hugging Face ModelArtifact's weights from each node's cache, and its CSIDriver `model.csi.gpustack.ai`. When enabled, the chart declares `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE=Node`, so the chart owns `model-artifact-delivery-mode`. The worker reasserts `Node` on every start and reverts a runtime edit to the Setting. `modelManager.enabled` is the only signal that reaches this value; the worker flag `--model-manager-service-account` defaults to a non-empty name and cannot indicate whether the plugin is deployed. With the plugin enabled, upgrading from a version without the Setting rolls every ModelDeployment on a Hugging Face artifact once, to node delivery. | | modelManager.rootPath | string | `"/var/lib/gpustack/models"` | The node's model cache directory, a hostPath. Mount a dedicated filesystem here: on one shared with kubelet the plugin caps its high watermark below kubelet's eviction and image collection thresholds. Changing it points the plugin at an empty cache; nothing is migrated or removed from the old path. | | modelManager.kubeletDir | string | `"/var/lib/kubelet"` | kubelet's root directory, the convention the bundled CSI drivers follow. The plugin mounts its pods directory with Bidirectional propagation so its bind mounts reach the Pods. | | modelManager.securePort | int | `32444` | Secure serving port of the model-manager container: metrics, readiness and liveness. | diff --git a/deploy/gpustack-operator/chart/templates/worker/deployment.yaml b/deploy/gpustack-operator/chart/templates/worker/deployment.yaml index 63449a6b9..c62e9f101 100644 --- a/deploy/gpustack-operator/chart/templates/worker/deployment.yaml +++ b/deploy/gpustack-operator/chart/templates/worker/deployment.yaml @@ -135,7 +135,8 @@ spec: - name: GPUSTACK_LEDGER_RELEASE_TERMINATED_PODS value: {{ .Values.ledgerReleaseTerminatedPods | quote }} {{- if .Values.modelManager.enabled }} - # Seeds the delivery Setting where the cluster has none yet; a stored value always wins. + # Declares the chart-owned delivery mode; the worker asserts it on every start, reverting + # any stored value that differs from this declaration at the next worker start. - name: GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE value: Node {{- end }} diff --git a/deploy/gpustack-operator/chart/values.schema.json b/deploy/gpustack-operator/chart/values.schema.json index 640d2928a..6c7a8bf83 100644 --- a/deploy/gpustack-operator/chart/values.schema.json +++ b/deploy/gpustack-operator/chart/values.schema.json @@ -1223,7 +1223,7 @@ "properties": { "enabled": { "default": true, - "description": "Deploy the model-manager: the CSI node plugin that delivers a Hugging Face ModelArtifact's\nweights from each node's cache, and its CSIDriver `model.csi.gpustack.ai`.\n\nWhen enabled, the worker seeds the `model-artifact-delivery-mode` Setting with `Node`. A seed\nfills only a Setting the cluster does not have yet, so a later upgrade never overrides an\nadministrator's choice. Upgrading from a version without that Setting rolls every\nModelDeployment on a Hugging Face artifact once, to node delivery; set the Setting to `Engine`\nbefore upgrading, or upgrade with this disabled, to avoid it.", + "description": "Deploy the model-manager: the CSI node plugin that delivers a Hugging Face ModelArtifact's\nweights from each node's cache, and its CSIDriver `model.csi.gpustack.ai`.\n\nWhen enabled, the chart declares `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE=Node`, so the chart\nowns `model-artifact-delivery-mode`. The worker reasserts `Node` on every start and reverts\na runtime edit to the Setting. `modelManager.enabled` is the only signal that reaches this\nvalue; the worker flag `--model-manager-service-account` defaults to a non-empty name and\ncannot indicate whether the plugin is deployed. With the plugin enabled, upgrading from a\nversion without the Setting rolls every ModelDeployment on a Hugging Face artifact once,\nto node delivery.", "title": "enabled", "type": "boolean" }, diff --git a/deploy/gpustack-operator/chart/values.yaml b/deploy/gpustack-operator/chart/values.yaml index 2b6fc8b7a..e091a4ec3 100644 --- a/deploy/gpustack-operator/chart/values.yaml +++ b/deploy/gpustack-operator/chart/values.yaml @@ -433,11 +433,13 @@ modelManager: # -- Deploy the model-manager: the CSI node plugin that delivers a Hugging Face ModelArtifact's # weights from each node's cache, and its CSIDriver `model.csi.gpustack.ai`. # - # When enabled, the worker seeds the `model-artifact-delivery-mode` Setting with `Node`. A seed - # fills only a Setting the cluster does not have yet, so a later upgrade never overrides an - # administrator's choice. Upgrading from a version without that Setting rolls every - # ModelDeployment on a Hugging Face artifact once, to node delivery; set the Setting to `Engine` - # before upgrading, or upgrade with this disabled, to avoid it. + # When enabled, the chart declares `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE=Node`, so the chart + # owns `model-artifact-delivery-mode`. The worker reasserts `Node` on every start and reverts + # a runtime edit to the Setting. `modelManager.enabled` is the only signal that reaches this + # value; the worker flag `--model-manager-service-account` defaults to a non-empty name and + # cannot indicate whether the plugin is deployed. With the plugin enabled, upgrading from a + # version without the Setting rolls every ModelDeployment on a Hugging Face artifact once, + # to node delivery. enabled: true # -- The node's model cache directory, a hostPath. Mount a dedicated filesystem here: on one # shared with kubelet the plugin caps its high watermark below kubelet's eviction and image diff --git a/docs/modules/model-delivery/operations.md b/docs/modules/model-delivery/operations.md index 304b36fcd..845442e23 100644 --- a/docs/modules/model-delivery/operations.md +++ b/docs/modules/model-delivery/operations.md @@ -199,16 +199,15 @@ names refuses to start, so a Kueue upgrade checks it first. ## Upgrade notes -**From the version before node delivery.** With the plugin enabled, the upgrade seeds -`model-artifact-delivery-mode=Node`, and every `ModelDeployment` using a hub artifact rolls -once, to Node delivery. A seed fills only a Setting the cluster does not have yet, so there are two -ways to avoid the roll: - -1. set the Setting to `Engine` before upgrading, which the seed then leaves alone; or -2. upgrade with `--set modelManager.enabled=false`, which seeds nothing and keeps `Engine`. - -Either way you can switch later, at a time of your choosing. A later `helm upgrade` never overrides -the Setting. +**From the version before node delivery.** With `modelManager.enabled: true`, the chart declares +`GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE=Node`. The worker reasserts that value on every start, so +a runtime edit of `model-artifact-delivery-mode` to `Engine` is reverted at the next worker start; +it does not avoid the upgrade roll. Every `ModelDeployment` using a hub artifact rolls once, to +Node delivery. + +To keep `Engine` through the upgrade, set `modelManager.enabled=false`; this also removes the +plugin and its CSIDriver. If you enable the plugin later, the chart declares `Node` again and the +worker writes it at its next start. **To the version with the placement preference.** Kueue restarts with `TASRespectNodeAffinityPreferred` on. Running replicas are not touched; new node-delivered Pods carry the preference. A Pod in a TAS diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md new file mode 100644 index 000000000..abb3c1182 --- /dev/null +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -0,0 +1,524 @@ +# Spec: Settings Sync Semantics — Environment Declarations and Default Tracking + +Status: Building +Blocked on: nothing. T1 to T5 are ready to build; the open question at the end is a confirmation, not a dependency. +Type: Feature + +## Summary + +A `GPUSTACK_*` environment variable on the operator worker seeds its Setting only when the delegated Secret +`gpustack-settings` is created. Every later align pass adds only the keys that are absent, so a variable added or +corrected after the first deploy never reaches its Setting, and the failure is silent. This spec replaces the +create-once seed with a three-tier sync model evaluated on every worker start: an explicitly declared, non-blank +environment variable is authoritative; a Setting named in a new `settings-default-tracking` Setting follows the +operator's built-in default; every other Setting keeps the value an administrator stored. Every override the +model performs is logged. + +## Motivation + +### Goals + +1. **An explicit environment declaration wins.** When `GPUSTACK_` is present in the worker + process environment and its value is non-blank, the worker writes that value to the Setting on every start. + A declaration is a standing desired state, not a one-time default. +2. **The operator's own defaults reach upgrades.** A Setting whose value comes from a built-in default and that no + administrator has claimed is overwritten when the operator ships a new default. Without this, a cluster stays + on whatever the default was at first install, forever, with no way to tell a deliberate pin from a stale + default. +3. **A stored value the operator does not own is never touched.** A Setting that is neither declared in the + environment nor named in the tracking Setting keeps the value an administrator wrote, including a blank one. +4. **Every override is visible.** Each write that changes a stored value logs one line naming the Setting, the + old value, the new value, and the rule that fired. A value that silently reverts after a restart is the worst + failure mode of this feature, and it must never happen without a log line. + +### Non-Goals + +1. **Inferring whether an administrator chose a value.** This design records no provenance marker and never + compares a stored value against a previous default to guess intent. Intent is either an environment + declaration or membership in `settings-default-tracking`. A per-setting provenance annotation was considered + and rejected; see [Alternatives](#alternatives). +2. **Continuous reconciliation while the worker runs.** An environment variable cannot change inside a running + Pod. "Every worker start" is the finest granularity the source allows. +3. **Restricting what an administrator may write.** `kubectl edit setting` and `kubectl patch setting` continue to + write any value, including a blank one. This spec governs only what the operator writes. +4. **Migrating existing clusters' stored values.** No value already in `gpustack-settings` changes at upgrade + time except through rules 1 and 2 below, on the first start of the new operator. +5. **A switch that turns the model off.** Rules 1 and 2 are unconditional once this ships. A cluster that wants + the previous create-once behavior removes the Setting's name from `settings-default-tracking` and unsets the + variable, which is the same procedure it already needs for a pin. + +## Proposal + +### User Stories + +#### Story 1: An Environment Declaration Takes Effect on the Next Start + +As a cluster administrator who declared `GPUSTACK_INSTANCE_ACCESS_STATIC_ADDRESS` in the worker Deployment, I +want that value to reach the `instance-access-static-address` Setting, so that my Instances publish the address I +configured instead of falling back to a node IP. + +#### Story 2: A Pin Is Not Reverted by a Restart + +As a cluster administrator who set `container-registry` to my own mirror at runtime, I want that value to survive +a worker restart, so that a node drain or a crash does not silently move my pulls back to the upstream registry. + +#### Story 3: Operator Upgrades Carry New Image Defaults + +As a cluster administrator on an older release, I want a new operator version's default for +`model-deployment-router-image` to reach my cluster, so that I pick up upstream fixes without hand-patching every +image Setting after each upgrade. + +#### Story 4: A Pin Is Kept Once the Setting Is Untracked + +As a cluster administrator who pinned `kv-cache-backend-image` to an image my air-gapped registry serves, I want +that pin kept once I remove the Setting from `settings-default-tracking`, so that later operator upgrades do not +break the KV cache backends I already run. + +The first operator upgrade that carries this spec is the exception: the tracking Setting ships with six names in +it, and an older cluster has no stored value for it, so the operator's default applies. A pin made before that +upgrade is replaced by the operator default on the first start, and the log line says so. After that upgrade the +Setting exists, and untracking it is what keeps the pin. + +#### Story 5: Every Override Is Explained + +As a cluster administrator whose Setting changed without my action, I want a log line naming the Setting, both +values, and the reason, so that I can tell an override from a mistake of my own. + +### Core Features & Acceptance Criteria + +Let `D` be a Setting's seed: the value of `GPUSTACK_` when that variable is present in the +worker environment, otherwise the built-in default. Let `envDeclared` mean the variable is present. Let `tracked` +mean the Setting's name appears in the `settings-default-tracking` value. + +On every worker start, for each registered Setting, in this order: + +| # | `envDeclared` | `D` non-blank | `tracked` | Action | +|---|---|---|---|---| +| 1 | yes | yes | any | Write `D`. Log when the stored value changes. | +| 2 | no, or `D` blank | — | yes | Write `D` when `D` is non-blank. Log when the stored value changes. | +| 3 | no, or `D` blank | — | no | No write. | +| 4 | any | any | any | On creation of the Secret only: write `D` for every Setting, as today. | + +**Rule 1 fires only for a non-blank declaration.** A variable present with an empty value is treated as no +declaration, because a blank carries no information. + +**Rules 1 and 2 never write a blank value.** A blank seed is never written by either rule, so a Setting with a +blank default that is named in `settings-default-tracking` leaves the stored value alone. Rule 4 is the one +exception: it reproduces today's creation behavior, which writes every seed including blank ones, so that +`kubectl get settings` lists every Setting and a blank-default Setting that did not exist yet is backfilled. + +**Rules 1 and 2 cannot disagree.** Both write `D`. When a variable is declared, `D` is that variable's value, so +a Setting is never written twice in one pass with two different values. + +**An absent key is backfilled before any rule is evaluated.** The align pass first adds every registered key the +Secret does not carry, exactly as today. Rule 1 and rule 2 then decide only whether to overwrite a key that +already exists. Without this order a newly registered blank-default Setting would never appear. + +**The tracking list is read from the live Secret, falling back to the built-in default.** On a Secret that +predates this spec the key is absent, so the built-in default applies on the same start that backfills the key. +Reading only the live Secret would defer the transition to a later start. + +**Removing a variable does not restore a built-in default.** With no declaration and no tracking membership, +row 3 applies and the stored value is kept, including a value a previous release declared. Unsetting a variable +stops the operator asserting it; it does not reset the Setting. A tracked Setting still falls back to its +built-in default under rule 2. + +**The only way to stop following a default is to untrack the Setting.** Because a blank declaration reads as no +declaration, and a blank default is never written, removing the name from `settings-default-tracking` is the sole +way to keep a stored value across an operator upgrade. + +**Setting 1: `settings-default-tracking`** — an editable Setting whose value is a comma-separated list of +Setting names. It is the only Setting this spec adds. It defaults to the six image Settings that carry a built-in +default: + +``` +instance-ssh-server-image,kv-cache-backend-image,model-deployment-router-image, +model-deployment-router-proxy-image,model-deployment-routing-sidecar-image, +model-prefetch-warmup-image +``` + +Its own name may not appear in the list. An entry naming itself is ignored and logged, because a Setting that +tracked itself would reconcile its own membership. + +The list is parsed before the align loop, and the parse is fixed so that a stored value cannot silently change +which Settings are tracked: + +1. Split on `,`. +2. Trim surrounding whitespace from each segment; a segment that is empty afterwards is dropped. +3. Drop a duplicate that has already been seen. +4. Drop the Setting's own name. +5. Drop any segment that names no registered Setting, so a name retired in a later release stops mattering. + +Every dropped segment is logged once, at `klog.V(4)`, naming the segment and the reason. + +**Override logging.** Each of rules 1 and 2 that changes a stored value emits one `klog.InfoS` line carrying the +Setting name, the old value, the new value, and the rule that fired. A pass that changes nothing logs nothing. +A rule-2 write where the stored value already equals `D` is not an override and is not logged. + +**The chart keeps `model-artifact-delivery-mode`.** `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` is a non-blank +declaration, so rule 1 makes it authoritative and the chart owns the Setting. The worker cannot take it over: +`--model-manager-service-account` defaults to a non-empty name, so it carries no signal about whether the plugin +is deployed, and the code comment in `pkg/worker/settings/model_store.go` states the Setting is never inferred +from plugin installation. The chart's `modelManager.enabled` is the only signal that reaches the value. + +The cost is that an administrator who sets the Setting to `Engine` at runtime sees it revert on the next worker +start, and `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE=Node` is reinstalled. Three places promise the opposite today, so +all three change: +[operations.md](../docs/modules/model-delivery/operations.md), +[values.yaml](../deploy/gpustack-operator/chart/values.yaml), and the comment above the variable in +[deployment.yaml](../deploy/gpustack-operator/chart/templates/worker/deployment.yaml), which says a stored value +always wins. The template renders the same variable and the same value; only its comment changes. The +`model-artifact-delivery-mode` row of the Settings table carries the same promise and is restated by the task +that owns that page. + +### Notes / Constraints / Caveats + +1. **Six image Settings become runtime-read-only.** Rule 2 fires on every worker start, not only on an upgrade. A + `kubectl patch setting kv-cache-backend-image` therefore holds until the next start, then reverts to the + operator default. Pinning means removing the name from `settings-default-tracking`. +2. **`image-pull-policy` becomes chart-owned.** The chart renders `GPUSTACK_IMAGE_PULL_POLICY=IfNotPresent`, which + is non-blank, so rule 1 applies and the chart is the source of truth. Change it through chart values, not + through the Setting resource. +3. **Manufacturer identity variables are unaffected.** The chart renders them on every start, but no + administrator patches them, so rule 1 writing them is a no-op. +4. **`container-registry`, `container-namespace`, and `image-pull-secrets` default to blank.** Rule 1 does not fire + for a blank declaration, so these stay administrator-owned exactly as today. +5. **Deleting the Secret resets the tracking list.** `settings-default-tracking` returns to its default, so image + pins made without untracking are lost. This is the same exposure as deleting any Setting. +6. **`systemmeta` notes are untouched by this spec.** The existing per-Setting `-uid` note is regenerated on each + `Initialize`, which makes `EqualResourceTypeAndNotes` false and forces a Secret update on every worker start. + That churn is pre-existing and out of scope here; this design deliberately needs no note to carry intent. +7. **Nothing reads a Setting value through a path that requires it to be create-once.** Startup writes defaults + directly and does not run `Admission`; the read paths fall back to a Setting's default when the key is absent; + `AllowOnceConfigure` has no caller. The 30-second read cache is per process, so a fresh worker sees the + aligned Secret immediately. + +### Boundaries + +- **Always:** treat a blank declaration as no declaration; backfill an absent key before evaluating any rule; + never let rules 1 or 2 write a blank value; log every value change with both values and the rule. +- **Ask first:** any new Setting that would make an existing chart-rendered variable authoritative; any change to + which chart-rendered variable the operator owns. +- **Never:** infer administrator intent by comparing a stored value against a default; overwrite a stored + non-blank value for a Setting that is neither declared nor tracked; restrict what `kubectl edit setting` may + write. + +### Risks and Mitigations + +- **Risk: an upgrade silently replaces an image pin made before this spec.** + → This is accepted, and it is the sharpest edge in the design. `settings-default-tracking` ships with six + names and an older Secret has no stored value for it, so the built-in default applies on the same start that + backfills the key. Mitigation: rule 2 logs the replacement with both values at the moment it happens, and the + Settings reference page states that a pin is kept by removing the Setting from the tracking list. No pre-upgrade + escape is documented, by decision. +- **Risk: a runtime patch of a rule-1 or rule-2 Setting is reverted on the next worker start and the administrator + does not know why.** → Mitigation: the log line names the rule and the variable, so the cause is either the + Deployment or the tracking list. The reference page says which Settings those are. +- **Risk: an administrator expects an environment variable to be a one-time default and does not expect it to be + re-asserted.** → Mitigation: this is the intended behavior change and is the headline of the Settings reference + page. The per-Setting table states, for every declared variable, that the Deployment owns it. +- **Risk: a worker start rewrites several Settings at once.** → Mitigation: every write lands in the one Secret + update the align pass already performs, so the whole pass costs a single API call at startup regardless of how + many keys change. The existing note churn already forces that update today, so a pass that changes no data costs + nothing extra. + +## Design Details + +### Commands + +Build and test run **locally**, on the developer host, in the worktree checked out at the base revision above. +No cluster, no accelerator, and no `helm install` is needed: every case in the Test Plan runs against +`pkg/kubeclients/kubernetes/fake`. + +The Makefile exposes one target per script in `hack/`, so `make ` runs `hack/.sh`. + +```bash +# Gate for every task: Go lint (golangci-lint) over the tree. +make lint + +# Gate for the docs task: link and anchor resolution, the docs/README.md index, the spec Status +# word, every `go test -run` this spec names, and cross-references. +make lint docs + +# Gate for the chart task: image assertions over rendered output, the chart's own shell checks, +# and `helm lint`. Needs no cluster. +make lint chart + +# Unit tests, matching hack/test.sh: race detector on, tests shuffled, coverage into .dist/test. +# RACE=false drops the detector and changes nothing else. +make test + +# Targeted run, the form every task's Verify line uses. +GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race -v ./pkg/setting \ + -run TestSettingsInitialize + +# Baseline coverage for the Test Plan. +GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -cover ./pkg/setting/... +``` + +`check-specs.sh` resolves every `go test -run` in this file against the real top-level test names in the tree. +Between this spec being written and T2 landing, the gate reports T2's `-run TestSettingsInitialize` as +selecting nothing, because the test it names does not exist yet. That is the gate working, not a defect: T2's +Verify line is the first one that turns it green, and T5 must not run before then. The `NOT RE-RUNNABLE` escape +is deliberately not used here, because these tests are re-runnable and will exist. + +**No test outside T2 may carry the `TestSettingsInitialize` prefix.** `-run` matches a prefix, so a test named +`TestSettingsInitializerMetadata` satisfies rule 3 without T2 having written the test it names. Measured on +2026-10-19: T1's test of that name turned the docs gate green while T2 was still unstarted, and the gate was then +reporting success for a pattern that ran something this specification never promised. That is the false green rule +3 exists to catch, arriving through a name rather than through a stale one, so it costs nothing to close. + +### Project Structure + +Repo-root-relative layout touched by this spec: + +```text +pkg/setting/ +├── kuberes.go # Settings.Initialize: create the delegated Secret, then apply rules 1, 2 and 3 +├── helper.go # InitializeFromEnv, InitializeFrom, isBlank (the one definition of "blank") +├── types.go # Setting, the Settings registry, and the reads against the delegated Secret +└── kuberes_test.go # NEW: table-driven sync cases driven through the fake clientset +pkg/worker/settings/ +├── value.go # Every built-in Setting, including settings-default-tracking +└── value_test.go # The tracking list's default value and self-reference refusal +pkg/worker/ +└── extensionapis/worker/instance.go # Reads instance-access-static-address to publish Instance addresses +deploy/gpustack-operator/chart/ +└── values.yaml # Retracts the modelManager prose that promises a seed never overrides +docs/reference/settings.md # Rewrites the seeding contract, adds the tracking Setting, restates the table +docs/README.md # Index entry for this spec +docs/modules/model-delivery/operations.md # Corrects the upgrade procedure under the new rules +specs/2026-10-19-settings-env-sync-and-default-tracking.md +``` + +### Code Style + +The align pass does two things in order, because they answer different questions: backfill what the Secret does +not carry, then decide whether the operator owns a value that is already there. A key that is absent and a key +that is present are different cases, and checking absence before blankness is what keeps a newly registered +blank-default Setting backfillable. + +```go +alignFn := func(aSec *core.Secret) (_ *core.Secret, skip bool, err error) { + skip = true + // Align delegated info. + if !systemmeta.EqualResourceTypeAndNotes(eSec, aSec) { + systemmeta.NoteResource(aSec, eResType, eNotes) + skip = false + } + + // Backfill every registered key the Secret lacks, as today. A blank default is a key too: + // kubectl get settings lists it, and a read of a missing key falls back with an error. + for name, s := range h { + if _, ok := aSec.Data[name]; !ok { + aSec.Data[name] = []byte(s.defVal) + skip = false + } + } + + // The tracking list is read from the live Secret, falling back to the built-in default, so a + // Secret that predates this Setting gets the default on the same start that backfills it. + tracking := parseTrackedSettings(trackingValue(aSec)) + + for name, s := range h { + stored := string(aSec.Data[name]) + desired, rule := s.desiredValue(tracking) + if desired == "" || desired == stored { + // Rules 1 and 2 never write a blank value, and an equal value is not an override. + continue + } + aSec.Data[name] = []byte(desired) + skip = false + klog.InfoS("setting overridden on start", + "setting", name, "stored", stored, "value", desired, "rule", rule) + } + return aSec, skip, nil +} +``` + +`desiredValue` carries the whole rule set, and it is where the collapsed predicate lives: a Setting is written +when its seed is non-blank and the environment declares it or the tracking list names it. + +```go +// desiredValue returns the value the operator owns for this Setting and the rule that claimed it. +// An empty rule means the operator owns nothing here, and the stored value is the administrator's. +func (s Setting) desiredValue(tracked sets.Set[string]) (value string, rule string) { + if isBlank(s.defVal) { + return "", "" + } + if s.declared { + return s.defVal, "env-declared" + } + if tracked.Has(s.name) { + return s.defVal, "default-tracked" + } + return "", "" +} +``` + +Conventions to keep: use `klog.InfoS` with key-value pairs, as in `pkg/kubeapp/lock.go`; reuse `isBlank` rather +than testing for an empty string; keep one Setting name per log line so a grep finds every override; do not +introduce a second meaning of "blank" anywhere in this path. + +### Implementation Plan + +``` +T1 ──▶ T2 ──▶ T3 ──┐ + ├──▶ T5 +T4 ───────────────┘ +``` + +- [ ] **T1 · Carry the declaration source on `Setting`** + Blocked by: None + Owns: `pkg/setting/helper.go`, `pkg/setting/types.go`, `pkg/setting/types_test.go`, + `pkg/worker/settings/value_test.go` + Gate: review + Acceptance: `Initializer` reports the environment variable it reads, so `Setting` carries an `envName` + and a `declared` flag fixed at registration; `InitializeFromEnv` and `InitializeFromSpecifiedEnv` set + it and `InitializeFrom` leaves it empty; `declared` means presence in the environment, not a non-blank + value; every one of the 36 registered Settings keeps the exact default value it produces today; the four + call sites in `pkg/worker/settings/value_test.go` that invoke an initializer directly bind only the value + they assert, and each keeps its expectation, its message and its `t.Setenv` line; no existing test changes + behavior; the whole tree compiles. + Verify: `GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race ./pkg/setting/... && make lint` + +- [ ] **T2 · Apply the sync predicate in the align pass, with the regression guard** + Blocked by: T1 + Owns: `pkg/setting/kuberes.go`, `pkg/setting/kuberes_test.go` + Gate: review + Acceptance: absent keys are backfilled first, so a blank-default Setting is still added; the tracking list + is read from the live Secret and falls back to the built-in default when the key is absent, and is parsed + by the five-step contract in Core Features; `desiredValue` returns a value only for a non-blank seed that + is either declared or tracked; every change to a stored value logs one `klog.InfoS` line carrying the + Setting, both values and the rule; `TestSettingsInitialize` covers the table below and passes. + Verify: `GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race -v ./pkg/setting -run TestSettingsInitialize && make lint` + +- [ ] **T3 · Register `settings-default-tracking`** + Blocked by: T2 + Owns: `pkg/worker/settings/value.go`, `pkg/worker/settings/value_test.go` + Gate: none + Acceptance: one new editable Setting whose value is a comma-separated list of Setting names, defaulting to + the six image Settings; an admission refuses a value containing its own name; `make lint` passes. + Verify: `GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race ./pkg/worker/settings/... && make lint` + +- [x] **T4 · Retract the chart's promise about the delivery-mode seed** + Blocked by: None + Owns: `deploy/gpustack-operator/chart/values.yaml`, `deploy/gpustack-operator/chart/templates/worker/deployment.yaml`, + `docs/modules/model-delivery/operations.md` + Gate: review + Acceptance: `values.yaml` no longer says a later upgrade never overrides an administrator's choice for + `modelManager.enabled`, and says the chart's declaration owns the Setting; `operations.md` drops the + "set the Setting to `Engine` before upgrading, which the seed then leaves alone" escape and states that + the variable is re-asserted on every worker start; the comment above `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` + in `deployment.yaml` no longer says a stored value always wins. The template renders the same variable with + the same value, because the chart keeps rendering `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE`; only the comment + changes. + Verify: `make lint chart && make lint docs` + +- [ ] **T5 · Restate the Settings contract and index this spec** + Blocked by: T2, T3, T4 + Owns: `docs/reference/settings.md`, `docs/README.md` + Gate: none + Acceptance: the seeding paragraph states rules 1 to 4 with the blank, backfill and tracking-read + exceptions; the per-Setting table gains `settings-default-tracking` and marks every chart-rendered + variable as owned by the Deployment, which includes dropping the `model-artifact-delivery-mode` row's + "a seed never overrides a stored value"; the page states that a tracked image Setting reverts to the operator + default on every worker start and that untracking is what keeps a pin; `docs/README.md` links this spec + under Installation, whose stated purpose covers upgrade and migration paths. This is the first task at which + the docs gate is green, because it resolves T2's `-run` pattern against a test that now exists. + Verify: `make lint docs` + +### Test Plan + +[x] I/we understand the owners of the involved components may require updates to existing tests to make this +code solid enough prior to committing the changes necessary to implement this enhancement. + +#### Prerequisite testing updates + +None. `pkg/setting` has no `kuberes_test.go`, so this work adds the first test of `Settings.Initialize` rather +than reshaping one. `pkg/setting/settingtest.MergeDelegatedSettings` already merges keys into the delegated +Secret for tests that read a value, and T2's cases drive the writer instead, so they build their own fake +clientset the way `pkg/extensionapi/setting_test.go` does. + +#### Unit tests + +Coverage baseline for the two packages measured on 2026-10-19 with +`GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -cover`: + +- `pkg/setting`: `2026-10-19` - `12.7%` + - `TestSettingsInitialize_DeclaredSettingOverridesStoredValue`: the guard that pins this feature. The Secret + stores `instance-access-static-address=old.example`, `GPUSTACK_INSTANCE_ACCESS_STATIC_ADDRESS=new.example` + is set, the Setting is untracked, and the stored value becomes `new.example`. Remove rules 1 and 2 and it + stays `old.example`, so this row alone fails without the feature. + - `TestSettingsInitialize_TrackedSettingFollowsBuiltInDefault`: no declaration, the tracking list names the + Setting, the stored value differs from the built-in default, and the stored value becomes the default. + - `TestSettingsInitialize_UntrackedStoredValueIsLeftAlone`: no declaration, not tracked, covering a non-blank + administrator value and a blank one. + - `TestSettingsInitialize_BackfillsAbsentKeysIncludingBlanks`: the Secret lacks a key whose seed is blank, and + the key is present afterwards, matching today's behavior. + - `TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent`: the Secret has no + `settings-default-tracking` key, and a tracked Setting follows the built-in default on that same pass. + - `TestSettingsInitialize_EqualValueIsNotAnOverride`: the stored value already equals the seed, and nothing is + written and nothing is logged. + - `TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration`: the variable is present with an empty value, and + an untracked Setting keeps its stored value. + - `TestParseTrackedSettings`: whitespace around a segment, empty segments, duplicates, an unknown name, and + the Setting's own name, asserting the five-step normalization and that a dropped segment does not track. + - `TestSettingsInitialize_CreationSeedsEverySetting`: an empty Secret receives every registered key, blank + defaults included. +- `pkg/worker/settings`: `2026-10-19` - `78.7%` + - `TestSettingsDefaultTracking_DefaultValue`: the default lists exactly the six image Settings. + - `TestSettingsDefaultTracking_RefusesItsOwnName`: an admission rejects a value containing + `settings-default-tracking`. + +Every case is one table row with declarative data, run through a single loop, against +`kubefake.NewSimpleClientset` with a reactor allowing `selfsubjectaccessreviews`, because `Initialize` calls +`review.CanDoUpdate` first and the fake client denies it by default. + +#### Integration tests + +None. No controller reads these keys on a path this change alters, and `Settings.Initialize` runs once at +startup against a single Secret, which the fake clientset models completely. + +#### e2e tests + +Justification for not adding one: the behavior under test is a table of writes against one Secret at worker +startup. An e2e run would assert the same rows through a slower path and would need a cluster this repository's +`testing/infra` provisions per session. The rows that an e2e could add value to — a Helm upgrade replacing a +pinned image Setting — is the accepted risk recorded above rather than a defect, and it is asserted by +`TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent` at the unit level. + +## Alternatives + +1. **Per-Setting provenance recorded in the Secret.** Store, beside each value, whether it came from an + environment declaration, an administrator, or a built-in default, and let the operator update a default only + when the recorded provenance says it was never claimed. Rejected: it requires the previous default to be + recoverable across versions, which the Secret cannot supply once the default changes, and it makes a decision + the operator cannot see. The tracking Setting states the same intent in a form an administrator can read with + `kubectl get settings`. The rejected option costs roughly the same to build and is far harder to audit. +2. **Seed only blank stored values, the narrow fix.** Write the seed when a stored value is blank and the seed is + not. Rejected: it fixes the reported symptom, but it cannot distinguish a deliberate blank from an unconfigured + Setting, and it leaves image defaults stranded across an operator upgrade. It has no answer to goal 2. +3. **Move the delivery-mode seed into the worker, conditioned on `--model-manager-service-account`.** Rejected, + and not because it was not tried: the flag defaults to a non-empty name in + `pkg/system/control.go`, so the condition is always true and the worker would seed `Node` on every install, + including `modelManager.enabled=false`. It also contradicts the comment in + `pkg/worker/settings/model_store.go` that the Setting is never inferred from plugin installation, and + `settings.Initialize` receives only the client, so no better condition exists inside the worker without a new + flag. The chart's `modelManager.enabled` is the only signal, so the chart keeps the declaration and the two + documents that promise otherwise are corrected instead. +4. **Track every Setting with a non-blank default, not only images.** Broaden `settings-default-tracking`'s + default to all 22 such Settings. Rejected for this spec: the reported problem was image Settings, and a broader + default makes behavioral switches such as `instance-privileged-allowed` revert on upgrade for anyone who + enabled them. Administrators can add any name to the list. +5. **Ship `settings-default-tracking` empty and let administrators opt in.** This is the option that would have + kept every pre-existing image pin across the first upgrade. Rejected by decision: a tracking list nobody + populates does not solve the problem that motivated it, which was an operator upgrade stranding a cluster on + a stale image default. The cost is recorded as the first risk: a pin made before this spec is replaced by the + operator default on the first start after the upgrade, and the log line announces it. + +## Open Questions + +None. The revised Story 4 was confirmed by the user on 2026-10-19, so `settings-default-tracking` +ships with the six image names and the first upgrade that carries this spec replaces an image pin made +before it. That one-time cost is the first entry under Risks. From a3b49d9d0ef8524b1fd7d41a55a27c5d10c5056d Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 17:49:37 +0800 Subject: [PATCH 02/12] feat(setting): carry the environment declaration source on Setting - Initializer reports the value, the variable it read, and whether that variable was present, so a blank declaration stays distinguishable from no declaration - Settings.New fixes both on the Setting at registration; InitializeFrom leaves them empty and InitializeFromEnv keeps the derived variable name - four direct initializer calls in pkg/worker/settings/value_test.go bind only the value they assert, so each expectation and its message stand Task 1 of settings-env-sync-and-default-tracking. Signed-off-by: thxCode --- pkg/setting/helper.go | 27 ++++--- pkg/setting/types.go | 13 +++- pkg/setting/types_test.go | 76 +++++++++++++++++++ pkg/worker/settings/value_test.go | 12 ++- ...-settings-env-sync-and-default-tracking.md | 2 +- 5 files changed, 111 insertions(+), 19 deletions(-) diff --git a/pkg/setting/helper.go b/pkg/setting/helper.go index 5007d6723..928595d4b 100644 --- a/pkg/setting/helper.go +++ b/pkg/setting/helper.go @@ -17,30 +17,35 @@ import ( "gpustack.ai/gpustack/pkg/utils/stringx" ) -// Initializer defines the stereotype for initializing the default value. -type Initializer func(name string) (value string) +// Initializer defines the stereotype for initializing a setting's value and environment metadata. +// declared reports whether envName was present, including when its value was empty. +type Initializer func(name string) (value, envName string, declared bool) // InitializeFromSpecifiedEnv searches the variable of the given environment name, -// returns the default value if not found the specified environment. +// returns the default value if not found, and reports the variable's registration-time presence. func InitializeFromSpecifiedEnv(envName string, defValue ...string) Initializer { - return func(_ string) string { - return osx.ExpandEnv(envName, defValue...) + return func(_ string) (string, string, bool) { + value := osx.ExpandEnv(envName, defValue...) + _, declared := osx.LookupEnv(envName) + return value, envName, declared } } // InitializeFromEnv searches the variable of the `GPUSTACK_${UpperSnakeCase_of_SettingName}`, -// returns the default value if not found the environment. +// returns the default value if not found, and reports the variable's registration-time presence. func InitializeFromEnv(defValue ...string) Initializer { - return func(name string) string { + return func(name string) (string, string, bool) { envName := "GPUSTACK_" + stringx.UnderscoreUpper(name) - return osx.ExpandEnv(envName, defValue...) + value := osx.ExpandEnv(envName, defValue...) + _, declared := osx.LookupEnv(envName) + return value, envName, declared } } -// InitializeFrom initializes with the given value. +// InitializeFrom initializes with the given value without an environment source. func InitializeFrom(value string) Initializer { - return func(_ string) string { - return value + return func(_ string) (string, string, bool) { + return value, "", false } } diff --git a/pkg/setting/types.go b/pkg/setting/types.go index 025e59d99..678a5e339 100644 --- a/pkg/setting/types.go +++ b/pkg/setting/types.go @@ -31,8 +31,12 @@ type ( name string description string defVal string - admit Admission - props Prop + // envName is the environment variable read during registration, or empty when none. + envName string + // declared reports whether envName was present during registration, including an empty value. + declared bool + admit Admission + props Prop } // IndexFunc is a function type that takes a setting name and returns the corresponding setting and a boolean indicating whether the setting exists. @@ -254,9 +258,12 @@ func (h Settings) New( name, description string, props Prop, init Initializer, admits ...Admission, ) Setting { + defVal, envName, declared := init(name) s := Setting{ name: name, - defVal: init(name), + defVal: defVal, + envName: envName, + declared: declared, admit: AdmitWith(admits...), description: description, props: props, diff --git a/pkg/setting/types_test.go b/pkg/setting/types_test.go index af087a86a..33e122900 100644 --- a/pkg/setting/types_test.go +++ b/pkg/setting/types_test.go @@ -3,6 +3,7 @@ package setting import ( "context" "errors" + "os" "testing" "github.com/stretchr/testify/assert" @@ -14,6 +15,81 @@ import ( "gpustack.ai/gpustack/pkg/system" ) +func TestInitializerDeclarationSource(t *testing.T) { + const ( + derivedName = "GPUSTACK_TEST_SETTING" + specifiedName = "TEST_SPECIFIED_SETTING" + ) + + type testCase struct { + name string + envName string + envValue *string + initializer func() Initializer + wantValue string + wantEnvName string + wantDeclared bool + } + + declaredValue := "configured" + emptyValue := "" + cases := []testCase{ + { + name: "environment declared", + envName: derivedName, + envValue: &declaredValue, + initializer: func() Initializer { return InitializeFromEnv("fallback") }, + wantValue: declaredValue, + wantEnvName: derivedName, + wantDeclared: true, + }, + { + name: "environment unset", + envName: derivedName, + initializer: func() Initializer { return InitializeFromEnv("fallback") }, + wantValue: "fallback", + wantEnvName: derivedName, + }, + { + name: "environment declared empty", + envName: derivedName, + envValue: &emptyValue, + initializer: func() Initializer { return InitializeFromEnv("fallback") }, + wantEnvName: derivedName, + wantDeclared: true, + }, + { + name: "specified environment declared", + envName: specifiedName, + envValue: &declaredValue, + initializer: func() Initializer { return InitializeFromSpecifiedEnv(specifiedName, "fallback") }, + wantValue: declaredValue, + wantEnvName: specifiedName, + wantDeclared: true, + }, + { + name: "fixed value", + initializer: func() Initializer { return InitializeFrom("fixed") }, + wantValue: "fixed", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if tc.envValue == nil { + require.NoError(t, os.Unsetenv(tc.envName)) + } else { + t.Setenv(tc.envName, *tc.envValue) + } + + s := Settings{}.New("test-setting", "", PropPrivate, tc.initializer()) + assert.Equal(t, tc.wantValue, s.DefaultValue()) + assert.Equal(t, tc.wantEnvName, s.envName) + assert.Equal(t, tc.wantDeclared, s.declared) + }) + } +} + // TestSettingValueBool pins what a bool Setting reads as. A read that fails, or finds no stored // value, yields the setting's default together with the error, so ShouldValueBool, which drops the // error, still returns the default: a setting that defaults to on must not turn off because one read diff --git a/pkg/worker/settings/value_test.go b/pkg/worker/settings/value_test.go index 1310dfdc6..ac84cb67a 100644 --- a/pkg/worker/settings/value_test.go +++ b/pkg/worker/settings/value_test.go @@ -58,7 +58,8 @@ func TestInstanceHostAccessSettings(t *testing.T) { assert.True(t, c.s.Editable(), "must stay editable for runtime adjustment") t.Setenv(c.wantEnv, "true") - assert.Equal(t, "true", setting.InitializeFromEnv("false")(c.s.Name()), + value, _, _ := setting.InitializeFromEnv("false")(c.s.Name()) + assert.Equal(t, "true", value, "env %s must override the default", c.wantEnv) }) } @@ -97,7 +98,8 @@ func TestModelDeploymentImageSettings(t *testing.T) { assert.True(t, c.s.Editable(), "must stay editable so a cluster can redirect or pin it") t.Setenv(c.wantEnv, "registry.example.com/override:v0") - assert.Equal(t, "registry.example.com/override:v0", setting.InitializeFromEnv(c.wantDefault)(c.s.Name()), + value, _, _ := setting.InitializeFromEnv(c.wantDefault)(c.s.Name()) + assert.Equal(t, "registry.example.com/override:v0", value, "env %s must override the default", c.wantEnv) }) } @@ -114,7 +116,8 @@ func TestModelDeploymentTCPTWReuseSetting(t *testing.T) { assert.True(t, s.Editable(), "must stay editable for runtime adjustment") t.Setenv("GPUSTACK_MODEL_DEPLOYMENT_TCP_TW_REUSE", "true") - assert.Equal(t, "true", setting.InitializeFromEnv("false")(s.Name()), + value, _, _ := setting.InitializeFromEnv("false")(s.Name()) + assert.Equal(t, "true", value, "env GPUSTACK_MODEL_DEPLOYMENT_TCP_TW_REUSE must override the default") } @@ -136,7 +139,8 @@ func TestInstanceTypeManagementSettingsEnvMapping(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { t.Setenv(c.env, c.set) - assert.Equal(t, c.want, setting.InitializeFromEnv(c.def)(c.name), + value, _, _ := setting.InitializeFromEnv(c.def)(c.name) + assert.Equal(t, c.want, value, "env %s must override the default", c.env) }) } diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index abb3c1182..9d8febe33 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -368,7 +368,7 @@ T1 ──▶ T2 ──▶ T3 ──┐ T4 ───────────────┘ ``` -- [ ] **T1 · Carry the declaration source on `Setting`** +- [x] **T1 · Carry the declaration source on `Setting`** Blocked by: None Owns: `pkg/setting/helper.go`, `pkg/setting/types.go`, `pkg/setting/types_test.go`, `pkg/worker/settings/value_test.go` From 9eddc4c5aeaa7b56c6540c4211939c0343208f35 Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 18:12:26 +0800 Subject: [PATCH 03/12] feat(setting): apply the sync predicate on every worker start - a declared environment variable and a tracked Setting each own their value, so a variable added after the first deploy is no longer ignored - absent keys are still backfilled first, so a blank-default Setting still appears and a Secret predating the tracking key gets it on the same pass - the tracking list follows the five-step parse, dropping a blank segment, a duplicate, the tracking name and any unregistered name, each logged - every change logs one line carrying the Setting, both values, the rule and the variable - settings-default-tracking is an exported constant, so the task that registers it cannot drift from the one that reads it - first test of Settings.Initialize; the declared-override row fails when the rules are removed Task 2 of settings-env-sync-and-default-tracking. Signed-off-by: thxCode --- pkg/setting/kuberes.go | 100 +++++- pkg/setting/kuberes_test.go | 337 ++++++++++++++++++ ...-settings-env-sync-and-default-tracking.md | 34 +- 3 files changed, 464 insertions(+), 7 deletions(-) create mode 100644 pkg/setting/kuberes_test.go diff --git a/pkg/setting/kuberes.go b/pkg/setting/kuberes.go index d7758ca6b..9960fcb2b 100644 --- a/pkg/setting/kuberes.go +++ b/pkg/setting/kuberes.go @@ -2,10 +2,13 @@ package setting import ( "context" + "strings" "github.com/google/uuid" core "k8s.io/api/core/v1" meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" + klog "k8s.io/klog/v2" "gpustack.ai/gpustack/pkg/kubeclients/kubernetes" "gpustack.ai/gpustack/pkg/kubeclientset" @@ -22,6 +25,14 @@ var ( DelegatedSecretName = "gpustack-settings" ) +// DefaultTrackingName is the name of the Setting whose comma-separated value lists the Settings +// that follow the operator's built-in default instead of an administrator's stored value. +// +// The align pass resolves it by name in the settings registry, and the registry registers it +// under this same name. The two must agree: a Setting registered under any other name would be +// matched by neither the read nor the list, and no Setting would be tracked at all. +const DefaultTrackingName = "settings-default-tracking" + // Initialize initializes Kubernetes resources for settings. // // Initialize creates the delegated Kubernetes Secret for settings. @@ -63,13 +74,32 @@ func (h Settings) Initialize(ctx context.Context, cli kubernetes.Interface) erro systemmeta.NoteResource(aSec, eResType, eNotes) skip = false } - // Align data. + // Align data. An absent key is backfilled first, before any rule is evaluated, so a + // newly registered setting with a blank default still appears and a Secret predating + // the tracking setting gets its key on this same pass. for k := range eSec.Data { if _, ok := aSec.Data[k]; !ok { aSec.Data[k] = eSec.Data[k] skip = false } } + tracking := parseTrackedSettings(h, h.trackingValue(aSec)) + for name, s := range h { + stored := string(aSec.Data[name]) + desired, rule := s.desiredValue(tracking) + if desired == "" || desired == stored { + // These rules never write a blank value, and an equal value is not an + // override, so a pass that changes nothing logs nothing. + continue + } + aSec.Data[name] = []byte(desired) + skip = false + pairs := []any{"setting", name, "stored", stored, "value", desired, "rule", rule} + if s.envName != "" { + pairs = append(pairs, "envName", s.envName) + } + klog.InfoS("setting overridden on start", pairs...) + } return aSec, skip, nil } @@ -82,3 +112,71 @@ func (h Settings) Initialize(ctx context.Context, cli kubernetes.Interface) erro return nil } + +// trackingValue returns the tracking list, read from the live Secret and falling back to the +// registered Setting's built-in default when the Secret carries no such key. Reading the Secret +// alone would defer the built-in list to a later start on a Secret that predates the setting. +// +// The setting is registered by the release that introduces it. Until the registry carries it the +// fallback is empty, so nothing is tracked, which is the correct intermediate state rather than a +// failure to report. +func (h Settings) trackingValue(aSec *core.Secret) string { + if raw, ok := aSec.Data[DefaultTrackingName]; ok { + return string(raw) + } + if s, ok := h.Index(DefaultTrackingName); ok { + return s.defVal + } + return "" +} + +// parseTrackedSettings returns the registered settings the raw tracking list names. It splits on a +// comma, trims each segment, drops a segment left empty, a duplicate, the tracking setting's own +// name, and any name the registry does not carry, so a stored value cannot track a setting that +// does not exist. Every dropped segment is logged once at the fourth verbosity with its reason. +func parseTrackedSettings(h Settings, raw string) sets.Set[string] { + tracked := sets.New[string]() + for _, segment := range strings.Split(raw, ",") { + name := strings.TrimSpace(segment) + reason := "" + switch { + case name == "": + reason = "blank" + case tracked.Has(name): + reason = "duplicate" + case name == DefaultTrackingName: + reason = "self-reference" + default: + if _, ok := h.Index(name); !ok { + reason = "not registered" + } + } + if reason != "" { + klog.V(4).InfoS("tracked setting segment dropped", + "setting", DefaultTrackingName, "segment", segment, "reason", reason) + continue + } + tracked.Insert(name) + } + return tracked +} + +// desiredValue returns the value the operator owns for this Setting and the rule that claimed it. +// An empty rule means the operator owns nothing here and the stored value is the administrator's. +// +// The operator writes a Setting when its seed is non-blank and the environment declares it or the +// tracking list names it. A variable present with an empty value yields a blank seed, because a +// built-in default is substituted only for an absent variable, so a blank declaration claims +// nothing even for a Setting that has a default. +func (s Setting) desiredValue(tracked sets.Set[string]) (value, rule string) { + if isBlank(s.defVal) { + return "", "" + } + if s.declared { + return s.defVal, "env-declared" + } + if tracked.Has(s.name) { + return s.defVal, "default-tracked" + } + return "", "" +} diff --git a/pkg/setting/kuberes_test.go b/pkg/setting/kuberes_test.go new file mode 100644 index 000000000..dcd3d344a --- /dev/null +++ b/pkg/setting/kuberes_test.go @@ -0,0 +1,337 @@ +package setting + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + authz "k8s.io/api/authorization/v1" + core "k8s.io/api/core/v1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + k8stesting "k8s.io/client-go/testing" + + kubefake "gpustack.ai/gpustack/pkg/kubeclients/kubernetes/fake" +) + +type ( + // caseSetting declares one Setting a case registers. A non-empty envName registers it + // through that environment variable, and such a case must also declare that variable, + // because a Setting's declaration is fixed when it is registered. + caseSetting struct { + name string + defVal string + envName string + } + + // initializeCase is one row: the registry it builds, the environment it declares, the + // Secret that already exists and the values it must hold afterwards. A nil stored map + // means no Secret exists, so Initialize takes the creation path; a non-nil map means the + // Secret exists holding exactly those keys, and a key the registry also carries is + // already present rather than backfilled. + initializeCase struct { + name string + settings []caseSetting + env map[string]string + stored map[string]string + want map[string]string + } +) + +// runInitializeCases runs each row through one execution loop: declare the environment, build +// the registry, run Initialize against a fake clientset, then assert on the Secret the clientset +// holds. It must not be called from a parallel test, because a row declares its environment. +func runInitializeCases(t *testing.T, cases []initializeCase) { + t.Helper() + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + for name, value := range c.env { + t.Setenv(name, value) + } + cli := kubefake.NewSimpleClientset(delegatedSecretObjects(c.stored)...) + allowSelfSubjectAccessReviews(cli) + + require.NoError(t, registerCaseSettings(c.settings). + Initialize(context.Background(), cli)) + + sec, err := cli.CoreV1().Secrets(DelegatedSecretNamespace). + Get(context.Background(), DelegatedSecretName, meta.GetOptions{}) + require.NoError(t, err) + for name, want := range c.want { + got, ok := sec.Data[name] + assert.True(t, ok, "setting %s is present", name) + assert.Equal(t, want, string(got), "setting %s", name) + } + }) + } +} + +// registerCaseSettings builds the registry a row declares. +func registerCaseSettings(declared []caseSetting) Settings { + h := Settings{} + for _, s := range declared { + if s.envName != "" { + h.NewEditable(s.name, "A setting registered by a case.", + InitializeFromSpecifiedEnv(s.envName, s.defVal)) + continue + } + h.NewEditable(s.name, "A setting registered by a case.", InitializeFrom(s.defVal)) + } + return h +} + +// delegatedSecretObjects returns the objects a fake clientset starts with, so that a row either +// finds no Secret at all or finds one holding exactly the given keys. +func delegatedSecretObjects(stored map[string]string) []runtime.Object { + if stored == nil { + return nil + } + data := map[string][]byte{} + for name, value := range stored { + data[name] = []byte(value) + } + return []runtime.Object{&core.Secret{ + ObjectMeta: meta.ObjectMeta{ + Namespace: DelegatedSecretNamespace, + Name: DelegatedSecretName, + }, + Data: data, + }} +} + +// allowSelfSubjectAccessReviews makes the permission check Initialize runs first pass, which a +// fake client denies by default. +func allowSelfSubjectAccessReviews(cli *kubefake.Clientset) { + cli.PrependReactor("create", "selfsubjectaccessreviews", + func(action k8stesting.Action) (bool, runtime.Object, error) { + sar, ok := action.(k8stesting.CreateAction).GetObject().(*authz.SelfSubjectAccessReview) + if !ok { + return false, nil, nil + } + sar = sar.DeepCopy() + sar.Status.Allowed = true + return true, sar, nil + }) +} + +const ( + trackedImage = "kv-cache-backend-image" + blankImage = "container-registry" +) + +// TestSettingsInitialize_DeclaredSettingOverridesStoredValue pins that a variable declared in the +// environment is a standing desired state rather than a one-time default. Removing the rules that +// claim a declared value leaves this row alone on its stored value. +func TestSettingsInitialize_DeclaredSettingOverridesStoredValue(t *testing.T) { + const ( + name = "instance-access-static-address" + envName = "GPUSTACK_INSTANCE_ACCESS_STATIC_ADDRESS" + ) + runInitializeCases(t, []initializeCase{ + { + name: "a declared variable overrides the stored value of an untracked setting", + settings: []caseSetting{{name: name, envName: envName}}, + env: map[string]string{envName: "new.example"}, + stored: map[string]string{name: "old.example"}, + want: map[string]string{name: "new.example"}, + }, + }) +} + +// TestSettingsInitialize_TrackedSettingFollowsBuiltInDefault pins that a Setting the tracking list +// names follows the operator's default, so a new default reaches a cluster on the next start. +func TestSettingsInitialize_TrackedSettingFollowsBuiltInDefault(t *testing.T) { + runInitializeCases(t, []initializeCase{ + { + name: "a tracked setting follows the built-in default", + settings: []caseSetting{ + {name: trackedImage, defVal: "operator-default/image:v1"}, + {name: DefaultTrackingName, defVal: trackedImage}, + }, + stored: map[string]string{trackedImage: "my.pin/image:1.0", DefaultTrackingName: trackedImage}, + want: map[string]string{trackedImage: "operator-default/image:v1", DefaultTrackingName: trackedImage}, + }, + }) +} + +// TestSettingsInitialize_UntrackedStoredValueIsLeftAlone pins that a value the operator owns +// nothing over stays the administrator's, a deliberate blank included. +func TestSettingsInitialize_UntrackedStoredValueIsLeftAlone(t *testing.T) { + runInitializeCases(t, []initializeCase{ + { + name: "a non-blank administrator value is kept", + settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1"}}, + stored: map[string]string{trackedImage: "my.pin/image:1.0"}, + want: map[string]string{trackedImage: "my.pin/image:1.0"}, + }, + { + name: "a blank administrator value is kept", + settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1"}}, + stored: map[string]string{trackedImage: ""}, + want: map[string]string{trackedImage: ""}, + }, + }) +} + +// TestSettingsInitialize_BackfillsAbsentKeysIncludingBlanks pins that the align pass still adds +// every registered key the Secret lacks, so a newly registered setting with a blank default +// appears instead of being missing. +func TestSettingsInitialize_BackfillsAbsentKeysIncludingBlanks(t *testing.T) { + runInitializeCases(t, []initializeCase{ + { + name: "a blank default is a key the Secret is given", + settings: []caseSetting{ + {name: trackedImage, defVal: "operator-default/image:v1"}, + {name: blankImage, defVal: ""}, + }, + stored: map[string]string{"another-setting": "kept"}, + want: map[string]string{ + trackedImage: "operator-default/image:v1", + blankImage: "", + "another-setting": "kept", + }, + }, + }) +} + +// TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent pins that the built-in tracking +// list applies on the same pass that backfills its key, so a Secret predating the setting does +// not defer the transition to a later start. +func TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent(t *testing.T) { + runInitializeCases(t, []initializeCase{ + { + name: "the built-in list applies when the Secret has no tracking key", + settings: []caseSetting{ + {name: trackedImage, defVal: "operator-default/image:v1"}, + {name: DefaultTrackingName, defVal: trackedImage}, + }, + stored: map[string]string{trackedImage: "my.pin/image:1.0"}, + want: map[string]string{ + trackedImage: "operator-default/image:v1", + DefaultTrackingName: trackedImage, + }, + }, + }) +} + +// TestSettingsInitialize_EqualValueIsNotAnOverride pins that a stored value equal to what the +// operator would write is not an override, so a quiet start logs nothing. +func TestSettingsInitialize_EqualValueIsNotAnOverride(t *testing.T) { + const envName = "GPUSTACK_TEST_IMAGE" + runInitializeCases(t, []initializeCase{ + { + name: "a declared variable equal to the stored value leaves it alone", + settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1", envName: envName}}, + env: map[string]string{envName: "operator-default/image:v1"}, + stored: map[string]string{trackedImage: "operator-default/image:v1"}, + want: map[string]string{trackedImage: "operator-default/image:v1"}, + }, + { + name: "a tracked setting already holding its default is left alone", + settings: []caseSetting{ + {name: trackedImage, defVal: "operator-default/image:v1"}, + {name: DefaultTrackingName, defVal: trackedImage}, + }, + stored: map[string]string{trackedImage: "operator-default/image:v1", DefaultTrackingName: trackedImage}, + want: map[string]string{trackedImage: "operator-default/image:v1"}, + }, + }) +} + +// TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration pins that a variable present with +// an empty value claims nothing, even for a Setting that has a built-in default to fall back on: +// a default is substituted only for an absent variable, so the seed here is blank. +func TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration(t *testing.T) { + const envName = "GPUSTACK_TEST_IMAGE" + runInitializeCases(t, []initializeCase{ + { + name: "an empty declaration leaves an untracked setting alone", + settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1", envName: envName}}, + env: map[string]string{envName: ""}, + stored: map[string]string{trackedImage: "my.pin/image:1.0"}, + want: map[string]string{trackedImage: "my.pin/image:1.0"}, + }, + }) +} + +// TestSettingsInitialize_CreationSeedsEverySetting pins that a cluster with no delegated Secret +// gets one carrying every registered key, blank defaults included. +func TestSettingsInitialize_CreationSeedsEverySetting(t *testing.T) { + runInitializeCases(t, []initializeCase{ + { + name: "an absent Secret is created carrying every registered key", + settings: []caseSetting{ + {name: trackedImage, defVal: "operator-default/image:v1"}, + {name: blankImage, defVal: ""}, + }, + want: map[string]string{ + trackedImage: "operator-default/image:v1", + blankImage: "", + }, + }, + }) +} + +// TestParseTrackedSettings pins the normalization a stored tracking value goes through, so a value +// an administrator writes cannot silently change which settings are tracked. +func TestParseTrackedSettings(t *testing.T) { + const ( + alpha = "alpha-image" + beta = "beta-image" + ) + h := Settings{} + h.NewEditable(alpha, "A setting registered by a case.", InitializeFrom("a")) + h.NewEditable(beta, "A setting registered by a case.", InitializeFrom("b")) + h.NewEditable(DefaultTrackingName, "A setting registered by a case.", InitializeFrom(alpha)) + + cases := []struct { + name string + raw string + want []string + }{ + { + name: "surrounding whitespace is trimmed", + raw: " " + alpha + " , " + beta + " ", + want: []string{alpha, beta}, + }, + { + name: "a segment empty after trimming is dropped", + raw: alpha + ", , " + beta, + want: []string{alpha, beta}, + }, + { + name: "a duplicate is dropped", + raw: alpha + ", " + alpha, + want: []string{alpha}, + }, + { + name: "the tracking setting's own name is dropped", + raw: DefaultTrackingName, + want: []string{}, + }, + { + name: "a name the registry does not carry is dropped", + raw: alpha + ",retired-image", + want: []string{alpha}, + }, + { + name: "an empty value tracks nothing", + raw: "", + want: []string{}, + }, + { + name: "every step applies together", + raw: " " + alpha + " , " + DefaultTrackingName + " , retired-image , " + alpha + " , ", + want: []string{alpha}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + assert.Equal(t, c.want, sets.List(parseTrackedSettings(h, c.raw))) + }) + } +} diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index 9d8febe33..95b4789b8 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -137,6 +137,11 @@ model-deployment-router-proxy-image,model-deployment-routing-sidecar-image, model-prefetch-warmup-image ``` +The name is a protocol constant, exported from `pkg/setting`, because the align pass reads it and the task that +registers the Setting writes it. Two copies of the literal that drift apart make the tracking list match nothing, +which is the silent failure this specification exists to eliminate, and no test covers the join between the reader +and the writer. One exported name is the whole defense. + Its own name may not appear in the list. An entry naming itself is ignored and logged, because a Setting that tracked itself would reconcile its own membership. @@ -318,7 +323,7 @@ alignFn := func(aSec *core.Secret) (_ *core.Secret, skip bool, err error) { // The tracking list is read from the live Secret, falling back to the built-in default, so a // Secret that predates this Setting gets the default on the same start that backfills it. - tracking := parseTrackedSettings(trackingValue(aSec)) + tracking := parseTrackedSettings(h, h.trackingValue(aSec)) for name, s := range h { stored := string(aSec.Data[name]) @@ -342,7 +347,7 @@ when its seed is non-blank and the environment declares it or the tracking list ```go // desiredValue returns the value the operator owns for this Setting and the rule that claimed it. // An empty rule means the operator owns nothing here, and the stored value is the administrator's. -func (s Setting) desiredValue(tracked sets.Set[string]) (value string, rule string) { +func (s Setting) desiredValue(tracked sets.Set[string]) (value, rule string) { if isBlank(s.defVal) { return "", "" } @@ -360,6 +365,21 @@ Conventions to keep: use `klog.InfoS` with key-value pairs, as in `pkg/kubeapp/l than testing for an empty string; keep one Setting name per log line so a grep finds every override; do not introduce a second meaning of "blank" anywhere in this path. +Two placement notes, because the sketch above is illustrative and the task boundaries are not negotiable. + +`desiredValue` is sketched as a method on `Setting`, which reads as though it belongs in `types.go` beside `Value` +and `Configure`. The task that applies the sync predicate owns `pkg/setting/kuberes.go` and not `types.go`, so the +method lands in `kuberes.go` and `Setting`'s methods end up split across two files. Ownership forced it, and moving +it later is a separate decision rather than a quiet cleanup. + +`parseTrackedSettings` is sketched with one argument. Its own steps four and five cannot be evaluated without the +registry — step four drops the tracking Setting's own name and step five drops a name no registered Setting +carries — so it takes the registry. The acceptance is the contract and the sketch is a shape, not a signature. + +`desiredValue` returns `(value, rule string)`, not the `(value string, rule string)` an earlier draft of this sketch +wrote. `gofumpt` collapses a repeated result type, so the earlier spelling does not survive `make lint`. The +signature and its meaning are unchanged, and no test or contract depends on the spelling. + ### Implementation Plan ``` @@ -382,15 +402,17 @@ T4 ───────────────┘ behavior; the whole tree compiles. Verify: `GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race ./pkg/setting/... && make lint` -- [ ] **T2 · Apply the sync predicate in the align pass, with the regression guard** +- [x] **T2 · Apply the sync predicate in the align pass, with the regression guard** Blocked by: T1 Owns: `pkg/setting/kuberes.go`, `pkg/setting/kuberes_test.go` Gate: review Acceptance: absent keys are backfilled first, so a blank-default Setting is still added; the tracking list is read from the live Secret and falls back to the built-in default when the key is absent, and is parsed - by the five-step contract in Core Features; `desiredValue` returns a value only for a non-blank seed that - is either declared or tracked; every change to a stored value logs one `klog.InfoS` line carrying the - Setting, both values and the rule; `TestSettingsInitialize` covers the table below and passes. + by the five-step contract in Core Features, which takes the registry because steps four and five cannot be + evaluated without it; the name is an exported constant so the task registering the Setting cannot drift from + the one reading it; `desiredValue` returns a value only for a non-blank seed that is either declared or + tracked; every change to a stored value logs one `klog.InfoS` line carrying the Setting, both values, the + rule and the variable; `TestSettingsInitialize` covers the table below and passes. Verify: `GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race -v ./pkg/setting -run TestSettingsInitialize && make lint` - [ ] **T3 · Register `settings-default-tracking`** From f8e5d08a4068d4edc2f8a7599bc52dc0921fd55b Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 18:25:37 +0800 Subject: [PATCH 04/12] feat(worker): register the settings-default-tracking Setting - the list defaults to the six image Settings, written out longhand so the registration and the registry cannot disagree, with a test that pins every name resolving to a registered Setting - initialized from a fixed value, so no environment variable can claim the list and the operator cannot take an administrator's choice over - blank is admitted, because clearing the list is the only way to keep an image pin across an upgrade - an admission refuses a list naming the tracking Setting itself, comparing trimmed segments exactly as the align pass parses them Task 3 of settings-env-sync-and-default-tracking. Signed-off-by: thxCode --- pkg/worker/settings/value.go | 47 ++++ pkg/worker/settings/value_test.go | 201 ++++++++++++++++++ ...-settings-env-sync-and-default-tracking.md | 47 +++- 3 files changed, 287 insertions(+), 8 deletions(-) diff --git a/pkg/worker/settings/value.go b/pkg/worker/settings/value.go index ba02e7e2f..d016d5f43 100644 --- a/pkg/worker/settings/value.go +++ b/pkg/worker/settings/value.go @@ -2,6 +2,8 @@ package settings import ( "context" + "fmt" + "strings" "time" "gpustack.ai/gpustack/pkg/modelartifact" @@ -54,6 +56,36 @@ var ( setting.AllowBlank(), ) + // DefaultTracking names the Settings the operator keeps on its own built-in default, so a default + // shipped in a later release reaches a cluster that never chose a value of its own. It is written + // out longhand because a name derived from the image Setting vars could not disagree with the + // registry, and a disagreement is what the resolve test pins: the align pass drops a name it + // cannot resolve, at a verbosity nobody reads. + // + // It is the one Setting here initialized from a fixed value rather than from the environment. + // InitializeFrom reports no declaration, so neither rule can claim it and no environment variable + // can take the list over. That is deliberate even though every other Setting reads its seed from + // one: which Settings the operator owns is the administrator's decision, and the chart renders no + // variable for this Setting. + // + // Clearing the list is the only way to keep a stored image across an operator upgrade, so a blank + // value is admitted. + DefaultTracking = settings.NewEditable( + setting.DefaultTrackingName, + "Indicates the comma-separated Settings that follow the operator's built-in default on every "+ + "worker start. Clear it to keep the value stored for a Setting, which is the only way to "+ + "keep an image pin across an operator upgrade.", + setting.InitializeFrom( + "instance-ssh-server-image,"+ + "kv-cache-backend-image,"+ + "model-deployment-router-image,"+ + "model-deployment-router-proxy-image,"+ + "model-deployment-routing-sidecar-image,"+ + "model-prefetch-warmup-image"), + setting.AllowBlank(), + admitNoSelfTracking, + ) + // Instance. // InstanceGeneralResourcesOvercommit indicates to overcommit instance general resources or not, @@ -459,3 +491,18 @@ var ( setting.AllowBool(), ) ) + +// admitNoSelfTracking refuses a tracking list naming the tracking Setting itself, which would make +// that Setting reconcile its own membership. It compares trimmed comma-separated segments exactly as +// the align pass parses the list, so a padded or buried name is the same case here as it is there. +// It refuses at the write rather than letting the align pass drop the segment, because a write that +// appears to succeed and then does nothing is the silent failure this list exists to prevent. +func admitNoSelfTracking(_ context.Context, _, newVal string) error { + for _, segment := range strings.Split(newVal, ",") { + if strings.TrimSpace(segment) == setting.DefaultTrackingName { + return fmt.Errorf("setting %s cannot name itself, remove it to track the other Settings", + setting.DefaultTrackingName) + } + } + return nil +} diff --git a/pkg/worker/settings/value_test.go b/pkg/worker/settings/value_test.go index ac84cb67a..4f401146d 100644 --- a/pkg/worker/settings/value_test.go +++ b/pkg/worker/settings/value_test.go @@ -1,11 +1,22 @@ package settings import ( + "context" + "os" + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + authz "k8s.io/api/authorization/v1" + core "k8s.io/api/core/v1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + k8stesting "k8s.io/client-go/testing" + kubefake "gpustack.ai/gpustack/pkg/kubeclients/kubernetes/fake" "gpustack.ai/gpustack/pkg/setting" + "gpustack.ai/gpustack/pkg/utils/stringx" ) // TestInstanceTypeManagementSettings pins the three runtime-adjustable switches @@ -166,3 +177,193 @@ func TestInstancePersistentVolumePlacementSetting(t *testing.T) { assert.Equal(t, "true", s.DefaultValue(), "boot default value") assert.True(t, s.Editable(), "must stay editable for runtime adjustment") } + +// trackedImageSettings is the built-in tracking list, written out longhand rather than joined from +// the image Setting vars. A name derived from those vars could not disagree with the registry, and a +// disagreement is what the resolve test below exists to catch: the align pass drops a name it cannot +// resolve, at a verbosity nobody reads. +var trackedImageSettings = []string{ + "instance-ssh-server-image", + "kv-cache-backend-image", + "model-deployment-router-image", + "model-deployment-router-proxy-image", + "model-deployment-routing-sidecar-image", + "model-prefetch-warmup-image", +} + +// TestSettingsDefaultTracking_DefaultValue pins the built-in list: the six image Settings, in this +// order. The order is the documentation order an administrator reads, so a change to it is a change +// to what an upgrade writes back. +func TestSettingsDefaultTracking_DefaultValue(t *testing.T) { + assert.Equal(t, trackedImageSettings, strings.Split(DefaultTracking.DefaultValue(), ","), + "the built-in list is the six image Settings, in this order") +} + +// TestSettingsDefaultTracking_IsRegistered pins that the registry carries this Setting under the name +// the align pass resolves. Two copies of that literal that drift leave the tracking list matching +// nothing at all, silently, and no other test would see it. +func TestSettingsDefaultTracking_IsRegistered(t *testing.T) { + assert.Equal(t, setting.DefaultTrackingName, DefaultTracking.Name(), + "the registration and the reader must share one name") + + s, ok := settings.Index(setting.DefaultTrackingName) + require.True(t, ok, "the align pass resolves the tracking list by name in this registry") + assert.True(t, s.Editable(), "an administrator clears the list through this Setting") +} + +// TestSettingsDefaultTracking_DefaultNamesResolve pins that every name in the built-in list resolves +// to a registered Setting, read from the registered default rather than from this file's own copy, so +// a typo in the registration fails here instead of being dropped at read time. +func TestSettingsDefaultTracking_DefaultNamesResolve(t *testing.T) { + for _, name := range strings.Split(DefaultTracking.DefaultValue(), ",") { + t.Run(name, func(t *testing.T) { + _, ok := settings.Index(name) + assert.True(t, ok, "a name the registry does not carry tracks nothing") + }) + } +} + +// TestSettingsDefaultTracking_RefusesItsOwnName pins that a list naming the tracking Setting itself is +// refused at the write, because a Setting that tracked itself would reconcile its own membership. +// The refusal compares trimmed segments, so padding and burial are the same case. +func TestSettingsDefaultTracking_RefusesItsOwnName(t *testing.T) { + cases := []struct { + name string + value string + wantErr bool + }{ + { + name: "its own name alone is refused", + value: setting.DefaultTrackingName, + wantErr: true, + }, + { + name: "its own name padded with whitespace is refused", + value: " " + trackedImageSettings[0] + " , " + setting.DefaultTrackingName + " ", + wantErr: true, + }, + { + name: "its own name buried among valid names is refused", + value: trackedImageSettings[0] + ", " + setting.DefaultTrackingName + ", " + trackedImageSettings[1], + wantErr: true, + }, + { + name: "a list that does not name itself is admitted", + value: strings.Join(trackedImageSettings, ","), + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := admitNoSelfTracking(context.Background(), "", c.value) + if !c.wantErr { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.ErrorContains(t, err, setting.DefaultTrackingName, + "the error must name the Setting an administrator has to fix") + }) + } +} + +// TestSettingsDefaultTracking_BlankListIsAllowed pins that a blank list is admitted. Clearing it is +// the only way an administrator stops tracking everything, so a refusal here would leave no way out +// of the rule the list applies. +// +// The chain is built here instead of run through the registered Setting, because Configure is the +// only path that runs it and it writes the delegated Secret through a process-wide loopback client +// another test in this package configures: driving it from here would make this case's coverage +// depend on that test running first, which the shuffled order of make test decides. What the +// blank value then does to the align pass is pinned by the cleared row of the test below. +func TestSettingsDefaultTracking_BlankListIsAllowed(t *testing.T) { + admit := setting.AdmitWith(setting.AllowBlank(), admitNoSelfTracking) + assert.NoError(t, admit(context.Background(), trackedImageSettings[0], ""), + "an administrator must be able to stop tracking everything") +} + +// TestSettingsDefaultTracking_FallsBackWhenKeyAbsent pins the seam between this registration and the +// align pass: on a Secret predating this Setting the key is absent, and the built-in list still +// applies, so a cluster reaches the new defaults on the upgrade rather than a start later. The +// second row clears the list and pins the other side of the same pass, so the first row cannot pass +// because the align pass rewrites those Settings whatever the list says. +func TestSettingsDefaultTracking_FallsBackWhenKeyAbsent(t *testing.T) { + const driftedImage = "pinned.example/image:v0" + + // A declaration claims a Setting ahead of the tracking list, so a variable set in the process + // environment would let both rows pass for the wrong reason. Their values are fixed when the + // package registers, which this test cannot change. + for _, name := range trackedImageSettings { + envName := "GPUSTACK_" + stringx.UnderscoreUpper(name) + if _, declared := os.LookupEnv(envName); declared { + t.Skipf("%s is declared in this process environment, so the env rule claims the six "+ + "ahead of the tracking list and this case cannot observe the fallback", envName) + } + } + + cleared := "" + cases := []struct { + name string + tracking *string // the stored tracking value; nil means the Secret predates the Setting + wantTracked bool + }{ + { + name: "the built-in list applies when the Secret has no tracking key", + wantTracked: true, + }, + { + name: "a list the administrator cleared tracks nothing", + tracking: &cleared, + wantTracked: false, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + data := map[string][]byte{} + for _, name := range trackedImageSettings { + data[name] = []byte(driftedImage) + } + if c.tracking != nil { + data[setting.DefaultTrackingName] = []byte(*c.tracking) + } + cli := kubefake.NewSimpleClientset(&core.Secret{ + ObjectMeta: meta.ObjectMeta{ + Namespace: setting.DelegatedSecretNamespace, + Name: setting.DelegatedSecretName, + }, + Data: data, + }) + allowSelfSubjectAccessReviews(cli) + + require.NoError(t, settings.Initialize(context.Background(), cli)) + + ctx := context.Background() + sec, err := cli.CoreV1().Secrets(setting.DelegatedSecretNamespace). + Get(ctx, setting.DelegatedSecretName, meta.GetOptions{}) + require.NoError(t, err) + for _, name := range trackedImageSettings { + want := driftedImage + if c.wantTracked { + s, ok := settings.Index(name) + require.True(t, ok) + want = s.DefaultValue() + } + assert.Equal(t, want, string(sec.Data[name]), "setting %s", name) + } + }) + } +} + +// allowSelfSubjectAccessReviews makes the permission check Initialize runs first pass, which a fake +// client denies by default. +func allowSelfSubjectAccessReviews(cli *kubefake.Clientset) { + cli.PrependReactor("create", "selfsubjectaccessreviews", + func(action k8stesting.Action) (bool, runtime.Object, error) { + sar, ok := action.(k8stesting.CreateAction).GetObject().(*authz.SelfSubjectAccessReview) + if !ok { + return false, nil, nil + } + sar = sar.DeepCopy() + sar.Status.Allowed = true + return true, sar, nil + }) +} diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index 95b4789b8..79d15c475 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -415,7 +415,7 @@ T4 ───────────────┘ rule and the variable; `TestSettingsInitialize` covers the table below and passes. Verify: `GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race -v ./pkg/setting -run TestSettingsInitialize && make lint` -- [ ] **T3 · Register `settings-default-tracking`** +- [x] **T3 · Register `settings-default-tracking`** Blocked by: T2 Owns: `pkg/worker/settings/value.go`, `pkg/worker/settings/value_test.go` Gate: none @@ -441,13 +441,32 @@ T4 ───────────────┘ Blocked by: T2, T3, T4 Owns: `docs/reference/settings.md`, `docs/README.md` Gate: none - Acceptance: the seeding paragraph states rules 1 to 4 with the blank, backfill and tracking-read - exceptions; the per-Setting table gains `settings-default-tracking` and marks every chart-rendered - variable as owned by the Deployment, which includes dropping the `model-artifact-delivery-mode` row's - "a seed never overrides a stored value"; the page states that a tracked image Setting reverts to the operator - default on every worker start and that untracking is what keeps a pin; `docs/README.md` links this spec - under Installation, whose stated purpose covers upgrade and migration paths. This is the first task at which - the docs gate is green, because it resolves T2's `-run` pattern against a test that now exists. + Acceptance: + - The callout under the page title that currently reads "**First-deploy seeding vs. runtime changes.** + ... *initial seed only*, so after the first deploy change the `Setting` resource, not the environment" is + replaced, not merely supplemented. That sentence is the rule this specification overturns, and a reader who + finds it has been told the opposite of the truth. + - The replacement states all four rules — backfill, declared override, tracked default, untracked + administrator-owned — in the order they apply on a worker start, each with its own condition, and names the + three that never write a blank value. + - The page answers the two questions an administrator will actually arrive with, each with the rule that + causes it and the way out of it: **a stored value stopped applying and an environment variable took over**, + and **an edited Setting reverted on the next worker start**. For each, the reader needs to know whether + untracking the Setting, clearing the variable, or changing the chart value is the escape, and which of those + is available for that particular Setting. + - The per-Setting table gains a `settings-default-tracking` row with its six defaults and the note that an + empty value tracks nothing. Every chart-rendered variable in the table's `Env seed` column is marked as + authoritative on every start rather than a first-deploy seed, which includes dropping the + `model-artifact-delivery-mode` row's "a seed never overrides a stored value". + - The page states that a tracked image Setting reverts to the operator default on every worker start, and that + untracking it is what keeps a pin. + - The page tells a reader how to work out which rule owns any given Setting, in terms they can check rather + than terms they must accept: whether the Deployment renders the variable, whether the name appears in + `settings-default-tracking`, and what follows when it does neither. A page that states the rules but leaves + the reader unable to place their own Setting in one of them has not explained the usage. + - `docs/README.md` links this spec under Installation, whose stated purpose covers upgrade and migration paths. + This is the first task at which the docs gate is green, because it resolves T2's `-run` pattern against a test + that now exists. Verify: `make lint docs` ### Test Plan @@ -497,6 +516,18 @@ Every case is one table row with declarative data, run through a single loop, ag `kubefake.NewSimpleClientset` with a reactor allowing `selfsubjectaccessreviews`, because `Initialize` calls `review.CanDoUpdate` first and the fake client denies it by default. +The two admission cases call the admission directly rather than running the registration's chain, and that leaves a +known gap. `Setting.admit` is unexported and `Configure` is the only path that runs it, and that path writes the +delegated Secret through a process-wide loopback client that `model_store_test.go` configures; driving it from these +cases would make their coverage depend on shuffle order, which `make test` decides. So the admission's logic is +pinned and its wiring into the registration is not. A test that rebuilt the chain by hand does not close this gap — +that is what the existing case does, and rebuilding the chain is exactly what cannot catch a later edit to the +registration. Closing it needs a `TestMain` in `pkg/worker/settings` that configures the loopback clients once, +which relocates `model_store_test.go`'s fixture and sits outside this change. `AllowBlank` is the concrete +consequence: deleting it, or replacing it with `DisallowBlank`, leaves the suite green, because a blank value is +admitted by `Allow()` plus the self-reference admission either way. `AllowBlank` is kept for intent and as a guard +against a stricter future admission, and its absence is not detected today. + #### Integration tests None. No controller reads these keys on a path this change alters, and `Settings.Initialize` runs once at From d786d646ece1750e3a361af7457857566ddb07de Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 18:35:36 +0800 Subject: [PATCH 05/12] fix(setting): withhold a sensitive Setting's values from the override log - the override line follows the Setting's own sensitivity marker: a sensitive one carries neither the stored value nor the new one, behind a marker no value can equal, and keeps the name, rule and variable that explain the change - a Setting that is not sensitive keeps its line pair for pair, so the refactor cannot redact everything and pass - the log reaches a wider audience than the delegated Secret, so a value kept out of the Secret must not reach the log either Signed-off-by: thxCode --- pkg/setting/kuberes.go | 37 ++++++++-- pkg/setting/kuberes_test.go | 139 +++++++++++++++++++++++++++++++++--- 2 files changed, 161 insertions(+), 15 deletions(-) diff --git a/pkg/setting/kuberes.go b/pkg/setting/kuberes.go index 9960fcb2b..1b9690cc3 100644 --- a/pkg/setting/kuberes.go +++ b/pkg/setting/kuberes.go @@ -94,11 +94,7 @@ func (h Settings) Initialize(ctx context.Context, cli kubernetes.Interface) erro } aSec.Data[name] = []byte(desired) skip = false - pairs := []any{"setting", name, "stored", stored, "value", desired, "rule", rule} - if s.envName != "" { - pairs = append(pairs, "envName", s.envName) - } - klog.InfoS("setting overridden on start", pairs...) + klog.InfoS("setting overridden on start", s.overrideLogPairs(stored, desired, rule)...) } return aSec, skip, nil } @@ -113,6 +109,37 @@ func (h Settings) Initialize(ctx context.Context, cli kubernetes.Interface) erro return nil } +// sensitiveValuesWithheld stands in for both values of a sensitive Setting on the override log +// line. It is a literal no value can equal, so a reader can tell it from an empty value, which +// reads as a Setting that holds nothing, and from an absent pair, which reads as one the operator +// never wrote. +const sensitiveValuesWithheld = "[withheld: sensitive setting]" + +// overrideLogPairs returns the key-value pairs the override log line carries for this Setting, +// given the stored value it replaces, the new value it writes and the rule that claimed it. +// +// A sensitive Setting's line carries neither value in any form. The log is a wider audience than +// the delegated Secret: it is usually readable more broadly and it is shipped to aggregators, CI +// output and backups, so a value the operator took care to keep out of the Secret must not appear +// there. The name, the rule and the variable stay, because together they are what tells an +// operator which Setting changed and why, and none of the three is the value. +// +// A Setting that is not sensitive keeps the line it carried before the marker was honored here, +// pair for pair and in the same order. +func (s Setting) overrideLogPairs(stored, desired, rule string) []any { + pairs := []any{"setting", s.name} + if s.Sensitive() { + pairs = append(pairs, "values", sensitiveValuesWithheld) + } else { + pairs = append(pairs, "stored", stored, "value", desired) + } + pairs = append(pairs, "rule", rule) + if s.envName != "" { + pairs = append(pairs, "envName", s.envName) + } + return pairs +} + // trackingValue returns the tracking list, read from the live Secret and falling back to the // registered Setting's built-in default when the Secret carries no such key. Reading the Secret // alone would defer the built-in list to a later start on a Secret that predates the setting. diff --git a/pkg/setting/kuberes_test.go b/pkg/setting/kuberes_test.go index dcd3d344a..25392901a 100644 --- a/pkg/setting/kuberes_test.go +++ b/pkg/setting/kuberes_test.go @@ -19,11 +19,27 @@ import ( type ( // caseSetting declares one Setting a case registers. A non-empty envName registers it // through that environment variable, and such a case must also declare that variable, - // because a Setting's declaration is fixed when it is registered. + // because a Setting's declaration is fixed when it is registered. A sensitive one is + // registered through the constructor that carries the sensitivity marker. caseSetting struct { - name string - defVal string - envName string + name string + defVal string + envName string + sensitive bool + } + + // overrideLogCase is one row: the Setting a case registers and the key-value pairs one + // override of it must then carry on its log line. A sensitive row must not list either value + // among them, or it passes for a line that published what it was meant to withhold. + overrideLogCase struct { + name string + setting string + sensitive bool + envName string + stored string + value string + rule string + want []any } // initializeCase is one row: the registry it builds, the environment it declares, the @@ -73,16 +89,24 @@ func runInitializeCases(t *testing.T, cases []initializeCase) { func registerCaseSettings(declared []caseSetting) Settings { h := Settings{} for _, s := range declared { - if s.envName != "" { - h.NewEditable(s.name, "A setting registered by a case.", - InitializeFromSpecifiedEnv(s.envName, s.defVal)) - continue - } - h.NewEditable(s.name, "A setting registered by a case.", InitializeFrom(s.defVal)) + registerCaseSetting(h, s) } return h } +// registerCaseSetting registers one declared Setting in the given registry and returns it, so a +// case can assert on the Setting a row built as well as on the Secret it goes on to produce. +func registerCaseSetting(h Settings, c caseSetting) Setting { + init := InitializeFrom(c.defVal) + if c.envName != "" { + init = InitializeFromSpecifiedEnv(c.envName, c.defVal) + } + if c.sensitive { + return h.NewSensitive(c.name, "A setting registered by a case.", init) + } + return h.NewEditable(c.name, "A setting registered by a case.", init) +} + // delegatedSecretObjects returns the objects a fake clientset starts with, so that a row either // finds no Secret at all or finds one holding exactly the given keys. func delegatedSecretObjects(stored map[string]string) []runtime.Object { @@ -120,6 +144,14 @@ func allowSelfSubjectAccessReviews(cli *kubefake.Clientset) { const ( trackedImage = "kv-cache-backend-image" blankImage = "container-registry" + + // overrideEnvName is the variable a case registers a Setting through when it wants that + // Setting's override line to carry a variable. + overrideEnvName = "GPUSTACK_TEST_CREDENTIAL" + + // credentialSetting is a name no registry carries, and the rows use it on both sides of the + // test, so what decides what a line carries is the Setting's marker and not its name. + credentialSetting = "registry-credential" ) // TestSettingsInitialize_DeclaredSettingOverridesStoredValue pins that a variable declared in the @@ -275,6 +307,93 @@ func TestSettingsInitialize_CreationSeedsEverySetting(t *testing.T) { }) } +// runOverrideLogCases runs each row through one execution loop: declare the environment the row +// registers its Setting through, register that Setting, build the line one override of it must +// carry, and assert on the pairs it carries. It must not be called from a parallel test, because +// a row declares its environment. +func runOverrideLogCases(t *testing.T, cases []overrideLogCase) { + t.Helper() + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + // Only a sensitive row claims to withhold, so only it can be voided by an expected + // line that carries the value anyway. + if c.sensitive { + require.NotContains(t, c.want, c.stored, + "the expected line already carries the stored value, so the row proves nothing") + require.NotContains(t, c.want, c.value, + "the expected line already carries the new value, so the row proves nothing") + } + + if c.envName != "" { + t.Setenv(c.envName, c.value) + } + s := registerCaseSetting(Settings{}, + caseSetting{name: c.setting, envName: c.envName, sensitive: c.sensitive}) + + assert.Equal(t, c.want, s.overrideLogPairs(c.stored, c.value, c.rule)) + }) + } +} + +// TestSetting_OverrideLogPairs pins that the override line follows the Setting's own sensitivity +// marker and nothing else. A sensitive value is withheld behind a marker a reader cannot mistake +// for an empty or a missing one, while the name, the rule and the variable stay, because they are +// what tells an operator why the value changed and none of them is the value. A Setting that is +// not sensitive keeps its line exactly as it was, so this cannot pass by withholding everything. +func TestSetting_OverrideLogPairs(t *testing.T) { + runOverrideLogCases(t, []overrideLogCase{ + { + name: "a sensitive setting withholds both values and keeps what explains the change", + setting: credentialSetting, + sensitive: true, + envName: overrideEnvName, + stored: "stored-secret", + value: "new-secret", + rule: "env-declared", + want: []any{ + "setting", credentialSetting, "values", sensitiveValuesWithheld, + "rule", "env-declared", "envName", overrideEnvName, + }, + }, + { + name: "a sensitive setting with no environment name withholds both values", + setting: credentialSetting, + sensitive: true, + stored: "stored-secret", + value: "new-secret", + rule: "default-tracked", + want: []any{ + "setting", credentialSetting, "values", sensitiveValuesWithheld, + "rule", "default-tracked", + }, + }, + { + name: "a setting that is not sensitive carries both values", + setting: credentialSetting, + envName: overrideEnvName, + stored: "stored-value", + value: "new-value", + rule: "env-declared", + want: []any{ + "setting", credentialSetting, "stored", "stored-value", + "value", "new-value", "rule", "env-declared", "envName", overrideEnvName, + }, + }, + { + name: "a setting that is not sensitive and has no environment name carries both values", + setting: credentialSetting, + stored: "stored-value", + value: "new-value", + rule: "default-tracked", + want: []any{ + "setting", credentialSetting, "stored", "stored-value", + "value", "new-value", "rule", "default-tracked", + }, + }, + }) +} + // TestParseTrackedSettings pins the normalization a stored tracking value goes through, so a value // an administrator writes cannot silently change which settings are tracked. func TestParseTrackedSettings(t *testing.T) { From 93c9513ab6e401911e4956778fa59ce1fc62b0e1 Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 18:36:46 +0800 Subject: [PATCH 06/12] docs: restate who owns a Setting value - replace the callout that called a GPUSTACK_* variable an initial seed only, since that is the rule this change overturns - state the four rules in run order, with the blank rule that keeps a variable set to the empty string from claiming a Setting - answer the two questions an administrator arrives with, each with the rule that causes it, the way out of it, and the chart value that produces a chart-rendered variable - give the two-check diagnostic for working out which rule owns a Setting - add the settings-default-tracking row, mark the chart-rendered variables as authoritative on every start, and drop the delivery mode row's retracted claim Task 5 of settings-env-sync-and-default-tracking. Signed-off-by: thxCode --- docs/README.md | 2 +- docs/reference/settings.md | 187 ++++++++++++++++-- ...-settings-env-sync-and-default-tracking.md | 25 ++- 3 files changed, 196 insertions(+), 18 deletions(-) diff --git a/docs/README.md b/docs/README.md index b8e0e1695..d71c717da 100644 --- a/docs/README.md +++ b/docs/README.md @@ -140,7 +140,7 @@ A skill link names a procedure; the entry's other fields carry shared facts. - Use for: choosing chart or image mode; vendored subcharts and their patches; vendor prerequisites; upgrade and migration paths. - Aliases: `Helm chart`, `subchart`, `chart mode`, `image mode`, `values.yaml` - Guides: choices in [Installation Modes](operate/installation-modes.md); node drivers in [Vendor Prerequisites](getting-started/vendor-prerequisites.md); transfer in [Migrating to Bundled Subcharts](operate/migration/to-subcharts.md). -- Specs: [bundled apps subchart split](../specs/2026-07-28-bundled-apps-subchart-split.md) +- Specs: [bundled apps subchart split](../specs/2026-07-28-bundled-apps-subchart-split.md) · [settings sync semantics](../specs/2026-10-19-settings-env-sync-and-default-tracking.md) - Code: [deploy/gpustack-operator/chart](../deploy/gpustack-operator/chart/) · vendoring in [hack/deps.sh](../hack/deps.sh) · image-mode in-cluster install in [pkg/worker/kuberess](../pkg/worker/kuberess/) - Related: [Development](#development) (chart targets and subchart patches) · [Devices Manager](#devices-manager) (the Device Manager DaemonSets the chart deploys) - Skills: [gpustack-operator-chart-e2e](../.agents/skills/gpustack-operator-chart-e2e/SKILL.md) (install, rollout and uninstall verification) · [gpustack-operator-chart-subcharts-manage](../.agents/skills/gpustack-operator-chart-subcharts-manage/SKILL.md) (add, patch or bump a vendored subchart) diff --git a/docs/reference/settings.md b/docs/reference/settings.md index 632c898c1..47725b4b9 100644 --- a/docs/reference/settings.md +++ b/docs/reference/settings.md @@ -1,19 +1,101 @@ # Settings & Environment Variables GPUStack Operator reads configuration from two places: the `Setting` resource, which you can change -at runtime, and environment variables, which the process reads once at startup. +at runtime, and environment variables, which the worker reads when it starts. -> **First-deploy seeding vs. runtime changes.** On first deploy, `settings.Initialize` creates the delegated -> Secret `gpustack-settings` in the system namespace and seeds every Setting from -> `GPUSTACK_`, or the built-in default when that variable is unset. Later restarts only -> **backfill missing** Settings and **never overwrite** a stored value: a `GPUSTACK_*` variable is an -> *initial seed only*, so after the first deploy change the `Setting` resource, not the environment. +> **The worker aligns every Setting on every start.** A `GPUSTACK_*` variable present in the worker +> process environment is a standing declaration rather than a one-time default: while it holds a +> non-blank value, the worker writes it to the Setting on every start and the stored value loses. A +> Setting named in `settings-default-tracking` is written with the operator's built-in default on every +> start. Every other Setting keeps the value you stored. [Who owns a value](#who-owns-a-value) states +> the four rules in the order they run, and [Working out which rule owns a +> Setting](#working-out-which-rule-owns-a-setting) tells you which of them owns yours. ## Contents +- [Who owns a value](#who-owns-a-value) - [Online-adjustable settings](#online-adjustable-settings) +- [When a Setting does not keep your value](#when-a-setting-does-not-keep-your-value) - [Deploy-time environment variables](#deploy-time-environment-variables) +## Who owns a value + +Every registered Setting lives as one key of a single delegated Secret, `gpustack-settings` in the +system namespace. The worker aligns that Secret once per worker start and asks one question about each +key: does the operator own this value, or does the administrator? A value the operator owns is written +on that start and logged; a value it does not own is left exactly as it was stored. + +Write `D` for a Setting's **seed**: the value of `GPUSTACK_` when that variable is +present in the worker process environment, and the built-in default when the variable is absent. A +variable present with an empty value yields a blank seed rather than the built-in default, and `{}` and +`[]` read as blank too. + +The alignment applies four rules, in this order: + +1. **Backfill.** A key the Secret does not carry is added with its seed. A blank seed is a key like any + other, so a Setting with a blank default still appears in `kubectl get settings`. +2. **A declared variable wins.** If the worker environment declares the variable for the Setting and + the seed is not blank, the seed replaces the stored value. +3. **A tracked Setting follows the operator's default.** If `settings-default-tracking` names the + Setting and no declared variable has claimed it, the built-in default replaces the stored value. +4. **Otherwise the stored value is the administrator's.** Nothing writes it, a blank one included. + +A Setting that is both declared and tracked is written by rule 2, with the variable's value. The two +rules cannot disagree, because a Setting has one seed and both of them write it. + +Rules 2, 3 and 4 never write a blank value, rule 4 writing nothing at all, which is why a variable set +to the empty string takes a Setting over nothing and a tracked Setting whose default is blank keeps what +you stored. Only backfill writes a blank, and it runs first so that a newly registered Setting appears +in the same pass that decides whether the operator owns it. + +Deleting the Secret is the case with nothing to preserve: the next worker start creates it again from +every seed, and `settings-default-tracking` returns to the six names it ships with. + +### Every override is logged + +A start that changes a value writes one line per Setting to the worker log, naming the Setting, the +value it replaced, the value it wrote and the rule that fired: + +```text +setting overridden on start setting="container-registry" stored="registry.example" value="mirror.example" rule="env-declared" envName="GPUSTACK_CONTAINER_REGISTRY" +``` + +`rule` is `env-declared` for rule 2 and `default-tracked` for rule 3, and `envName` names the variable +when the Setting reads one. A start that changes nothing logs nothing, and a stored value that already +equals the seed is not an override. + +```bash +kubectl -n gpustack-system logs deploy/gpustack-operator-worker | grep 'setting overridden on start' +``` + +### The tracking list + +`settings-default-tracking` holds a comma-separated list of Setting names. The worker reads it from the +Secret and falls back to the six names the operator ships when the Secret carries no such key, so a +cluster that predates the Setting is tracked from the first start that backfills the key. + +A stored value cannot silently change what is tracked. Each segment is trimmed, and a segment left +empty, a duplicate, the list's own name and a name this operator does not register are dropped, each +logged once at verbosity 4. A write that names the list itself is refused outright, because a Setting +tracking its own membership would reconcile that membership against itself. + +### Working out which rule owns a Setting + +Two checks place any Setting you did not write yourself, and they are the two the rules read: + +```bash +# 1. Does the worker Deployment render a variable for it, and with what value? +kubectl -n gpustack-system get deploy -o yaml | grep -A1 'GPUSTACK_' + +# 2. Does the tracking list name it? +kubectl -n gpustack-system get setting settings-default-tracking -o yaml +``` + +- The Deployment renders the variable with a non-blank value, so rule 2 owns the Setting whatever is + stored. The chart renders five of them itself, marked in the table below. +- The Deployment does not render it and the tracking list names it, so rule 3 owns the Setting. +- Neither, so rule 4 applies and the value you stored stands until you change it yourself. + ## Online-adjustable settings A fixed catalog of named values, served as the namespaced `Setting` aggregated API resource @@ -21,6 +103,12 @@ A fixed catalog of named values, served as the namespaced `Setting` aggregated A picks a new value up on its next reconcile. Fixed means the resource serves `get,list,watch,apply,update,patch` and no create/delete: you edit a Setting's **value**, not its existence. +The `Env seed` column names the variable a Setting reads. Every one of them is authoritative on every +worker start while the variable is present and holds a non-blank value, whether the chart renders it or +you add it yourself ([who owns a value](#who-owns-a-value)). The rows marked *chart-rendered* name a +variable the chart writes onto the worker Deployment itself, so the chart re-asserts it on every start +and every upgrade. + ```bash # List every setting and its current value kubectl -n gpustack-system get settings # or: kubectl -n gpustack-system get set @@ -33,10 +121,11 @@ kubectl -n gpustack-system patch setting instance-type-derived-from-node --type | Setting | Env seed | Default | Effect | |---------|-------------------------|---------|--------| -| `container-registry` | `GPUSTACK_CONTAINER_REGISTRY` | *(blank)* | Registry to pull images from, for all built-in applications' deployments. | -| `container-namespace` | `GPUSTACK_CONTAINER_NAMESPACE` | *(blank)* | Namespace to pull images from, for all built-in applications' deployments. | -| `image-pull-secrets` | `GPUSTACK_IMAGE_PULL_SECRETS` | *(blank)* | Image pull secret for pulling images, for all built-in applications' deployments — the subcharts the chart or the worker installs. It does NOT reach a workload a controller renders from a custom resource: a [KV cache backend](../modules/kv-cache/backend.md) names its own `spec.imagePullSecrets`. | -| `image-pull-policy` | `GPUSTACK_IMAGE_PULL_POLICY` | `IfNotPresent` | Image pull policy for all built-in applications' deployments, on the same boundary as the secret above: a controller-rendered workload carries its own. | +| `container-registry` | `GPUSTACK_CONTAINER_REGISTRY` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Registry to pull images from, for all built-in applications' deployments. | +| `container-namespace` | `GPUSTACK_CONTAINER_NAMESPACE` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Namespace to pull images from, for all built-in applications' deployments. | +| `image-pull-secrets` | `GPUSTACK_IMAGE_PULL_SECRETS` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Image pull secret for pulling images, for all built-in applications' deployments — the subcharts the chart or the worker installs. It does NOT reach a workload a controller renders from a custom resource: a [KV cache backend](../modules/kv-cache/backend.md) names its own `spec.imagePullSecrets`. | +| `image-pull-policy` | `GPUSTACK_IMAGE_PULL_POLICY` — *chart-rendered; authoritative on every start* | `IfNotPresent` | Image pull policy for all built-in applications' deployments, on the same boundary as the secret above: a controller-rendered workload carries its own. The chart renders it, so a runtime edit to this Setting is reverted at the next worker start; change it through `global.imagePullPolicy` or `image.pullPolicy` and leave this Setting alone. | +| `settings-default-tracking` | *(none)* | `instance-ssh-server-image`, `kv-cache-backend-image`, `model-deployment-router-image`, `model-deployment-router-proxy-image`, `model-deployment-routing-sidecar-image`, `model-prefetch-warmup-image` | Comma-separated names of the Settings the operator rewrites with its built-in default on every worker start, so that a default shipped in a later release reaches a cluster that never chose a value. A name in the list makes its Setting follow rule 3 ([who owns a value](#who-owns-a-value)); removing a name is what keeps an image pin across an operator upgrade ([keeping a pin](#keeping-an-image-pin-across-an-operator-upgrade)). An empty value tracks nothing, and whitespace, duplicates and unregistered names are dropped ([the tracking list](#the-tracking-list)). No environment variable reads this Setting, so nothing on the Deployment can take the list over. | | `instance-general-resources-overcommit` | `GPUSTACK_INSTANCE_GENERAL_RESOURCES_OVERCOMMIT` | `true` | Overcommit an Instance's general resources: when enabled, a general unit requests 800m CPU / 128Mi RAM and one-eighth local storage, an accelerated unit 100m CPU / 128Mi RAM and one-eighth local storage — so e.g. a 1C/4Gi + 128Gi type requesting 2 accelerators and 64Gi storage resolves to 200m/256Mi + 8Gi. A CPU limit whose whole cores cost more than the limit itself (e.g. `100m`, or `1500m` whose half core rounds up) requests the limit instead — a request may never exceed its limit. | | `instance-ssh-server-image` | `GPUSTACK_INSTANCE_SSH_SERVER_IMAGE` | `gpustack/ssh-server:v1.3.0` | Image of the SSH server used when deploying Instances. | | `kv-cache-backend-image` | `GPUSTACK_KV_CACHE_BACKEND_IMAGE` | `gpustack/mirrored-mooncake:0.3.13.post1-cpu` | Image every role of a [KV cache backend](../modules/kv-cache/backend.md) runs when the object does not name one itself. The default is this project's own build — a CPU build carrying TCP and EFA over DRAM, and the only image that can run [`leader.electionBackend`](../modules/kv-cache/leader.md#high-availability). The per-vendor variant builds, their tags and which one a VRAM group needs are under [The project's own build variants](../modules/kv-cache/backend.md#the-projects-own-build-variants). **It does not fit every backend**: a backend on another transport or runtime names its own `spec.image`, which always wins over this. **Clearing this Setting restores the admission refusal** for a backend naming no image anywhere. Why one default cannot fit every backend, and what having one costs, is under [The image](../modules/kv-cache/backend.md#the-image). | @@ -52,7 +141,7 @@ kubectl -n gpustack-system patch setting instance-type-derived-from-node --type | `model-artifact-no-proxy` | `GPUSTACK_MODEL_ARTIFACT_NO_PROXY` | *(blank)* | Comma-separated hosts that bypass `model-artifact-https-proxy`, for the node plugin too, given to engines as `NO_PROXY` on the same terms. | | `model-artifact-ca-bundle` | `GPUSTACK_MODEL_ARTIFACT_CA_BUNDLE` | *(blank)* | Name of a ConfigMap in the worker's namespace whose `ca.crt` the resolution and the node plugin trust beside the system pool. **It is not given to engine Pods**: they run in tenant namespaces, which cannot mount it. | | `model-artifact-revalidate-interval` | `GPUSTACK_MODEL_ARTIFACT_REVALIDATE_INTERVAL` | `24h` | How often a resolved hub artifact's access is checked again, at least `1m`; a Secret change checks it at once. What a refusal does is under [Resolution and revalidation](../modules/model-delivery/artifact.md#resolution-and-revalidation). | -| `model-artifact-delivery-mode` | `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` | `Engine` | How a hub artifact's weights reach a `ModelDeployment`: `Engine`, the engine downloads them, or `Node`, the node's [`model-manager` plugin](../modules/model-delivery/node-store.md) mounts a verified copy. The chart seeds `Node` when it deploys the plugin, and a seed never overrides a stored value. `Node` is refused while the CSIDriver `model.csi.gpustack.ai` does not exist. **Changing it rolls every deployment using a hub artifact once**; see [Switch delivery](../modules/model-delivery/operations.md#switch-delivery). | +| `model-artifact-delivery-mode` | `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` — *chart-rendered when the plugin is deployed; authoritative on every start while it is rendered* | `Engine` | How a hub artifact's weights reach a `ModelDeployment`: `Engine`, the engine downloads them, or `Node`, the node's [`model-manager` plugin](../modules/model-delivery/node-store.md) mounts a verified copy. The chart declares `Node` when it deploys the plugin, and the chart owns the Setting from then on: a runtime edit is reverted at the next worker start. Deploying or removing the plugin (`modelManager.enabled`) is what changes the value. `Node` is refused while the CSIDriver `model.csi.gpustack.ai` does not exist. **Changing it rolls every deployment using a hub artifact once**; see [Switch delivery](../modules/model-delivery/operations.md#switch-delivery). | | `model-store-high-watermark` | `GPUSTACK_MODEL_STORE_HIGH_WATERMARK` | `80` | The node cache filesystem's usage percent above which the plugin removes content no Pod mounts; above the low watermark, at most `95`. On a filesystem shared with kubelet the plugin caps it below kubelet's thresholds ([the capacity rule](../modules/model-delivery/operations.md#the-capacity-rule)). | | `model-store-low-watermark` | `GPUSTACK_MODEL_STORE_LOW_WATERMARK` | `70` | The usage percent a collection removes down to; at least `1`, below the high watermark. | | `model-store-download-concurrency` | `GPUSTACK_MODEL_STORE_DOWNLOAD_CONCURRENCY` | `8` | Concurrent HTTP requests the plugin makes per node, across every download; `1` to `64`. | @@ -199,9 +288,83 @@ A namespace enforcing the `baseline` or `restricted` Pod Security Standard refus creation instead, since the sysctl is not on that standard's allowed list either: no Pod appears, and the operator logs the refusal. +## When a Setting does not keep your value + +Both symptoms below are rules 2 and 3 seen from the other side. A value that changed without your +action was written by the operator at a worker start, and the override log line says which rule wrote +it, along with the variable when one did. + +### A stored value stopped applying and a variable took over + +Rule 2 wrote a variable's value over the one you stored. The log line reports it as +`rule="env-declared"` and names the variable in `envName`, so you do not have to work out which +declaration claimed the Setting. + +The way out is the variable, not the Secret: + +- A variable you set yourself, through the chart's `worker.env` or by editing the worker Deployment. + Change it or remove it and your stored value stands at the next worker start. A hand-edited + Deployment is outside the chart's record, so a rollback or an upgrade run with + `--reset-then-reuse-values` drops it; a variable meant to last belongs in a chart value. +- A variable the chart renders, the five rows marked above. Change the chart value, or clear it so the + chart renders an empty one, and `helm upgrade`. A blank declaration claims nothing, so clearing the + chart value is what hands the Setting back to you. + +| Chart-rendered variable | Chart value that produces it | Chart default | +|---|---|---| +| `GPUSTACK_CONTAINER_REGISTRY` | `global.imageRegistry`, falling back to `global.hub` | *(empty)* | +| `GPUSTACK_CONTAINER_NAMESPACE` | `global.imageNamespace` | *(empty)* | +| `GPUSTACK_IMAGE_PULL_SECRETS` | `global.imagePullSecrets` | *(empty)* | +| `GPUSTACK_IMAGE_PULL_POLICY` | `global.imagePullPolicy`, falling back to `worker.image.pullPolicy` and then `image.pullPolicy` | `IfNotPresent` | +| `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` | `modelManager.enabled`, which renders `Node` while the plugin is deployed | *(not rendered)* | + +Setting the chart value to an empty string is the same as clearing it. With `modelManager.enabled: false` +the chart stops rendering the delivery mode entirely, and the stored value is then nobody's to write. + +### An edit reverted on the next worker start + +Either rule wrote over the edit, and `rule` in the log line says which one: + +- `rule="env-declared"` means a variable holds the value. Changing or clearing that variable is the way + out, as above. +- `rule="default-tracked"` means the Setting's name is in `settings-default-tracking`. Removing the name + from that list is the way out. + +Your edit holds until the next worker start, and nothing reverts it while the worker runs. A node drain, +a crash, a restart and an upgrade each start a worker, so each of them applies the rules again. + +### Keeping an image pin across an operator upgrade + +The six image Settings are tracked by default, so the operator writes each of them with its built-in +default on **every** worker start, not only on the start where a release changes that default. A pin +made with `kubectl patch` holds until the next start, and reverts to the default at that one: + +```bash +kubectl -n gpustack-system patch setting kv-cache-backend-image --type merge \ + -p '{"spec":{"value":"mirror.example/kv-cache:1.2.3"}}' +``` + +Untracking the Setting is the only thing that keeps the pin, and nothing else stands in for it: a +variable set to an empty value is no declaration, and a default that is blank is never written at all. + +```bash +# Read the list, drop the one name whose Setting you want to keep your own value for, +# and write the rest back. This removes model-prefetch-warmup-image and tracks the other five. +kubectl -n gpustack-system get setting settings-default-tracking -o yaml +kubectl -n gpustack-system patch setting settings-default-tracking --type merge \ + -p '{"spec":{"value":"instance-ssh-server-image,kv-cache-backend-image,model-deployment-router-image,model-deployment-router-proxy-image,model-deployment-routing-sidecar-image"}}' +``` + +A pin that predates this behavior is replaced on the first worker start of the release that introduces +it, and the log line names both values when it happens. Deleting the Secret has the same effect on the +list, which comes back as its six default names. + ## Deploy-time environment variables -Read **once at process startup**, so changing one means restarting or redeploying the affected component. +Read when the process starts, so changing one means restarting or redeploying the affected component. +A variable a Setting reads is re-read on each of those starts rather than applied once +([who owns a value](#who-owns-a-value)); the rest are plain process configuration. + The Worker (WK) copies every `GPUSTACK_`-prefixed variable from its own Pod spec onto the Device Manager (DM) DaemonSets, so setting one on the Worker Deployment reaches the DMs automatically. diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index 79d15c475..6f59a02c0 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -157,8 +157,20 @@ which Settings are tracked: Every dropped segment is logged once, at `klog.V(4)`, naming the segment and the reason. **Override logging.** Each of rules 1 and 2 that changes a stored value emits one `klog.InfoS` line carrying the -Setting name, the old value, the new value, and the rule that fired. A pass that changes nothing logs nothing. -A rule-2 write where the stored value already equals `D` is not an override and is not logged. +Setting name, the old value, the new value, the rule that fired, and the variable when the Setting declares one. +A pass that changes nothing logs nothing. A rule-2 write where the stored value already equals `D` is not an +override and is not logged. + +A Setting marked sensitive carries neither value. The line keeps the name, the rule and the variable, because +together they are what tells an operator which Setting changed and why, and none of the three is the value; both +values are replaced by one marker no value can equal, so a reader can tell it from an empty value and from an +absent pair. The log is a wider audience than the delegated Secret — usually readable more broadly, and shipped to +aggregators, CI output and backups — so a value the operator kept out of the Secret must not reach it. The marker +is the repository's existing sensitivity flag, `Setting.Sensitive()`; no redaction convention was invented, because +there is none to follow. No registered Setting carries that flag today, and none of the 37 can currently hold a +credential: `container-registry` refuses credentials through its own admission, proxy admissions refuse them, image +references are `repo:tag@digest`, and `image-pull-secrets` holds Secret names. The rule is what keeps the first +sensitive Setting from becoming the first leak. **The chart keeps `model-artifact-delivery-mode`.** `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` is a non-blank declaration, so rule 1 makes it authoritative and the chart owns the Setting. The worker cannot take it over: @@ -184,8 +196,11 @@ that owns that page. 2. **`image-pull-policy` becomes chart-owned.** The chart renders `GPUSTACK_IMAGE_PULL_POLICY=IfNotPresent`, which is non-blank, so rule 1 applies and the chart is the source of truth. Change it through chart values, not through the Setting resource. -3. **Manufacturer identity variables are unaffected.** The chart renders them on every start, but no - administrator patches them, so rule 1 writing them is a no-op. +3. **Manufacturer identity variables are unaffected.** The chart renders `GPUSTACK__PCI_VENDOR_ID` and its + siblings into Pod environments from `_helpers.tpl`, and they are not among the registered Settings. Rule 1 + matches a registered Setting's declared variable, so it cannot fire on them at any value. An earlier draft of + this caveat said rule 1 writing them is a no-op, which implied they were Settings rule 1 considered; measured + against this checkout on 2026-10-19, no such registration exists. 4. **`container-registry`, `container-namespace`, and `image-pull-secrets` default to blank.** Rule 1 does not fire for a blank declaration, so these stay administrator-owned exactly as today. 5. **Deleting the Secret resets the tracking list.** `settings-default-tracking` returns to its default, so image @@ -437,7 +452,7 @@ T4 ───────────────┘ changes. Verify: `make lint chart && make lint docs` -- [ ] **T5 · Restate the Settings contract and index this spec** +- [x] **T5 · Restate the Settings contract and index this spec** Blocked by: T2, T3, T4 Owns: `docs/reference/settings.md`, `docs/README.md` Gate: none From 8c47996ac1a14e8fab90835cd1bab7a47a5f12d8 Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 18:37:31 +0800 Subject: [PATCH 07/12] docs: document the withheld form of the override log - the page described the line as always carrying both values, which stopped being true when a sensitive Setting began withholding them - state the withheld form, keep the operator's guide unchanged, and say that no registered Setting carries the mark today The spec records the same rule and corrects caveat 3, which called the manufacturer identity variables Settings rule 1 could write; they are Pod environment the chart renders, and no such Setting is registered. Signed-off-by: thxCode --- docs/reference/settings.md | 6 ++++++ specs/2026-10-19-settings-env-sync-and-default-tracking.md | 5 +++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/reference/settings.md b/docs/reference/settings.md index 47725b4b9..b00d1810d 100644 --- a/docs/reference/settings.md +++ b/docs/reference/settings.md @@ -64,6 +64,12 @@ setting overridden on start setting="container-registry" stored="registry.exampl when the Setting reads one. A start that changes nothing logs nothing, and a stored value that already equals the seed is not an override. +A Setting the operator marks sensitive logs neither value. Its line carries the name, the rule and the +variable — enough to tell you which Setting changed and why — and replaces both values with +`values="[withheld: sensitive setting]"`. The log reaches a wider audience than this Secret, so a value +kept out of the Secret stays out of the log. No registered Setting carries that mark today; the operator +sets it on a Setting whose value should not appear in a log at all. + ```bash kubectl -n gpustack-system logs deploy/gpustack-operator-worker | grep 'setting overridden on start' ``` diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index 6f59a02c0..37ee83530 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -1,6 +1,6 @@ # Spec: Settings Sync Semantics — Environment Declarations and Default Tracking -Status: Building +Status: Built Blocked on: nothing. T1 to T5 are ready to build; the open question at the end is a confirmation, not a dependency. Type: Feature @@ -427,7 +427,8 @@ T4 ───────────────┘ evaluated without it; the name is an exported constant so the task registering the Setting cannot drift from the one reading it; `desiredValue` returns a value only for a non-blank seed that is either declared or tracked; every change to a stored value logs one `klog.InfoS` line carrying the Setting, both values, the - rule and the variable; `TestSettingsInitialize` covers the table below and passes. + rule and the variable, and a sensitive Setting carries neither value behind a marker instead; `TestSettingsInitialize` + covers the table below and passes. Verify: `GOTOOLCHAIN=local GODEBUG=gotypesalias=0 CGO_ENABLED=1 go test -race -v ./pkg/setting -run TestSettingsInitialize && make lint` - [x] **T3 · Register `settings-default-tracking`** From 700d958388ed58edf75c2b2406a8b2b6da364d30 Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 18:47:10 +0800 Subject: [PATCH 08/12] docs: sort the Settings table by Setting name - the table grew by insertion, so related rows sat far apart and a reader scanned the whole table to locate one name - reorder the 37 rows alphabetically; every row keeps its content byte for byte, and the per-reconcile paragraph below the table still lists the same five Settings The reorder came from the review agent's pass over the page. The docs gate reports 76 pages and no broken internal link. Signed-off-by: thxCode --- docs/reference/settings.md | 44 +++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/docs/reference/settings.md b/docs/reference/settings.md index b00d1810d..785e053e9 100644 --- a/docs/reference/settings.md +++ b/docs/reference/settings.md @@ -127,43 +127,43 @@ kubectl -n gpustack-system patch setting instance-type-derived-from-node --type | Setting | Env seed | Default | Effect | |---------|-------------------------|---------|--------| -| `container-registry` | `GPUSTACK_CONTAINER_REGISTRY` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Registry to pull images from, for all built-in applications' deployments. | | `container-namespace` | `GPUSTACK_CONTAINER_NAMESPACE` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Namespace to pull images from, for all built-in applications' deployments. | -| `image-pull-secrets` | `GPUSTACK_IMAGE_PULL_SECRETS` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Image pull secret for pulling images, for all built-in applications' deployments — the subcharts the chart or the worker installs. It does NOT reach a workload a controller renders from a custom resource: a [KV cache backend](../modules/kv-cache/backend.md) names its own `spec.imagePullSecrets`. | +| `container-registry` | `GPUSTACK_CONTAINER_REGISTRY` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Registry to pull images from, for all built-in applications' deployments. | | `image-pull-policy` | `GPUSTACK_IMAGE_PULL_POLICY` — *chart-rendered; authoritative on every start* | `IfNotPresent` | Image pull policy for all built-in applications' deployments, on the same boundary as the secret above: a controller-rendered workload carries its own. The chart renders it, so a runtime edit to this Setting is reverted at the next worker start; change it through `global.imagePullPolicy` or `image.pullPolicy` and leave this Setting alone. | -| `settings-default-tracking` | *(none)* | `instance-ssh-server-image`, `kv-cache-backend-image`, `model-deployment-router-image`, `model-deployment-router-proxy-image`, `model-deployment-routing-sidecar-image`, `model-prefetch-warmup-image` | Comma-separated names of the Settings the operator rewrites with its built-in default on every worker start, so that a default shipped in a later release reaches a cluster that never chose a value. A name in the list makes its Setting follow rule 3 ([who owns a value](#who-owns-a-value)); removing a name is what keeps an image pin across an operator upgrade ([keeping a pin](#keeping-an-image-pin-across-an-operator-upgrade)). An empty value tracks nothing, and whitespace, duplicates and unregistered names are dropped ([the tracking list](#the-tracking-list)). No environment variable reads this Setting, so nothing on the Deployment can take the list over. | +| `image-pull-secrets` | `GPUSTACK_IMAGE_PULL_SECRETS` — *chart-rendered; authoritative on every start once its chart value is non-blank* | *(blank)* | Image pull secret for pulling images, for all built-in applications' deployments — the subcharts the chart or the worker installs. It does NOT reach a workload a controller renders from a custom resource: a [KV cache backend](../modules/kv-cache/backend.md) names its own `spec.imagePullSecrets`. | +| `instance-access-static-address` | `GPUSTACK_INSTANCE_ACCESS_STATIC_ADDRESS` | *(blank)* | Static access address for all Instances; when unset, the access address is generated from host IPs. | +| `instance-access-wildcard-dns` | `GPUSTACK_INSTANCE_ACCESS_WILDCARD_DNS` | *(blank)* | Wildcard DNS for all Instances (e.g. `traefik.me`), used to build a per-Instance domain `.`. Only effective when `instance-access-static-address` is not set. | | `instance-general-resources-overcommit` | `GPUSTACK_INSTANCE_GENERAL_RESOURCES_OVERCOMMIT` | `true` | Overcommit an Instance's general resources: when enabled, a general unit requests 800m CPU / 128Mi RAM and one-eighth local storage, an accelerated unit 100m CPU / 128Mi RAM and one-eighth local storage — so e.g. a 1C/4Gi + 128Gi type requesting 2 accelerators and 64Gi storage resolves to 200m/256Mi + 8Gi. A CPU limit whose whole cores cost more than the limit itself (e.g. `100m`, or `1500m` whose half core rounds up) requests the limit instead — a request may never exceed its limit. | +| `instance-host-path-volume-allowed` | `GPUSTACK_INSTANCE_HOST_PATH_VOLUME_ALLOWED` | `false` | Whether an Instance may mount a hostPath volume (`spec.additionalVolumes[*].hostPath`), which reaches the node's filesystem. Kept separate from `instance-privileged-allowed` because it grants strictly less — the filesystem, but not the node's devices or kernel — so an admin can allow node-path mounts without allowing a container escape. Enforced whenever an Instance **takes** a hostPath mount, on the same terms; a mount it already has is matched by value, so reordering the list is not a new grant while repointing an entry at a different node path is. | +| `instance-persistent-volume-placement` | `GPUSTACK_INSTANCE_PERSISTENT_VOLUME_PLACEMENT` | `true` | Whether an Instance's persistent claims (`spec.volume.persistent`, `spec.additionalVolumes[*].persistent`) decide where its Pod runs, by the [claim placement rules](../modules/model-delivery/artifact.md#claim-delivery-and-placement): a bound claim adds its PV's required node affinity, and a claim nothing binds holds the Pod back with the reason in `status.phaseMessage`. **`false` is the escape path**: the Pod is built from the claim names alone, as before, and Kueue's topology-aware scheduling may then assign a node the volume cannot reach, leaving the Pod Pending while its Workload holds quota. Read when a Pod is created, so a change never moves a running Instance; stop and start one to apply it. | +| `instance-privileged-allowed` | `GPUSTACK_INSTANCE_PRIVILEGED_ALLOWED` | `false` | Whether an Instance may request privileged mode (`spec.privileged`), which escapes the container boundary and exposes the node's devices and kernel surface. Enforced by the Instance admission webhook whenever an Instance **takes** privileged mode — at creation, or through a later change, including one made while it is stopped. An Instance that already runs privileged keeps it: with this off it stays updatable, editable while stopped, and restartable. Like every setting here, a change takes up to 30s to reach the webhook, so an Instance created in that window is judged against the previous value. | | `instance-ssh-server-image` | `GPUSTACK_INSTANCE_SSH_SERVER_IMAGE` | `gpustack/ssh-server:v1.3.0` | Image of the SSH server used when deploying Instances. | +| `instance-type-aware-cpu-manufacturer` | `GPUSTACK_INSTANCE_TYPE_AWARE_CPU_MANUFACTURER` | `false` | Whether the derived ClusterQueue/InstanceType/InstanceTypeFlavor aggregation splits by CPU manufacturer. When `false`, non-accelerated flavors collapse into one `generic` pool per os/arch and accelerated flavors pool per accelerator (CPU ignored); when `true`, every pool splits by the CPU key (`gpustack--${gKey}-…` / `gpustack--${gKey}--${aKey}-…`) and the InstanceType records the raw CPU detail. The `ResourceFlavor`s themselves are unaffected — they always carry the CPU key, so a flip only re-groups the aggregation layer. Read per-reconcile. | +| `instance-type-derived-from-node` | `GPUSTACK_INSTANCE_TYPE_DERIVED_FROM_NODE` | `true` | Whether the operator auto-derives the InstanceType (and its backing ClusterQueue) from node hardware. When `true`, the `NodeFlavorReconciler` authors the derived InstanceType (create-only); when `false`, it only aligns the ResourceFlavor and the administrator defines the InstanceType via the API, and owns feasibility with it — see [Authoring the InstanceType](#authoring-the-instancetype). Read per-reconcile. | +| `instance-type-drain-when-no-flavors` | `GPUSTACK_INSTANCE_TYPE_DRAIN_WHEN_NO_FLAVORS` | `true` | Whether a ClusterQueue whose pool has lost all its ResourceFlavors is drained (`HoldAndDrain`, so Kueue evicts admitted workloads) before its resource groups are emptied. When `true`, the queue is drained first; when `false`, the operator waits for the reservations to clear on their own, then empties. Either way the groups are emptied only once every reservation is zero, so Kueue's counters never go negative. Read per-reconcile. | +| `instance-type-mixed-on-node` | `GPUSTACK_INSTANCE_TYPE_MIXED_ON_NODE` | `true` | Whether one node may surface both a GPU and a CPU-only InstanceType. When `true`, a node is summarized into every type it can serve; when `false`, a node with accelerators yields only a GPU InstanceType and a CPU-only node only a general one. Read per-reconcile. | | `kv-cache-backend-image` | `GPUSTACK_KV_CACHE_BACKEND_IMAGE` | `gpustack/mirrored-mooncake:0.3.13.post1-cpu` | Image every role of a [KV cache backend](../modules/kv-cache/backend.md) runs when the object does not name one itself. The default is this project's own build — a CPU build carrying TCP and EFA over DRAM, and the only image that can run [`leader.electionBackend`](../modules/kv-cache/leader.md#high-availability). The per-vendor variant builds, their tags and which one a VRAM group needs are under [The project's own build variants](../modules/kv-cache/backend.md#the-projects-own-build-variants). **It does not fit every backend**: a backend on another transport or runtime names its own `spec.image`, which always wins over this. **Clearing this Setting restores the admission refusal** for a backend naming no image anywhere. Why one default cannot fit every backend, and what having one costs, is under [The image](../modules/kv-cache/backend.md#the-image). | +| `model-artifact-ca-bundle` | `GPUSTACK_MODEL_ARTIFACT_CA_BUNDLE` | *(blank)* | Name of a ConfigMap in the worker's namespace whose `ca.crt` the resolution and the node plugin trust beside the system pool. **It is not given to engine Pods**: they run in tenant namespaces, which cannot mount it. | +| `model-artifact-delivery-mode` | `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` — *chart-rendered when the plugin is deployed; authoritative on every start while it is rendered* | `Engine` | How a hub artifact's weights reach a `ModelDeployment`: `Engine`, the engine downloads them, or `Node`, the node's [`model-manager` plugin](../modules/model-delivery/node-store.md) mounts a verified copy. The chart declares `Node` when it deploys the plugin, and the chart owns the Setting from then on: a runtime edit is reverted at the next worker start. Deploying or removing the plugin (`modelManager.enabled`) is what changes the value. `Node` is refused while the CSIDriver `model.csi.gpustack.ai` does not exist. **Changing it rolls every deployment using a hub artifact once**; see [Switch delivery](../modules/model-delivery/operations.md#switch-delivery). | +| `model-artifact-https-proxy` | `GPUSTACK_MODEL_ARTIFACT_HTTPS_PROXY` | *(blank)* | HTTPS proxy for resolution, revalidation and the node plugin's downloads, and the `HTTPS_PROXY` given to an engine downloading the weights, where a role's own value wins. Blank keeps the worker's own environment and renders nothing. Only an `http` or `https` URL without credentials is accepted, because the value reaches tenant Pods; a value from the environment that is not refuses the worker's start. | +| `model-artifact-huggingface-endpoint` | `GPUSTACK_MODEL_ARTIFACT_HUGGINGFACE_ENDPOINT` | `https://huggingface.co` | The Hugging Face Hub a [`ModelArtifact`](../modules/model-delivery/artifact.md) resolves and revalidates against, and the `HF_ENDPOINT` an engine downloading the weights is given; the node plugin downloads from it too. What changing it does is under [Engine delivery](../modules/model-delivery/artifact.md#engine-delivery). | +| `model-artifact-modelscope-endpoint` | `GPUSTACK_MODEL_ARTIFACT_MODELSCOPE_ENDPOINT` | `https://www.modelscope.cn` | The ModelScope hub a [`ModelArtifact`](../modules/model-delivery/artifact.md) resolves and revalidates against and the node plugin downloads from; an engine downloading the weights gets its host as `MODELSCOPE_DOMAIN` — the runners' SDK takes a bare host. What changing it does is under [Engine delivery](../modules/model-delivery/artifact.md#engine-delivery). | +| `model-artifact-no-proxy` | `GPUSTACK_MODEL_ARTIFACT_NO_PROXY` | *(blank)* | Comma-separated hosts that bypass `model-artifact-https-proxy`, for the node plugin too, given to engines as `NO_PROXY` on the same terms. | +| `model-artifact-revalidate-interval` | `GPUSTACK_MODEL_ARTIFACT_REVALIDATE_INTERVAL` | `24h` | How often a resolved hub artifact's access is checked again, at least `1m`; a Secret change checks it at once. What a refusal does is under [Resolution and revalidation](../modules/model-delivery/artifact.md#resolution-and-revalidation). | | `model-deployment-kv-cache-dtype-owned` | `GPUSTACK_MODEL_DEPLOYMENT_KV_CACHE_DTYPE_OWNED` | `true` | Hand every `KVCachePoolBinding`'s `spec.domain.dtype` to the engines attached through it as `--kv-cache-dtype`, verbatim, and refuse a pool-attached role or injected Pod naming that flag itself, and a new Binding declaring `auto` — see [The dtype is handed to the engine](../modules/kv-cache/pool.md#engine-dtype). **It is the escape from a Binding whose spelling the engine rejects**, which makes every new Pod fail argument parsing: `false` renders and refuses nothing, as before the flag was owned. Flipping it recreates every pool-attached replica at its next reconcile. The recovery is under [Upgrading to an enforced Binding dtype](../operate/migration/kv-cache-dtype.md). | | `model-deployment-router-image` | `GPUSTACK_MODEL_DEPLOYMENT_ROUTER_IMAGE` | `gpustack/llm-router:v0.3.1` | Image a managed router runs when the `ModelDeployment` does not name one in `spec.router.image`. **It carries every router this operator supports, and which binary runs is decided by the rendered command rather than by the image** — `spec.router.name` picks the binary, and this setting only says where the binaries come from. Its default is safe for every cluster at once in a way `kv-cache-backend-image`'s is not: it runs no model, so it links no accelerator runtime and cannot be paired with the wrong one. The default is this project's own build rather than an upstream tag, because the three routers are compiled from three separate sources and one of them carries a patch this repository ships, so no upstream image holds them together. An image named on the object is used verbatim and is never redirected to the cluster mirror, because it is the user's own reference. | | `model-deployment-router-proxy-image` | `GPUSTACK_MODEL_DEPLOYMENT_ROUTER_PROXY_IMAGE` | `gpustack/mirrored-envoy:distroless-v1.33.2` | Proxy fronting a managed router's endpoint picker. It has **no field on the API** to override it: this operator renders the proxy's configuration against one proxy's configuration schema, so swapping the binary would mean swapping that configuration too. The setting exists for registry redirection and for pinning a release back, not for running a different proxy. | | `model-deployment-routing-sidecar-image` | `GPUSTACK_MODEL_DEPLOYMENT_ROUTING_SIDECAR_IMAGE` | `gpustack/mirrored-llm-d-router-disagg-sidecar:v0.11.0` | Sidecar a decoder runs to accept a remote prefill handoff. Same terms as the proxy above: this operator renders its arguments, so the setting is for redirection and pinning rather than for a different implementation. | | `model-deployment-tcp-tw-reuse` | `GPUSTACK_MODEL_DEPLOYMENT_TCP_TW_REUSE` | `false` | Render `net.ipv4.tcp_tw_reuse=1` on the prefill half of every SGLang prefill/decode pair, which otherwise [runs out of local ports](../modules/model-deployment/engine-versions.md#known-failures-at-the-minimum) under sustained load. **Allow the sysctl on the kubelet of every node that can run such a Pod before turning it on**: without that the Pod fails with `SysctlForbidden`, and so does every replacement. The steps, which Pods it reaches and how to confirm the refusal are under [SGLang TIME-WAIT port reuse](#sglang-time-wait-port-reuse). | | `model-prefetch-warmup-image` | `GPUSTACK_MODEL_PREFETCH_WARMUP_IMAGE` | `gpustack/mirrored-python:3.12-alpine` | Image the [`ModelPrefetch`](../modules/model-delivery/prefetch.md) warm-up pod runs on each target node. The pod mounts the artifact's volume, verifies every file reads back and exits, so the image needs only a shell, `find` and `sha256sum` — it never runs a model. It is a setting because the one thing it must guarantee is that the node can pull it, which only the cluster's own registry arrangement decides; an air-gapped cluster points it at its mirror. | -| `model-artifact-huggingface-endpoint` | `GPUSTACK_MODEL_ARTIFACT_HUGGINGFACE_ENDPOINT` | `https://huggingface.co` | The Hugging Face Hub a [`ModelArtifact`](../modules/model-delivery/artifact.md) resolves and revalidates against, and the `HF_ENDPOINT` an engine downloading the weights is given; the node plugin downloads from it too. What changing it does is under [Engine delivery](../modules/model-delivery/artifact.md#engine-delivery). | -| `model-artifact-modelscope-endpoint` | `GPUSTACK_MODEL_ARTIFACT_MODELSCOPE_ENDPOINT` | `https://www.modelscope.cn` | The ModelScope hub a [`ModelArtifact`](../modules/model-delivery/artifact.md) resolves and revalidates against and the node plugin downloads from; an engine downloading the weights gets its host as `MODELSCOPE_DOMAIN` — the runners' SDK takes a bare host. What changing it does is under [Engine delivery](../modules/model-delivery/artifact.md#engine-delivery). | -| `model-artifact-https-proxy` | `GPUSTACK_MODEL_ARTIFACT_HTTPS_PROXY` | *(blank)* | HTTPS proxy for resolution, revalidation and the node plugin's downloads, and the `HTTPS_PROXY` given to an engine downloading the weights, where a role's own value wins. Blank keeps the worker's own environment and renders nothing. Only an `http` or `https` URL without credentials is accepted, because the value reaches tenant Pods; a value from the environment that is not refuses the worker's start. | -| `model-artifact-no-proxy` | `GPUSTACK_MODEL_ARTIFACT_NO_PROXY` | *(blank)* | Comma-separated hosts that bypass `model-artifact-https-proxy`, for the node plugin too, given to engines as `NO_PROXY` on the same terms. | -| `model-artifact-ca-bundle` | `GPUSTACK_MODEL_ARTIFACT_CA_BUNDLE` | *(blank)* | Name of a ConfigMap in the worker's namespace whose `ca.crt` the resolution and the node plugin trust beside the system pool. **It is not given to engine Pods**: they run in tenant namespaces, which cannot mount it. | -| `model-artifact-revalidate-interval` | `GPUSTACK_MODEL_ARTIFACT_REVALIDATE_INTERVAL` | `24h` | How often a resolved hub artifact's access is checked again, at least `1m`; a Secret change checks it at once. What a refusal does is under [Resolution and revalidation](../modules/model-delivery/artifact.md#resolution-and-revalidation). | -| `model-artifact-delivery-mode` | `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE` — *chart-rendered when the plugin is deployed; authoritative on every start while it is rendered* | `Engine` | How a hub artifact's weights reach a `ModelDeployment`: `Engine`, the engine downloads them, or `Node`, the node's [`model-manager` plugin](../modules/model-delivery/node-store.md) mounts a verified copy. The chart declares `Node` when it deploys the plugin, and the chart owns the Setting from then on: a runtime edit is reverted at the next worker start. Deploying or removing the plugin (`modelManager.enabled`) is what changes the value. `Node` is refused while the CSIDriver `model.csi.gpustack.ai` does not exist. **Changing it rolls every deployment using a hub artifact once**; see [Switch delivery](../modules/model-delivery/operations.md#switch-delivery). | +| `model-store-download-bandwidth` | `GPUSTACK_MODEL_STORE_DOWNLOAD_BANDWIDTH` | `0` | Per-node download rate limit, a quantity of bytes per second (`200Mi`); `0` is unlimited. | +| `model-store-download-concurrency` | `GPUSTACK_MODEL_STORE_DOWNLOAD_CONCURRENCY` | `8` | Concurrent HTTP requests the plugin makes per node, across every download; `1` to `64`. | | `model-store-high-watermark` | `GPUSTACK_MODEL_STORE_HIGH_WATERMARK` | `80` | The node cache filesystem's usage percent above which the plugin removes content no Pod mounts; above the low watermark, at most `95`. On a filesystem shared with kubelet the plugin caps it below kubelet's thresholds ([the capacity rule](../modules/model-delivery/operations.md#the-capacity-rule)). | | `model-store-low-watermark` | `GPUSTACK_MODEL_STORE_LOW_WATERMARK` | `70` | The usage percent a collection removes down to; at least `1`, below the high watermark. | -| `model-store-download-concurrency` | `GPUSTACK_MODEL_STORE_DOWNLOAD_CONCURRENCY` | `8` | Concurrent HTTP requests the plugin makes per node, across every download; `1` to `64`. | | `model-store-peer-sync` | `GPUSTACK_MODEL_STORE_PEER_SYNC` | `true` | Whether a node's plugin may pull a model's content from the plugins of other nodes that hold it published, instead of the hub. | -| `model-store-download-bandwidth` | `GPUSTACK_MODEL_STORE_DOWNLOAD_BANDWIDTH` | `0` | Per-node download rate limit, a quantity of bytes per second (`200Mi`); `0` is unlimited. | -| `instance-access-static-address` | `GPUSTACK_INSTANCE_ACCESS_STATIC_ADDRESS` | *(blank)* | Static access address for all Instances; when unset, the access address is generated from host IPs. | -| `instance-access-wildcard-dns` | `GPUSTACK_INSTANCE_ACCESS_WILDCARD_DNS` | *(blank)* | Wildcard DNS for all Instances (e.g. `traefik.me`), used to build a per-Instance domain `.`. Only effective when `instance-access-static-address` is not set. | -| `instance-privileged-allowed` | `GPUSTACK_INSTANCE_PRIVILEGED_ALLOWED` | `false` | Whether an Instance may request privileged mode (`spec.privileged`), which escapes the container boundary and exposes the node's devices and kernel surface. Enforced by the Instance admission webhook whenever an Instance **takes** privileged mode — at creation, or through a later change, including one made while it is stopped. An Instance that already runs privileged keeps it: with this off it stays updatable, editable while stopped, and restartable. Like every setting here, a change takes up to 30s to reach the webhook, so an Instance created in that window is judged against the previous value. | -| `instance-host-path-volume-allowed` | `GPUSTACK_INSTANCE_HOST_PATH_VOLUME_ALLOWED` | `false` | Whether an Instance may mount a hostPath volume (`spec.additionalVolumes[*].hostPath`), which reaches the node's filesystem. Kept separate from `instance-privileged-allowed` because it grants strictly less — the filesystem, but not the node's devices or kernel — so an admin can allow node-path mounts without allowing a container escape. Enforced whenever an Instance **takes** a hostPath mount, on the same terms; a mount it already has is matched by value, so reordering the list is not a new grant while repointing an entry at a different node path is. | -| `instance-persistent-volume-placement` | `GPUSTACK_INSTANCE_PERSISTENT_VOLUME_PLACEMENT` | `true` | Whether an Instance's persistent claims (`spec.volume.persistent`, `spec.additionalVolumes[*].persistent`) decide where its Pod runs, by the [claim placement rules](../modules/model-delivery/artifact.md#claim-delivery-and-placement): a bound claim adds its PV's required node affinity, and a claim nothing binds holds the Pod back with the reason in `status.phaseMessage`. **`false` is the escape path**: the Pod is built from the claim names alone, as before, and Kueue's topology-aware scheduling may then assign a node the volume cannot reach, leaving the Pod Pending while its Workload holds quota. Read when a Pod is created, so a change never moves a running Instance; stop and start one to apply it. | -| `workload-fit-affinity` | `GPUSTACK_WORKLOAD_FIT_AFFINITY` | `true` | Whether the Workload webhook pins a logical slice, or a shared request of two or more accelerators, on an operator queue to nodes whose [fit labels](../modules/devices/scheduling.md#per-card-fit-labels) show that one accelerator can still host it. When `true`, Kueue's topology-aware scheduling skips a fragmented node; when `false`, Workloads pass unchanged while the fit labels are still published, so the pin can be switched off on its own. Read on every webhook call, so a change takes up to 30s to reach it, and it applies to Workloads created or updated after that. | | `node-management-manual` | `GPUSTACK_NODE_MANAGEMENT_MANUAL` | `false` | Skip auto-managing nodes. When `false`, the operator auto-onboards discovered nodes by injecting the `gpustack.ai/managed=true` label; when `true`, an administrator must opt nodes in manually. Read per-reconcile. | -| `instance-type-mixed-on-node` | `GPUSTACK_INSTANCE_TYPE_MIXED_ON_NODE` | `true` | Whether one node may surface both a GPU and a CPU-only InstanceType. When `true`, a node is summarized into every type it can serve; when `false`, a node with accelerators yields only a GPU InstanceType and a CPU-only node only a general one. Read per-reconcile. | -| `instance-type-derived-from-node` | `GPUSTACK_INSTANCE_TYPE_DERIVED_FROM_NODE` | `true` | Whether the operator auto-derives the InstanceType (and its backing ClusterQueue) from node hardware. When `true`, the `NodeFlavorReconciler` authors the derived InstanceType (create-only); when `false`, it only aligns the ResourceFlavor and the administrator defines the InstanceType via the API, and owns feasibility with it — see [Authoring the InstanceType](#authoring-the-instancetype). Read per-reconcile. | -| `instance-type-drain-when-no-flavors` | `GPUSTACK_INSTANCE_TYPE_DRAIN_WHEN_NO_FLAVORS` | `true` | Whether a ClusterQueue whose pool has lost all its ResourceFlavors is drained (`HoldAndDrain`, so Kueue evicts admitted workloads) before its resource groups are emptied. When `true`, the queue is drained first; when `false`, the operator waits for the reservations to clear on their own, then empties. Either way the groups are emptied only once every reservation is zero, so Kueue's counters never go negative. Read per-reconcile. | -| `instance-type-aware-cpu-manufacturer` | `GPUSTACK_INSTANCE_TYPE_AWARE_CPU_MANUFACTURER` | `false` | Whether the derived ClusterQueue/InstanceType/InstanceTypeFlavor aggregation splits by CPU manufacturer. When `false`, non-accelerated flavors collapse into one `generic` pool per os/arch and accelerated flavors pool per accelerator (CPU ignored); when `true`, every pool splits by the CPU key (`gpustack--${gKey}-…` / `gpustack--${gKey}--${aKey}-…`) and the InstanceType records the raw CPU detail. The `ResourceFlavor`s themselves are unaffected — they always carry the CPU key, so a flip only re-groups the aggregation layer. Read per-reconcile. | +| `settings-default-tracking` | *(none)* | `instance-ssh-server-image`, `kv-cache-backend-image`, `model-deployment-router-image`, `model-deployment-router-proxy-image`, `model-deployment-routing-sidecar-image`, `model-prefetch-warmup-image` | Comma-separated names of the Settings the operator rewrites with its built-in default on every worker start, so that a default shipped in a later release reaches a cluster that never chose a value. A name in the list makes its Setting follow rule 3 ([who owns a value](#who-owns-a-value)); removing a name is what keeps an image pin across an operator upgrade ([keeping a pin](#keeping-an-image-pin-across-an-operator-upgrade)). An empty value tracks nothing, and whitespace, duplicates and unregistered names are dropped ([the tracking list](#the-tracking-list)). No environment variable reads this Setting, so nothing on the Deployment can take the list over. | +| `workload-fit-affinity` | `GPUSTACK_WORKLOAD_FIT_AFFINITY` | `true` | Whether the Workload webhook pins a logical slice, or a shared request of two or more accelerators, on an operator queue to nodes whose [fit labels](../modules/devices/scheduling.md#per-card-fit-labels) show that one accelerator can still host it. When `true`, Kueue's topology-aware scheduling skips a fragmented node; when `false`, Workloads pass unchanged while the fit labels are still published, so the pin can be switched off on its own. Read on every webhook call, so a change takes up to 30s to reach it, and it applies to Workloads created or updated after that. | These settings (`node-management-manual`, `instance-type-mixed-on-node`, `instance-type-derived-from-node`, `instance-type-drain-when-no-flavors`, `instance-type-aware-cpu-manufacturer`) are read **per-reconcile** From 3884394af03ac59c316347f5d6ecd5e3ee99a812 Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 19:16:31 +0800 Subject: [PATCH 09/12] fix(setting): make the align pass's guards load-bearing - the tracking fallback in trackingValue could not run, because the align pass backfills every registered key into the Secret before it reads the list; the branch and the receiver it needed are gone - the equal-value test asserted only the Secret, so removing the equal-value clause left it green; the operator's log is now captured and the test fails when the align pass logs an override it did not perform - the blank-declaration test covered only the empty string, so removing the isBlank gate left it green; the empty list and map forms are covered now Each guard was proven by deletion, and the proofs were run again against the final tree. No runtime behavior changes. The spec records the three findings, the proofs, the renamed test, and the coupling a renamed log message leaves behind. Signed-off-by: thxCode --- pkg/setting/kuberes.go | 23 ++-- pkg/setting/kuberes_test.go | 102 ++++++++++++++---- ...-settings-env-sync-and-default-tracking.md | 43 ++++++-- 3 files changed, 126 insertions(+), 42 deletions(-) diff --git a/pkg/setting/kuberes.go b/pkg/setting/kuberes.go index 1b9690cc3..8d6c81002 100644 --- a/pkg/setting/kuberes.go +++ b/pkg/setting/kuberes.go @@ -83,7 +83,7 @@ func (h Settings) Initialize(ctx context.Context, cli kubernetes.Interface) erro skip = false } } - tracking := parseTrackedSettings(h, h.trackingValue(aSec)) + tracking := parseTrackedSettings(h, trackingValue(aSec)) for name, s := range h { stored := string(aSec.Data[name]) desired, rule := s.desiredValue(tracking) @@ -140,21 +140,12 @@ func (s Setting) overrideLogPairs(stored, desired, rule string) []any { return pairs } -// trackingValue returns the tracking list, read from the live Secret and falling back to the -// registered Setting's built-in default when the Secret carries no such key. Reading the Secret -// alone would defer the built-in list to a later start on a Secret that predates the setting. -// -// The setting is registered by the release that introduces it. Until the registry carries it the -// fallback is empty, so nothing is tracked, which is the correct intermediate state rather than a -// failure to report. -func (h Settings) trackingValue(aSec *core.Secret) string { - if raw, ok := aSec.Data[DefaultTrackingName]; ok { - return string(raw) - } - if s, ok := h.Index(DefaultTrackingName); ok { - return s.defVal - } - return "" +// trackingValue returns the raw tracking list from the live Secret, or an empty string when the +// Secret carries no such key. The Secret is the only source the align pass consults, because its +// backfill has already written the built-in default into a Secret that predates the setting, so +// what the Secret holds here is the administrator's list or that default. +func trackingValue(aSec *core.Secret) string { + return string(aSec.Data[DefaultTrackingName]) } // parseTrackedSettings returns the registered settings the raw tracking list names. It splits on a diff --git a/pkg/setting/kuberes_test.go b/pkg/setting/kuberes_test.go index 25392901a..3ffbdda28 100644 --- a/pkg/setting/kuberes_test.go +++ b/pkg/setting/kuberes_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" authz "k8s.io/api/authorization/v1" @@ -12,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" k8stesting "k8s.io/client-go/testing" + klog "k8s.io/klog/v2" kubefake "gpustack.ai/gpustack/pkg/kubeclients/kubernetes/fake" ) @@ -46,19 +48,22 @@ type ( // Secret that already exists and the values it must hold afterwards. A nil stored map // means no Secret exists, so Initialize takes the creation path; a non-nil map means the // Secret exists holding exactly those keys, and a key the registry also carries is - // already present rather than backfilled. + // already present rather than backfilled. wantNoOverrideLog opts the row into asserting + // that its run logs no override line, which only a row that changes nothing can claim. initializeCase struct { - name string - settings []caseSetting - env map[string]string - stored map[string]string - want map[string]string + name string + settings []caseSetting + env map[string]string + stored map[string]string + want map[string]string + wantNoOverrideLog bool } ) // runInitializeCases runs each row through one execution loop: declare the environment, build // the registry, run Initialize against a fake clientset, then assert on the Secret the clientset -// holds. It must not be called from a parallel test, because a row declares its environment. +// holds. It must not be called from a parallel test, because a row declares its environment and +// a row that captures the log replaces the global klog logger. func runInitializeCases(t *testing.T, cases []initializeCase) { t.Helper() @@ -67,6 +72,12 @@ func runInitializeCases(t *testing.T, cases []initializeCase) { for name, value := range c.env { t.Setenv(name, value) } + var capture *logCapture + if c.wantNoOverrideLog { + capture = &logCapture{} + klog.SetLogger(logr.New(capture)) + t.Cleanup(klog.ClearLogger) + } cli := kubefake.NewSimpleClientset(delegatedSecretObjects(c.stored)...) allowSelfSubjectAccessReviews(cli) @@ -81,6 +92,12 @@ func runInitializeCases(t *testing.T, cases []initializeCase) { assert.True(t, ok, "setting %s is present", name) assert.Equal(t, want, string(got), "setting %s", name) } + if c.wantNoOverrideLog { + for _, line := range capture.lines { + assert.NotEqual(t, "setting overridden on start", line.message, + "the run logs an override line") + } + } }) } } @@ -141,6 +158,33 @@ func allowSelfSubjectAccessReviews(cli *kubefake.Clientset) { }) } +// logLine is one captured log line: its message and its key-value pairs. +type logLine struct { + message string + pairs []any +} + +// logCapture is a logr sink that records every line it receives, so a row can assert on what its +// run through Initialize logged. klog routes its log calls through the sink once a row installs +// it with klog.SetLogger. +type logCapture struct { + lines []logLine +} + +func (*logCapture) Init(logr.RuntimeInfo) {} +func (*logCapture) Enabled(int) bool { return true } + +func (l *logCapture) Info(_ int, message string, pairs ...any) { + l.lines = append(l.lines, logLine{message: message, pairs: pairs}) +} + +func (l *logCapture) Error(_ error, message string, pairs ...any) { + l.lines = append(l.lines, logLine{message: message, pairs: pairs}) +} + +func (l *logCapture) WithValues(...any) logr.LogSink { return l } +func (l *logCapture) WithName(string) logr.LogSink { return l } + const ( trackedImage = "kv-cache-backend-image" blankImage = "container-registry" @@ -229,10 +273,10 @@ func TestSettingsInitialize_BackfillsAbsentKeysIncludingBlanks(t *testing.T) { }) } -// TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent pins that the built-in tracking -// list applies on the same pass that backfills its key, so a Secret predating the setting does -// not defer the transition to a later start. -func TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent(t *testing.T) { +// TestSettingsInitialize_TrackingAppliesOnThePassThatBackfillsItsKey pins that the built-in +// tracking list applies on the same pass that backfills its key, so a Secret predating the +// setting does not defer the transition to a later start. +func TestSettingsInitialize_TrackingAppliesOnThePassThatBackfillsItsKey(t *testing.T) { runInitializeCases(t, []initializeCase{ { name: "the built-in list applies when the Secret has no tracking key", @@ -255,11 +299,12 @@ func TestSettingsInitialize_EqualValueIsNotAnOverride(t *testing.T) { const envName = "GPUSTACK_TEST_IMAGE" runInitializeCases(t, []initializeCase{ { - name: "a declared variable equal to the stored value leaves it alone", - settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1", envName: envName}}, - env: map[string]string{envName: "operator-default/image:v1"}, - stored: map[string]string{trackedImage: "operator-default/image:v1"}, - want: map[string]string{trackedImage: "operator-default/image:v1"}, + name: "a declared variable equal to the stored value leaves it alone", + settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1", envName: envName}}, + env: map[string]string{envName: "operator-default/image:v1"}, + stored: map[string]string{trackedImage: "operator-default/image:v1"}, + want: map[string]string{trackedImage: "operator-default/image:v1"}, + wantNoOverrideLog: true, }, { name: "a tracked setting already holding its default is left alone", @@ -267,15 +312,18 @@ func TestSettingsInitialize_EqualValueIsNotAnOverride(t *testing.T) { {name: trackedImage, defVal: "operator-default/image:v1"}, {name: DefaultTrackingName, defVal: trackedImage}, }, - stored: map[string]string{trackedImage: "operator-default/image:v1", DefaultTrackingName: trackedImage}, - want: map[string]string{trackedImage: "operator-default/image:v1"}, + stored: map[string]string{trackedImage: "operator-default/image:v1", DefaultTrackingName: trackedImage}, + want: map[string]string{trackedImage: "operator-default/image:v1"}, + wantNoOverrideLog: true, }, }) } // TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration pins that a variable present with -// an empty value claims nothing, even for a Setting that has a built-in default to fall back on: -// a default is substituted only for an absent variable, so the seed here is blank. +// a blank value claims nothing, even for a Setting that has a built-in default to fall back on: +// a default is substituted only for an absent variable, so the seed here is blank. Blank is the +// empty string and the empty list and map forms, each of which reads as no declaration rather +// than as a value to write over the administrator's. func TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration(t *testing.T) { const envName = "GPUSTACK_TEST_IMAGE" runInitializeCases(t, []initializeCase{ @@ -286,6 +334,20 @@ func TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration(t *testing.T) { stored: map[string]string{trackedImage: "my.pin/image:1.0"}, want: map[string]string{trackedImage: "my.pin/image:1.0"}, }, + { + name: "an empty-list declaration leaves an untracked setting alone", + settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1", envName: envName}}, + env: map[string]string{envName: "[]"}, + stored: map[string]string{trackedImage: "my.pin/image:1.0"}, + want: map[string]string{trackedImage: "my.pin/image:1.0"}, + }, + { + name: "an empty-map declaration leaves an untracked setting alone", + settings: []caseSetting{{name: trackedImage, defVal: "operator-default/image:v1", envName: envName}}, + env: map[string]string{envName: "{}"}, + stored: map[string]string{trackedImage: "my.pin/image:1.0"}, + want: map[string]string{trackedImage: "my.pin/image:1.0"}, + }, }) } diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index 37ee83530..c98c8b4dd 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -513,12 +513,15 @@ Coverage baseline for the two packages measured on 2026-10-19 with administrator value and a blank one. - `TestSettingsInitialize_BackfillsAbsentKeysIncludingBlanks`: the Secret lacks a key whose seed is blank, and the key is present afterwards, matching today's behavior. - - `TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent`: the Secret has no - `settings-default-tracking` key, and a tracked Setting follows the built-in default on that same pass. + - `TestSettingsInitialize_TrackingAppliesOnThePassThatBackfillsItsKey`: the Secret has no + `settings-default-tracking` key, and a tracked Setting follows the built-in default on that same pass. The + backfill writes the built-in default into the Secret before any rule runs, and that ordering is what makes the + list apply on that same pass. - `TestSettingsInitialize_EqualValueIsNotAnOverride`: the stored value already equals the seed, and nothing is - written and nothing is logged. - - `TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration`: the variable is present with an empty value, and - an untracked Setting keeps its stored value. + written and nothing is logged. The log capture is the only guard the equal-value clause has, because writing an + equal value is a data no-op. + - `TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration`: the variable is present with a blank value, and + an untracked Setting keeps its stored value. Blank is the empty string and the empty list and map forms. - `TestParseTrackedSettings`: whitespace around a segment, empty segments, duplicates, an unknown name, and the Setting's own name, asserting the five-step normalization and that a dropped segment does not track. - `TestSettingsInitialize_CreationSeedsEverySetting`: an empty Secret receives every registered key, blank @@ -544,6 +547,34 @@ consequence: deleting it, or replacing it with `DisallowBlank`, leaves the suite admitted by `Allow()` plus the self-reference admission either way. `AllowBlank` is kept for intent and as a guard against a stricter future admission, and its absence is not detected today. +#### Post-build corrections + +A review of the T2 commit found three places where the code or a test said something the code did not do. All +three are closed, and no runtime behavior changed. + +1. **`trackingValue` carried a fallback that could not run.** The align pass backfills every registered key into the + Secret before it reads the tracking list, so the Secret already holds the tracking key whenever the registry + carries it. The fallback branch and the receiver it needed are gone; the function reads the Secret alone. +2. **The equal-value test did not assert that it logs nothing.** The operator's log is now captured, and the test + fails when the align pass logs an override line for a value it did not change. The capture is the only guard that + clause has, because writing an equal value is a data no-op. +3. **The blank-declaration test never reached the blank gate.** The `isBlank` gate treats the empty list and the + empty map as blank, and only the empty string was covered. Both forms are covered now, and the gate is + load-bearing. + +Each correction is proven by deletion, and each proof was run again against the final tree. + +| Guard | Deleted or moved | Failing test | +|---|---|---| +| 1 | the backfill loop, moved below the rule loop | `TestSettingsInitialize_TrackingAppliesOnThePassThatBackfillsItsKey` | +| 2 | `desired == stored`, from the rule loop | `TestSettingsInitialize_EqualValueIsNotAnOverride` | +| 3 | the `isBlank` gate, from `desiredValue` | `TestSettingsInitialize_BlankDeclarationReadsAsNoDeclaration` | + +The override log line's message is pinned as a literal in both the align pass and the test, and the two are not +joined by a shared constant. A renamed message therefore needs the test updated with it, and a rename that misses +the test leaves the guard green over the change it is meant to catch. This coupling is known and accepted by +decision. + #### Integration tests None. No controller reads these keys on a path this change alters, and `Settings.Initialize` runs once at @@ -555,7 +586,7 @@ Justification for not adding one: the behavior under test is a table of writes a startup. An e2e run would assert the same rows through a slower path and would need a cluster this repository's `testing/infra` provisions per session. The rows that an e2e could add value to — a Helm upgrade replacing a pinned image Setting — is the accepted risk recorded above rather than a defect, and it is asserted by -`TestSettingsInitialize_TrackingFallsBackToDefaultWhenKeyAbsent` at the unit level. +`TestSettingsInitialize_TrackingAppliesOnThePassThatBackfillsItsKey` at the unit level. ## Alternatives From ef1f35b4973033811f9c351f90537de3235d9a9b Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 20:11:33 +0800 Subject: [PATCH 10/12] fix(setting): retire the last promises of the create-once seed - the comment on ModelArtifactDeliveryMode said a seed fills only a key the store does not have yet, which is the rule this branch retired; the chart declares the value and the worker reasserts it on every start - two cells on the model delivery operations page still called the chart's declaration a seed, and the tracking-list paragraph on the settings reference page still said the worker falls back to the six names - the test that pins the built-in list against a Secret that predates the Setting was named for a fallback the align pass no longer has; it is renamed for the path it actually covers, and the comment and the skip message follow The specification's own count of the places that promised the old model was short by one, so it now names them instead of counting them. Signed-off-by: thxCode --- docs/modules/model-delivery/operations.md | 4 ++-- docs/reference/settings.md | 5 +++-- pkg/worker/settings/model_store.go | 4 ++-- pkg/worker/settings/value_test.go | 15 ++++++++------- ...19-settings-env-sync-and-default-tracking.md | 17 +++++++++++++---- 5 files changed, 28 insertions(+), 17 deletions(-) diff --git a/docs/modules/model-delivery/operations.md b/docs/modules/model-delivery/operations.md index 845442e23..706248864 100644 --- a/docs/modules/model-delivery/operations.md +++ b/docs/modules/model-delivery/operations.md @@ -47,7 +47,7 @@ hands each mount its artifact's Secret. | Layer | Carrier | Who changes it | What | | --- | --- | --- | --- | -| Deploy time | chart values; in image mode the worker's overlay, which sets only the switch | installer | the switch, the cache root, the kubelet directory, images, resources and placement, the delivery seed | +| Deploy time | chart values; in image mode the worker's overlay, which sets only the switch | installer | the switch, the cache root, the kubelet directory, images, resources and placement, the delivery mode | | Cluster runtime | [Settings](../../reference/settings.md#online-adjustable-settings) in the operator namespace | administrator | delivery mode, Hub endpoint, proxy, no-proxy, CA bundle, watermarks, download concurrency and bandwidth | | Object | the `ModelArtifact` | tenant | source, revision, patterns, Secret; immutable | @@ -57,7 +57,7 @@ writes chooses where a node connects. The Settings this adds: | Setting | Default | Check | | --- | --- | --- | -| `model-artifact-delivery-mode` | `Engine`; the chart seeds `Node` with the plugin | `Engine` or `Node`; `Node` needs the CSIDriver | +| `model-artifact-delivery-mode` | `Engine`; the chart declares `Node` with the plugin, and the worker reasserts it | `Engine` or `Node`; `Node` needs the CSIDriver | | `model-store-high-watermark` | `80` | integer, above the low watermark, at most `95` | | `model-store-low-watermark` | `70` | integer, at least `1`, below the high watermark | | `model-store-download-concurrency` | `8` | concurrent requests per node, `1` to `64` | diff --git a/docs/reference/settings.md b/docs/reference/settings.md index 785e053e9..b02626a96 100644 --- a/docs/reference/settings.md +++ b/docs/reference/settings.md @@ -77,8 +77,9 @@ kubectl -n gpustack-system logs deploy/gpustack-operator-worker | grep 'setting ### The tracking list `settings-default-tracking` holds a comma-separated list of Setting names. The worker reads it from the -Secret and falls back to the six names the operator ships when the Secret carries no such key, so a -cluster that predates the Setting is tracked from the first start that backfills the key. +Secret, and the backfill has already written the six names the operator ships into a Secret that carries +no such key, so a cluster that predates the Setting is tracked from the first start that backfills the +key. A stored value cannot silently change what is tracked. Each segment is trimmed, and a segment left empty, a duplicate, the list's own name and a name this operator does not register are dropped, each diff --git a/pkg/worker/settings/model_store.go b/pkg/worker/settings/model_store.go index 13e23e9c9..a4500daec 100644 --- a/pkg/worker/settings/model_store.go +++ b/pkg/worker/settings/model_store.go @@ -34,8 +34,8 @@ const ( var ( // ModelArtifactDeliveryMode is how a Hugging Face artifact's weights reach a ModelDeployment's // engine. It is explicit and never inferred from whether the plugin is installed: changing it - // changes every Hugging Face deployment's Pods, which each roll once. The chart seeds "Node" - // when it deploys the plugin, and a seed fills only a key the store does not have yet. + // changes every Hugging Face deployment's Pods, which each roll once. The chart declares "Node" + // when it deploys the plugin, and the worker reasserts that declaration on every start. ModelArtifactDeliveryMode = settings.NewEditable( "model-artifact-delivery-mode", "Indicates how a Hugging Face ModelArtifact's weights reach a ModelDeployment: Engine, the engine "+ diff --git a/pkg/worker/settings/value_test.go b/pkg/worker/settings/value_test.go index 4f401146d..72dd5953d 100644 --- a/pkg/worker/settings/value_test.go +++ b/pkg/worker/settings/value_test.go @@ -281,12 +281,13 @@ func TestSettingsDefaultTracking_BlankListIsAllowed(t *testing.T) { "an administrator must be able to stop tracking everything") } -// TestSettingsDefaultTracking_FallsBackWhenKeyAbsent pins the seam between this registration and the -// align pass: on a Secret predating this Setting the key is absent, and the built-in list still -// applies, so a cluster reaches the new defaults on the upgrade rather than a start later. The -// second row clears the list and pins the other side of the same pass, so the first row cannot pass -// because the align pass rewrites those Settings whatever the list says. -func TestSettingsDefaultTracking_FallsBackWhenKeyAbsent(t *testing.T) { +// TestSettingsDefaultTracking_AppliesOnThePassThatBackfillsItsKey pins the seam between this +// registration and the align pass: on a Secret predating this Setting the key is absent, and the +// backfill writes the built-in list into the Secret before the align pass reads it, so the built-in +// list still applies and a cluster reaches the new defaults on the upgrade rather than a start +// later. The second row clears the list and pins the other side of the same pass, so the first row +// cannot pass because the align pass rewrites those Settings whatever the list says. +func TestSettingsDefaultTracking_AppliesOnThePassThatBackfillsItsKey(t *testing.T) { const driftedImage = "pinned.example/image:v0" // A declaration claims a Setting ahead of the tracking list, so a variable set in the process @@ -296,7 +297,7 @@ func TestSettingsDefaultTracking_FallsBackWhenKeyAbsent(t *testing.T) { envName := "GPUSTACK_" + stringx.UnderscoreUpper(name) if _, declared := os.LookupEnv(envName); declared { t.Skipf("%s is declared in this process environment, so the env rule claims the six "+ - "ahead of the tracking list and this case cannot observe the fallback", envName) + "ahead of the tracking list and this case cannot observe the built-in list", envName) } } diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index c98c8b4dd..c46b92b00 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -179,12 +179,14 @@ is deployed, and the code comment in `pkg/worker/settings/model_store.go` states from plugin installation. The chart's `modelManager.enabled` is the only signal that reaches the value. The cost is that an administrator who sets the Setting to `Engine` at runtime sees it revert on the next worker -start, and `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE=Node` is reinstalled. Three places promise the opposite today, so -all three change: +start, and `GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODE=Node` is reinstalled. Every place that promises the opposite +changes: [operations.md](../docs/modules/model-delivery/operations.md), -[values.yaml](../deploy/gpustack-operator/chart/values.yaml), and the comment above the variable in +[values.yaml](../deploy/gpustack-operator/chart/values.yaml), the comment above the variable in [deployment.yaml](../deploy/gpustack-operator/chart/templates/worker/deployment.yaml), which says a stored value -always wins. The template renders the same variable and the same value; only its comment changes. The +always wins, and the comment in +[model_store.go](../pkg/worker/settings/model_store.go), which says a seed fills only a key the store does not have +yet. The template renders the same variable and the same value; only its comment changes. The `model-artifact-delivery-mode` row of the Settings table carries the same promise and is restated by the task that owns that page. @@ -562,6 +564,13 @@ three are closed, and no runtime behavior changed. empty map as blank, and only the empty string was covered. Both forms are covered now, and the gate is load-bearing. +A second review of the branch found a fourth place that still promised the retired model: the comment on +`ModelArtifactDeliveryMode` in `pkg/worker/settings/model_store.go` said a seed fills only a key the store does not +have yet. The chart's declaration is reasserted on every worker start, so a stored `Engine` is overwritten. The +comment now says the chart declares the value and the worker reasserts it, and the two residual "seed" cells on the +model delivery operations page were restated in the same way. The specification's own count of the places that +promised the old model was short by one, and it named them without a count instead. + Each correction is proven by deletion, and each proof was run again against the final tree. | Guard | Deleted or moved | Failing test | From 4b29739a137e61bd67b194bbe4ed98ae1780a602 Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 20:11:34 +0800 Subject: [PATCH 11/12] docs(spec): reconcile the settings spec with the shipped code - the acceptance criteria, the code-style sketch and the core-features paragraph still described a fallback the align pass no longer has: the backfill writes the built-in default into the Secret before the list is read, and the read itself carries no fallback - the sentence that justified the fallback was false once the fallback was gone: reading only the live Secret does not defer the transition, because the backfill has already run - this spec supersedes the node model store spec in part, and states which of its statements the operator no longer follows The module map now names the settings sync among the Installation code entry points and links this spec from Model Delivery. Signed-off-by: thxCode --- docs/README.md | 4 +-- ...-settings-env-sync-and-default-tracking.md | 26 ++++++++++++------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/docs/README.md b/docs/README.md index d71c717da..f02ae3ec4 100644 --- a/docs/README.md +++ b/docs/README.md @@ -97,7 +97,7 @@ A skill link names a procedure; the entry's other fields carry shared facts. - Use for: giving model weights a stable identity; node caching and delivery; prefetch, peer sync, progress views and cache operations. - Aliases: `ModelArtifact`, `NodeModelStore`, `ModelStore`, `ModelPrefetch`, `model cache`, `Hugging Face` - Guides: [Model Delivery](modules/model-delivery/_index.md); the artifact contract in [Model Artifact](modules/model-delivery/artifact.md); the node cache in [Node Model Store](modules/model-delivery/node-store.md); the runbook in [Model Store Operations](modules/model-delivery/operations.md); a measured example in [Model Prefetch Walkthrough](walkthroughs/model-delivery/prefetch.md). -- Specs: [model artifact](../specs/2026-09-25-model-artifact.md) · [node model store](../specs/2026-09-25-node-model-store.md) · [prefetch](../specs/2026-09-27-model-prefetch.md) · [peer sync](../specs/2026-09-27-model-peer-sync.md) · [image source](../specs/2026-09-27-model-image-source.md) +- Specs: [model artifact](../specs/2026-09-25-model-artifact.md) · [node model store](../specs/2026-09-25-node-model-store.md) · [prefetch](../specs/2026-09-27-model-prefetch.md) · [peer sync](../specs/2026-09-27-model-peer-sync.md) · [image source](../specs/2026-09-27-model-image-source.md) · [settings sync semantics](../specs/2026-10-19-settings-env-sync-and-default-tracking.md) - Code: Hub resolution in [pkg/modelartifact](../pkg/modelartifact/) · the per-node plugin and cache in [pkg/modelmanager](../pkg/modelmanager/) · cache configuration in [pkg/modelstore](../pkg/modelstore/) - Related: [Model Deployment](#model-deployment) (deployments mount or download artifacts) · [KV Cache](#kv-cache) (the weight identity enters KV keys) · [GPU Instances](#gpu-instances) (an Instance can mount weights) - Skills: [gpustack-operator-e2e](../.agents/skills/gpustack-operator-e2e/SKILL.md) (node-delivery cases) @@ -141,7 +141,7 @@ A skill link names a procedure; the entry's other fields carry shared facts. - Aliases: `Helm chart`, `subchart`, `chart mode`, `image mode`, `values.yaml` - Guides: choices in [Installation Modes](operate/installation-modes.md); node drivers in [Vendor Prerequisites](getting-started/vendor-prerequisites.md); transfer in [Migrating to Bundled Subcharts](operate/migration/to-subcharts.md). - Specs: [bundled apps subchart split](../specs/2026-07-28-bundled-apps-subchart-split.md) · [settings sync semantics](../specs/2026-10-19-settings-env-sync-and-default-tracking.md) -- Code: [deploy/gpustack-operator/chart](../deploy/gpustack-operator/chart/) · vendoring in [hack/deps.sh](../hack/deps.sh) · image-mode in-cluster install in [pkg/worker/kuberess](../pkg/worker/kuberess/) +- Code: [deploy/gpustack-operator/chart](../deploy/gpustack-operator/chart/) · vendoring in [hack/deps.sh](../hack/deps.sh) · image-mode in-cluster install in [pkg/worker/kuberess](../pkg/worker/kuberess/) · the settings sync on every worker start in [pkg/setting](../pkg/setting/) and [pkg/worker/settings](../pkg/worker/settings/) - Related: [Development](#development) (chart targets and subchart patches) · [Devices Manager](#devices-manager) (the Device Manager DaemonSets the chart deploys) - Skills: [gpustack-operator-chart-e2e](../.agents/skills/gpustack-operator-chart-e2e/SKILL.md) (install, rollout and uninstall verification) · [gpustack-operator-chart-subcharts-manage](../.agents/skills/gpustack-operator-chart-subcharts-manage/SKILL.md) (add, patch or bump a vendored subchart) diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index c46b92b00..e025f10e6 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -114,9 +114,11 @@ a Setting is never written twice in one pass with two different values. Secret does not carry, exactly as today. Rule 1 and rule 2 then decide only whether to overwrite a key that already exists. Without this order a newly registered blank-default Setting would never appear. -**The tracking list is read from the live Secret, falling back to the built-in default.** On a Secret that -predates this spec the key is absent, so the built-in default applies on the same start that backfills the key. -Reading only the live Secret would defer the transition to a later start. +**The tracking list is read from the live Secret, and the backfill has already given the Secret the built-in +default.** On a Secret that predates this spec the key is absent. The backfill writes the built-in default into the +Secret before the list is read, so the built-in list applies on the same start that backfills the key. The read +carries no fallback of its own: a list the Secret does not hold is an empty list. The backfill that runs first is +what puts the default there, and a test pins that order. **Removing a variable does not restore a built-in default.** With no declaration and no tracking membership, row 3 applies and the stored value is kept, including a value a previous release declared. Unsetting a variable @@ -338,9 +340,9 @@ alignFn := func(aSec *core.Secret) (_ *core.Secret, skip bool, err error) { } } - // The tracking list is read from the live Secret, falling back to the built-in default, so a - // Secret that predates this Setting gets the default on the same start that backfills it. - tracking := parseTrackedSettings(h, h.trackingValue(aSec)) + // The tracking list is read from the live Secret. The backfill above has already given the + // Secret the built-in default when the key was absent. + tracking := parseTrackedSettings(h, trackingValue(aSec)) for name, s := range h { stored := string(aSec.Data[name]) @@ -424,9 +426,9 @@ T4 ───────────────┘ Owns: `pkg/setting/kuberes.go`, `pkg/setting/kuberes_test.go` Gate: review Acceptance: absent keys are backfilled first, so a blank-default Setting is still added; the tracking list - is read from the live Secret and falls back to the built-in default when the key is absent, and is parsed - by the five-step contract in Core Features, which takes the registry because steps four and five cannot be - evaluated without it; the name is an exported constant so the task registering the Setting cannot drift from + is read from the live Secret, which the backfill has already given the built-in default when the key was + absent, and is parsed by the five-step contract in Core Features, which takes the registry because steps + four and five cannot be evaluated without it; the name is an exported constant so the task registering the Setting cannot drift from the one reading it; `desiredValue` returns a value only for a non-blank seed that is either declared or tracked; every change to a stored value logs one `klog.InfoS` line carrying the Setting, both values, the rule and the variable, and a sensitive Setting carries neither value behind a marker instead; `TestSettingsInitialize` @@ -584,6 +586,12 @@ joined by a shared constant. A renamed message therefore needs the test updated the test leaves the guard green over the change it is meant to catch. This coupling is known and accepted by decision. +This spec supersedes [the node model store spec](2026-09-25-node-model-store.md) in part. That spec states the +create-once seeding rule in its "Seeding" paragraph, in its "Upgrading from the version before this one" paragraph, +in the heading of its settings table and in its `model-artifact-delivery-mode` row, and it states that a seed fills +only a key the cluster does not have yet. The operator no longer follows that rule. The sections of the older spec +stay as written, being the historical record of what it delivered; this spec states what replaced them. + #### Integration tests None. No controller reads these keys on a path this change alters, and `Settings.Initialize` runs once at From 88d27148617fe913f65cb1abf4f3d40f2a32fcdc Mon Sep 17 00:00:00 2001 From: thxCode Date: Mon, 19 Oct 2026 20:11:51 +0800 Subject: [PATCH 12/12] docs(spec): mark the settings sync spec shipped The spec's terminal state, recorded before the pull request opens. The "Blocked on" line no longer says the tasks are ready to build, and the open question at the end was confirmed before the build. Signed-off-by: thxCode --- specs/2026-10-19-settings-env-sync-and-default-tracking.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specs/2026-10-19-settings-env-sync-and-default-tracking.md b/specs/2026-10-19-settings-env-sync-and-default-tracking.md index e025f10e6..b4feb3c7e 100644 --- a/specs/2026-10-19-settings-env-sync-and-default-tracking.md +++ b/specs/2026-10-19-settings-env-sync-and-default-tracking.md @@ -1,7 +1,7 @@ # Spec: Settings Sync Semantics — Environment Declarations and Default Tracking -Status: Built -Blocked on: nothing. T1 to T5 are ready to build; the open question at the end is a confirmation, not a dependency. +Status: Shipped +Blocked on: nothing. T1 to T5 are built and shipped; the open question at the end was confirmed before the build. Type: Feature ## Summary