feat(deployments): K8s DeploymentBackend scaffold (AIRCORE-757 phase 1) - #533
Merged
Conversation
Register K8sDeploymentBackend with executor config, per-instance Kubernetes clients, and shared identity labels. ABC methods stub NotImplementedError until later phases implement PVC, Job, and Deployment+Service paths. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a Kubernetes deployment backend ( ChangesKubernetes backend addition
Sequence Diagram(s)sequenceDiagram
participant Registry as ExecutorRegistry
participant Backend as K8sDeploymentBackend
participant Client as KubernetesClients
participant K8sAPI as kubernetes.client
Registry->>Backend: resolve("k8s") -> init()
Backend->>Backend: K8sExecutorConfig.model_validate(config)
Backend->>Client: KubernetesClients(kubeconfig_path, request_timeout)
Client->>K8sAPI: build_api_client()
K8sAPI-->>Client: ApiClient
Backend-->>Registry: backend instance ready
Registry->>Backend: shutdown()
Backend->>Client: close()
Client->>K8sAPI: ApiClient.close()
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (6)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py (1)
80-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInaccurate return type hint on
managed_by_filter.Declared as
dict[str, str | bool]but the implementation only ever returns astrvalue. Narrow the hint todict[str, str]unless a bool value is genuinely expected elsewhere.♻️ Proposed fix
-def managed_by_filter() -> dict[str, str | bool]: +def managed_by_filter() -> dict[str, str]: return {"label": f"{MANAGED_BY_KEY}={MANAGED_BY_LABEL}"}🤖 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/labels.py` around lines 80 - 81, The return type on managed_by_filter is too broad because it only returns a string-valued dict entry today. Update the type hint in managed_by_filter to use dict[str, str] and keep the implementation unchanged unless there is a real caller that needs a bool value.plugins/nemo-deployments/tests/unit/backends/k8s/test_k8s_registry.py (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
from __future__ import annotationsmakes hints string-based.Conflicts with the repo guideline to prefer concrete type hints over string-based ones.
As per coding guidelines, "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 `@plugins/nemo-deployments/tests/unit/backends/k8s/test_k8s_registry.py` at line 4, The test module is importing future annotations, which forces string-based type hints and conflicts with the repo guideline to use concrete types. Remove the from __future__ import annotations import from the k8s registry test module, and update any affected annotations in the nearby test code to use direct, concrete imports or runtime-resolvable types instead of postponed/stringified hints.Source: Coding guidelines
plugins/nemo-deployments/tests/unit/backends/k8s/conftest.py (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
from __future__ import annotationsmakes hints string-based.Same concern as other new test files in this cohort.
As per coding guidelines, "Always prefer concrete type hints over string-based ones in Python code."
🤖 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/tests/unit/backends/k8s/conftest.py` at line 6, The test module is enabling string-based annotations via from __future__ import annotations, which conflicts with the coding guideline to prefer concrete type hints. Remove that future import from this conftest module and update any affected annotations in the related test setup code to use real type references so symbols like conftest.py remain consistent with the rest of the cohort.Source: Coding guidelines
plugins/nemo-deployments/tests/unit/backends/k8s/test_backend.py (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
from __future__ import annotationsmakes hints string-based.Same concern as other new test files in this cohort.
As per coding guidelines, "Always prefer concrete type hints over string-based ones in Python code."
🤖 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/tests/unit/backends/k8s/test_backend.py` at line 4, The new test module is importing future annotations, which causes type hints to be stored as strings instead of concrete types. Remove the from __future__ import annotations line from this test file and keep the existing annotations expressed with concrete Python types, following the same pattern used in the other new test modules and any nearby test helpers.Source: Coding guidelines
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/client.py (1)
35-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo cleanup path for the lazily-created
ApiClient.
KubernetesClientsnever exposes a way to release the underlyingApiClient's connection pool. Once later phases start invokingcore_v1/apps_v1/batch_v1, backend shutdown should close these to avoid leaking connections.♻️ Suggested addition
`@property` def batch_v1(self) -> client.BatchV1Api: if self._batch_v1 is None: from kubernetes import client self._batch_v1 = client.BatchV1Api(self._api()) return self._batch_v1 + + def close(self) -> None: + """Release the underlying ApiClient's connection pool, if created.""" + if self._api_client is not None: + self._api_client.close() + self._api_client = None + self._core_v1 = self._apps_v1 = self._batch_v1 = None🤖 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/client.py` around lines 35 - 83, KubernetesClients lazily creates a shared ApiClient in _api() but never provides a cleanup path, so its connection pool can remain open after use. Add a close/shutdown method on KubernetesClients that releases the underlying _api_client and resets the cached client fields, and make sure backend shutdown calls it after core_v1, apps_v1, or batch_v1 usage. Keep the cleanup centered around the existing _api(), _api_client, and API property caches so the lifecycle is explicit and safe.plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py (1)
49-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
shutdown()doesn't release the Kubernetes API client's connection pool.Once
_clientsis used to create real API objects, this no-op leaks connections. Wire it to theclose()method suggested inclient.py.♻️ Suggested fix (depends on adding `KubernetesClients.close()`)
def shutdown(self) -> None: - pass + self._clients.close()🤖 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/backend.py` around lines 49 - 50, The backend shutdown path is a no-op, so the Kubernetes API client connection pool is never released once _clients has been used. Update the Backend.shutdown() method to delegate to the client cleanup logic by calling the new KubernetesClients.close() method from client.py, and ensure the Backend class uses that closeable client instance during teardown.
🤖 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/tests/unit/backends/k8s/test_client.py`:
- Around line 66-76: The fallback test in
test_build_api_client_falls_back_to_kubeconfig is tautological because it
derives the expected client_configuration from mock_kube.call_args itself.
Update the test to assert that load_kube_config in build_api_client receives the
mocked kubernetes.client.Configuration() return value (from the Configuration
patch) rather than reading back the call args, so the test actually verifies the
passed instance.
---
Nitpick comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py`:
- Around line 49-50: The backend shutdown path is a no-op, so the Kubernetes API
client connection pool is never released once _clients has been used. Update the
Backend.shutdown() method to delegate to the client cleanup logic by calling the
new KubernetesClients.close() method from client.py, and ensure the Backend
class uses that closeable client instance during teardown.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/client.py`:
- Around line 35-83: KubernetesClients lazily creates a shared ApiClient in
_api() but never provides a cleanup path, so its connection pool can remain open
after use. Add a close/shutdown method on KubernetesClients that releases the
underlying _api_client and resets the cached client fields, and make sure
backend shutdown calls it after core_v1, apps_v1, or batch_v1 usage. Keep the
cleanup centered around the existing _api(), _api_client, and API property
caches so the lifecycle is explicit and safe.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py`:
- Around line 80-81: The return type on managed_by_filter is too broad because
it only returns a string-valued dict entry today. Update the type hint in
managed_by_filter to use dict[str, str] and keep the implementation unchanged
unless there is a real caller that needs a bool value.
In `@plugins/nemo-deployments/tests/unit/backends/k8s/conftest.py`:
- Line 6: The test module is enabling string-based annotations via from
__future__ import annotations, which conflicts with the coding guideline to
prefer concrete type hints. Remove that future import from this conftest module
and update any affected annotations in the related test setup code to use real
type references so symbols like conftest.py remain consistent with the rest of
the cohort.
In `@plugins/nemo-deployments/tests/unit/backends/k8s/test_backend.py`:
- Line 4: The new test module is importing future annotations, which causes type
hints to be stored as strings instead of concrete types. Remove the from
__future__ import annotations line from this test file and keep the existing
annotations expressed with concrete Python types, following the same pattern
used in the other new test modules and any nearby test helpers.
In `@plugins/nemo-deployments/tests/unit/backends/k8s/test_k8s_registry.py`:
- Line 4: The test module is importing future annotations, which forces
string-based type hints and conflicts with the repo guideline to use concrete
types. Remove the from __future__ import annotations import from the k8s
registry test module, and update any affected annotations in the nearby test
code to use direct, concrete imports or runtime-resolvable types instead of
postponed/stringified hints.
🪄 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: bf79059c-5615-4e0e-ab83-0ed967e86bcf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
plugins/nemo-deployments/pyproject.tomlplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/labels.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/client.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/registry.pyplugins/nemo-deployments/tests/unit/backends/k8s/conftest.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_backend.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_client.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_k8s_registry.py
Contributor
|
Centralize kubernetes imports in _kubernetes_modules() so optional-dep loading stays lazy without repeated inline imports. Add KubernetesClients.close() and wire K8sDeploymentBackend.shutdown(). Fix tautological kubeconfig fallback test and narrow managed_by_filter return type. Signed-off-by: Tyler Bray <tbray@nvidia.com>
mckornfield
approved these changes
Jul 1, 2026
Drop docker/labels.py re-export shim; import shared backends.labels directly. Remove per-instance kubeconfig busywork test. Validate default_namespace as a DNS-1123 label. Document label prefix/workspace-key and ApiClient close behavior. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py (1)
37-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming:
k8s_*helpers used for Docker resource naming.
docker_volume_name/container_namenow delegate tok8s_deployment_resource_name/k8s_volume_resource_name. Prefix (dep-/dep-vol-) and hash input (deployment_key) are unchanged, so generated Docker names stay stable — no breaking rename. Naming these helpersk8s_*while they're also the canonical Docker naming path is a bit confusing since this is a shared module now.Consider a backend-neutral name (e.g.
dns1123_resource_name) since it's used by both backends.🤖 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/labels.py` around lines 37 - 58, The naming helpers in labels.py are backend-neutral, but the current k8s_* function names make them look Kubernetes-only even though docker_volume_name and the container naming path also rely on them. Rename the shared helpers to a backend-agnostic symbol such as dns1123_resource_name (and update docker_volume_name, k8s_deployment_resource_name, and k8s_volume_resource_name call sites accordingly) so the shared naming logic is clearer without changing the generated names.
🤖 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 `@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py`:
- Around line 37-58: The naming helpers in labels.py are backend-neutral, but
the current k8s_* function names make them look Kubernetes-only even though
docker_volume_name and the container naming path also rely on them. Rename the
shared helpers to a backend-agnostic symbol such as dns1123_resource_name (and
update docker_volume_name, k8s_deployment_resource_name, and
k8s_volume_resource_name call sites accordingly) so the shared naming logic is
clearer without changing the generated names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 925fc156-0f32-47d8-8c80-17dbea4723b6
📒 Files selected for processing (17)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/containers.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/gpu.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/ports.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/volumes.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/client.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.pyplugins/nemo-deployments/tests/integration/backends/docker/test_docker_backend.pyplugins/nemo-deployments/tests/integration/test_reconcile_docker.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_gpu.pyplugins/nemo-deployments/tests/unit/backends/docker/test_idempotency.pyplugins/nemo-deployments/tests/unit/backends/docker/test_labels.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_backend.pyplugins/nemo-deployments/tests/unit/backends/k8s/test_client.py
💤 Files with no reviewable changes (1)
- plugins/nemo-deployments/tests/unit/backends/k8s/test_client.py
✅ Files skipped from review due to trivial changes (5)
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/containers.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/ports.py
- plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/volumes.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.py
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/config.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/client.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py
This was referenced Jul 2, 2026
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…1) (#533) * feat(deployments): scaffold K8s DeploymentBackend (AIRCORE-757 phase 1) Register K8sDeploymentBackend with executor config, per-instance Kubernetes clients, and shared identity labels. ABC methods stub NotImplementedError until later phases implement PVC, Job, and Deployment+Service paths. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address PR 533 review feedback for k8s client Centralize kubernetes imports in _kubernetes_modules() so optional-dep loading stays lazy without repeated inline imports. Add KubernetesClients.close() and wire K8sDeploymentBackend.shutdown(). Fix tautological kubeconfig fallback test and narrow managed_by_filter return type. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address mckornfield PR 533 review comments Drop docker/labels.py re-export shim; import shared backends.labels directly. Remove per-instance kubeconfig busywork test. Validate default_namespace as a DNS-1123 label. Document label prefix/workspace-key and ApiClient close behavior. Signed-off-by: Tyler Bray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com>
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.
Summary
K8sDeploymentBackendinBACKEND_CLASSESwithK8sExecutorConfig(kubeconfig path, default namespace, request timeout).KubernetesClients(CoreV1Api,AppsV1Api,BatchV1Api) with optionalkubernetesextra.backends/labels.py; docker re-exports for compatibility.NotImplementedError— PVC/Job/Deployment paths land in phases 2–4.Design notes (phase 1)
batch/v1.Job(not bare Pod) — documented on Linear ticket.KubernetesClients; API calls will pass_request_timeoutin phase 2+.Test plan
uv run pytest plugins/nemo-deployments/tests/unit -v(175 passed)Linear
AIRCORE-757 — Phase 1 of 7
Summary by CodeRabbit
New Features
Bug Fixes
Chores