feat(mcp): add a read-only MCP server for certification state - #372
Open
ntheanh201 wants to merge 7 commits into
Open
ntheanh201 wants to merge 7 commits into
ntheanh201 wants to merge 7 commits into
Conversation
Operators increasingly run agents alongside NVCRE. Answering 'did this certification pass, and which nodes failed?' today means a person running the CLI and reading CRD status — a repetitive lookup loop an agent could run, but only against a typed interface rather than scraped CLI output. Add 'nvcrectl mcp serve' on the official Go MCP SDK (github.com/modelcontextprotocol/go-sdk v1.7.0, served over stdio). It exposes four read-only tools backed by the same data sources nvcrectl uses: list_categories (pkg/catalog), get_certification_status (Certification status + pkg/report.CertFailedNodes), get_certification_report (pkg/report.Build — the same JSON that 'report --results-file' writes), and list_failed_nodes (per-node reason/message from the failed-nodes ConfigMaps via pkg/report.FailedNodesFromRef). The server is deliberately read-only (issue NVIDIA#242): no tool creates, mutates, or deletes a resource and nothing triggers a run, since runs consume real GPU time. All tools carry the MCP readOnlyHint annotation. Authentication flows strictly through the caller's kubeconfig via the standard client-go loading rules (--kubeconfig/--context flags, then KUBECONFIG, then ~/.kube/config), so an agent can never exceed the permissions of whoever launched it; no service account tokens, no credential storage. Tests drive a full MCP session over in-memory transports against a fake client: a golden-file test pinning all four tools' JSON output, plus checks that exactly four read-only-annotated tools are exposed and that not-found certifications return a tool error. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
THIRD_PARTY_NOTICES.md lists the license of every direct dependency of the nvcrectl binary and ships as a release asset, so the new MCP SDK dependency belongs here. There is no generator target for this file; it is maintained by hand (as in dbf9121), so this adds the v1.7.0 index entry and the verbatim license text. The SDK is in a MIT-to-Apache-2.0 licensing transition, hence both licenses listed. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
get_certification_status walked the Certification CR and re-derived the
verdict itself, so it drifted from the report every other surface prints:
- it never applied the PASSED -> INCOMPLETE downgrade report.Build makes
when a Workflow excluded nodes (report.go:308), and carried no
excludedNodes field at all. A run that left eight of forty nodes
untested was reported to an agent as PASSED, in the cheaper tool an
agent reaches for first.
- it returned the raw InProgress category status where the report says
Running, so the two tools disagreed on vocabulary for the same object.
Project the summary from report.Build instead. The agreement stops being a
convention two code paths must maintain and becomes structural, and
excludedNodes/exclusionReason are surfaced so the INCOMPLETE verdict is
explainable rather than bare.
TestStatusAgreesWithReport asserts the two tools describe a Certification
identically across every fixture. The golden files could not have caught
this class: they record each tool independently, so a divergence sits
unnoticed in two blocks sixty lines apart. A new excluded-nodes fixture
covers the INCOMPLETE path; reintroducing the bug fails both the new
assertion and that golden.
Also corrects the tool descriptions, which are the model's contract:
get_certification_report no longer advertises per-node results, which
report.Build never populates, and list_failed_nodes now says it returns one
row per distinct reason and points at get_certification_status.failedNodes
for a unique node count.
Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
AGENTS.md requires an ADR for a new component; the MCP server landed without one. ADR-084 records the decisions that are not obvious from the code: why the surface is read-only (a run occupies the fleet it certifies, so the write surface is what needs justifying), why every verdict is projected from report.Build rather than re-derived, and why the two failed-node views deliberately differ. It also corrects a security claim that was not true. The docs, the cobra help and the package doc each stated the server "never reads in-cluster service account tokens". pkg/kubeconfig uses client-go's standard loading rules, which end in an in-cluster fallback: run `nvcrectl mcp serve` in a pod with no kubeconfig and it authenticates as that pod's ServiceAccount, which may be broader than the operator running the agent. The tools stay read-only either way, so this is a confidentiality claim rather than a privilege-escalation bug — but a guarantee that only holds outside a pod is worse than none, in a feature aimed at agents that commonly run in-cluster. State the resolution order accurately instead, and document the RBAC the tools need. That includes the ConfigMap read: pkg/report returns empty results rather than errors when it cannot read node results, so a caller missing that permission is told "no nodes failed" when the truth is "not allowed to look". Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
The tool table listed the status result as PASSED/FAILED/RUNNING. The tool returns INCOMPLETE too, and it is the value that matters most to a reader: it means the run passed but left targeted nodes untested, so treating it as a pass is exactly the mistake the value exists to prevent. Document what it means, not just that it exists. The report row promised per-node results in the same table. report.Build never populates NodeResults -- only pkg/workloadrun does -- so the promise was empty. The tool description in server.go was already corrected; this brings the docs in line. Also states the deliberate difference between the two failed-node views, since a caller counting rows from list_failed_nodes will over-count a node that failed in several categories. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
Addresses review on NVIDIA#288. The Instructions string still said authentication uses the kubeconfig of whoever launched the server. That is the claim this branch corrects in the docs, the cobra help, and the package doc, and Instructions is the one string a client reads at session start, so it was the copy that mattered most. An agent had three ways to be told nothing failed. get_certification_status omitted failedNodes entirely (omitempty), get_certification_report emitted null, and list_failed_nodes emitted []. A missing key and a null read as "unknown", not "zero", and the status tool's description points callers at failedNodes for the node count -- so the field that vanished was the one the feature exists to make unambiguous. Every collection now serializes as []. The invariant lives on the type as normalize() rather than in the handler, so a second construction site cannot lose it, and TestEmptyCollectionsSerializeAsArrays asserts the serialized bytes: a missing key, a null, and [] all decode to a nil slice, so TestStatusAgreesWithReport structurally cannot see the difference. That test then forced the remaining half. Normalising only the MCP side made status report [] where the shared report still returned null, and the agreement test failed -- correctly. CertFailedNodes built its union from a nil slice, so "no failed nodes" marshalled as null for the CLI too. It now starts non-nil. This is a change to shared code and to the JSON `nvcrectl certification report --results-file` writes, which is wider than this PR set out to touch: flagging it explicitly, and happy to split it out if you would rather take it separately. The RBAC paragraph listed three resources. report.Build also Gets the nvcre Job and the batch/v1 Job, and Lists GoodputMeasurement, BandwidthMeasurement, and Job -- every one best-effort, so a partial binding returns a successful report with metrics, bandwidth, and diagnose data quietly missing. The full set is now a table, with the note that certifications is fetched by name and needs get rather than list. Also moves a doc comment that described ExcludedNodes but sat above TotalNodes, where godoc attached it to the wrong field. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
- list_failed_nodes and report.CertFailedNodes now share one walk, report.CertFailedNodeDetails, and the agreement test checks that list_failed_nodes returns the same node names as the report. - get_certification_status declares result authoritative: on an INCOMPLETE run the raw conditions still read Succeeded=True. Signed-off-by: The Anh Nguyen <ntheanh201@gmail.com>
ntheanh201
marked this pull request as ready for review
September 24, 2026 03:50
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: NVIDIA/cluster-readiness-engine/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (12)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe CLI now exposes Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds a read-only 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 67.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 6 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
9 of 11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces #288, which was closed because a leaked credential was used to force-push unauthorized commits (
053bbe22) over its branch. Do not check out053bbe22: it adds a.vscode/tasks.jsonthat runs on folder open. This branch is rebuilt from the last clean state (e8a08ab3plus one unpushed review commit from 2026-09-09), rebased on currentmain.Adds
nvcrectl mcp serve: a read-only MCP server over stdio with four tools (list_categories,get_certification_status,get_certification_report,list_failed_nodes). Every certification verdict is projected fromreport.Build, so the tools cannot disagree withnvcrectl certification report. Design: ADR-084 (Status: Proposed).Closes #242
New dependency
github.com/modelcontextprotocol/go-sdk v1.7.0(Apache-2.0 / MIT), in thenvcrectlbinary only; recorded inTHIRD_PARTY_NOTICES.md. Happy to hand-roll the JSON-RPC framing instead if a new dependency is unwelcome.Review follow-up from #288
INCOMPLETEin the tool table (CodeRabbit)Instructionsstring still claimed kubeconfig-only auth (@ndipebot)get_certification_report(@ndipebot)certificationsneedsgetonlynull,[](@ndipebot)[];TestEmptyCollectionsSerializeAsArrayschecks the bytes.report.CertFailedNodesnow returns[]too, which also changes--results-fileJSONresultandconditionsdisagree onINCOMPLETE(@asivanadi0)resultdeclared authoritative in the tool description, field docs and CLI reference; pinned by the agreement testlist_failed_nodeswalked failures on its own (@asivanadi0)report.CertFailedNodeDetailsbehind both; agreement test checks the node names matchTesting
make testpasses (26 packages, envtest);make lint0 issues.TestMCPToolsdrives a real MCP session over in-memory transports;TestStatusAgreesWithReportchecks status, report andlist_failed_nodesagree across all fixtures;TestListToolspins the four read-only tools.Type of Change
Component(s) Affected
Checklist
git commit -s)make manifests generaterun (if*_types.gowas modified) — n/a, no API types changed