fix(deployments): gate READY on workload reachability (AIRCORE-998) - #1139
Merged
maxdubrinsky merged 4 commits intoAug 7, 2026
Conversation
maxdubrinsky
marked this pull request as ready for review
August 6, 2026 20:14
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDocker now performs bounded TCP readiness checks for published TCP ports. OpenShell now supports default and declared readiness probes, applies execution timeouts, and delays service exposure until readiness succeeds. Tests cover TCP, HTTP, UDP-only, portless, and failed-readiness cases. ChangesDeployment readiness probing
Sequence Diagram(s)sequenceDiagram
participant OpenShellBackend
participant WorkloadProcess
participant SandboxRPC
participant ServiceExposure
OpenShellBackend->>WorkloadProcess: Confirm live process
OpenShellBackend->>SandboxRPC: Run readiness probe
SandboxRPC->>WorkloadProcess: Check loopback HTTP or TCP endpoint
SandboxRPC-->>OpenShellBackend: Return probe status
OpenShellBackend->>ServiceExposure: Expose ports after success
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py`:
- Around line 700-704: Update OpenShellBackend readiness handling at
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py:700-704
to return ready only when _exec_detached yields exit code 0, keeping None in
STARTING; at
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py:762-765,
ensure the probe uses a guaranteed runtime or returns a nonzero result when no
interpreter exists, rather than exiting 0.
In `@plugins/nemo-deployments/tests/unit/backends/docker/test_probes.py`:
- Around line 84-93: Keep each dynamically bound test socket open until its
probe completes: in
plugins/nemo-deployments/tests/unit/backends/docker/test_probes.py lines 84-93,
move check_readiness_probe inside the socket context; in
plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py lines
667-670, move read_status inside the socket context. Do not call listen() before
the respective check.
🪄 Autofix
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: 98cb5aa7-e6e0-4213-ad95-5eec7b016173
📒 Files selected for processing (6)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/backend.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/docker/probes.pyplugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_probes.pyplugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py (1)
448-470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the retry and liveness branches.
Line 462 uses
_exec_events(0), which includes an exit event, so this test does not exercise fail-open liveness. The test callsread_statusonce, so it also does not verify the later-poll retry described by the test comment. Add an undecidable liveness stream, a complete second-poll event sequence, and assertions forREADYandExposeService.Based on the PR objective, readiness must retry after a transient no-exit result while liveness remains fail-open.
🤖 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/openshell/test_openshell_backend_mocked.py` around lines 448 - 470, Update test_read_status_starting_when_readiness_probe_yields_no_exit to use an undecidable no-exit stream for the liveness probe instead of _exec_events(0), then supply the complete ExecSandbox sequence needed for a second poll. Call read_status again and assert the retry transitions to READY and invokes ExposeService, while preserving the first-poll STARTING and no-exposure assertions.
🤖 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/tests/unit/backends/openshell/test_openshell_backend_mocked.py`:
- Around line 448-470: Update
test_read_status_starting_when_readiness_probe_yields_no_exit to use an
undecidable no-exit stream for the liveness probe instead of _exec_events(0),
then supply the complete ExecSandbox sequence needed for a second poll. Call
read_status again and assert the retry transitions to READY and invokes
ExposeService, while preserving the first-poll STARTING and no-exposure
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 452698bb-3a0b-46ee-aa4b-8743a63db03a
📒 Files selected for processing (4)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.pyplugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.pyplugins/nemo-deployments/tests/unit/backends/docker/test_probes.pyplugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/nemo-deployments/tests/unit/backends/docker/test_backend_mocked.py
- plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py (1)
465-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the undecidable liveness branch.
At Line 465,
_exec_events(0)includes an exit event, so liveness receives a definitive success. The test does not exercise the no-exit (None) path that liveness must fail open on. (raw.githubusercontent.com)Use
[stdout_only]for the liveness entry, then keep the existing readiness and second-poll assertions.Suggested test adjustment
_exec_events(0), # marker present - _exec_events(0), # liveness: alive + [stdout_only], # liveness: no exit event; fail open [stdout_only], # readiness: no exit event🤖 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/openshell/test_openshell_backend_mocked.py` at line 465, Update the liveness test entry using _exec_events so it supplies stdout-only output without an exit event, exercising the undecidable None liveness path and its fail-open behavior. Keep the existing readiness assertion and second-poll assertions unchanged.Source: MCP tools
🤖 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 @.pi/tasks/session-83576-83576/b744c26ca.json:
- Line 4: Update the polling loop using latest and newinline so it breaks on any
newer issue comment or inline review comment, and also detect newer submitted
reviews during polling. Remove error suppression from all gh calls so polling
failures are visible, and make the command exit nonzero when all 20 polls
complete without detected activity.
- Line 4: Update the polling command around the `gh api` calls to propagate API
failures instead of converting them into empty results: enable `set -o
pipefail`, remove the `2>/dev/null` suppression, and add explicit status checks
for each API request so the loop exits or fails when polling cannot complete.
Ensure the final review-summary and unresolved-thread commands also preserve
failures rather than allowing `tail` to mask them.
---
Nitpick comments:
In
`@plugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py`:
- Line 465: Update the liveness test entry using _exec_events so it supplies
stdout-only output without an exit event, exercising the undecidable None
liveness path and its fail-open behavior. Keep the existing readiness assertion
and second-poll assertions unchanged.
🪄 Autofix
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: 1569399e-b805-4b55-8a57-db4020a40d43
📒 Files selected for processing (5)
.pi/tasks/session-83576-83576/b744c26ca.json.pi/tasks/session-83576-83576/b744c26ca.output.pi/tasks/session-83576-83576/b8de4a85b.json.pi/tasks/session-83576-83576/b8de4a85b.outputplugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py
maxdubrinsky
force-pushed
the
mdubrinsky/aircore-998-sandboxed-deployment-reports-ready-before-the-workload-can
branch
from
August 6, 2026 22:21
dd8d75b to
58e96be
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
mckornfield
approved these changes
Aug 6, 2026
Contributor
|
maxdubrinsky
force-pushed
the
mdubrinsky/aircore-998-sandboxed-deployment-reports-ready-before-the-workload-can
branch
2 times, most recently
from
August 7, 2026 18:54
548580c to
c63e369
Compare
maxdubrinsky
enabled auto-merge
August 7, 2026 18:55
Both the openshell and docker backends published READY on a signal that does not prove the workload is serving. openshell exposed a port as soon as the serve pid was alive; docker treated a running container with no declared readinessProbe as ready. A live pid or a running container has not necessarily bound its socket, so a caller trusting READY hit repeated 502s for the roughly 14s until nat serve finished starting. Gate READY on the port actually accepting a connection. Honour a declared readinessProbe (httpGet/tcpSocket/exec); with none declared, default to a loopback TCP connect on the primary container port. A not-yet-reachable workload reports STARTING, so the reconciler's existing starting-timeout acts as the k8s-style progress deadline that eventually fails a workload that never binds. Death detection is unchanged. openshell probes inside the sandbox against 127.0.0.1 so readiness does not depend on the gateway route, and exposing only once reachable keeps the fast path's READY sticky (no flapping). docker changes only the no-declared-probe branch; portless and declared-probe paths are untouched. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
…-998) The openshell readiness gate treated an undecidable probe (no exit event) the same as success, so a None exit promoted the workload to sticky READY and exposed its port. Readiness is the inverse of liveness: liveness fails open so a flaky RPC never demotes a healthy deployment, but readiness must require positive proof of reachability (exit 0) before exposing. A transient no-exit-event now stays STARTING and self-heals on the next poll; timeouts and RPC errors already surface as UNKNOWN upstream, not as a None exit. Also harden the docker not-ready port tests: hold the probe port bound-but-not-listening for the whole probe so nothing else can bind and listen on it mid-test, while a connect still gets ECONNREFUSED. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
The no-exit readiness test claimed the gate self-heals on the next poll but only asserted the first poll. Drive read_status twice (undecidable then reachable) and assert the second poll exposes the port and reads READY, so the retry path the fix relies on is actually exercised. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Address review feedback on the openshell backend: reformat the readiness fail-closed rationale as a bulleted exit-code table, and cut the _exec_detached docstring's narration down to the non-obvious facts (the None exit-code semantics and the timeout rationale) instead of restating what the code does. Also satisfy ty in the openshell readiness tests: construct Probe via pydantic aliases (httpGet/tcpSocket), since ty synthesizes __init__ from aliases and does not honor populate_by_name, and route _readiness_probe_command unpacks through a helper that narrows its tuple|None return. Test-only; no behavior change. Rename test_openshell_backend_mocked.py -> test_backend.py to match the k8s unit backend test; the unit/ vs integration/ tree already conveys mocked vs live, so the _mocked suffix was redundant. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
maxdubrinsky
force-pushed
the
mdubrinsky/aircore-998-sandboxed-deployment-reports-ready-before-the-workload-can
branch
from
August 7, 2026 22:55
c63e369 to
7b252f7
Compare
maxdubrinsky
deleted the
mdubrinsky/aircore-998-sandboxed-deployment-reports-ready-before-the-workload-can
branch
August 7, 2026 23:26
This was referenced Aug 10, 2026
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
3 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
Deployments reported
READYbefore the workload had bound its port. On the openshell backend a port was exposed (which reads as READY) as soon as the serve pid was alive; on docker a running container with no declaredreadinessProbewas treated as READY. A live pid or a running container has not necessarily calledbind(), so a caller trusting READY hit repeated502s for the roughly 14s untilnat servefinished starting. This gates READY on the port actually accepting a connection.Related Issue
AIRCORE-998.
Changes
127.0.0.1:<port>(gateway-independent) before exposing a port. Docker: TCP-connect the published port in the no-declared-probe branch ofcheck_readiness_probe. Both honour a declaredreadinessProbe(httpGet/tcpSocket/exec) and otherwise default to a TCP connect; portless workloads stay ready.STARTING, so the reconciler's existingstarting_timeout_secondsis the progress deadline that eventually fails a never-binding workload. No new deadline, knob, or status; death detection unchanged. Exposing only once reachable keeps the openshellendpoints -> READYfast path sticky (no flapping).Type of Change
Quality Gates
deploy-sandboxSKILL text that documents the 502-retry workaround is tracked to follow via AIRCORE-981.Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
pre-commit run --files <changed files>: ruff, ruff format, ty, copyright headers, plugin-import guard, and merge-conflict hooks all pass; the remaining hooks skip as not-applicable. Run scoped to the changed files rather than-abecause a full workspace sync could not be built in this worktree (a transitive dependency needsPython.h); the applicable hooks are the ones exercised above.pytest plugins/nemo-deployments/tests/unit/backends/{openshell,docker}: 101 passed.plugins/nemo-deployments/tests/unit: passes excepttest_ports.py::test_find_available_port_excludes_pending_assignments, a pre-existing real-socket flake that reproduces identically on unmodifiedmain(host port 9000 is occupied on the dev box), unrelated to this change.Summary by CodeRabbit