Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions docs/contributor/validator.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Comment thread
coderabbitai[bot] marked this conversation as resolved.

**Supported operators** (`pkg/constraints/constraint.go`):

Expand All @@ -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
(`<label-key>=<value>` / `!<label-key>`, 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`.
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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`.
Expand Down
13 changes: 9 additions & 4 deletions docs/design/015-recipe-configuration-profiles.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions docs/integrator/recipe-development.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
`<label-key>=<value>` asserts every GPU node carries the label with exactly
that value, and `!<label-key>` 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
Expand Down
4 changes: 3 additions & 1 deletion docs/user/component-catalog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down
1 change: 1 addition & 0 deletions docs/user/validation.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <az dump>`, 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

Expand Down
15 changes: 15 additions & 0 deletions pkg/collector/topology/topology.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"context"
"fmt"
"log/slog"
"regexp"
"sort"
"strings"

Expand Down Expand Up @@ -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 {
Expand Down
27 changes: 27 additions & 0 deletions pkg/collector/topology/topology_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
8 changes: 8 additions & 0 deletions pkg/constraints/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions pkg/constraints/evaluate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 3 additions & 2 deletions pkg/constraints/extractor.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
const (
keyType = "type"
keyPath = "path"
keyKey = "key"
keySubtype = "subtype"
keySelector = "selector"
)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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})
}
Loading
Loading