fix(kwok): fail closed on unmapped profiles; skip in batch - #2033
Conversation
|
Welcome to AICR, @framsouza! Thanks for your first pull request. Before review, please ensure:
A maintainer will review this soon. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughKWOK now discovers system and GPU profiles from metadata labels instead of fixed mappings. Recipe criteria default to Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kwok-recipes.yaml:
- Around line 95-97: Update the workflow test step running sync-budget_test.sh
and profile-select_test.sh to verify yq is available with command -v yq and exit
non-zero when it is missing, ensuring profile-selection assertions cannot be
silently skipped in CI.
In `@kwok/README.md`:
- Around line 152-164: Update the new cloud provider onboarding guidance to
require metadata.labels.accelerator: <accelerator> alongside provider and
nodeType for accelerated profiles, matching the selection behavior in
select_profiles. Keep the existing directory and system/accelerated profile
requirements unchanged.
- Around line 57-70: The profile-selection documentation must describe criteria
normalization before matching profiles. Update the README to state that
resolve_recipe_criteria maps missing or null service and accelerator values, as
well as any, to eks and h100 respectively, and that apply-nodes.sh and
run-all-recipes.sh use this shared resolver.
In `@kwok/scripts/lib/profile-select.sh`:
- Around line 77-94: Separate unmapped-profile skips from selector failures: in
kwok/scripts/lib/profile-select.sh lines 77-94 and 122-142, return a dedicated
no-match status only for policy-approved absent services/profiles, while keeping
ambiguous matches and invalid or malformed selector states nonzero with
diagnostics. In kwok/scripts/run-all-recipes.sh lines 261-266, succeed only for
that dedicated status and log and propagate every other failure. In
kwok/scripts/lib/profile-select_test.sh lines 91-145, add or update tests
asserting distinct absent and ambiguous outcomes.
- Around line 42-45: Validate $# before expanding positional parameters in
resolve_recipe_criteria and select_profiles, returning their intended
diagnostics when called without arguments instead of triggering set -u errors.
Update kwok/scripts/lib/profile-select.sh lines 42-45 and 63-70 accordingly, and
add or adjust tests in kwok/scripts/lib/profile-select_test.sh lines 137-140 to
invoke both functions without arguments and assert the expected errors.
- Around line 49-52: Update the criteria parsing in profile selection to
distinguish missing/null values from boolean or other invalid types, while still
accepting the string "any" and defaulting only missing/null criteria to "eks" or
"h100". Ensure both yq queries propagate evaluation or malformed-YAML failures
instead of silently selecting defaults, and add coverage for boolean criteria
and malformed YAML.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 59221ab9-3ce8-4825-a63f-b7bd09247244
📒 Files selected for processing (6)
.github/workflows/kwok-recipes.yamlkwok/README.mdkwok/scripts/apply-nodes.shkwok/scripts/lib/profile-select.shkwok/scripts/lib/profile-select_test.shkwok/scripts/run-all-recipes.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@kwok/README.md`:
- Around line 180-184: Update the new cloud provider profile requirements in the
README to require exactly one system profile per provider and exactly one
accelerated profile for each supported accelerator, replacing the current “at
least one” wording while preserving the specified metadata labels.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c886fc6e-e4af-475d-8b6b-9e84efa489df
📒 Files selected for processing (1)
kwok/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@kwok/scripts/lib/profile-select_test.sh`:
- Around line 183-193: Add a test fixture and assertions in the
profile-selection test suite for an overlay with service set to null and
accelerator set to an empty string, verifying resolve_recipe_criteria returns
eks h100 with success. Update the reported test count to include this case,
alongside the existing missing-field and any-placeholder tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 880328a4-ccdb-44d6-9f72-e412ce0f9924
📒 Files selected for processing (2)
kwok/scripts/lib/profile-select.shkwok/scripts/lib/profile-select_test.sh
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kwok/scripts/lib/profile-select.sh (1)
162-184: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate profile metadata read failures.
select_profilesignores failures from theyq evalcalls that readprovider,nodeType, andaccelerator. A malformed profile YAML can therefore produce empty labels and returnPROFILE_SELECT_RC_NO_MATCH.
run_recipe_testtreats that status as a successful skip. This can make batch CI pass when the profile tree is invalid.Check each
yqcommand and return status 1 with a diagnostic on failure. Add a malformed-profile fixture that asserts status 1.As per coding guidelines, write tests alongside code changes.
Proposed fix
- provider=$(yq eval '.metadata.labels.provider // ""' "${profile}") - nodeType=$(yq eval '.metadata.labels.nodeType // ""' "${profile}") + if ! provider=$(yq eval '.metadata.labels.provider // ""' "${profile}"); then + echo "[ERROR] failed to read provider label from ${profile}" >&2 + return 1 + fi + if ! nodeType=$(yq eval '.metadata.labels.nodeType // ""' "${profile}"); then + echo "[ERROR] failed to read nodeType label from ${profile}" >&2 + return 1 + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kwok/scripts/lib/profile-select.sh` around lines 162 - 184, Update select_profiles so every yq eval reading provider, nodeType, or accelerator checks its exit status, emits a diagnostic identifying the malformed profile, and returns status 1 immediately on failure instead of treating missing labels as no match. Add a malformed-profile fixture and accompanying run_recipe_test coverage asserting status 1, placing the test alongside the implementation changes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@kwok/scripts/lib/profile-select.sh`:
- Around line 162-184: Update select_profiles so every yq eval reading provider,
nodeType, or accelerator checks its exit status, emits a diagnostic identifying
the malformed profile, and returns status 1 immediately on failure instead of
treating missing labels as no match. Add a malformed-profile fixture and
accompanying run_recipe_test coverage asserting status 1, placing the test
alongside the implementation changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fa004579-5680-43b3-97ad-d3c17b1a4feb
📒 Files selected for processing (4)
kwok/README.mdkwok/scripts/lib/profile-select.shkwok/scripts/lib/profile-select_test.shkwok/scripts/run-all-recipes.sh
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Requesting changes: 1 remaining merge blocker on 2af2b69.
Welcome to AICR, @framsouza, and thanks for taking on this false-positive gap. The label-driven selection and focused regression suite are a strong direction. I verified the latest malformed-profile fix locally (22/22 tests pass), so CodeRabbit's earlier blocker is addressed. The remaining inline issue is the CI no-match path: explicit matrix jobs can still report success without running bundle or scheduling validation. Once that path cannot produce a green false-success, happy to re-review.
Current required CI has not run yet because external-contributor validation is still pending.
| sel_err=$(select_profiles "${svc}" "${accel}" "${KWOK_DIR}/profiles" 2>&1 >/dev/null) || select_rc=$? | ||
| if (( select_rc == PROFILE_SELECT_RC_NO_MATCH )); then | ||
| log_warn "SKIP ${recipe}: no KWOK profile for service=${svc} accelerator=${accel} (add one under kwok/profiles/${svc}/ — see #1997)" | ||
| return 0 |
There was a problem hiding this comment.
Blocking: could we keep an unmapped profile from becoming a successful CI job? Each matrix cell calls run-all-recipes.sh with one explicit recipe, but this returns 0 and main records it in passed. On this head, 72 of 90 service-scoped overlays resolve to PROFILE_SELECT_RC_NO_MATCH, so those jobs can remain green without bundle or scheduling validation - the false-success direction #1997 is meant to close. Please make explicit or CI recipes propagate a nonzero result or gate no-coverage separately, and add a regression showing that a single unmapped recipe cannot be reported as passed.
There was a problem hiding this comment.
Thanks for the specific reproduction, the 72/90 number pushed me to close both layers on branch head f8242bf:
- Non-zero on explicit invocation:
kwok/scripts/run-all-recipes.sh gb200-oke-training(exact form used at.github/actions/kwok-test/action.yml:178) exits 1 with a clear diagnostic. main records it infailed[], notpassed[]. Implicit batch mode still SKIPs somake kwok-test-alllocally isn't disrupted. - No-coverage gated at CI: c069573 filters unmapped recipes at the classify step, local sim reproduces your 18-in / 72-dropped split.
- Regression:
kwok/scripts/run-all-recipes_test.shassertsexplicit-unmapped-recipe-is-not-reported-as-passed(rc must be non-zero). Passes on this head.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kwok/scripts/lib/profile-select.sh (1)
178-205: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate
findfailures as fatal selector failures.
findruns in process substitution on Line 205. Bash does not propagate that command exit status to thewhileloop. If traversal fails, both match arrays can remain empty. Lines 207-210 then returnPROFILE_SELECT_RC_NO_MATCHinstead of1.Batch execution can skip a broken profile tree and return success, even though profile discovery failed. Capture and check the
findexit status before returning no-match. Add a regression test that makesfindfail and asserts status1.As per coding guidelines, write tests alongside code changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kwok/scripts/lib/profile-select.sh` around lines 178 - 205, Update the profile-selection loop around the process-substitution find invocation to capture its exit status and return 1 when traversal fails, before the existing no-match handling returns PROFILE_SELECT_RC_NO_MATCH. Add a regression test that forces find to fail and verifies the selector exits with status 1.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@kwok/scripts/lib/profile-select.sh`:
- Around line 178-205: Update the profile-selection loop around the
process-substitution find invocation to capture its exit status and return 1
when traversal fails, before the existing no-match handling returns
PROFILE_SELECT_RC_NO_MATCH. Add a regression test that forces find to fail and
verifies the selector exits with status 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 65aa935e-de16-492f-9631-063b1f026ac4
📒 Files selected for processing (2)
kwok/scripts/lib/profile-select.shkwok/scripts/lib/profile-select_test.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kwok-recipes.yaml:
- Around line 192-200: Update both profile_status handling sites at
.github/workflows/kwok-recipes.yaml lines 192-200 and 244-249 to capture the
command substitution output and exit status separately. Preserve fatal nonzero
statuses by terminating discovery, add overlays to dropped only when the status
equals PROFILE_SELECT_RC_NO_MATCH, and retain the existing skip behavior for
that specific no-match result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3b195816-63ea-4af2-9af2-a3110ef347be
📒 Files selected for processing (1)
.github/workflows/kwok-recipes.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/kwok-recipes.yaml (1)
112-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove recipe classification out of the workflow YAML.
This step now handles criteria resolution, profile selection, dispatch validation, tier discovery, diff traversal, and batching. Move this logic to a composite action or a checked-in script. Keep the workflow step focused on orchestration. If the logic remains inline, document why composite indirection is not appropriate.
Based on learnings, prefer the repository’s three-layer composite architecture; inline shell is an exception only for narrowly scoped, single-use steps with an inline rationale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/kwok-recipes.yaml around lines 112 - 146, The workflow step is accumulating recipe classification and discovery logic that should not remain inline. Move profile and recipe classification, including functions such as profile_status and related criteria/profile selection, into the repository’s established three-layer composite-action architecture (or a checked-in script invoked by it), leaving the workflow responsible only for orchestration, inputs, outputs, and batching; if inline logic is retained, add an explicit rationale explaining why composite indirection is inappropriate.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/kwok-recipes.yaml:
- Around line 112-146: The workflow step is accumulating recipe classification
and discovery logic that should not remain inline. Move profile and recipe
classification, including functions such as profile_status and related
criteria/profile selection, into the repository’s established three-layer
composite-action architecture (or a checked-in script invoked by it), leaving
the workflow responsible only for orchestration, inputs, outputs, and batching;
if inline logic is retained, add an explicit rationale explaining why composite
indirection is inappropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6a6565f3-bed7-44bc-850a-3680a557d4f4
📒 Files selected for processing (3)
.github/workflows/kwok-recipes.yamlkwok/scripts/run-all-recipes.shkwok/scripts/run-all-recipes_test.sh
|
@mchmarny thanks for catching that. I verified end-to-end locally (branch head f8242bf). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kwok-recipes.yaml:
- Around line 198-205: Update both yq overlay-parsing sites in
.github/workflows/kwok-recipes.yaml at lines 198-205 and 254-261: replace the yq
eval || true handlers with explicit status checks that immediately exit
discovery when parsing fails, rather than treating the empty result as an
unmapped recipe; preserve the existing filtering behavior for successfully
parsed overlays.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8626754e-79ee-43b2-86e9-0fe72a00368e
📒 Files selected for processing (1)
.github/workflows/kwok-recipes.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/kwok-recipes.yaml (1)
214-218: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply shared criteria defaults before filtering recipes.
resolve_recipe_criteriamaps absent, null, andanyservice criteria toeks. These checks skip those overlays beforeprofile_statuscalls that resolver. A recipe that direct execution selects aseks/h100is therefore omitted from the CI matrix.
.github/workflows/kwok-recipes.yaml#L214-L218: Skip only explicitly unsupportedocprecipes. Let absent, null, andanyvalues reachprofile_status..github/workflows/kwok-recipes.yaml#L266-L274: Apply the same rule in Tier 2.As per PR objectives, recipe criteria must use consistent
eksandh100defaults across execution paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/kwok-recipes.yaml around lines 214 - 218, Update the filtering logic in .github/workflows/kwok-recipes.yaml at lines 214-218 and 266-274 to skip only explicitly unsupported ocp services; allow absent, null, and any values to reach profile_status and resolve_recipe_criteria so shared eks and h100 defaults are applied consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kwok-recipes.yaml:
- Around line 120-141: Update the comments near the inline helpers
read_overlay_field and profile_status to document why this reusable control flow
remains local to the discovery step instead of being extracted into a Layer-2
composite action: it has no meaningful reuse beyond this workflow step. Keep the
rationale brief and preserve the existing helper behavior.
---
Outside diff comments:
In @.github/workflows/kwok-recipes.yaml:
- Around line 214-218: Update the filtering logic in
.github/workflows/kwok-recipes.yaml at lines 214-218 and 266-274 to skip only
explicitly unsupported ocp services; allow absent, null, and any values to reach
profile_status and resolve_recipe_criteria so shared eks and h100 defaults are
applied consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d2c6e828-94ac-456e-858b-5e252ec459b3
📒 Files selected for processing (1)
.github/workflows/kwok-recipes.yaml
|
@framsouza also please squash commits |
apply-nodes.sh::get_profiles silently fell back to eks/p5-h100.yaml for any unknown service+accelerator. NVIDIA#1985 showed the failure mode: GB300 GKE lanes reported green while running amd64 H100 nodes. Replace the hardcoded map with label-driven discovery in kwok/scripts/lib/profile-select.sh (match metadata.labels {provider,nodeType,accelerator}). Adding a profile is now a YAML file, no bash edit. Layered fail-closed: - Direct (apply-nodes.sh, make kwok-e2e): full diagnostic, exit 1. - Batch (run-all-recipes.sh): explicit invocations — every CI matrix cell — fail on unmapped SKIP; implicit get_recipes() still SKIPs for local make kwok-test-all. - CI discovery (kwok-recipes.yaml classify): unmapped recipes never enter the matrix; malformed overlay YAML aborts classify; workflow_dispatch on an unmapped recipe fails at classify. Selector tightened: reject booleans/integers in criteria (yq's // alternative was falsy-swallowing false); malformed profile YAML is fatal; ambiguous match returns rc=1, genuine no-match returns PROFILE_SELECT_RC_NO_MATCH (rc=2) so batch can distinguish "no profile yet" from "broken tree". Tests wired into the discover job's script-tests step (profile-select_test.sh: 22 cases; run-all-recipes_test.sh: 2 cases proving explicit unmapped cannot be passed). kwok/README.md updated. Fixes NVIDIA#1997 Signed-off-by: framsouza <fram.souza14@gmail.com>
630cff3 to
5e11668
Compare
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
APPROVE — prior blocker resolved on 1275249. Explicit unmapped recipes now fail instead of being reported as passed, and the regression test covers the CI invocation shape. Focused shell tests pass; live CI has no failures and remaining lanes are still running.
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
🔬 Multi-persona review — approve with comments
Method: four independent persona reviewers (bash-correctness · CI/DX-operability · test-coverage · domain/fail-closed) whose findings were each re-derived from the resolved code by an adversarial senior meta-reviewer. Unit tests run locally (profile-select_test 22/22, run-all-recipes_test 2/2); shellcheck -x clean on the changed scripts.
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall
Strong PR. It replaces the silent-fallback profile map (root cause of the #1985 false-green — arm64 GB300 GKE lanes validated on amd64 H100 nodes) with label-driven, fail-closed selection, a well-reasoned direct-vs-batch asymmetry, and a thorough unit suite. The bash is careful: subshell-exit propagation is handled at every $(...) call site, the skippable-NO_MATCH vs fatal-rc=1 split is disciplined, and the yq-required gate fails closed without adding an unpinned install.
@mchmarny's CHANGES_REQUESTED blocker is resolved on this head, three independent ways:
- The classify-step drop filter removes unmapped recipes before dispatch — no matrix cell is ever created.
- Defense-in-depth: the cell invokes
run-all-recipes.sh --deployer X <recipe>positionally (action.yml:178) →EXPLICIT_RECIPES→run_recipe_testreturns rc=1, not a skip. - A regression test proves an explicitly-named unmapped recipe cannot be reported as passed.
No blockers survived meta-review. Approving; the findings below are Minor/Nitpick. The one worth landing is F1 — a new adjacent fail-open the drop-filter introduces.
Findings not anchorable to the diff (pre-existing lines)
🟡 F1 — Empty-tier fail-open: no minimum-coverage floor. The new drop-filter can empty a whole tier. Tier jobs gate on tierN_pairs != '[]', so an empty tier is skipped, and the (pre-existing) summary job at .github/workflows/kwok-recipes.yaml L497-512 treats a skipped tier as pass (it only fails on failure/cancelled). Because only kwok/profiles/eks/ ships today, deleting/renaming that directory makes every recipe resolve to the skippable PROFILE_SELECT_RC_NO_MATCH, empties all three tiers, and the run prints "All recipe validations passed" with zero bundle/scheduling coverage. Downgraded to Minor because reaching zero coverage requires removing the whole profile tree (a conspicuous diff); the realistic single-profile case is safe exclusion. Fix: floor guard in discover — fail when the push/nightly tier3 (or tier1) pair count is 0 (::error::KWOK matrix is empty — profile tree likely broken). Only workflow_dispatch no-match should legitimately produce no cells.
🟡 F1b — summary needs the tier jobs but not discover (kwok-recipes.yaml L473). This PR widens discover's fatal-exit surface; if discover fails, the tiers skip and summary (if: always()) sees them skipped → exit 0. The overall workflow run still goes red, so this only bites if KWOK Test Summary specifically is the required branch-protection check. Fix: add discover to summary's needs and fail when needs.discover.result != 'success'.
🔵 F11 — pre-existing / out of scope: kwok/scripts/apply-nodes.sh:152 hardcodes DEFAULT_REGION/DEFAULT_ZONES to us-east-1 for every provider. Untouched by this PR and inert today (only eks profiles), but latent once gke/aks profiles land. Follow-up, not a merge concern.
Confirmed non-issues (examined & cleared)
- ✅ @mchmarny's blocker — resolved at both discovery (drop) and in-runner (explicit-fail).
- ✅
yq-required gate — fails closed, no unpinned install (matches the repo convention thatubuntu-latestpreinstalls yq/jq). - ✅ Silent
eks/p5-h100fallback — fully removed from all changed scripts. - ✅
workflow_dispatchfails loud on an unmapped recipe; localmake kwok-test-allstill skips → no dev-loop regression.
Summary
🔴 0 · 🟠 0 · 🟡 3 · 🔵 8 — Approve with comments. The single follow-up worth landing is F1's coverage-floor guard; everything else is polish.
Reviewed with a multi-persona + adversarial-meta-review pass. Inline comments follow.
| # caller's variable via $(...) and become an annotation only in | ||
| # the step log. Caller MUST use `|| exit 1` — an inner exit | ||
| # inside command substitution only kills the subshell. | ||
| read_overlay_field() { |
There was a problem hiding this comment.
🟡 Minor — CI read_overlay_field reuses the // "" falsy-swallow the library reader abandoned
The library's _read_criteria_field was rewritten to reject service: false by YAML tag, but classify still reads service via yq '... // ""' here and does -z→continue (~L217) before the strict resolve_recipe_criteria runs. So service: false collapses to "" and is silently dropped as non-testable instead of erroring like the direct path does. Contrived input, but it's an inconsistency with the fix's own stated guarantee.
Blast radius: Coverage-exclusion of a malformed overlay (not a wrong-hardware false-pass); the direct run_recipe_test path still fails the same input strictly.
Fix: Route the service field through the strict reader in classify (or add a tag check) so a malformed-typed service errors instead of being silently excluded.
| profile_status() { | ||
| local overlay="$1" criteria svc accel rc=0 | ||
| if ! criteria=$(resolve_recipe_criteria "${overlay}" 2>/dev/null); then | ||
| echo "::error file=${overlay}::resolve_recipe_criteria failed" |
There was a problem hiding this comment.
🔵 Nitpick — profile_status writes ::error:: to stdout inside $(...), so the annotation is captured and discarded
Both ::error:: lines here (L153 and L163) lack >&2, unlike the sibling read_overlay_field (L136) whose header documents the rule. profile_status is always called as status=$(profile_status ...), so these lines are captured into the discarded var and never render as GitHub annotations. Both branches exit 1 (caught by || exit 1 at every call site), so the job still fails closed and the raw stderr re-emit (L154/L164) still shows — only the file-pinned annotation is lost.
Blast radius: Lost operator-facing annotation; no misclassification, no false green.
Fix: Add >&2 to the two echo "::error..." lines to match read_overlay_field.
| log_warn "SKIP ${recipe}: no KWOK profile for service=${svc} accelerator=${accel} (add one under kwok/profiles/${svc}/ — see #1997)" | ||
| return 0 | ||
| fi | ||
| if (( select_rc != 0 )); then |
There was a problem hiding this comment.
🔵 Nitpick — FATAL rc=1 propagation in run_recipe_test is untested at the integration layer
Both harness cases drive gb200-oke-training (rc=2 no-match), exercising the explicit-fail and SKIP branches but never this select_rc != 0 → return select_rc path — the dup/broken-profile-must-not-be-swallowed case central to mchmarny's concern. select_profiles returning 1 is unit-covered (profile-select_test tests 6/7), and this propagation line is trivially correct, so it's a coverage gap rather than a defect.
Blast radius: A future edit that reordered these branches to swallow rc=1 as a skip would have no regression guard.
Fix: Add a run-all-recipes_test.sh case with a temp KWOK_DIR fixture containing duplicate system/GPU profiles + matching overlay; assert run_recipe_test returns 1 in both explicit and implicit modes.
| **Direct vs batch semantics.** `apply-nodes.sh <recipe>` (and | ||
| `make kwok-e2e RECIPE=...`) fails closed with the full diagnostic when | ||
| no profile matches. `run-all-recipes.sh` (and `make kwok-test-all`, | ||
| plus the CI matrix) instead **skips** unmapped recipes with a WARN so |
There was a problem hiding this comment.
🟡 Minor — README direct-vs-batch says the CI matrix 'skips unmapped recipes with a WARN' — CI actually DROPS at classify and explicit-fails
Accurate only for implicit make kwok-test-all. In CI, discover DROPS the recipe at classify time (::notice, never dispatched); and an explicitly-named recipe reaching run-all-recipes.sh via action.yml:178 fails rc=1, not skips. This paragraph is the canonical description of the new fail-closed contract, so the drift is worth fixing.
Blast radius: Doc-only, but a future contributor could 'fix' the explicit-fail path believing CI is meant to skip, reopening the false-green hole.
Fix: Document three distinct modes — direct → fail closed; implicit make kwok-test-all → SKIP-with-WARN; CI → DROP at classify + explicit-fail — and qualify 'Zero matches is a hard error' as direct-path only.
| relpath="${profile#"${profiles_root}"/}" | ||
| # Skip anything whose provider label disagrees with its directory — | ||
| # protects against a mis-copied file living under the wrong service. | ||
| if [[ "${provider}" != "${service}" ]]; then |
There was a problem hiding this comment.
🔵 Nitpick — provider != directory mismatch is silently continue'd, not a hard error
A profile whose provider label disagrees with its parent directory is skipped. For a stray file this is the intended defense (documented at L188-189). But if the mislabeled file is the SOLE system/GPU profile for that role, the mislabel degrades to an invisible rc=2 no-match (batch SKIP / CI drop) rather than surfacing the tree-integrity fault.
Blast radius: A single-character typo in provider on the sole eks profile silently zeroes coverage for every eks recipe, routed through the skippable no-match path.
Fix: Optional: return rc=1 with a diagnostic when a file under / declares provider=, rather than continue.
| EXPLICIT_RECIPES="${UNMAPPED_RECIPE}" | ||
| rc=0 | ||
| run_recipe_test "${UNMAPPED_RECIPE}" >/dev/null 2>&1 || rc=$? | ||
| check "explicit-unmapped-recipe-is-not-reported-as-passed" ne 0 "${rc}" |
There was a problem hiding this comment.
🔵 Nitpick — Regression test 1 asserts only rc != 0 with output discarded — vacuous-pass risk
apply-nodes.sh is not stubbed (only cleanup_between_tests is). If the explicit-fail branch were removed, execution would fall through to apply-nodes.sh and fail there (no cluster) → rc!=0, and the test would still PASS — for a downstream failure, not the explicit-unmapped guard it claims to protect. ne 0 can't distinguish 'failed for the right reason' from 'failed for any reason'.
Blast radius: The test proving explicit-unmapped fails closed can go green even after that logic is removed.
Fix: Assert rc == 1 specifically and capture the log to assert it contains the explicit-branch diagnostic ('explicitly requested but has no KWOK profile').
| # resolution (which uses BASH_SOURCE) points at the real lib directory. | ||
| TMP_SOURCE=$(mktemp "${SCRIPT_DIR}/.run-all-recipes.test.XXXXXX.sh") | ||
| trap 'rm -f "${TMP_SOURCE}"' EXIT | ||
| grep -v '^main "\$@"$' "${SCRIPT_UNDER_TEST}" > "${TMP_SOURCE}" |
There was a problem hiding this comment.
🔵 Nitpick — Harness strips main "$@" with an exact-anchored grep; a reformat silently no-ops the strip
grep -v '^main "\$@"$' removes the tail call by exact-line match before sourcing. If run-all-recipes.sh's tail line ever changes (trailing whitespace, exec main, a comment), the pattern stops matching, main "$@" stays in the temp copy, and sourcing executes main() with the test's args — driving into real kind/kubectl ops. No assertion that a line was removed.
Blast radius: Currently benign; would fail loudly (cluster ops) rather than pass silently, so future-fragility only.
Fix: Assert exactly one line was removed (or that main "$@" is absent from TMP_SOURCE) before sourcing; optionally define a no-op main() after sourcing.
| echo "${fails} test(s) failed" | ||
| exit 1 | ||
| fi | ||
| echo "All 22 tests passed" |
There was a problem hiding this comment.
🔵 Nitpick — 'All 22 tests passed' is a hardcoded literal, not a derived counter
The success banner prints a literal count rather than a per-check counter. Counts match today, but if a contributor adds a case and forgets to bump the string — or an early exit skips trailing checks without failing — the harness can print 'All 22 tests passed' while a different number ran, and CI stays green.
Blast radius: A silently short-circuited or under-counted run reports success.
Fix: Increment a ran counter inside check() and print/assert it (echo "All ${ran} tests passed").
| write_overlay "${OVERLAY_DIR}/missing.yaml" | ||
| got_out=$(resolve_recipe_criteria "${OVERLAY_DIR}/missing.yaml" 2>/dev/null) | ||
| got_rc=$? | ||
| check "resolve-missing-defaults-to-eks-h100" 0 "eks h100" "" "${got_rc}" "${got_out}" "" |
There was a problem hiding this comment.
🔵 Nitpick — Untested select_profiles / criteria branches
No case covers: an unknown nodeType (the case statement has no default arm, so a typo'd nodeType silently drops the profile → skippable no-match), an accelerated profile missing its accelerator label (joins available_accels as ""), a mislabeled SYSTEM profile (test 8 covers only the accelerated role), or mixed explicit-service + defaulted-accelerator criteria.
Blast radius: A one-character typo in nodeType/provider on a real profile turns a should-run recipe into a silent skip — the same coverage-lie the PR targets, via a different field.
Fix: Add the four cases; consider a default nodeType arm that warns on unrecognized values and test it.
3 participants
Replace
apply-nodes.sh's hardcoded profile mapping with label-drivendiscovery so unmapped
(service, accelerator)combinations can no longersilently fall back to
eks/p5-h100.yaml.Motivation / Context
get_profiles()inkwok/scripts/apply-nodes.shhardcoded a smallservice+accelerator → profile map and defaulted everything else to
amd64 H100. #1985 hit the failure mode: four GB300 GKE lanes reported
green while the CI logs showed the simulated nodes were amd64 H100 —
the arm64 GB300 profiles the PR added were never exercised. The tests
said "your recipe schedules correctly" when they had actually tested
completely different hardware.
Fixes: #1997
Related: #1985
Type of Change
Component(s) Affected
kwok/scripts/,kwok/README.md,.github/workflows/kwok-recipes.yaml)Implementation Notes
Profile selection moves to
kwok/scripts/lib/profile-select.shand isdriven by profile-declared labels rather than a hardcoded case
statement.
select_profiles(service, accelerator, profiles_root)walkskwok/profiles/<service>/and matches:provider == <service>andnodeType == systemprovider == <service>andnodeType == acceleratedandaccelerator == <accelerator>Exactly one match per role is required. Zero or multiple matches — or
an unknown service/accelerator — is an error, with a diagnostic that
enumerates what is on disk. Adding a profile now requires only a YAML
file with the right labels; no bash edit.
Direct-vs-batch semantics are intentionally different:
apply-nodes.sh <recipe>,make kwok-e2e RECIPE=…) —fails closed with the full diagnostic. If you explicitly ask about a
recipe, you get the honest answer.
run-all-recipes.sh,make kwok-test-all, CI) — skipsunmapped recipes with a
WARNand returnsrc=0. Matrix coveragestays green while profiles are backfilled in follow-up PRs; a recipe
that can't be simulated is not the same as a recipe that failed.
resolve_recipe_criteriais extracted alongsideselect_profilessoapply-nodes.shandrun-all-recipes.shshare one defaulting policy(
null/any→eks/h100) and cannot drift.Exposed by this change (currently silently passing, will
SKIPaftermerge until profiles are added):
a100-eks-*, allgke-*, allaks-*,all
oke-*, allbcm-*recipes. Each is a candidate for a follow-upprofile PR.
Testing
New unit tests cover: happy paths (eks/h100, eks/gb200), unmapped
service, unmapped accelerator, missing system profile, ambiguous
system/gpu profiles, mislabeled provider in wrong directory, argument
validation, nonexistent roots, and resolve_recipe_criteria
explicit/default/any/missing-overlay paths. End-to-end skip logic
verified against the real overlay tree — 4 mapped recipes proceed,
7 unmapped ones skip cleanly.
Risk Assessment
production code paths affected, purely CI/dev-loop scaffolding.
Rollout notes: No user-visible change. Recipes without matching
profiles will move from silent false-passes to explicit SKIP lines in
the CI logs; follow-up PRs add the missing profiles per provider/GPU.
Checklist