feat: add device-api-server with NVML fallback provider

[ArangoGutierrez]

Summary

Implements the Device API Server — a node-local gRPC cache server for GPU device state, deployed as a Kubernetes DaemonSet. It acts as an intermediary between providers (e.g., NVSentinel health monitors that update GPU state) and consumers (e.g., Device Plugins, DRA Drivers that read state for scheduling decisions).

Architecture

┌──────────────────────────────────────────────────────────────────────┐
│                          Kubernetes Node                             │
│                                                                      │
│  ┌──────────────────┐                  ┌──────────────────────────┐  │
│  │   NVSentinel      │                  │  Device Plugin / DRA     │  │
│  │  Health Monitor   │                  │       Driver             │  │
│  │   [Provider]      │                  │     [Consumer]           │  │
│  └────────┬─────────┘                  └────────────┬─────────────┘  │
│           │ CreateGpu()                              │ GetGpu()       │
│           │ UpdateGpuStatus()                        │ ListGpus()     │
│           │ (gRPC)                                   │ WatchGpus()    │
│           ▼                                          ▼                │
│  ┌────────────────────────────────────────────────────────────────┐  │
│  │                  Device API Server (DaemonSet)                  │  │
│  │                                                                 │  │
│  │  ┌─────────────────────────────────────────────────────────┐   │  │
│  │  │                   GpuService (Unified)                   │   │  │
│  │  │  Write: CreateGpu, UpdateGpu, UpdateGpuStatus, DeleteGpu │   │  │
│  │  │  Read:  GetGpu, ListGpus, WatchGpus                      │   │  │
│  │  └──────────────────────┬──────────────────────────────────┘   │  │
│  │                         ▼                                       │  │
│  │  ┌─────────────────────────────────────────────────────────┐   │  │
│  │  │               Cache (sync.RWMutex)                       │   │  │
│  │  │  • Writer-preference: blocks ALL new reads during write  │   │  │
│  │  │  • proto.Clone on every read/write (clone-safety)        │   │  │
│  │  │  • Optimistic concurrency via resource_version           │   │  │
│  │  │  • Watch broadcaster with ListAndSubscribe atomicity     │   │  │
│  │  └─────────────────────────────────────────────────────────┘   │  │
│  │                                                                 │  │
│  │  ┌───────────┐  ┌───────────┐  ┌─────────────────────────┐    │  │
│  │  │ Health    │  │ Metrics   │  │ Unix Socket             │    │  │
│  │  │ :8081     │  │ :9090     │  │ /var/run/device-api/    │    │  │
│  │  │ /healthz  │  │ /metrics  │  │ device.sock             │    │  │
│  │  └───────────┘  └───────────┘  └─────────────────────────┘    │  │
│  └────────────────────────────────────────────────────────────────┘  │
└──────────────────────────────────────────────────────────────────────┘

Why read-blocking semantics?

The sync.RWMutex with writer-preference ensures consumers never read stale "healthy" state when a provider is updating to "unhealthy". Once a write is pending, new readers block until the write completes. This is critical for correct scheduling decisions.

NVML Provider (Sidecar)

The NVML provider runs as a sidecar container that:

1. Enumerates GPUs via NVML and registers them via CreateGpu
2. Monitors XID events for hardware health (ECC errors, GPU fallen off bus, etc.)
3. Updates GPU conditions via UpdateGpuStatus when health changes
4. Classifies XIDs by severity: ignored (application errors), warning, critical (hardware failures)

Decoupled as a sidecar for independent updates and glibc isolation (NVML requires RTLD_DEEPBIND).

Key Design Decisions

Decision	Rationale
Unified GpuService (not split provider/consumer)	Simpler API surface, standard CRUD, matches K8s patterns
proto.Clone on all cache paths	Prevents external mutation of cached state; verified by clone-safety tests
Optimistic concurrency (resource_version)	Prevents lost updates from concurrent providers without pessimistic locking
MaxConditionsPerGpu = 32	Bounds memory growth from unbounded condition lists
Build-tagged NVML (//go:build nvml)	Pure Go server binary; NVML only compiled when needed for sidecar
gRPC resource limits (4MB msg, 100 streams)	Prevents resource exhaustion from errant local processes
Generic error messages to clients	Internal error details logged server-side, not leaked to gRPC clients

Key Components

• GpuService — Unified gRPC service with standard CRUD + Watch
• GpuCache — In-memory store with sync.RWMutex, clone-safety, and monotonic resource versioning
• Watch Broadcaster — Atomic ListAndSubscribe to prevent duplicate events; slow subscriber isolation
• NVML Provider — GPU enumeration and XID-based health monitoring (build-tagged sidecar)
• Prometheus Metrics — Cache stats, NVML status, gRPC latency, watch event drops
• Helm Chart — DaemonSet with ServiceMonitor, PrometheusRule, sidecar support

Type of Change

• New feature (new message, field, or service method)
• Documentation
• Build/Tooling

Test Results

• 86 unit tests passing with -race flag (cache: 34, service: 21, nvml: 22, metrics: 7, config: 2)
• go vet clean (including -tags nvml)
• All cache mutations use proto.Clone (verified via clone-safety tests)
• Concurrent stress tests: 10 readers + 1 writer, 5 concurrent writers
• Non-numeric resource_version rejected with InvalidArgument
• Health classification: False (unhealthy) overrides Unknown regardless of condition ordering

Checklist

• Proto files compile successfully (make protos-generate)
• Generated code is up to date and committed
• Self-review completed
• Documentation updated
• Signed-off commits (DCO)
• gRPC server has message size and stream limits
• Internal errors do not leak details to clients
• XID severity maps are immutable (unexported)
• Port collision validated at startup
• Helm template handles both :port and host:port formats

[Copilot]

Pull request overview

This pull request implements a comprehensive Device API Server with NVML fallback provider support for GPU device information and health monitoring. The server acts as a node-local gRPC cache intermediary between providers (health monitors) and consumers (device plugins, DRA drivers).

Changes:

• Implements complete Device API Server with gRPC services (GpuService for consumers, ProviderService for providers)
• Adds built-in NVML fallback provider for direct GPU enumeration and XID-based health monitoring
• Implements thread-safe in-memory cache with RWMutex for read-blocking semantics during writes
• Adds watch broadcaster for real-time GPU state change notifications
• Includes comprehensive Prometheus metrics for observability
• Provides Helm charts for Kubernetes deployment with ServiceMonitor and alerting

Reviewed changes

Copilot reviewed 51 out of 54 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/version/version.go	Version information package with build-time ldflags support
pkg/deviceapiserver/config.go	Server configuration with flags, environment variables, and validation
pkg/deviceapiserver/server.go	Main server implementation with lifecycle management and graceful shutdown
pkg/deviceapiserver/cache/*.go	Thread-safe GPU cache with broadcaster for watch events
pkg/deviceapiserver/service/*.go	gRPC service implementations (consumer and provider APIs)
pkg/deviceapiserver/nvml/*.go	NVML provider for GPU enumeration and XID health monitoring
pkg/deviceapiserver/metrics/metrics.go	Prometheus metrics definitions and recording
cmd/device-api-server/main.go	Main entry point with flag parsing and signal handling
docs/*.md	Comprehensive API, operations, and design documentation
charts/device-api-server/*	Helm chart templates for Kubernetes deployment
go.mod/go.sum	Dependencies including NVML, gRPC, and Prometheus libraries

[pteranodan]

I’m concerned about adding a new service with custom methods. Should we extend the existing one with standard methods instead?

Usually, "registering" is just a Create call, "unregistering" is a Delete call, and condition updates are either an UpdateStatus or Patch call. I think we should stick to these well understood conventions.

Using custom methods also risks compatibility with standard tooling (e.g., code generators and controller-runtime). Making things harder for us both now and later.

[pteranodan]

The tight coupling between the API server and NVML feels off.

While we definitely need a default health check for GPUs, embedding it directly into the API server makes it hardware-aware rather than a ‘dumb’ transport pipe.

What do you think about moving the default health checks out of the API server entirely? The API server should be agnostic to how the state is produced. The default health checks should interact with the API server just like any other would: via the standard client interface.

[pteranodan]

What's the difference between this and the resource version inside of the metadata field?

[pteranodan]

Is this the standard or conventional environment variable used to hold the driver root location?

[pteranodan]

FYI: I’ve updated client-gen to support Create methods (#718). Once that's merged, you can use versioned clients here instead of proto stubs:

gpu, err := clientset.DeviceV1alpha1().GPUs().Create(ctx, &devicev1alpha1.GPU{}, metav1.CreateOptions{}) 

[pteranodan]

FYI: I’ve updated client-gen to support Update methods (#718). Once that's merged, you can use versioned clients here instead of proto stubs:

gpu, err := clientset.DeviceV1alpha1().GPUs().Update(ctx, &devicev1alpha1.GPU{}, metav1.UpdateOptions{})

[Copilot]

Pull request overview

Copilot reviewed 50 out of 52 changed files in this pull request and generated 2 comments.

[Copilot]

The Go version is specified as "1.25" here, but in api/go.mod (before the change) it was "1.25.5". While Go considers these equivalent, it's better to be consistent across all go.mod files in the repository. Consider using the same patch version everywhere for consistency.

[Copilot]

The "Network Security" section documents the gRPC API as unauthenticated and recommends keeping it node-local, but the Helm chart currently defaults server.grpcAddress to :50051 (see deployments/helm/nvsentinel/values.yaml), which binds the unauthenticated gRPC listener on all pod interfaces and makes it reachable by any pod in the cluster over plaintext. In a multi-tenant or partially untrusted cluster, an attacker can connect to this port and invoke write RPCs (CreateGpu, UpdateGpuStatus, DeleteGpu) to tamper with GPU health/scheduling state, despite the documentation implying a safer localhost-only default. To close this gap, align the chart defaults with the documented security posture (e.g., default to 127.0.0.1:50051 or Unix socket only) and require explicit opt-in plus TLS/mTLS and restrictive NetworkPolicy when exposing the TCP listener beyond localhost.

Suggested change

- If TCP gRPC must be exposed beyond the node (e.g., `--grpc-address=:50051`), terminate it behind TLS/mTLS or an equivalent authenticated tunnel (for example, a sidecar proxy or ingress with client auth) and enforce a least-privilege `NetworkPolicy`.

- If TCP gRPC must be exposed beyond the node, **do not** publish a plaintext, all-interfaces listener (for example, `--grpc-address=:50051`) directly to the cluster network. Instead, keep the server bound to localhost (for example, `127.0.0.1:50051`) or a Unix domain socket and expose it only behind TLS/mTLS or an equivalent authenticated tunnel (such as a sidecar proxy or ingress with client auth), combined with a least-privilege `NetworkPolicy`.

[Copilot]

Pull request overview

Copilot reviewed 66 out of 70 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 66 out of 70 changed files in this pull request and generated 12 comments.

[Copilot]

XidDescriptions is exported as a mutable map, which allows other packages to modify it at runtime (and contradicts the stated goal of keeping XID maps immutable). Since it’s only used internally, consider making it unexported (xidDescriptions) and/or exposing a function that returns a copy to prevent accidental mutation.

[Copilot]

The device-api-server container image exposes ports 8080/8081, but the server’s admin health gRPC and metrics HTTP endpoints are configured via flags (defaulting to :50051 for health gRPC and :9090 for metrics in pkg/controlplane/apiserver/options). Also, gRPC device service is on a unix socket, not a TCP port. Please align EXPOSE/comments with the actual runtime ports (or drop EXPOSE if they’re not meaningful for UDS-based deployment).

[Copilot]

Helm targets reference deployments/helm/nvsentinel, but this PR adds the chart under deployments/helm/device-api-server. As written, make helm-lint, make helm-template, and make helm-package will fail because the referenced path doesn’t exist. Please update these targets to point at the correct chart directory (and ensure the chart name matches).

Suggested change

	helm lint deployments/helm/nvsentinel
	.PHONY: helm-template
	helm-template: ## Render Helm chart templates
	helm template nvsentinel deployments/helm/nvsentinel
	.PHONY: helm-package
	helm-package: ## Package Helm chart
	@mkdir -p dist/
	helm package deployments/helm/nvsentinel -d dist/
	helm lint deployments/helm/device-api-server
	.PHONY: helm-template
	helm-template: ## Render Helm chart templates
	helm template device-api-server deployments/helm/device-api-server
	.PHONY: helm-package
	helm-package: ## Package Helm chart
	@mkdir -p dist/
	helm package deployments/helm/device-api-server -d dist/

[Copilot]

The chart README lists configuration keys that don’t exist in values.yaml (e.g., server.grpcAddress) and implies a TCP listener by default, while the chart template configures --bind-address as a unix socket. Please update the parameter table/examples to reflect the actual values.yaml schema and the current server flags to avoid broken installs.

[Copilot]

The storage backend is run in a goroutine and only awaited after the gRPC server exits. If the storage backend fails while the server is running, the server will keep serving with no coordination/shutdown (this regresses from the previous errgroup-based lifecycle). Consider running storage and server under an errgroup (or select on storageErrCh) so a storage failure triggers cancellation and a clean shutdown of the server.

[Copilot]

The CLI flag documentation here appears out of sync with the actual server flags wired up in pkg/controlplane/apiserver/options (e.g., it documents --grpc-address/--unix-socket/--health-port/--metrics-port, but the code defines --bind-address, --health-probe-bind-address, and --metrics-bind-address). Please update this doc to match the real flags so operators can deploy/configure the server correctly.

[Copilot]

This static DaemonSet manifest uses flags like --grpc-address/--health-port/--metrics-port/--unix-socket/--provider-address that do not match the current device-api-server binary flags (which use --bind-address, --health-probe-bind-address, --metrics-bind-address, etc.). As written, the container will fail to start due to unknown flags. Please update the args to the new flag names/semantics or clearly mark this manifest as deprecated/out-of-date.

[Copilot]

UpdateGpuStatusRequest lacks a namespace field while other RPCs are namespace-aware, forcing the server implementation to hardcode "default" for status updates (see TODO in gpu_service.go). This makes it impossible to update status for GPUs created outside default and is an API inconsistency. Consider adding string namespace (and possibly object meta) to UpdateGpuStatusRequest now before the API stabilizes, or explicitly document/enforce that GPUs are cluster-singleton in the API.

Suggested change

	UpdateOptions opts = 4;
	UpdateOptions opts = 4;
	// namespace specifies the namespace scoping for the GPU whose status is being updated.
	// An empty namespace is equivalent to the "default" namespace, but "default" is the
	// canonical representation.
	//
	// Optional. Defaults to unset.
	string namespace = 5;

[Copilot]

cmd/device-api-server is importing github.com/nvidia/nvsentinel/pkg/util/version, but the build/linker flags in the updated Makefile are wired to github.com/nvidia/nvsentinel/pkg/version. As a result the reported version will stay at util/version defaults and won’t reflect ldflags. Please switch this file to use pkg/version (and update the logged field names accordingly), or revert Makefile/Dockerfile ldflags to target pkg/util/version consistently across the repo.

[pteranodan]

I forgot to remove this, good catch!

[pteranodan]

Good catch again!

[pteranodan]

Meant to get back to this, thank you!

[pteranodan]

Note: all the code under pkg/service/device/<version>/ will eventually be generated.

[pteranodan]

This change hardcodes the storage backend to in-memory and completely ignores the passed storagebackend.Config.

This prevents a service from using durable storage (like etcd or an etcdshim) which was previously supported via factory.Create. This change effectively locks us into a single-node, non-durable architecture and removes the ability to configure the storage layer via flags.

Since the goal is to add an in-memory option, can we switch on the config instead? For example:

// Pseudo-code
if storageConfig.Type == "memory" {
    // In-memory path
    s, destroy, err = memory.CreateStorage(...)
} else {
    // Use the standard K8s factory
    s, destroy, err = factory.Create(configForResource, ...)
}

[pteranodan]

Minor: I think we need an error check on runtime.SerializerInfoForMediaType to prevent panics if the serializer isn't found.

[pteranodan]

Critical Issue

We cannot ignore the ResourceVersion (RV) from the request. By omitting req.GetOpts().GetResourceVersion() from storage.ListOptions, we break the fundamental List/Watch consistency contract.

Clients rely on passing a RV to ensure they don't miss events that occur in the split second between a List request finishing and a Watch request starting. Without it, clients can permanently miss events.

TestIntegration_WatchResumption demonstrates where a client lists, a new item is created, and then the client Watches (using the List RV) and the client never receives an event for the item created in between the List and Watch.

[pteranodan]

This request structure deviates from the established design pattern for Updates in Kubernetes and in this file (see UpdateGpuRequest). Typically, update operations (including status updates) accept the whole object and use the metadata (e.g., RV) within it for addressing concurrency control.

Additionally, by splitting fields out manually we risk accidentally omitting necessary metadata (e.g., RV, namespace, generation, etc.).

Suggestion:

message UpdateGpuRequest {
  // gpu is the GPU resource whose status to update.
  Gpu gpu = 1;

  // opts contains the options for the update.
  UpdateOptions opts = 2;
}

[pteranodan]

This introduces a duplicate version package. We already have an implementation at pkg/util/version. We should combine these to prevent confusion about which to use.

I like the pkg/version import path you chose; it is cleaner and more discoverable. I suggest we move the logic from pkg/util/version into this new package and delete the existing directory.

The existing package includes a HTTP handler (for /version) and a UserAgent helper that are actively used. We should keep those.

We can remove RegisterComponent, it's not being used. Forgot to remove it earlier.

[pteranodan]

We should use the generated Kubernetes-style client SDK (pkg/client-go) instead of raw gRPC service client.

Example:

// Get the latest object to ensure we have the current ResourceVersion.
gpu, err := p.client.DeviceV1alpha1().GPUs().Get(ctx, gpuName, metav1.GetOptions{})
if err != nil {
    return err
}

// Modify status
gpu.Status = newStatus

// Update
_, err = p.client.DeviceV1alpha1().GPUs().UpdateStatus(ctx, gpu, metav1.UpdateOptions{})

[pteranodan]

Minor: Providers must never import internal packages.

[pteranodan]

As noted in other comments, the current implementation has some gaps regarding ResourceVersion semantics, which will break controllers and informers that rely on those guarantees.

I suggest we make Kine (with SQLite) the default in-memory path. Because Kine is the default backend for K3s (a CNCF certified distro), we know it passes the upstream Kubernetes storage conformance tests. This gives us verified correctness out of the box.

We can still include this custom implementation, but perhaps behind a flag like --experimental-in-memory-storage until we can verify it against the full storage compliance suite.

[Copilot]

Pull request overview

Copilot reviewed 87 out of 91 changed files in this pull request and generated 1 comment.

[Copilot]

The method GetRetiredPagesPendingStatus() is called on the device but is not defined in the Device interface (pkg/providers/nvml/interface.go:36-42). This will cause a compilation error when building with -tags=nvml. Add this method to the Device interface and implement it in both the wrapper and mock implementations.

Suggested change

	// 2. Check for pending page retirements (ECC errors)
	if pending, ret := device.GetRetiredPagesPendingStatus(); ret == nvml.SUCCESS {
	if pending == nvml.FEATURE_ENABLED {
	p.logger.V(1).Info("GPU has pending page retirements", "uuid", uuid)
	return false, nil
	}
	}
	// Device is accessible and no pending issues
	// Device is accessible according to basic checks

[Copilot]

Pull request overview

Copilot reviewed 110 out of 114 changed files in this pull request and generated 13 comments.

[Copilot]

The build-info metric still uses the name/help text "device_apiserver_build_info" / "device-apiserver" even though the binary/component is now "device-api-server". At minimum, consider updating the Help string and the service label value to match the new component name; if the metric name is kept for compatibility, documenting that would help avoid confusion.

[Copilot]

Install() now unconditionally uses the in-memory storage backend and explicitly ignores the provided storagebackend.Config. This means configuring the server for a persistent backend won’t actually persist GPU resources, which is a surprising behavior change. Consider either wiring cfg into backend selection (restore the factory.Create path when persistence is desired) or returning an explicit error when a non-in-memory backend is configured but unsupported.

[Copilot]

The PR description states MaxConditionsPerGpu = 32, but this provider-side cap uses maxConditions = 100. Please reconcile the intended limit (and ideally share a single constant) so behavior matches the documented design decision.

[Copilot]

sendError also uses time.After in a select; repeated errors can accumulate timers similarly. Consider using a reusable timer (or a non-blocking send with explicit drop policy) to avoid timer churn/leaks.

[Copilot]

Validate() accepts both "unix://" and "unix:" targets, but the error message only mentions "unix://" (and omits "unix:"). Please update the message to reflect the actual accepted schemes (unix://, unix:, dns:, passthrough:) to avoid confusing users.

[Copilot]

The PrometheusRule runbook_url links point to https://github.com/nvidia/device-api/... which doesn’t match this repository (nvidia/nvsentinel). If these docs live in-repo, update the URLs to the correct location to avoid broken runbook links.

[Copilot]

Image references are inconsistent across deployment artifacts: the Helm chart defaults to ghcr.io/nvidia/device-api-server, while the static manifest uses ghcr.io/nvidia/nvsentinel/device-api-server. This can cause users to deploy the wrong images; consider standardizing on a single registry/repository naming scheme (or documenting why they differ).

[Copilot]

Using time.After inside the per-event select allocates a new timer each loop iteration; if the send succeeds quickly, the timer still lives until it fires, which can cause unbounded timer buildup under load. Prefer a reusable time.Timer (stop+drain) or a non-allocating timeout strategy (e.g., context with deadline) for the send timeout.

[Copilot]

README examples reference the module path "github.com/nvidia/device-api/...", but this repo’s Go module is "github.com/nvidia/nvsentinel" (see go.mod). These import paths / go get instructions won’t work as written; please update them to the correct module paths (or clarify if the repo/module is being renamed).

[Copilot]

This manifest references images under ghcr.io/nvidia/nvsentinel/... while the Helm chart defaults to ghcr.io/nvidia/device-api-server / device-api-server-sidecar. Please align these image repositories/tags (or explicitly document the intended difference) to avoid deployment confusion.