From 0c7c89c105a878d6c55f61dbdddf5e5306c364ac Mon Sep 17 00:00:00 2001 From: The Anh Nguyen Date: Wed, 2 Sep 2026 09:59:11 +0700 Subject: [PATCH 1/7] feat(mcp): add read-only MCP server exposing certification state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 --- cmd/nvcrectl/main.go | 2 + docs/cli-reference/mcp.md | 64 ++++ docs/cli-reference/overview.md | 1 + docs/index.yml | 2 + go.mod | 5 + go.sum | 12 + pkg/mcp/command.go | 67 ++++ pkg/mcpserver/codec.go | 23 ++ pkg/mcpserver/server.go | 335 ++++++++++++++++++ pkg/mcpserver/server_test.go | 208 +++++++++++ .../testdata/mcp-tools/basic/expected.txt | 158 +++++++++ .../testdata/mcp-tools/basic/input_calls.json | 26 ++ .../mcp-tools/basic/input_client_objects.yaml | 232 ++++++++++++ 13 files changed, 1135 insertions(+) create mode 100644 docs/cli-reference/mcp.md create mode 100644 pkg/mcp/command.go create mode 100644 pkg/mcpserver/codec.go create mode 100644 pkg/mcpserver/server.go create mode 100644 pkg/mcpserver/server_test.go create mode 100644 pkg/mcpserver/testdata/mcp-tools/basic/expected.txt create mode 100644 pkg/mcpserver/testdata/mcp-tools/basic/input_calls.json create mode 100644 pkg/mcpserver/testdata/mcp-tools/basic/input_client_objects.yaml diff --git a/cmd/nvcrectl/main.go b/cmd/nvcrectl/main.go index d4416bea..bf22c1e0 100644 --- a/cmd/nvcrectl/main.go +++ b/cmd/nvcrectl/main.go @@ -11,6 +11,7 @@ import ( _ "github.com/NVIDIA/cluster-readiness-engine/pkg/catalog" "github.com/NVIDIA/cluster-readiness-engine/pkg/certification" "github.com/NVIDIA/cluster-readiness-engine/pkg/cluster" + "github.com/NVIDIA/cluster-readiness-engine/pkg/mcp" "github.com/NVIDIA/cluster-readiness-engine/pkg/render" "github.com/NVIDIA/cluster-readiness-engine/pkg/setup" "github.com/NVIDIA/cluster-readiness-engine/pkg/workloadrun" @@ -34,6 +35,7 @@ func newRootCommand() *cobra.Command { root.AddCommand( certification.NewCommand(version), cluster.NewCommand(), + mcp.NewCommand(version), render.NewWorkflowCommand(), setup.NewCommand(version), workloadrun.NewCommand(), diff --git a/docs/cli-reference/mcp.md b/docs/cli-reference/mcp.md new file mode 100644 index 00000000..89300840 --- /dev/null +++ b/docs/cli-reference/mcp.md @@ -0,0 +1,64 @@ +--- +title: nvcrectl mcp +description: Serve read-only NVCRE certification state to MCP agents. +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +--- + + +## nvcrectl mcp serve + +Exposes NVCRE certification state to [Model Context Protocol](https://modelcontextprotocol.io) (MCP) agents over the stdio transport, so an agent can answer "did this certification pass, and which nodes failed?" without scraping CLI output. + +```bash +nvcrectl mcp serve [flags] +``` + +### Tools + +The server is strictly read-only: no tool creates, mutates, or deletes a resource, and nothing triggers a run — runs consume real GPU time. Every tool carries the MCP `readOnlyHint` annotation. + +| Tool | Description | +|------|-------------| +| `list_categories` | List the certification catalog: every registered `domain/variant` category a Certification can run | +| `get_certification_status` | Overall result (`PASSED`/`FAILED`/`RUNNING`), conditions, per-category state, and failed node names for one Certification | +| `get_certification_report` | The full report that `nvcrectl certification report` prints — categories with metrics, bandwidth, cliques, diagnose results, and per-node results | +| `list_failed_nodes` | Failed nodes for one Certification with per-node failure reason and message | + +The three certification-scoped tools accept `name` and `namespace` (default `default`). + +### Authentication + +All cluster access uses the kubeconfig of whoever launches the server, resolved with the standard client-go rules: the `--kubeconfig`/`--context` flags, then the `KUBECONFIG` environment variable, then `~/.kube/config`. The server holds no credentials of its own, never reads in-cluster service account tokens, and can therefore never exceed the permissions of the user who runs it. + +### Client configuration + +Most MCP clients spawn the server themselves over stdio. For example, in Claude Desktop or any client with a similar config format: + +```json +{ + "mcpServers": { + "nvcre": { + "command": "nvcrectl", + "args": ["mcp", "serve"] + } + } +} +``` + +### Flags + +| Flag | Default | Description | +|------|---------|-------------| +| `--kubeconfig` | `~/.kube/config` | Path to the kubeconfig file to use for requests | +| `--context` | current context | Name of the kubeconfig context to use | + +### Example + +```bash +# Serve with the default kubeconfig (typical: launched by the MCP client) +nvcrectl mcp serve + +# Serve against a specific kubeconfig and context +nvcrectl mcp serve --kubeconfig /path/to/kubeconfig --context gpu-cluster +``` diff --git a/docs/cli-reference/overview.md b/docs/cli-reference/overview.md index 12eb229e..ab127761 100644 --- a/docs/cli-reference/overview.md +++ b/docs/cli-reference/overview.md @@ -28,6 +28,7 @@ The installer also creates a `kubectl-nvcre` symlink so the CLI is available as | `nvcrectl workloadrun` | Run, render, report, status, and cancel WorkloadRun resources | | `nvcrectl cluster` | Inspect GPU nodes, platform, and network topology | | `nvcrectl workflow` | Render Workflow manifests offline with overrides applied | +| `nvcrectl mcp` | Serve read-only certification state to MCP agents over stdio | ## Global flags diff --git a/docs/index.yml b/docs/index.yml index ce46fa13..6d335815 100644 --- a/docs/index.yml +++ b/docs/index.yml @@ -66,6 +66,8 @@ navigation: path: cli-reference/cluster.md - page: workflow path: cli-reference/workflow.md + - page: mcp + path: cli-reference/mcp.md - section: API Reference contents: diff --git a/go.mod b/go.mod index ea58a78a..1681399c 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/evanphx/json-patch/v5 v5.9.11 github.com/google/cel-go v0.31.0 github.com/kubeflow/trainer/v2 v2.2.1 + github.com/modelcontextprotocol/go-sdk v1.7.0 github.com/prometheus/client_golang v1.24.1 github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 @@ -52,6 +53,7 @@ require ( github.com/go-openapi/swag/typeutils v0.28.0 // indirect github.com/go-openapi/swag/yamlutils v0.28.0 // indirect github.com/google/gnostic-models v0.7.1 // indirect + github.com/google/jsonschema-go v0.4.3 // indirect github.com/google/pprof v0.0.0-20260802141513-ef3492d7dac3 // indirect github.com/google/uuid v1.6.0 // indirect github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674 // indirect @@ -69,7 +71,10 @@ require ( github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.70.1 // indirect github.com/prometheus/procfs v0.21.1 // indirect + github.com/segmentio/asm v1.1.3 // indirect + github.com/segmentio/encoding v0.5.4 // indirect github.com/x448/float16 v0.8.4 // indirect + github.com/yosida95/uritemplate/v3 v3.0.2 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.70.0 // indirect go.opentelemetry.io/otel v1.45.0 // indirect diff --git a/go.sum b/go.sum index 1a5c5f98..cb12fbf6 100644 --- a/go.sum +++ b/go.sum @@ -74,6 +74,8 @@ github.com/go-openapi/testify/v2 v2.6.0 h1:5PKH2HE7YJ/LuRPQGvSxBRlFXNQhSetBLlGAg github.com/go-openapi/testify/v2 v2.6.0/go.mod h1:SgsVHtfooshd0tublTtJ50FPKhujf47YRqauXXOUxfw= github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI= github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8= +github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= +github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek= github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= github.com/google/cel-go v0.31.0 h1:H0bhpFTqOvmHrBGrWKp7ZlhBm5Hh8PYUEXnwxT1LL7A= @@ -85,6 +87,8 @@ github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0= github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/google/jsonschema-go v0.4.3 h1:/DBOLZTfDow7pe2GmaJNhltueGTtDKICi8V8p+DQPd0= +github.com/google/jsonschema-go v0.4.3/go.mod h1:r5quNTdLOYEz95Ru18zA0ydNbBuYoo9tgaYcxEYhJVE= github.com/google/pprof v0.0.0-20260802141513-ef3492d7dac3 h1:LMLX+LgTNWpfvCBdFebv6EsYotImrt/Ppc5cXIriCSo= github.com/google/pprof v0.0.0-20260802141513-ef3492d7dac3/go.mod h1:jl5iWTm0/hd5PjEYEOuwAJ57L/CibdZfrqZ5XA5GrCk= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= @@ -105,6 +109,8 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/moby/spdystream v0.5.1 h1:9sNYeYZUcci9R6/w7KDaFWEWeV4LStVG78Mpyq/Zm/Y= github.com/moby/spdystream v0.5.1/go.mod h1:xBAYlnt/ay+11ShkdFKNAG7LsyK/tmNBVvVOwrfMgdI= +github.com/modelcontextprotocol/go-sdk v1.7.0 h1:yqjY2dsbKAC0LSuWZVBMrHgiG8ukXv6NRo0JiALay44= +github.com/modelcontextprotocol/go-sdk v1.7.0/go.mod h1:dL7u98E/zjJTGzEq+j30jQ8K2k1mb6LeAH4inEcSGts= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= @@ -131,6 +137,10 @@ github.com/prometheus/common v0.70.1/go.mod h1:VdFUQDMZK3VLkurFUVhia6uys/0suUp86 github.com/prometheus/procfs v0.21.1 h1:GljZCt+zSTS+NZq88cyQ1LjZ+RCHp3uVuabBWA5+OJI= github.com/prometheus/procfs v0.21.1/go.mod h1:aB55Cww9pdSJVHk0hUf0inxWyyjPogFIjmHKYgMKmtY= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= +github.com/segmentio/asm v1.1.3 h1:WM03sfUOENvvKexOLp+pCqgb/WDjsi7EK8gIsICtzhc= +github.com/segmentio/asm v1.1.3/go.mod h1:Ld3L4ZXGNcSLRg4JBsZ3//1+f/TjYl0Mzen/DQy1EJg= +github.com/segmentio/encoding v0.5.4 h1:OW1VRern8Nw6ITAtwSZ7Idrl3MXCFwXHPgqESYfvNt0= +github.com/segmentio/encoding v0.5.4/go.mod h1:HS1ZKa3kSN32ZHVZ7ZLPLXWvOVIiZtyJnO1gPH1sKt0= github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= @@ -146,6 +156,8 @@ github.com/vladimirvivien/gexe v0.5.0 h1:AWBVaYnrTsGYBktXvcO0DfWPeSiZxn6mnQ5nvL+ github.com/vladimirvivien/gexe v0.5.0/go.mod h1:3gjgTqE2c0VyHnU5UOIwk7gyNzZDGulPb/DJPgcw64E= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= +github.com/yosida95/uritemplate/v3 v3.0.2 h1:Ed3Oyj9yrmi9087+NczuL5BwkIc4wvTb5zIM+UJPGz4= +github.com/yosida95/uritemplate/v3 v3.0.2/go.mod h1:ILOh0sOhIJR3+L/8afwt/kE++YT040gmv5BQTMR2HP4= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= go.opentelemetry.io/auto/sdk v1.2.1/go.mod h1:KRTj+aOaElaLi+wW1kO/DZRXwkF4C5xPbEe3ZiIhN7Y= go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.70.0 h1:LMuyCAyfalSjDyjdC65nK6N0zoTT63+E/u95X0JovZI= diff --git a/pkg/mcp/command.go b/pkg/mcp/command.go new file mode 100644 index 00000000..cd0525cd --- /dev/null +++ b/pkg/mcp/command.go @@ -0,0 +1,67 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package mcp wires the read-only MCP server into the nvcrectl command tree. +package mcp + +import ( + "github.com/spf13/cobra" + + "github.com/NVIDIA/cluster-readiness-engine/pkg/kubeconfig" + "github.com/NVIDIA/cluster-readiness-engine/pkg/mcpserver" + "github.com/NVIDIA/cluster-readiness-engine/pkg/render" +) + +// NewCommand returns the "mcp" cobra command. +func NewCommand(version string) *cobra.Command { + cmd := &cobra.Command{ + Use: "mcp", + Short: "Model Context Protocol (MCP) server exposing read-only certification state", + } + cmd.AddCommand(newServeCommand(version)) + return cmd +} + +// newServeCommand serves the MCP server over stdio. The flags mirror every +// other cluster-connecting subcommand: authentication resolves strictly +// through the caller's kubeconfig (explicit flags, KUBECONFIG env, then +// ~/.kube/config) — the server never reads in-cluster service account tokens, +// so it cannot exceed the permissions of whoever runs it. +func newServeCommand(version string) *cobra.Command { + configFlags := kubeconfig.NewConfigFlags(true) + configFlags.Namespace = nil // the namespace is a per-tool argument + + cmd := &cobra.Command{ + Use: "serve", + Short: "Serve the read-only NVCRE MCP server over stdio", + Long: `Expose NVCRE certification state to MCP agents over stdio. + +Four read-only tools are available: + - list_categories the certification catalog (domain/variant) + - get_certification_status overall result, conditions, and per-category state + - get_certification_report the full report nvcrectl certification report prints + - list_failed_nodes failed nodes with per-node reason and message + +The server is strictly read-only: no tool creates, mutates, or deletes a +resource, and nothing triggers a run. Authentication flows through the +kubeconfig of whoever launches the server, so an agent can never exceed that +user's permissions. + +Most MCP clients spawn this command themselves, e.g.: + + "nvcre": { + "command": "nvcrectl", + "args": ["mcp", "serve"] + }`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + c, err := render.NewK8sClient(configFlags) + if err != nil { + return err + } + return mcpserver.Run(cmd.Context(), &mcpserver.Store{Client: c}, version) + }, + } + configFlags.AddFlags(cmd.Flags()) + return cmd +} diff --git a/pkg/mcpserver/codec.go b/pkg/mcpserver/codec.go new file mode 100644 index 00000000..72e12d1f --- /dev/null +++ b/pkg/mcpserver/codec.go @@ -0,0 +1,23 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mcpserver + +import ( + "encoding/json" + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// textResult marshals out as the tool's text content. All four tools return +// JSON so agents parse one stable shape. +func textResult(out any) (*mcp.CallToolResult, any, error) { + data, err := json.MarshalIndent(out, "", " ") + if err != nil { + return nil, nil, fmt.Errorf("marshal tool output: %w", err) + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: string(data)}}, + }, nil, nil +} diff --git a/pkg/mcpserver/server.go b/pkg/mcpserver/server.go new file mode 100644 index 00000000..fdaffcdb --- /dev/null +++ b/pkg/mcpserver/server.go @@ -0,0 +1,335 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package mcpserver exposes NVCRE certification state to MCP (Model Context +// Protocol) agents as a set of read-only tools. +// +// The server answers "did this certification pass, and which nodes failed?" +// from the same typed data sources the nvcrectl report command reads — +// catalog.List, report.Build, and the failed-nodes ConfigMaps — so an agent +// never has to scrape CLI output. +// +// Authentication flows strictly through the caller's kubeconfig (standard +// client-go loading rules: --kubeconfig/--context flags, KUBECONFIG env, then +// ~/.kube/config). The server can therefore never exceed the permissions of +// whoever runs it, and holds no credentials of its own. +// +// The tool set is read-only by design (issue #242): no tool creates, mutates, +// or deletes a resource, and nothing triggers a run — runs consume real GPU +// time. Every tool also carries the MCP readOnlyHint annotation so clients +// can treat the whole surface as non-mutating. +package mcpserver + +import ( + "context" + "fmt" + "sort" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + nvcrev1alpha1 "github.com/NVIDIA/cluster-readiness-engine/api/v1alpha1" + "github.com/NVIDIA/cluster-readiness-engine/pkg/catalog" + "github.com/NVIDIA/cluster-readiness-engine/pkg/controller" + "github.com/NVIDIA/cluster-readiness-engine/pkg/report" +) + +// defaultNamespace matches the nvcrectl default: tools that omit the +// namespace argument resolve against "default". +const defaultNamespace = "default" + +// Store holds the cluster-backed data sources behind the MCP tools. Tests +// inject a fake client; production builds the client from the kubeconfig +// flags (see NewCommand). +type Store struct { + // Client reads Certification, Workflow, and node-results ConfigMap + // objects. Only read paths are exercised — the handlers call + // report.Build, report.FailedNodesFromRef, and plain Gets, all of which + // issue reads against the API server. + Client client.Client + // Catalog lists the registered certification categories. A nil Catalog + // defaults to catalog.List. + Catalog func() []catalog.CategoryInfo +} + +// New returns an MCP server with the four read-only NVCRE tools registered. +// An empty version resolves to "dev". +func New(store *Store, version string) *mcp.Server { + if store.Catalog == nil { + store.Catalog = catalog.List + } + if version == "" { + version = "dev" + } + s := mcp.NewServer(&mcp.Implementation{ + Name: "nvcre", + Title: "NVIDIA Cluster Readiness Engine", + Version: version, + }, &mcp.ServerOptions{ + Instructions: "Read-only access to NVCRE certification state. " + + "Authentication uses the kubeconfig of whoever launched the server.", + }) + + // Generic mcp.AddTool derives the input/output JSON schemas from the + // typed handler arguments, validates tool input before it reaches the + // handler, and packs handler errors into the tool result (IsError=true) + // instead of failing the protocol call. + readOnly := &mcp.ToolAnnotations{ReadOnlyHint: true} + add0 := func(tool *mcp.Tool, handler mcp.ToolHandlerFor[struct{}, any]) { + tool.Annotations = readOnly + mcp.AddTool(s, tool, handler) + } + add1 := func(tool *mcp.Tool, handler mcp.ToolHandlerFor[certRef, any]) { + tool.Annotations = readOnly + mcp.AddTool(s, tool, handler) + } + + add0(listCategoriesTool(), listCategoriesHandler(store)) + add1(getCertStatusTool(), getCertStatusHandler(store)) + add1(getCertReportTool(), getCertReportHandler(store)) + add1(listFailedNodesTool(), listFailedNodesHandler(store)) + return s +} + +// Run serves the MCP server over stdio until the context is cancelled or the +// client disconnects. Stdio keeps the transport local: the agent spawns the +// server itself, so kubeconfig credentials never leave the machine. +func Run(ctx context.Context, store *Store, version string) error { + return New(store, version).Run(ctx, &mcp.StdioTransport{}) +} + +// --------------------------------------------------------------------------- +// Tool descriptors +// --------------------------------------------------------------------------- + +func listCategoriesTool() *mcp.Tool { + return &mcp.Tool{ + Name: "list_categories", + Description: "List the certification catalog: every registered category (domain/variant) a Certification can run.", + } +} + +func getCertStatusTool() *mcp.Tool { + return &mcp.Tool{ + Name: "get_certification_status", + Description: "Get the status of one Certification: overall result (PASSED/FAILED/RUNNING), conditions, per-category state, and failed node names.", + } +} + +func getCertReportTool() *mcp.Tool { + return &mcp.Tool{ + Name: "get_certification_report", + Description: "Fetch the full certification report: categories with metrics, bandwidth, cliques, diagnose results, and per-node results.", + } +} + +func listFailedNodesTool() *mcp.Tool { + return &mcp.Tool{ + Name: "list_failed_nodes", + Description: "List the failed nodes for a Certification with per-node failure reason and message.", + } +} + +// --------------------------------------------------------------------------- +// Tool handlers — thin adapters over the shared NVCRE data access +// --------------------------------------------------------------------------- + +func listCategoriesHandler(store *Store) mcp.ToolHandlerFor[struct{}, any] { + return func(ctx context.Context, req *mcp.CallToolRequest, _ struct{}) (*mcp.CallToolResult, any, error) { + out := listCategoriesOutput{Categories: []categorySummary{}} + for _, c := range store.Catalog() { + out.Categories = append(out.Categories, categorySummary{ + Domain: c.Domain, + Variant: c.Variant, + }) + } + return textResult(out) + } +} + +func getCertStatusHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { + return func(ctx context.Context, req *mcp.CallToolRequest, in certRef) (*mcp.CallToolResult, any, error) { + cert, err := store.certification(ctx, in) + if err != nil { + return nil, nil, err + } + + out := &getCertStatusOutput{ + Name: cert.Name, + Namespace: cert.Namespace, + Result: certResult(cert), + Conditions: []conditionInfo{}, + Categories: []categoryState{}, + FailedNodes: report.CertFailedNodes(ctx, store.Client, cert), + } + for _, c := range cert.Status.Conditions { + out.Conditions = append(out.Conditions, conditionInfo{ + Type: c.Type, + Status: string(c.Status), + Reason: c.Reason, + Message: c.Message, + }) + } + for _, cs := range cert.Status.CategoryStatuses { + out.Categories = append(out.Categories, categoryState{ + Domain: cs.Domain, + Variant: cs.Variant, + Status: cs.Status, + }) + } + return textResult(out) + } +} + +func getCertReportHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { + return func(ctx context.Context, req *mcp.CallToolRequest, in certRef) (*mcp.CallToolResult, any, error) { + cert, err := store.certification(ctx, in) + if err != nil { + return nil, nil, err + } + // report.Build is the same builder 'nvcrectl certification report' + // uses; the JSON shape matches --results-file output. + return textResult(&getCertReportOutput{ + Report: report.Build(ctx, store.Client, cert), + }) + } +} + +func listFailedNodesHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { + return func(ctx context.Context, req *mcp.CallToolRequest, in certRef) (*mcp.CallToolResult, any, error) { + cert, err := store.certification(ctx, in) + if err != nil { + return nil, nil, err + } + + seen := map[string]bool{} + details := []failedNodeDetail{} + for _, cat := range cert.Status.CategoryStatuses { + for _, n := range report.FailedNodesFromRef(ctx, store.Client, cert.Namespace, cat.FailedNodesRef) { + key := n.Name + "|" + string(n.Reason) + "|" + n.Message + if n.Name == "" || seen[key] { + continue + } + seen[key] = true + details = append(details, failedNodeDetail{ + Name: n.Name, + Reason: string(n.Reason), + Message: n.Message, + }) + } + } + // Deterministic order: node name, then reason, then message. + sort.Slice(details, func(i, j int) bool { + a, b := details[i], details[j] + if a.Name != b.Name { + return a.Name < b.Name + } + if a.Reason != b.Reason { + return a.Reason < b.Reason + } + return a.Message < b.Message + }) + return textResult(&listFailedNodesOutput{ + Name: cert.Name, + Namespace: cert.Namespace, + FailedNodes: details, + }) + } +} + +// --------------------------------------------------------------------------- +// Shared handler plumbing +// --------------------------------------------------------------------------- + +// certRef is the shared input of the three certification-scoped tools. +type certRef struct { + Name string `json:"name" jsonschema:"name of the Certification resource"` + // Namespace defaults to "default" when omitted. + Namespace string `json:"namespace,omitempty" jsonschema:"namespace of the Certification (default: default)"` +} + +// listCategoriesOutput lists every registered certification category. The +// catalog is static, so the tool takes no arguments. +type listCategoriesOutput struct { + Categories []categorySummary `json:"categories"` +} + +type categorySummary struct { + Domain string `json:"domain"` + Variant string `json:"variant"` +} + +// getCertStatusOutput summarizes a Certification's overall and per-category +// state without pulling measurement data. +type getCertStatusOutput struct { + Name string `json:"name"` + Namespace string `json:"namespace"` + Result string `json:"result"` // "PASSED", "FAILED", or "RUNNING" + Conditions []conditionInfo `json:"conditions,omitempty"` + Categories []categoryState `json:"categories,omitempty"` + FailedNodes []string `json:"failedNodes,omitempty"` +} + +type conditionInfo struct { + Type string `json:"type"` + Status string `json:"status"` + Reason string `json:"reason,omitempty"` + Message string `json:"message,omitempty"` +} + +type categoryState struct { + Domain string `json:"domain"` + Variant string `json:"variant"` + Status string `json:"status"` +} + +// getCertReportOutput wraps the shared report model, so agents parse the +// same JSON shape nvcrectl writes with --results-file. +type getCertReportOutput struct { + Report *report.CertReport `json:"report"` +} + +// listFailedNodesOutput returns per-node failure details. +type listFailedNodesOutput struct { + Name string `json:"name"` + Namespace string `json:"namespace"` + FailedNodes []failedNodeDetail `json:"failedNodes"` +} + +type failedNodeDetail struct { + Name string `json:"name"` + Reason string `json:"reason"` + Message string `json:"message,omitempty"` +} + +// certification fetches the named Certification, mirroring the nvcrectl +// report command's error messages. +func (s *Store) certification(ctx context.Context, ref certRef) (*nvcrev1alpha1.Certification, error) { + ns := ref.Namespace + if ns == "" { + ns = defaultNamespace + } + cert := &nvcrev1alpha1.Certification{} + key := client.ObjectKey{Name: ref.Name, Namespace: ns} + if err := s.Client.Get(ctx, key, cert); err != nil { + if errors.IsNotFound(err) { + return nil, fmt.Errorf("certification %q not found in namespace %q", ref.Name, ns) + } + return nil, fmt.Errorf("get certification %q: %w", ref.Name, err) + } + return cert, nil +} + +// certResult maps Certification conditions to the report's result strings, +// matching report.Build. +func certResult(cert *nvcrev1alpha1.Certification) string { + switch { + case controller.CondIsTrue(cert.Status.Conditions, nvcrev1alpha1.CertificationFailed): + return "FAILED" + case controller.CondIsTrue(cert.Status.Conditions, nvcrev1alpha1.CertificationSucceeded): + return "PASSED" + default: + return "RUNNING" + } +} diff --git a/pkg/mcpserver/server_test.go b/pkg/mcpserver/server_test.go new file mode 100644 index 00000000..33c63b56 --- /dev/null +++ b/pkg/mcpserver/server_test.go @@ -0,0 +1,208 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mcpserver + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + nvcrev1alpha1 "github.com/NVIDIA/cluster-readiness-engine/api/v1alpha1" + "github.com/NVIDIA/cluster-readiness-engine/pkg/catalog" + "github.com/NVIDIA/cluster-readiness-engine/pkg/testutil" +) + +// connect builds a server on top of a fake client and drives a full MCP +// session against it over in-memory transports, so the tests exercise the +// same protocol surface an agent sees (initialize, tools/list, tools/call). +func connect(t *testing.T, store *Store) *mcp.ClientSession { + t.Helper() + server := New(store, "test") + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "0.0.1"}, nil) + ctx := context.Background() + serverTransport, clientTransport := mcp.NewInMemoryTransports() + serverSession, err := server.Connect(ctx, serverTransport, nil) + if err != nil { + t.Fatalf("connect server: %v", err) + } + t.Cleanup(func() { _ = serverSession.Close() }) + clientSession, err := client.Connect(ctx, clientTransport, nil) + if err != nil { + t.Fatalf("connect client: %v", err) + } + t.Cleanup(func() { _ = clientSession.Close() }) + return clientSession +} + +// fakeStore builds a Store backed by a controller-runtime fake client holding +// the test case's input objects, mirroring the pkg/report golden tests. +func fakeStore(t *testing.T, tc *testutil.TestCase) *Store { + t.Helper() + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := nvcrev1alpha1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + objs, _, err := tc.GetObjects(scheme) + if err != nil { + t.Fatal(err) + } + builder := fake.NewClientBuilder().WithScheme(scheme) + for _, o := range objs { + builder = builder.WithObjects(o) + } + return &Store{Client: builder.Build()} +} + +// callTool invokes the named tool with args and returns the parsed JSON of +// its text content. +func callTool(t *testing.T, session *mcp.ClientSession, name string, args any) string { + t.Helper() + res, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: name, + Arguments: args, + }) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + if res.IsError { + t.Fatalf("%s returned tool error: %v", name, res.Content) + } + if len(res.Content) != 1 { + t.Fatalf("%s: want 1 content item, got %d", name, len(res.Content)) + } + text, ok := res.Content[0].(*mcp.TextContent) + if !ok { + t.Fatalf("%s: content is %T, want TextContent", name, res.Content[0]) + } + // Round-trip through a generic map so the golden file records the parsed + // JSON object rather than an escaped string. + var v any + if err := json.Unmarshal([]byte(text.Text), &v); err != nil { + t.Fatalf("%s: text content is not JSON: %v", name, err) + } + pretty, err := json.MarshalIndent(v, "", " ") + if err != nil { + t.Fatal(err) + } + return string(pretty) +} + +// TestMCPTools exercises every tool against the golden files under +// testdata/mcp-tools. Each case directory holds the input cluster objects +// (input_client_objects.yaml) and the tool calls to run in input_calls.json; +// the golden file records the JSON responses in order. +func TestMCPTools(t *testing.T) { + p := testutil.TestCaseParser{ + Subdir: "mcp-tools", + ExpectedSuffix: ".txt", + } + p.TestDir(t, func(tc *testutil.TestCase) error { + store := fakeStore(t, tc) + session := connect(t, store) + + var calls []struct { + Tool string `json:"tool"` + Arguments map[string]any `json:"arguments,omitempty"` + } + if err := json.Unmarshal([]byte(tc.Inputs["input_calls.json"]), &calls); err != nil { + return fmt.Errorf("parse input_calls.json: %w", err) + } + + var out strings.Builder + for _, c := range calls { + fmt.Fprintf(&out, "### %s\n", c.Tool) + fmt.Fprintf(&out, "%s\n", callTool(t, session, c.Tool, c.Arguments)) + } + tc.Actual = out.String() + return nil + }) +} + +// emptyStore returns a Store backed by a fake client holding no objects and +// no registered catalog categories. +func emptyStore() *Store { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + panic(err) + } + if err := nvcrev1alpha1.AddToScheme(scheme); err != nil { + panic(err) + } + return &Store{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + Catalog: func() []catalog.CategoryInfo { return nil }, + } +} + +// TestListTools pins the tool surface: exactly the four read-only tools, all +// carrying the readOnlyHint annotation. An extra or mutating tool added by +// mistake fails here. +func TestListTools(t *testing.T) { + store := emptyStore() + session := connect(t, store) + + res, err := session.ListTools(context.Background(), nil) + if err != nil { + t.Fatal(err) + } + got := map[string]bool{} + for _, tool := range res.Tools { + if tool.Annotations == nil || !tool.Annotations.ReadOnlyHint { + t.Errorf("tool %q is missing the readOnlyHint annotation", tool.Name) + } + got[tool.Name] = true + } + for _, want := range []string{ + "list_categories", + "get_certification_status", + "get_certification_report", + "list_failed_nodes", + } { + if !got[want] { + t.Errorf("tool %q is not exposed", want) + } + } + if len(res.Tools) != 4 { + t.Errorf("got %d tools, want exactly the 4 read-only tools: %v", + len(res.Tools), res.Tools) + } +} + +// TestNotFound verifies the not-found path of the certification-scoped tools +// returns a tool error mentioning the name and namespace. +func TestNotFound(t *testing.T) { + store := emptyStore() + session := connect(t, store) + + for _, tool := range []string{"get_certification_status", "get_certification_report", "list_failed_nodes"} { + res, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: tool, + Arguments: map[string]any{"name": "missing", "namespace": "ns1"}, + }) + if err != nil { + t.Fatalf("%s: %v", tool, err) + } + if !res.IsError { + t.Fatalf("%s: want tool error for missing certification", tool) + } + text, ok := res.Content[0].(*mcp.TextContent) + if !ok { + t.Fatalf("%s: content is %T, want TextContent", tool, res.Content[0]) + } + want := `certification "missing" not found in namespace "ns1"` + if !strings.Contains(text.Text, want) { + t.Errorf("%s: error %q does not contain %q", tool, text.Text, want) + } + } +} diff --git a/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt b/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt new file mode 100644 index 00000000..3a388f85 --- /dev/null +++ b/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt @@ -0,0 +1,158 @@ +### list_categories +{ + "categories": [ + { + "domain": "communication", + "variant": "nccl-all-gather" + }, + { + "domain": "communication", + "variant": "nccl-all-reduce" + }, + { + "domain": "communication", + "variant": "nccl-alltoall" + }, + { + "domain": "communication", + "variant": "nccl-loopback" + }, + { + "domain": "communication", + "variant": "nccl-loopback-nvswitch" + }, + { + "domain": "diagnostics", + "variant": "dcgm-level4" + }, + { + "domain": "training", + "variant": "nemotron5-56b" + }, + { + "domain": "training", + "variant": "nemotron5-8b" + } + ] +} +### get_certification_status +{ + "categories": [ + { + "domain": "communication", + "status": "Failed", + "variant": "nccl-all-reduce" + }, + { + "domain": "training", + "status": "Succeeded", + "variant": "nemotron5-8b" + } + ], + "conditions": [ + { + "message": "1 of 2 certification categories failed", + "reason": "WorkflowsFailed", + "status": "True", + "type": "Failed" + } + ], + "failedNodes": [ + "node001", + "node002" + ], + "name": "cert-mcp", + "namespace": "default", + "result": "FAILED" +} +### get_certification_status +{ + "categories": [ + { + "domain": "training", + "status": "Succeeded", + "variant": "nemotron5-8b" + } + ], + "conditions": [ + { + "message": "1 of 1 certification categories succeeded", + "reason": "CategoriesSucceeded", + "status": "True", + "type": "Succeeded" + } + ], + "name": "cert-pass", + "namespace": "default", + "result": "PASSED" +} +### get_certification_report +{ + "report": { + "categories": [ + { + "domain": "communication", + "failedGroups": [ + { + "name": "group-0", + "nodeCount": 3, + "nodes": [ + "node001", + "node002", + "node003" + ], + "reason": "" + } + ], + "failureReason": "1 groups failed across 1 iterations", + "jobs": 1, + "nodesPerJob": 3, + "runtime": "10m 0s", + "status": "Failed", + "testScale": "full-scale", + "variant": "nccl-all-reduce" + }, + { + "domain": "training", + "jobs": 1, + "nodesPerJob": 2, + "runtime": "20m 0s", + "status": "Succeeded", + "testScale": "full-scale", + "variant": "nemotron5-8b" + } + ], + "failedNodes": [ + "node001", + "node002" + ], + "gpu": "h100", + "name": "cert-mcp", + "platform": "onprem", + "result": "FAILED", + "totalNodes": 3 + } +} +### list_failed_nodes +{ + "failedNodes": [ + { + "message": "Hardware failure detected: CEL expression 'nvidia.com/gpu.health != Healthy' matched node condition", + "name": "node001", + "reason": "HardwareFailureDetected" + }, + { + "message": "Threshold \"busBandwidth\" violated: measured 412.50 GB/s, expression: value \u003e= 400", + "name": "node002", + "reason": "ThresholdViolation" + } + ], + "name": "cert-mcp", + "namespace": "default" +} +### list_failed_nodes +{ + "failedNodes": [], + "name": "cert-pass", + "namespace": "default" +} diff --git a/pkg/mcpserver/testdata/mcp-tools/basic/input_calls.json b/pkg/mcpserver/testdata/mcp-tools/basic/input_calls.json new file mode 100644 index 00000000..800f45bb --- /dev/null +++ b/pkg/mcpserver/testdata/mcp-tools/basic/input_calls.json @@ -0,0 +1,26 @@ +[ + { + "tool": "list_categories", + "arguments": {} + }, + { + "tool": "get_certification_status", + "arguments": {"name": "cert-mcp"} + }, + { + "tool": "get_certification_status", + "arguments": {"name": "cert-pass"} + }, + { + "tool": "get_certification_report", + "arguments": {"name": "cert-mcp"} + }, + { + "tool": "list_failed_nodes", + "arguments": {"name": "cert-mcp"} + }, + { + "tool": "list_failed_nodes", + "arguments": {"name": "cert-pass"} + } +] diff --git a/pkg/mcpserver/testdata/mcp-tools/basic/input_client_objects.yaml b/pkg/mcpserver/testdata/mcp-tools/basic/input_client_objects.yaml new file mode 100644 index 00000000..a06314e3 --- /dev/null +++ b/pkg/mcpserver/testdata/mcp-tools/basic/input_client_objects.yaml @@ -0,0 +1,232 @@ +# End-to-end fixture for the four read-only MCP tools: a failed +# Certification with two categories — one with a failed-nodes ConfigMap +# (hardware + threshold failures) and one succeeded — plus an succeeded-only +# second certification for the pass path. +apiVersion: nvcre.nvidia.com/v1alpha1 +kind: Certification +metadata: + name: cert-mcp + namespace: default +spec: + target: + nodeSelector: + nvidia.com/gpu.present: "true" + categories: + - domain: communication + variant: nccl-all-reduce + - domain: training + variant: nemotron5-8b +status: + conditions: + - type: Failed + status: "True" + reason: WorkflowsFailed + message: 1 of 2 certification categories failed + lastTransitionTime: "2026-08-07T01:00:00Z" + categoryStatuses: + - domain: communication + variant: nccl-all-reduce + status: Failed + workflowRef: + name: cert-mcp-communication-nccl-all-reduce + namespace: default + failedNodesRef: + kind: ConfigMap + name: cert-mcp-communication-nccl-all-reduce-node-results + - domain: training + variant: nemotron5-8b + status: Succeeded + workflowRef: + name: cert-mcp-training-nemotron5-8b + namespace: default +--- +apiVersion: nvcre.nvidia.com/v1alpha1 +kind: Workflow +metadata: + name: cert-mcp-communication-nccl-all-reduce + namespace: default + labels: + nvcre.nvidia.com/certification: cert-mcp +spec: + jobTemplate: + spec: + workload: + trainJob: + runtimeRef: + kind: TrainingRuntime + name: rt + trainer: + image: img:latest + numNodes: 2 + orchestration: + iterations: 1 + target: + nodeSelector: + nvidia.com/gpu.present: "true" +status: + orchestration: + totalNodes: 3 + nodesPerJob: 3 + totalGroups: 1 + currentIteration: 1 + detectedPlatform: onprem + detectedGPUArchitecture: h100 + groups: + - name: group-0 + nodes: + - node001 + - node002 + - node003 + phase: Failed + jobRef: + apiVersion: nvcre.nvidia.com/v1alpha1 + kind: Job + name: cert-mcp-communication-nccl-all-reduce-job-group-0 + namespace: default + startTime: "2026-08-07T00:00:00Z" + completionTime: "2026-08-07T00:10:00Z" + conditions: + - type: Failed + status: "True" + reason: GroupsFailed + message: 1 groups failed across 1 iterations + lastTransitionTime: "2026-08-07T00:10:00Z" +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cert-mcp-communication-nccl-all-reduce-node-results + namespace: default + labels: + nvcre.nvidia.com/workflow: cert-mcp-communication-nccl-all-reduce +binaryData: + failed-nodes.json.gz: H4sIAAAAAAAA/1zOu07rQBDG8VeZM00ay9nkhMZSmnBLQYloMMXg+ciuZO9Ge3FAKO+OYhCy6Eaa/yf9nj/ZywBu2AeFMSuuOEJS8NzwXqKeJOJOXF8ibpDRZShXPCAlOWDW0Nt3RPpTNXR9+0B4P0ak5IKnhR+dOqm7MCwPx1JbSJ8t/dvSfro+FjRI7iyULhbqgleXXfB8rv4o13Plo41INvT65EIv02AO/H1Ty68l7cTryWm2LdM4DS7UAZJKhNJmta6vDN3vlqma4RsapS+gthjzH1vaGMPnl68AAAD//67xLaI9AQAA +--- +apiVersion: nvcre.nvidia.com/v1alpha1 +kind: Workflow +metadata: + name: cert-mcp-training-nemotron5-8b + namespace: default + labels: + nvcre.nvidia.com/certification: cert-mcp +spec: + jobTemplate: + spec: + workload: + trainJob: + runtimeRef: + kind: TrainingRuntime + name: rt + trainer: + image: img:latest + numNodes: 2 + orchestration: + iterations: 1 + target: + nodeSelector: + nvidia.com/gpu.present: "true" +status: + orchestration: + totalNodes: 2 + nodesPerJob: 2 + totalGroups: 1 + currentIteration: 1 + detectedPlatform: onprem + detectedGPUArchitecture: h100 + groups: + - name: group-0 + nodes: + - node001 + - node002 + phase: Succeeded + jobRef: + apiVersion: nvcre.nvidia.com/v1alpha1 + kind: Job + name: cert-mcp-training-nemotron5-8b-job-group-0 + namespace: default + startTime: "2026-08-07T00:00:00Z" + completionTime: "2026-08-07T00:20:00Z" + conditions: + - type: Succeeded + status: "True" + reason: AllGroupsSucceeded + message: 1 groups succeeded across 1 iterations + lastTransitionTime: "2026-08-07T00:20:00Z" +--- +apiVersion: nvcre.nvidia.com/v1alpha1 +kind: Certification +metadata: + name: cert-pass + namespace: default +spec: + target: + nodeSelector: + nvidia.com/gpu.present: "true" + categories: + - domain: training + variant: nemotron5-8b +status: + conditions: + - type: Succeeded + status: "True" + reason: CategoriesSucceeded + message: 1 of 1 certification categories succeeded + lastTransitionTime: "2026-08-07T01:00:00Z" + categoryStatuses: + - domain: training + variant: nemotron5-8b + status: Succeeded + workflowRef: + name: cert-pass-training-nemotron5-8b + namespace: default +--- +apiVersion: nvcre.nvidia.com/v1alpha1 +kind: Workflow +metadata: + name: cert-pass-training-nemotron5-8b + namespace: default + labels: + nvcre.nvidia.com/certification: cert-pass +spec: + jobTemplate: + spec: + workload: + trainJob: + runtimeRef: + kind: TrainingRuntime + name: rt + trainer: + image: img:latest + numNodes: 2 + orchestration: + iterations: 1 + target: + nodeSelector: + nvidia.com/gpu.present: "true" +status: + orchestration: + totalNodes: 2 + nodesPerJob: 2 + totalGroups: 1 + currentIteration: 1 + detectedPlatform: onprem + detectedGPUArchitecture: h100 + groups: + - name: group-0 + nodes: + - node001 + - node002 + phase: Succeeded + jobRef: + apiVersion: nvcre.nvidia.com/v1alpha1 + kind: Job + name: cert-pass-training-nemotron5-8b-job-group-0 + namespace: default + startTime: "2026-08-07T00:00:00Z" + completionTime: "2026-08-07T00:20:00Z" + conditions: + - type: Succeeded + status: "True" + reason: AllGroupsSucceeded + message: 1 groups succeeded across 1 iterations + lastTransitionTime: "2026-08-07T00:20:00Z" From f5ffd4d85e28da65a258e645eacf9f0f7f9425f4 Mon Sep 17 00:00:00 2001 From: The Anh Nguyen Date: Wed, 2 Sep 2026 09:59:13 +0700 Subject: [PATCH 2/7] docs: add MCP Go SDK to third-party notices 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 --- THIRD_PARTY_NOTICES.md | 228 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 228 insertions(+) diff --git a/THIRD_PARTY_NOTICES.md b/THIRD_PARTY_NOTICES.md index 71336db0..98118fe1 100644 --- a/THIRD_PARTY_NOTICES.md +++ b/THIRD_PARTY_NOTICES.md @@ -15,6 +15,7 @@ also excluded. | `golang.org/x/text` | v0.42.0 | BSD-3-Clause | https://github.com/golang/text/blob/v0.42.0/LICENSE | | `github.com/evanphx/json-patch/v5` | v5.9.11 | BSD-3-Clause | https://github.com/evanphx/json-patch/blob/v5.9.11/LICENSE | | `github.com/google/cel-go` | v0.31.0 | Apache-2.0, BSD-3-Clause | https://github.com/google/cel-go/blob/v0.31.0/LICENSE | +| `github.com/modelcontextprotocol/go-sdk` | v1.7.0 | Apache-2.0, MIT | https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE | | `github.com/kubeflow/trainer/v2` | v2.2.1 | Apache-2.0 | https://github.com/kubeflow/trainer/blob/v2.2.1/LICENSE | | `github.com/prometheus/client_golang` | v1.24.1 | Apache-2.0 | https://github.com/prometheus/client_golang/blob/v1.24.1/LICENSE | | `github.com/spf13/cobra` | v1.10.2 | Apache-2.0 | https://github.com/spf13/cobra/blob/v1.10.2/LICENSE.txt | @@ -2372,3 +2373,230 @@ Apache license: See the License for the specific language governing permissions and limitations under the License. ``` + +### github.com/modelcontextprotocol/go-sdk + +* Version: v1.7.0 +* License: Apache-2.0, MIT +* Source: https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE + +#### LICENSE + +```text +The MCP project is undergoing a licensing transition from the MIT License to the Apache License, Version 2.0 ("Apache-2.0"). All new code and specification contributions to the project are licensed under Apache-2.0. Documentation contributions (excluding specifications) are licensed under CC-BY-4.0. + +Contributions for which relicensing consent has been obtained are licensed under Apache-2.0. Contributions made by authors who originally licensed their work under the MIT License and who have not yet granted explicit permission to relicense remain licensed under the MIT License. + +No rights beyond those granted by the applicable original license are conveyed for such contributions. + +--- + + Apache License + Version 2.0, January 2004 + http://www.apache.org/licenses/ + + TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION + + 1. Definitions. + + "License" shall mean the terms and conditions for use, reproduction, + and distribution as defined by Sections 1 through 9 of this document. + + "Licensor" shall mean the copyright owner or entity authorized by + the copyright owner that is granting the License. + + "Legal Entity" shall mean the union of the acting entity and all + other entities that control, are controlled by, or are under common + control with that entity. For the purposes of this definition, + "control" means (i) the power, direct or indirect, to cause the + direction or management of such entity, whether by contract or + otherwise, or (ii) ownership of fifty percent (50%) or more of the + outstanding shares, or (iii) beneficial ownership of such entity. + + "You" (or "Your") shall mean an individual or Legal Entity + exercising permissions granted by this License. + + "Source" form shall mean the preferred form for making modifications, + including but not limited to software source code, documentation + source, and configuration files. + + "Object" form shall mean any form resulting from mechanical + transformation or translation of a Source form, including but + not limited to compiled object code, generated documentation, + and conversions to other media types. + + "Work" shall mean the work of authorship, whether in Source or + Object form, made available under the License, as indicated by a + copyright notice that is included in or attached to the work + (an example is provided in the Appendix below). + + "Derivative Works" shall mean any work, whether in Source or Object + form, that is based on (or derived from) the Work and for which the + editorial revisions, annotations, elaborations, or other modifications + represent, as a whole, an original work of authorship. For the purposes + of this License, Derivative Works shall not include works that remain + separable from, or merely link (or bind by name) to the interfaces of, + the Work and Derivative Works thereof. + + "Contribution" shall mean any work of authorship, including + the original version of the Work and any modifications or additions + to that Work or Derivative Works thereof, that is intentionally + submitted to Licensor for inclusion in the Work by the copyright + owner or by an individual or Legal Entity authorized to submit on behalf + of the copyright owner. For the purposes of this definition, "submitted" + means any form of electronic, verbal, or written communication sent + to the Licensor or its representatives, including but not limited to + communication on electronic mailing lists, source code control systems, + and issue tracking systems that are managed by, or on behalf of, the + Licensor for the purpose of discussing and improving the Work, but + excluding communication that is conspicuously marked or otherwise + designated in writing by the copyright owner as "Not a Contribution." + + "Contributor" shall mean Licensor and any individual or Legal Entity + on behalf of whom a Contribution has been received by Licensor and + subsequently incorporated within the Work. + + 2. Grant of Copyright License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + copyright license to reproduce, prepare Derivative Works of, + publicly display, publicly perform, sublicense, and distribute the + Work and such Derivative Works in Source or Object form. + + 3. Grant of Patent License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + (except as stated in this section) patent license to make, have made, + use, offer to sell, sell, import, and otherwise transfer the Work, + where such license applies only to those patent claims licensable + by such Contributor that are necessarily infringed by their + Contribution(s) alone or by combination of their Contribution(s) + with the Work to which such Contribution(s) was submitted. If You + institute patent litigation against any entity (including a + cross-claim or counterclaim in a lawsuit) alleging that the Work + or a Contribution incorporated within the Work constitutes direct + or contributory patent infringement, then any patent licenses + granted to You under this License for that Work shall terminate + as of the date such litigation is filed. + + 4. Redistribution. You may reproduce and distribute copies of the + Work or Derivative Works thereof in any medium, with or without + modifications, and in Source or Object form, provided that You + meet the following conditions: + + (a) You must give any other recipients of the Work or + Derivative Works a copy of this License; and + + (b) You must cause any modified files to carry prominent notices + stating that You changed the files; and + + (c) You must retain, in the Source form of any Derivative Works + that You distribute, all copyright, patent, trademark, and + attribution notices from the Source form of the Work, + excluding those notices that do not pertain to any part of + the Derivative Works; and + + (d) If the Work includes a "NOTICE" text file as part of its + distribution, then any Derivative Works that You distribute must + include a readable copy of the attribution notices contained + within such NOTICE file, excluding those notices that do not + pertain to any part of the Derivative Works, in at least one + of the following places: within a NOTICE text file distributed + as part of the Derivative Works; within the Source form or + documentation, if provided along with the Derivative Works; or, + within a display generated by the Derivative Works, if and + wherever such third-party notices normally appear. The contents + of the NOTICE file are for informational purposes only and + do not modify the License. You may add Your own attribution + notices within Derivative Works that You distribute, alongside + or as an addendum to the NOTICE text from the Work, provided + that such additional attribution notices cannot be construed + as modifying the License. + + You may add Your own copyright statement to Your modifications and + may provide additional or different license terms and conditions + for use, reproduction, or distribution of Your modifications, or + for any such Derivative Works as a whole, provided Your use, + reproduction, and distribution of the Work otherwise complies with + the conditions stated in this License. + + 5. Submission of Contributions. Unless You explicitly state otherwise, + any Contribution intentionally submitted for inclusion in the Work + by You to the Licensor shall be under the terms and conditions of + this License, without any additional terms or conditions. + Notwithstanding the above, nothing herein shall supersede or modify + the terms of any separate license agreement you may have executed + with Licensor regarding such Contributions. + + 6. Trademarks. This License does not grant permission to use the trade + names, trademarks, service marks, or product names of the Licensor, + except as required for reasonable and customary use in describing the + origin of the Work and reproducing the content of the NOTICE file. + + 7. Disclaimer of Warranty. Unless required by applicable law or + agreed to in writing, Licensor provides the Work (and each + Contributor provides its Contributions) on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + implied, including, without limitation, any warranties or conditions + of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A + PARTICULAR PURPOSE. You are solely responsible for determining the + appropriateness of using or redistributing the Work and assume any + risks associated with Your exercise of permissions under this License. + + 8. Limitation of Liability. In no event and under no legal theory, + whether in tort (including negligence), contract, or otherwise, + unless required by applicable law (such as deliberate and grossly + negligent acts) or agreed to in writing, shall any Contributor be + liable to You for damages, including any direct, indirect, special, + incidental, or consequential damages of any character arising as a + result of this License or out of the use or inability to use the + Work (including but not limited to damages for loss of goodwill, + work stoppage, computer failure or malfunction, or any and all + other commercial damages or losses), even if such Contributor + has been advised of the possibility of such damages. + + 9. Accepting Warranty or Additional Liability. While redistributing + the Work or Derivative Works thereof, You may choose to offer, + and charge a fee for, acceptance of support, warranty, indemnity, + or other liability obligations and/or rights consistent with this + License. However, in accepting such obligations, You may act only + on Your own behalf and on Your sole responsibility, not on behalf + of any other Contributor, and only if You agree to indemnify, + defend, and hold each Contributor harmless for any liability + incurred by, or claims asserted against, such Contributor by reason + of your accepting any such warranty or additional liability. + + END OF TERMS AND CONDITIONS + +--- + +MIT License + +Copyright (c) 2024-2025 Model Context Protocol a Series of LF Projects, LLC. + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. + +--- + +Creative Commons Attribution 4.0 International (CC-BY-4.0) + +Documentation in this project (excluding specifications) is licensed under +CC-BY-4.0. See https://creativecommons.org/licenses/by/4.0/legalcode for +the full license text. +``` From 93dfc5b8e8dcf44a28839724205b5447109241d2 Mon Sep 17 00:00:00 2001 From: The Anh Nguyen Date: Wed, 2 Sep 2026 17:32:04 +0700 Subject: [PATCH 3/7] fix(mcp): project certification status from report.Build 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 --- docs/cli-reference/mcp.md | 8 +- pkg/mcp/command.go | 16 +-- pkg/mcpserver/server.go | 88 ++++++++--------- pkg/mcpserver/server_test.go | 99 +++++++++++++++++-- .../testdata/mcp-tools/basic/expected.txt | 6 +- .../mcp-tools/excluded-nodes/expected.txt | 54 ++++++++++ .../mcp-tools/excluded-nodes/input_calls.json | 10 ++ .../excluded-nodes/input_client_objects.yaml | 80 +++++++++++++++ 8 files changed, 301 insertions(+), 60 deletions(-) create mode 100644 pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt create mode 100644 pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_calls.json create mode 100644 pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_client_objects.yaml diff --git a/docs/cli-reference/mcp.md b/docs/cli-reference/mcp.md index 89300840..8ce052f9 100644 --- a/docs/cli-reference/mcp.md +++ b/docs/cli-reference/mcp.md @@ -29,7 +29,13 @@ The three certification-scoped tools accept `name` and `namespace` (default `def ### Authentication -All cluster access uses the kubeconfig of whoever launches the server, resolved with the standard client-go rules: the `--kubeconfig`/`--context` flags, then the `KUBECONFIG` environment variable, then `~/.kube/config`. The server holds no credentials of its own, never reads in-cluster service account tokens, and can therefore never exceed the permissions of the user who runs it. +All cluster access uses the kubeconfig of whoever launches the server, resolved with the standard client-go rules: the `--kubeconfig`/`--context` flags, then the `KUBECONFIG` environment variable, then `~/.kube/config`. The server holds no credentials of its own and adds no privilege of its own — it is exactly as authorized as the identity client-go resolves. + + +Client-go's loading rules end in an in-cluster fallback. Run `nvcrectl mcp serve` inside a pod with no kubeconfig and it authenticates as that pod's ServiceAccount, which may be broader than the operator running the agent. When you deploy the server in-cluster, bind its ServiceAccount to a role that grants no more than the reads below. + + +The tools need `get`/`list` on `certifications` and `workflows` in the target namespace, and `get` on the `configmaps` holding failed-node results. A caller missing the ConfigMap read still gets a successful response with an empty `failedNodes` list rather than an error, so grant the ConfigMap read explicitly — otherwise an agent can read "no nodes failed" from what is really "not allowed to look". ### Client configuration diff --git a/pkg/mcp/command.go b/pkg/mcp/command.go index cd0525cd..2afa3b10 100644 --- a/pkg/mcp/command.go +++ b/pkg/mcp/command.go @@ -23,10 +23,11 @@ func NewCommand(version string) *cobra.Command { } // newServeCommand serves the MCP server over stdio. The flags mirror every -// other cluster-connecting subcommand: authentication resolves strictly -// through the caller's kubeconfig (explicit flags, KUBECONFIG env, then -// ~/.kube/config) — the server never reads in-cluster service account tokens, -// so it cannot exceed the permissions of whoever runs it. +// other cluster-connecting subcommand, and so do the credentials: client-go's +// standard loading rules (explicit flags, KUBECONFIG env, then ~/.kube/config, +// falling back to the in-cluster ServiceAccount when the process runs in a pod +// with no kubeconfig). The server holds no credentials of its own and adds no +// privilege — it is exactly as authorized as the identity it resolves. func newServeCommand(version string) *cobra.Command { configFlags := kubeconfig.NewConfigFlags(true) configFlags.Namespace = nil // the namespace is a per-tool argument @@ -43,9 +44,10 @@ Four read-only tools are available: - list_failed_nodes failed nodes with per-node reason and message The server is strictly read-only: no tool creates, mutates, or deletes a -resource, and nothing triggers a run. Authentication flows through the -kubeconfig of whoever launches the server, so an agent can never exceed that -user's permissions. +resource, and nothing triggers a run. It holds no credentials of its own and +grants no privilege of its own — it acts as whatever identity client-go +resolves, which is the launching user's kubeconfig, or the pod ServiceAccount +if it is run in-cluster without one. Most MCP clients spawn this command themselves, e.g.: diff --git a/pkg/mcpserver/server.go b/pkg/mcpserver/server.go index fdaffcdb..c7ad2bdf 100644 --- a/pkg/mcpserver/server.go +++ b/pkg/mcpserver/server.go @@ -6,13 +6,9 @@ // // The server answers "did this certification pass, and which nodes failed?" // from the same typed data sources the nvcrectl report command reads — -// catalog.List, report.Build, and the failed-nodes ConfigMaps — so an agent -// never has to scrape CLI output. -// -// Authentication flows strictly through the caller's kubeconfig (standard -// client-go loading rules: --kubeconfig/--context flags, KUBECONFIG env, then -// ~/.kube/config). The server can therefore never exceed the permissions of -// whoever runs it, and holds no credentials of its own. +// catalog.List, report.Build, and the failed-nodes ConfigMaps. Every +// certification verdict is projected from report.Build rather than re-derived +// from the CR, so a tool can never disagree with the report the CLI prints. // // The tool set is read-only by design (issue #242): no tool creates, mutates, // or deletes a resource, and nothing triggers a run — runs consume real GPU @@ -31,7 +27,6 @@ import ( nvcrev1alpha1 "github.com/NVIDIA/cluster-readiness-engine/api/v1alpha1" "github.com/NVIDIA/cluster-readiness-engine/pkg/catalog" - "github.com/NVIDIA/cluster-readiness-engine/pkg/controller" "github.com/NVIDIA/cluster-readiness-engine/pkg/report" ) @@ -113,21 +108,21 @@ func listCategoriesTool() *mcp.Tool { func getCertStatusTool() *mcp.Tool { return &mcp.Tool{ Name: "get_certification_status", - Description: "Get the status of one Certification: overall result (PASSED/FAILED/RUNNING), conditions, per-category state, and failed node names.", + Description: "Get the status of one Certification: overall result (PASSED/INCOMPLETE/FAILED/RUNNING), conditions, per-category state, any nodes excluded from the run, and the unique names of failed nodes. INCOMPLETE means the run passed but left some targeted nodes untested.", } } func getCertReportTool() *mcp.Tool { return &mcp.Tool{ Name: "get_certification_report", - Description: "Fetch the full certification report: categories with metrics, bandwidth, cliques, diagnose results, and per-node results.", + Description: "Fetch the full certification report: categories with metrics, bandwidth, cliques and diagnose results. This is the same JSON 'nvcrectl certification report --results-file' writes.", } } func listFailedNodesTool() *mcp.Tool { return &mcp.Tool{ Name: "list_failed_nodes", - Description: "List the failed nodes for a Certification with per-node failure reason and message.", + Description: "List failure detail for a Certification: one row per distinct (node, reason, message). A node that failed in several categories appears once per distinct reason, so this is not a node count — use get_certification_status.failedNodes for unique node names.", } } @@ -155,13 +150,22 @@ func getCertStatusHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { return nil, nil, err } + // Project the summary from the same report the CLI prints rather than + // re-deriving it from the CR. Re-deriving drifted: it missed the + // PASSED -> INCOMPLETE downgrade for excluded nodes, and reported the + // raw InProgress category status where the report says Running. + rep := report.Build(ctx, store.Client, cert) + out := &getCertStatusOutput{ - Name: cert.Name, - Namespace: cert.Namespace, - Result: certResult(cert), - Conditions: []conditionInfo{}, - Categories: []categoryState{}, - FailedNodes: report.CertFailedNodes(ctx, store.Client, cert), + Name: cert.Name, + Namespace: cert.Namespace, + Result: rep.Result, + TotalNodes: rep.TotalNodes, + ExcludedNodes: rep.ExcludedNodes, + ExclusionReason: rep.ExclusionReason, + Conditions: []conditionInfo{}, + Categories: []categoryState{}, + FailedNodes: rep.FailedNodes, } for _, c := range cert.Status.Conditions { out.Conditions = append(out.Conditions, conditionInfo{ @@ -171,11 +175,11 @@ func getCertStatusHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { Message: c.Message, }) } - for _, cs := range cert.Status.CategoryStatuses { + for _, c := range rep.Categories { out.Categories = append(out.Categories, categoryState{ - Domain: cs.Domain, - Variant: cs.Variant, - Status: cs.Status, + Domain: c.Domain, + Variant: c.Variant, + Status: c.Status, }) } return textResult(out) @@ -219,7 +223,7 @@ func listFailedNodesHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { }) } } - // Deterministic order: node name, then reason, then message. + // Deterministic order for the goldens. sort.Slice(details, func(i, j int) bool { a, b := details[i], details[j] if a.Name != b.Name { @@ -244,8 +248,7 @@ func listFailedNodesHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { // certRef is the shared input of the three certification-scoped tools. type certRef struct { - Name string `json:"name" jsonschema:"name of the Certification resource"` - // Namespace defaults to "default" when omitted. + Name string `json:"name" jsonschema:"name of the Certification resource"` Namespace string `json:"namespace,omitempty" jsonschema:"namespace of the Certification (default: default)"` } @@ -261,14 +264,24 @@ type categorySummary struct { } // getCertStatusOutput summarizes a Certification's overall and per-category -// state without pulling measurement data. +// state without pulling measurement data. Every field except Conditions is +// projected from report.Build, so it agrees with get_certification_report. type getCertStatusOutput struct { - Name string `json:"name"` - Namespace string `json:"namespace"` - Result string `json:"result"` // "PASSED", "FAILED", or "RUNNING" - Conditions []conditionInfo `json:"conditions,omitempty"` - Categories []categoryState `json:"categories,omitempty"` - FailedNodes []string `json:"failedNodes,omitempty"` + Name string `json:"name"` + Namespace string `json:"namespace"` + Result string `json:"result"` // "PASSED", "INCOMPLETE", "FAILED", or "RUNNING" + // ExcludedNodes lists nodes that matched the target but were left + // untested; a run reports INCOMPLETE rather than PASSED when it has any. + // Surfaced here so the cheaper tool cannot hide them from an agent. + TotalNodes int `json:"totalNodes,omitempty"` + ExcludedNodes []string `json:"excludedNodes,omitempty"` + ExclusionReason string `json:"exclusionReason,omitempty"` + Conditions []conditionInfo `json:"conditions,omitempty"` + Categories []categoryState `json:"categories,omitempty"` + // FailedNodes is the unique node names that failed, deduplicated across + // categories. Use this for a node count; list_failed_nodes returns one + // row per distinct failure reason and so can repeat a name. + FailedNodes []string `json:"failedNodes,omitempty"` } type conditionInfo struct { @@ -320,16 +333,3 @@ func (s *Store) certification(ctx context.Context, ref certRef) (*nvcrev1alpha1. } return cert, nil } - -// certResult maps Certification conditions to the report's result strings, -// matching report.Build. -func certResult(cert *nvcrev1alpha1.Certification) string { - switch { - case controller.CondIsTrue(cert.Status.Conditions, nvcrev1alpha1.CertificationFailed): - return "FAILED" - case controller.CondIsTrue(cert.Status.Conditions, nvcrev1alpha1.CertificationSucceeded): - return "PASSED" - default: - return "RUNNING" - } -} diff --git a/pkg/mcpserver/server_test.go b/pkg/mcpserver/server_test.go index 33c63b56..88286bcf 100644 --- a/pkg/mcpserver/server_test.go +++ b/pkg/mcpserver/server_test.go @@ -7,6 +7,9 @@ import ( "context" "encoding/json" "fmt" + "os" + "path/filepath" + "reflect" "strings" "testing" @@ -23,7 +26,7 @@ import ( // connect builds a server on top of a fake client and drives a full MCP // session against it over in-memory transports, so the tests exercise the // same protocol surface an agent sees (initialize, tools/list, tools/call). -func connect(t *testing.T, store *Store) *mcp.ClientSession { +func connect(t testing.TB, store *Store) *mcp.ClientSession { t.Helper() server := New(store, "test") client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "0.0.1"}, nil) @@ -44,7 +47,7 @@ func connect(t *testing.T, store *Store) *mcp.ClientSession { // fakeStore builds a Store backed by a controller-runtime fake client holding // the test case's input objects, mirroring the pkg/report golden tests. -func fakeStore(t *testing.T, tc *testutil.TestCase) *Store { +func fakeStore(t testing.TB, tc *testutil.TestCase) *Store { t.Helper() scheme := runtime.NewScheme() if err := clientgoscheme.AddToScheme(scheme); err != nil { @@ -66,7 +69,7 @@ func fakeStore(t *testing.T, tc *testutil.TestCase) *Store { // callTool invokes the named tool with args and returns the parsed JSON of // its text content. -func callTool(t *testing.T, session *mcp.ClientSession, name string, args any) string { +func callTool(t testing.TB, session *mcp.ClientSession, name string, args any) string { t.Helper() res, err := session.CallTool(context.Background(), &mcp.CallToolParams{ Name: name, @@ -98,6 +101,86 @@ func callTool(t *testing.T, session *mcp.ClientSession, name string, args any) s return string(pretty) } +// TestStatusAgreesWithReport pins the invariant the golden files structurally +// cannot: get_certification_status and get_certification_report describe the +// same Certification, so they must never disagree about it. Recording each +// tool's output in a golden separately lets a divergence sit unnoticed in two +// blocks sixty lines apart, which is how the status tool came to report PASSED +// for a run the report called INCOMPLETE. +func TestStatusAgreesWithReport(t *testing.T) { + dirs, err := os.ReadDir(filepath.Join("testdata", "mcp-tools")) + if err != nil { + t.Fatalf("read testdata: %v", err) + } + for _, d := range dirs { + if !d.IsDir() { + continue + } + t.Run(d.Name(), func(t *testing.T) { + raw, err := os.ReadFile(filepath.Join("testdata", "mcp-tools", d.Name(), "input_client_objects.yaml")) + if err != nil { + t.Fatalf("read fixture objects: %v", err) + } + store := fakeStore(t, &testutil.TestCase{ + T: t, + Inputs: map[string]string{"input_client_objects.yaml": string(raw)}, + }) + session := connect(t, store) + + certs := &nvcrev1alpha1.CertificationList{} + if err := store.Client.List(context.Background(), certs); err != nil { + t.Fatalf("list certifications: %v", err) + } + if len(certs.Items) == 0 { + t.Fatal("fixture has no Certification to compare") + } + + for i := range certs.Items { + cert := &certs.Items[i] + args := map[string]any{"name": cert.Name, "namespace": cert.Namespace} + + var status certView + var rep struct { + Report certView `json:"report"` + } + unmarshalTool(t, callTool(t, session, "get_certification_status", args), &status) + unmarshalTool(t, callTool(t, session, "get_certification_report", args), &rep) + + if status.Result != rep.Report.Result { + t.Errorf("%s: result mismatch: status=%q report=%q", cert.Name, status.Result, rep.Report.Result) + } + if !reflect.DeepEqual(status.Categories, rep.Report.Categories) { + t.Errorf("%s: categories mismatch: status=%v report=%v", + cert.Name, status.Categories, rep.Report.Categories) + } + if !reflect.DeepEqual(status.FailedNodes, rep.Report.FailedNodes) { + t.Errorf("%s: failedNodes mismatch: status=%v report=%v", + cert.Name, status.FailedNodes, rep.Report.FailedNodes) + } + } + }) + } +} + +// certView is the subset of both tools' output that must agree. +type certView struct { + Result string `json:"result"` + Categories []struct { + Domain string `json:"domain"` + Variant string `json:"variant"` + Status string `json:"status"` + } `json:"categories"` + FailedNodes []string `json:"failedNodes"` +} + +// unmarshalTool decodes a tool's JSON text into v. +func unmarshalTool(t testing.TB, payload string, v any) { + t.Helper() + if err := json.Unmarshal([]byte(payload), v); err != nil { + t.Fatalf("decode tool output: %v", err) + } +} + // TestMCPTools exercises every tool against the golden files under // testdata/mcp-tools. Each case directory holds the input cluster objects // (input_client_objects.yaml) and the tool calls to run in input_calls.json; @@ -108,8 +191,12 @@ func TestMCPTools(t *testing.T) { ExpectedSuffix: ".txt", } p.TestDir(t, func(tc *testutil.TestCase) error { - store := fakeStore(t, tc) - session := connect(t, store) + // tc.T is the subtest's *testing.T. Using the outer t here would make + // any Fatal inside a case abort the parent, which surfaces as + // "subtest may have called FailNow on a parent test" and skips the + // golden comparison entirely. + store := fakeStore(tc.T, tc) + session := connect(tc.T, store) var calls []struct { Tool string `json:"tool"` @@ -122,7 +209,7 @@ func TestMCPTools(t *testing.T) { var out strings.Builder for _, c := range calls { fmt.Fprintf(&out, "### %s\n", c.Tool) - fmt.Fprintf(&out, "%s\n", callTool(t, session, c.Tool, c.Arguments)) + fmt.Fprintf(&out, "%s\n", callTool(tc.T, session, c.Tool, c.Arguments)) } tc.Actual = out.String() return nil diff --git a/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt b/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt index 3a388f85..7ea8fb3d 100644 --- a/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt +++ b/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt @@ -63,7 +63,8 @@ ], "name": "cert-mcp", "namespace": "default", - "result": "FAILED" + "result": "FAILED", + "totalNodes": 3 } ### get_certification_status { @@ -84,7 +85,8 @@ ], "name": "cert-pass", "namespace": "default", - "result": "PASSED" + "result": "PASSED", + "totalNodes": 2 } ### get_certification_report { diff --git a/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt new file mode 100644 index 00000000..ea164a24 --- /dev/null +++ b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt @@ -0,0 +1,54 @@ +### get_certification_status +{ + "categories": [ + { + "domain": "communication", + "status": "Succeeded", + "variant": "nccl-all-reduce" + } + ], + "conditions": [ + { + "message": "all certification categories succeeded", + "reason": "AllCategoriesSucceeded", + "status": "True", + "type": "Succeeded" + } + ], + "excludedNodes": [ + "node003", + "node004" + ], + "exclusionReason": "cordoned", + "name": "cert-excluded", + "namespace": "default", + "result": "INCOMPLETE", + "totalNodes": 2 +} +### get_certification_report +{ + "report": { + "categories": [ + { + "domain": "communication", + "jobs": 1, + "nodesPerJob": 2, + "runtime": "10m 0s", + "status": "Succeeded", + "testScale": "full-scale", + "variant": "nccl-all-reduce" + } + ], + "excludedNodes": [ + "node003", + "node004" + ], + "exclusionReason": "cordoned", + "failedNodes": null, + "gpu": "h100", + "name": "cert-excluded", + "platform": "onprem", + "result": "INCOMPLETE", + "totalNodes": 2 + } +} diff --git a/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_calls.json b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_calls.json new file mode 100644 index 00000000..8f7e41ca --- /dev/null +++ b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_calls.json @@ -0,0 +1,10 @@ +[ + { + "tool": "get_certification_status", + "arguments": {"name": "cert-excluded"} + }, + { + "tool": "get_certification_report", + "arguments": {"name": "cert-excluded"} + } +] diff --git a/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_client_objects.yaml b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_client_objects.yaml new file mode 100644 index 00000000..86c9d039 --- /dev/null +++ b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/input_client_objects.yaml @@ -0,0 +1,80 @@ +# A Certification that SUCCEEDED but whose Workflow left targeted nodes +# untested. report.Build downgrades that to INCOMPLETE (report.go:308), so +# get_certification_status must report INCOMPLETE too — reporting PASSED here +# would tell an agent the fleet is certified when two nodes were never tested. +apiVersion: nvcre.nvidia.com/v1alpha1 +kind: Certification +metadata: + name: cert-excluded + namespace: default +spec: + target: + nodeSelector: + nvidia.com/gpu.present: "true" + categories: + - domain: communication + variant: nccl-all-reduce +status: + conditions: + - type: Succeeded + status: "True" + reason: AllCategoriesSucceeded + message: all certification categories succeeded + lastTransitionTime: "2026-08-07T01:00:00Z" + categoryStatuses: + - domain: communication + variant: nccl-all-reduce + status: Succeeded + workflowRef: + name: cert-excluded-communication-nccl-all-reduce + namespace: default +--- +apiVersion: nvcre.nvidia.com/v1alpha1 +kind: Workflow +metadata: + name: cert-excluded-communication-nccl-all-reduce + namespace: default + labels: + nvcre.nvidia.com/certification: cert-excluded +spec: + jobTemplate: + spec: + workload: + trainJob: + runtimeRef: + kind: TrainingRuntime + name: rt + trainer: + image: img:latest + numNodes: 2 + orchestration: + iterations: 1 + target: + nodeSelector: + nvidia.com/gpu.present: "true" +status: + orchestration: + totalNodes: 2 + nodesPerJob: 2 + totalGroups: 1 + currentIteration: 1 + detectedPlatform: onprem + detectedGPUArchitecture: h100 + excludedNodes: + - node003 + - node004 + exclusionReason: cordoned + groups: + - name: group-0 + nodes: + - node001 + - node002 + phase: Succeeded + startTime: "2026-08-07T00:00:00Z" + completionTime: "2026-08-07T00:10:00Z" + conditions: + - type: Succeeded + status: "True" + reason: AllGroupsSucceeded + message: 1 groups succeeded across 1 iterations + lastTransitionTime: "2026-08-07T00:10:00Z" From 5f670bea2e2a0a7565fbd4e6317884f188f76b7c Mon Sep 17 00:00:00 2001 From: The Anh Nguyen Date: Wed, 2 Sep 2026 17:32:24 +0700 Subject: [PATCH 4/7] docs(designs): add ADR-084 and correct the MCP credential claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/designs/084-mcp-server.md | 41 ++++++++++++++++++++++++++++++++++ docs/designs/README.md | 1 + 2 files changed, 42 insertions(+) create mode 100644 docs/designs/084-mcp-server.md diff --git a/docs/designs/084-mcp-server.md b/docs/designs/084-mcp-server.md new file mode 100644 index 00000000..0888c0ef --- /dev/null +++ b/docs/designs/084-mcp-server.md @@ -0,0 +1,41 @@ +# ADR-084: A Read-Only MCP Server for Certification State + +> **Status:** Proposed + +## Context + +Answering "did this certification pass, and which nodes failed?" today means a person running `nvcrectl` and reading CRD status. Operators increasingly put agents in that loop, and an agent can only work against a typed interface — scraping CLI output couples it to formatting that is free to change (issue #242). + +Two properties make this more than a transport question. + +**Runs cost real GPU time.** A certification occupies the fleet it certifies. An interface that lets an agent start one turns a misread prompt into hours of contended hardware, so the write surface is the part that needs justifying, not the read surface. + +**Two views of the same certification can disagree.** `report.Build` ([report.go](../../pkg/report/report.go)) already owns the verdict: it normalises `InProgress` to `Running`, deduplicates failed nodes by name across categories, and downgrades `PASSED` to `INCOMPLETE` when the Workflow excluded nodes from the run — a rule that exists precisely so untested nodes cannot be invisible. Any second implementation of that logic is free to drift from it, and a drifted summary is worse than no summary: an agent quoting "PASSED" for a run that left eight nodes untested is confidently wrong in the direction that gets bad hardware shipped. + +## Decision + +1. **Ship the server in-repo, over stdio, exposing four read-only tools**: `list_categories`, `get_certification_status`, `get_certification_report`, `list_failed_nodes`. Stdio keeps the transport local — the agent spawns the process itself — which avoids introducing a network listener and the bearer-token or OAuth design that would come with it. + +2. **No tool creates, mutates, or deletes anything, and none triggers a run.** Every tool carries the MCP `readOnlyHint` annotation, and `TestListTools` pins the surface so adding a fifth or mutating tool fails the build. Whether agents may ever trigger runs is deferred; it needs its own decision about consumption and preemption, not a flag on this one. + +3. **Every certification verdict is projected from `report.Build`, never re-derived from the CR.** `get_certification_status` calls the same builder `nvcrectl certification report` uses and reads its fields, rather than walking conditions itself. This is the load-bearing decision: it makes "the tools agree with the CLI" a structural property instead of a convention two code paths have to keep. `TestStatusAgreesWithReport` asserts the two tools describe the same certification identically, across every fixture. + +4. **`list_failed_nodes` returns one row per distinct (node, reason, message); `get_certification_status.failedNodes` returns unique node names.** These are deliberately different: a node can fail two categories for two reasons, and both the detail and the count are useful. Because that difference is a real trap for a caller doing `len()`, each tool's description states which one it is and points at the other. + +5. **Credentials are whatever client-go resolves, and the docs say so exactly.** The server holds no credentials and adds no privilege. It is not, however, true that it "never reads service account tokens": client-go's standard loading rules end in an in-cluster fallback, so running it inside a pod without a kubeconfig authenticates as that pod's ServiceAccount. The documentation states this and tells operators to bind a least-privilege role when deploying in-cluster, because a security guarantee that is only true outside a pod is worse than none. + +## Consequences + +An agent gets certification state without scraping, and cannot start a run through this interface. + +The server inherits `pkg/report`'s error handling, which returns empty results rather than errors when a Workflow or node-results ConfigMap cannot be read. For a CLI a human reads that is a minor annoyance; for a tool an agent quotes, "no nodes failed" and "not allowed to look" become indistinguishable. This ADR does not change that shared behaviour — the docs name the RBAC the tools need and call out the failure mode. Surfacing read errors through `report.Build` is worth doing on its own merits and affects the CLI equally. + +Adding the MCP Go SDK makes it the first MCP dependency in `go.mod`, and it lands in the `nvcrectl` binary rather than the controller. + +## Alternatives Considered + +**Re-derive status from the Certification CR.** Cheaper — it avoids the Workflow reads `report.Build` performs — and it is what the first implementation did. It produced three divergences from the report (missing `INCOMPLETE`, raw `InProgress` instead of `Running`, and a different failed-node dedupe key) before any of them were noticed, because nothing compared the two. Rejected: the saving is a few reads, and the cost is an agent being told a run passed when it did not. + +**An HTTP/SSE transport.** Would let one server serve several agents, but requires deciding how it authenticates them and how it maps a caller to a kubeconfig. Deferred until something asks for it. + +**MCP resources instead of, or alongside, tools.** Resources suit static documents; certification state is queried by name and namespace. Four tools cover it without a second surface to keep consistent. diff --git a/docs/designs/README.md b/docs/designs/README.md index da73be22..9c8d3b01 100644 --- a/docs/designs/README.md +++ b/docs/designs/README.md @@ -89,3 +89,4 @@ Read the relevant record before you change the behaviour it describes. `CLAUDE.m | 079 | [Declarative Labels for Generated Workload Objects](079-workload-object-labels.md) | | 080 | [Phase Transition Events Across the Lifecycle Tiers](080-phase-transition-events.md) | | 081 | [Support for Cordoned Node Selection](081-cordoned-node-selection.md) | +| 084 | [A Read-Only MCP Server for Certification State](084-mcp-server.md) | From bf76279bf50a4b486300daaff5c44f41c9056b84 Mon Sep 17 00:00:00 2001 From: The Anh Nguyen Date: Thu, 3 Sep 2026 06:26:20 +0700 Subject: [PATCH 5/7] docs(mcp): document INCOMPLETE and drop the per-node results claim 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 --- docs/cli-reference/mcp.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/cli-reference/mcp.md b/docs/cli-reference/mcp.md index 8ce052f9..1024e673 100644 --- a/docs/cli-reference/mcp.md +++ b/docs/cli-reference/mcp.md @@ -21,12 +21,16 @@ The server is strictly read-only: no tool creates, mutates, or deletes a resourc | Tool | Description | |------|-------------| | `list_categories` | List the certification catalog: every registered `domain/variant` category a Certification can run | -| `get_certification_status` | Overall result (`PASSED`/`FAILED`/`RUNNING`), conditions, per-category state, and failed node names for one Certification | -| `get_certification_report` | The full report that `nvcrectl certification report` prints — categories with metrics, bandwidth, cliques, diagnose results, and per-node results | +| `get_certification_status` | Overall result (`PASSED`/`INCOMPLETE`/`FAILED`/`RUNNING`), conditions, per-category state, any nodes excluded from the run, and the unique names of failed nodes for one Certification | +| `get_certification_report` | The full report that `nvcrectl certification report` prints — categories with metrics, bandwidth, cliques, and diagnose results | | `list_failed_nodes` | Failed nodes for one Certification with per-node failure reason and message | The three certification-scoped tools accept `name` and `namespace` (default `default`). +`INCOMPLETE` means the run passed but left some targeted nodes untested — the Workflow excluded them, and `excludedNodes`/`exclusionReason` say which and why. Treat it as "not certified": nothing was observed about those nodes either way. Both `get_certification_status` and `get_certification_report` report it, because both derive the verdict from the same builder. + +`get_certification_status.failedNodes` is the unique node names, deduplicated across categories. `list_failed_nodes` returns one row per distinct failure reason instead, so a node that failed in several categories appears more than once — use the former for a count. + ### Authentication All cluster access uses the kubeconfig of whoever launches the server, resolved with the standard client-go rules: the `--kubeconfig`/`--context` flags, then the `KUBECONFIG` environment variable, then `~/.kube/config`. The server holds no credentials of its own and adds no privilege of its own — it is exactly as authorized as the identity client-go resolves. From e9096153aab11f7603ee580af4aec22294c23cb9 Mon Sep 17 00:00:00 2001 From: The Anh Nguyen Date: Wed, 9 Sep 2026 09:07:13 +0700 Subject: [PATCH 6/7] fix(mcp): one shape for "nothing here", and correct the credential copy Addresses review on #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 --- docs/cli-reference/mcp.md | 13 ++++- pkg/mcpserver/server.go | 52 +++++++++++++++---- pkg/mcpserver/server_test.go | 24 +++++++++ .../testdata/mcp-tools/basic/expected.txt | 3 ++ .../mcp-tools/excluded-nodes/expected.txt | 3 +- pkg/report/report.go | 4 +- .../execution-failed-precedence/expected.txt | 2 +- .../hardware-failed/expected.txt | 2 +- .../no-failure-data/expected.txt | 2 +- .../threshold-violation/expected.txt | 2 +- 10 files changed, 89 insertions(+), 18 deletions(-) diff --git a/docs/cli-reference/mcp.md b/docs/cli-reference/mcp.md index 1024e673..d8254f74 100644 --- a/docs/cli-reference/mcp.md +++ b/docs/cli-reference/mcp.md @@ -39,7 +39,18 @@ All cluster access uses the kubeconfig of whoever launches the server, resolved Client-go's loading rules end in an in-cluster fallback. Run `nvcrectl mcp serve` inside a pod with no kubeconfig and it authenticates as that pod's ServiceAccount, which may be broader than the operator running the agent. When you deploy the server in-cluster, bind its ServiceAccount to a role that grants no more than the reads below. -The tools need `get`/`list` on `certifications` and `workflows` in the target namespace, and `get` on the `configmaps` holding failed-node results. A caller missing the ConfigMap read still gets a successful response with an empty `failedNodes` list rather than an error, so grant the ConfigMap read explicitly — otherwise an agent can read "no nodes failed" from what is really "not allowed to look". +In the target namespace the tools need: + +| Verb | Resources | +|------|-----------| +| `get` | `certifications`, `workflows`, `jobs` (`nvcre.nvidia.com`), `jobs` (`batch`), `configmaps` | +| `list` | `goodputmeasurements`, `bandwidthmeasurements`, `jobs` (`nvcre.nvidia.com`) | + +`certifications` is always fetched by name, so `get` is enough; it does not need `list`. + + +`report.Build` treats every one of these reads as best-effort: a read it is not permitted to make is skipped, not reported. Bind all of them, or `get_certification_report` returns a successful response with metrics, bandwidth, and diagnose data silently absent, and `get_certification_status` reports an empty `failedNodes` for what is really "not allowed to look". The missing data is not one obvious field, so a partial binding is hard to notice from the output alone. + ### Client configuration diff --git a/pkg/mcpserver/server.go b/pkg/mcpserver/server.go index c7ad2bdf..db1eb87b 100644 --- a/pkg/mcpserver/server.go +++ b/pkg/mcpserver/server.go @@ -62,8 +62,10 @@ func New(store *Store, version string) *mcp.Server { Title: "NVIDIA Cluster Readiness Engine", Version: version, }, &mcp.ServerOptions{ - Instructions: "Read-only access to NVCRE certification state. " + - "Authentication uses the kubeconfig of whoever launched the server.", + Instructions: "Read-only access to NVCRE certification state. The server " + + "holds no credentials of its own and adds no privilege: it acts as " + + "whatever identity client-go resolves, which is the launching user's " + + "kubeconfig, or the pod ServiceAccount when run in-cluster without one.", }) // Generic mcp.AddTool derives the input/output JSON schemas from the @@ -182,7 +184,7 @@ func getCertStatusHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { Status: c.Status, }) } - return textResult(out) + return textResult(out.normalize()) } } @@ -266,22 +268,26 @@ type categorySummary struct { // getCertStatusOutput summarizes a Certification's overall and per-category // state without pulling measurement data. Every field except Conditions is // projected from report.Build, so it agrees with get_certification_report. +// Every collection here is emitted even when empty. An absent key reads as +// "unknown" to an agent, not "zero" — and this struct's failedNodes is the +// field the tool description points at for a node count, so a vanishing key +// is the one misreading this feature exists to prevent. type getCertStatusOutput struct { - Name string `json:"name"` - Namespace string `json:"namespace"` - Result string `json:"result"` // "PASSED", "INCOMPLETE", "FAILED", or "RUNNING" + Name string `json:"name"` + Namespace string `json:"namespace"` + Result string `json:"result"` // "PASSED", "INCOMPLETE", "FAILED", or "RUNNING" + TotalNodes int `json:"totalNodes,omitempty"` // ExcludedNodes lists nodes that matched the target but were left // untested; a run reports INCOMPLETE rather than PASSED when it has any. // Surfaced here so the cheaper tool cannot hide them from an agent. - TotalNodes int `json:"totalNodes,omitempty"` - ExcludedNodes []string `json:"excludedNodes,omitempty"` + ExcludedNodes []string `json:"excludedNodes"` ExclusionReason string `json:"exclusionReason,omitempty"` - Conditions []conditionInfo `json:"conditions,omitempty"` - Categories []categoryState `json:"categories,omitempty"` + Conditions []conditionInfo `json:"conditions"` + Categories []categoryState `json:"categories"` // FailedNodes is the unique node names that failed, deduplicated across // categories. Use this for a node count; list_failed_nodes returns one // row per distinct failure reason and so can repeat a name. - FailedNodes []string `json:"failedNodes,omitempty"` + FailedNodes []string `json:"failedNodes"` } type conditionInfo struct { @@ -316,6 +322,30 @@ type failedNodeDetail struct { Message string `json:"message,omitempty"` } +// normalize fills every nil collection so the value serializes with [] rather +// than null. Kept on the type rather than in the handler so the guarantee +// survives a second construction site. +func (o *getCertStatusOutput) normalize() *getCertStatusOutput { + o.ExcludedNodes = orEmpty(o.ExcludedNodes) + o.FailedNodes = orEmpty(o.FailedNodes) + if o.Conditions == nil { + o.Conditions = []conditionInfo{} + } + if o.Categories == nil { + o.Categories = []categoryState{} + } + return o +} + +// orEmpty returns s, or an empty slice when s is nil, so the field serializes +// as [] rather than null. +func orEmpty(s []string) []string { + if s == nil { + return []string{} + } + return s +} + // certification fetches the named Certification, mirroring the nvcrectl // report command's error messages. func (s *Store) certification(ctx context.Context, ref certRef) (*nvcrev1alpha1.Certification, error) { diff --git a/pkg/mcpserver/server_test.go b/pkg/mcpserver/server_test.go index 88286bcf..682724e2 100644 --- a/pkg/mcpserver/server_test.go +++ b/pkg/mcpserver/server_test.go @@ -162,6 +162,30 @@ func TestStatusAgreesWithReport(t *testing.T) { } } +// TestEmptyCollectionsSerializeAsArrays asserts the serialized bytes, not the +// decoded value. A missing key, an explicit null, and [] all decode to a nil +// slice, so TestStatusAgreesWithReport cannot see the difference — but an +// agent can, and only [] unambiguously means zero. The tool description points +// callers at failedNodes for a node count, so a key that vanishes on a passing +// certification is the misreading this feature exists to prevent. +func TestEmptyCollectionsSerializeAsArrays(t *testing.T) { + out := (&getCertStatusOutput{Name: "c", Namespace: "default", Result: "PASSED"}).normalize() + raw, err := json.Marshal(out) + if err != nil { + t.Fatalf("marshal: %v", err) + } + got := string(raw) + + for _, field := range []string{"failedNodes", "excludedNodes", "conditions", "categories"} { + if !strings.Contains(got, `"`+field+`":[]`) { + t.Errorf("%s must serialize as [] when empty, got: %s", field, got) + } + } + if strings.Contains(got, "null") { + t.Errorf("no field may serialize as null: %s", got) + } +} + // certView is the subset of both tools' output that must agree. type certView struct { Result string `json:"result"` diff --git a/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt b/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt index 7ea8fb3d..30db4d66 100644 --- a/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt +++ b/pkg/mcpserver/testdata/mcp-tools/basic/expected.txt @@ -57,6 +57,7 @@ "type": "Failed" } ], + "excludedNodes": [], "failedNodes": [ "node001", "node002" @@ -83,6 +84,8 @@ "type": "Succeeded" } ], + "excludedNodes": [], + "failedNodes": [], "name": "cert-pass", "namespace": "default", "result": "PASSED", diff --git a/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt index ea164a24..508f27a1 100644 --- a/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt +++ b/pkg/mcpserver/testdata/mcp-tools/excluded-nodes/expected.txt @@ -20,6 +20,7 @@ "node004" ], "exclusionReason": "cordoned", + "failedNodes": [], "name": "cert-excluded", "namespace": "default", "result": "INCOMPLETE", @@ -44,7 +45,7 @@ "node004" ], "exclusionReason": "cordoned", - "failedNodes": null, + "failedNodes": [], "gpu": "h100", "name": "cert-excluded", "platform": "onprem", diff --git a/pkg/report/report.go b/pkg/report/report.go index f451775c..0cfbd337 100644 --- a/pkg/report/report.go +++ b/pkg/report/report.go @@ -216,7 +216,9 @@ func FailedNodesFromRef( // categories, resolved from each category's nodeResultsRef ConfigMap. func CertFailedNodes(ctx context.Context, c client.Client, cert *nvcrev1alpha1.Certification) []string { seen := make(map[string]struct{}) - var union []string + // Non-nil so an empty result serializes as [] rather than null: null reads + // as "unknown" to a consumer, where the truth is "no nodes failed". + union := []string{} for _, cat := range cert.Status.CategoryStatuses { for _, n := range FailedNodesFromRef(ctx, c, cert.Namespace, cat.FailedNodesRef) { if n.Name == "" { diff --git a/pkg/report/testdata/build-failed-groups/execution-failed-precedence/expected.txt b/pkg/report/testdata/build-failed-groups/execution-failed-precedence/expected.txt index e57e6e8d..31239fed 100644 --- a/pkg/report/testdata/build-failed-groups/execution-failed-precedence/expected.txt +++ b/pkg/report/testdata/build-failed-groups/execution-failed-precedence/expected.txt @@ -76,6 +76,6 @@ ] } ], - "failedNodes": null, + "failedNodes": [], "result": "FAILED" } diff --git a/pkg/report/testdata/build-failed-groups/hardware-failed/expected.txt b/pkg/report/testdata/build-failed-groups/hardware-failed/expected.txt index 60325c11..91833e8c 100644 --- a/pkg/report/testdata/build-failed-groups/hardware-failed/expected.txt +++ b/pkg/report/testdata/build-failed-groups/hardware-failed/expected.txt @@ -62,6 +62,6 @@ ] } ], - "failedNodes": null, + "failedNodes": [], "result": "FAILED" } diff --git a/pkg/report/testdata/build-failed-groups/no-failure-data/expected.txt b/pkg/report/testdata/build-failed-groups/no-failure-data/expected.txt index 57d212e4..37a5f6a6 100644 --- a/pkg/report/testdata/build-failed-groups/no-failure-data/expected.txt +++ b/pkg/report/testdata/build-failed-groups/no-failure-data/expected.txt @@ -61,6 +61,6 @@ ] } ], - "failedNodes": null, + "failedNodes": [], "result": "FAILED" } diff --git a/pkg/report/testdata/build-failed-groups/threshold-violation/expected.txt b/pkg/report/testdata/build-failed-groups/threshold-violation/expected.txt index 0019d0ee..17b01d43 100644 --- a/pkg/report/testdata/build-failed-groups/threshold-violation/expected.txt +++ b/pkg/report/testdata/build-failed-groups/threshold-violation/expected.txt @@ -62,6 +62,6 @@ ] } ], - "failedNodes": null, + "failedNodes": [], "result": "FAILED" } From 277dda36b154cf60fc54200704e29b7c47e5b8ea Mon Sep 17 00:00:00 2001 From: The Anh Nguyen Date: Thu, 24 Sep 2026 10:41:15 +0700 Subject: [PATCH 7/7] fix(mcp): address #288 review - 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 --- docs/cli-reference/mcp.md | 4 ++- docs/designs/084-mcp-server.md | 4 +-- pkg/mcpserver/server.go | 46 ++++++++++++---------------------- pkg/mcpserver/server_test.go | 22 ++++++++++++++++ pkg/report/report.go | 43 ++++++++++++++++++++++++------- 5 files changed, 77 insertions(+), 42 deletions(-) diff --git a/docs/cli-reference/mcp.md b/docs/cli-reference/mcp.md index d8254f74..57dc7c73 100644 --- a/docs/cli-reference/mcp.md +++ b/docs/cli-reference/mcp.md @@ -29,7 +29,9 @@ The three certification-scoped tools accept `name` and `namespace` (default `def `INCOMPLETE` means the run passed but left some targeted nodes untested — the Workflow excluded them, and `excludedNodes`/`exclusionReason` say which and why. Treat it as "not certified": nothing was observed about those nodes either way. Both `get_certification_status` and `get_certification_report` report it, because both derive the verdict from the same builder. -`get_certification_status.failedNodes` is the unique node names, deduplicated across categories. `list_failed_nodes` returns one row per distinct failure reason instead, so a node that failed in several categories appears more than once — use the former for a count. +`result` is the authoritative outcome. `get_certification_status.conditions` are the Certification's raw conditions, which the `INCOMPLETE` downgrade does not touch: an `INCOMPLETE` run still carries `Succeeded=True` (`AllCategoriesSucceeded`). Never infer a pass from `conditions`. + +`get_certification_status.failedNodes` is the unique node names, deduplicated across categories. `list_failed_nodes` returns one row per distinct failure reason instead, so a node that failed in several categories appears more than once — use the former for a count. Both come from the same walk over the categories' node-results ConfigMaps, so the set of node names always matches. ### Authentication diff --git a/docs/designs/084-mcp-server.md b/docs/designs/084-mcp-server.md index 0888c0ef..5a4f86ca 100644 --- a/docs/designs/084-mcp-server.md +++ b/docs/designs/084-mcp-server.md @@ -18,9 +18,9 @@ Two properties make this more than a transport question. 2. **No tool creates, mutates, or deletes anything, and none triggers a run.** Every tool carries the MCP `readOnlyHint` annotation, and `TestListTools` pins the surface so adding a fifth or mutating tool fails the build. Whether agents may ever trigger runs is deferred; it needs its own decision about consumption and preemption, not a flag on this one. -3. **Every certification verdict is projected from `report.Build`, never re-derived from the CR.** `get_certification_status` calls the same builder `nvcrectl certification report` uses and reads its fields, rather than walking conditions itself. This is the load-bearing decision: it makes "the tools agree with the CLI" a structural property instead of a convention two code paths have to keep. `TestStatusAgreesWithReport` asserts the two tools describe the same certification identically, across every fixture. +3. **Every certification verdict is projected from `report.Build`, never re-derived from the CR.** `get_certification_status` calls the same builder `nvcrectl certification report` uses and reads its fields, rather than walking conditions itself. This is the load-bearing decision: it makes "the tools agree with the CLI" a structural property instead of a convention two code paths have to keep. `TestStatusAgreesWithReport` asserts the two tools describe the same certification identically, across every fixture. The one field not projected from the report is `conditions`, which carries the raw Certification conditions: the `INCOMPLETE` downgrade does not touch them, so an `INCOMPLETE` run still shows `Succeeded=True`. `result` is therefore declared authoritative in the tool description, the field docs, and the CLI reference, and the agreement test fails if an `INCOMPLETE` fixture meets a description that does not say so. -4. **`list_failed_nodes` returns one row per distinct (node, reason, message); `get_certification_status.failedNodes` returns unique node names.** These are deliberately different: a node can fail two categories for two reasons, and both the detail and the count are useful. Because that difference is a real trap for a caller doing `len()`, each tool's description states which one it is and points at the other. +4. **`list_failed_nodes` returns one row per distinct (node, reason, message); `get_certification_status.failedNodes` returns unique node names.** These are deliberately different: a node can fail two categories for two reasons, and both the detail and the count are useful. Because that difference is a real trap for a caller doing `len()`, each tool's description states which one it is and points at the other. Both come from one walk, `report.CertFailedNodeDetails`, which `report.CertFailedNodes` also uses, and the agreement test asserts that the set of names `list_failed_nodes` returns equals the report's `failedNodes`, so a later filter or dedupe change cannot make them drift. 5. **Credentials are whatever client-go resolves, and the docs say so exactly.** The server holds no credentials and adds no privilege. It is not, however, true that it "never reads service account tokens": client-go's standard loading rules end in an in-cluster fallback, so running it inside a pod without a kubeconfig authenticates as that pod's ServiceAccount. The documentation states this and tells operators to bind a least-privilege role when deploying in-cluster, because a security guarantee that is only true outside a pod is worse than none. diff --git a/pkg/mcpserver/server.go b/pkg/mcpserver/server.go index db1eb87b..820d7063 100644 --- a/pkg/mcpserver/server.go +++ b/pkg/mcpserver/server.go @@ -19,7 +19,6 @@ package mcpserver import ( "context" "fmt" - "sort" "github.com/modelcontextprotocol/go-sdk/mcp" "k8s.io/apimachinery/pkg/api/errors" @@ -110,7 +109,7 @@ func listCategoriesTool() *mcp.Tool { func getCertStatusTool() *mcp.Tool { return &mcp.Tool{ Name: "get_certification_status", - Description: "Get the status of one Certification: overall result (PASSED/INCOMPLETE/FAILED/RUNNING), conditions, per-category state, any nodes excluded from the run, and the unique names of failed nodes. INCOMPLETE means the run passed but left some targeted nodes untested.", + Description: "Get the status of one Certification: overall result (PASSED/INCOMPLETE/FAILED/RUNNING), conditions, per-category state, any nodes excluded from the run, and the unique names of failed nodes. INCOMPLETE means the run passed but left some targeted nodes untested. result is authoritative: conditions are the raw Certification conditions and can still read Succeeded=True on an INCOMPLETE run, so never infer the outcome from conditions.", } } @@ -209,33 +208,16 @@ func listFailedNodesHandler(store *Store) mcp.ToolHandlerFor[certRef, any] { return nil, nil, err } - seen := map[string]bool{} + // Same walk report.CertFailedNodes uses, so this tool and the report + // cannot disagree on which nodes failed. details := []failedNodeDetail{} - for _, cat := range cert.Status.CategoryStatuses { - for _, n := range report.FailedNodesFromRef(ctx, store.Client, cert.Namespace, cat.FailedNodesRef) { - key := n.Name + "|" + string(n.Reason) + "|" + n.Message - if n.Name == "" || seen[key] { - continue - } - seen[key] = true - details = append(details, failedNodeDetail{ - Name: n.Name, - Reason: string(n.Reason), - Message: n.Message, - }) - } + for _, n := range report.CertFailedNodeDetails(ctx, store.Client, cert) { + details = append(details, failedNodeDetail{ + Name: n.Name, + Reason: string(n.Reason), + Message: n.Message, + }) } - // Deterministic order for the goldens. - sort.Slice(details, func(i, j int) bool { - a, b := details[i], details[j] - if a.Name != b.Name { - return a.Name < b.Name - } - if a.Reason != b.Reason { - return a.Reason < b.Reason - } - return a.Message < b.Message - }) return textResult(&listFailedNodesOutput{ Name: cert.Name, Namespace: cert.Namespace, @@ -273,9 +255,13 @@ type categorySummary struct { // field the tool description points at for a node count, so a vanishing key // is the one misreading this feature exists to prevent. type getCertStatusOutput struct { - Name string `json:"name"` - Namespace string `json:"namespace"` - Result string `json:"result"` // "PASSED", "INCOMPLETE", "FAILED", or "RUNNING" + Name string `json:"name"` + Namespace string `json:"namespace"` + // Result is the authoritative outcome: "PASSED", "INCOMPLETE", "FAILED", + // or "RUNNING". It is projected from report.Build, which downgrades + // PASSED to INCOMPLETE when nodes were excluded; Conditions are the raw + // CR conditions and do not, so they can disagree on that path. + Result string `json:"result"` TotalNodes int `json:"totalNodes,omitempty"` // ExcludedNodes lists nodes that matched the target but were left // untested; a run reports INCOMPLETE rather than PASSED when it has any. diff --git a/pkg/mcpserver/server_test.go b/pkg/mcpserver/server_test.go index 682724e2..403f633d 100644 --- a/pkg/mcpserver/server_test.go +++ b/pkg/mcpserver/server_test.go @@ -157,6 +157,28 @@ func TestStatusAgreesWithReport(t *testing.T) { t.Errorf("%s: failedNodes mismatch: status=%v report=%v", cert.Name, status.FailedNodes, rep.Report.FailedNodes) } + + // list_failed_nodes repeats a name once per distinct reason, + // but its set of names must be exactly the report's. + var detail listFailedNodesOutput + unmarshalTool(t, callTool(t, session, "list_failed_nodes", args), &detail) + names := []string{} + for _, n := range detail.FailedNodes { + if len(names) == 0 || names[len(names)-1] != n.Name { + names = append(names, n.Name) + } + } + if !reflect.DeepEqual(names, orEmpty(rep.Report.FailedNodes)) { + t.Errorf("%s: list_failed_nodes names %v disagree with report failedNodes %v", + cert.Name, names, rep.Report.FailedNodes) + } + + // result is authoritative over the raw conditions: on the + // INCOMPLETE path the CR still says Succeeded=True, and the + // tool description must tell an agent not to trust that. + if status.Result == "INCOMPLETE" && !strings.Contains(getCertStatusTool().Description, "result is authoritative") { + t.Errorf("%s: INCOMPLETE result but the status tool does not declare result authoritative", cert.Name) + } } }) } diff --git a/pkg/report/report.go b/pkg/report/report.go index 0cfbd337..0c5bc438 100644 --- a/pkg/report/report.go +++ b/pkg/report/report.go @@ -212,26 +212,51 @@ func FailedNodesFromRef( return nodes } -// CertFailedNodes returns the deduped union of failed node names across all -// categories, resolved from each category's nodeResultsRef ConfigMap. -func CertFailedNodes(ctx context.Context, c client.Client, cert *nvcrev1alpha1.Certification) []string { +// CertFailedNodeDetails returns every distinct (node, reason, message) failure +// across all categories, resolved from each category's nodeResultsRef +// ConfigMap and sorted by node, reason, then message. It is the single walk +// behind both CertFailedNodes and the MCP list_failed_nodes tool, so the two +// cannot disagree on which nodes failed. +func CertFailedNodeDetails(ctx context.Context, c client.Client, cert *nvcrev1alpha1.Certification) []nvcrev1alpha1.FailedNode { seen := make(map[string]struct{}) - // Non-nil so an empty result serializes as [] rather than null: null reads - // as "unknown" to a consumer, where the truth is "no nodes failed". - union := []string{} + details := []nvcrev1alpha1.FailedNode{} for _, cat := range cert.Status.CategoryStatuses { for _, n := range FailedNodesFromRef(ctx, c, cert.Namespace, cat.FailedNodesRef) { + key := n.Name + "|" + string(n.Reason) + "|" + n.Message if n.Name == "" { continue } - if _, ok := seen[n.Name]; ok { + if _, ok := seen[key]; ok { continue } - seen[n.Name] = struct{}{} + seen[key] = struct{}{} + details = append(details, n) + } + } + sort.Slice(details, func(i, j int) bool { + a, b := details[i], details[j] + if a.Name != b.Name { + return a.Name < b.Name + } + if a.Reason != b.Reason { + return a.Reason < b.Reason + } + return a.Message < b.Message + }) + return details +} + +// CertFailedNodes returns the deduped union of failed node names across all +// categories: the unique names from CertFailedNodeDetails. +func CertFailedNodes(ctx context.Context, c client.Client, cert *nvcrev1alpha1.Certification) []string { + // Non-nil so an empty result serializes as [] rather than null: null reads + // as "unknown" to a consumer, where the truth is "no nodes failed". + union := []string{} + for _, n := range CertFailedNodeDetails(ctx, c, cert) { + if len(union) == 0 || union[len(union)-1] != n.Name { union = append(union, n.Name) } } - sort.Strings(union) return union }