Skip to content

Integrate k8s-launch-kit as a snapshot Collector #828

Description

@mchmarny

Part of #827.

Goal

Persist NIC/fabric/rail topology in AICR's snapshot by consuming k8s-launch-kit (l8k) as a Go library that implements AICR's Collector interface (pkg/collector/types.go:27).

Background

l8k already encodes the discovery logic AICR needs (kernel module graph from /sys/module, GPU topology via nvidia-smi, NIC inventory via NicDevice CRs, machine-type detection, node-label patching). Today that logic is invoked by the l8k CLI via a mutating flow:

  1. Patch/create NicClusterPolicy so the network-operator stands up nic-configuration-daemon.
  2. Exec into those daemon pods to collect kernel/hardware data.
  3. Read NicDevice CRs.
  4. Patch node labels.
  5. Defer-cleanup the patch.

See pkg/networkoperatorplugin/discovery.go:99-260. This violates AICR's collector contract (read-only, bounded, no side effects). Phase 1 must reconcile that.

Options

Option A — Static collector via presets + releases catalog (no cluster contact)

Wrap pkg/presets.LoadPreset(machineType, gpuType) and pkg/networkoperatorplugin.LookupRelease. Caller (or AICR's existing GPU collector) supplies machine type + GPU product; the new collector returns l8k's LaunchKubernetesConfig populated from baked-in presets.

  • Pros: ships immediately. Zero footprint. No cluster mutation. No new RBAC.
  • Cons: no live data — degenerate "collector" that just looks up a table. Doesn't validate actual hardware. Useful as a fallback / offline mode, not as the primary path.

Option B — Wrap DiscoverClusterConfig as-is

AICR collector calls l8k's existing mutating DiscoverClusterConfig directly.

  • Pros: full topology truth — fabric, rails, NIC inventory, NUMA, GPU↔NIC proximity.
  • Cons:
    • Violates Collector contract: mutates NicClusterPolicy, requires pod-exec RBAC.
    • Requires network-operator already installed — chicken-and-egg if AICR is meant to produce that config.
    • Race conditions on parallel snapshots against the same cluster (both will fight over the patch).
    • Hard-coded namespace fallback (discovery.go:201) may conflict with multi-tenant deployments.

Option C — Read-only discovery upstream, then wrap (recommended)

File an l8k issue requesting DiscoverReadOnly(ctx, client) (*ClusterConfig, error):

  • Uses l8k's existing reuseExisting branch (discovery.go:112-126) when nic-configuration-daemon is already running.
  • Returns ErrNotInstalled (or similar) when not installed.
  • Does not create/patch NicClusterPolicy. Does not patch node labels.

AICR collector calls it; falls back to Option A semantics when not installed.

  • Pros: clean contract, honors AICR collector invariants, still gets live data when available.
  • Cons: blocked on upstream PR.

Recommended: Option C, with Option A wired in as the no-cluster fallback.

Work breakdown

  1. Upstream first (Option C path): open issue/PR on Expose read-only DiscoverReadOnly() API for library consumers k8s-launch-kit#62 exposing DiscoverReadOnly. Tracked separately so AICR work isn't blocked on the entire feature.
  2. Extend Factory (pkg/collector/factory.go:30) with CreateNetworkCollector() Collector + functional option to enable. Note the factory is a closed interface today — adding a method is a breaking change for external implementers (audit first).
  3. New package pkg/collector/network implementing Collector. Embed l8k's LaunchKubernetesConfig as a typed sub-section in *measurement.Measurement (define the type in pkg/measurement rather than depending on l8k's struct directly, to insulate AICR from upstream churn).
  4. Snapshotter wiring: add to pkg/snapshotter agent + aicr snapshot CLI. Gate behind a flag (--collect-network) while shape stabilizes.
  5. --no-cluster mode: collector returns empty/skipped measurement; never touches the cluster.
  6. RBAC: if Option B/C lands, document the pod-exec/list-pods/list-NicDevices delta and update Helm chart RBAC for aicrd.
  7. Tests: unit-level table-driven tests with a fake client.Client; KWOK e2e for the live-discovery path (gated on cluster having network-operator).

Open questions

  • Module path: l8k advertises github.com/nvidia/k8s-launch-kit (lowercase) but the GitHub org is NVIDIA. Confirm the canonical Go module path before vendoring.
  • Transitive dep cost: importing pkg/networkoperatorplugin pulls Mellanox/network-operator, Mellanox/nic-configuration-operator, controller-runtime, and Mellanox CRD modules. Acceptable, or do we ask l8k to split the read-only discovery into a leaf package with thinner deps?
  • Versioning: l8k has no v1.0.0 tag. Pin by SHA, vendor with replace, or ask upstream for a semver tag before we depend on it?
  • Namespace assumptions: l8k's alternateNamespace fallback assumes one of two well-known namespaces — does this break AICR's multi-tenant target?
  • Schema stability: if LaunchKubernetesConfig shape changes, snapshot YAML changes downstream. Should AICR vendor its own DTO and translate at the boundary?

Acceptance criteria

  • aicr snapshot --collect-network populates a network (or equivalent) section with NIC/fabric/rail data when network-operator is present.
  • When network-operator is absent or --no-cluster is set, the collector emits a skipped/empty measurement and the snapshot still succeeds.
  • No mutation of the target cluster from the collector path. (Option B is explicitly rejected unless we revisit the policy.)
  • Unit tests for happy path, no-cluster path, and l8k-not-installed path.
  • Documentation: docs/contributor/collectors.md (new section), docs/user/cli-reference.md (new flag).

Metadata

Metadata

Assignees

Fields

No fields configured for Enhancement.

Projects

No projects

Milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions