feat(cluster): rolling 'update all workers' — drain one-at-a-time fleet orchestration#1878
Conversation
…et orchestration Adds POST /api/cluster/workers/update-all: updates every online remote worker sequentially, draining and updating at most one worker at any moment so the fleet never fully drains. Skips the local worker. Fault-tolerant: one worker's failure continues the roll. Refactors the single-worker update logic into _do_single_worker_update shared helper, keeping identical drain→deploy→restart semantics. Closes jaylfc#1876.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughChangesCluster rolling updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant update_all_workers
participant _do_single_worker_update
participant ClusterManager
Admin->>update_all_workers: POST update-all
loop online non-local workers
update_all_workers->>_do_single_worker_update: update one worker
_do_single_worker_update->>ClusterManager: drain worker and deploy update
_do_single_worker_update-->>update_all_workers: success or failure
update_all_workers->>ClusterManager: poll re-registration
end
update_all_workers-->>Admin: aggregate results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| updated: list[str] = [] | ||
| failed: list[dict] = [] | ||
| skipped: list[str] = [ |
There was a problem hiding this comment.
WARNING: skipped is computed once up front and includes any worker whose status is not online — this also catches workers that are currently draining. A worker that was already in the middle of a drain (e.g. from another admin action, or two concurrent update-all calls) will be silently skipped rather than updated, and it is never added to updated/failed. The docstring/comment says skipped is "only for workers that were never online," but a draining worker was online. Consider also excluding draining from skipped (or explicitly documenting that mid-drain workers are intentionally skipped), otherwise an admin's fleet update can silently leave a worker untouched.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| worker.name, result.get("error", "unknown"), | ||
| ) | ||
|
|
||
| # Re-read latest statuses after the roll for accurate skipped count |
There was a problem hiding this comment.
SUGGESTION: This comment block is stale/misleading — it says "Re-read latest statuses after the roll for accurate skipped count," but the code does not re-read any statuses; skipped was already fully computed at lines 1332-1335 before the loop. Either remove this comment or actually re-compute skipped from a fresh cluster.get_workers() after the roll if you want post-roll accuracy.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/routes/cluster.py (1)
1207-1253: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent one unexpected helper exception from aborting the fleet rollout.
The helper’s “never raises” contract does not cover
drain_workeror failingcancel_draincalls. Any such exception escapes the update-all loop and stops remaining workers from being processed.
tinyagentos/routes/cluster.py#L1207-L1253: catch orchestration and cancellation failures and return structured errors.tinyagentos/routes/cluster.py#L1337-L1342: add per-worker exception isolation as a final safeguard.tests/test_routes_cluster.py#L634-L654: make the mocked helper raise and verify later workers still run.🤖 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 `@tinyagentos/routes/cluster.py` around lines 1207 - 1253, Prevent rollout interruption when worker orchestration helpers raise: in tinyagentos/routes/cluster.py lines 1207-1253, wrap drain_worker and cancel_drain failures in structured error handling, including cancellation failures, while preserving the existing deploy outcomes; in tinyagentos/routes/cluster.py lines 1337-1342, add final per-worker exception isolation so one failure does not stop later workers; in tests/test_routes_cluster.py lines 634-654, make the mocked helper raise and assert subsequent workers are still processed.
🤖 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 `@tinyagentos/routes/cluster.py`:
- Around line 1325-1335: In tinyagentos/routes/cluster.py lines 1325-1335,
compute skipped workers before the no-target early return and include
total_targets: 0 in that response, preserving local and offline worker names. In
tests/test_routes_cluster.py lines 708-716, register local and offline workers
and assert they are returned in skipped.
- Around line 1299-1304: Update _do_single_worker_update and its caller so the
helper returns whether cancel_drain was actually performed, including the error
path where it exits before cancellation. In the route’s unsuccessful result
handling, set drain_cancelled from that returned state instead of always
reporting true.
- Around line 1337-1340: The rollout loop around _do_single_worker_update must
wait for each worker’s explicit re-registration/online completion signal, with a
timeout, before processing the next target. Update _do_single_worker_update
around tinyagentos/routes/cluster.py lines 1222-1240 to expose state
distinguishing accepted from completed, use that state in the loop at lines
1337-1340, and update tests/test_routes_cluster.py lines 616-631 to track active
drains and assert the maximum remains one.
---
Outside diff comments:
In `@tinyagentos/routes/cluster.py`:
- Around line 1207-1253: Prevent rollout interruption when worker orchestration
helpers raise: in tinyagentos/routes/cluster.py lines 1207-1253, wrap
drain_worker and cancel_drain failures in structured error handling, including
cancellation failures, while preserving the existing deploy outcomes; in
tinyagentos/routes/cluster.py lines 1337-1342, add final per-worker exception
isolation so one failure does not stop later workers; in
tests/test_routes_cluster.py lines 634-654, make the mocked helper raise and
assert subsequent workers are still processed.
🪄 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: Pro Plus
Run ID: 7d113a8d-0197-43a6-b6eb-0a4ac7c86a0b
📒 Files selected for processing (2)
tests/test_routes_cluster.pytinyagentos/routes/cluster.py
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Resolved Since Last Review
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 49e15b4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 49e15b4)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Resolved Since Last Review
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 84d3d3d)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by hy3:free · Input: 82.3K · Output: 3.4K · Cached: 206K |
…aylfc#1878) Addresses all Kilo and CodeRabbit findings: - Kilo WARNING: exclude 'draining' workers from 'skipped' list - Kilo SUGGESTION: remove stale comment about re-reading statuses - CodeRabbit Major: per-worker exception isolation in _do_single_worker_update (wrap drain_worker + cancel_drain in try/except) - CodeRabbit inline: compute skipped before no-target early return, include skipped list + total_targets:0 in empty response - CodeRabbit inline: return accurate drain_cancelled bool from helper instead of hardcoding True in update_worker route - CodeRabbit inline: re-registration poll loop with timeout (300s) after each successful update, ensuring truly one-at-a-time rollout Tests: 12/12 pass (3 existing + 9 update-all tests, including 3 new)
|
|
||
| workers = cluster.get_workers() | ||
| targets = [w for w in workers if w.status == "online" and w.name != "local"] | ||
| skipped: list[str] = [ |
There was a problem hiding this comment.
SUGGESTION: draining workers are silently dropped from the entire response.
targets only includes workers with status == "online" (line 1362), and skipped now explicitly excludes draining (line 1365). As a result, a worker that is draining when update-all is called appears in none of updated, failed, skipped, and is not counted in total_targets. The admin gets no indication that such a worker exists or was left untouched.
The docstring says draining workers "are likely mid-update from a prior action," but omitting them entirely from the aggregate response is an observability gap — an admin running a fleet update cannot tell that a worker was intentionally left out. Consider surfacing mid-drain workers in a distinct bucket (e.g. in_progress/draining) or documenting in the response why they were excluded, rather than dropping them from all output lists.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
House convention: no em dashes in repo text. Scoped to the lines this PR adds/changes; pre-existing dashes elsewhere are left for a separate sweep. Maintainer polish on @hognek's rolling update-all-workers.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_routes_cluster.py (2)
932-942: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd an authenticated non-admin rejection case.
An unauthenticated request only verifies authentication. Add a valid non-admin session and assert rejection so removing the
is_admincheck cannot pass this suite.🤖 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 `@tests/test_routes_cluster.py` around lines 932 - 942, Add an authenticated non-admin test case alongside test_update_all_workers_admin_gate_rejected, using the existing test fixture or helper for creating a valid non-admin session, then call /api/cluster/workers/update-all and assert it returns 401 or 403. Keep the current unauthenticated coverage and ensure the request reaches the admin authorization check.
645-669: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert polling occurs before the next worker update.
call_order == ["w1", "w2"]still passes if both updates begin before re-registration polling. Record update and poll/sleep events, then assert thatw1re-registers—or exhausts its timeout—before_mock_doreceivesw2. This directly protects the one-worker-at-a-time requirement.Also applies to: 915-929
🤖 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 `@tests/test_routes_cluster.py` around lines 645 - 669, Strengthen the sequential-update test around `_mock_do`, `_fake_sleep`, and `_get_worker` by recording both worker-update and polling/sleep events. Assert that w1’s re-registration or timeout exhaustion occurs before `_mock_do` is invoked for w2, rather than only asserting update call order; apply the same verification to the corresponding test block near the second referenced section.
♻️ Duplicate comments (1)
tinyagentos/routes/cluster.py (1)
1393-1416: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDo not advance the rollout after re-registration times out.
The timed-out worker may still be draining or updating, so continuing can take another worker out of rotation and violate the one-at-a-time guarantee. Only add it to
updatedafter it returns online; otherwise report the timeout and stop until a non-active state is confirmed.🤖 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 `@tinyagentos/routes/cluster.py` around lines 1393 - 1416, Move the updated.append(worker.name) call in the rollout flow so it executes only after the worker is confirmed online by the re-registration polling loop. In the timeout branch of the loop around RE_REGISTER_TIMEOUT, report the timeout and stop advancing the rollout until the worker reaches a confirmed non-active state; do not proceed to the next target or mark the timed-out worker updated.
🤖 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.
Outside diff comments:
In `@tests/test_routes_cluster.py`:
- Around line 932-942: Add an authenticated non-admin test case alongside
test_update_all_workers_admin_gate_rejected, using the existing test fixture or
helper for creating a valid non-admin session, then call
/api/cluster/workers/update-all and assert it returns 401 or 403. Keep the
current unauthenticated coverage and ensure the request reaches the admin
authorization check.
- Around line 645-669: Strengthen the sequential-update test around `_mock_do`,
`_fake_sleep`, and `_get_worker` by recording both worker-update and
polling/sleep events. Assert that w1’s re-registration or timeout exhaustion
occurs before `_mock_do` is invoked for w2, rather than only asserting update
call order; apply the same verification to the corresponding test block near the
second referenced section.
---
Duplicate comments:
In `@tinyagentos/routes/cluster.py`:
- Around line 1393-1416: Move the updated.append(worker.name) call in the
rollout flow so it executes only after the worker is confirmed online by the
re-registration polling loop. In the timeout branch of the loop around
RE_REGISTER_TIMEOUT, report the timeout and stop advancing the rollout until the
worker reaches a confirmed non-active state; do not proceed to the next target
or mark the timed-out worker updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ead6afeb-c0cc-4dca-a0d2-46a99a71fba2
📒 Files selected for processing (2)
tests/test_routes_cluster.pytinyagentos/routes/cluster.py
2 participants
Closes #1876.
What
Adds
POST /api/cluster/workers/update-all— one admin action to update the entire worker fleet without taking the cluster down. Updates every online remote worker sequentially, draining and updating at most one worker at any moment (Umbrel-style 'update all').How
update_workerinto shared_do_single_worker_update(cluster, worker)helperPOST /api/cluster/workers/update-all(admin-gated, same_require_admingate)localworker, only targetsonlineremote workers{updated: [...], failed: [{name, error}], skipped: [...]}Tests (9/9 pass local, Python 3.11)
Summary by CodeRabbit
POST /api/cluster/workers/update-allto perform a sequential rolling update of eligible workers.updated,failed, andskippedoutcomes, excludinglocaland offline workers.POST /api/cluster/workers/{name}/updateto use the full drain/deploy flow and return consistent failure details.