test(e2e): e2e k8s environment tests using chainsaw#365
Conversation
Greptile SummaryThis PR adds a full Chainsaw-based E2E test suite covering six scenarios across the Kubernetes and Slinky engines, plus a CI workflow, Makefile targets, and documentation updates. Previously raised concerns about image tag sanitization,
Confidence Score: 5/5Safe to merge; all changes are additive test infrastructure with no modifications to production code paths. All six Chainsaw suites are additive; the workflow, Makefile targets, and documentation introduce no changes to the production binary. The previously raised concerns about image-tag formatting, variable propagation to chainsaw, and binary checksum verification have each been resolved in this revision. The only observations are non-critical quality nits in the CI workflow itself. No files require special attention; the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant GH as GitHub Actions / make e2e-local
participant Kind as kind cluster
participant Helm as helm upgrade --install
participant NO as Node Observer
participant TG as Topograph pod
participant K8s as Kubernetes API
GH->>Kind: kind create cluster (kind-config.yaml)
GH->>GH: make image-build (short-SHA tag)
GH->>Kind: kind load docker-image
GH->>K8s: kubectl apply fake Nodes + ConfigMap (topology-model)
GH->>K8s: kubectl apply fake slurmd Pods (Slinky suites)
GH->>K8s: kubectl patch pod status to Ready
GH->>Helm: helm upgrade --install topograph (values.yaml + --set image/ns)
Helm-->>TG: pod starts (imagePullPolicy: Never)
Helm-->>NO: node-observer starts
NO->>K8s: "watch Nodes (kubernetes.io/os=linux)"
K8s-->>NO: Added events for fake/real nodes
NO->>TG: POST /v1/generate (modelFileName)
TG->>TG: "run provider -> build graph -> run engine"
TG->>K8s: patch Node labels (k8s engine) OR create slurm-topology ConfigMap (Slinky) OR annotate Nodes (dynamic-nodes)
GH->>K8s: chainsaw assert Node labels / ConfigMap content / Node annotations
GH->>Helm: helm uninstall (finally block)
GH->>Kind: kind delete cluster
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant GH as GitHub Actions / make e2e-local
participant Kind as kind cluster
participant Helm as helm upgrade --install
participant NO as Node Observer
participant TG as Topograph pod
participant K8s as Kubernetes API
GH->>Kind: kind create cluster (kind-config.yaml)
GH->>GH: make image-build (short-SHA tag)
GH->>Kind: kind load docker-image
GH->>K8s: kubectl apply fake Nodes + ConfigMap (topology-model)
GH->>K8s: kubectl apply fake slurmd Pods (Slinky suites)
GH->>K8s: kubectl patch pod status to Ready
GH->>Helm: helm upgrade --install topograph (values.yaml + --set image/ns)
Helm-->>TG: pod starts (imagePullPolicy: Never)
Helm-->>NO: node-observer starts
NO->>K8s: "watch Nodes (kubernetes.io/os=linux)"
K8s-->>NO: Added events for fake/real nodes
NO->>TG: POST /v1/generate (modelFileName)
TG->>TG: "run provider -> build graph -> run engine"
TG->>K8s: patch Node labels (k8s engine) OR create slurm-topology ConfigMap (Slinky) OR annotate Nodes (dynamic-nodes)
GH->>K8s: chainsaw assert Node labels / ConfigMap content / Node annotations
GH->>Helm: helm uninstall (finally block)
GH->>Kind: kind delete cluster
Reviews (10): Last reviewed commit: "Incorporated review comments" | Re-trigger Greptile |
|
/ok-to-test 9fee2e8 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
==========================================
+ Coverage 68.46% 72.36% +3.90%
==========================================
Files 82 86 +4
Lines 4842 5312 +470
==========================================
+ Hits 3315 3844 +529
+ Misses 1395 1278 -117
- Partials 132 190 +58 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
31d1489 to
91eea39
Compare
|
/ok-to-test 91eea39 |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-365.docs.buildwithfern.com/topograph |
|
/ok-to-test a10bc7d |
b7ac4d8 to
5a07060
Compare
|
/ok-to-test 5a07060 |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Found 5 issues.
- Every suite's values.yaml disables the node-data-broker DaemonSet but leaves
node-observer.topograph.nodeDataBroker.enabledat its defaulttrue, so the observer'sallBrokerPodsReady()waits forever for broker pods that never exist and/v1/generateis never triggered - all six suites should time out on their assert steps. Per the node-data-broker subchart's values comment, also setnode-observer.topograph.nodeDataBroker.enabled: false(same fix in the other five values.yaml files). (tests/chainsaw/k8s/label-application/values.yaml:19) - Chainsaw only auto-discovers a file named
.chainsaw.yaml, and neither thee2e/e2e-localMakefile targets nor the workflow pass--config, so this file - includingparallel: 1- is silently ignored. With default parallelism the six suites race: all use the fixed release nametopograph, so the release-scoped ClusterRole/ClusterRoleBinding names collide and each suite's pre-installkubectl delete clusterroletears down RBAC another suite owns. Renaming to.chainsaw.yamlor passing--configfixes it. (tests/chainsaw/chainsaw-config.yaml:14) kind get kubeconfigprints kubeconfig content to stdout, not a path, so this setsKUBECONFIGto inline YAML and every kubectl/helm/chainsaw call ine2e-localfails to find a config. Write it to a temp file (or usekind export kubeconfig) and pointKUBECONFIGat that. (Makefile:152)- go.mod requires Go 1.25.11, and the four pins in go.yml were deliberately bumped from 1.25.9 to 1.25.11 (cb0362e) to clear reachable stdlib vulnerabilities - this reintroduces the pre-fix toolchain. Pin
1.25.11here too. (.github/workflows/e2e.yml:32) - This row claims
label-applicationverifiesacceleratorlabels, but the suite's topology model has no NVLink data and its assert step only checksleafandspine(tests/chainsaw/README.md lists it correctly). The table also omits theslinky/block-complementandslinky/dynamic-nodessuites shipped in this PR. (docs/engines/k8s.md:357)
| kubernetes.io/os: linux | ||
|
|
||
| node-data-broker: | ||
| enabled: false |
There was a problem hiding this comment.
Every suite's values.yaml disables the node-data-broker DaemonSet but leaves node-observer.topograph.nodeDataBroker.enabled at its default true, so the observer's allBrokerPodsReady() waits forever for broker pods that never exist and /v1/generate is never triggered - all six suites should time out on their assert steps. Per the node-data-broker subchart's values comment, also set node-observer.topograph.nodeDataBroker.enabled: false (same fix in the other five values.yaml files).
There was a problem hiding this comment.
With the new chart changes node-observer.topograph.nodeDataBroker.enabled is not required.
| error: 30s | ||
| exec: 120s | ||
| fullName: true | ||
| parallel: 1 |
There was a problem hiding this comment.
Chainsaw only auto-discovers a file named .chainsaw.yaml, and neither the e2e/e2e-local Makefile targets nor the workflow pass --config, so this file - including parallel: 1 - is silently ignored. With default parallelism the six suites race: all use the fixed release name topograph, so the release-scoped ClusterRole/ClusterRoleBinding names collide and each suite's pre-install kubectl delete clusterrole tears down RBAC another suite owns. Renaming to .chainsaw.yaml or passing --config fixes it.
There was a problem hiding this comment.
Renamed it to .chainsaw.yaml now
| --config tests/chainsaw/kind-config.yaml --wait 120s \ | ||
| || kind get clusters | grep -q "^$(KIND_CLUSTER)$$" | ||
| kind load docker-image $(IMAGE_REPO):$(E2E_IMAGE_TAG) --name $(KIND_CLUSTER) | ||
| KUBECONFIG="$$(kind get kubeconfig --name $(KIND_CLUSTER))" \ |
There was a problem hiding this comment.
kind get kubeconfig prints kubeconfig content to stdout, not a path, so this sets KUBECONFIG to inline YAML and every kubectl/helm/chainsaw call in e2e-local fails to find a config. Write it to a temp file (or use kind export kubeconfig) and point KUBECONFIG at that.
| - name: Set up Go | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: '1.25.9' |
There was a problem hiding this comment.
go.mod requires Go 1.25.11, and the four pins in go.yml were deliberately bumped from 1.25.9 to 1.25.11 (cb0362e) to clear reachable stdlib vulnerabilities - this reintroduces the pre-fix toolchain. Pin 1.25.11 here too.
|
|
||
| | Suite | What it checks | | ||
| |---|---| | ||
| | `k8s/label-application` | `leaf`, `spine`, and `accelerator` labels applied to nodes after generation | |
There was a problem hiding this comment.
This row claims label-application verifies accelerator labels, but the suite's topology model has no NVLink data and its assert step only checks leaf and spine (tests/chainsaw/README.md lists it correctly). The table also omits the slinky/block-complement and slinky/dynamic-nodes suites shipped in this PR.
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
|
/ok-to-test 9e38d2c |
Signed-off-by: Ravi Shankar <ravish@nvidia.com>
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Found 6 issues.
- This pin went stale mid-review: #395 has since bumped the repo to Go 1.26.5 (go.mod and all four pins in go.yml). On runners where toolchain auto-fetch is restricted this fails outright; everywhere else it silently downloads 1.26.5 anyway, so best to align it. My earlier 1.25.11 suggestion predates the #395 bump. (.github/workflows/e2e.yml:32)
- This delete (and the same pattern in all six suites, setup and cleanup) removes cluster-scoped objects that share names with a real Topograph deployment's RBAC, against whatever cluster KUBECONFIG currently points at. Since the README documents running
make e2eagainst an arbitrary context, a stale KUBECONFIG would silently strip a live cluster's Topograph RBAC - consider guarding on cluster identity, e.g. only proceed when the current context is the expected kind cluster. (tests/chainsaw/k8s/label-application/chainsaw-test.yaml:80) - This
kind loadruns as its own recipe line, outside the shell block that capturesE2E_STATUSand deletes the cluster, so a load failure aborts the target and orphans the just-created kind cluster. Folding it into the guarded block keeps the teardown promise. (Makefile:151) kind@latestfloats, so a new kind release can change cluster behavior between runs with no way to pin or bisect. Worth the same treatment as the chainsaw install below: a pinned, overridable version. (.github/workflows/e2e.yml:35)- This comment points users at
TOPOGRAPH_IMAGE_REPO/TOPOGRAPH_IMAGE_TAG, but the recipe unconditionally overwrites both from$(IMAGE_REPO)/$(E2E_IMAGE_TAG), and the new README explicitly warns against setting them as env vars. Recommending theIMAGE_REPO/E2E_IMAGE_TAGmake overrides here would keep the two in agreement. (Makefile:136) - The suite list names
node-observer, but no such suite exists undertests/chainsaw/- the actual suites arek8s/{label-application,label-truncation}andslinky/{block-complement,dra-provider,dynamic-nodes,tree-topology}. The synced line in.claude/CLAUDE.mdneeds the same fix. (AGENTS.md:46)
| - name: Set up Go | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: '1.25.11' |
There was a problem hiding this comment.
This pin went stale mid-review: #395 has since bumped the repo to Go 1.26.5 (go.mod and all four pins in go.yml). On runners where toolchain auto-fetch is restricted this fails outright; everywhere else it silently downloads 1.26.5 anyway, so best to align it. My earlier 1.25.11 suggestion predates the #395 bump.
| IMAGE_TAG="${TOPOGRAPH_IMAGE_TAG:-}" | ||
| PULL_POLICY="${TOPOGRAPH_IMAGE_PULL_POLICY:-IfNotPresent}" | ||
|
|
||
| kubectl delete clusterrole topograph topograph-node-observer \ |
There was a problem hiding this comment.
This delete (and the same pattern in all six suites, setup and cleanup) removes cluster-scoped objects that share names with a real Topograph deployment's RBAC, against whatever cluster KUBECONFIG currently points at. Since the README documents running make e2e against an arbitrary context, a stale KUBECONFIG would silently strip a live cluster's Topograph RBAC - consider guarding on cluster identity, e.g. only proceed when the current context is the expected kind cluster.
| kind create cluster --name $(KIND_CLUSTER) \ | ||
| --config tests/chainsaw/kind-config.yaml --wait 120s \ | ||
| || kind get clusters | grep -q "^$(KIND_CLUSTER)$$" | ||
| kind load docker-image $(IMAGE_REPO):$(E2E_IMAGE_TAG) --name $(KIND_CLUSTER) |
There was a problem hiding this comment.
This kind load runs as its own recipe line, outside the shell block that captures E2E_STATUS and deletes the cluster, so a load failure aborts the target and orphans the just-created kind cluster. Folding it into the guarded block keeps the teardown promise.
| go-version: '1.25.11' | ||
|
|
||
| - name: Install kind | ||
| run: go install sigs.k8s.io/kind@latest |
There was a problem hiding this comment.
kind@latest floats, so a new kind release can change cluster behavior between runs with no way to pin or bisect. Worth the same treatment as the chainsaw install below: a pinned, overridable version.
| kind load docker-image $(IMAGE_REPO):$(E2E_IMAGE_TAG) --name $(KIND_CLUSTER) | ||
|
|
||
| # Run all Chainsaw E2E suites against the current KUBECONFIG context. | ||
| # For a pre-pushed registry image: set TOPOGRAPH_IMAGE_REPO and TOPOGRAPH_IMAGE_TAG. |
There was a problem hiding this comment.
This comment points users at TOPOGRAPH_IMAGE_REPO/TOPOGRAPH_IMAGE_TAG, but the recipe unconditionally overwrites both from $(IMAGE_REPO)/$(E2E_IMAGE_TAG), and the new README explicitly warns against setting them as env vars. Recommending the IMAGE_REPO/E2E_IMAGE_TAG make overrides here would keep the two in agreement.
| CHANGELOG.md # Release history (Keep a Changelog format); update [Unreleased] for user-facing PRs | ||
| docs/ # Public-facing docs — overview.md, architecture.md, api.md + providers/, engines/, reference/ subdirectories | ||
| tests/models/ # YAML simulation fixtures | ||
| tests/chainsaw/ # Chainsaw E2E test suites (label-application, label-truncation, node-observer, slinky) |
There was a problem hiding this comment.
The suite list names node-observer, but no such suite exists under tests/chainsaw/ - the actual suites are k8s/{label-application,label-truncation} and slinky/{block-complement,dra-provider,dynamic-nodes,tree-topology}. The synced line in .claude/CLAUDE.md needs the same fix.
2 participants
Description
End-2-end tests for the Kubernetes environment using Chainsaw.
Addresses issue #263
Checklist
git commit -s).