Skip to content

Commit 0eda0fe

Browse files
committed
fix: make the ComputeDomain guard path-aware and update kernelModuleType consumers
The guard matched numNodes anywhere in the ComputeDomain document, so moving the key to metadata.numNodes still passed while Kubernetes would reject the absent spec.numNodes. Matching a key without its parent is not a weaker check, it is the wrong check. The scanner now requires numNodes as a DIRECT CHILD of spec, walking indentation because the manifests are Helm templates no YAML parser accepts. Adds table cases covering every shape that has fooled a previous version of this scanner or must keep working: present, absent, under metadata, comment-only, nested under spec.channel, and templated. Plus a multi-document case asserting only the offending document is reported. Replacing useOpenKernelModules also broke four consumers that were not updated with it — a chainsaw bundle assertion (which fails CLI E2E), the OpenAPI response example, the query demo output, and the OCP values comment. Changing a values key requires sweeping its consumers, the same discipline applied to the version strings in this branch. Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
1 parent 1cbaa63 commit 0eda0fe

5 files changed

Lines changed: 137 additions & 28 deletions

File tree

api/aicr/v1/server.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -650,7 +650,7 @@ paths:
650650
value:
651651
version: "580.105.08"
652652
enabled: true
653-
useOpenKernelModules: true
653+
kernelModuleType: auto
654654
"400":
655655
description: >
656656
Invalid request. Common causes: (1) no criteria provided — all

demos/query.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ enabled: true
5353
maxParallelUpgrades: 5
5454
rdma:
5555
enabled: false
56-
useOpenKernelModules: true
56+
kernelModuleType: auto
5757
version: 595.91.07
5858
```
5959

pkg/recipe/computedomain_numnodes_test.go

Lines changed: 133 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -21,41 +21,74 @@ import (
2121
"testing"
2222
)
2323

24-
// numNodesKeyRE matches an indented mapping key. Anchored per line so a key
25-
// inside a comment or a quoted error string cannot satisfy it.
26-
var numNodesKeyRE = regexp.MustCompile(`(?m)^[ \t]+numNodes[ \t]*:`)
24+
// specKeyRE matches the document's top-level `spec:` mapping key.
25+
var specKeyRE = regexp.MustCompile(`^(\s*)spec\s*:\s*$`)
2726

28-
// computeDomainDocsMissingNumNodes returns the 0-based indexes of YAML
29-
// documents that declare kind: ComputeDomain without a numNodes key.
27+
// numNodesChildRE matches `numNodes:` at a given exact indentation.
28+
func numNodesChildRE(indent string) *regexp.Regexp {
29+
return regexp.MustCompile(`^` + regexp.QuoteMeta(indent) + `numNodes\s*:`)
30+
}
31+
32+
// specHasNumNodes reports whether a single YAML document declares numNodes as a
33+
// DIRECT CHILD of spec.
3034
//
31-
// Scoped per document rather than per file. A multi-document manifest where one
32-
// ComputeDomain sets numNodes and a second omits it would satisfy a whole-file
33-
// scan while still failing admission, and so would an unrelated resource that
34-
// happens to carry a numNodes key. No such manifest exists in the catalog
35-
// today; the guard is document-scoped so that adding one cannot silently
36-
// bypass it.
35+
// Path-aware on purpose. An earlier version matched `numNodes:` anywhere in the
36+
// document, which accepted `metadata.numNodes` — a key Kubernetes ignores, while
37+
// the required `spec.numNodes` stays absent and admission still fails. Matching
38+
// the key without its parent is not a weaker check, it is the wrong check.
3739
//
3840
// Comment lines are stripped first: these manifests legitimately discuss
39-
// "spec.numNodes: Required value" in prose, and a naive substring scan matches
40-
// that instead of the real key, passing even when the key is deleted.
41+
// "spec.numNodes: Required value" in prose, and a scan that does not strip them
42+
// matches that instead of the real key, passing even when the key is deleted.
43+
//
44+
// A full YAML parse is unavailable — the manifests are Helm templates containing
45+
// {{ }} expressions that no YAML parser accepts — so this walks indentation.
46+
func specHasNumNodes(doc string) bool {
47+
var lines []string
48+
for _, line := range strings.Split(doc, "\n") {
49+
if strings.HasPrefix(strings.TrimSpace(line), "#") || strings.TrimSpace(line) == "" {
50+
continue
51+
}
52+
lines = append(lines, line)
53+
}
54+
for i, line := range lines {
55+
m := specKeyRE.FindStringSubmatch(line)
56+
if m == nil {
57+
continue
58+
}
59+
specIndent := m[1]
60+
var childRE *regexp.Regexp
61+
for _, sub := range lines[i+1:] {
62+
subIndent := sub[:len(sub)-len(strings.TrimLeft(sub, " \t"))]
63+
// Dedent to spec's level or shallower ends the spec mapping.
64+
if len(subIndent) <= len(specIndent) {
65+
break
66+
}
67+
if childRE == nil {
68+
childRE = numNodesChildRE(subIndent)
69+
}
70+
if childRE.MatchString(sub) {
71+
return true
72+
}
73+
}
74+
}
75+
return false
76+
}
77+
78+
// computeDomainDocsMissingNumNodes returns the 0-based indexes of YAML
79+
// documents that declare kind: ComputeDomain without spec.numNodes.
4180
//
42-
// A full YAML parse is unavailable — the manifests are Helm templates and
43-
// contain {{ }} expressions that no YAML parser accepts.
81+
// Scoped per document: a multi-document manifest where one ComputeDomain sets
82+
// the key and a second omits it would satisfy a whole-file scan while still
83+
// failing admission. No such manifest exists in the catalog today; the guard is
84+
// document-scoped so adding one cannot silently bypass it.
4485
func computeDomainDocsMissingNumNodes(content string) []int {
4586
var missing []int
4687
for i, doc := range strings.Split(content, "\n---") {
4788
if !strings.Contains(doc, "kind: ComputeDomain") {
4889
continue
4990
}
50-
var b strings.Builder
51-
for _, line := range strings.Split(doc, "\n") {
52-
if strings.HasPrefix(strings.TrimSpace(line), "#") {
53-
continue
54-
}
55-
b.WriteString(line)
56-
b.WriteString("\n")
57-
}
58-
if !numNodesKeyRE.MatchString(b.String()) {
91+
if !specHasNumNodes(doc) {
5992
missing = append(missing, i)
6093
}
6194
}
@@ -137,3 +170,79 @@ func TestComputeDomainManifestsSetNumNodes(t *testing.T) {
137170
}
138171
t.Logf("verified %d ComputeDomain manifest(s) set spec.numNodes", checked)
139172
}
173+
174+
// TestComputeDomainScannerCases pins the scanner's behavior directly, so the
175+
// catalog guard above cannot quietly stop discriminating if the catalog changes.
176+
// Each case is a shape that has either fooled a previous version of this
177+
// scanner or must keep working.
178+
func TestComputeDomainScannerCases(t *testing.T) {
179+
t.Parallel()
180+
181+
const header = "apiVersion: resource.nvidia.com/v1beta1\nkind: ComputeDomain\n"
182+
183+
tests := []struct {
184+
name string
185+
doc string
186+
wantMissing bool
187+
}{
188+
{
189+
name: "spec.numNodes present",
190+
doc: header + "metadata:\n name: cd\nspec:\n numNodes: 0\n channel:\n allocationMode: All\n",
191+
},
192+
{
193+
name: "spec.numNodes absent",
194+
doc: header + "metadata:\n name: cd\nspec:\n channel:\n allocationMode: All\n",
195+
wantMissing: true,
196+
},
197+
{
198+
// Regression: an earlier scanner matched numNodes anywhere in the
199+
// document, so this passed while admission would still fail.
200+
name: "numNodes under metadata, not spec",
201+
doc: header + "metadata:\n name: cd\n numNodes: 0\nspec:\n channel:\n allocationMode: All\n",
202+
wantMissing: true,
203+
},
204+
{
205+
// Regression: an earlier scanner did not strip comments, so the
206+
// prose in the real manifest satisfied it even with the key gone.
207+
name: "numNodes only mentioned in a comment",
208+
doc: header + "metadata:\n name: cd\nspec:\n # numNodes: Required value\n channel:\n allocationMode: All\n",
209+
wantMissing: true,
210+
},
211+
{
212+
name: "nested numNodes does not satisfy the direct-child rule",
213+
doc: header + "metadata:\n name: cd\nspec:\n channel:\n numNodes: 0\n",
214+
// numNodes exists but under spec.channel, not spec.
215+
wantMissing: true,
216+
},
217+
{
218+
name: "templated value is acceptable",
219+
doc: header + "metadata:\n name: cd\nspec:\n numNodes: {{ .Values.numNodes }}\n",
220+
},
221+
}
222+
223+
for _, tt := range tests {
224+
t.Run(tt.name, func(t *testing.T) {
225+
t.Parallel()
226+
got := !specHasNumNodes(tt.doc)
227+
if got != tt.wantMissing {
228+
t.Errorf("specHasNumNodes reported missing=%v, want %v\ndoc:\n%s",
229+
got, tt.wantMissing, tt.doc)
230+
}
231+
})
232+
}
233+
}
234+
235+
// TestComputeDomainMultiDocument covers the per-document scoping: a file where
236+
// one ComputeDomain is valid and a second is not must report only the second.
237+
func TestComputeDomainMultiDocument(t *testing.T) {
238+
t.Parallel()
239+
240+
content := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: unrelated\n" +
241+
"\n---\napiVersion: resource.nvidia.com/v1beta1\nkind: ComputeDomain\nmetadata:\n name: ok\nspec:\n numNodes: 0\n" +
242+
"\n---\napiVersion: resource.nvidia.com/v1beta1\nkind: ComputeDomain\nmetadata:\n name: bad\nspec:\n channel:\n allocationMode: All\n"
243+
244+
missing := computeDomainDocsMissingNumNodes(content)
245+
if len(missing) != 1 || missing[0] != 2 {
246+
t.Errorf("missing documents = %v, want [2] (only the third document lacks spec.numNodes)", missing)
247+
}
248+
}

recipes/components/gpu-operator-ocp/values.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ daemonsets:
160160
# operator.upgradeCRD: Helm chart setting, not a ClusterPolicy field.
161161
# operator.resources: Helm chart setting for the operator Deployment, not CR.
162162
# driver.version: OCP operator manages driver version via the certified driver container.
163-
# driver.useOpenKernelModules: OCP uses pre-built driver containers from the certified catalog.
163+
# driver.kernelModuleType: OCP uses pre-built driver containers from the certified catalog.
164164
# driver.maxParallelUpgrades: Use daemonsets.rollingUpdate.maxUnavailable instead.
165165
# devicePlugin.env: OCP sets device plugin env via the operator's own defaults.
166166
# validator: OCP operator manages validation internally.

tests/chainsaw/cli/cuj1-training/assert-bundle-scheduling.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ driver:
4646
enabled: true
4747
rdma:
4848
enabled: false
49-
useOpenKernelModules: true
49+
kernelModuleType: auto
5050

5151
# ── GDRCopy: GPU-direct memory for high-performance training ─────────
5252
gdrcopy:

0 commit comments

Comments
 (0)