Repository navigation
feat(worker): align Settings with declarations and the tracking list on every start - #773
Merged
Merged
Conversation
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
… 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
- 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 <thxcode0824@gmail.com>
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 <thxcode0824@gmail.com>
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 10 selected item(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind enhancement
/area worker
What this PR does / why we need it:
A
GPUSTACK_*environment variable on the operator worker seeded its Setting only when the delegated Secretgpustack-settingswas created. Every later align pass added only the keys that were absent, so a variable added or corrected after the first deploy never reached its Setting, and the failure was silent.This PR replaces the create-once seed with a three-tier model evaluated on every worker start:
GPUSTACK_*variable present in the worker environment and carrying a non-blank value is written to its Setting on every start. A variable present with a blank value reads as no declaration, because a blank carries no information.settings-default-trackingSetting holds a comma-separated list of Setting names. A name in the list makes that Setting follow the operator's built-in default, so a default shipped in a later release reaches a cluster that never chose a value. Removing a name is what keeps a pin across an operator upgrade. The list ships with the six image Settings that carry a built-in default.klog.InfoSline names the Setting, the stored value, the new value and the rule that fired. A sensitive Setting carries neither value, behind one marker no value can equal.Absent keys are still backfilled before any rule runs, so a newly registered Setting with a blank default appears, and the creation path still writes every registered key.
The chart keeps
model-artifact-delivery-mode:GPUSTACK_MODEL_ARTIFACT_DELIVERY_MODEis a non-blank declaration, so the chart owns the Setting and a runtime edit reverts at the next worker start. Every place that promised the opposite now says so, including the comment on the Setting itself and the two residual "seed" cells on the model delivery operations page.Which issue(s) this PR links to:
NONE
The report came from an operator install that added
GPUSTACK_INSTANCE_ACCESS_STATIC_ADDRESSthroughworker.envand upgraded the release, and found the Setting still blank. It was not filed as an issue.Special notes for your reviewer:
specs/2026-10-19-settings-env-sync-and-default-tracking.md. It is the owning design record, and the module map indexes it under Installation and Model Delivery.specs/2026-09-25-node-model-store.md) owned the create-once rule. This spec supersedes it in part, and the older spec's sections stay as written, being the historical record of what it delivered.pkg/setting/kuberes_test.goandpkg/worker/settings/value_test.goagainst a fake clientset. Each guard is proven by deletion: moving the backfill below the rule loop, dropping the equal-value clause, and dropping theisBlankgate each turn a test red.kindest/node:v1.35.5):assert-core.shall checks pass, and CASES 1, 97, 98 and 102 pass. CASE 98 pinsmodel-artifact-delivery-modetoEngineat runtime and its Engine-delivery rows pass, which is the path this spec changes. CASE 19 skips itself without real accelerator hardware.make lintfails in this environment on the.agentsshell gate's own self-test teardown, which predates this change.make lint docs,make lint chart,golangci-lint run ./...andgo test -race ./...pass.Does this PR introduce a user-facing change?