feat(models): vllm k8s support - #305
Merged
Merged
Conversation
Contributor
|
benmccown
marked this pull request as ready for review
June 15, 2026 20:26
benmccown
force-pushed
the
vllm-k8s-support/bmccown
branch
from
June 15, 2026 20:29
d78cd03 to
29d3f93
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors all backends to accept ChangesModelContext interface refactor across backends
Kubernetes reconciler architecture: base classes and helpers
Kubernetes reconciler implementations
K8s backend and configuration
Testing, configuration, and documentation
Sequence Diagram(s)sequenceDiagram
participant DeploymentReconciler
participant K8sBackend as K8sNimOperatorServiceBackend
participant K8sReconciler
participant K8s as Kubernetes API
DeploymentReconciler->>K8sBackend: get_model_deployment_status(ctx: ModelContext)
K8sBackend->>K8sBackend: _resolve() → ResolvedDeployment
K8sBackend->>K8sBackend: _select_reconciler(engine=vllm)
K8sBackend->>K8sReconciler: get_status(resolved)
alt Serving Deployment absent, puller Job running
K8sReconciler->>K8s: read Job status
K8sReconciler-->>K8sBackend: PENDING
else Puller Job succeeded
K8sReconciler->>K8s: delete Job (release RWO)
K8sReconciler->>K8s: create Deployment + Service
K8sReconciler-->>K8sBackend: PENDING (server startup)
else Serving Deployment ready
K8sReconciler->>K8s: check pod replicas
K8sReconciler-->>K8sBackend: READY
end
K8sBackend-->>DeploymentReconciler: DeploymentStatusUpdate
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py (1)
264-328:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPending timeout uses hardcoded
_nim_reconcilerinstead of selected reconciler.Line 315-316 calls
self._nim_reconciler._find_pod_nameand_build_pending_timeout_erroreven for vLLM deployments. Since both methods are inherited fromBaseReconciler, it works, but should use the already-selectedreconcilerfor clarity and correctness if the methods are ever overridden.if elapsed >= self._backend_config.pending_timeout_seconds: - pod_name = self._nim_reconciler._find_pod_name(resource_name) - return self._nim_reconciler._build_pending_timeout_error(resource_name, elapsed, pod_name) + pod_name = reconciler._find_pod_name(resource_name) + return reconciler._build_pending_timeout_error(resource_name, elapsed, pod_name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py` around lines 264 - 328, In the get_model_deployment_status method, the PENDING timeout handling at lines 315-316 incorrectly uses the hardcoded self._nim_reconciler instead of using the already-selected reconciler variable. Replace the two method calls (self._nim_reconciler._find_pod_name and self._nim_reconciler._build_pending_timeout_error) with calls to the reconciler variable that was selected earlier in the method based on the deployment's engine type. This ensures consistency and correctness if reconciler implementations override these methods.
🧹 Nitpick comments (1)
services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py (1)
221-237: 💤 Low valueMissing return type annotation on
_select_reconciler.- def _select_reconciler(self, engine: str): + def _select_reconciler(self, engine: str) -> Optional[BaseReconciler]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py` around lines 221 - 237, Add a return type annotation to the `_select_reconciler` method. Since the method can return either a reconciler object (self._k8s_reconciler or self._nim_reconciler) or None, use an Optional type annotation that properly reflects the possible return types of the reconciler instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/core/models/src/nmp/core/models/controllers/backends/engine.py`:
- Around line 52-55: The `resolve_health_path` function returns the
user-supplied `health_check_path` attribute without normalizing it, which can
result in paths without a leading slash (like `health` instead of `/health`)
that produce invalid downstream probe URLs. Normalize the `explicit_path` before
returning it by ensuring it has a leading slash - you can do this by checking if
the path starts with `/` and prepending it if it doesn't, or by using a string
formatting approach that guarantees the correct format.
---
Outside diff comments:
In
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py`:
- Around line 264-328: In the get_model_deployment_status method, the PENDING
timeout handling at lines 315-316 incorrectly uses the hardcoded
self._nim_reconciler instead of using the already-selected reconciler variable.
Replace the two method calls (self._nim_reconciler._find_pod_name and
self._nim_reconciler._build_pending_timeout_error) with calls to the reconciler
variable that was selected earlier in the method based on the deployment's
engine type. This ensures consistency and correctness if reconciler
implementations override these methods.
---
Nitpick comments:
In
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py`:
- Around line 221-237: Add a return type annotation to the `_select_reconciler`
method. Since the method can return either a reconciler object
(self._k8s_reconciler or self._nim_reconciler) or None, use an Optional type
annotation that properly reflects the possible return types of the reconciler
instances.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2dc7131b-6abd-4bfc-b17e-3be3cb08c715
📒 Files selected for processing (20)
services/core/models/src/nmp/core/models/controllers/backends/backends.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.pyservices/core/models/src/nmp/core/models/controllers/backends/engine.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/config.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/base.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/nim_operator.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.pyservices/core/models/src/nmp/core/models/controllers/backends/none_backend.pyservices/core/models/src/nmp/core/models/controllers/backends/vllm_compiler.pyservices/core/models/src/nmp/core/models/controllers/deployment_reconciler.pyservices/core/models/tests/integration/test_models.pyservices/core/models/tests/unit/controllers/backends/test_vllm_compiler.pyservices/core/models/tests/unit/controllers/backends/test_vllm_k8s_compiler.pyservices/core/models/tests/unit/controllers/test_backend_config_fields.pyservices/core/models/tests/unit/controllers/test_backend_registry.pyservices/core/models/tests/unit/controllers/test_deployment_reconciler.pyservices/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.py
💤 Files with no reviewable changes (2)
- services/core/models/tests/unit/controllers/test_backend_registry.py
- services/core/models/tests/integration/test_models.py
Contributor
|
🌿 Preview your docs: https://nvidia-preview-vllm-k8s-support-bmccown.docs.buildwithfern.com/nemo-platform |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/core/inference-gateway/tests/integration/conftest.py (1)
110-112: ⚡ Quick winUse concrete types for the updated status signature.
This changed method uses
Anyfor new params; switch to concrete types to match the backend contract and catch signature drift in tests.Suggested patch
-from typing import Any, Generator +from typing import Any, Generator, Optional +from nemo_platform.types.inference.model_deployment import ModelDeployment +from nemo_platform.types.inference.model_deployment_config import ModelDeploymentConfig +from nemo_platform.types.models.model_entity import ModelEntity @@ - async def get_model_deployment_status( - self, deployment: Any, config: Any = None, model_entity: Any = None - ) -> DeploymentStatusUpdate: + async def get_model_deployment_status( + self, + deployment: ModelDeployment, + config: Optional[ModelDeploymentConfig] = None, + model_entity: Optional[ModelEntity] = None, + ) -> DeploymentStatusUpdate:As per coding guidelines, "**/*.py`: Always prefer concrete type hints over string-based ones in Python code; do not import types under TYPE_CHECKING, instead import types as regular imports when possible".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/inference-gateway/tests/integration/conftest.py` around lines 110 - 112, The get_model_deployment_status method in the mock class uses `Any` type hints for the deployment, config, and model_entity parameters instead of concrete types. Replace these `Any` type annotations with the appropriate concrete types that match the actual backend contract for this method. Import any required types as regular imports (not under TYPE_CHECKING) at the top of the file to ensure the signature accurately reflects the backend implementation and enables catching any unintended signature drift during testing.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@services/core/inference-gateway/tests/integration/conftest.py`:
- Around line 110-112: The get_model_deployment_status method in the mock class
uses `Any` type hints for the deployment, config, and model_entity parameters
instead of concrete types. Replace these `Any` type annotations with the
appropriate concrete types that match the actual backend contract for this
method. Import any required types as regular imports (not under TYPE_CHECKING)
at the top of the file to ensure the signature accurately reflects the backend
implementation and enables catching any unintended signature drift during
testing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 071e1488-5ec9-4939-b31f-9b89d9763dd1
📒 Files selected for processing (4)
docs/set-up/config-reference.mdxservices/core/inference-gateway/tests/integration/conftest.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/config.pyservices/core/models/tests/integration/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/set-up/config-reference.mdx
mckornfield
reviewed
Jun 16, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py (1)
360-365: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIsolate each reconciler delete.
If the NIM delete raises, the vLLM delete is never attempted. That breaks the stated “one reconciler’s failure never aborts the other” behavior.
Proposed fix
- for result in ( - await self._nim_reconciler.delete(workspace, name), - await self._k8s_reconciler.delete(workspace, name), - ): + for path, reconciler in (("nim", self._nim_reconciler), ("vllm", self._k8s_reconciler)): + try: + result = await reconciler.delete(workspace, name) + except Exception as e: + logger.warning( + "Delete path failed", + extra={"workspace": workspace, "deployment_name": name, "path": path, "error": str(e)}, + ) + errors.append(f"{path} delete failed: {type(e).__name__}: {e}") + continue if result.status == "ERROR" and result.error_details: errors.extend(result.error_details.get("errors", []))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py` around lines 360 - 365, The current code structure uses tuple unpacking where both self._nim_reconciler.delete(workspace, name) and self._k8s_reconciler.delete(workspace, name) are awaited together, which means if the NIM reconciler delete raises an exception, the K8S reconciler delete will never execute. Isolate each reconciler delete operation by executing them separately with individual try-except blocks around each delete call, ensuring that an exception from one reconciler does not prevent the other from attempting to delete.
🧹 Nitpick comments (1)
services/core/inference-gateway/tests/integration/conftest.py (1)
90-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ModelContextinstead ofAnyfor mock backend context methods.These signatures should stay concrete to preserve contract checks.
Proposed change
- async def create_model_deployment(self, ctx: Any) -> DeploymentStatusUpdate: + async def create_model_deployment(self, ctx: ModelContext) -> DeploymentStatusUpdate: """Record call and return configured response.""" self.create_calls.append((ctx.model_deployment, ctx.model_deployment_config, ctx.model_entity)) return self.create_response - async def update_model_deployment(self, ctx: Any) -> DeploymentStatusUpdate: + async def update_model_deployment(self, ctx: ModelContext) -> DeploymentStatusUpdate: """Record call and return configured response.""" self.update_calls.append((ctx.model_deployment, ctx.model_deployment_config, ctx.model_entity)) return self.create_response - async def get_model_deployment_status(self, ctx: Any) -> DeploymentStatusUpdate: + async def get_model_deployment_status(self, ctx: ModelContext) -> DeploymentStatusUpdate: """Record call and return configured response.""" self.status_calls.append(ctx.model_deployment) return self.status_responseAs per coding guidelines,
**/*.py: Always prefer concrete type hints over string-based ones in Python code; do not import types under TYPE_CHECKING, instead import types as regular imports when possible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/inference-gateway/tests/integration/conftest.py` around lines 90 - 102, Replace the generic `Any` type hint with the concrete `ModelContext` type hint in the ctx parameter for all three mock backend methods: create_model_deployment, update_model_deployment, and get_model_deployment_status. This preserves the contract checks and follows the coding guideline to prefer concrete type hints over generic ones.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/resource_deleter.py`:
- Around line 39-47: The exception handling in the resource deletion logic does
not catch all possible failures from the Kubernetes client, allowing unhandled
exceptions to escape and violate the documented contract. Replace the specific
`except k8s_dynamic_exceptions.ForbiddenError as e:` handler with a general
`except Exception as e:` catch-all to ensure all exceptions (including any
unexpected failures) are passed to the _classify_delete_error method for proper
error classification and return, rather than propagating unhandled to abort
cleanup.
In
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/status_projector.py`:
- Around line 120-154: The error messages in build_pending_timeout_error and
build_crash_loop_error methods hardcode the string "NIM" in their status
messages, but since this projector is shared by both NIM and vLLM backends, vLLM
failures will display misleading diagnostics. Replace the hardcoded "NIM"
references with engine-neutral language or a dynamic reference that represents
the actual backend type being used, ensuring both NIM and vLLM deployments
display accurate status messages.
---
Outside diff comments:
In
`@services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.py`:
- Around line 360-365: The current code structure uses tuple unpacking where
both self._nim_reconciler.delete(workspace, name) and
self._k8s_reconciler.delete(workspace, name) are awaited together, which means
if the NIM reconciler delete raises an exception, the K8S reconciler delete will
never execute. Isolate each reconciler delete operation by executing them
separately with individual try-except blocks around each delete call, ensuring
that an exception from one reconciler does not prevent the other from attempting
to delete.
---
Nitpick comments:
In `@services/core/inference-gateway/tests/integration/conftest.py`:
- Around line 90-102: Replace the generic `Any` type hint with the concrete
`ModelContext` type hint in the ctx parameter for all three mock backend
methods: create_model_deployment, update_model_deployment, and
get_model_deployment_status. This preserves the contract checks and follows the
coding guideline to prefer concrete type hints over generic ones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b11748a1-b7a8-48d7-965e-d40528a8bb88
📒 Files selected for processing (17)
services/core/inference-gateway/tests/integration/conftest.pyservices/core/models/src/nmp/core/models/controllers/backends/backends.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/base.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/nim_operator.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/resource_deleter.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/status_projector.pyservices/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.pyservices/core/models/src/nmp/core/models/controllers/backends/none_backend.pyservices/core/models/src/nmp/core/models/controllers/deployment_reconciler.pyservices/core/models/tests/integration/conftest.pyservices/core/models/tests/unit/controllers/backends/test_vllm_k8s_compiler.pyservices/core/models/tests/unit/controllers/test_deployment_reconciler.pyservices/core/models/tests/unit/controllers/test_docker_backend.pyservices/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.py
🚧 Files skipped from review as they are similar to previous changes (4)
- services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/nim_operator.py
- services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/reconcilers/k8s.py
- services/core/models/src/nmp/core/models/controllers/backends/k8s_nim_operator/vllm_k8s_compiler.py
- services/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.py
mckornfield
reviewed
Jun 23, 2026
Signed-off-by: Ben McCown <bmccown@nvidia.com>
Fixes found validating the k8s vLLM path on a real cluster: - model-source and health-path are stored as annotations, not labels (their values contain '/', invalid for k8s label values). - puller Job runs the image entrypoint via container args, not command (command would override the entrypoint and exec the first token). - puller HF_ENDPOINT resolves the cluster-routable Files URL (service discovery / base_url), bypassing the in-process local-service routing that returns localhost (unreachable from the puller pod). - release the RWO weights volume before serving: delete the completed puller Job at P3 so its pod releases the volume attachment, avoiding a Multi-Attach error when the server pod schedules on another node. Only on the success path; a failed Job is kept for status/log reporting. - status reads the Deployment first (source of truth once created); a missing Job with the PVC still present resumes P3 instead of reporting LOST (prevents a re-pull loop via drift recovery). - pod securityContext: do not force runAsUser on the server pod (images like vLLM lack a passwd entry for uid 1000 -> getpwuid crash); the puller keeps fsGroup so it can write the freshly-provisioned PVC. - orphan listing tolerates NIM CRD 403 quietly on the vLLM-only path. Signed-off-by: Ben McCown <bmccown@nvidia.com>
Teardown deletes every resource a deployment could own, by name, across
both the operator path (NIMService/NIMCache CRs) and the directly-emitted
vLLM path (Deployment/Service/Job/PVC). delete_model_deployment has only
workspace/name (no engine/config -- it is also driven by orphan
reconciliation), so attempting all resource types by name is the correct,
idempotent, self-healing teardown.
Previously a single delete failure (notably a 403 deleting a NIMService
when the ServiceAccount lacked that RBAC on the vLLM-only path) raised
out of the routine and aborted the whole teardown, leaving the deployment
stuck in DELETING and re-erroring every reconcile.
Now each delete is independent and 404-tolerant ('already gone' is
success); non-404 failures are logged concisely (no stack trace) and
aggregated, so one resource's failure never blocks the others, and the
result is ERROR (visible/stuck) rather than a false DELETED that would
orphan cluster resources.
The platform Helm chart's models ServiceAccount must grant get/list/
watch/delete on apps.nvidia.com nimservices/nimcaches (in addition to the
core/apps/batch resources) so engine-agnostic teardown does not 403.
Signed-off-by: Ben McCown <bmccown@nvidia.com>
The vLLM puller + server pods previously inherited the NIM-oriented default securityContext (or none). Running the server as an arbitrary uid 1000 made torch/inductor crash at startup (getpass.getuser -> pwd.getpwuid: 'uid not found: 1000') because the vllm/vllm-openai image has no /etc/passwd entry for 1000, and running unset defaulted to root. Inspecting the image, its provisioned non-root user is 'vllm' (uid 2000, gid 0) with a real passwd entry. Add dedicated default_vllm_user_id (2000) / default_vllm_group_id (0) config and apply them to both the puller and the server: the server runs as a non-root user that resolves cleanly, and the puller writes the weights under the same uid/gid so the server can read them. Operators can override via config; we do not hardcode root. Signed-off-by: Ben McCown <bmccown@nvidia.com>
… raw-object migration When NIM is migrated off k8s-nim-operator onto the shared raw-object compilers (vllm_k8s_compiler), the securityContext uid/gid params are engine-specific and must not be shared: vLLM uses 2000/0 (its image's 'vllm' user, which has an /etc/passwd entry), while NIM images expect the operator's historical 1000/2000. Reusing default_vllm_user_id for NIM (or hardcoding either in the compiler) would reintroduce the getpwuid crash or break NIM. Add a FUTURE note in the compiler module docstring and cross-references on the NIM uid/gid config fields and the vLLM puller call site so a future implementer (possibly not us) does not hit this. Signed-off-by: Ben McCown <bmccown@nvidia.com>
Extract the Kubernetes reconciliation logic out of the monolithic K8sNimOperatorServiceBackend into a reconciler hierarchy under reconcilers/: * BaseReconciler (base.py): the reconciler interface plus the shared, engine-agnostic Kubernetes status helpers (pod log fetch, crash-loop detection, pod-status drill-down, pending-timeout/crash-loop error builders) and the idempotent 404-tolerant single-object delete. * NimOperatorReconciler (nim_operator.py): emits NIMService / NIMCache CRs for the in-cluster operator to reconcile. * K8sReconciler (k8s.py): emits native Kubernetes objects directly (PVC / weight-puller Job / Deployment / Service) for the vLLM engine and drives the staged rollout itself. The ServiceBackend now owns only the nemo_platform SDK and API-object work: it resolves all reconciler inputs into a ResolvedDeployment, selects the reconciler by config.engine, and delegates. Delete asks both reconcilers (no engine context) and aggregates; list unions both. This is behavior-neutral: the NIM operator path is moved verbatim and the vLLM path is unchanged. Tests are retargeted at the reconciler modules/instances; the shared status-helper unit tests now exercise BaseReconciler directly. Signed-off-by: Ben McCown <bmccown@nvidia.com>
Remove _validate_nim_operator_crds and its call site so the k8s backend no longer fails fast when the k8s-nim-operator CRDs are absent. A cluster without the operator can now run vLLM-only deployments (vLLM emits native Kubernetes objects and needs no operator); missing NIM CRDs surface lazily when a NIM deployment is actually created. Drop the associated test mocks. Signed-off-by: Ben McCown <bmccown@nvidia.com>
get_model_deployment_status now requires a config (engine is taken from it, same selection as create/update). When the controller can't supply a config this cycle (e.g. a transient config-fetch failure), the backend returns UNKNOWN rather than probing the cluster to guess the engine. The controller's existing _handle_unknown_status retries on the next poll (which normally has a config) and escalates to ERROR after its retry budget -- so we get the ultimate-timeout behavior for free. Removes the get_status_orphan entry point from all three reconcilers and the _vllm_objects_exist probe from the status path (the probe is still used internally by the vLLM update re-pull check). Signed-off-by: Ben McCown <bmccown@nvidia.com>
Drop the refactor-provenance note ("moved verbatim from the previous
monolithic K8sNimOperatorServiceBackend") from both reconciler module
docstrings. In k8s.py it read as if the file were the NIM-operator
implementation; replace with a concise statement of the actual contract
(inputs pre-resolved on ResolvedDeployment; reconciler talks only to
Kubernetes).
Signed-off-by: Ben McCown <bmccown@nvidia.com>
Run generate-config-docs to pick up the new K8sNimOperatorConfig vLLM fields (default_vllm_user_id/group_id, default_vllm_image/_tag, service_account_name, default_shared_memory_size_limit). Fixes the lint-config-reference-docs and lint-web-sdk CI jobs, which both diff regenerated artifacts against the tree. Signed-off-by: Ben McCown <bmccown@nvidia.com>
…ault The integration MockServiceBackend.get_model_deployment_status kept the old single-arg signature (deployment), but the controller now threads (deployment, config, model_entity) through. The resulting TypeError was swallowed by the reconciler's per-deployment except, so the status call was never recorded and test_controller_polls_pending_deployment failed with assert 0 == 1. Update both the models and inference-gateway integration mock backends to match the ServiceBackend ABC signature. Also qualify the k8s backend busybox_image default (busybox -> docker.io/library/busybox) so the LoRA cache-init container resolves on container runtimes that enforce fully-qualified image names (short names fail there); regenerate the config reference doc. Signed-off-by: Ben McCown <bmccown@nvidia.com>
Default the model-weights puller image to the platform nmp-api image
(resolved via get_qualified_image("nmp-api"), so registry/tag follow
platform config and remain overridable), replacing the bespoke
nds-v2-huggingface-cli image. The nmp-api image already ships the
huggingface_hub CLI, so the k8s weight-puller Job overrides the
container entrypoint to `hf` (command=["hf"]) and runs
`hf download <repo> --local-dir /model-store [...]`.
Regenerate the config reference doc for the new default.
Signed-off-by: Ben McCown <bmccown@nvidia.com>
Composition over inheritance for the k8s reconcilers (review #2/#3): * Extract StatusProjector (pod-status projection, crash-loop/pending-timeout error builders, host URL) and ResourceDeleter (idempotent 404-tolerant delete) as standalone collaborators. * Reconciler is now a pure interface (the 5 verbs); NimOperatorReconciler and K8sReconciler compose the projector + deleter instead of inheriting them. The backend builds both collaborators in init() and injects them. Thread the reconcile context through the backend interface (review #19): * create/update/get_model_deployment_status now take a single ctx: ModelContext instead of (deployment, config, model_entity); applied across the ServiceBackend ABC and the docker / none / k8s backends, the deployment reconciler call sites, and the test mocks. delete stays (workspace, name). Fixes + nits: * Harden NIMService status read against a null status/state (review #15): (nim_status.get("state") or "").lower() can no longer raise. * Convert nim_operator logging to structured extra={} (review #13); avoid the reserved LogRecord 'name' key (use resource_name / deployment_name). * Flatten the Files-service create/update branches into a guard-clause helper (review #14). * compile_puller_job: rename args -> container_args (review #17). * Reconciler nits: import the vllm_k8s_compiler module under its full name (review #7), reflow the P3 (a)/(b) comment (review #8), quote values in the model-source error (review #9), drop the _ = image_pull_secrets dance (review #12), name the event-message cap MAX_EVENT_MESSAGE_CHARS (review #6), and document the _select_reconciler None contract (review #16). Signed-off-by: Ben McCown <bmccown@nvidia.com>
The vLLM k8s Deployment stamped the resolved health path as the nmp.nvidia.com/health-path annotation on both the Deployment metadata and the pod template. Nothing ever read it back: the status path recomputes the health path via resolve_health_path(engine, view) each cycle, and the probes consume health_path directly. It was leftover write-only state from an earlier iteration where status recovered the path from the annotation. Remove the HEALTH_PATH_ANNOTATION constant and both write sites; the probes are unaffected. Drop the two test assertions on the annotation (the health_path -> probe behavior is already covered). Unrelated to the docker path's HEALTH_PATH_LABEL, which is genuinely read and is left untouched. Signed-off-by: Ben McCown <bmccown@nvidia.com>
delete_one() promised that any non-404 failure is returned as a short error string so the caller can aggregate it, but only ApiException / NotFoundError / ForbiddenError were caught. Any other failure (transport/connection error, dynamic API error, ...) would escape and abort the caller's per-resource delete loop partway -- leaving later resources unattempted and risking a false DELETED. Replace the ForbiddenError-specific handler with a catch-all except Exception so every non-404 failure is routed through _classify_delete_error and returned, not raised. Add unit tests for delete_one covering 404 success (typed + dynamic), 403 -> classified, and an unexpected exception -> classified (not raised). Signed-off-by: Ben McCown <bmccown@nvidia.com>
Signed-off-by: Ben McCown <bmccown@nvidia.com>
benmccown
force-pushed
the
vllm-k8s-support/bmccown
branch
from
June 24, 2026 22:11
bde648d to
d04a705
Compare
benmccown
enabled auto-merge
June 24, 2026 22:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
vLLM deployments on the Kubernetes backend (no operator) + reconciler refactor
Summary
Adds first-class vLLM support to the models-controller Kubernetes backend by
emitting native Kubernetes objects directly (PVC / weight-puller Job /
Deployment / Service) — no
k8s-nim-operatorrequired for the vLLM path. As partof this, the previously monolithic
K8sNimOperatorServiceBackendis split into asmall reconciler hierarchy so the operator path (NIM) and the direct-emission
path (vLLM) live in separate, focused units behind a common interface.
The NIM-on-operator path is preserved and behavior-neutral throughout.
Linear: AIRCORE-694
Motivation
The k8s backend previously only spoke
NIMService/NIMCacheCRs and delegatedall reconciliation to the in-cluster
k8s-nim-operator. To run open models viavLLM we need to stand up the serving stack ourselves. Rather than bolt vLLM logic
onto the operator backend, the engine paths are separated cleanly.
What's in this PR
vLLM on k8s (native objects, no operator)
vllm_k8s_compiler.py: engine-agnostic compilers for the PVC, weight-pullerJob, Deployment, and Service.
so the controller can gate on weight readiness).
to release its ReadWriteOnce volume, then emit the serving Deployment + Service
with ownerReferences so a later delete cascades the PVC/Service.
vllmuser (uid 2000 / gid 0) to avoid the torchgetpwuidcrash; the puller writes weights as the same uid/gid.(
apis/files/v2/hf), not the local-servicelocalhostURL the puller can't reach.(name/revision) changes; otherwise the Deployment is patched in place and the
owned PVC/Job survive.
when LoRA is enabled.
genericengine is explicitly rejected on the k8s backend (clear error).Reconciler refactor
reconcilers/package:base.py—BaseReconcilerABC, theResolvedDeploymentinput dataclass, andthe shared, engine-agnostic Kubernetes status helpers (pod-log fetch, crash-loop
detection, pod-status drill-down, pending-timeout/crash-loop error builders) +
the idempotent, 404-tolerant single-object delete.
nim_operator.py—NimOperatorReconciler: emitsNIMService/NIMCacheCRsand projects status from the operator-reported
NIMService.status.k8s.py—K8sReconciler: the direct-emission vLLM reconciler described above.K8sNimOperatorServiceBackendis now a thin coordinator: it owns thenemo_platformSDK and API-object work, resolves everything a reconciler needsinto a
ResolvedDeployment, selects the reconciler byconfig.engine, anddelegates. Delete asks both reconcilers (delete has no engine context) and
aggregates results; list unions both paths for orphan reconciliation.
Behavior changes worth calling out
fast when the
k8s-nim-operatorCRDs are absent, so a cluster without theoperator can run vLLM-only deployments. Missing NIM CRDs now surface lazily only
if a NIM deployment is actually created (delete/list tolerate their absence).
get_model_deployment_statustakes the engine fromthe config (same selection as create/update). When the controller can't supply a
config for a cycle (e.g. a transient config-fetch failure), the backend returns
UNKNOWNinstead of probing the cluster to guess the engine; the controllerretries on the next poll and escalates to
ERRORafter its existing retry budget.Testing
services/core/modelsunit suite: passing (added coverage for the vLLMcompiler, the k8s backend phases/teardown/list/delete aggregation, the
config-less
UNKNOWNpath, and the new config fields).ruff(lint + format),ty, andpre-commitall pass on the changed files.deployment pulled weights, reached READY, and served inference (non-streaming and
streaming) through the inference gateway.
Review guide
This PR does two things at once: it adds the vLLM-on-k8s path and refactors
the k8s backend into reconcilers. Reading the files in dependency order (shared
pieces → reconcilers → coordinator → tests) makes both much easier to follow than
reading the raw diff top-to-bottom.
Suggested reading order
1. Engine plumbing (shared, small) — start here
controllers/backends/engine.py(+55) — engine discriminants (nim/vllm/generic),config_engine(), andresolve_health_path(). This is thevocabulary the rest of the PR dispatches on.
controllers/backends/vllm_compiler.py(moved fromdocker/, ~178 lines) — thebackend-agnostic vLLM compiler (image,
vllm serveargs, env). Relocated so boththe docker and k8s backends share it; the diff is mostly the move + import fixups.
Compare with
docker/creation_reconciler.pyto confirm the docker path isunchanged behaviorally.
2. The vLLM k8s object compiler
controllers/backends/k8s_nim_operator/vllm_k8s_compiler.py(new, 428) — purefunctions that build the PVC, weight-puller Job, Deployment, Service. No
control flow, no API calls — easiest place to verify the actual k8s objects
(volumes, securityContext uid/gid,
/dev/shm, probes, ownerRefs, labels).Paired tests:
tests/unit/controllers/backends/test_vllm_k8s_compiler.py(+307).3. The reconciler split (the heart of the refactor)
Read base → the two implementations → the coordinator.
.../reconcilers/base.py(new, 448) —BaseReconcilerABC, theResolvedDeploymentinput dataclass (what the coordinator pre-resolves andhands down), and the shared engine-agnostic status helpers (pod-log fetch,
crash-loop detection, pod-status drill-down, pending-timeout/crash-loop error
builders) + the idempotent, 404-tolerant single-object delete.
.../reconcilers/nim_operator.py(new, 466) —NimOperatorReconciler. This isthe previous operator logic, moved verbatim (NIMService/NIMCache CRUD, status
from
NIMService.status). The cheapest way to review: diff its method bodiesagainst the old
backend.pyand confirm they're unchanged apart from taking aResolvedDeployment..../reconcilers/k8s.py(new, 600) —K8sReconciler, the new vLLM logic and themost important file to review carefully (see "Where to focus" below).
.../k8s_nim_operator/backend.py(919 deletions, now 377) — the coordinator. Thebig deletion is code moving into the reconcilers. New responsibilities:
_resolve()(buildResolvedDeployment),_select_reconciler(engine),delegation in create/update/status, delete that calls both reconcilers and
aggregates, and list that unions both paths.
4. Config + tests
.../k8s_nim_operator/config.py(+57) — new vLLM-related fields (image/tag,service account, uid/gid defaults,
/dev/shmsize limit). Tests:tests/unit/controllers/test_backend_config_fields.py.tests/unit/controllers/test_k8s_nim_operator_backend.py(+681) — the bulk of thetest churn. Note the helpers near the top:
_sync_reconcilers()(propagates theper-test mock clients onto the reconcilers the backend built at init),
_StatusHelperReconciler(exercises theBaseReconcilerstatus helpers directly),and
_nim_config()/_vllm_config().Where to focus your attention (the subtle bits)
These are the parts most worth scrutiny — they encode hard-won operational behavior:
The staged P0 → P3 rollout in
k8s.py(create+get_status+_create_vllm_serving_objects). Create emits only the PVC + puller Job;the serving Deployment/Service are created later from the status path once the
puller Job succeeds. This is intentional weight-readiness gating.
RWO volume hand-off (
_delete_puller_job+_create_vllm_serving_objects).The completed puller Job is deleted before the serving Deployment is created so
its pod releases the ReadWriteOnce volume (avoids a Multi-Attach error if the
server schedules on another node). Note the "still terminating → defer" return,
and the
Job-absent + PVC-present → resume P3 (not LOST)branch inget_status.ownerReferences— PVC and Service are owned by the Deployment so a singleDeployment delete cascades the rest.
Update re-pull policy (
update+_existing_model_source) — weights re-pullonly when the model-source annotation changes; unchanged source patches in place.
Engine-agnostic teardown (
backend.delete_model_deployment→both reconcilers → aggregate). Delete has no engine context (also used for orphan
reconciliation), so it attempts both paths, is 404-tolerant per-resource, and
surfaces real failures as ERROR rather than falsely reporting DELETED. The
forbidden-CR-doesn't-block-vLLM-cleanup case is covered by tests.
uid/gid 2000/0 for vLLM pods + puller (
config.pydefaults, used invllm_k8s_compiler.py). Avoids the torchgetpwuidcrash; there's a FUTURE notein the compiler flagging that a NIM raw-object path must pass its own uid/gid.
Behavior changes to sanity-check (not just refactor)
_validate_nim_operator_crdsis gone — a clusterwithout
k8s-nim-operatorcan run vLLM-only. Confirm you're comfortable thatmissing NIM CRDs now surface lazily (on NIM create) rather than at boot.
UNKNOWN.get_model_deployment_statusno longerprobes the cluster to guess the engine when
config is None; it returnsUNKNOWNand lets the controller retry/escalate. See the short branch at the top of that
method and
deployment_reconciler.py's existing_handle_unknown_status.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation