From 48a87ff164851532ee788587f475aa14e2406754 Mon Sep 17 00:00:00 2001 From: Atif Mahmood Date: Sat, 22 Aug 2026 21:12:08 -0400 Subject: [PATCH 1/7] feat(recipe): per-value readiness constraints for configuration profiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A profile value's distinguishing signal can be a post-deployment property (ADR-015 Deferred Decision 5): something the value's own deployment creates, such as a node label its installer DaemonSet applies. Such a signal cannot be declared today — profile-value constraints are evaluated at snapshot-based generation, where the property cannot yet exist, and the overlay-level readiness block cannot vary per value. Add ProfileValue.readinessConstraints: - validated at catalog load like constraints (non-empty name/value, per-list dedupe) and covered by the measurement-path gate, reported as spec.profile.values..readinessConstraints[i] - never evaluated at generation: applyEffectiveProfile routes the selected value's list into spec.validation.readiness.constraints, where the aicr validate readiness pre-flight evaluates it fail closed - names deduplicate per phase: the same measurement path may carry a generation-time pre-condition and a readiness-time post-deployment state (the DD5 shape reads NodeTopology.gpu-nodes.label in both phases) - ValidationConfig is cloned before mutation so a cached overlay's pointer is never aliased No embedded declaration uses the field yet, so resolved recipes, digests, and committed evidence are byte-identical. ADR-015 is amended with the DD5 resolution direction and a correction to the operator-selfdriver sketch: the component gate must be a nested key (installer.enabled), not top-level install, which is a component-presence gate and would deadlock resolution. Related #1716 Signed-off-by: Atif Mahmood --- docs/contributor/recipe.md | 8 +- .../015-recipe-configuration-profiles.md | 44 +++- docs/integrator/recipe-development.md | 16 +- pkg/recipe/constraint_paths.go | 4 + pkg/recipe/profile.go | 65 +++-- pkg/recipe/profile_readiness_test.go | 240 ++++++++++++++++++ pkg/recipe/profile_resolution.go | 37 +++ 7 files changed, 393 insertions(+), 21 deletions(-) create mode 100644 pkg/recipe/profile_readiness_test.go diff --git a/docs/contributor/recipe.md b/docs/contributor/recipe.md index 26af87ab8..61a185988 100644 --- a/docs/contributor/recipe.md +++ b/docs/contributor/recipe.md @@ -214,7 +214,7 @@ the legacy version, is 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 @@ -251,6 +251,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..ded260d7a 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -277,13 +277,24 @@ 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} constraints: - name: NodeTopology.gpu-nodes.label value: gke-no-default-nvidia-gpu-device-plugin=true + # Amended 2026-08-22: the DD5 distinguishing signal is declared + # under readinessConstraints (see the DD5 amendment), evaluated by + # the validate pre-flight only — it is a property the value's own + # deployment creates and cannot exist in a pre-deployment snapshot. ``` The GKE declaration lives once in `gke-cos`; accelerator/intent leaves @@ -1522,3 +1533,34 @@ 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-22 (issue #1716).* Two parts land with the value's + adoption: + + - **Mechanism.** `ProfileValue` gains `readinessConstraints` — same + catalog-load validation as `constraints` with per-phase name + deduplication (the same measurement path may carry a generation + pre-condition and a readiness post-deployment state — this is + exactly the DD5 shape, since both signals here are + `NodeTopology.gpu-nodes.label` readings), routed into + `spec.validation.readiness.constraints` at resolution and **never + evaluated at generation time**. This is required for any + post-deployment signal: generation-time evaluation runs against a + pre-deployment snapshot in which the signal cannot yet exist, and + the overlay-level readiness block cannot vary per value. The + `aicr validate` readiness pre-flight evaluates them with the same + fail-closed exit as every other readiness gate. + - **Signal.** GCP-native labels cannot distinguish the two unmanaged + values: both require identical pool shapes + (`gpu-driver-version=disabled` + the opt-out label; Google's own + installer DaemonSet schedules only where + `cloud.google.com/gke-gpu-driver-version` is absent, so a + version-labeled pool is unreachable for either). The signal is + therefore AICR-owned: the `gcp-driver-installer` DaemonSet stamps + a durable node label after a successful install; + `operator-selfdriver` asserts it under `readinessConstraints` and + `operator` (shipped name `driver-installer`) asserts its absence. + Both unmanaged values additionally gain the generation-time + constraint `!cloud.google.com/gke-gpu-driver-version`, converting + the documented "opt-out label + managed install = driverless + pool" misconfiguration into a fail-closed recipe error. diff --git a/docs/integrator/recipe-development.md b/docs/integrator/recipe-development.md index bbcd3ae5c..be3a5eef3 100644 --- a/docs/integrator/recipe-development.md +++ b/docs/integrator/recipe-development.md @@ -464,6 +464,19 @@ 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. Use this for properties the value's +own workload creates (e.g. a node label its DaemonSet applies after a +successful install — ADR-015 Deferred Decision 5), which by construction +cannot be present in the pre-deployment snapshot that generation-time +constraints are checked against. + **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 +486,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 db2f8acec..fe52edc2a 100644 --- a/pkg/recipe/profile.go +++ b/pkg/recipe/profile.go @@ -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"` + 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)) diff --git a/pkg/recipe/profile_readiness_test.go b/pkg/recipe/profile_readiness_test.go new file mode 100644 index 000000000..1cc83bb73 --- /dev/null +++ b/pkg/recipe/profile_readiness_test.go @@ -0,0 +1,240 @@ +// 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 ( + stderrors "errors" + "strings" + "testing" + + 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, one shared per-value name namespace across both lists. +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 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 name post-deployment properties absent from any pre-deployment +// snapshot (ADR-015 DD5). +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("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) + } +} diff --git a/pkg/recipe/profile_resolution.go b/pkg/recipe/profile_resolution.go index 81e2f6780..2249bdf7f 100644 --- a/pkg/recipe/profile_resolution.go +++ b/pkg/recipe/profile_resolution.go @@ -227,6 +227,43 @@ func applyEffectiveProfile( return mergedSpec.Constraints[i].Name < mergedSpec.Constraints[j].Name }) + // Readiness constraints are deliberately NOT evaluated here: they name + // post-deployment properties (ADR-015 DD5) that cannot exist in the + // pre-deployment snapshot generation evaluates against. 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 { + // Clone before mutating: mergedSpec may alias a cached overlay's + // ValidationConfig (same reason the fragment merge below deep-copies + // Overrides). + 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 { From 6de819669fecf83616635308dd7a94e5ec49e640 Mon Sep 17 00:00:00 2001 From: Atif Mahmood Date: Sun, 23 Aug 2026 00:12:31 -0400 Subject: [PATCH 2/7] docs(adr): align the operator-selfdriver sketch with the DD5 amendment Address review: the sketch now declares the readinessConstraints it claims (positive marker on operator-selfdriver, symmetric absence on operator), the deferred gke-gpu-driver-version hardening is stated as deferred rather than landing (one constraint per measurement path per phase; needs a label conjunction grammar), a stale shared-namespace test comment now describes per-phase namespaces, and fail-closed is hyphenated as a compound modifier. Signed-off-by: Atif Mahmood --- .../015-recipe-configuration-profiles.md | 33 ++++++++++++++----- docs/integrator/recipe-development.md | 2 +- pkg/recipe/profile_readiness_test.go | 3 +- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/docs/design/015-recipe-configuration-profiles.md b/docs/design/015-recipe-configuration-profiles.md index ded260d7a..95c684106 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -254,6 +254,11 @@ spec: constraints: - name: NodeTopology.gpu-nodes.label # requires #1755 value: gke-no-default-nvidia-gpu-device-plugin=true + # Amended 2026-08-22: DD5 symmetry — this value asserts the + # ABSENCE of the installer's ownership marker at readiness. + readinessConstraints: + - name: NodeTopology.gpu-nodes.label + value: "!feature.node.kubernetes.io/gcp-driver-installer" # GKE-installed driver AND GKE's managed device plugin — a # default-provisioned GKE cluster (no node label required). The # declared default: the only value satisfied with zero setup. @@ -291,10 +296,13 @@ spec: constraints: - name: NodeTopology.gpu-nodes.label value: gke-no-default-nvidia-gpu-device-plugin=true - # Amended 2026-08-22: the DD5 distinguishing signal is declared - # under readinessConstraints (see the DD5 amendment), evaluated by - # the validate pre-flight only — it is a property the value's own - # deployment creates and cannot exist in a pre-deployment snapshot. + # Amended 2026-08-22: the DD5 distinguishing signal — a property + # the value's own deployment creates, absent from any + # pre-deployment snapshot — is declared under readinessConstraints + # and evaluated by the validate pre-flight only. + readinessConstraints: + - name: NodeTopology.gpu-nodes.label + value: feature.node.kubernetes.io/gcp-driver-installer=true ``` The GKE declaration lives once in `gke-cos`; accelerator/intent leaves @@ -1559,8 +1567,15 @@ work that resolves it. therefore AICR-owned: the `gcp-driver-installer` DaemonSet stamps a durable node label after a successful install; `operator-selfdriver` asserts it under `readinessConstraints` and - `operator` (shipped name `driver-installer`) asserts its absence. - Both unmanaged values additionally gain the generation-time - constraint `!cloud.google.com/gke-gpu-driver-version`, converting - the documented "opt-out label + managed install = driverless - pool" misconfiguration into a fail-closed recipe error. + `operator` (shipped name `driver-installer`) asserts its absence — + both declarations land together with the value's adoption (the + sketch above shows the declared shape). + A further hardening — a generation-time + `!cloud.google.com/gke-gpu-driver-version` constraint on both + unmanaged values, converting the documented "opt-out label + + managed install = driverless pool" misconfiguration into a + fail-closed recipe error — is DEFERRED: a value may carry one + constraint per measurement path per phase, and + `NodeTopology.gpu-nodes.label` is already occupied at generation + by the pool-label constraint. It requires a conjunction grammar + for the label form, tracked as follow-up work. diff --git a/docs/integrator/recipe-development.md b/docs/integrator/recipe-development.md index be3a5eef3..da2706315 100644 --- a/docs/integrator/recipe-development.md +++ b/docs/integrator/recipe-development.md @@ -470,7 +470,7 @@ 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 +into `spec.validation.readiness.constraints` and are evaluated fail-closed by the `aicr validate` readiness pre-flight. Use this for properties the value's own workload creates (e.g. a node label its DaemonSet applies after a successful install — ADR-015 Deferred Decision 5), which by construction diff --git a/pkg/recipe/profile_readiness_test.go b/pkg/recipe/profile_readiness_test.go index 1cc83bb73..1f3458cd7 100644 --- a/pkg/recipe/profile_readiness_test.go +++ b/pkg/recipe/profile_readiness_test.go @@ -52,7 +52,8 @@ func readinessSpec() *RecipeMetadataSpec { // TestValidateProfileDeclaration_ReadinessConstraints covers the catalog-load // gate for the readiness list: same non-empty rules as generation-time -// constraints, one shared per-value name namespace across both lists. +// 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{ From 9ed09dfd3028f17e7acc241a3555f9e03a75d39d Mon Sep 17 00:00:00 2001 From: Atif Mahmood Date: Mon, 24 Aug 2026 14:10:10 -0400 Subject: [PATCH 3/7] docs(adr): fix sketch union totality; reframe the mechanism amendment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review: the amended example broke the union-totality rule this PR introduces — operator-selfdriver owned the nested installer.enabled while operator and csp-managed still drew the top-level install. All three drawn values now own installer.enabled, and the ownedPaths example records the nested path. The mechanism amendment no longer carries the GKE marker rationale (settled separately by value replacement) and instead records the two rules that govern readinessConstraints use: the self-falsifying pre-condition trap (a pre-condition the value's success erases must not be a generation constraint, since those are re-evaluated at validate), and that self-rendered readings (deployed ClusterPolicy fields) are drift checks, not qualification — a value's distinguishing constraint must read cluster state independent of the bundle's own output. Signed-off-by: Atif Mahmood --- .../015-recipe-configuration-profiles.md | 91 ++++++++----------- 1 file changed, 39 insertions(+), 52 deletions(-) diff --git a/docs/design/015-recipe-configuration-profiles.md b/docs/design/015-recipe-configuration-profiles.md index 95c684106..6f189b01b 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -247,18 +247,15 @@ 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} constraints: - name: NodeTopology.gpu-nodes.label # requires #1755 value: gke-no-default-nvidia-gpu-device-plugin=true - # Amended 2026-08-22: DD5 symmetry — this value asserts the - # ABSENCE of the installer's ownership marker at readiness. - readinessConstraints: - - name: NodeTopology.gpu-nodes.label - value: "!feature.node.kubernetes.io/gcp-driver-installer" # GKE-installed driver AND GKE's managed device plugin — a # default-provisioned GKE cluster (no node label required). The # declared default: the only value satisfied with zero setup. @@ -267,7 +264,7 @@ spec: componentRefs: - name: gcp-driver-installer overrides: - install: false + installer: {enabled: false} - name: gpu-operator overrides: devicePlugin: {enabled: false} @@ -296,13 +293,6 @@ spec: constraints: - name: NodeTopology.gpu-nodes.label value: gke-no-default-nvidia-gpu-device-plugin=true - # Amended 2026-08-22: the DD5 distinguishing signal — a property - # the value's own deployment creates, absent from any - # pre-deployment snapshot — is declared under readinessConstraints - # and evaluated by the validate pre-flight only. - readinessConstraints: - - name: NodeTopology.gpu-nodes.label - value: feature.node.kubernetes.io/gcp-driver-installer=true ``` The GKE declaration lives once in `gke-cos`; accelerator/intent leaves @@ -530,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] ``` @@ -1542,40 +1532,37 @@ work that resolves it. **Proposed: identify a durable signal during the value's adoption; the `operator` and `csp-managed` values do not wait on it.** - *Amended 2026-08-22 (issue #1716).* Two parts land with the value's - adoption: - - - **Mechanism.** `ProfileValue` gains `readinessConstraints` — same - catalog-load validation as `constraints` with per-phase name - deduplication (the same measurement path may carry a generation - pre-condition and a readiness post-deployment state — this is - exactly the DD5 shape, since both signals here are - `NodeTopology.gpu-nodes.label` readings), routed into - `spec.validation.readiness.constraints` at resolution and **never - evaluated at generation time**. This is required for any - post-deployment signal: generation-time evaluation runs against a - pre-deployment snapshot in which the signal cannot yet exist, and - the overlay-level readiness block cannot vary per value. The - `aicr validate` readiness pre-flight evaluates them with the same - fail-closed exit as every other readiness gate. - - **Signal.** GCP-native labels cannot distinguish the two unmanaged - values: both require identical pool shapes - (`gpu-driver-version=disabled` + the opt-out label; Google's own - installer DaemonSet schedules only where - `cloud.google.com/gke-gpu-driver-version` is absent, so a - version-labeled pool is unreachable for either). The signal is - therefore AICR-owned: the `gcp-driver-installer` DaemonSet stamps - a durable node label after a successful install; - `operator-selfdriver` asserts it under `readinessConstraints` and - `operator` (shipped name `driver-installer`) asserts its absence — - both declarations land together with the value's adoption (the - sketch above shows the declared shape). - A further hardening — a generation-time - `!cloud.google.com/gke-gpu-driver-version` constraint on both - unmanaged values, converting the documented "opt-out label + - managed install = driverless pool" misconfiguration into a - fail-closed recipe error — is DEFERRED: a value may carry one - constraint per measurement path per phase, and - `NodeTopology.gpu-nodes.label` is already occupied at generation - by the pool-label constraint. It requires a conjunction grammar - for the label form, tracked as follow-up work. + *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). + + 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. From 9d8f6b47f395269b0f3c9d2bcd639a26d6215bf6 Mon Sep 17 00:00:00 2001 From: Atif Mahmood Date: Mon, 24 Aug 2026 14:44:19 -0400 Subject: [PATCH 4/7] docs: correct the adoption-step union path to installer.enabled Signed-off-by: Atif Mahmood --- docs/design/015-recipe-configuration-profiles.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/design/015-recipe-configuration-profiles.md b/docs/design/015-recipe-configuration-profiles.md index 6f189b01b..4bd38d293 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -1462,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 From 5f5cfa190ebba4af6cd2f9ec1470cbd99e292ffa Mon Sep 17 00:00:00 2001 From: Atif Mahmood Date: Thu, 27 Aug 2026 13:42:32 -0400 Subject: [PATCH 5/7] docs: distinguish outcome checks from qualification in readiness constraints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ReadinessConstraints godoc and integrator guide presented a marker the value's own workload writes as a distinguishing signal, contradicting the ADR rule that qualification requires bundle-independent state. The contract now names both legal kinds — deployment-outcome checks (post-forms of self-falsified pre-conditions, workload-written markers: they verify execution and can fail, unlike a rendered .spec readback) and externally-grounded state — and states that only the latter qualifies a value, in the ADR mechanism section, the godoc, and the integrator guide. Signed-off-by: Atif Mahmood --- .../015-recipe-configuration-profiles.md | 12 +++++++++ docs/integrator/recipe-development.md | 17 +++++++++---- pkg/recipe/profile.go | 25 +++++++++++++------ 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/docs/design/015-recipe-configuration-profiles.md b/docs/design/015-recipe-configuration-profiles.md index 4bd38d293..08b6e1ae3 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -1562,6 +1562,18 @@ work that resolves it. 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 proves + the workload actually ran, and it can fail. But 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 diff --git a/docs/integrator/recipe-development.md b/docs/integrator/recipe-development.md index da2706315..1e3d69dfc 100644 --- a/docs/integrator/recipe-development.md +++ b/docs/integrator/recipe-development.md @@ -471,11 +471,18 @@ 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. Use this for properties the value's -own workload creates (e.g. a node label its DaemonSet applies after a -successful install — ADR-015 Deferred Decision 5), which by construction -cannot be present in the pre-deployment snapshot that generation-time -constraints are checked against. +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 by construction cannot be present 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 +verifies that the deployment executed; every value's own success satisfies +its own markers, so it 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 diff --git a/pkg/recipe/profile.go b/pkg/recipe/profile.go index fe52edc2a..cdfa408f1 100644 --- a/pkg/recipe/profile.go +++ b/pkg/recipe/profile.go @@ -70,12 +70,18 @@ type ProfileValue struct { // 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. + // 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 cannot exist 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 verifies execution, + // which every value's own success satisfies. 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"` @@ -238,9 +244,12 @@ func ValidateProfileDeclaration(decl *ProfileDeclaration) (map[string][]string, 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 constraint %q", - decl.Name, valueName, constraint.Name)) + fmt.Sprintf("profile %q value %q repeats %s %q", + decl.Name, valueName, kind, constraint.Name)) } seen[constraint.Name] = struct{}{} } From 3bb0dae14c6fe1fa5755bd4390821b47df3c243b Mon Sep 17 00:00:00 2001 From: Atif Mahmood Date: Thu, 27 Aug 2026 13:42:45 -0400 Subject: [PATCH 6/7] test: pin the DD5 both-phases invariant, tag round-trip, and repeat diagnostics Adds the same-path-in-both-phases routing test (gen pre-condition evaluated and kept in spec.constraints; readiness post-form routed unevaluated), the no-readiness negative (nil Validation stays nil), and a strict-decode YAML round-trip for the readinessConstraints tag. The duplicate-name diagnostic now names which list the repeat is in (the same path is legal in both), and the clone comment no longer overstates the aliasing risk. Signed-off-by: Atif Mahmood --- pkg/recipe/profile_readiness_test.go | 98 +++++++++++++++++++++++++++- pkg/recipe/profile_resolution.go | 6 +- 2 files changed, 100 insertions(+), 4 deletions(-) diff --git a/pkg/recipe/profile_readiness_test.go b/pkg/recipe/profile_readiness_test.go index 1f3458cd7..b544baf3e 100644 --- a/pkg/recipe/profile_readiness_test.go +++ b/pkg/recipe/profile_readiness_test.go @@ -15,10 +15,13 @@ package recipe import ( + "bytes" stderrors "errors" "strings" "testing" + "gopkg.in/yaml.v3" + aicrerrors "github.com/NVIDIA/aicr/pkg/errors" ) @@ -93,7 +96,7 @@ func TestValidateProfileDeclaration_ReadinessConstraints(t *testing.T) { {Name: "NodeTopology.gpu-nodes.label", Value: "a=b"}, {Name: "NodeTopology.gpu-nodes.label", Value: "c=d"}, }, - wantErr: `repeats constraint "NodeTopology.gpu-nodes.label"`, + wantErr: `repeats readiness constraint "NodeTopology.gpu-nodes.label"`, }, { // The DD5 pattern: the same measurement path carries a @@ -188,6 +191,59 @@ func TestApplyEffectiveProfile_ReadinessConstraints(t *testing.T) { } }) + 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{ @@ -239,3 +295,43 @@ func TestValidateSpecConstraintPaths_ProfileReadiness(t *testing.T) { 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 2249bdf7f..e772eca91 100644 --- a/pkg/recipe/profile_resolution.go +++ b/pkg/recipe/profile_resolution.go @@ -237,9 +237,9 @@ func applyEffectiveProfile( // 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 { - // Clone before mutating: mergedSpec may alias a cached overlay's - // ValidationConfig (same reason the fragment merge below deep-copies - // Overrides). + // 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{} From 49f2dbb1c6cd1e3342225c1e5933a819dc9e2aeb Mon Sep 17 00:00:00 2001 From: Atif Mahmood Date: Thu, 27 Aug 2026 17:28:05 -0400 Subject: [PATCH 7/7] docs: qualify outcome-check claims and state consumer status accurately MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An outcome check observes post-deployment state but binds no deployment identity — the readiness gate compares only the snapshot value, so a stale marker from an earlier deployment satisfies a later check; workload-written markers are valid only when their producer owns the marker lifecycle. The routing and test comments now describe both legal kinds instead of asserting universal pre-deployment absence. Signed-off-by: Atif Mahmood --- .../design/015-recipe-configuration-profiles.md | 17 ++++++++++++----- docs/integrator/recipe-development.md | 14 ++++++++++---- pkg/recipe/profile.go | 15 ++++++++------- pkg/recipe/profile_readiness_test.go | 5 +++-- pkg/recipe/profile_resolution.go | 6 ++++-- 5 files changed, 37 insertions(+), 20 deletions(-) diff --git a/docs/design/015-recipe-configuration-profiles.md b/docs/design/015-recipe-configuration-profiles.md index 08b6e1ae3..15d8217e5 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -1566,11 +1566,18 @@ work that resolves it. 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 proves - the workload actually ran, and it can fail. But 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 + **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. diff --git a/docs/integrator/recipe-development.md b/docs/integrator/recipe-development.md index 1e3d69dfc..3d86469c6 100644 --- a/docs/integrator/recipe-development.md +++ b/docs/integrator/recipe-development.md @@ -476,13 +476,19 @@ 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 by construction cannot be present in the +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 -verifies that the deployment executed; every value's own success satisfies -its own markers, so it can never distinguish one value from another (ADR-015, -"Self-rendered readings do not qualify"). +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 diff --git a/pkg/recipe/profile.go b/pkg/recipe/profile.go index cdfa408f1..d26f67174 100644 --- a/pkg/recipe/profile.go +++ b/pkg/recipe/profile.go @@ -75,13 +75,14 @@ type ProfileValue struct { // 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 cannot exist 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 verifies execution, - // which every value's own success satisfies. Same fail-closed - // semantics as Constraints once the pre-flight runs; same - // catalog-load validation. + // 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"` diff --git a/pkg/recipe/profile_readiness_test.go b/pkg/recipe/profile_readiness_test.go index b544baf3e..84876aac4 100644 --- a/pkg/recipe/profile_readiness_test.go +++ b/pkg/recipe/profile_readiness_test.go @@ -134,8 +134,9 @@ func TestValidateProfileDeclaration_ReadinessConstraints(t *testing.T) { // TestApplyEffectiveProfile_ReadinessConstraints covers resolution-time // routing: readiness constraints reach validation.readiness.constraints, // never spec.constraints, and are never evaluated at generation time — -// they name post-deployment properties absent from any pre-deployment -// snapshot (ADR-015 DD5). +// 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"} diff --git a/pkg/recipe/profile_resolution.go b/pkg/recipe/profile_resolution.go index e772eca91..0bf485451 100644 --- a/pkg/recipe/profile_resolution.go +++ b/pkg/recipe/profile_resolution.go @@ -228,8 +228,10 @@ func applyEffectiveProfile( }) // Readiness constraints are deliberately NOT evaluated here: they name - // post-deployment properties (ADR-015 DD5) that cannot exist in the - // pre-deployment snapshot generation evaluates against. They route into + // 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