feat(deployments): reconcile controller and prerequisite DAG (AIRCORE-758) - #315
Merged
tylersbray merged 4 commits intoJun 25, 2026
Merged
Conversation
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
✅ Action performedFull review finished. |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughAdds a DeploymentsController with reconcilers for Deployment and Volume entities, paginated listing, prerequisite and volume-mount gating, drift-recovery backoff, orphan cleanup, config/types updates, entry-point registration, and unit/integration tests. ChangesDeployments Reconciliation Controller
🚥 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 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: 10
🧹 Nitpick comments (2)
plugins/nemo-deployments/tests/integration/test_reconcile_docker.py (1)
14-20: 📐 Maintainability & Code QualityGate this Docker integration test on backend registration (avoid permanent module skip)
- The module-level
pytestmark = pytest.mark.skip(...)at line 14 will keep the whole integration module disabled even after AIRCORE-756 backends register.BACKEND_CLASSESis empty until docker/k8s backends land, and backend type keys are registry strings (unit tests use"docker"/"k8s").- Replace the unconditional skip with
skipif("docker" not in BACKEND_CLASSES, ...)so the test auto-enables once AIRCORE-756 registers the Docker backend.🤖 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_docker.py` around lines 14 - 20, Replace the unconditional module-level skip (pytestmark = pytest.mark.skip(...)) with a conditional skip using pytest.mark.skipif so the tests re-enable when the Docker backend registers: import or reference BACKEND_CLASSES and set pytestmark = pytest.mark.skipif("docker" not in BACKEND_CLASSES, reason="Requires DockerDeploymentBackend (AIRCORE-756)"); keep the existing reason text and leave test_puller_server_prerequisite_chain unchanged so it automatically runs once "docker" appears in BACKEND_CLASSES.plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/listing.py (1)
71-72: 🩺 Stability & Availability | ⚡ Quick winOverly broad exception handling suppresses error details.
Catching all exceptions and returning
Nonehides the distinction between "entity not found" vs real errors (network, permissions, etc.). Caller logs "not yet available" but can't distinguish transient failures from missing data.Consider catching only expected exceptions or logging before returning
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/reconciler/listing.py` around lines 71 - 72, The bare "except Exception: return None" in listing.py hides real errors; replace it by catching only expected exceptions (e.g., KeyError, NotFoundError, requests.HTTPError or your storage-specific NotFound) and return None only for those, and for any other exception log the full error with logger.exception(...) (or re-raise) so transient/network/permission errors are not suppressed; update the block that currently reads "except Exception: return None" to explicitly handle expected exception types and use logger.exception to record unexpected errors before returning or re-raising.
🤖 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/README.md`:
- Line 24: Update the README line that currently states a single
`is_healthy=False` flag to document the new split health signals: replace the
one-line reference to `is_healthy=False` with a note that failures are reported
separately for deployment-list and volume-list (e.g., `deployment_list_healthy`
and `volume_list_healthy` or similar list-health signals), and mention that
operators should check each list-specific health flag to determine the failure
source.
- Around line 46-50: Move the "## Next steps" section so it appears after the
"## Tests" (i.e., at the very end of the README) and add cross-links from its
bullet items to the relevant docs: link "Docker and Kubernetes
`DeploymentBackend` implementations" to the DeploymentBackend implementation
docs and link "Models/agents adoption projecting from plugin `Deployment`
status" to the models/agents projection doc (update the two bullets to include
those explicit anchors/URLs and ensure the header remains "## Next steps").
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/config.py`:
- Around line 20-31: ControllerConfig currently allows invalid values; add
Pydantic field constraints and a cross-field validator: use Field(..., gt=0) for
interval_seconds and orphan_cleanup_every_n_cycles, use Field(..., ge=0) for
drift_recovery_base_delay_seconds and Field(..., ge=0) for
drift_recovery_max_delay_seconds (and Field(..., ge=0) for
drift_recovery_max_attempts), then implement a `@root_validator` (e.g.,
validate_backoff) on ControllerConfig to assert
drift_recovery_base_delay_seconds <= drift_recovery_max_delay_seconds and raise
a ValueError with a clear message if violated; reference the ControllerConfig
class and the field names interval_seconds, drift_recovery_base_delay_seconds,
drift_recovery_max_delay_seconds, orphan_cleanup_every_n_cycles, and
drift_recovery_max_attempts when locating where to add these checks.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/controller.py`:
- Line 33: Add an explicit type annotation for the instance attribute _registry
on the controller class: replace the untyped assignment "self._registry = None"
with a typed declaration "self._registry: ExecutorRegistry | None = None" so the
attribute matches the pattern used by other instance variables (referencing the
_registry attribute and the ExecutorRegistry type in this file).
- Around line 255-258: The code currently uses setdefault in
_index_deployments()/ _ensure_prerequisite_refs_loaded so by_config[(workspace,
dep.deployment_config_name)] picks the first element returned by an unordered
list_all_pages, causing nondeterministic prerequisite resolution in
get_deployment_for_config_name(); fix by either enforcing uniqueness of
deployment_config_name per workspace (validate and raise an error in
_index_deployments or during creation when duplicate keys are found) or
implement a deterministic selection rule (e.g., always choose the Deployment
with the smallest lexicographic dep.name or earliest created_at) instead of
setdefault; update _index_deployments, _ensure_prerequisite_refs_loaded and
get_deployment_for_config_name to detect duplicates and apply the chosen
deterministic policy or raise an explicit error so prerequisite gating is
deterministic and well-logged.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/entities.py`:
- Around line 182-193: Add a Pydantic validator to the DriftRecoveryPolicy model
to ensure base_delay_seconds ≤ max_delay_seconds when both are provided: inside
the DriftRecoveryPolicy class (the class that declares max_attempts,
base_delay_seconds, max_delay_seconds) add a `@root_validator` or `@validator` that
checks if base_delay_seconds and max_delay_seconds are not None and raises a
ValueError with a clear message if base_delay_seconds > max_delay_seconds; keep
existing field names (base_delay_seconds, max_delay_seconds, max_attempts) and
ensure the validator runs on model instantiation and test cases cover the
invalid scenario.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/deployment_reconciler.py`:
- Around line 316-320: The code re-resolves the blocking prerequisite using the
current deployment.workspace causing cross-workspace prerequisites to be missed;
update the prerequisite resolution flow to have PrerequisiteResult include the
resolved target's workspace/identifier or a reference to the resolved deployment
entity (e.g., add fields like resolved_workspace or resolved_target to
PrerequisiteResult when the prerequisite contract resolves by
workspace/config/name), then change the check that currently uses
deployments_by_name/deployments_by_config and deployment.workspace to instead
use the resolved target from result (e.g., use result.resolved_target.status or
look up by result.resolved_workspace and the target id) so FAILED upstreams in
other workspaces are detected correctly.
- Around line 24-25: The delete path doesn't clear the drift recovery state, so
when a Deployment with the same workspace/name is recreated it inherits prior
backoff/exhaustion state; update _reconcile_delete to compute the key with
deployment_id(deployment) and remove that entry from self._drift_cache (if
present), and make the same removal in the alternate delete branch referenced
around the _reconcile_delete sibling code (the block at lines ~172-191) so
recreated Deployments start with a fresh recovery state.
- Around line 87-106: The delete path currently fetches DeploymentConfig before
checking for STOPPED/DELETING, causing missing configs to trigger
_project_failure; change the logic in deployment_reconciler.py so the check "if
deployment.desired_state == 'STOPPED' or deployment.status == 'DELETING': await
self._reconcile_delete(deployment, config)" happens before any call to
self._config_cache.get or self._entities.get (or alternatively make
_reconcile_delete not require a config and call it without fetching/using
config); specifically reorder the block around dep_id/config_key/_config_cache
lookup and the NemoEntityNotFoundError handling so _reconcile_delete is invoked
immediately for delete/stop cases, avoiding lookup-induced failures.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_reconciler.py`:
- Around line 45-60: The exception handlers in volume_reconciler.py (around
backend.create_volume, the subsequent _project_status calls, and the block
updating status) currently catch all Exceptions and convert
NemoEntityConflictError into a FAILED status; change these handlers to let
NemoEntityConflictError bubble up to the controller conflict handler by
explicitly catching NemoEntityConflictError (the conflict exception type) first
and re-raising it, and only then handle other exceptions by logging and calling
_project_status with VolumeStatusUpdate(status="FAILED", ...). Update the
try/except blocks surrounding backend.create_volume, the later update path, and
any other blocks noted (lines referenced in the review) to follow this pattern
so optimistic-lock conflicts are not swallowed.
---
Nitpick comments:
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/listing.py`:
- Around line 71-72: The bare "except Exception: return None" in listing.py
hides real errors; replace it by catching only expected exceptions (e.g.,
KeyError, NotFoundError, requests.HTTPError or your storage-specific NotFound)
and return None only for those, and for any other exception log the full error
with logger.exception(...) (or re-raise) so transient/network/permission errors
are not suppressed; update the block that currently reads "except Exception:
return None" to explicitly handle expected exception types and use
logger.exception to record unexpected errors before returning or re-raising.
In `@plugins/nemo-deployments/tests/integration/test_reconcile_docker.py`:
- Around line 14-20: Replace the unconditional module-level skip (pytestmark =
pytest.mark.skip(...)) with a conditional skip using pytest.mark.skipif so the
tests re-enable when the Docker backend registers: import or reference
BACKEND_CLASSES and set pytestmark = pytest.mark.skipif("docker" not in
BACKEND_CLASSES, reason="Requires DockerDeploymentBackend (AIRCORE-756)"); keep
the existing reason text and leave test_puller_server_prerequisite_chain
unchanged so it automatically runs once "docker" appears in BACKEND_CLASSES.
🪄 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: 273201cc-6cdf-4da3-b8ef-41b24205791e
📒 Files selected for processing (24)
plugins/nemo-deployments/README.mdplugins/nemo-deployments/pyproject.tomlplugins/nemo-deployments/src/nemo_deployments_plugin/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/controller.pyplugins/nemo-deployments/src/nemo_deployments_plugin/entities.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/deployment_reconciler.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/drift_recovery.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/listing.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/orphan_cleanup.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/prerequisite.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_mounts.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_reconciler.pyplugins/nemo-deployments/src/nemo_deployments_plugin/types.pyplugins/nemo-deployments/tests/integration/test_reconcile_docker.pyplugins/nemo-deployments/tests/unit/reconciler/conftest.pyplugins/nemo-deployments/tests/unit/reconciler/test_controller.pyplugins/nemo-deployments/tests/unit/reconciler/test_deployment_reconciler.pyplugins/nemo-deployments/tests/unit/reconciler/test_drift_recovery.pyplugins/nemo-deployments/tests/unit/reconciler/test_listing.pyplugins/nemo-deployments/tests/unit/reconciler/test_prerequisite.pyplugins/nemo-deployments/tests/unit/reconciler/test_volume_mounts.pyplugins/nemo-deployments/tests/unit/reconciler/test_volume_reconciler.pyplugins/nemo-deployments/tests/unit/test_config.pyplugins/nemo-deployments/tests/unit/test_service_startup.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
plugins/nemo-deployments/src/nemo_deployments_plugin/entities.py (1)
182-193: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd bounds and cross-field validation for drift recovery overrides.
Line 182-Line 193allow negative overrides andbase_delay_seconds > max_delay_seconds, which can make per-config backoff invalid.Proposed fix
-from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, model_validator @@ class DriftRecoveryPolicy(BaseModel): action: DriftRecoveryAction = "recreate" max_attempts: int | None = Field( - default=None, + default=None, + ge=0, description="Override controller drift_recovery_max_attempts when set.", ) base_delay_seconds: int | None = Field( - default=None, + default=None, + ge=0, description="Override controller drift_recovery_base_delay_seconds when set.", ) max_delay_seconds: int | None = Field( - default=None, + default=None, + ge=0, description="Override controller drift_recovery_max_delay_seconds when set.", ) + + `@model_validator`(mode="after") + def _validate_delays(self) -> "DriftRecoveryPolicy": + if ( + self.base_delay_seconds is not None + and self.max_delay_seconds is not None + and self.base_delay_seconds > self.max_delay_seconds + ): + raise ValueError("base_delay_seconds must not exceed max_delay_seconds") + return self🤖 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/entities.py` around lines 182 - 193, Add validation to the Pydantic model that defines max_attempts, base_delay_seconds, and max_delay_seconds so negative values are rejected and base_delay_seconds cannot exceed max_delay_seconds; implement field validators for max_attempts, base_delay_seconds, and max_delay_seconds to ensure each non-None value is >= 0 and add a root_validator (or cross-field validator) to check if both base_delay_seconds and max_delay_seconds are set then base_delay_seconds <= max_delay_seconds, raising a ValueError with a clear message referencing those field names.plugins/nemo-deployments/src/nemo_deployments_plugin/controller.py (1)
256-258: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake deployment_config_name indexing deterministic (or reject duplicates).
Line 256,Line 258, andLine 273usesetdefaulton config-key maps, so duplicate(workspace, deployment_config_name)picks whichever item appears first. That makes prerequisite resolution order-dependent.Define one policy: reject duplicates loudly, or deterministically pick one (with an explicit sort/selector) before indexing.
Also applies to: 272-274
🤖 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/controller.py` around lines 256 - 258, The indexing using by_config.setdefault((workspace, dep.deployment_config_name), dep) (and the similar setdefault with key_by_config) makes selection order-dependent; change this to detect duplicates and fail fast: before inserting, check if (workspace, dep.deployment_config_name) (and key_by_config) already exists in by_config and if the existing value is a different deployment instance, raise an explicit exception (including workspace, deployment_config_name, existing dep.name and new dep.name) to reject duplicate config names; alternatively, if you prefer deterministic selection instead of rejecting, gather all deps for the same (workspace, deployment_config_name) first and then pick one deterministically (e.g., min by dep.name or sort by a stable attribute) before assigning into by_config so resolution is order-independent.plugins/nemo-deployments/src/nemo_deployments_plugin/config.py (1)
23-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate controller timing/backoff fields at the model boundary.
Line 23/Line 27accept non-positive values, andLine 25/Line 26allowbase > max. That permits invalid reconcile/backoff schedules.Proposed fix
class ControllerConfig(BaseModel): @@ - interval_seconds: int = Field(default=5, description="Reconciliation loop interval in seconds.") - drift_recovery_max_attempts: int = Field(default=5, description="Max drift recovery attempts before FAILED.") - drift_recovery_base_delay_seconds: int = Field(default=5, description="Base delay for drift recovery backoff.") - drift_recovery_max_delay_seconds: int = Field(default=300, description="Max delay cap for drift recovery backoff.") + interval_seconds: int = Field(default=5, gt=0, description="Reconciliation loop interval in seconds.") + drift_recovery_max_attempts: int = Field(default=5, ge=0, description="Max drift recovery attempts before FAILED.") + drift_recovery_base_delay_seconds: int = Field(default=5, ge=0, description="Base delay for drift recovery backoff.") + drift_recovery_max_delay_seconds: int = Field(default=300, ge=0, description="Max delay cap for drift recovery backoff.") @@ - orphan_cleanup_every_n_cycles: int = Field( + orphan_cleanup_every_n_cycles: int = Field( default=6, + gt=0, description="Run orphan substrate cleanup every N reconcile cycles.", ) + + `@model_validator`(mode="after") + def _validate_backoff_bounds(self) -> "ControllerConfig": + if self.drift_recovery_base_delay_seconds > self.drift_recovery_max_delay_seconds: + raise ValueError( + "drift_recovery_base_delay_seconds must be <= drift_recovery_max_delay_seconds" + ) + return self🤖 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/config.py` around lines 23 - 30, The model allows invalid timing/backoff values; add validation on the fields interval_seconds, drift_recovery_max_attempts, drift_recovery_base_delay_seconds, drift_recovery_max_delay_seconds, and orphan_cleanup_every_n_cycles so they must be positive (use Field constraints like gt=0 or ge=1) and enforce drift_recovery_base_delay_seconds <= drift_recovery_max_delay_seconds (use a `@root_validator` or `@validator` that compares those two fields and raises a ValueError if base > max). Ensure validators run at model-creation to prevent non-positive or inconsistent backoff schedules from being accepted.plugins/nemo-deployments/README.md (2)
24-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument split list-health signals, not just combined
is_healthyflag.Line 24 states
is_healthy=Falsebehavior but doesn't expose the separate deployment-list vs volume-list failure flags that operators need for troubleshooting.🤖 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` at line 24, The README currently only documents the combined is_healthy flag; update the "list-health" documentation to explicitly describe the separate signals for deployment-list and volume-list failures (e.g., deployment-list health and volume-list health), show their possible values and how they map to the aggregated is_healthy boolean, and include examples and troubleshooting guidance so operators can distinguish a failing deployment-list vs volume-list when is_healthy=False; reference the is_healthy, deployment-list, and volume-list terms in the updated text.Source: Coding guidelines
46-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove Next Steps after Tests section and add cross-links.
Next Steps appears before Tests (line 51+), violating the guideline. No cross-links present in bullets.
🤖 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 46 - 50, The "Next steps" section (heading "Next steps") is placed before the "Tests" section and lacks cross-links; move the entire "Next steps" block so it appears after the "Tests" section and update each bullet to include markdown cross-links pointing to the related sections or artifacts (e.g., link "Docker and Kubernetes `DeploymentBackend` implementations" to the DeploymentBackend docs or section, and link "Models/agents adoption..." to the plugin `Deployment` status or relevant models page); ensure the heading text remains "Next steps" and that links use relative anchors or section headers to follow repo linking conventions.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
`@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/orphan_cleanup.py`:
- Around line 29-33: The split of deployment_id into parts only ensures a
delimiter exists but doesn't prevent empty components (e.g., "/job" or "ws/"),
so update the logic around deployment_id/parts/workspace/name in
orphan_cleanup.py to validate that both workspace and name are non-empty (after
optional .strip()), log a warning via logger.warning including the malformed
deployment_id, and continue without calling the delete code when either is
empty; ensure the code path that issues the delete uses only the validated
workspace and name variables.
In `@plugins/nemo-deployments/tests/unit/reconciler/test_controller.py`:
- Line 97: The assertions currently compare the method object ctrl.is_healthy to
booleans instead of invoking it; change the two assertions that reference
ctrl.is_healthy (the failing assertions around line 97 and 113 in the
test_controller) to call the method (ctrl.is_healthy()) so the health check
executes and returns a boolean for comparison.
In
`@plugins/nemo-deployments/tests/unit/reconciler/test_deployment_reconciler.py`:
- Around line 394-420: The test currently only checks dep.status and
dep.status_message so it can pass even if create is retried immediately; change
the test_drift_recovery_create_failure_stays_lost_and_backoffs to explicitly
assert the number of create attempts on mock_backend.create_deployment (e.g.,
replace failing_create assignment with a mock/spy that raises once and records
calls, or wrap failing_create and increment a counter) and after the first
reconcile assert the create was called exactly once and after the second
reconcile assert no additional create call was made (total call count
unchanged), referencing reconcile_one and mock_backend.create_deployment to
locate the logic to modify.
- Line 4: Remove the postponed annotations import by deleting the line "from
__future__ import annotations" from the test module so annotations are evaluated
normally; if any type hints in this file (e.g., in test functions or class
definitions) were written as strings, convert them back to concrete type hints
to comply with the project's guideline.
In `@plugins/nemo-deployments/tests/unit/reconciler/test_prerequisite.py`:
- Line 6: The tests use a bare "from helpers import make_deployment,
make_deployment_config" which breaks package-relative imports; replace the bare
import with an explicit relative import (e.g., "from .helpers import
make_deployment, make_deployment_config") in test_prerequisite.py and similarly
update the other tests (test_volume_mounts.py, test_volume_reconciler.py) to use
the appropriate relative form (from .helpers or from ..helpers depending on
their package layout) so the test modules import helpers correctly.
---
Duplicate comments:
In `@plugins/nemo-deployments/README.md`:
- Line 24: The README currently only documents the combined is_healthy flag;
update the "list-health" documentation to explicitly describe the separate
signals for deployment-list and volume-list failures (e.g., deployment-list
health and volume-list health), show their possible values and how they map to
the aggregated is_healthy boolean, and include examples and troubleshooting
guidance so operators can distinguish a failing deployment-list vs volume-list
when is_healthy=False; reference the is_healthy, deployment-list, and
volume-list terms in the updated text.
- Around line 46-50: The "Next steps" section (heading "Next steps") is placed
before the "Tests" section and lacks cross-links; move the entire "Next steps"
block so it appears after the "Tests" section and update each bullet to include
markdown cross-links pointing to the related sections or artifacts (e.g., link
"Docker and Kubernetes `DeploymentBackend` implementations" to the
DeploymentBackend docs or section, and link "Models/agents adoption..." to the
plugin `Deployment` status or relevant models page); ensure the heading text
remains "Next steps" and that links use relative anchors or section headers to
follow repo linking conventions.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/config.py`:
- Around line 23-30: The model allows invalid timing/backoff values; add
validation on the fields interval_seconds, drift_recovery_max_attempts,
drift_recovery_base_delay_seconds, drift_recovery_max_delay_seconds, and
orphan_cleanup_every_n_cycles so they must be positive (use Field constraints
like gt=0 or ge=1) and enforce drift_recovery_base_delay_seconds <=
drift_recovery_max_delay_seconds (use a `@root_validator` or `@validator` that
compares those two fields and raises a ValueError if base > max). Ensure
validators run at model-creation to prevent non-positive or inconsistent backoff
schedules from being accepted.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/controller.py`:
- Around line 256-258: The indexing using by_config.setdefault((workspace,
dep.deployment_config_name), dep) (and the similar setdefault with
key_by_config) makes selection order-dependent; change this to detect duplicates
and fail fast: before inserting, check if (workspace,
dep.deployment_config_name) (and key_by_config) already exists in by_config and
if the existing value is a different deployment instance, raise an explicit
exception (including workspace, deployment_config_name, existing dep.name and
new dep.name) to reject duplicate config names; alternatively, if you prefer
deterministic selection instead of rejecting, gather all deps for the same
(workspace, deployment_config_name) first and then pick one deterministically
(e.g., min by dep.name or sort by a stable attribute) before assigning into
by_config so resolution is order-independent.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/entities.py`:
- Around line 182-193: Add validation to the Pydantic model that defines
max_attempts, base_delay_seconds, and max_delay_seconds so negative values are
rejected and base_delay_seconds cannot exceed max_delay_seconds; implement field
validators for max_attempts, base_delay_seconds, and max_delay_seconds to ensure
each non-None value is >= 0 and add a root_validator (or cross-field validator)
to check if both base_delay_seconds and max_delay_seconds are set then
base_delay_seconds <= max_delay_seconds, raising a ValueError with a clear
message referencing those field names.
🪄 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: b5d7c440-e45a-4d0d-8204-453bdfe2b22f
📒 Files selected for processing (24)
plugins/nemo-deployments/README.mdplugins/nemo-deployments/pyproject.tomlplugins/nemo-deployments/src/nemo_deployments_plugin/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/controller.pyplugins/nemo-deployments/src/nemo_deployments_plugin/entities.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/deployment_reconciler.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/drift_recovery.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/listing.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/orphan_cleanup.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/prerequisite.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_mounts.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_reconciler.pyplugins/nemo-deployments/src/nemo_deployments_plugin/types.pyplugins/nemo-deployments/tests/integration/test_reconcile_docker.pyplugins/nemo-deployments/tests/unit/reconciler/conftest.pyplugins/nemo-deployments/tests/unit/reconciler/test_controller.pyplugins/nemo-deployments/tests/unit/reconciler/test_deployment_reconciler.pyplugins/nemo-deployments/tests/unit/reconciler/test_drift_recovery.pyplugins/nemo-deployments/tests/unit/reconciler/test_listing.pyplugins/nemo-deployments/tests/unit/reconciler/test_prerequisite.pyplugins/nemo-deployments/tests/unit/reconciler/test_volume_mounts.pyplugins/nemo-deployments/tests/unit/reconciler/test_volume_reconciler.pyplugins/nemo-deployments/tests/unit/test_config.pyplugins/nemo-deployments/tests/unit/test_service_startup.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/orphan_cleanup.py (1)
29-33: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject malformed
workspace/namebefore delete.Delimiter-only validation accepts
"/job"and"ws/", which can call delete with empty identifiers. Require both parts to be non-empty (after trim) before issuing delete.🤖 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/reconciler/orphan_cleanup.py` around lines 29 - 33, The current check only ensures a "/" is present but allows empty workspace or name; update the validation around deployment_id splitting so after parts = deployment_id.split("/", 1) you verify len(parts) == 2 and that both parts[0].strip() and parts[1].strip() are non-empty before proceeding to delete. Assign workspace, name = parts[0].strip(), parts[1].strip(), and if either is empty log the same warning (e.g., "Invalid deployment id from backend: %r, skipping") and continue to avoid issuing delete with empty identifiers.plugins/nemo-deployments/src/nemo_deployments_plugin/entities.py (1)
182-193: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate
drift_recoveryoverride bounds.Override fields allow invalid values (negative attempts/delays and inconsistent base/max delay), which can produce incorrect backoff decisions. Add non-negative constraints and enforce
base_delay_seconds <= max_delay_secondswhen both are set.🤖 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/entities.py` around lines 182 - 193, Add validation to the model that owns max_attempts, base_delay_seconds, and max_delay_seconds so overrides cannot be negative and, when both base_delay_seconds and max_delay_seconds are set, require base_delay_seconds <= max_delay_seconds; implement this via Pydantic field validators or a root_validator on the same class to raise a ValidationError for negative values and for the inconsistent base/max relationship.plugins/nemo-deployments/src/nemo_deployments_plugin/config.py (1)
23-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd numeric bounds to controller cadence/backoff settings.
These fields currently accept invalid values (zero/negative cadence, negative delays, inconsistent base/max delay), which can break reconcile timing and backoff behavior. Enforce bounds and base≤max validation.
🤖 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/config.py` around lines 23 - 30, The numeric cadence/backoff fields (interval_seconds, drift_recovery_max_attempts, drift_recovery_base_delay_seconds, drift_recovery_max_delay_seconds, orphan_cleanup_every_n_cycles) accept invalid values; add Pydantic validation: set Field constraints (e.g., gt=0 for interval_seconds, orphan_cleanup_every_n_cycles, drift_recovery_base_delay_seconds >=0, drift_recovery_max_delay_seconds >=0 and drift_recovery_max_attempts >=0) and implement a model-level validator (root_validator) to enforce drift_recovery_base_delay_seconds <= drift_recovery_max_delay_seconds and raise a clear ValidationError if violated; update error messages to state the offending field and required bounds.plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_reconciler.py (1)
45-60: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRe-raise optimistic-lock conflicts in
volume_reconciler.pyanddeployment_reconciler.py.Both files wrap backend create/read flows in
except Exceptionblocks that also catchNemoEntityConflictErrorfrom_project_status(). That turns a concurrent entity update into a syntheticFAILEDorLOSTprojection instead of surfacing the conflict to the controller retry path.🤖 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/reconciler/volume_reconciler.py` around lines 45 - 60, The except blocks in volume_reconciler.py and deployment_reconciler.py are catching NemoEntityConflictError thrown by _project_status and converting concurrency conflicts into FAILED/LOST projections; change the handlers so NemoEntityConflictError is re-raised instead of translated. Concretely, in the try/except around backend.create_volume (and the analogous read/create flows in deployment_reconciler) add a specific check to re-raise NemoEntityConflictError (or add an explicit except NemoEntityConflictError: raise) before handling other exceptions, and ensure NemoEntityConflictError is imported where used; leave the existing logging/status update logic for all other Exception cases.plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/deployment_reconciler.py (3)
172-198: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear drift recovery state on delete.
deployment_id()is stable across recreations. Leaving_drift_cachepopulated means a new deployment with the same workspace/name inherits old backoff or exhaustion state.Suggested fix
- async def _reconcile_delete(self, deployment: Deployment, config: DeploymentConfig) -> None: + async def _reconcile_delete(self, deployment: Deployment, config: DeploymentConfig) -> None: dep_id = deployment_id(deployment) + self._drift_cache.remove(dep_id) backend = self._try_resolve_backend(deployment)🤖 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/reconciler/deployment_reconciler.py` around lines 172 - 198, When deleting a deployment in _reconcile_delete, clear any drift recovery state so a recreated deployment doesn't inherit backoff/exhaustion: after computing dep_id = deployment_id(deployment) and after attempting to remove the Deployment entity (handle both success and NemoEntityNotFoundError), remove the key from self._drift_cache (e.g. self._drift_cache.pop(dep_id, None)) so it silently no-ops if absent; ensure this is done regardless of whether backend.delete_deployment was called and without raising on missing keys.
87-106: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake delete reconciliation config-free.
_reconcile_delete()does not useconfig, but the lookup happens first. If the config is deleted before the deployment, STOPPED/DELETING entities get projected toFAILEDand never removed.Suggested fix
async def reconcile_one( self, deployment: Deployment, *, deployments_by_config: dict[tuple[str, str], Deployment], deployments_by_name: dict[tuple[str, str], Deployment], volumes_by_name: dict[tuple[str, str], Volume], ) -> None: dep_id = deployment_id(deployment) + if deployment.desired_state == "STOPPED" or deployment.status == "DELETING": + await self._reconcile_delete(deployment) + return + config_key = (deployment.workspace, deployment.deployment_config_name) config = self._config_cache.get(config_key) if config is None: try: config = await self._entities.get( @@ except NemoEntityNotFoundError: await self._project_failure( deployment, f"DeploymentConfig '{deployment.deployment_config_name}' not found" ) return - - if deployment.desired_state == "STOPPED" or deployment.status == "DELETING": - await self._reconcile_delete(deployment, config) - return @@ - async def _reconcile_delete(self, deployment: Deployment, config: DeploymentConfig) -> None: + async def _reconcile_delete(self, deployment: Deployment) -> 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/reconciler/deployment_reconciler.py` around lines 87 - 106, The code fetches DeploymentConfig before checking for delete/stop, which causes missing configs to mark deletions as FAILED; move the early-return for deletion cases so that the check "if deployment.desired_state == 'STOPPED' or deployment.status == 'DELETING': await self._reconcile_delete(deployment, config); return" runs before any config lookup (i.e., before computing config_key, accessing self._config_cache, or calling self._entities.get), or alternatively skip the config fetch when those conditions are true; update references to deployment_id, _config_cache, _entities.get, NemoEntityNotFoundError, _project_failure and _reconcile_delete accordingly so deletion reconciliation proceeds without requiring a DeploymentConfig.
308-320: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse the resolved prerequisite target, not
deployment.workspace.This helper re-resolves the blocker only in the dependent deployment's workspace. If a prerequisite points at another workspace, a
FAILEDupstream is missed and the dependent deployment staysPENDING.🤖 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/reconciler/deployment_reconciler.py` around lines 308 - 320, The helper _prerequisite_failed incorrectly always looks up the blocking prerequisite in deployment.workspace; change the lookup to use the prerequisite's own workspace when available: when resolving target use the exact key represented by result.blocking_prerequisite (if it already encodes workspace+name as a tuple/key) or, if result.blocking_prerequisite is a string that includes a workspace qualifier (parse it into (workspace, name)), call deployments_by_name.get((prereq_workspace, prereq_name)) / deployments_by_config.get((prereq_workspace, prereq_name)); only fall back to using deployment.workspace if the prerequisite does not include its own workspace. Ensure you still return target is not None and target.status == "FAILED" (and preserve the early return when blocking_prerequisite is None).plugins/nemo-deployments/tests/unit/reconciler/test_controller.py (1)
97-97: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInvoke
is_healthy()in these assertions.Line 97 and Line 113 compare a bound method object, not the health result. Call the method.
Patch
- assert ctrl.is_healthy is False + assert ctrl.is_healthy() is False ... - assert ctrl.is_healthy is False + assert ctrl.is_healthy() is FalseAlso applies to: 113-113
🤖 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/reconciler/test_controller.py` at line 97, The tests are asserting against the bound method object instead of its return value; update the assertions that reference ctrl.is_healthy to call the method (e.g., use ctrl.is_healthy() in the failing assertions at the spots that currently read ctrl.is_healthy) so they compare the boolean result rather than the method object (apply same change for both occurrences mentioned).
🤖 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/reconciler/drift_recovery.py`:
- Around line 54-56: The backoff calculation uses state.attempts directly,
causing the first delay to be base_delay_seconds * 2; change the exponent to use
attempts-1 (but not less than 0) so the first wait is base_delay_seconds. Update
the expression that computes backoff_seconds (which currently uses
limits.base_delay_seconds * (2**state.attempts) and min with
limits.max_delay_seconds) to use 2**max(state.attempts - 1, 0) instead; keep the
min(..., limits.max_delay_seconds) bound and reference state.add_attempt
behavior in your reasoning.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/listing.py`:
- Around line 67-72: The current try/except around entities.get(Deployment,
name=config_name, workspace=workspace) swallows all exceptions; remove the broad
"except Exception" (or replace it with re-raising) so only
NemoEntityNotFoundError is handled and all other errors propagate. Specifically,
keep the except NemoEntityNotFoundError: return None block, delete or change the
catch-all so that failures from entities.get or other backend errors are not
converted into a missing-deployment result (i.e., let the exception bubble up or
re-raise it).
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/prerequisite.py`:
- Around line 29-32: Change the lookup order to consult deployments_by_config
first using key_by_config = (workspace, prerequisite.deployment_name), returning
that entry if present; only if that returns None, fall back to the name index
(key_by_name) but only accept it when it is an explicit match for the
config-name case (i.e., ensure deployments_by_name[key_by_name] corresponds to
the same deployment name/config before returning). Update the logic around
deployments_by_config, deployments_by_name, key_by_config, key_by_name, and
prerequisite.deployment_name to reflect this order.
In `@plugins/nemo-deployments/tests/integration/test_reconcile_docker.py`:
- Around line 14-15: Replace the unconditional module skip with a conditional
skip that checks the registered backend keys: change the pytest mark from
pytest.mark.skip(...) to pytest.mark.skipif("docker" not in BACKEND_CLASSES,
reason="Requires DockerDeploymentBackend (AIRCORE-756)"), referencing
BACKEND_CLASSES and using pytest.mark.skipif so the tests only skip when the
"docker" backend key is not present.
In `@plugins/nemo-deployments/tests/unit/reconciler/test_volume_reconciler.py`:
- Line 4: Remove the unnecessary "from __future__ import annotations" import
from the test module so type annotations remain normal runtime types;
specifically delete that import statement at the top of
test_volume_reconciler.py and run tests to ensure existing concrete annotations
(e.g., return types "-> None", "-> VolumeStatusUpdate" and parameters like
"kwargs: object") behave as expected.
---
Duplicate comments:
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/config.py`:
- Around line 23-30: The numeric cadence/backoff fields (interval_seconds,
drift_recovery_max_attempts, drift_recovery_base_delay_seconds,
drift_recovery_max_delay_seconds, orphan_cleanup_every_n_cycles) accept invalid
values; add Pydantic validation: set Field constraints (e.g., gt=0 for
interval_seconds, orphan_cleanup_every_n_cycles,
drift_recovery_base_delay_seconds >=0, drift_recovery_max_delay_seconds >=0 and
drift_recovery_max_attempts >=0) and implement a model-level validator
(root_validator) to enforce drift_recovery_base_delay_seconds <=
drift_recovery_max_delay_seconds and raise a clear ValidationError if violated;
update error messages to state the offending field and required bounds.
In `@plugins/nemo-deployments/src/nemo_deployments_plugin/entities.py`:
- Around line 182-193: Add validation to the model that owns max_attempts,
base_delay_seconds, and max_delay_seconds so overrides cannot be negative and,
when both base_delay_seconds and max_delay_seconds are set, require
base_delay_seconds <= max_delay_seconds; implement this via Pydantic field
validators or a root_validator on the same class to raise a ValidationError for
negative values and for the inconsistent base/max relationship.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/deployment_reconciler.py`:
- Around line 172-198: When deleting a deployment in _reconcile_delete, clear
any drift recovery state so a recreated deployment doesn't inherit
backoff/exhaustion: after computing dep_id = deployment_id(deployment) and after
attempting to remove the Deployment entity (handle both success and
NemoEntityNotFoundError), remove the key from self._drift_cache (e.g.
self._drift_cache.pop(dep_id, None)) so it silently no-ops if absent; ensure
this is done regardless of whether backend.delete_deployment was called and
without raising on missing keys.
- Around line 87-106: The code fetches DeploymentConfig before checking for
delete/stop, which causes missing configs to mark deletions as FAILED; move the
early-return for deletion cases so that the check "if deployment.desired_state
== 'STOPPED' or deployment.status == 'DELETING': await
self._reconcile_delete(deployment, config); return" runs before any config
lookup (i.e., before computing config_key, accessing self._config_cache, or
calling self._entities.get), or alternatively skip the config fetch when those
conditions are true; update references to deployment_id, _config_cache,
_entities.get, NemoEntityNotFoundError, _project_failure and _reconcile_delete
accordingly so deletion reconciliation proceeds without requiring a
DeploymentConfig.
- Around line 308-320: The helper _prerequisite_failed incorrectly always looks
up the blocking prerequisite in deployment.workspace; change the lookup to use
the prerequisite's own workspace when available: when resolving target use the
exact key represented by result.blocking_prerequisite (if it already encodes
workspace+name as a tuple/key) or, if result.blocking_prerequisite is a string
that includes a workspace qualifier (parse it into (workspace, name)), call
deployments_by_name.get((prereq_workspace, prereq_name)) /
deployments_by_config.get((prereq_workspace, prereq_name)); only fall back to
using deployment.workspace if the prerequisite does not include its own
workspace. Ensure you still return target is not None and target.status ==
"FAILED" (and preserve the early return when blocking_prerequisite is None).
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/orphan_cleanup.py`:
- Around line 29-33: The current check only ensures a "/" is present but allows
empty workspace or name; update the validation around deployment_id splitting so
after parts = deployment_id.split("/", 1) you verify len(parts) == 2 and that
both parts[0].strip() and parts[1].strip() are non-empty before proceeding to
delete. Assign workspace, name = parts[0].strip(), parts[1].strip(), and if
either is empty log the same warning (e.g., "Invalid deployment id from backend:
%r, skipping") and continue to avoid issuing delete with empty identifiers.
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_reconciler.py`:
- Around line 45-60: The except blocks in volume_reconciler.py and
deployment_reconciler.py are catching NemoEntityConflictError thrown by
_project_status and converting concurrency conflicts into FAILED/LOST
projections; change the handlers so NemoEntityConflictError is re-raised instead
of translated. Concretely, in the try/except around backend.create_volume (and
the analogous read/create flows in deployment_reconciler) add a specific check
to re-raise NemoEntityConflictError (or add an explicit except
NemoEntityConflictError: raise) before handling other exceptions, and ensure
NemoEntityConflictError is imported where used; leave the existing
logging/status update logic for all other Exception cases.
In `@plugins/nemo-deployments/tests/unit/reconciler/test_controller.py`:
- Line 97: The tests are asserting against the bound method object instead of
its return value; update the assertions that reference ctrl.is_healthy to call
the method (e.g., use ctrl.is_healthy() in the failing assertions at the spots
that currently read ctrl.is_healthy) so they compare the boolean result rather
than the method object (apply same change for both occurrences mentioned).
🪄 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: 334ae41b-2176-463f-bd08-1dac2e6ccb5f
📒 Files selected for processing (24)
plugins/nemo-deployments/README.mdplugins/nemo-deployments/pyproject.tomlplugins/nemo-deployments/src/nemo_deployments_plugin/config.pyplugins/nemo-deployments/src/nemo_deployments_plugin/controller.pyplugins/nemo-deployments/src/nemo_deployments_plugin/entities.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/deployment_reconciler.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/drift_recovery.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/listing.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/orphan_cleanup.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/prerequisite.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_mounts.pyplugins/nemo-deployments/src/nemo_deployments_plugin/reconciler/volume_reconciler.pyplugins/nemo-deployments/src/nemo_deployments_plugin/types.pyplugins/nemo-deployments/tests/integration/test_reconcile_docker.pyplugins/nemo-deployments/tests/unit/reconciler/conftest.pyplugins/nemo-deployments/tests/unit/reconciler/test_controller.pyplugins/nemo-deployments/tests/unit/reconciler/test_deployment_reconciler.pyplugins/nemo-deployments/tests/unit/reconciler/test_drift_recovery.pyplugins/nemo-deployments/tests/unit/reconciler/test_listing.pyplugins/nemo-deployments/tests/unit/reconciler/test_prerequisite.pyplugins/nemo-deployments/tests/unit/reconciler/test_volume_mounts.pyplugins/nemo-deployments/tests/unit/reconciler/test_volume_reconciler.pyplugins/nemo-deployments/tests/unit/test_config.pyplugins/nemo-deployments/tests/unit/test_service_startup.py
tylersbray
force-pushed
the
758-deployments-reconciler-prerequisite-dag/tbray
branch
from
June 12, 2026 21:48
dacaf46 to
331dba3
Compare
tylersbray
force-pushed
the
755-deployments-plugin-scaffold-the-plugin-entities-api-backend/tbray
branch
from
June 22, 2026 00:14
16b2cb9 to
4f7b14a
Compare
tylersbray
force-pushed
the
758-deployments-reconciler-prerequisite-dag/tbray
branch
from
June 22, 2026 00:20
331dba3 to
00f371a
Compare
tylersbray
added a commit
that referenced
this pull request
Jun 22, 2026
Re-raise NemoEntityConflictError in deployment reconciler create/drift/save paths, gate Docker integration tests on BACKEND_CLASSES, document split list-health signals in README, and add ge=0 validation on DriftRecoveryPolicy. AIRCORE-758 Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
755-deployments-plugin-scaffold-the-plugin-entities-api-backend/tbray
branch
from
June 22, 2026 22:07
4f7b14a to
da124d3
Compare
Base automatically changed from
755-deployments-plugin-scaffold-the-plugin-entities-api-backend/tbray
to
main
June 22, 2026 22:54
tylersbray
added a commit
that referenced
this pull request
Jun 22, 2026
Re-raise NemoEntityConflictError in deployment reconciler create/drift/save paths, gate Docker integration tests on BACKEND_CLASSES, document split list-health signals in README, and add ge=0 validation on DriftRecoveryPolicy. AIRCORE-758 Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
758-deployments-reconciler-prerequisite-dag/tbray
branch
from
June 22, 2026 23:05
16c9570 to
fc03c2e
Compare
Contributor
|
4 tasks
benmccown
reviewed
Jun 24, 2026
benmccown
left a comment
benmccown
left a comment
Contributor
There was a problem hiding this comment.
Quite a few nits that you can take or leave. Mostly just focused on code quality and edge cases. Core patterns LGTM.
Introduce DeploymentsController with deployment/volume reconcilers, prerequisite gating, drift recovery, and orphan cleanup on top of the 755 plugin scaffold. Stacks on PR #280 (AIRCORE-755). AIRCORE-758 Signed-off-by: Tyler Bray <tbray@nvidia.com>
Re-raise NemoEntityConflictError in deployment reconciler create/drift/save paths, gate Docker integration tests on BACKEND_CLASSES, document split list-health signals in README, and add ge=0 validation on DriftRecoveryPolicy. AIRCORE-758 Signed-off-by: Tyler Bray <tbray@nvidia.com>
… fields The lint-web-sdk check failed because plugins/nemo-deployments/openapi/openapi.yaml was out of sync with the updated DriftRecoveryPolicy and Prerequisite models. Signed-off-by: Tyler Bray <tbray@nvidia.com>
…CodeRabbit Retry backend delete failures in DELETING state, rename drift/orphan config fields, rename listing helpers to entity_client, trim README to Controller section, and apply remaining review nits (docstrings, RuntimeError guards, substrate wording, test coverage). AIRCORE-758 Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
758-deployments-reconciler-prerequisite-dag/tbray
branch
from
June 24, 2026 21:51
0b92da1 to
d036a92
Compare
benmccown
self-requested a review
June 25, 2026 16:32
benmccown
approved these changes
Jun 25, 2026
benmccown
left a comment
benmccown
left a comment
Contributor
There was a problem hiding this comment.
Thanks for addressing feedback!
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jun 25, 2026
4 tasks
tylersbray
added a commit
that referenced
this pull request
Jun 25, 2026
Add Docker backend for nemo-deployments-plugin with single-container v1 lifecycle, entity-store config fetch, port allocation, exec/http probes, plugin-local GPU pool, and unit plus integration test coverage. Stacks on #315 (AIRCORE-758 reconciler prerequisite DAG). Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
added a commit
that referenced
this pull request
Jun 26, 2026
Add Docker backend for nemo-deployments-plugin with single-container v1 lifecycle, entity-store config fetch, port allocation, exec/http probes, plugin-local GPU pool, and unit plus integration test coverage. Stacks on #315 (AIRCORE-758 reconciler prerequisite DAG). Signed-off-by: Tyler Bray <tbray@nvidia.com>
Zenodia
pushed a commit
to Zenodia/nemo-platform
that referenced
this pull request
Jun 30, 2026
…VIDIA-NeMo#399) * feat(deployments): implement DockerDeploymentBackend (AIRCORE-756) Add Docker backend for nemo-deployments-plugin with single-container v1 lifecycle, entity-store config fetch, port allocation, exec/http probes, plugin-local GPU pool, and unit plus integration test coverage. Stacks on NVIDIA-NeMo#315 (AIRCORE-758 reconciler prerequisite DAG). Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address CodeRabbit review on Docker backend (AIRCORE-756) - Exclude already-assigned host ports during multi-port allocation - Use label constants; fix HTTP/TCP probe host/port resolution - Remove TYPE_CHECKING-only docker imports - Deduplicate Docker availability check via docker_availability module - Harden port unit tests; add type hints and cleanup improvements Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): align Docker integration tests with NVIDIA-NeMo#469 prerequisite model Move server prerequisite from DeploymentConfig to Deployment, reference puller by deployment name, and drop deployments_by_config from reconcile_one. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): filter reconciler lists on data.status (AIRCORE-756) Entity store persists deployment and volume status under the data JSON column; filtering on top-level status caused list queries to fail and left DELETING deployments stuck. Align controller and status_in list API with data.status and update unit test expectations. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(test): add deployments integration test path to pytest pythonpath CI collection failed with ModuleNotFoundError for docker_availability because integration test helpers live outside the repo root import path. Mirror the existing unit-test helper entry for plugins/nemo-deployments. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): recover GPU pool allocations after process restart On platform restart, seed the shared DockerGPUPool from running deployment-managed containers so GPUs already in use are not handed out again. Scan managed container labels and HostConfig DeviceRequests during pool bootstrap and add unit coverage for parsing and recovery. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(test): serialize docker integration tests under xdist Docker integration tests share the itest workspace on one daemon. pytest-xdist loadscope spread them across workers, causing container remove/create races (409 removal in progress). Pin the package to a single xdist group, harden cleanup, and extend server reconcile polling. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): retry GPU pool init after recovery failure Only cache the shared GPU pool once container discovery succeeds so a transient Docker error does not leave all GPUs marked free on reuse. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): bind loopback for port-free check (CodeQL) Use 127.0.0.1 instead of 0.0.0.0 when probing host port availability, matching the nemo-agents pattern. Also colocate docker test helpers under backends/docker/ for clearer pytest imports. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address benmccown PR NVIDIA-NeMo#399 review feedback (AIRCORE-756) Return UNKNOWN for transient Docker API errors with reconciler retry handling, fix HTTP/TCP probes when host_url is missing, set client.api.timeout, and bump docker_timeout default to 600s. Tighten Docker SDK typing via TYPE_CHECKING, remove unused GPUPoolStatus, and migrate identity labels to nemo.nvidia.com. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(test): use mock_backend fixture in unknown-status exhaustion test Avoid isinstance against MockDeploymentBackend from executor_registry.resolve(), which fails in CI when conftest is loaded under a different module name than reconciler.conftest. Signed-off-by: Tyler Bray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com>
This was referenced Jul 2, 2026
Merged
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
Adds the deployments plugin reconcile loop on top of #280 (AIRCORE-755): a background
DeploymentsControllerthat drives Deployment and Volume state machines against registered executor backends, with prerequisite DAG gating, volume-mount readiness, drift recovery, and deployment orphan cleanup.755-deployments-plugin-scaffold-the-plugin-entities-api-backend/tbray)plugins/nemo-deployments/tests/unit(integration E2E skipped pending AIRCORE-756)Breaking changes
Prerequisite.conditionfield — new enum onDeploymentConfig.prerequisitesentries:ready(prerequisiteDeployment.status == READY) orsucceeded(default:SUCCEEDEDwithexit_code == 0). Existing configs without the field getsucceededby default.nemo.controllersentry point — registeringDeploymentsControllerstarts a background reconcile loop when the deployments plugin service runs. Empty executor registry causes new deployments to projectFAILED(no silent no-op).desired_state=STOPPEDsemantics — reconciler deletes the deployment entity (no terminalSTOPPEDstatus persisted). Matches RFC intent; callers should not expect a long-lived stopped deployment row.DeploymentConfig.driftRecovery.maxAttempts,baseDelaySeconds, andmaxDelaySecondsoverride controller defaults when set (includingmaxAttempts=0to disable recovery).Noteworthy net-new behavior
DeploymentsController— paginated cross-workspace listing (no 100-item cap), volumes reconciled before deployments, split health flags for deployment vs volume list failures, orphan cleanup gated when deployment list fails.SUCCEEDED) fetched from entity store bydeployment_config_nameeven when absent from the non-terminal list.BOUND; failed mounts can projectFAILED.LOSTAlways-policy deployments recreated with exponential backoff; per-config policy overrides; failed recreate staysLOST(preserves backoff path).list_managed_deployment_names; skipped when entity list is unhealthy.PENDING→ create volume;BOUND→ read status. Delete/RELEASEDdeferred.Deferred to follow-on PRs
Tracked in plugin README and piggybacked onto the AIRCORE-756 plan:
DockerDeploymentBackend/ K8s backendtest_reconcile_docker.py(puller→server E2E)RELEASED+ volume orphan cleanuplist_managed_volume_nameson backend ABCVolume.executorfield (755 schema)create_deploymentidempotency validationTest plan
uv run pytest plugins/nemo-deployments/tests/unit -q(83 passed)Summary by CodeRabbit
New Features
Configuration
Documentation
Tests