diff --git a/docs/contributor/recipe.md b/docs/contributor/recipe.md index b59db58bf..a98a9866d 100644 --- a/docs/contributor/recipe.md +++ b/docs/contributor/recipe.md @@ -215,7 +215,7 @@ rejected. Profile-version metadata and recipe artifacts are strictly decoded so an unknown field cannot silently disappear. The core `ProfileValue` contract is closed to `advertiser`, `constraints`, -and `componentRefs{name,overrides}`. It rejects `valuesFile`, component +`readinessConstraints`, and `componentRefs{name,overrides}`. It rejects `valuesFile`, component identity/deployment fields, root `overrides.enabled`, literal dotted keys, and nested empty maps. The `advertiser` field accepts exactly one non-empty value, `external` (validated against `pkg/allocpolicy`, the canonical @@ -252,6 +252,12 @@ Resolution enforces these invariants: collisions. 5. Evaluate selected profile constraints fail closed. A missing reading has a distinct invalid-request diagnostic; other evaluator failures propagate. + A value's `readinessConstraints` are exempt from this step by design: + they name post-deployment properties (ADR-015 DD5) and route into + `spec.validation.readiness.constraints`, where the `aicr validate` + readiness pre-flight evaluates them fail closed. Names deduplicate + per phase — the same measurement path may carry a generation-time + pre-condition and a readiness-time post-deployment state. 6. Stamp the result `aicr.run/v1alpha3` and persist `metadata.selectedProfile`. Its sorted `ownedPaths` is the declaration-wide path union plus synthetic `enabled` for each referenced diff --git a/docs/design/015-recipe-configuration-profiles.md b/docs/design/015-recipe-configuration-profiles.md index c3a520d6a..15d8217e5 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -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} - 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,57 @@ 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 + 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). + + Deployment-created markers sit between the two: state the value's + own workload writes at runtime (the loaded driver a self-falsified + pre-condition asserts in post-form, a label its DaemonSet applies + after a successful install) is a legal readiness constraint as an + **outcome check** — unlike a rendered `.spec` readback it can + fail, and it observes state some deployment actually produced. It + does not establish *which* deployment produced it: the readiness + gate compares only the snapshot value, with no deployment + identity, owner, or timestamp binding, so an unversioned marker + left by an earlier deployment satisfies a later check. A + workload-written marker is therefore valid only when its producer + owns the marker's full lifecycle — clearing or versioning it when + the outcome no longer holds. And an outcome check verifies + *execution*, not *selection*: every value's own success satisfies + its own markers, so it cannot establish that the cluster's + pre-existing mode matches the selected value. A value's + **qualifying** constraint must rest on the bundle-independent + state above, whichever list it is declared in. + + 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. diff --git a/docs/integrator/recipe-development.md b/docs/integrator/recipe-development.md index bbcd3ae5c..3d86469c6 100644 --- a/docs/integrator/recipe-development.md +++ b/docs/integrator/recipe-development.md @@ -464,6 +464,32 @@ identical to a sibling's — does not support the "validated against deployed config" claim and must not be declared. The snippet above shows the declaration shape only; it is not a declaration you should copy into an overlay. +**When the distinguishing signal only exists after deployment**, declare it +under the value's `readinessConstraints` instead of `constraints`. Both lists +get the same catalog-load validation (names deduplicate per list; the same +measurement path may appear in both, carrying a pre-condition at generation +and a post-deployment state at readiness), but +`readinessConstraints` are never evaluated at generation time — they route +into `spec.validation.readiness.constraints` and are evaluated fail-closed by +the `aicr validate` readiness pre-flight. Two kinds of state belong here: +externally-grounded cluster state evaluated post-deployment (provider +properties, node labels set at provisioning), and **deployment-outcome +checks** — properties the value's own workload creates (the post-deployment +form of a self-falsified pre-condition, or a node label its DaemonSet applies +after a successful install), which a fresh deployment cannot find in the +pre-deployment snapshot that generation-time constraints are checked against. +Only the externally-grounded kind can **qualify** the value — establish that +the cluster's pre-existing mode matches the selection. An outcome check +observes post-deployment state without establishing which deployment +produced it: the readiness gate compares only the snapshot value, with no +deployment identity, owner, or timestamp binding, so a marker left by an +earlier deployment satisfies a later check. Declare a workload-written +marker only when its producer owns the marker's full lifecycle — clearing +or versioning it when the outcome no longer holds. And because every +value's own success satisfies its own markers, an outcome check can never +distinguish one value from another (ADR-015, "Self-rendered readings do +not qualify"). + **Constraint names must be measurement paths a supported snapshot producer actually emits** — a collector, or a provider projection attached at the snapshot orchestration layer (e.g. `K8s.aks-gpu-pools.gpu-driver` from @@ -473,7 +499,8 @@ snapshot orchestration layer (e.g. `K8s.aks-gpu-pools.gpu-driver` from **Paths are validated when recipe data is loaded, not when a snapshot is evaluated.** Every constraint name in `spec.constraints`, -`spec.validation.readiness.constraints`, and `spec.profile.values.*.constraints` +`spec.validation.readiness.constraints`, `spec.profile.values.*.constraints`, +and `spec.profile.values.*.readinessConstraints` is checked against the measurement catalog (`pkg/measurement/catalog.go`) as the overlay, mixin, or base file is read. A path the catalog cannot address fails the load with the file, the field, and — where there is a near match — a diff --git a/pkg/recipe/constraint_paths.go b/pkg/recipe/constraint_paths.go index be56c0fd0..b4daf954b 100644 --- a/pkg/recipe/constraint_paths.go +++ b/pkg/recipe/constraint_paths.go @@ -97,6 +97,10 @@ func validateSpecConstraintPaths(spec *RecipeMetadataSpec, source string) error if err := validateConstraintPaths(spec.Profile.Values[name].Constraints, source, location); err != nil { return err } + location = fmt.Sprintf("%s.%s.readinessConstraints", locProfileConstraints, name) + if err := validateConstraintPaths(spec.Profile.Values[name].ReadinessConstraints, source, location); err != nil { + return err + } } } diff --git a/pkg/recipe/profile.go b/pkg/recipe/profile.go index f7a8aa9b1..9bad6c1f5 100644 --- a/pkg/recipe/profile.go +++ b/pkg/recipe/profile.go @@ -66,8 +66,27 @@ 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. + // Two kinds of state legally live here (ADR-015, "Self-rendered readings + // do not qualify"): externally-grounded cluster state evaluated + // post-deployment (provider properties, provisioning-set node labels), + // and deployment-outcome checks — the post-deployment form of a + // self-falsified pre-condition, or a marker the value's own workload + // writes, which a fresh deployment cannot find in the pre-deployment + // snapshot that generation-time constraints are evaluated against. + // Only the first kind QUALIFIES the value (establishes the cluster's + // pre-existing mode matches the selection). An outcome check binds no + // deployment identity — a stale marker from an earlier deployment + // satisfies it — so declare workload-written markers only when the + // producer owns the marker's lifecycle. Same fail-closed semantics as + // Constraints once the pre-flight runs; same catalog-load validation. + ReadinessConstraints []Constraint `json:"readinessConstraints,omitempty" yaml:"readinessConstraints,omitempty"` + ComponentRefs []ProfileComponentRef `json:"componentRefs,omitempty" yaml:"componentRefs,omitempty"` } @@ -207,23 +226,43 @@ 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 { + // Name the list: the same measurement path is legal in + // both constraints and readinessConstraints (the DD5 + // pattern), so a repeat must say which list to fix. + return errors.New(errors.ErrCodeInvalidRequest, + fmt.Sprintf("profile %q value %q repeats %s %q", + decl.Name, valueName, kind, 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)) diff --git a/pkg/recipe/profile_readiness_test.go b/pkg/recipe/profile_readiness_test.go new file mode 100644 index 000000000..84876aac4 --- /dev/null +++ b/pkg/recipe/profile_readiness_test.go @@ -0,0 +1,338 @@ +// Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package recipe + +import ( + "bytes" + stderrors "errors" + "strings" + "testing" + + "gopkg.in/yaml.v3" + + aicrerrors "github.com/NVIDIA/aicr/pkg/errors" +) + +// readinessDecl builds a single-value declaration carrying both a +// generation-time constraint and the given readiness constraints. +func readinessDecl(readiness ...Constraint) *effectiveProfileDeclaration { + return &effectiveProfileDeclaration{ + Source: "test-overlay", + Declaration: &ProfileDeclaration{ + Name: "gpuStack", Default: "preinstalled", + Values: map[string]ProfileValue{ + "preinstalled": { + ComponentRefs: []ProfileComponentRef{{ + Name: "gpu-operator", + Overrides: map[string]any{"driver": map[string]any{"enabled": false}}, + }}, + Constraints: []Constraint{{Name: "Driver.gpu.mode", Value: "preinstalled"}}, + ReadinessConstraints: readiness, + }, + }, + }, + } +} + +func readinessSpec() *RecipeMetadataSpec { + return &RecipeMetadataSpec{ + ComponentRefs: []ComponentRef{{Name: "gpu-operator", Type: ComponentTypeHelm}}, + Constraints: []Constraint{{Name: "K8s.server.version", Value: ">= 1.30"}}, + } +} + +// TestValidateProfileDeclaration_ReadinessConstraints covers the catalog-load +// gate for the readiness list: same non-empty rules as generation-time +// constraints, with each list deduplicating in its own per-value namespace — +// the same measurement path may appear in both phases (the DD5 shape). +func TestValidateProfileDeclaration_ReadinessConstraints(t *testing.T) { + base := func(readiness []Constraint, generation []Constraint) *ProfileDeclaration { + return &ProfileDeclaration{ + Name: "gpuStack", Default: "a", + Values: map[string]ProfileValue{ + "a": { + ComponentRefs: []ProfileComponentRef{{ + Name: "gpu-operator", + Overrides: map[string]any{"devicePlugin": map[string]any{"enabled": true}}, + }}, + Constraints: generation, + ReadinessConstraints: readiness, + }, + }, + } + } + + tests := []struct { + name string + readiness []Constraint + generation []Constraint + wantErr string + }{ + { + name: "empty name rejected", + readiness: []Constraint{{Name: "", Value: "x"}}, + wantErr: "declares a readiness constraint with no name", + }, + { + name: "empty value rejected", + readiness: []Constraint{{Name: "NodeTopology.gpu-nodes.label", Value: ""}}, + wantErr: `readiness constraint "NodeTopology.gpu-nodes.label" has no value`, + }, + { + name: "duplicate within readiness rejected", + readiness: []Constraint{ + {Name: "NodeTopology.gpu-nodes.label", Value: "a=b"}, + {Name: "NodeTopology.gpu-nodes.label", Value: "c=d"}, + }, + wantErr: `repeats readiness constraint "NodeTopology.gpu-nodes.label"`, + }, + { + // The DD5 pattern: the same measurement path carries a + // generation-time pre-condition and a readiness-time + // post-deployment state. Phases evaluate independently, so + // cross-list reuse is legal; only within-list repeats reject. + name: "same name across phases accepted", + generation: []Constraint{{Name: "NodeTopology.gpu-nodes.label", Value: "pool-label=true"}}, + readiness: []Constraint{{Name: "NodeTopology.gpu-nodes.label", Value: "marker=true"}}, + }, + { + name: "valid readiness constraint accepted", + readiness: []Constraint{{Name: "NodeTopology.gpu-nodes.label", Value: "a=b"}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ValidateProfileDeclaration(base(tt.readiness, tt.generation)) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("ValidateProfileDeclaration() error = %v, want nil", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("ValidateProfileDeclaration() error = %v, want containing %q", err, tt.wantErr) + } + if !stderrors.Is(err, aicrerrors.New(aicrerrors.ErrCodeInvalidRequest, "")) { + t.Fatalf("ValidateProfileDeclaration() error = %v, want ErrCodeInvalidRequest", err) + } + }) + } +} + +// TestApplyEffectiveProfile_ReadinessConstraints covers resolution-time +// routing: readiness constraints reach validation.readiness.constraints, +// never spec.constraints, and are never evaluated at generation time — +// they carry either kind of readiness-scoped state (deployment-outcome +// checks or externally-grounded qualification, ADR-015), neither of which +// generation may gate on. +func TestApplyEffectiveProfile_ReadinessConstraints(t *testing.T) { + readiness := Constraint{Name: "NodeTopology.gpu-nodes.label", Value: "aicr.run/gpu-driver-owner=x"} + + t.Run("routed to validation.readiness, not spec.constraints", func(t *testing.T) { + spec := readinessSpec() + selected, err := applyEffectiveProfile(spec, readinessDecl(readiness), "", nil) + if err != nil { + t.Fatalf("applyEffectiveProfile() error = %v", err) + } + if selected == nil { + t.Fatal("applyEffectiveProfile() returned nil selection") + return + } + if spec.Validation == nil || spec.Validation.Readiness == nil { + t.Fatalf("validation.readiness not populated: %+v", spec.Validation) + return + } + got := spec.Validation.Readiness.Constraints + if len(got) != 1 || got[0].Name != readiness.Name || got[0].Value != readiness.Value { + t.Fatalf("readiness constraints = %v, want exactly %v", got, readiness) + } + for _, c := range spec.Constraints { + if c.Name == readiness.Name { + t.Fatalf("readiness constraint leaked into spec.constraints: %v", spec.Constraints) + } + } + }) + + t.Run("never evaluated at generation time", func(t *testing.T) { + spec := readinessSpec() + evaluator := func(c Constraint) ConstraintEvalResult { + if c.Name == readiness.Name { + t.Fatalf("generation-time evaluator invoked for readiness constraint %q", c.Name) + } + return ConstraintEvalResult{Passed: true} + } + if _, err := applyEffectiveProfile(spec, readinessDecl(readiness), "", evaluator); err != nil { + t.Fatalf("applyEffectiveProfile() error = %v", err) + } + }) + + t.Run("generation-time name reuse is allowed across phases", func(t *testing.T) { + // A spec-level generation constraint name may recur in readiness: + // phases evaluate and report independently (the DD5 pattern). + spec := readinessSpec() + reuse := Constraint{Name: "K8s.server.version", Value: ">= 1.32"} + if _, err := applyEffectiveProfile(spec, readinessDecl(reuse), "", nil); err != nil { + t.Fatalf("applyEffectiveProfile() error = %v, want cross-phase reuse accepted", err) + } + got := spec.Validation.Readiness.Constraints + if len(got) != 1 || got[0].Name != reuse.Name { + t.Fatalf("readiness constraints = %v, want the reused name routed to readiness", got) + } + }) + + t.Run("same name in BOTH the value's own phases routes independently", func(t *testing.T) { + // The marquee DD5 invariant: one value carries the same measurement + // path as its own generation pre-condition AND its readiness + // post-form. The two collision namespaces are independent, so gen-X + // stays in spec.Constraints (and is evaluated) while readiness-X + // routes to validation.readiness (and is not) — a future refactor + // merging the maps fails here. + spec := readinessSpec() + post := Constraint{Name: "Driver.gpu.mode", Value: "installed"} + genEvaluated := 0 + evaluator := func(c Constraint) ConstraintEvalResult { + if c.Name == post.Name { + genEvaluated++ + } + return ConstraintEvalResult{Passed: true} + } + if _, err := applyEffectiveProfile(spec, readinessDecl(post), "", evaluator); err != nil { + t.Fatalf("applyEffectiveProfile() error = %v, want same-path-in-both-phases accepted", err) + } + if genEvaluated != 1 { + t.Fatalf("generation evaluations of %q = %d, want exactly 1 (the value's own pre-condition)", + post.Name, genEvaluated) + } + var gen *Constraint + for i := range spec.Constraints { + if spec.Constraints[i].Name == post.Name { + gen = &spec.Constraints[i] + } + } + if gen == nil || gen.Value != "preinstalled" { + t.Fatalf("spec.Constraints = %v, want the value's generation pre-condition %q=preinstalled", + spec.Constraints, post.Name) + } + got := spec.Validation.Readiness.Constraints + if len(got) != 1 || got[0].Name != post.Name || got[0].Value != post.Value { + t.Fatalf("readiness constraints = %v, want exactly the post-form %v", got, post) + } + }) + + t.Run("no readiness constraints leaves nil Validation untouched", func(t *testing.T) { + spec := readinessSpec() + if spec.Validation != nil { + t.Fatal("precondition: readinessSpec must start with nil Validation") + return + } + if _, err := applyEffectiveProfile(spec, readinessDecl(), "", nil); err != nil { + t.Fatalf("applyEffectiveProfile() error = %v", err) + } + if spec.Validation != nil { + t.Fatalf("Validation = %+v, want nil (no clone, no empty Readiness phase synthesized)", spec.Validation) + } + }) + + t.Run("collision with pre-existing readiness constraint rejected", func(t *testing.T) { + spec := readinessSpec() + spec.Validation = &ValidationConfig{Readiness: &ValidationPhase{ + Constraints: []Constraint{{Name: readiness.Name, Value: "other=y"}}, + }} + _, err := applyEffectiveProfile(spec, readinessDecl(readiness), "", nil) + if err == nil || !strings.Contains(err.Error(), "collides with the composed recipe's readiness constraints") { + t.Fatalf("applyEffectiveProfile() error = %v, want readiness collision", err) + } + if !stderrors.Is(err, aicrerrors.New(aicrerrors.ErrCodeInvalidRequest, "")) { + t.Fatalf("applyEffectiveProfile() error = %v, want ErrCodeInvalidRequest", err) + } + }) + + t.Run("does not mutate an aliased ValidationConfig", func(t *testing.T) { + shared := &ValidationConfig{Readiness: &ValidationPhase{ + Constraints: []Constraint{{Name: "Deployment.gpu-operator.version", Value: ">= v24.6.0"}}, + }} + spec := readinessSpec() + spec.Validation = shared + if _, err := applyEffectiveProfile(spec, readinessDecl(readiness), "", nil); err != nil { + t.Fatalf("applyEffectiveProfile() error = %v", err) + } + if len(shared.Readiness.Constraints) != 1 { + t.Fatalf("aliased ValidationConfig mutated: %v", shared.Readiness.Constraints) + } + if len(spec.Validation.Readiness.Constraints) != 2 { + t.Fatalf("merged readiness constraints = %v, want existing + profile", spec.Validation.Readiness.Constraints) + } + }) +} + +// TestValidateSpecConstraintPaths_ProfileReadiness pins the #2126 catalog-load +// gate on the readiness list: an unaddressable path is rejected with a +// location naming the value's readinessConstraints field. +func TestValidateSpecConstraintPaths_ProfileReadiness(t *testing.T) { + spec := &RecipeMetadataSpec{ + Profile: &ProfileDeclaration{ + Name: "gpuStack", Default: "a", + Values: map[string]ProfileValue{ + "a": { + ReadinessConstraints: []Constraint{{Name: "K8s.server.versionn", Value: ">= 1.32"}}, + }, + }, + }, + } + err := validateSpecConstraintPaths(spec, "overlays/test.yaml") + if err == nil || !strings.Contains(err.Error(), "spec.profile.values.a.readinessConstraints") { + t.Fatalf("validateSpecConstraintPaths() error = %v, want readinessConstraints location", err) + } +} + +// TestProfileValueReadinessConstraintsYAMLRoundTrip pins the struct tag at +// the same strictness the catalog load path uses (KnownFields(true), see +// metadata_store.go): a typo'd tag would turn a real readinessConstraints: +// key into an unknown field and fail decode here. +func TestProfileValueReadinessConstraintsYAMLRoundTrip(t *testing.T) { + in := []byte(` +name: gpuStack +default: a +values: + a: + componentRefs: + - name: gpu-operator + overrides: + devicePlugin: {enabled: true} + constraints: + - name: Driver.gpu.mode + value: preinstalled + readinessConstraints: + - name: NodeTopology.gpu-nodes.label + value: aicr.run/gpu-driver-owner=x +`) + var decl ProfileDeclaration + decoder := yaml.NewDecoder(bytes.NewReader(in)) + decoder.KnownFields(true) + if err := decoder.Decode(&decl); err != nil { + t.Fatalf("strict decode failed: %v", err) + } + got := decl.Values["a"].ReadinessConstraints + if len(got) != 1 || got[0].Name != "NodeTopology.gpu-nodes.label" || got[0].Value != "aicr.run/gpu-driver-owner=x" { + t.Fatalf("readinessConstraints = %v, want the declared constraint", got) + } + out, err := yaml.Marshal(&decl) + if err != nil { + t.Fatalf("marshal failed: %v", err) + } + if !bytes.Contains(out, []byte("readinessConstraints:")) { + t.Fatalf("re-marshaled declaration lost the readinessConstraints key:\n%s", out) + } +} diff --git a/pkg/recipe/profile_resolution.go b/pkg/recipe/profile_resolution.go index 81e2f6780..0bf485451 100644 --- a/pkg/recipe/profile_resolution.go +++ b/pkg/recipe/profile_resolution.go @@ -227,6 +227,45 @@ func applyEffectiveProfile( return mergedSpec.Constraints[i].Name < mergedSpec.Constraints[j].Name }) + // Readiness constraints are deliberately NOT evaluated here: they name + // state that is only meaningful post-deployment (ADR-015) — a + // deployment-outcome check absent from a fresh pre-deployment snapshot, + // or externally-grounded qualification state asserted at readiness — + // so generation must not gate on them. They route into + // spec.validation.readiness.constraints, where the aicr validate + // pre-flight (checkReadiness) evaluates them with the same fail-closed + // exit as every other readiness gate. Collisions are checked against the + // readiness phase only: the same measurement path may legitimately carry + // a generation-time pre-condition AND a readiness-time post-deployment + // state (the DD5 pattern), and the phases report independently. + if len(value.ReadinessConstraints) > 0 { + // Defensive clone: both callers already hand in a deep-cloned + // Validation (initBaseMergedSpec, RecipeMetadataSpec.Merge), so + // this guards future call sites rather than a live aliasing risk. + mergedSpec.Validation = cloneValidationConfig(mergedSpec.Validation) + if mergedSpec.Validation == nil { + mergedSpec.Validation = &ValidationConfig{} + } + if mergedSpec.Validation.Readiness == nil { + mergedSpec.Validation.Readiness = &ValidationPhase{} + } + readinessNames := make(map[string]struct{}, + len(mergedSpec.Validation.Readiness.Constraints)+len(value.ReadinessConstraints)) + for _, existing := range mergedSpec.Validation.Readiness.Constraints { + readinessNames[existing.Name] = struct{}{} + } + for _, constraint := range value.ReadinessConstraints { + if _, collision := readinessNames[constraint.Name]; collision { + return nil, errors.New(errors.ErrCodeInvalidRequest, + fmt.Sprintf("profile %q value %q readiness constraint %q collides with the composed recipe's readiness constraints", + effective.Declaration.Name, valueName, constraint.Name)) + } + readinessNames[constraint.Name] = struct{}{} + mergedSpec.Validation.Readiness.Constraints = + append(mergedSpec.Validation.Readiness.Constraints, constraint) + } + } + for _, profileRef := range value.ComponentRefs { index := componentIndex[profileRef.Name] if mergedSpec.ComponentRefs[index].Overrides == nil {