feat(deployments): k8s PodSpec compiler for full workload config (AIRCORE-757 phase 5) - #553
Conversation
…(AIRCORE-757 phase 5) Extract shared PodSpec compilation for init containers, native sidecars, multi-container deployments, ConfigMap file mounts, and K8sDeploymentConfig. Wire ConfigMap lifecycle with ownership checks and rollback on partial creates. Signed-off-by: Tyler Bray <tbray@nvidia.com>
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds shared k8s workload compilation and ConfigMap lifecycle helpers, updates Deployment and Job flows to use compiled multi-container workloads, adds container restart policy schema support, and changes status reads to use all containers. ChangesK8s workload compiler and multi-container support
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py (1)
67-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak types on
build_deployment_body.k8s_config: Anyand-> tuple[Any, Any]lose type safety;jobs.py.build_job_bodyalready usesK8sDeploymentConfig | None. Tighten for consistency.Proposed change
-from nemo_deployments_plugin.entities import Container, DeploymentConfig +from nemo_deployments_plugin.entities import Container, DeploymentConfig, K8sDeploymentConfigworkspace: str, deployment_name: str, - k8s_config: Any, -) -> tuple[Any, Any]: + k8s_config: K8sDeploymentConfig | None, +) -> tuple[Any, CompiledWorkload]:
CompiledWorkloadis exported fromcompiler; add it to the existing compiler import.🤖 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 `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py` around lines 67 - 69, The `build_deployment_body` signature is too weakly typed, with `k8s_config: Any` and `-> tuple[Any, Any]`, so update it to match the stronger typing used in `jobs.py.build_job_body`. Change the function in `build_deployment_body` to accept `K8sDeploymentConfig | None` and return the appropriate typed tuple, then add the exported `CompiledWorkload` to the existing compiler import in this module so the stronger annotations resolve correctly.
🤖 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
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/compiler.py`:
- Around line 369-375: The init container build path in compiler.py is dropping
probes for every entry in init_containers because build_container is always
called with include_probes=False. Update the logic around
_ordered_init_containers and build_container so probes are excluded only for
true init containers, while native sidecars with restartPolicy=Always retain
liveness/readiness probes by gating include_probes on the container restart
policy.
---
Nitpick comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py`:
- Around line 67-69: The `build_deployment_body` signature is too weakly typed,
with `k8s_config: Any` and `-> tuple[Any, Any]`, so update it to match the
stronger typing used in `jobs.py.build_job_body`. Change the function in
`build_deployment_body` to accept `K8sDeploymentConfig | None` and return the
appropriate typed tuple, then add the exported `CompiledWorkload` to the
existing compiler import in this module so the stronger annotations resolve
correctly.
🪄 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: 3c8229d8-2507-4131-96c2-89cb48a49715
📒 Files selected for processing (9)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/compiler.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/src/nemo_deployments_plugin/entities.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_compiler.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_deployments.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_k8s_status_mapping.py
Retain liveness/readiness probes on native sidecar init containers, tighten build_deployment_body typing, and regenerate deployments OpenAPI for Container.restart_policy. Signed-off-by: Tyler Bray <tbray@nvidia.com>
mckornfield
left a comment
There was a problem hiding this comment.
have some style things, but I think you can ship it assuming you address em
Use BuiltJob/BuiltDeployment dataclasses instead of tuple returns, SimpleNamespace for k8s deserialization, _reraise_api_unless helper, clearer validation wording, and structural pod_spec golden asserts. Signed-off-by: Tyler Bray <tbray@nvidia.com>
…CORE-757 phase 5) (#553) * feat(deployments): add k8s PodSpec compiler for full workload config (AIRCORE-757 phase 5) Extract shared PodSpec compilation for init containers, native sidecars, multi-container deployments, ConfigMap file mounts, and K8sDeploymentConfig. Wire ConfigMap lifecycle with ownership checks and rollback on partial creates. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address CodeRabbit review on phase 5 PodSpec compiler Retain liveness/readiness probes on native sidecar init containers, tighten build_deployment_body typing, and regenerate deployments OpenAPI for Container.restart_policy. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address mckornfield review on phase 5 compiler Use BuiltJob/BuiltDeployment dataclasses instead of tuple returns, SimpleNamespace for k8s deserialization, _reraise_api_unless helper, clearer validation wording, and structural pod_spec golden asserts. Signed-off-by: Tyler Bray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com>
Summary
compiler.pyto compileDeploymentConfig+K8sDeploymentConfiginto Kubernetes PodSpec objects: init containers, native sidecars (restartPolicy: Alwayson init, k8s ≥1.29), multi-container workloads, ConfigMap file mounts, tolerations, affinity, security context, and service account.jobs.pyanddeployments.pyto use the shared compiler; wire ConfigMap create/delete lifecycle with ownership checks on 409 conflicts and rollback on partial creates.Container.restart_policyfor native sidecar init containers; extend validation for duplicate port names and duplicate listen ports across containers.Test plan
uv run --frozen pytest plugins/nemo-deployments/tests/unit— 251 passeduv run ruff check plugins/nemo-deployments/uv run --frozen ty check plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/test_compiler.pySummary by CodeRabbit
restartPolicysupport on container definitions for init containers, exposed in the API schema.Alwaysrestart behavior.