-
Notifications
You must be signed in to change notification settings - Fork 91
feat(recipe): per-value readiness constraints for configuration profiles #2347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
48a87ff
6de8196
9ed09df
9d8f6b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,7 +247,9 @@ spec: | |
| componentRefs: | ||
| - name: gcp-driver-installer | ||
| overrides: | ||
| install: false # every value assigns every union path | ||
| # every value assigns every union path; nested gate — see the | ||
| # amendment on operator-selfdriver below. | ||
| installer: {enabled: false} | ||
| - name: gpu-operator | ||
| overrides: | ||
| devicePlugin: {enabled: true} | ||
|
|
@@ -262,7 +264,7 @@ spec: | |
| componentRefs: | ||
| - name: gcp-driver-installer | ||
| overrides: | ||
| install: false | ||
| installer: {enabled: false} | ||
| - name: gpu-operator | ||
| overrides: | ||
| devicePlugin: {enabled: false} | ||
|
|
@@ -277,7 +279,14 @@ spec: | |
| componentRefs: | ||
| - name: gcp-driver-installer | ||
| overrides: | ||
| install: true # the chart-level gate | ||
| # Amended 2026-08-22: the gate is the nested installer.enabled, | ||
| # not the top-level `install` originally drawn here — top-level | ||
| # `install`/`enabled` are component-PRESENCE gates (IsEnabled), | ||
| # so a false default would make the component "not enabled in | ||
| # the surviving composition" and deadlock resolution for every | ||
| # value; root `overrides.enabled` is separately rejected in | ||
| # fragments. A nested key is an ordinary owned value path. | ||
| installer: {enabled: true} | ||
|
yuanchen8911 marked this conversation as resolved.
|
||
| - name: gpu-operator | ||
| overrides: | ||
| devicePlugin: {enabled: true} | ||
|
|
@@ -511,7 +520,7 @@ to the surviving composition: | |
| # digest, so ordering must be byte-stable | ||
| # Post-DD5 state shown; the initial recording is | ||
| # gpu-operator: [devicePlugin.enabled, enabled] only. | ||
| gcp-driver-installer: [enabled, install] | ||
| gcp-driver-installer: [enabled, installer.enabled] | ||
| gpu-operator: [devicePlugin.enabled, enabled] | ||
| ``` | ||
|
|
||
|
|
@@ -1453,9 +1462,10 @@ recurrence — the shape the Problem section expects. | |
| distinguishing signal is identified (Deferred Decision 5). The other | ||
| two values do not wait on it. The dormant component and the third value | ||
| land **together**, in one event: declaring the value later is an | ||
| ownership-surface expansion (`install` joins the union and the | ||
| installer's synthetic `enabled` joins `ownedPaths`), which is a | ||
| family-wide re-qualification and evidence re-signing event. | ||
| ownership-surface expansion (`installer.enabled` joins the union, so | ||
| every existing value gains an assignment for it — the sketch above | ||
| draws that end state), which is a family-wide re-qualification and | ||
| evidence re-signing event. | ||
|
|
||
| Any dcgm-exporter GPU-ID-mapping adjustment for `csp-managed` is an | ||
| external GKE behavior not verifiable from this repository. It is | ||
|
|
@@ -1522,3 +1532,38 @@ work that resolves it. | |
| absence, so the two values stay mutually distinguishable. | ||
| **Proposed: identify a durable signal during the value's adoption; | ||
| the `operator` and `csp-managed` values do not wait on it.** | ||
|
|
||
| *Amended 2026-08-24: mechanism only.* `ProfileValue` gains | ||
| `readinessConstraints` — same catalog-load validation as `constraints` | ||
| with per-phase name deduplication (the same measurement path may carry | ||
| a generation-time pre-condition and a readiness-time post-deployment | ||
| state), routed into `spec.validation.readiness.constraints` at | ||
| resolution and **never evaluated at generation time**. The | ||
| `aicr validate` readiness pre-flight evaluates them with the same | ||
| fail-closed exit as every other readiness gate. | ||
|
|
||
| The mechanism exists for values whose distinguishers are | ||
| deployment-created — where no generation-time reading can hold. Two | ||
| rules govern its use: | ||
|
|
||
| - **The self-falsifying pre-condition trap.** Generation-time | ||
| constraints are re-evaluated by the validate pre-flight, so a | ||
| pre-condition that the value's own success erases (e.g. "no NVIDIA | ||
| driver loaded" on a value whose operator installs the driver) must | ||
| never be declared as a generation constraint — it fails every | ||
| post-deployment validate on a correctly working cluster. Such state | ||
| belongs in `readinessConstraints`, asserted in its post-deployment | ||
| form. | ||
| - **Self-rendered readings do not qualify.** A reading the selected | ||
| bundle itself renders (e.g. deployed ClusterPolicy fields) is | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor — PR-body OKE motivation contradicts the self-rendered-readings rule this PR adds The PR-body Motivation names OKE's durable distinguishers as "the deployed ClusterPolicy readings (K8s.policy.driver.enabled, K8s.policy.devicePlugin.enabled)", but the ADR rule this same PR adds (this line) disqualifies exactly that: a reading the selected bundle itself renders ("e.g. deployed ClusterPolicy fields") is satisfied by construction under every value and cannot distinguish. Verified: K8s.policy.driver.enabled reads ClusterPolicy Blast radius: None on this PR (additive, unused, byte-identical goldens; the mechanism's valid leg — the self-falsifying-precondition trap, "no driver loaded" erased by success — justifies it independently, and the ADR text is internally consistent). It's a coherence gap in the justification and a trap for the follow-up OKE PR, which will hit this rule at catalog load. Fix: In the PR body, repoint OKE's stated distinguisher at bundle-independent cluster state (a provisioning-set node label or provider property) — which the ADR carve-out and DD5.5 already prescribe. No ADR/code change needed. |
||
| satisfied by construction under every value — it is a useful | ||
| rendered-policy **drift check**, but it cannot serve as a value's | ||
| distinguishing constraint. Qualification requires cluster state | ||
| independent of the bundle's own output (provider properties, node | ||
| labels set at provisioning, externally-owned objects). | ||
|
|
||
| This PR resolves no GKE signal: the GKE family's DD5 question was | ||
| settled separately by value replacement (see the adoption-step | ||
| amendment), and its shipped values are generation-time | ||
| distinguishable. The mechanism's consumers are families whose values | ||
| are distinct cluster shapes with deployment-created distinguishers. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,8 +64,20 @@ type ProfileDeclaration struct { | |
| // metadata.selectedProfile.advertiser and extends the dual-advertisement | ||
| // gates fail-closed. Any other value is rejected. | ||
| type ProfileValue struct { | ||
| Advertiser string `json:"advertiser,omitempty" yaml:"advertiser,omitempty"` | ||
| Constraints []Constraint `json:"constraints,omitempty" yaml:"constraints,omitempty"` | ||
| Advertiser string `json:"advertiser,omitempty" yaml:"advertiser,omitempty"` | ||
| Constraints []Constraint `json:"constraints,omitempty" yaml:"constraints,omitempty"` | ||
|
|
||
| // ReadinessConstraints are evaluated only by the aicr validate readiness | ||
| // pre-flight, never at generation time: applyEffectiveProfile routes them | ||
| // into spec.validation.readiness.constraints instead of spec.constraints. | ||
| // This is the home for a value's post-deployment distinguishing signals | ||
| // (ADR-015 Deferred Decision 5) — properties a correct deployment CREATES, | ||
| // such as a node label the value's own workload applies, which therefore | ||
| // cannot exist in the pre-deployment snapshot that generation-time | ||
| // constraints are evaluated against. Same fail-closed semantics as | ||
| // Constraints once the pre-flight runs; same catalog-load validation. | ||
| ReadinessConstraints []Constraint `json:"readinessConstraints,omitempty" yaml:"readinessConstraints,omitempty"` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Nitpick — No YAML round-trip test for the readinessConstraints struct tag No test deserializes readinessConstraints from YAML, so a struct-tag typo isn't caught at this seam. Blast radius: Largely mitigated already: profiled artifacts decode under KnownFields(true), so a typo'd tag would make a real Fix: Optional round-trip assertion on a ProfileValue with |
||
|
|
||
| ComponentRefs []ProfileComponentRef `json:"componentRefs,omitempty" yaml:"componentRefs,omitempty"` | ||
| } | ||
|
|
||
|
|
@@ -205,23 +217,40 @@ func ValidateProfileDeclaration(decl *ProfileDeclaration) (map[string][]string, | |
| // constraints already fail closed on an empty name or value | ||
| // (validateConstraintWarningSource); catalog load is the equivalent | ||
| // boundary for profile-contributed ones. | ||
| seenConstraints := make(map[string]struct{}, len(value.Constraints)) | ||
| for _, constraint := range value.Constraints { | ||
| if constraint.Name == "" { | ||
| return nil, errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("profile %q value %q declares a constraint with no name", decl.Name, valueName)) | ||
| } | ||
| if constraint.Value == "" { | ||
| return nil, errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("profile %q value %q constraint %q has no value", | ||
| decl.Name, valueName, constraint.Name)) | ||
| } | ||
| if _, repeat := seenConstraints[constraint.Name]; repeat { | ||
| return nil, errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("profile %q value %q repeats constraint %q", | ||
| decl.Name, valueName, constraint.Name)) | ||
| // Each list deduplicates independently: constraint names are | ||
| // measurement paths, and the same reading legitimately appears in | ||
| // both lists of one value with different expected states — the DD5 | ||
| // pattern reads NodeTopology.gpu-nodes.label at generation (a pool | ||
| // pre-condition) AND at readiness (a post-deployment marker). The | ||
| // two lists evaluate in different phases with per-phase diagnostics, | ||
| // so cross-list reuse is unambiguous; a repeat WITHIN a list is two | ||
| // gates with one identity and stays rejected. | ||
| checkConstraints := func(constraints []Constraint, kind string) error { | ||
| seen := make(map[string]struct{}, len(constraints)) | ||
| for _, constraint := range constraints { | ||
| if constraint.Name == "" { | ||
| return errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("profile %q value %q declares a %s with no name", decl.Name, valueName, kind)) | ||
| } | ||
| if constraint.Value == "" { | ||
| return errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("profile %q value %q %s %q has no value", | ||
| decl.Name, valueName, kind, constraint.Name)) | ||
| } | ||
| if _, repeat := seen[constraint.Name]; repeat { | ||
| return errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("profile %q value %q repeats constraint %q", | ||
| decl.Name, valueName, constraint.Name)) | ||
| } | ||
| seen[constraint.Name] = struct{}{} | ||
| } | ||
| seenConstraints[constraint.Name] = struct{}{} | ||
| return nil | ||
| } | ||
| if err := checkConstraints(value.Constraints, "constraint"); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := checkConstraints(value.ReadinessConstraints, "readiness constraint"); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| seenComponents := make(map[string]struct{}, len(value.ComponentRefs)) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.