Skip to content

Fix missing continuous GPU metrics being reported healthy - #267

Open
Kevin Cho (chokevin) wants to merge 7 commits into
mainfrom
chokevin-taugrid-live-demo
Open

Kevin Cho (chokevin) wants to merge 7 commits into
mainfrom
chokevin-taugrid-live-demo

Conversation

@chokevin

@chokevin Kevin Cho (chokevin) commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What

Add an explicit continuous metric-coverage contract to the GPU metrics collector and wire it through the gpu-monitoring chart. Required missing, incomplete, non-finite, stale, or warming-up inputs produce Kubernetes Unknown, not False/...Ok.

  • Count distinct physical GPU identities using minSamples and sampleLabel.
  • Support explicit metricNames sets: every listed family must cover the same required identities. Evaluate rates per series so one counter reset cannot cancel another link's increase.
  • Preserve Unknown through status persistence without bridging collector downtime or missing observations with an old rate baseline.
  • Keep sparse XID selectors optional and give measured faults precedence over incomplete coverage.
  • Add an optional externally owned exporter CSV with existing fleet signals, continuous health inputs and all 54 per-link NVLink counters.
  • Correct H100 NVL topology checks: accept indented expected-inactive links; report an unexposed single-GPU topology as Unknown while retaining failures for query errors and empty multi-GPU topology.
  • Clear obsolete diagnostic text on recovery and preserve server transition timestamps during heartbeat merge patches.
  • Add an opt-in, reversible AKS-managed exporter counter configurator. It retains all existing CSV fields, preserves package/AKS-owned files and port 19400, and restarts only the existing exporter. Foreign/changed configuration is rejected; a failed apply restores the prior configuration.

Why

A reachable exporter can omit required fields, and the original evaluator treated missing input as if it were an explicit zero. NVLink also has a distinct API issue: on newer drivers, legacy aggregate fields can return unsupported while DCGM's per-link fields use the supported direct NVML path. Enabling aggregate names in CSV alone cannot fix that.

Live recovery also exposed merge-patch serialization problems: omitted empty messages retained old warmup text, while null transition timestamps erased the previous transition. Regression cases reproduce both against actual fake-client Node objects.

The managed A100 exporter was already correctly listening on 19400, but AKS's service explicitly loaded the package-default limited counter CSV. GPU Operator configuration does not control this separate host service. The H100 pools were provisioned driver-only, without managed-GPU enablement or host DCGM packages/services; their monitoring profiles nevertheless required host dcgmi. Container-side DCGM does not imply a host binary is installed.

Rollout

metricsCollector.requireMetricCoverage remains off by default because the pinned public image predates this capability. Publish through the approved post-merge pipeline, pin an immutable compatible digest, then enable coverage. The --require-metric-coverage flag makes old images fail rather than silently ignore the contract; coverage-enabled binaries also reject a stale config without coverage rules.

Per-profile nvlinkMetricLinkIds requires coverage and a collector supporting metricNames, not merely the initial coverage feature. The list must enumerate the verified topology. An older coverage reader rejects the absent single metricName rather than accepting an unmonitored selector. Other profiles remain unchanged.

The optional exporter ConfigMap does not mutate GPU Operator or another Helm release. Its owner must separately reference the file and establish finite per-device field coverage. A CSV declaration alone does not prove support. Reconcile the tested image/chart/configuration with GitOps ownership before resuming sync.

Explicitly approved H200 preview

A digest-pinned preview built from 0c58b5a2 is active on two eight-GPU H200 nodes. This is not public-image publication or default production activation. The existing GPU Operator Helm release was updated at the same chart version, changing only its exporter configuration reference.

All 54 per-link fields have readings on the exact 16 physical GPU identities: 864 metric/device combinations. Each NVLink rule requires all 18 links on the same eight identities per node. All ten configured continuous conditions report measured False/OK on both nodes, with obsolete recovery text cleared and transition timestamps retained. Missing-link and warmup cases remain Unknown; fault thresholds, rate windows and debounce periods were not relaxed. Nonzero accumulated data-CRC counters were not reset or replaced with zeros.

Explicitly approved A100 preview

The same final collector preview is now active on both eight-GPU A100 nodes. The existing AKS host exporter stays on 19400. The repair retained its package CSV, binary, service unit, AKS override and host-engine processes, adding only a TauGrid-owned content-addressed CSV and 90-taugrid-metrics.conf drop-in. All existing field definitions are preserved; 63 missing declarations are appended.

All 43 required families have finite readings on the exact 16 physical GPUs: 688 field/device combinations. The NVLink rules require all 12 physically verified links per GPU, not H200's 18-link topology. At 2026-09-11T02:53:28Z, all ten continuous conditions on both nodes reported measured False/OK after rate warmup. Final postflight preserved driver versions, memory-error/retirement/remapping inventory, the occupied A100 research workload, non-target monitoring templates and GPU Operator configuration.

The host configurator is an explicit node-owner operation, not an AKS API setting or automatic Helm action. Its override survives normal reboot, not node replacement/reimage. Approved provisioning/reapplication and GitOps ownership remain separate production work. The helper provides an ownership-checked rollback; changed host-engine/vendor state requires fresh review rather than stale assumptions.

H100 provisioning is diagnosed and topology fixes are committed, but exporter-mode/profile activation is still pending. No H100 package installation or node-pool migration was performed. Argo remains held; shared ADX settings were not changed. No all-fleet hardware-health or burn-in claim is made.

Non-goals

  • Change fault thresholds, GPU reset behavior or drain policies.
  • Automatically modify external exporters, migrate H100 node pools, or change shared ingestion settings.
  • Publish production images from a contributor branch, enable defaults against an incompatible image, or resume Argo over unreconciled desired state.

Testing

  • Go 1.26.7 collector race tests, including HTTP-to-Node integration, complete versus partial metric sets, identity intersections, rate gaps/resets/restarts, and actual Node recovery-message/transition-time behavior.
  • Existing collector golangci-lint: zero issues.
  • Helm lint and all 129 gpu-monitoring Helm unit tests passed for the coverage/topology changes.
  • Final existing chart script harness: all 83 tests passed, including eight managed-exporter ownership, idempotence, invalid-input, symlink, partial-write and rollback regressions.
  • License-header gate passed for 789 source files.
  • Bounded live H200 and A100 deployment/coverage receipts retained outside the repository. CI for the newest commit is tracked separately; prior green checks are not represented as new-head results.

Risk

Incomplete and warming-up signals intentionally change from apparent OK to Unknown. Consumers must preserve that distinction. Select links per verified profile; do not copy H200's topology to single-GPU or partially connected VMs. Reverting to a legacy image/config also reverts these reporting guarantees and is not proof of healthy hardware. Host customization is opt-in and must be owned through future node lifecycle operations.

AI assistance

Implementation and validation were performed with GitHub Copilot.

Comment thread monitoring/gpu-metrics-collector/internal/rules/rules.go Outdated
Comment thread monitoring/gpu-metrics-collector/internal/rules/rules.go Outdated
Comment thread monitoring/gpu-metrics-collector/internal/rules/rules.go
Comment thread monitoring/gpu-metrics-collector/internal/rules/rules.go Outdated
Comment thread monitoring/gpu-metrics-collector/internal/rules/rules.go
Comment thread monitoring/gpu-metrics-collector/internal/rules/rules.go Outdated

@feiskyer Pengfei Ni (feiskyer) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants