From 02a3e805ba5f4f01e12faa6ee3ddefd69e34f2f8 Mon Sep 17 00:00:00 2001 From: Yuan Chen Date: Mon, 3 Aug 2026 13:59:34 -0700 Subject: [PATCH] validate: fail-closed GKE device-plugin ownership readiness check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the node-set constraint form NodeTopology.gpu-nodes.label (issue #1755): a name-dispatched evaluator in pkg/constraints that quantifies a label predicate over the snapshot's GPU-node set instead of comparing a scalar reading. The value grammar is "key=value" (every GPU node carries the label with exactly that value) or "!key" (no GPU node carries the key) -- the shape ADR-015's GKE gpuStack profile consumes unchanged when profile recipes land. The GPU-node universe is synthesized from the snapshot's existing NodeTopology.label readings: nodes carrying GKE's native cloud.google.com/gke-accelerator label, which exists from pool creation -- before the GPU Operator or NFD run. Both predicate directions fail closed on a truncated node list (snapshots captured with --max-nodes-per-entry append a "(+N more)" tail that makes set membership unprovable), on an empty GPU-node universe (a vacuous pass is the dangerous direction), and on values that parse as neither grammar form. Disambiguated label keys (key.value, emitted when a key carries multiple values across the cluster) are handled in both shapes, and mixed values fail the positive predicate naming the offending nodes. The readiness gate runs in both validator entry points: ValidatePhases and the per-phase ValidatePhase (SDK callers running a single phase must not bypass the recipe's readiness constraints), and declared readiness constraints with no snapshot to evaluate them against fail closed rather than silently skipping. Mark ADR-015 Deferred Decision 2 resolved in the design doc, note the node-set form's name-dispatched bypass of the scalar operator grammar in the contributor guide, and caution in the remediation text that --node-labels replaces the pool's full label set. Wire the check into the GKE-COS base overlay's validation.readiness.constraints -- not spec.constraints, which would exclude the GKE overlays during snapshot-based generation on exactly the unlabeled cluster the diagnostic exists to fix. checkReadiness now evaluates readiness-phase constraints alongside the top-level set (they were declared and merged but never consumed) and carries the constraint's remediation text into the failure message, so an unlabeled GKE cluster fails `aicr validate` closed (exit 2) with the device-plugin ownership diagnostic before any phase runs. Also label the UAT GKE GPU pool with gke-no-default-nvidia-gpu-device-plugin=true: the pool predated the documented prerequisite and would fail the new gate. Review rounds hardened two fail-open paths: label keys and values are validated with Kubernetes's own validators (a key no node can legally carry would make the negated predicate pass vacuously), and disambiguated-shape decoding now enforces encodeLabels' invariants (plain and disambiguated forms never coexist; genuine disambiguation yields at least two entries), so a distinct dotted label whose value equals its own suffix can no longer satisfy the predicate. The truncation detector moved next to the collector's encoder (topology.IsTruncatedNodeList) so format and detector cannot drift apart. A further round closed a hybrid-collision fail-open: the encoder's "." disambiguation can collide with a real label of that literal name (silently overwriting one reading — #2003), so an accepted disambiguated set must now partition its nodes; overlapping node sets fail closed as an ambiguous reading. Two further negated- predicate fail-opens are closed: a single disambiguated-shape entry without the plain key (possibly a collision remnant) and structurally malformed readings are rejected instead of decoding to sets the negated form passes vacuously: missing or extra separators, empty node lists, and node tokens that are not canonical Kubernetes node names (RFC 1123 subdomains) all fail closed. This resolves ADR-015 Deferred Decision 2 in place: the node-set constraint form lands under #1755, and the follow-up GKE profile recipes consume it. Fixes #1755 Refs #1761 Signed-off-by: Yuan Chen --- docs/contributor/validator.md | 34 +- .../015-recipe-configuration-profiles.md | 13 +- docs/integrator/recipe-development.md | 18 + docs/user/component-catalog.md | 4 +- docs/user/validation.md | 1 + pkg/collector/topology/topology.go | 15 + pkg/collector/topology/topology_test.go | 27 + pkg/constraints/doc.go | 8 + pkg/constraints/evaluate.go | 7 + pkg/constraints/extractor.go | 5 +- pkg/constraints/gpu_nodes.go | 382 ++++++++++ pkg/constraints/gpu_nodes_test.go | 683 ++++++++++++++++++ pkg/validator/doc.go | 3 +- pkg/validator/validator.go | 58 +- pkg/validator/validator_test.go | 197 +++++ recipes/overlays/gke-cos.yaml | 22 + tests/uat/gcp/cluster-config.yaml | 5 + 17 files changed, 1457 insertions(+), 25 deletions(-) create mode 100644 pkg/constraints/gpu_nodes.go create mode 100644 pkg/constraints/gpu_nodes_test.go diff --git a/docs/contributor/validator.md b/docs/contributor/validator.md index 4a72eb656..4383c91db 100644 --- a/docs/contributor/validator.md +++ b/docs/contributor/validator.md @@ -44,9 +44,13 @@ spec: value: ">= 450" # GB/s ``` -Top-level `constraints` are evaluated as a **pre-flight gate** before -phase checks run; phase-specific `constraints` are evaluated against -each container check's reported metrics. +Top-level `constraints` — and any declared under +`validation.readiness.constraints` — are evaluated as a **pre-flight +gate** before phase checks run; other phases' `constraints` are +evaluated against each container check's reported metrics. Readiness +placement matters for gates that must not participate in +generation-time overlay filtering (e.g. the GKE device-plugin +ownership check, issue #1755). **Supported operators** (`pkg/constraints/constraint.go`): @@ -68,6 +72,16 @@ an error (not `false`) when a value claimed to be a version fails to parse — callers in `pkg/validator/validator.go::checkReadiness` treat parse errors as `ErrCodeInvalidRequest`, fail-closed. +**One name bypasses the scalar flow entirely:** the node-set form +`NodeTopology.gpu-nodes.label` ([#1755](https://github.com/NVIDIA/aicr/issues/1755)) +is dispatched by exact name in `constraints.Evaluate` *before* +`ParseConstraintPath`, uses its own value grammar +(`=` / `!`, validated with the Kubernetes +label validators), and quantifies the predicate over the GPU-node set +synthesized from `NodeTopology.label` readings instead of comparing a +single reading. See `pkg/constraints/gpu_nodes.go` for its fail-closed +rules (truncation, empty universe, malformed or ambiguous encodings). + **Adding a new operator:** 1. Add an `Operator` constant in `pkg/constraints/constraint.go`. @@ -467,10 +481,10 @@ return; this is one of the two CLAUDE.md-sanctioned uses of `Background()`. ### Pre-flight gates are fail-closed -`pkg/validator/validator.go::checkReadiness` evaluates top-level -`validation.constraints` *before* any phase runs. A parse error or a -failing constraint returns `ErrCodeInvalidRequest` and aborts the -entire run. **Do not** `slog.Warn; continue` on an evaluator +`pkg/validator/validator.go::checkReadiness` evaluates the recipe's +top-level `constraints` plus any `validation.readiness.constraints` +*before* any phase runs. A parse error or a failing constraint returns +`ErrCodeInvalidRequest` and aborts the entire run. **Do not** `slog.Warn; continue` on an evaluator error — that masquerades a broken validation YAML as a passing constraint, which is an explicit anti-pattern in CLAUDE.md. @@ -1060,6 +1074,12 @@ assert budget (`TestExpectedResourcesCatalogEnvelope` guards this). `pkg/constraints` is shared by surface 1, surface 2's recipe constraints, and the readiness pre-flight gate. The evaluation flow: +0. **Name dispatch.** `constraints.Evaluate` first matches the + constraint name against the node-set form + `NodeTopology.gpu-nodes.label` + ([#1755](https://github.com/NVIDIA/aicr/issues/1755)), which has its + own value grammar and evaluator and never reaches the steps below. + Every other name proceeds through the scalar flow. 1. **Parse.** `ParseConstraintExpression(expr)` strips whitespace, finds the **longest** matching operator prefix (so `>=` wins over `>`), splits into `{Operator, Value}`. Empty value → `ErrCodeInvalidRequest`. diff --git a/docs/design/015-recipe-configuration-profiles.md b/docs/design/015-recipe-configuration-profiles.md index 2b67bd73d..03b899669 100644 --- a/docs/design/015-recipe-configuration-profiles.md +++ b/docs/design/015-recipe-configuration-profiles.md @@ -1424,10 +1424,15 @@ work that resolves it. diagnostic, or a distinguishable "reading unavailable — regenerate the snapshot"? Both fail closed; only the second is actionable. **Proposed: distinguish.** -2. **#1755 scope confirmation.** This ADR reads #1755 as delivering the - node-set constraint *form* (every GPU node has label X, including - the negated form) — a new reading/evaluator capability. **Proposed: - confirm during GKE adoption; the GKE consumer is gated on it.** +2. **#1755 scope confirmation — resolved by PR #2000.** This ADR reads + #1755 as delivering the node-set constraint *form* (every GPU node + has label X, including the negated form) — a new reading/evaluator + capability. **Resolved: confirmed.** The form + (`NodeTopology.gpu-nodes.label`, `pkg/constraints`) landed under + #1755 with both predicate directions and the fail-closed semantics + this ADR's acceptance requirements specify. Today the GKE overlays + declare it directly under readiness constraints; the GKE `gpuStack` + profile will consume it unchanged when that profile lands (#1761). 3. **AKS node-pool-mode signal — resolved by the 2026-07-27 amendment.** The provider-facing AgentPool `gpuProfile.driver` property is the durable ownership marker. AKS adoption projects it into a snapshot diff --git a/docs/integrator/recipe-development.md b/docs/integrator/recipe-development.md index d9e4464ac..f566c104f 100644 --- a/docs/integrator/recipe-development.md +++ b/docs/integrator/recipe-development.md @@ -483,6 +483,24 @@ Do not borrow paths from `validation.deployment.constraints`, such as evaluated against a live cluster, not snapshot readings, and `Deployment` is not a measurement type. +**One name is a node-set form, not a reading path:** +`NodeTopology.gpu-nodes.label` +([#1755](https://github.com/NVIDIA/aicr/issues/1755)). No snapshot producer +emits a `gpu-nodes` subtype; the evaluator synthesizes the GPU-node set from +the snapshot's `NodeTopology.label` readings (nodes carrying +`cloud.google.com/gke-accelerator`) and quantifies a label predicate over it. +Its value grammar is also not the operator grammar: +`=` asserts every GPU node carries the label with exactly +that value, and `!` asserts no GPU node carries the key. Both +directions fail closed on a truncated node list (a snapshot captured with +`--max-nodes-per-entry` whose cap actually truncated a participating +reading), on an empty GPU-node universe, and on malformed or ambiguous +label readings (an encoding collision between a disambiguated entry and a +distinct dotted label name — see #2003). Declare it under +`validation.readiness.constraints`, not `spec.constraints` — as a top-level +constraint it would exclude the overlay during snapshot-based generation on +the very cluster the diagnostic exists to fix. + **Which signal qualifies a driver-ownership profile depends on the service.** The example above names none, which is why it is shape only. `GPU.hardware` readings do not settle it: `driver-loaded` proves a driver is *present*, not diff --git a/docs/user/component-catalog.md b/docs/user/component-catalog.md index 4ba828ec3..90a2648bf 100644 --- a/docs/user/component-catalog.md +++ b/docs/user/component-catalog.md @@ -168,7 +168,9 @@ Both settings are required, and they cover different halves of the GPU stack: The label controls device-plugin ownership only; it does not affect driver provisioning. -**AICR has no deterministic check for a violation today.** `aicr bundle` is offline by design and cannot read node labels; `aicr validate` has no constraint form that can express "every GPU node carries this label". The operator-health deployment check passes because it verifies only that GPU Operator controller pods are Running — it never inspects the device plugin. Allocation probes such as `check-nvidia-smi` schedule a pod requesting `nvidia.com/gpu` on each schedulable GPU node, but skip cordoned nodes and skip entirely when any schedulable GPU node is busy; when they do run, they may fail nondeterministically without identifying the missing label as the cause. Elsewhere the conflict surfaces only as nondeterministic workload failures. A fail-closed validation check is tracked in [#1755](https://github.com/NVIDIA/aicr/issues/1755). +**`aicr validate` enforces this prerequisite deterministically, before any phase runs.** The GKE recipes declare a readiness constraint (`NodeTopology.gpu-nodes.label`, [#1755](https://github.com/NVIDIA/aicr/issues/1755)) requiring every GPU node — identified by its `cloud.google.com/gke-accelerator` label — to carry `gke-no-default-nvidia-gpu-device-plugin=true`. The check fails closed: missing or mixed labels, an empty GPU-node set, and readings that `--max-nodes-per-entry` actually truncated (a cap larger than the node count truncates nothing and validates normally) all fail validation with exit 2 and remediation text pointing back at this section, before any check Jobs deploy. See [Validation](validation.md) for the readiness-gate mechanics. + +The readiness gate is the only deterministic detection point. `aicr bundle` is offline by design and cannot read node labels. The operator-health deployment check passes under the conflict because it verifies only that GPU Operator controller pods are Running — it never inspects the device plugin. Allocation probes such as `check-nvidia-smi` schedule a pod requesting `nvidia.com/gpu` on each schedulable GPU node, but skip cordoned nodes and skip entirely when any schedulable GPU node is busy; when they do run, they may fail nondeterministically without identifying the missing label as the cause. See GKE's [GPU node-pool guide](https://cloud.google.com/kubernetes-engine/docs/how-to/gpus) for the authoritative pool-creation procedure. The [NVIDIA GPU Operator GKE guide](https://docs.nvidia.com/datacenter/cloud-native/gpu-operator/latest/google-gke.html) documents a **different mode** — `gpu-driver-version=disabled` plus a manually applied COS driver-installer DaemonSet. AICR's GKE recipes support only the GKE-managed driver install shown above; the manual-installer mode is not supported until profile-based recipes land, and is tracked separately in [#1716](https://github.com/NVIDIA/aicr/issues/1716). diff --git a/docs/user/validation.md b/docs/user/validation.md index 54e3145bd..0bf8fa5be 100644 --- a/docs/user/validation.md +++ b/docs/user/validation.md @@ -38,6 +38,7 @@ any phase. If pre-flight fails, no validator Jobs are deployed. - `kubectl` configured for the target cluster (validator dispatches K8s Jobs; pre-flight only needs the snapshot). - Cluster service account with RBAC to create Jobs, ConfigMaps, and read cluster state (AICR creates its own `aicr-validation` namespace on first run). - **AKS profiled recipes**: the readiness pre-flight re-evaluates the recipe's profile constraint (`K8s.aks-gpu-pools.gpu-driver`), so the snapshot must carry that reading — capture it with `aicr snapshot --aks-gpu-pools `, or pass the same flag to `aicr validate` when it captures live. A snapshot without the reading fails readiness closed (exit 2). +- **GKE recipes**: the readiness pre-flight requires every GPU node (nodes carrying `cloud.google.com/gke-accelerator`) to have the label `gke-no-default-nvidia-gpu-device-plugin=true` — without it, GKE's managed device plugin conflicts with the GPU Operator's plugin over `nvidia.com/gpu` ownership. The check fails closed (exit 2) on missing or mixed labels, on malformed or ambiguous label readings, on a snapshot with no identifiable GPU nodes, and when `--max-nodes-per-entry` actually truncated a participating label reading (a truncated node list cannot prove set membership — regenerate without the flag; a cap larger than the node count truncates nothing and validates normally). ## Training performance validation diff --git a/pkg/collector/topology/topology.go b/pkg/collector/topology/topology.go index df395b941..6a73afbaf 100644 --- a/pkg/collector/topology/topology.go +++ b/pkg/collector/topology/topology.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "log/slog" + "regexp" "sort" "strings" @@ -185,6 +186,20 @@ func encodeLabels(labels map[labelID][]string, maxNodes int) map[string]measurem return data } +// truncatedNodeListRE matches the suffix formatNodeList appends when a node +// list is truncated. Kept next to formatNodeList so the format and its +// detector cannot drift apart; consumers that must fail closed on truncated +// membership lists (pkg/constraints' node-set form, issue #1755) call +// IsTruncatedNodeList instead of re-encoding this knowledge. A structured +// marker is tracked in #2002. +var truncatedNodeListRE = regexp.MustCompile(`\(\+\d+ more\)$`) + +// IsTruncatedNodeList reports whether an encoded node list carries the +// truncation suffix formatNodeList appends under --max-nodes-per-entry. +func IsTruncatedNodeList(nodes string) bool { + return truncatedNodeListRE.MatchString(nodes) +} + // formatNodeList joins sorted node names with commas, optionally truncating. func formatNodeList(nodes []string, maxNodes int) string { if maxNodes > 0 && len(nodes) > maxNodes { diff --git a/pkg/collector/topology/topology_test.go b/pkg/collector/topology/topology_test.go index ab9232965..2f4fb85ba 100644 --- a/pkg/collector/topology/topology_test.go +++ b/pkg/collector/topology/topology_test.go @@ -457,3 +457,30 @@ func TestLabelEncoding(t *testing.T) { t.Errorf("nodes = %q, want worker-1", parts[1]) } } + +// TestIsTruncatedNodeListRoundTrip pins the truncation detector to +// formatNodeList's actual output, so a change to the suffix wording breaks +// this test instead of silently failing open in consumers that must reject +// truncated membership lists (pkg/constraints' node-set form, issue #1755). +func TestIsTruncatedNodeListRoundTrip(t *testing.T) { + nodes := []string{"node-a", "node-b", "node-c"} + + tests := []struct { + name string + maxNodes int + wantTruncated bool + }{ + {"truncated below count", 2, true}, + {"no limit", 0, false}, + {"limit equals count", 3, false}, + {"limit above count", 4, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + encoded := formatNodeList(nodes, tt.maxNodes) + if got := IsTruncatedNodeList(encoded); got != tt.wantTruncated { + t.Errorf("IsTruncatedNodeList(%q) = %v, want %v", encoded, got, tt.wantTruncated) + } + }) + } +} diff --git a/pkg/constraints/doc.go b/pkg/constraints/doc.go index ac4a76ace..a8cbbce09 100644 --- a/pkg/constraints/doc.go +++ b/pkg/constraints/doc.go @@ -29,6 +29,14 @@ // reading, and reports a Result describing whether the constraint // passed and why. // +// One constraint name carries a non-scalar form: the node-set constraint +// GPUNodesLabelConstraintName ("NodeTopology.gpu-nodes.label", issue #1755) +// quantifies a label predicate over the snapshot's GPU-node set instead of +// comparing a single reading. Its value grammar is "key=value" (every GPU +// node carries the label) or "!key" (no GPU node carries the key); it is +// dispatched by exact name before the scalar path and fails closed on +// truncated node lists and on an empty GPU-node universe. +// // Evaluation is deliberately side-effect free and never performs network // or cluster I/O; consumers (pkg/validator, pkg/recipe) supply the // snapshot context. diff --git a/pkg/constraints/evaluate.go b/pkg/constraints/evaluate.go index 9098a0f04..976ccfec9 100644 --- a/pkg/constraints/evaluate.go +++ b/pkg/constraints/evaluate.go @@ -38,6 +38,13 @@ type EvalResult struct { // Used by the recipe package to filter overlays based on constraint // evaluation during snapshot-based recipe generation. func Evaluate(constraint recipe.Constraint, snap *snapshotter.Snapshot) EvalResult { + // The node-set form dispatches by exact name before the scalar path: its + // value grammar (key=value / !key) is not the operator grammar, and its + // name is a virtual path no snapshot producer emits directly. + if constraint.Name == GPUNodesLabelConstraintName { + return evaluateGPUNodesLabel(constraint.Value, snap) + } + result := EvalResult{} path, err := ParseConstraintPath(constraint.Name) diff --git a/pkg/constraints/extractor.go b/pkg/constraints/extractor.go index 21df58833..6da75c401 100644 --- a/pkg/constraints/extractor.go +++ b/pkg/constraints/extractor.go @@ -28,6 +28,7 @@ import ( const ( keyType = "type" keyPath = "path" + keyKey = "key" keySubtype = "subtype" keySelector = "selector" ) @@ -259,7 +260,7 @@ func (cp *ConstraintPath) ExtractValue(snap *snapshotter.Snapshot) (string, erro if !exists { return "", errors.NewWithContext(errors.ErrCodeNotFound, "key not found in subtype", - map[string]any{"key": cp.Key, keySubtype: cp.Subtype, keyType: cp.Type}) + map[string]any{keyKey: cp.Key, keySubtype: cp.Subtype, keyType: cp.Type}) } // Convert reading to string @@ -334,5 +335,5 @@ func lookupInItem(item *measurement.ItemEntry, cp *ConstraintPath) (string, erro } return "", errors.NewWithContext(errors.ErrCodeNotFound, "key not found in item", - map[string]any{"key": cp.Key, keySubtype: cp.Subtype, keyType: cp.Type, keySelector: cp.Selector.Raw}) + map[string]any{keyKey: cp.Key, keySubtype: cp.Subtype, keyType: cp.Type, keySelector: cp.Selector.Raw}) } diff --git a/pkg/constraints/gpu_nodes.go b/pkg/constraints/gpu_nodes.go new file mode 100644 index 000000000..733aa2fd7 --- /dev/null +++ b/pkg/constraints/gpu_nodes.go @@ -0,0 +1,382 @@ +// 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 constraints + +import ( + "fmt" + "sort" + "strings" + + "k8s.io/apimachinery/pkg/api/validate/content" + "k8s.io/apimachinery/pkg/util/validation" + + "github.com/NVIDIA/aicr/pkg/collector/topology" + "github.com/NVIDIA/aicr/pkg/errors" + "github.com/NVIDIA/aicr/pkg/measurement" + "github.com/NVIDIA/aicr/pkg/snapshotter" +) + +// GPUNodesLabelConstraintName is the node-set constraint form from issue +// #1755 (declared in the GKE overlays' readiness constraints today; ADR-015's +// GKE gpuStack profile will consume it when that profile lands — #1761). +// Unlike scalar +// constraint paths, it does not name a reading the snapshot carries +// directly; the evaluator synthesizes the GPU-node set from the snapshot's +// NodeTopology.label readings and quantifies the value predicate over it. +// +// Value grammar (distinct from the scalar operator grammar): +// +// key=value every GPU node carries label key with exactly this value +// (an empty value — "key=" — is valid; Kubernetes permits it) +// !key no GPU node carries label key (any value) +// +// Both directions fail closed: on truncated node lists (snapshots taken +// with --max-nodes-per-entry), on an empty GPU-node universe, and on a +// value that parses as neither form. +const GPUNodesLabelConstraintName = "NodeTopology.gpu-nodes.label" + +// gpuNodeUniverseLabel defines the authoritative GPU-node universe: nodes +// carrying GKE's native accelerator label. It is present on GKE GPU nodes +// from pool creation — before the GPU Operator or NFD run — which is +// exactly the pre-deployment cluster this constraint form validates. +// (NFD's nvidia.com/gpu.* labels do not exist on such a cluster.) +const gpuNodeUniverseLabel = "cloud.google.com/gke-accelerator" + +// maxReportedNodes caps the node names named in a failure message. +const maxReportedNodes = 5 + +// ctxValue / ctxConstraint key structured error context entries. +const ( + ctxValue = "value" + ctxConstraint = "constraint" + ctxReading = "reading" +) + +// labelNodeSet is one decoded NodeTopology.label entry: the label value, +// the set of nodes carrying it, the raw reading key it came from, and +// whether its node list was truncated by the collector. +type labelNodeSet struct { + value string + nodes []string + raw string + truncated bool +} + +// evaluateGPUNodesLabel evaluates the node-set constraint form against the +// snapshot's NodeTopology.label readings. See GPUNodesLabelConstraintName +// for the grammar and fail-closed semantics. +func evaluateGPUNodesLabel(value string, snap *snapshotter.Snapshot) EvalResult { + key, want, negated, err := parseGPUNodesLabelValue(value) + if err != nil { + return EvalResult{Error: err} + } + + labels := findLabelSubtype(snap) + if labels == nil { + return EvalResult{Error: errors.New(errors.ErrCodeNotFound, + "snapshot carries no NodeTopology label readings — re-capture with a current aicr build and verify the snapshot agent can list nodes")} + } + + universeEntries, err := decodeLabelEntries(labels.Data, gpuNodeUniverseLabel) + if err != nil { + return EvalResult{Error: err} + } + universe := make(map[string]bool) + for _, e := range universeEntries { + for _, n := range e.nodes { + universe[n] = true + } + } + if len(universe) == 0 { + // An empty universe must fail closed, never satisfy either predicate + // vacuously (#1755 acceptance requirement 2). + return EvalResult{Error: errors.NewWithContext(errors.ErrCodeNotFound, + fmt.Sprintf("GPU-node universe is empty: snapshot has no %q label readings — "+ + "the constraint cannot be evaluated on a cluster without identifiable GPU nodes", gpuNodeUniverseLabel), + map[string]any{ctxConstraint: GPUNodesLabelConstraintName})} + } + + targetEntries, err := decodeLabelEntries(labels.Data, key) + if err != nil { + return EvalResult{Error: err} + } + + if negated { + return evaluateNoGPUNodeHasKey(key, universe, targetEntries) + } + return evaluateEveryGPUNodeHasValue(key, want, universe, targetEntries) +} + +// parseGPUNodesLabelValue parses the node-set value grammar: "key=value" +// (positive) or "!key" (negated). Anything else is rejected — in particular +// the scalar operator grammar (">= x", "!= x") is not valid here. Keys and +// values are validated with Kubernetes's own label validators: a key no +// node can legally carry (e.g. a double slash) would otherwise make the +// negated predicate pass vacuously — the fail-open direction. +func parseGPUNodesLabelValue(raw string) (key, want string, negated bool, err error) { + v := strings.TrimSpace(raw) + if after, ok := strings.CutPrefix(v, "!"); ok { + key = strings.TrimSpace(after) + if errs := content.IsLabelKey(key); len(errs) > 0 { + return "", "", false, errors.NewWithContext(errors.ErrCodeInvalidRequest, + "invalid node-set constraint value: negated form is \"!\" with a valid label key and no value", + map[string]any{ctxValue: raw, "key_errors": strings.Join(errs, "; ")}) + } + return key, "", true, nil + } + // "key=" (empty value) is deliberately valid: Kubernetes permits empty + // label values and the collector encodes them ("|"; the + // disambiguated map key "." cannot collide with a real label key, + // which may not end in a dot). + key, want, ok := strings.Cut(v, "=") + if !ok { + return "", "", false, errors.NewWithContext(errors.ErrCodeInvalidRequest, + "invalid node-set constraint value: expected \"=\" or \"!\"", + map[string]any{ctxValue: raw}) + } + if errs := append(content.IsLabelKey(key), content.IsLabelValue(want)...); len(errs) > 0 { + return "", "", false, errors.NewWithContext(errors.ErrCodeInvalidRequest, + "invalid node-set constraint value: label key or value is not valid", + map[string]any{ctxValue: raw, "errors": strings.Join(errs, "; ")}) + } + return key, want, false, nil +} + +// findLabelSubtype returns the NodeTopology label subtype, or nil when the +// snapshot does not carry it. +func findLabelSubtype(snap *snapshotter.Snapshot) *measurement.Subtype { + if snap == nil { + return nil + } + for _, m := range snap.Measurements { + if m == nil || m.Type != measurement.TypeNodeTopology { + continue + } + return m.GetSubtype("label") + } + return nil +} + +// decodeLabelEntries collects the decoded entries for one label key from the +// topology collector's encoding ("|"). It handles +// both encoded shapes: the plain key, and the "." disambiguation +// encodeLabels applies when the key carries multiple distinct values across +// the cluster. +// +// The encoding is lossy: a *different* label key literally named +// "." whose value is "" produces the same map entry as a genuine +// disambiguated reading of . Counting such an entry would let e.g. +// ".true=true" satisfy "=true" — fail-open. encodeLabels gives us +// two invariants to reject impostors with: a disambiguated key never +// coexists with its plain form, and genuine disambiguation always yields at +// least two entries (it happens only when the key carries ≥2 distinct +// values). Prefixed matches are therefore accepted only when the plain key +// is absent AND two or more prefixed entries match. Exactly ONE matching +// prefixed entry with the plain key absent is an ambiguous shape and fails +// closed: it is either a distinct dotted label (harmless) or the surviving +// remnant of a collision that overwrote the key's other disambiguated +// entries — and treating it as the former lets the negated predicate pass +// while the key is genuinely present on GPU nodes. +// +// An accepted set is additionally required to partition its nodes: one node +// carries exactly one value of one label, so overlapping node sets prove the +// map entries collided — encodeLabels writes a genuine disambiguated entry +// and a distinct label named "." to the same map key, one silently +// overwriting the other by map iteration order. When the distinct label +// wins, its node list can cover nodes whose genuine value differs, which +// would pass the positive predicate on a mixed cluster — fail-open. Overlap +// therefore fails closed as an ambiguous reading. The residual ambiguity +// (colliding entries with identical node sets, ≥2 distinct dotted labels +// forming a clean partition with the plain key absent, or every +// disambiguated entry overwritten by value≠suffix distinct labels) is what +// the lossy encoding cannot express; the durable fix is a lossless +// collector encoding (#2003). +// +// Any accepted entry whose node list is truncated fails closed (a partial +// membership list can falsely satisfy both predicate directions — #1755 +// acceptance requirement 1), and a structurally malformed reading is +// rejected rather than decoded: no "|" separator, more than one "|", an +// empty node list, or a node token that is not a canonical Kubernetes node +// name (RFC 1123 subdomain — rejects whitespace, empties, and embedded +// separators). A malformed token would otherwise never equal a real +// universe member, letting the negated predicate pass vacuously. Truncated +// readings skip token validation — their "(+N more)" tail is not a node +// name by design — and keep their distinct truncation diagnostic. +func decodeLabelEntries(data map[string]measurement.Reading, key string) ([]labelNodeSet, error) { + prefix := key + "." + var plain, prefixed []labelNodeSet + for k, reading := range data { + suffix, hasPrefix := strings.CutPrefix(k, prefix) + if k != key && !hasPrefix { + continue + } + raw := reading.String() + value, nodesRaw, wellFormed := cutLabelEncoding(raw) + if k != key && value != suffix { + continue // different label key sharing the dotted prefix + } + truncated := topology.IsTruncatedNodeList(nodesRaw) + var nodes []string + ok := wellFormed && strings.Count(raw, "|") == 1 + if ok && !truncated { + nodes, ok = splitNodes(nodesRaw) + } + if !ok { + return nil, errors.NewWithContext(errors.ErrCodeInvalidRequest, + fmt.Sprintf("label reading %q is malformed (%q) — expected \"|\" with "+ + "canonical node names; regenerate the snapshot with a current aicr build", k, raw), + map[string]any{ctxConstraint: GPUNodesLabelConstraintName, ctxReading: k}) + } + entry := labelNodeSet{value: value, nodes: nodes, raw: k, truncated: truncated} + if k == key { + plain = append(plain, entry) + } else { + prefixed = append(prefixed, entry) + } + } + + entries := plain + if len(plain) == 0 && len(prefixed) == 1 { + return nil, errors.NewWithContext(errors.ErrCodeInvalidRequest, + fmt.Sprintf("label readings for %q are ambiguous: a single disambiguated-shape entry %q exists without "+ + "the plain key — either a distinct label sharing the dotted name, or the remnant of an encoding "+ + "collision that overwrote the key's other entries (#2003); rename the conflicting label or use a "+ + "lossless snapshot encoding", key, prefixed[0].raw), + map[string]any{ctxConstraint: GPUNodesLabelConstraintName, keyKey: key, ctxReading: prefixed[0].raw}) + } + if len(plain) == 0 && len(prefixed) >= 2 { + seen := make(map[string]bool) + for _, e := range prefixed { + for _, n := range e.nodes { + if seen[n] { + return nil, errors.NewWithContext(errors.ErrCodeInvalidRequest, + fmt.Sprintf("label readings for %q are ambiguous: node %q appears under two values, so a "+ + "distinct dotted label collided with the disambiguated encoding (reading %q is one of "+ + "the colliding parties); rename the conflicting label or use a lossless snapshot "+ + "encoding", key, n, e.raw), + map[string]any{ctxConstraint: GPUNodesLabelConstraintName, keyKey: key, "node": n}) + } + seen[n] = true + } + } + entries = prefixed + } + for _, e := range entries { + if e.truncated { + return nil, errors.NewWithContext(errors.ErrCodeInvalidRequest, + fmt.Sprintf("label reading %q is truncated — node-set constraints cannot be evaluated on a partial "+ + "node list; regenerate the snapshot without --max-nodes-per-entry", e.raw), + map[string]any{ctxConstraint: GPUNodesLabelConstraintName, ctxReading: e.raw}) + } + } + return entries, nil +} + +// splitNodes splits the comma-joined node list. It reports ok=false for an +// empty list or any member that is not a canonical Kubernetes node name +// (RFC 1123 subdomain) — shapes the collector never emits for a present +// label. A non-canonical token (embedded "|", surrounding whitespace, +// empty) can never equal a real universe member, so decoding it would +// vacuously pass the negated predicate. +func splitNodes(nodesRaw string) ([]string, bool) { + if nodesRaw == "" { + return nil, false + } + nodes := strings.Split(nodesRaw, ",") + for _, n := range nodes { + if len(validation.IsDNS1123Subdomain(n)) > 0 { + return nil, false + } + } + return nodes, true +} + +// cutLabelEncoding splits the topology collector's "|" +// encoding at the first pipe. Kubernetes label values cannot contain "|", +// so the first pipe is always the separator; a reading without one is +// malformed (ok=false). +func cutLabelEncoding(raw string) (value, nodes string, ok bool) { + return strings.Cut(raw, "|") +} + +// evaluateEveryGPUNodeHasValue passes when every node in the GPU universe +// carries the target label with exactly the wanted value. A node carrying a +// different value, or not carrying the label at all, fails. +func evaluateEveryGPUNodeHasValue(key, want string, universe map[string]bool, entries []labelNodeSet) EvalResult { + matched := make(map[string]bool) + for _, e := range entries { + if e.value != want { + continue + } + for _, n := range e.nodes { + matched[n] = true + } + } + + var missing []string + for n := range universe { + if !matched[n] { + missing = append(missing, n) + } + } + if len(missing) == 0 { + return EvalResult{ + Passed: true, + Actual: fmt.Sprintf("all %d GPU node(s) carry %s=%s", len(universe), key, want), + } + } + sort.Strings(missing) + return EvalResult{ + Actual: fmt.Sprintf("%d of %d GPU node(s) missing %s=%s: %s", + len(missing), len(universe), key, want, summarizeNodes(missing)), + } +} + +// evaluateNoGPUNodeHasKey passes when no node in the GPU universe carries +// the target label key with any value. +func evaluateNoGPUNodeHasKey(key string, universe map[string]bool, entries []labelNodeSet) EvalResult { + offenders := make(map[string]bool) + for _, e := range entries { + for _, n := range e.nodes { + if universe[n] { + offenders[n] = true + } + } + } + if len(offenders) == 0 { + return EvalResult{ + Passed: true, + Actual: fmt.Sprintf("none of %d GPU node(s) carry label %s", len(universe), key), + } + } + names := make([]string, 0, len(offenders)) + for n := range offenders { + names = append(names, n) + } + sort.Strings(names) + return EvalResult{ + Actual: fmt.Sprintf("%d of %d GPU node(s) carry label %s: %s", + len(names), len(universe), key, summarizeNodes(names)), + } +} + +// summarizeNodes renders a sorted node list capped at maxReportedNodes. +func summarizeNodes(nodes []string) string { + if len(nodes) <= maxReportedNodes { + return strings.Join(nodes, ",") + } + return strings.Join(nodes[:maxReportedNodes], ",") + + fmt.Sprintf(" (+%d more)", len(nodes)-maxReportedNodes) +} diff --git a/pkg/constraints/gpu_nodes_test.go b/pkg/constraints/gpu_nodes_test.go new file mode 100644 index 000000000..7360f8066 --- /dev/null +++ b/pkg/constraints/gpu_nodes_test.go @@ -0,0 +1,683 @@ +// 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 constraints + +import ( + "context" + stderrors "errors" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + + "github.com/NVIDIA/aicr/pkg/collector/topology" + "github.com/NVIDIA/aicr/pkg/errors" + "github.com/NVIDIA/aicr/pkg/measurement" + "github.com/NVIDIA/aicr/pkg/recipe" + "github.com/NVIDIA/aicr/pkg/snapshotter" +) + +const optOutLabel = "gke-no-default-nvidia-gpu-device-plugin" + +// topologySnapshot builds a snapshot carrying a NodeTopology label subtype +// with the given encoded entries (key -> "value|node1,node2,..."). +func topologySnapshot(labels map[string]string) *snapshotter.Snapshot { + data := make(map[string]measurement.Reading, len(labels)) + for k, v := range labels { + data[k] = measurement.Str(v) + } + return &snapshotter.Snapshot{ + Measurements: []*measurement.Measurement{ + { + Type: measurement.TypeNodeTopology, + Subtypes: []measurement.Subtype{ + {Name: "label", Data: data}, + }, + }, + }, + } +} + +// TestEvaluateGPUNodesLabel pins the node-set constraint form's semantics +// (issue #1755): every-node and no-node predicates over the GPU universe +// derived from cloud.google.com/gke-accelerator readings, failing closed on +// truncation, empty universes, and malformed values — in both directions. +func TestEvaluateGPUNodesLabel(t *testing.T) { + t.Parallel() + + positive := optOutLabel + "=true" + negated := "!" + optOutLabel + + tests := []struct { + name string + value string + labels map[string]string + wantPassed bool + wantCode errors.ErrorCode // "" means no error expected + wantActual string // substring match; "" skips the check + }{ + { + name: "positive passes when every GPU node is labeled", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel: "true|gpu-a,gpu-b", + }, + wantPassed: true, + wantActual: "all 2 GPU node(s)", + }, + { + name: "positive fails when one GPU node is unlabeled", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b,gpu-c", + optOutLabel: "true|gpu-a,gpu-c", + }, + wantPassed: false, + wantActual: "1 of 3 GPU node(s) missing " + optOutLabel + "=true: gpu-b", + }, + { + name: "positive fails when the label is absent entirely", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + }, + wantPassed: false, + wantActual: "1 of 1 GPU node(s) missing", + }, + { + name: "positive fails on mixed values via disambiguated keys", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel + ".true": "true|gpu-a", + optOutLabel + ".false": "false|gpu-b", + }, + wantPassed: false, + wantActual: "gpu-b", + }, + { + name: "positive passes across multiple accelerator types", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator.nvidia-h100-80gb": "nvidia-h100-80gb|gpu-a", + "cloud.google.com/gke-accelerator.nvidia-l4": "nvidia-l4|gpu-b", + optOutLabel: "true|gpu-a,gpu-b", + }, + wantPassed: true, + }, + { + name: "positive ignores non-GPU nodes", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true|gpu-a,system-b", + }, + wantPassed: true, + wantActual: "all 1 GPU node(s)", + }, + { + name: "negated passes when no GPU node carries the key", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + }, + wantPassed: true, + wantActual: "none of 2 GPU node(s)", + }, + { + name: "negated fails when a GPU node carries the key", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel: "true|gpu-b", + }, + wantPassed: false, + wantActual: "1 of 2 GPU node(s) carry label " + optOutLabel + ": gpu-b", + }, + { + name: "negated ignores the key on non-GPU nodes", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true|system-b", + }, + wantPassed: true, + }, + { + name: "empty GPU universe fails closed for the positive form", + value: positive, + labels: map[string]string{ + optOutLabel: "true|node-a", + }, + wantCode: errors.ErrCodeNotFound, + }, + { + name: "empty GPU universe fails closed for the negated form", + value: negated, + labels: map[string]string{ + "kubernetes.io/os": "linux|node-a", + }, + wantCode: errors.ErrCodeNotFound, + }, + { + name: "truncated universe reading fails closed", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b (+3 more)", + optOutLabel: "true|gpu-a,gpu-b", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "truncated target reading fails closed for the positive form", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel: "true|gpu-a (+1 more)", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "truncated target reading fails closed for the negated form", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel: "true|gpu-a (+1 more)", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "disambiguated prefix from a different label key is not misattributed", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + // A distinct label key that happens to extend the target key + // with a dot: its decoded value ("on") does not equal the key + // suffix ("mode"), so it must not count as the target label. + optOutLabel + ".mode": "on|gpu-a", + }, + wantPassed: true, + }, + { + name: "single dotted-shape entry without the plain key is ambiguous", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + // A lone ".true"="true" entry with the plain key absent + // is either a distinct label or the surviving remnant of an + // encoding collision — indistinguishable, so it must error, + // never be counted or silently skipped. + optOutLabel + ".true": "true|gpu-a", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "collision remnant cannot vacuously pass the negated predicate", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + // Codex repro: real key mixed (true on gpu-a, false on gpu-b) + // while a distinct label ".true"=other overwrote the + // genuine ".true" entry. The visible state is one + // discarded value!=suffix entry plus a single remnant — the + // remnant must fail closed, not be skipped (skipping would + // pass !key while the key is on both GPU nodes). + optOutLabel + ".true": "other|system-a", + optOutLabel + ".false": "false|gpu-b", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "malformed target reading without separator fails closed for the negated form", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "malformed target reading without separator fails closed for the positive form", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "empty node list fails closed", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true|", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "empty node member fails closed", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true|gpu-a,,gpu-b", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "extra separator in target reading fails closed for the negated form", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + // Cut at the first pipe leaves node token "gpu-a|junk", + // which can never equal a real universe member — decoding it + // would vacuously pass the negated predicate. + optOutLabel: "true|gpu-a|junk", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "whitespace node token fails closed for the negated form", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true| gpu-a", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "non-canonical node token fails closed for the positive form", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true|GPU-A", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "malformed universe reading fails closed", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb", + optOutLabel: "true|gpu-a", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "plain key present means prefixed entries are distinct labels", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel: "true|gpu-a", + // encodeLabels never emits plain and disambiguated shapes for + // one key, so this entry is a distinct label: gpu-b stays + // unlabeled and the predicate fails. + optOutLabel + ".true": "true|gpu-b", + }, + wantPassed: false, + wantActual: "gpu-b", + }, + { + name: "single dotted universe entry without the plain key is ambiguous", + value: positive, + labels: map[string]string{ + // No plain gke-accelerator reading and only one prefixed + // entry — distinct label or collision remnant, so the + // universe cannot be trusted; fail closed. + "cloud.google.com/gke-accelerator.foo": "foo|node-a", + optOutLabel: "true|node-a", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "hybrid encoding collision fails closed on overlapping node sets", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + // The real key is mixed (true on gpu-a, false on gpu-b) AND a + // distinct label named ".true" exists on both nodes. + // encodeLabels writes both to the map key ".true"; when + // the distinct label wins, gpu-b appears under both "true" + // and "false" — an impossible partition. Must error, never + // pass (gpu-b genuinely carries key=false). + optOutLabel + ".true": "true|gpu-a,gpu-b", + optOutLabel + ".false": "false|gpu-b", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "hybrid collision fails closed for the negated form too", + value: negated, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel + ".true": "true|gpu-a,gpu-b", + optOutLabel + ".false": "false|gpu-b", + }, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "dotted label value through the disambiguated shape", + value: optOutLabel + "=a.b", + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + // Genuine disambiguation with dotted values: suffix equality + // must compare the full remainder after the key prefix. + optOutLabel + ".a.b": "a.b|gpu-a", + optOutLabel + ".c.d": "c.d|gpu-b", + }, + wantPassed: false, + wantActual: "1 of 2 GPU node(s) missing " + optOutLabel + "=a.b: gpu-b", + }, + { + name: "truncated entry of a different dotted key is skipped, not an error", + value: positive, + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a", + optOutLabel: "true|gpu-a", + // Shares the dotted prefix but is a different key (value != + // suffix); its truncation must not fail the evaluation + // because the entry never participates. + optOutLabel + ".mode": "on|gpu-a,gpu-b (+3 more)", + }, + wantPassed: true, + }, + { + name: "negated form with an illegal double-slash key is rejected", + value: "!invalid//label", + labels: map[string]string{"cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a"}, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "positive form with an illegal key is rejected", + value: "invalid//label=true", + labels: map[string]string{"cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a"}, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "scalar operator grammar is rejected", + value: ">= true", + labels: map[string]string{"cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a"}, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "bare key without value is rejected", + value: optOutLabel, + labels: map[string]string{"cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a"}, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + name: "negated form with a value is rejected", + value: "!" + optOutLabel + "=true", + labels: map[string]string{"cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a"}, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + // Valid key, invalid label value: IsLabelValue must be the sole + // rejecting predicate (the key passes IsLabelKey). + name: "positive form with an invalid label value is rejected", + value: optOutLabel + "=bad value", + labels: map[string]string{"cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a"}, + wantCode: errors.ErrCodeInvalidRequest, + }, + { + // Kubernetes permits empty label values and the collector + // encodes them ("|"), so "key=" is a valid positive + // form: every GPU node must carry the key with an empty value. + name: "empty-value form passes when every GPU node carries the key with an empty value", + value: optOutLabel + "=", + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel: "|gpu-a,gpu-b", + }, + wantPassed: true, + wantActual: "all 2 GPU node(s)", + }, + { + // Mixed empty/non-empty values arrive disambiguated; the + // empty-value entry's map key is "." (suffix ""), which no + // real label key can collide with (keys may not end in a dot). + name: "empty-value form fails on mixed values via disambiguated keys", + value: optOutLabel + "=", + labels: map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb|gpu-a,gpu-b", + optOutLabel + ".": "|gpu-a", + optOutLabel + ".true": "true|gpu-b", + }, + wantPassed: false, + wantActual: "gpu-b", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + c := recipe.Constraint{Name: GPUNodesLabelConstraintName, Value: tt.value} + result := Evaluate(c, topologySnapshot(tt.labels)) + + if tt.wantCode != "" { + if result.Error == nil { + t.Fatalf("expected error with code %s, got none (passed=%v, actual=%q)", + tt.wantCode, result.Passed, result.Actual) + } + if !stderrors.Is(result.Error, errors.New(tt.wantCode, "")) { + t.Fatalf("error code = %v, want %s", result.Error, tt.wantCode) + } + return + } + if result.Error != nil { + t.Fatalf("unexpected error: %v", result.Error) + } + if result.Passed != tt.wantPassed { + t.Errorf("Passed = %v, want %v (actual=%q)", result.Passed, tt.wantPassed, result.Actual) + } + if tt.wantActual != "" && !strings.Contains(result.Actual, tt.wantActual) { + t.Errorf("Actual = %q, want substring %q", result.Actual, tt.wantActual) + } + }) + } +} + +// TestEvaluateGPUNodesLabelUnavailableReadings pins the fail-closed behavior +// when the snapshot carries no NodeTopology label subtype at all. +func TestEvaluateGPUNodesLabelUnavailableReadings(t *testing.T) { + t.Parallel() + + c := recipe.Constraint{Name: GPUNodesLabelConstraintName, Value: optOutLabel + "=true"} + for _, tt := range []struct { + name string + snap *snapshotter.Snapshot + }{ + {"nil snapshot", nil}, + {"no NodeTopology measurement", evalSnapshot()}, + {"NodeTopology without label subtype", &snapshotter.Snapshot{ + Measurements: []*measurement.Measurement{ + {Type: measurement.TypeNodeTopology, Subtypes: []measurement.Subtype{{Name: "summary"}}}, + }, + }}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := Evaluate(c, tt.snap) + if result.Error == nil { + t.Fatalf("expected error, got passed=%v actual=%q", result.Passed, result.Actual) + } + if !stderrors.Is(result.Error, errors.New(errors.ErrCodeNotFound, "")) { + t.Fatalf("error code = %v, want %s", result.Error, errors.ErrCodeNotFound) + } + }) + } +} + +// TestSummarizeNodesCapsList pins the failure-message cap so a large cluster +// does not dump hundreds of node names into a diagnostic — including the +// exact-cap boundary, where no "(+N more)" suffix may appear. +func TestSummarizeNodesCapsList(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + nodes []string + want string + }{ + {"over cap", []string{"n1", "n2", "n3", "n4", "n5", "n6", "n7"}, "n1,n2,n3,n4,n5 (+2 more)"}, + {"one over cap", []string{"n1", "n2", "n3", "n4", "n5", "n6"}, "n1,n2,n3,n4,n5 (+1 more)"}, + {"exactly at cap", []string{"n1", "n2", "n3", "n4", "n5"}, "n1,n2,n3,n4,n5"}, + {"single node", []string{"n1"}, "n1"}, + {"nil input", nil, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := summarizeNodes(tt.nodes); got != tt.want { + t.Errorf("summarizeNodes(%v) = %q, want %q", tt.nodes, got, tt.want) + } + }) + } +} + +// TestGPUNodesLabelRoundTripsCollectorEncoding pins the cross-package wire +// format: real topology.Collector output (not hand-built strings) must +// decode and evaluate correctly, so a collector-side format change — a +// separator swap, a different disambiguation trigger — cannot silently +// break the decoder or fail it closed on healthy snapshots. Covers the +// plain, disambiguated, and truncated encoded shapes. The lossless-encoding +// rework (#2003) is the durable resolution; until then this test is the +// contract. +func TestGPUNodesLabelRoundTripsCollectorEncoding(t *testing.T) { + t.Parallel() + + node := func(name string, labels map[string]string) *corev1.Node { + return &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: name, Labels: labels}} + } + gpuLabels := func(optOut string) map[string]string { + l := map[string]string{"cloud.google.com/gke-accelerator": "nvidia-h100-80gb"} + if optOut != "" { + l[optOutLabel] = optOut + } + return l + } + collect := func(t *testing.T, maxNodes int, nodes ...*corev1.Node) *snapshotter.Snapshot { + t.Helper() + objects := make([]runtime.Object, len(nodes)) + for i, n := range nodes { + objects[i] = n + } + c := &topology.Collector{ + ClientSet: fake.NewClientset(objects...), + MaxNodesPerEntry: maxNodes, + } + m, err := c.Collect(context.Background()) + if err != nil { + t.Fatalf("Collect() failed: %v", err) + } + return &snapshotter.Snapshot{Measurements: []*measurement.Measurement{m}} + } + eval := func(snap *snapshotter.Snapshot, value string) EvalResult { + return Evaluate(recipe.Constraint{Name: GPUNodesLabelConstraintName, Value: value}, snap) + } + + t.Run("plain shape: uniform value passes the positive form", func(t *testing.T) { + t.Parallel() + snap := collect(t, 0, + node("gpu-a", gpuLabels("true")), + node("gpu-b", gpuLabels("true"))) + result := eval(snap, optOutLabel+"=true") + if result.Error != nil { + t.Fatalf("unexpected error: %v", result.Error) + } + if !result.Passed { + t.Errorf("Passed = false, want true (actual=%q)", result.Actual) + } + }) + + t.Run("disambiguated shape: mixed values fail the positive form with the offender named", func(t *testing.T) { + t.Parallel() + snap := collect(t, 0, + node("gpu-a", gpuLabels("true")), + node("gpu-b", gpuLabels("false"))) + result := eval(snap, optOutLabel+"=true") + if result.Error != nil { + t.Fatalf("unexpected error: %v", result.Error) + } + if result.Passed { + t.Error("Passed = true, want false: gpu-b carries value false") + } + if !strings.Contains(result.Actual, "gpu-b") { + t.Errorf("Actual = %q, want it to name gpu-b", result.Actual) + } + }) + + t.Run("disambiguated shape: label absent everywhere passes the negated form", func(t *testing.T) { + t.Parallel() + snap := collect(t, 0, + node("gpu-a", gpuLabels("")), + node("gpu-b", gpuLabels(""))) + result := eval(snap, "!"+optOutLabel) + if result.Error != nil { + t.Fatalf("unexpected error: %v", result.Error) + } + if !result.Passed { + t.Errorf("Passed = false, want true (actual=%q)", result.Actual) + } + }) + + t.Run("empty-value shape: uniform empty value passes the empty-value form", func(t *testing.T) { + t.Parallel() + withEmpty := func(name string) *corev1.Node { + return node(name, map[string]string{ + "cloud.google.com/gke-accelerator": "nvidia-h100-80gb", + optOutLabel: "", + }) + } + snap := collect(t, 0, withEmpty("gpu-a"), withEmpty("gpu-b")) + result := eval(snap, optOutLabel+"=") + if result.Error != nil { + t.Fatalf("unexpected error: %v", result.Error) + } + if !result.Passed { + t.Errorf("Passed = false, want true (actual=%q)", result.Actual) + } + // The same snapshot must fail the "=true" form: an empty value is + // not "true". + result = eval(snap, optOutLabel+"=true") + if result.Error != nil { + t.Fatalf("unexpected error: %v", result.Error) + } + if result.Passed { + t.Error("Passed = true for =true against empty-valued labels, want false") + } + }) + + t.Run("truncated shape fails closed in both directions", func(t *testing.T) { + t.Parallel() + snap := collect(t, 1, // 2 nodes, cap 1 -> "(+1 more)" tail on every entry + node("gpu-a", gpuLabels("true")), + node("gpu-b", gpuLabels("true"))) + for _, value := range []string{optOutLabel + "=true", "!" + optOutLabel} { + result := eval(snap, value) + if result.Error == nil { + t.Errorf("eval(%q) on truncated readings: error = nil, want fail-closed (passed=%v)", + value, result.Passed) + continue + } + if !stderrors.Is(result.Error, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("eval(%q) error code = %v, want %s", value, result.Error, errors.ErrCodeInvalidRequest) + } + } + }) +} diff --git a/pkg/validator/doc.go b/pkg/validator/doc.go index cb41cc824..8ee4a095a 100644 --- a/pkg/validator/doc.go +++ b/pkg/validator/doc.go @@ -17,7 +17,8 @@ // // The validator runs in two phases: // -// 1. Readiness pre-flight: top-level constraint expressions are evaluated +// 1. Readiness pre-flight: top-level constraint expressions, plus any +// declared under validation.readiness.constraints, are evaluated // against the snapshot inline (no cluster access required). A malformed // expression fails closed so misconfigured rules cannot masquerade as // passing. diff --git a/pkg/validator/validator.go b/pkg/validator/validator.go index 56e670f74..a6a5bde80 100644 --- a/pkg/validator/validator.go +++ b/pkg/validator/validator.go @@ -47,26 +47,52 @@ import ( // namespace, RBAC, and Job objects share a single conflict domain. const validatorFieldManager = "aicr" -// checkReadiness evaluates top-level validation constraints against the snapshot. -// Returns an error if any constraint fails, nil if all pass or no constraints exist. +// checkReadiness evaluates the recipe's readiness constraints against the +// snapshot: the top-level constraint set plus the readiness phase's own +// constraints (validation.readiness.constraints — declared for recipes whose +// pre-flight gates must not participate in generation-time overlay +// filtering, e.g. the GKE device-plugin ownership check, issue #1755). +// Returns an error if any constraint fails, nil if all pass or none exist. func checkReadiness(validationInput *v1.ValidationInput, snap *snapshotter.Snapshot) error { - if validationInput == nil || snap == nil || len(validationInput.Constraints) == 0 { + if validationInput == nil { return nil } + cs := validationInput.Constraints + if r := validationInput.Config.Readiness; r != nil { + cs = append(cs[:len(cs):len(cs)], r.Constraints...) + } + if len(cs) == 0 { + return nil + } + // Declared readiness constraints with no snapshot to evaluate them + // against must fail closed — silently skipping the gate would let a + // direct SDK caller bypass it by passing a nil snapshot. + if snap == nil { + return errors.New(errors.ErrCodeInvalidRequest, + "readiness constraints are declared but no snapshot is available to evaluate them — supply a snapshot") + } - slog.Info("readiness pre-flight", "constraints", len(validationInput.Constraints)) + slog.Info("readiness pre-flight", "constraints", len(cs)) - for _, c := range validationInput.Constraints { + for _, c := range cs { result := constraints.Evaluate(c, snap) if result.Error != nil { + // Deliberately flattens the evaluator's code (incl. the + // ErrCodeNotFound gpu_nodes.go returns for empty-universe / + // missing readings): the readiness contract is one uniform + // fail-closed exit, and "constraint cannot be evaluated" is an + // invalid request at this boundary. return errors.WrapWithContext(errors.ErrCodeInvalidRequest, fmt.Sprintf("readiness check could not evaluate: %s", c.Name), result.Error, map[string]any{"constraint": c.Name, "expected": c.Value}) } if !result.Passed { - return errors.New(errors.ErrCodeInvalidRequest, - fmt.Sprintf("readiness check failed: %s expected %s, got %s", c.Name, c.Value, result.Actual)) + msg := fmt.Sprintf("readiness check failed: %s expected %s, got %s", c.Name, c.Value, result.Actual) + if c.Remediation != "" { + msg += "\n" + strings.TrimSpace(c.Remediation) + } + return errors.New(errors.ErrCodeInvalidRequest, msg) } slog.Info("readiness constraint passed", "name", c.Name, "expected", c.Value, "actual", result.Actual) } @@ -204,8 +230,9 @@ func (v *Validator) ValidatePhases( return nil, err } - // Pre-flight: evaluate top-level validation constraints against snapshot. - // Fails fast before deploying any Jobs if prerequisites aren't met. + // Pre-flight: evaluate the top-level and readiness-phase constraints + // against the snapshot. Fails fast before deploying any Jobs if + // prerequisites aren't met. if err := checkReadiness(validationInput, snap); err != nil { return nil, err } @@ -279,7 +306,11 @@ func (v *Validator) runPhases( return results, nil } -// ValidatePhase runs a single validation phase. +// ValidatePhase runs a single validation phase. The readiness pre-flight +// runs first, exactly as in ValidatePhases: per-phase SDK callers must not +// be able to execute a phase against a cluster that fails the recipe's +// readiness gates (e.g. the GKE device-plugin ownership constraint, issue +// #1755). func (v *Validator) ValidatePhase( ctx context.Context, phase Phase, @@ -293,6 +324,13 @@ func (v *Validator) ValidatePhase( return nil, err } + // Readiness pre-flight — before the no-cluster short-circuit, matching + // ValidatePhases: constraints are evaluated inline against the snapshot + // even in test mode. + if err := checkReadiness(validationInput, snap); err != nil { + return nil, err + } + cat, err := catalog.LoadWithDataProvider(ctx, v.dataProvider, v.Version, v.Commit) if err != nil { return nil, errors.PropagateOrWrap(err, errors.ErrCodeInternal, "failed to load validator catalog") diff --git a/pkg/validator/validator_test.go b/pkg/validator/validator_test.go index 0ce4c0590..8e8868250 100644 --- a/pkg/validator/validator_test.go +++ b/pkg/validator/validator_test.go @@ -20,11 +20,13 @@ import ( "fmt" "io/fs" "path/filepath" + "strings" "testing" "gopkg.in/yaml.v3" "github.com/NVIDIA/aicr/pkg/errors" + "github.com/NVIDIA/aicr/pkg/measurement" "github.com/NVIDIA/aicr/pkg/recipe" "github.com/NVIDIA/aicr/pkg/snapshotter" "github.com/NVIDIA/aicr/pkg/validator/catalog" @@ -302,6 +304,94 @@ func TestValidatePhaseNoCluster(t *testing.T) { } } +func TestValidatePhaseRunsReadinessPreflight(t *testing.T) { + // The per-phase SDK entry point must enforce the same readiness gate as + // ValidatePhases: a caller running a single phase must not be able to + // bypass the recipe's readiness constraints (e.g. the GKE device-plugin + // ownership check, issue #1755). Runs in no-cluster mode — readiness is + // evaluated inline before the no-cluster short-circuit. + v := New( + WithVersion("1.0.0"), + WithNoCluster(true), + ) + + rec := &recipe.RecipeResult{ + Constraints: []recipe.Constraint{ + {Name: "K8s.server.version", Value: ">= 99.0"}, + }, + } + snap := &snapshotter.Snapshot{ + Measurements: []*measurement.Measurement{ + { + Type: measurement.TypeK8s, + Subtypes: []measurement.Subtype{ + { + Name: "server", + Data: map[string]measurement.Reading{ + "version": measurement.Str("v1.30.0"), + }, + }, + }, + }, + }, + } + + _, err := v.ValidatePhase(context.Background(), PhaseDeployment, v1.ToValidationInput(rec), snap) + if err == nil { + t.Fatal("ValidatePhase() = nil error, want readiness failure") + } + if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("error code = %v, want %s", err, errors.ErrCodeInvalidRequest) + } +} + +func TestValidatePhasesRunsReadinessPreflight(t *testing.T) { + // The plural entry point is the DEFAULT client validate path + // (pkg/client/v1/aicr.go routes full-phase validation through + // ValidatePhases), so its readiness gate needs its own pin: a regression + // that reorders checkReadiness below the NoCluster short-circuit would + // pass every singular-path test while silently fail-opening the primary + // path — the exact #1755 failure this gate exists to prevent. Uses a + // non-nil snapshot so the gate must actually evaluate the constraint + // rather than fail on snapshot absence. + v := New( + WithVersion("1.0.0"), + WithNoCluster(true), + ) + + rec := &recipe.RecipeResult{ + Constraints: []recipe.Constraint{ + {Name: "K8s.server.version", Value: ">= 99.0"}, + }, + } + snap := &snapshotter.Snapshot{ + Measurements: []*measurement.Measurement{ + { + Type: measurement.TypeK8s, + Subtypes: []measurement.Subtype{ + { + Name: "server", + Data: map[string]measurement.Reading{ + "version": measurement.Str("v1.30.0"), + }, + }, + }, + }, + }, + } + + results, err := v.ValidatePhases(context.Background(), nil, v1.ToValidationInput(rec), snap) + if err == nil { + t.Fatal("ValidatePhases() = nil error, want readiness failure") + } + if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("error code = %v, want %s", err, errors.ErrCodeInvalidRequest) + } + if results != nil { + t.Errorf("PhaseResults = %v, want nil: no phase may run after a readiness failure", results) + } +} + func TestCheckReadinessNilInputs(t *testing.T) { tests := []struct { name string @@ -322,6 +412,24 @@ func TestCheckReadinessNilInputs(t *testing.T) { } } +func TestCheckReadinessNilSnapshotWithConstraintsFailsClosed(t *testing.T) { + // Declared readiness constraints with no snapshot must error, not + // silently skip — a direct SDK caller passing a nil snapshot must not + // bypass the gate. + rec := &recipe.RecipeResult{ + Constraints: []recipe.Constraint{ + {Name: "K8s.server.version", Value: ">= 1.28"}, + }, + } + err := checkReadiness(v1.ToValidationInput(rec), nil) + if err == nil { + t.Fatal("checkReadiness() = nil, want error for declared constraints without a snapshot") + } + if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("error code = %v, want %s", err, errors.ErrCodeInvalidRequest) + } +} + func TestCheckReadinessEmptyConstraints(t *testing.T) { rec := &recipe.RecipeResult{ Constraints: []recipe.Constraint{}, @@ -350,6 +458,95 @@ func TestCheckReadinessUnparseableConstraintFailsClosed(t *testing.T) { } } +func TestCheckReadinessEvaluatesReadinessPhaseConstraints(t *testing.T) { + // validation.readiness.constraints must be evaluated by the pre-flight + // gate alongside the top-level constraint set (issue #1755): they exist + // for gates that must fail `aicr validate` closed without participating + // in generation-time overlay filtering. The failure message must carry + // the constraint's remediation so the operator gets the diagnostic. + snap := &snapshotter.Snapshot{ + Measurements: []*measurement.Measurement{ + { + Type: measurement.TypeK8s, + Subtypes: []measurement.Subtype{ + { + Name: "server", + Data: map[string]measurement.Reading{ + "version": measurement.Str("v1.30.0"), + }, + }, + }, + }, + }, + } + + passC := recipe.Constraint{Name: "K8s.server.version", Value: ">= 1.28"} + failC := recipe.Constraint{ + Name: "K8s.server.version", + Value: ">= 99.0", + Remediation: "upgrade the control plane", + } + + tests := []struct { + name string + topLevel []recipe.Constraint + readiness []recipe.Constraint + wantErr bool + wantIn string // substring the error must carry; "" skips + }{ + { + name: "failing readiness-phase constraint carries remediation", + readiness: []recipe.Constraint{failC}, + wantErr: true, + wantIn: "upgrade the control plane", + }, + { + name: "passing readiness-phase constraint", + readiness: []recipe.Constraint{passC}, + }, + { + name: "passing top-level with failing readiness-phase", + topLevel: []recipe.Constraint{passC}, + readiness: []recipe.Constraint{failC}, + wantErr: true, + wantIn: "upgrade the control plane", + }, + { + name: "passing top-level with passing readiness-phase", + topLevel: []recipe.Constraint{passC}, + readiness: []recipe.Constraint{passC}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec := &recipe.RecipeResult{ + Constraints: tt.topLevel, + Validation: &recipe.ValidationConfig{ + Readiness: &recipe.ValidationPhase{Constraints: tt.readiness}, + }, + } + vi := v1.ToValidationInput(rec) + topBefore := len(vi.Constraints) + + err := checkReadiness(vi, snap) + if (err != nil) != tt.wantErr { + t.Fatalf("checkReadiness() = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantIn != "" && !strings.Contains(err.Error(), tt.wantIn) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantIn) + } + // The combined evaluation must not grow the input's top-level + // slice: ToValidationInput aliases the recipe's Constraints, so + // an aliasing append would write readiness constraints into the + // caller's recipe (the capped append in checkReadiness prevents + // this). + if len(vi.Constraints) != topBefore { + t.Errorf("checkReadiness mutated Constraints: len %d -> %d", topBefore, len(vi.Constraints)) + } + }) + } +} + func TestPhaseOrder(t *testing.T) { // performance runs last: its benchmark saturates all node GPUs and releases // DRA claims asynchronously, which would otherwise starve conformance's diff --git a/recipes/overlays/gke-cos.yaml b/recipes/overlays/gke-cos.yaml index 9e9f4b073..c44c415bd 100644 --- a/recipes/overlays/gke-cos.yaml +++ b/recipes/overlays/gke-cos.yaml @@ -78,6 +78,28 @@ spec: reapplyOnReboot: "true" validation: + # Readiness (not spec.constraints): this gate must fail `aicr validate` + # closed on an unlabeled cluster without excluding the GKE overlays at + # snapshot-based recipe generation — the user needs the recipe to exist + # in order to diagnose and fix the pool. Issue #1755. ADR-015's gpuStack + # profile relocates this constraint into profile values when it lands. + readiness: + constraints: + - name: NodeTopology.gpu-nodes.label + value: gke-no-default-nvidia-gpu-device-plugin=true + remediation: >- + GKE's managed device plugin must be disabled on AICR-managed GPU + node pools via the node label + gke-no-default-nvidia-gpu-device-plugin=true; otherwise it + conflicts with the GPU Operator's device plugin + (devicePlugin.enabled: true). Add the label to the GPU node pool + (gcloud container node-pools update ... --node-labels=...); + note --node-labels REPLACES the pool's full label set, so pass + every existing label plus the new one (see the Component + Catalog's GKE Device-Plugin Ownership section for the + discovery-then-update procedure). Because the recipe also sets + driver.enabled: false, compatible driver provisioning remains a + separate prerequisite. conformance: checks: - platform-health diff --git a/tests/uat/gcp/cluster-config.yaml b/tests/uat/gcp/cluster-config.yaml index fd93e416c..8727f1273 100644 --- a/tests/uat/gcp/cluster-config.yaml +++ b/tests/uat/gcp/cluster-config.yaml @@ -131,3 +131,8 @@ compute: labels: nodeGroup: gpu-worker dedicated: user-workload + # Disable GKE's managed device plugin so the GPU Operator's + # plugin is the sole advertiser of nvidia.com/gpu — the cluster + # prerequisite AICR's GKE recipes document and (as of #1755) + # enforce at validate readiness. + gke-no-default-nvidia-gpu-device-plugin: "true"