Repository navigation
test(deployments): add kind/kubeconfig integration tests for k8s backend (AIRCORE-757 phase 7) - #581
Merged
Merged
Conversation
Contributor
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds Kubernetes cluster-gated integration tests for ChangesKubernetes integration test suite
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🧹 Nitpick comments (4)
plugins/nemo-deployments/README.md (1)
60-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a prerequisites block before the commands.
This section jumps straight into execution steps; list
kind,kubectl,uv, and cluster access/kubeconfig requirements first, then the kind/dev-blue variants. As per coding guidelines, prerequisites should appear at the top of markdown docs.🤖 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/README.md` around lines 60 - 96, The Cluster validation section jumps straight into run commands, but it needs a prerequisites block first. Update the README content around the integration test instructions to add the required tools and setup at the top: kind, kubectl, uv, and valid cluster access/kubeconfig. Then keep the existing Local run against kind and Run against dev-blue subsections beneath that prerequisites section.Source: Coding guidelines
plugins/nemo-deployments/tests/integration/test_reconcile_k8s.py (2)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
from __future__ import annotationsdefers annotations to strings.As per coding guidelines,
**/*.pyshould prefer concrete type hints over string-based ones. This future import makes all annotations in this module deferred/stringified at runtime, which cuts against that guideline. If this matches an existing repo-wide convention elsewhere in the plugin, feel free to disregard.🤖 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/integration/test_reconcile_k8s.py` at line 16, The module-level future import in test_reconcile_k8s.py is causing annotations to be deferred/stringified, which conflicts with the preference for concrete type hints. Remove the from __future__ import annotations line and, if needed, update any affected annotations in the test module to use regular runtime-resolved types so the test file follows the repo’s typing convention.Source: Coding guidelines
92-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent fallback masks name typos.
return puller_dep if name == "puller" else server_dep— any unexpectedname(e.g. a typo) silently resolves toserver_depinstead of failing loudly.🔧 Proposed tightening
if entity_type is Deployment: - return puller_dep if name == "puller" else server_dep + if name == "puller": + return puller_dep + if name == "server": + return server_dep + raise KeyError(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 `@plugins/nemo-deployments/tests/integration/test_reconcile_k8s.py` around lines 92 - 102, The get_side_effect helper in the test currently falls back to server_dep for any Deployment name other than "puller", which can hide typos and make the test pass incorrectly. Update the Deployment branch in get_side_effect to explicitly handle only the expected names ("puller" and "server") and raise an error for any unexpected name so failures are loud and obvious; keep the DeploymentConfig lookup unchanged.plugins/nemo-deployments/tests/integration/kubeconfig_availability.py (1)
10-20: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winImport-time cluster probe runs for every integration collection, not just k8s tests.
This module does a real network call (
get_code(_request_timeout=5)) as a side effect of import. Sinceconftest.pyimports this unconditionally to populate__all__, every integration test session — including docker-only runs — pays this latency/timeout cost even when no k8s test is selected.Consider deferring the check (e.g., compute lazily behind a cached function/fixture, or only import this module from the k8s-specific test files rather than the shared
conftest.py).🤖 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/integration/kubeconfig_availability.py` around lines 10 - 20, The kubeconfig availability probe in KUBECONFIG_AVAILABLE is running a real cluster call at import time, which impacts every integration collection. Move the VersionApi().get_code(_request_timeout=5) check behind a lazy, cached helper or fixture instead of executing it on module import, and update the shared conftest.py/import path so only k8s-specific tests trigger the probe.
🤖 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/integration/backends/k8s/test_k8s_backend.py`:
- Around line 123-134: The _wait_for_status helper in test_k8s_backend should
fail explicitly instead of silently exhausting retries; if
k8s_backend.read_status never returns a status in terminal_statuses after the
loop in _wait_for_status, raise an assertion or timeout error that includes the
name and number of attempts. Keep the existing polling logic, but add a clear
timeout path so the calling tests get an immediate, descriptive failure rather
than a later ambiguous assertion.
- Around line 138-155: The cleanup in test_pvc_lifecycle is masking failures
because the assert in the finally block can replace the original assertion error
from created.status or read.status. Update the test to avoid asserting inside
the cleanup path: keep the delete_volume call in finally for teardown, but do
any status verification outside cleanup or guard it so cleanup errors do not
override the primary failure. Use test_pvc_lifecycle, create_volume,
read_volume_status, and delete_volume to locate the fix.
In `@plugins/nemo-deployments/tests/integration/test_reconcile_k8s.py`:
- Around line 135-137: The cleanup in the test's finally block only deletes
deployments sequentially, so a failure in backend.delete_deployment("itest",
"puller") prevents backend.delete_deployment("itest", "server") from running.
Update the cleanup around the backend.delete_deployment calls to isolate
failures per resource (for example by wrapping each deletion in its own
exception handling or using a safer teardown pattern) so both the puller and
server cleanup always get a chance to run.
---
Nitpick comments:
In `@plugins/nemo-deployments/README.md`:
- Around line 60-96: The Cluster validation section jumps straight into run
commands, but it needs a prerequisites block first. Update the README content
around the integration test instructions to add the required tools and setup at
the top: kind, kubectl, uv, and valid cluster access/kubeconfig. Then keep the
existing Local run against kind and Run against dev-blue subsections beneath
that prerequisites section.
In `@plugins/nemo-deployments/tests/integration/kubeconfig_availability.py`:
- Around line 10-20: The kubeconfig availability probe in KUBECONFIG_AVAILABLE
is running a real cluster call at import time, which impacts every integration
collection. Move the VersionApi().get_code(_request_timeout=5) check behind a
lazy, cached helper or fixture instead of executing it on module import, and
update the shared conftest.py/import path so only k8s-specific tests trigger the
probe.
In `@plugins/nemo-deployments/tests/integration/test_reconcile_k8s.py`:
- Line 16: The module-level future import in test_reconcile_k8s.py is causing
annotations to be deferred/stringified, which conflicts with the preference for
concrete type hints. Remove the from __future__ import annotations line and, if
needed, update any affected annotations in the test module to use regular
runtime-resolved types so the test file follows the repo’s typing convention.
- Around line 92-102: The get_side_effect helper in the test currently falls
back to server_dep for any Deployment name other than "puller", which can hide
typos and make the test pass incorrectly. Update the Deployment branch in
get_side_effect to explicitly handle only the expected names ("puller" and
"server") and raise an error for any unexpected name so failures are loud and
obvious; keep the DeploymentConfig lookup unchanged.
🪄 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: 2c3c1ab1-ebdd-4fc3-8b7b-dc3f1686383d
📒 Files selected for processing (5)
plugins/nemo-deployments/README.mdplugins/nemo-deployments/tests/integration/backends/k8s/test_k8s_backend.pyplugins/nemo-deployments/tests/integration/conftest.pyplugins/nemo-deployments/tests/integration/kubeconfig_availability.pyplugins/nemo-deployments/tests/integration/test_reconcile_k8s.py
Contributor
|
tylersbray
added a commit
that referenced
this pull request
Jul 6, 2026
Fixes 3 actionable review comments on #581: - _wait_for_status now fails explicitly with an attempt count instead of silently returning on timeout, so callers get a clear message instead of a later ambiguous assertion failure. - test_pvc_lifecycle no longer asserts inside its finally block, so a cleanup-path failure can't mask the real assertion failure from the try block. - test_reconcile_k8s's teardown deletes puller and server concurrently via asyncio.gather(return_exceptions=True) so a failure deleting one doesn't leak the other. Also addresses 2 nitpicks: - README: added a prerequisites block ahead of the kind/dev-blue run commands in the new Cluster validation section. - test_reconcile_k8s's get_side_effect now raises KeyError for any Deployment name other than "puller"/"server" instead of silently falling back to server_dep, so a typo fails loudly. Left 2 nitpicks as-is (noted in the PR): the `from __future__ import annotations` import and the import-time cluster/daemon availability probe both match the pre-existing docker_availability.py / test_reconcile_docker.py convention already used throughout this test suite; fixing only the new k8s files would create inconsistency, and a broader refactor of the shared pattern is out of scope here. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Contributor
Author
|
Addressed CodeRabbit's feedback in 72a53c5: Actionable comments (all fixed):
Nitpicks (fixed):
Nitpicks (left as-is, noted for reviewers):
Verified after the fix: 251 unit tests + all 10 integration tests (5 docker + 5 k8s) pass locally against a |
…end (AIRCORE-757 phase 7) Mirror the existing Docker integration-test layout for the k8s deployments backend: PVC lifecycle, Job success, ConfigMap mount round-trip, Deployment+Service readiness, and label-gated delete rejection, plus a reconciler-level prerequisite-chain test. All tests skip cleanly when no kubeconfig/cluster is reachable, matching the Docker suite's CI behavior. Branched from origin/main ahead of Phase 6 (#575) landing, since these tests instantiate K8sDeploymentBackend directly with the developer's own kubeconfig rather than the restricted nmp-core ServiceAccount that Phase 6's RBAC changes target -- no functional dependency between the two. Will rebase onto main once #575 merges. Signed-off-by: Tyler Bray <tbray@nvidia.com>
Fixes 3 actionable review comments on #581: - _wait_for_status now fails explicitly with an attempt count instead of silently returning on timeout, so callers get a clear message instead of a later ambiguous assertion failure. - test_pvc_lifecycle no longer asserts inside its finally block, so a cleanup-path failure can't mask the real assertion failure from the try block. - test_reconcile_k8s's teardown deletes puller and server concurrently via asyncio.gather(return_exceptions=True) so a failure deleting one doesn't leak the other. Also addresses 2 nitpicks: - README: added a prerequisites block ahead of the kind/dev-blue run commands in the new Cluster validation section. - test_reconcile_k8s's get_side_effect now raises KeyError for any Deployment name other than "puller"/"server" instead of silently falling back to server_dep, so a typo fails loudly. Left 2 nitpicks as-is (noted in the PR): the `from __future__ import annotations` import and the import-time cluster/daemon availability probe both match the pre-existing docker_availability.py / test_reconcile_docker.py convention already used throughout this test suite; fixing only the new k8s files would create inconsistency, and a broader refactor of the shared pattern is out of scope here. Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
757-k8s-phase7-kind-it/tbray
branch
from
July 7, 2026 18:51
72a53c5 to
91bd3ab
Compare
mckornfield
approved these changes
Jul 7, 2026
mckornfield
left a comment
mckornfield
left a comment
Contributor
There was a problem hiding this comment.
some funny wording things, yay integration and not e2e tests.
Addresses mckornfield's PR #581 nitpick that "ambient" reads oddly out of context. Signed-off-by: tbray <tbray@nvidia.com>
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…end (AIRCORE-757 phase 7) (#581) * test(deployments): add kind/kubeconfig integration tests for k8s backend (AIRCORE-757 phase 7) Mirror the existing Docker integration-test layout for the k8s deployments backend: PVC lifecycle, Job success, ConfigMap mount round-trip, Deployment+Service readiness, and label-gated delete rejection, plus a reconciler-level prerequisite-chain test. All tests skip cleanly when no kubeconfig/cluster is reachable, matching the Docker suite's CI behavior. Branched from origin/main ahead of Phase 6 (#575) landing, since these tests instantiate K8sDeploymentBackend directly with the developer's own kubeconfig rather than the restricted nmp-core ServiceAccount that Phase 6's RBAC changes target -- no functional dependency between the two. Will rebase onto main once #575 merges. Signed-off-by: Tyler Bray <tbray@nvidia.com> * test(deployments): address CodeRabbit feedback on k8s integration tests Fixes 3 actionable review comments on #581: - _wait_for_status now fails explicitly with an attempt count instead of silently returning on timeout, so callers get a clear message instead of a later ambiguous assertion failure. - test_pvc_lifecycle no longer asserts inside its finally block, so a cleanup-path failure can't mask the real assertion failure from the try block. - test_reconcile_k8s's teardown deletes puller and server concurrently via asyncio.gather(return_exceptions=True) so a failure deleting one doesn't leak the other. Also addresses 2 nitpicks: - README: added a prerequisites block ahead of the kind/dev-blue run commands in the new Cluster validation section. - test_reconcile_k8s's get_side_effect now raises KeyError for any Deployment name other than "puller"/"server" instead of silently falling back to server_dep, so a typo fails loudly. Left 2 nitpicks as-is (noted in the PR): the `from __future__ import annotations` import and the import-time cluster/daemon availability probe both match the pre-existing docker_availability.py / test_reconcile_docker.py convention already used throughout this test suite; fixing only the new k8s files would create inconsistency, and a broader refactor of the shared pattern is out of scope here. Signed-off-by: Tyler Bray <tbray@nvidia.com> * docs(deployments): clarify ambient kubeconfig wording in README Addresses mckornfield's PR #581 nitpick that "ambient" reads oddly out of context. Signed-off-by: tbray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com> Signed-off-by: tbray <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
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
test_reconcile_k8s.py) exercising a puller→server prerequisite chain against a real cluster. PVC-mount gating is intentionally not covered here — kind's defaultlocal-pathStorageClass usesWaitForFirstConsumerbinding, which deadlocks againstDeploymentReconciler's requirement that mounted volumes already beBOUNDbefore pod creation (documented in the test's docstring and in the README).kubeconfig_availability.pyskip gate (mirrorsdocker_availability.py) so these tests skip cleanly in CI when no cluster is reachable — no new CI job required for this phase's closure.conftest.py's xdist grouping so k8s and docker integration tests run in separate serialized groups (they share cluster/daemon state within their own group but don't collide with each other).kindor dev-blue) in the plugin README's new "Cluster validation" section.Branched ahead of Phase 6
This branches from
origin/mainrather than waiting for Phase 6 (#575, RBAC + Helm chart integration) to merge. Phase 6's RBAC change only matters when the realnmp-corecontroller pod acts through its restricted ServiceAccount; these integration tests instantiateK8sDeploymentBackenddirectly in pytest, authenticated with the developer's own kubeconfig — not that constrained ServiceAccount — so there's no functional dependency. #575 was open/mergeable with green CI, blocked only on reviewer availability. Will rebase ontomainonce #575 lands (both touch the plugin README, but only via non-conflicting appends).Test plan
uv run --frozen pytest plugins/nemo-deployments/tests/unit -q— 251 passeduv run --frozen pytest plugins/nemo-deployments/tests/integration -vagainst a localkindcluster — 10 passed (5 docker + 5 k8s, including the new tests)uv run pre-commit run -aon changed files — ruff, ty, copyright headers, merge-conflict checks all passstatus == SUCCEEDEDwithout also waiting onexit_code, which thesucceededprerequisite condition requires)Summary by CodeRabbit