Skip to content

Commit eea2a81

Browse files
authored
fix(cli): validate/recipe UX papercuts (#1383 items 1-7) (#1391)
1 parent f2f8ed9 commit eea2a81

9 files changed

Lines changed: 234 additions & 12 deletions

File tree

pkg/cli/recipe.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,11 @@ Override snapshot-detected criteria:
146146
return err
147147
}
148148

149+
// Mode banner: recipe generation reads inputs (criteria, embedded
150+
// data, an optional snapshot) and never deploys to or modifies a
151+
// live cluster, so make that explicit up front (issue #1383).
152+
slog.Info("generating recipe offline — reads inputs only; does not deploy to or modify any cluster")
153+
149154
// Build a per-command Client bound to the resolved data source
150155
// (--data / spec.recipe.data, else embedded). The Client owns its
151156
// own DataProvider and per-provider criteria registry, replacing
@@ -227,9 +232,15 @@ Override snapshot-detected criteria:
227232
return errors.Wrap(errors.ErrCodeInternal, "failed to serialize recipe", err)
228233
}
229234

235+
componentNames := make([]string, len(resolved.ComponentRefs))
236+
for i, ref := range resolved.ComponentRefs {
237+
componentNames[i] = ref.Name
238+
}
239+
230240
slog.Info("recipe generation completed",
231241
"output", output,
232242
"components", len(resolved.ComponentRefs),
243+
"componentNames", strings.Join(componentNames, ", "),
233244
"overlays", len(resolved.Metadata.AppliedOverlays))
234245

235246
return nil

pkg/cli/recipe_list.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,12 @@ func writeCatalogEntries(ctx context.Context, cmd *cli.Command, entries []aicr.C
326326
if _, err := fmt.Fprintln(w, "(no matching overlays)"); err != nil {
327327
return errors.Wrap(errors.ErrCodeInternal, "failed to write empty message", err)
328328
}
329+
} else {
330+
// Legend so a bare "any" in the criteria columns reads as an
331+
// intentional wildcard rather than a missing/unknown value (issue #1383).
332+
if _, err := fmt.Fprintf(w, "\n%s = wildcard (dimension unconstrained — matches any value)\n", criteriaAny); err != nil {
333+
return errors.Wrap(errors.ErrCodeInternal, "failed to write legend", err)
334+
}
329335
}
330336
}
331337

pkg/cli/recipe_list_test.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,15 @@ func TestRecipeList_NonLeafStatusPlaceholder(t *testing.T) {
179179
}
180180
}
181181

182+
// TestRecipeList_WildcardLegend confirms the table output explains that a
183+
// bare "any" in the criteria columns is an intentional wildcard (issue #1383).
184+
func TestRecipeList_WildcardLegend(t *testing.T) {
185+
out := runRecipeList(t)
186+
if !strings.Contains(out, criteriaAny+" = wildcard") {
187+
t.Errorf("expected wildcard legend for %q in table output:\n%s", criteriaAny, out)
188+
}
189+
}
190+
182191
// TestRecipeList_FilteredJSONHealth drives the CLI filter end-to-end and
183192
// confirms ListCatalog and ComputeHealth agree on the same narrowed set: every
184193
// returned leaf carries a health block and matches the filter.

pkg/cli/validate.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -675,6 +675,16 @@ Run validation without failing on check errors (informational mode):
675675
failFast := boolFlagOrConfig(cmd, "fail-fast", derefBoolOr(resolved.FailFast, false))
676676
noCluster := boolFlagOrConfig(cmd, "no-cluster", resolved.NoCluster)
677677

678+
// Mode banner: make it explicit whether this run touches a live
679+
// cluster (issue #1383). --no-cluster is an offline dry-run that
680+
// reports checks as skipped; otherwise validation deploys
681+
// validator Jobs against the active kube-context.
682+
if noCluster {
683+
slog.Info("validating in --no-cluster mode — offline dry-run; checks are reported as skipped, no cluster is contacted")
684+
} else {
685+
slog.Info("validating against the live cluster — validator Jobs will be deployed to the active kube-context")
686+
}
687+
678688
// Resolve shared fields once, before the snapshot/agent split, so
679689
// CLI-overrides-config log lines fire exactly once per field even
680690
// when both the agent-deploy path and the validator Job want the

pkg/logging/cli.go

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"io"
2121
"log/slog"
2222
"os"
23+
"slices"
2324
"strings"
2425

2526
"github.com/NVIDIA/aicr/pkg/errors"
@@ -106,9 +107,12 @@ func (h *CLIHandler) Handle(_ context.Context, r slog.Record) error {
106107
msg = msg + ": " + strings.Join(attrs, " ")
107108
}
108109

109-
// Add color for error messages and success messages when supported.
110+
// Add color when supported. Error-level records are red; otherwise the
111+
// record is red when it carries a failure status attr (so a
112+
// "validator completed status=failed" line logged at Info reads red, not
113+
// green) and green for everything else.
110114
if h.color {
111-
if r.Level >= slog.LevelError {
115+
if r.Level >= slog.LevelError || h.hasFailureStatus(r) {
112116
msg = colorRed + msg + colorReset
113117
} else {
114118
msg = colorGreen + msg + colorReset
@@ -121,6 +125,37 @@ func (h *CLIHandler) Handle(_ context.Context, r slog.Record) error {
121125
return nil
122126
}
123127

128+
// hasFailureStatus reports whether the record (or a handler-bound attr)
129+
// carries a `status` attribute whose value indicates failure. This lets the
130+
// CLI color a non-error-level completion line red — e.g. CTRF reports a failed
131+
// validator via slog.Info("validator completed", "status", "failed"), which
132+
// would otherwise render green because it is below LevelError.
133+
func (h *CLIHandler) hasFailureStatus(r slog.Record) bool {
134+
isFailure := func(a slog.Attr) bool {
135+
if a.Key != "status" {
136+
return false
137+
}
138+
switch strings.ToLower(a.Value.String()) {
139+
case "failed", "error":
140+
return true
141+
default:
142+
return false
143+
}
144+
}
145+
if slices.ContainsFunc(h.attrs, isFailure) {
146+
return true
147+
}
148+
found := false
149+
r.Attrs(func(a slog.Attr) bool {
150+
if isFailure(a) {
151+
found = true
152+
return false
153+
}
154+
return true
155+
})
156+
return found
157+
}
158+
124159
// formatAttr renders a slog.Attr as "key=value", prefixing key with the
125160
// group path when present.
126161
func formatAttr(groupPrefix string, a slog.Attr) string {

pkg/logging/cli_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,54 @@ func TestCLIHandler_ErrorMessage(t *testing.T) {
9393
}
9494
}
9595

96+
func TestCLIHandler_FailureStatusColoredRed(t *testing.T) {
97+
tests := []struct {
98+
name string
99+
status string
100+
wantRed bool
101+
}{
102+
{"failed status at info level is red", "failed", true},
103+
{"error status at info level is red", "error", true},
104+
{"passed status at info level is green", "passed", false},
105+
{"skipped status at info level is green", "skipped", false},
106+
}
107+
for _, tt := range tests {
108+
t.Run(tt.name, func(t *testing.T) {
109+
var buf bytes.Buffer
110+
handler := newCLIHandler(&buf, slog.LevelInfo)
111+
handler.color = true // force color even though the writer is a buffer
112+
logger := slog.New(handler)
113+
114+
logger.Info("validator completed", "name", "deployment", "status", tt.status)
115+
116+
output := buf.String()
117+
gotRed := strings.Contains(output, colorRed)
118+
if gotRed != tt.wantRed {
119+
t.Errorf("status=%q colored red = %v, want %v (output: %q)", tt.status, gotRed, tt.wantRed, output)
120+
}
121+
// A non-red line must still be colored green (not left uncolored).
122+
if !tt.wantRed && !strings.Contains(output, colorGreen) {
123+
t.Errorf("status=%q should be green, got: %q", tt.status, output)
124+
}
125+
})
126+
}
127+
128+
// A handler-bound status attr (logger.With) must also trigger red, since
129+
// hasFailureStatus inspects h.attrs in addition to the record attrs.
130+
t.Run("handler-bound failed status at info level is red", func(t *testing.T) {
131+
var buf bytes.Buffer
132+
handler := newCLIHandler(&buf, slog.LevelInfo)
133+
handler.color = true
134+
logger := slog.New(handler).With("status", "failed")
135+
136+
logger.Info("validator completed", "name", "deployment")
137+
138+
if output := buf.String(); !strings.Contains(output, colorRed) {
139+
t.Errorf("handler-bound status=failed should be red, got: %q", output)
140+
}
141+
})
142+
}
143+
96144
func TestCLIHandler_NoColorWhenNotTTY(t *testing.T) {
97145
var buf bytes.Buffer
98146
handler := newCLIHandler(&buf, slog.LevelInfo)

pkg/serializer/writer.go

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -255,21 +255,74 @@ func flattenValue(out map[string]any, val reflect.Value, prefix string) {
255255
flattenValue(out, val.Field(i), key)
256256
}
257257
case reflect.Map:
258-
for _, mapKey := range val.MapKeys() {
259-
key := joinKey(prefix, fmt.Sprintf("%v", mapKey.Interface()))
260-
flattenValue(out, val.MapIndex(mapKey), key)
258+
// Render maps as a single-line compact-JSON value rather than
259+
// exploding every key into its own row. Deeply nested values.yaml
260+
// fragments would otherwise flood the table, and a raw %v dump of a
261+
// nested map breaks the FIELD/VALUE columns (issue #1383). An empty
262+
// top-level map yields no rows so the caller prints "<empty>".
263+
if prefix == "" && val.Len() == 0 {
264+
return
261265
}
266+
storeTableLeaf(out, prefix, compactJSONLeaf(val))
262267
case reflect.Slice, reflect.Array:
263-
for i := 0; i < val.Len(); i++ {
264-
key := joinKey(prefix, fmt.Sprintf("[%d]", i))
265-
flattenValue(out, val.Index(i), key)
268+
// Slices of structs (e.g. []ComponentRef) still recurse so each
269+
// element's scalar fields get their own row; slices of scalars
270+
// (e.g. deploymentOrder) collapse to one compact-JSON value.
271+
if val.Len() > 0 && derefedKind(val.Index(0)) == reflect.Struct {
272+
for i := 0; i < val.Len(); i++ {
273+
key := joinKey(prefix, fmt.Sprintf("[%d]", i))
274+
flattenValue(out, val.Index(i), key)
275+
}
276+
} else {
277+
// An empty top-level slice yields no rows so the caller prints
278+
// "<empty>"; a nested empty slice still renders as "[]".
279+
if prefix == "" && val.Len() == 0 {
280+
return
281+
}
282+
storeTableLeaf(out, prefix, compactJSONLeaf(val))
266283
}
267284
default:
268-
if prefix == "" {
269-
prefix = defaultValueKey
285+
// Scalars render natively; a string with an embedded newline, tab, or
286+
// carriage return is JSON-escaped so it can't shatter the tabwriter
287+
// FIELD/VALUE columns.
288+
if s, ok := val.Interface().(string); ok && strings.ContainsAny(s, "\n\r\t") {
289+
storeTableLeaf(out, prefix, compactJSONLeaf(val))
290+
} else {
291+
storeTableLeaf(out, prefix, val.Interface())
292+
}
293+
}
294+
}
295+
296+
// storeTableLeaf records a flattened leaf, substituting defaultValueKey when
297+
// the value sits at the root (no prefix).
298+
func storeTableLeaf(out map[string]any, prefix string, v any) {
299+
if prefix == "" {
300+
prefix = defaultValueKey
301+
}
302+
out[prefix] = v
303+
}
304+
305+
// compactJSONLeaf renders a non-scalar (or multi-line) value as single-line
306+
// JSON so the table's FIELD/VALUE columns stay intact. Falls back to %v when
307+
// the value cannot be marshaled (e.g. a map with non-string keys).
308+
func compactJSONLeaf(val reflect.Value) string {
309+
b, err := json.Marshal(val.Interface())
310+
if err != nil {
311+
return fmt.Sprintf("%v", val.Interface())
312+
}
313+
return string(b)
314+
}
315+
316+
// derefedKind returns the underlying kind of v after unwrapping pointers and
317+
// interfaces; reflect.Invalid for a nil along the way.
318+
func derefedKind(v reflect.Value) reflect.Kind {
319+
for v.Kind() == reflect.Pointer || v.Kind() == reflect.Interface {
320+
if v.IsNil() {
321+
return reflect.Invalid
270322
}
271-
out[prefix] = val.Interface()
323+
v = v.Elem()
272324
}
325+
return v.Kind()
273326
}
274327

275328
func joinKey(prefix, suffix string) string {

pkg/serializer/writer_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,51 @@ func TestWriter_SerializeTable(t *testing.T) {
117117
}
118118
}
119119

120+
func TestWriter_SerializeTable_CompactNestedLeaves(t *testing.T) {
121+
var buf bytes.Buffer
122+
writer := NewWriter(FormatTable, &buf)
123+
124+
data := map[string]any{
125+
"driver": map[string]any{"version": "570.86", "rdma": true},
126+
"deploymentOrder": []string{"cert-manager", "gpu-operator"},
127+
"notes": "line1\nline2",
128+
"tabbed": "col1\tcol2",
129+
"replicas": 3,
130+
}
131+
if err := writer.Serialize(context.Background(), data); err != nil {
132+
t.Fatalf("Serialize failed: %v", err)
133+
}
134+
output := buf.String()
135+
136+
// Nested map collapses to one compact-JSON cell (not exploded into
137+
// driver.version / driver.rdma rows).
138+
if !strings.Contains(output, `driver`) || !strings.Contains(output, `"version":"570.86"`) {
139+
t.Errorf("nested map not rendered as compact JSON: %q", output)
140+
}
141+
if strings.Contains(output, "driver.version") {
142+
t.Errorf("nested map should not be exploded into dotted rows: %q", output)
143+
}
144+
// Scalar slice collapses to a compact-JSON array.
145+
if !strings.Contains(output, `["cert-manager","gpu-operator"]`) {
146+
t.Errorf("scalar slice not rendered as compact JSON: %q", output)
147+
}
148+
// A multi-line string must not leak a raw newline into the value cell.
149+
if strings.Contains(output, "line1\nline2") {
150+
t.Errorf("multi-line string should be escaped, not raw: %q", output)
151+
}
152+
if !strings.Contains(output, `"line1\nline2"`) {
153+
t.Errorf("multi-line string should be JSON-escaped: %q", output)
154+
}
155+
// A tab-containing string must likewise be escaped so it can't break columns.
156+
if !strings.Contains(output, `"col1\tcol2"`) {
157+
t.Errorf("tab string should be JSON-escaped: %q", output)
158+
}
159+
// Plain scalar stays native.
160+
if !strings.Contains(output, "replicas") || !strings.Contains(output, "3") {
161+
t.Errorf("scalar value missing: %q", output)
162+
}
163+
}
164+
120165
func TestWriter_UnsupportedFormat(t *testing.T) {
121166
// Note: NewWriter now defaults unknown formats to JSON instead of erroring
122167
// This test is kept to verify the fallback behavior

pkg/snapshotter/agent.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,8 @@ func deployAndWaitForResult(ctx context.Context, clientset k8sclient.Interface,
206206
// understand why agent logs are missing from their output.
207207
if logCtx.Err() == nil {
208208
slog.Warn("agent log streaming skipped: pod did not become ready",
209+
slog.String("namespace", agentConfig.Namespace),
210+
slog.String("job", agentConfig.JobName),
209211
"error", podErr)
210212
}
211213
return
@@ -227,7 +229,10 @@ func deployAndWaitForResult(ctx context.Context, clientset k8sclient.Interface,
227229
msg := "job failed"
228230
if autoInjectedGPUSelector {
229231
msg = "job failed (auto-injected node selector nvidia.com/gpu.present=true — " +
230-
"if no GPU nodes are schedulable, pass --node-selector or --require-gpu explicitly)"
232+
"if no GPU nodes are schedulable, target a GPU node explicitly, e.g. " +
233+
"--node-selector kubernetes.io/hostname=<gpu-node> " +
234+
"(repeat the flag per key=value), or pass --require-gpu to schedule onto " +
235+
"a node advertising the nvidia.com/gpu resource)"
231236
}
232237
return nil, errors.Wrap(errors.ErrCodeInternal, msg, waitErr)
233238
}

0 commit comments

Comments
 (0)