feat(deployment): add llm-d as a deployment target

[coketaste]

Summary

• Adds llm-d (Gateway API Inference Extension-based distributed inference) as a new madengine deployment target, benchmarked via an ordinary single-pod Kubernetes Job that subclasses KubernetesDeployment.
• Supports two modes: attach (llm_d.endpoint_url set — benchmark an existing stack, never installs/uninstalls anything) and managed (madengine stands up 3 pinned helm releases, waits for readiness, resolves the endpoint from the live Gateway, benchmarks, and tears down — even on success, with unwind-on-failure and no-mask-on-teardown-failure).
• Strictly additive: no edits to kubernetes.py, the k8s mixins, or any existing function in config_loader.py. Every existing path takes the identical branch when llm_d is absent, guarded by tests/unit/test_llm_d_regression.py.
• Adds docs/llm-d.md, example configs under examples/llm-d-configs/, and updates the README/deployment/configuration/launchers docs.

Validation status. Code-complete and green across unit, integration and e2e. The one thing this branch has not had is a run against a real llm-d stack on a GPU cluster — no such cluster was available here. Every llm-d test mocks the Kubernetes client and helm, so they prove the client contract and the control flow, not that the charts stand up. Reviewers with cluster access: that is the gap worth pointing at.

Review feedback addressed

• factory.py: llm-d registration now catches any exception (not just ImportError) from the optional import and warns instead of letting a non-ImportError escape register_default_deployments() and take slurm/k8s down with it.
• llm_d.py (_resolve_endpoint): fails with the ambiguous Gateway names listed when more than one owned Gateway matches, instead of silently benchmarking an arbitrary candidate.
• llm_d.py (_wait_for_model_servers): now best-effort on Kubernetes API failure (e.g. RBAC forbidding list_namespaced_deployment) — warns and falls back to helm's own readiness gate instead of aborting the run.
• test_llm_d.py: replaced a source-text regression guard with behavioral tests that exercise _load_and_merge_manifest directly.

Self-review pass (64676ec)

A re-read of the full build → run → deploy path turned up four correctness bugs, none of which a test was failing on:

• The benchmark client reserved a GPU it never uses. load_k8s_config selects k8s/profiles/single-gpu.json for the client Job, and that preset pins gpu_count: 1 — so the CPU-only load generator competed for a GPU with the very stack it measures. load_llmd_config now defaults k8s.gpu_count to 0. The fix cannot live in presets/llm-d/defaults.json: those are the base of the merge, so the profile would overwrite them. Keying off the raw user_config is what separates "the user asked for GPUs" from "a profile defaulted them" — an explicit k8s.gpu_count still wins, as do the runtime overrides.
• A helm release could be orphaned by Ctrl-C. _standup recorded each release after install returned. KeyboardInterrupt is a BaseException and passes straight through prepare()'s except Exception, while helm may already have created a GPU-holding release. Teardown only walks that list. Releases are now recorded before helm runs; uninstall --ignore-not-found makes naming one that was never created free.
• Attach mode required a GPU node. It called KubernetesDeployment.validate(), which rejects a cluster with no node advertising gpu_resource_name — but attach mode only runs the CPU-only client here; the GPUs live in a stack madengine did not install, possibly not even in this cluster. New _validate_cluster_access() keeps the connectivity and namespace checks and drops the GPU gate.
• An unreadable cache Job surfaced as a stack trace. _wait_for_cache_job let a raw ApiException escape on a 403 or a deleted Job; it is now a ConfigurationError with recovery guidance.

Cheap hardening in the same pass:

• Warn when the model servers will run the benchmark client's own image. _resolve_model_images defaults prefill/decode.image to it, which is right for a vLLM image and wrong for the slim client image the docs otherwise recommend. The two are indistinguishable from madengine's side, so it warns rather than fails.
• Warn when --tags matched several models: llm-d benchmarks only the first, and here that choice also decides what the stack serves.
• _resolve_endpoint prefers a plain-HTTP listener over listeners[0] and derives scheme/port from it. Listener order is not meaningful and gateways commonly publish both http and https.
• Docs: record the new gpu_count default, and reconcile "a python:3.11-slim base is enough" with the section stating the client image also serves the model — the first holds in attach mode, or when prefill/decode.image are set explicitly.

19 new tests cover each fix. Two existing stack tests asserted the old record-after-success behaviour and now assert the new one; test_attach_mode_does_not_require_helm patched a parent validate() that attach mode no longer calls.

Follow-ups (considered, deliberately not in this PR)

• Split llm_d.gateway into two keys. One value currently feeds both gateway.gatewayClassName (infra chart) and provider.name (gaie chart). Those coincide for agentgateway and istio, but they are different namespaces of value, and a cluster whose GatewayClass name is not also a GAIE provider name has no way to express that. Wants llm_d.gateway.class_name / llm_d.gateway.provider — a config-shape change, better made once the pins are known-good than speculatively.
• A helm template smoke test in CI. Rendering the pinned chart refs without a cluster would catch values-schema drift between llm_d_stack.py and an upstream chart release. Needs network access to the OCI registry from CI, which is a separate question from this PR.

Test plan

• pytest tests/unit — 830 passed
• pytest tests/integration — 155 passed, 1 skipped (unrelated GPU-arch skip)
• pytest tests/e2e — 60 passed (32m; confirms the llm-d work is additive — no e2e test touches k8s/helm)
• black, isort, flake8 clean on all new/changed files
• mypy — no new errors (pre-existing types-PyYAML stub warning only)
• Internal doc links verified
• End-to-end run against a real llm-d stack on a GPU cluster (not exercised in this environment)

[Copilot]

🟡 Changes recommended

The optional llm-d registration currently only catches ImportError and can still break importing the deployment factory (and thus core targets) if llm-d import fails with a non-ImportError exception.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds llm-d as a new deployment target in madengine’s deployment layer, enabling benchmarking of a Kubernetes-native distributed inference stack either by attaching to an existing gateway endpoint or managing the stack lifecycle via Helm (including dry-run rendering).

Changes:

• Introduces LlmdDeployment (subclassing KubernetesDeployment) plus LlmdStack to render Helm values, install/uninstall releases, and resolve a live Gateway endpoint.
• Extends orchestration/config plumbing to persist/merge the llm_d block through build→run and to infer llm-d ahead of k8s when llm_d is present.
• Adds presets, extensive unit/regression tests, example configs, and documentation for llm-d usage and behavior.

File summaries

File	Description
tests/unit/test_llm_d.py	Unit tests for llm-d attach mode, config merge, rendered Job env contract, teardown behavior
tests/unit/test_llm_d_stack.py	Unit tests for managed-mode Helm mechanics, readiness/endpoint resolution logic, dry-run behavior
tests/unit/test_llm_d_regression.py	Regression guards to ensure non-llm-d paths remain unchanged
src/madengine/orchestration/run_orchestrator.py	Adds llm_d to manifest merge lists; target inference prefers llm-d over k8s
src/madengine/orchestration/build_orchestrator.py	Records llm-d as target and persists llm_d into deployment_config
src/madengine/deployment/presets/llm-d/defaults.json	Adds llm-d defaults layered beneath k8s presets
src/madengine/deployment/llm_d.py	Implements llm-d deployment target (attach/managed/dry-run, readiness, endpoint resolution, teardown)
src/madengine/deployment/llm_d_stack.py	Implements Helm values generation and helm command construction for llm-d components
src/madengine/deployment/factory.py	Registers llm-d as an optional deployment target
src/madengine/deployment/config_loader.py	Adds load_llmd_config() to apply llm-d defaults + k8s preset stack
src/madengine/cli/validators.py	Allows llm_d as a valid top-level additional_context key
README.md	Documents llm-d as a deployment target and adds docs link
examples/llm-d-configs/README.md	Explains llm-d example configs and how to run them
examples/llm-d-configs/01-attach-existing-stack.json	Attach-mode example config
examples/llm-d-configs/02-managed-dry-run.json	Managed dry-run example config
examples/llm-d-configs/03-managed-disaggregated.json	Managed disaggregated serving example config
examples/llm-d-configs/04-managed-aggregated-keep-stack.json	Managed aggregated serving example config with teardown: false
docs/README.md	Adds llm-d doc entry
docs/llm-d.md	Full llm-d deployment guide (modes, config reference, reliability notes, troubleshooting)
docs/launchers.md	Clarifies llm-d is a deployment target (not a launcher) and adds guidance reference
docs/deployment.md	Adds llm-d to deployment overview, inference diagrams, and comparison table
docs/configuration.md	Adds llm-d configuration section and updates config layering notes
CLAUDE.md	Updates repository architecture notes to include llm-d deployment target

Review details

• Files reviewed: 23/23 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

A few concrete issues in optional deployment registration and in runnable example/docs chart pin placeholders should be fixed to avoid silent feature disablement and confusing helm-time failures.

Review details

Suppressed comments (7)

Previously missed (5) — in code that hasn't changed since the last review.

docs/configuration.md:631

• This managed-mode example uses "" for chart versions, which bypasses the "unpinned" check (non-null) and turns a missing-pin into a helm failure later. Consider using null in the docs snippet so managed mode refuses to run until the user pins real versions.
  docs/llm-d.md:106
• The quick-start snippet uses "" for chart versions, but madengine treats any non-null version as pinned. Copy/pasting this will likely fail inside helm rather than triggering the intended fast validation error. Using null here makes the snippet fail fast until real versions are provided.
  examples/llm-d-configs/02-managed-dry-run.json:23
• These example configs use "" as the chart version, which is truthy and will bypass madengine's "unpinned charts" validation. If a user runs this without editing, they'll get a helm failure instead of the intended fast refusal. Using null here makes the examples fail fast until the user pins real versions.
  examples/llm-d-configs/03-managed-disaggregated.json:22
• Using "" as a placeholder makes the config look pinned to madengine (non-null), so it will attempt a managed run and fail later in helm. Prefer null in committed example configs so madengine refuses until a real version is set.
  examples/llm-d-configs/04-managed-aggregated-keep-stack.json:19
• "" bypasses the unpinned-chart guard (it isn't null/empty), so forgetting to replace it yields a helm error instead of a clear validation failure. Setting these to null makes the example safe to run and forces explicit pinning.

src/madengine/deployment/factory.py:108

• The llm-d registration is guarded only by except ImportError and silently ignores failures. If the optional module fails to import for a different reason (e.g., SyntaxError, RuntimeError from a missing runtime dependency), this will still crash import-time registration and can break unrelated deployments; additionally, users get no warning that llm-d is unavailable.

        DeploymentFactory.register("llm-d", LlmdDeployment)
        DeploymentFactory.register("llm_d", LlmdDeployment)
    except ImportError:
        pass

tests/unit/test_llm_d.py:136

• This assertion is brittle because it depends on the exact quoting/style in run_orchestrator's source (e.g., Black could change quotes), causing false negatives. If you keep a source-based guard here, make it tolerant of single/double quotes at least.

        source = inspect.getsource(run_orchestrator)
        # Both merge loops enumerate their keys literally; neither may omit llm_d.
        assert source.count('"llm_d"') >= 2

• Files reviewed: 23/23 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new llm-d code has a couple of validated correctness/robustness issues (notably ambiguous Gateway selection and missing type checks on user-provided config fields) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/madengine/deployment/llm_d.py:204

• llm_d.charts is assumed to be a dict (charts.items()), so a malformed config (e.g. string/null) will raise an AttributeError during validation instead of producing a clear configuration error. Consider normalizing to {} and explicitly rejecting non-mapping values so validate() fails cleanly with a helpful message.
  src/madengine/deployment/llm_d_stack.py:140
• llm_d.extra_values is treated as a dict, but if a user supplies a non-mapping (string/list/null) this code can behave incorrectly (e.g., substring checks on strings) and then fail with an unhelpful AttributeError. Consider validating it’s a mapping and raising a clear LlmdStackError early.

• Files reviewed: 28/28 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Optional llm-d registration and managed-mode readiness polling have failure modes that can unnecessarily break core deployment availability or abort runs under common RBAC constraints.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/madengine/deployment/factory.py:108

• register_default_deployments() only swallows ImportError for the optional llm-d target. If llm_d.py raises a different exception at import time (e.g., due to a transitive dependency/API mismatch), it will prevent factory.py from importing and can break registration of the core slurm/k8s targets. To keep llm-d truly optional and avoid cascading failures, catch a broader exception here and emit a warning (similar to the Kubernetes optional dependency path).

    # Register llm-d in its own try/except: it builds on the Kubernetes target,
    # so a failure here must never take slurm or k8s down with it.
    try:
        from .llm_d import LlmdDeployment

        DeploymentFactory.register("llm-d", LlmdDeployment)
        DeploymentFactory.register("llm_d", LlmdDeployment)
    except ImportError:
        pass

• Files reviewed: 28/28 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There’s a concrete build-time target inference mismatch for empty llm_d configs and a couple of test issues that should be tightened/cleaned up before approval.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/madengine/orchestration/build_orchestrator.py:1323

• Target auto-detection uses self.additional_context.get('llm_d'), which is falsey for an empty dict. Since deployment inference elsewhere keys off presence of llm_d, a config like { "llm_d": {} } would be recorded as non-llm-d at build time but inferred as llm-d at run time, leading to confusing mismatches. Prefer checking key presence instead of truthiness.
  tests/unit/test_llm_d.py:1165
• This test claims to verify that cache population happens between _ensure_model_pvc() and stack.write_values(), but the current calls list only records those two events, so it doesn't actually assert that _populate_model_cache() occurred in between. Recording an event off create_namespaced_job makes the ordering contract explicit and will catch regressions if _standup() is reordered.
  tests/unit/test_llm_d_stack.py:512
• _api() constructs and returns patch.dict('sys.modules', {}), but none of the call sites uses it (they immediately bind it to _). This adds noise and implies a sys.modules patch that never actually happens. Returning None keeps the existing tuple shape without creating an unused patch object.

• Files reviewed: 28/28 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

A unit test doesn’t actually enforce its stated ordering guarantee, and the CRD-install guidance is incomplete/misleading for InferencePool/GAIE prerequisites (docs + runtime error hint).

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

docs/llm-d.md:63

• This prerequisites section only points to the standard Gateway API CRD install, but managed mode also requires the Gateway API Inference Extension (InferencePool/EPP) CRDs. Add a second reference so users can install both sets of CRDs before running managed mode.
  src/madengine/deployment/llm_d.py:507
• The missing-CRD hint only shows how to install the core Gateway API CRDs, but llm-d also requires GAIE / InferencePool CRDs. When InferencePool is missing, this guidance will send users in circles (they’ll re-run after installing standard Gateway API and still fail). Include a second hint/link for the inference extension CRDs.
  tests/unit/test_llm_d.py:1165
• This test claims to assert that model-cache population runs between _ensure_model_pvc() and stack.write_values(), but calls never records the cache population step, so the ordering isn’t actually checked (only that a Job was created at some point). Add a marker (e.g., via create_namespaced_job.side_effect) and assert the full call order.

• Files reviewed: 28/28 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Build-time target inference can mis-detect llm-d/k8s/slurm when blocks are empty dicts, and llm-d teardown currently clears tracked releases even if uninstalls fail, making retries/cleanup harder.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/madengine/deployment/llm_d.py:986

• _teardown_stack() clears self._installed_releases unconditionally, even when one or more helm uninstall calls fail. That loses the remaining release names, prevents a later retry (e.g., if teardown runs again in another finally/cleanup path), and makes post-mortem cleanup harder despite the failures being explicitly tolerated.
  src/madengine/orchestration/build_orchestrator.py:1326
• Build-time target auto-detection uses truthiness checks (e.g. self.additional_context.get("llm_d")), so a valid-but-empty block like {"llm_d": {}} (or {"k8s": {}} / {"slurm": {}}) will be treated as absent and the target will fall back to local. This is inconsistent with run-time inference ("llm_d" in config) and can cause manifests to record the wrong target for minimal configs that rely on presets/defaults.

• Files reviewed: 28/28 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There are confirmed robustness bugs in LlmdDeployment around role image defaulting when image: null is present and validation crashing when llm_d.charts is explicitly set to null.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/madengine/deployment/llm_d.py:208

• _resolve_model_images() only defaults a role image when the "image" key is absent. If a user config (or JSON preset) explicitly includes "image": null, this code will treat it as user-specified and skip defaulting, leaving the chart to pick its own image (and also skipping the client-image warning tracking). That contradicts the documented behavior that the built image is the default unless the user names one explicitly.
  src/madengine/deployment/llm_d.py:529
• validate() can crash if a user explicitly sets llm_d.charts to null (or any non-dict) in additional_context: self.llmd_config.get("charts", {}) will return None when the key exists, and the subsequent charts.items() will raise AttributeError. Coercing to an empty dict keeps validation on the intended False/print-path.

• Files reviewed: 29/29 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Managed-mode validation currently accepts the example placeholder charts.<name>.version: "<pin>", so users can pass validate() and then fail later inside helm with a harder-to-diagnose error.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/madengine/deployment/llm_d.py:542

• Managed-mode chart version validation treats any truthy value as “pinned”, so the example placeholder string "" will pass validate() and only fail later during helm upgrade --install (harder to diagnose). Since the repo’s example configs and docs use "", it’s worth rejecting that placeholder explicitly and telling the user to replace it with a real chart version.

• Files reviewed: 29/29 changed files
• Comments generated: 0 new
• Review effort level: Lite