feat: change a lane's slots on a running system, over the broker (4292)
CI / lint (push) Successful in 2s
CI / extension-version (push) Successful in 2s
Build images / sign-extension (push) Successful in 4s
Build images / build-agent (push) Successful in 6s
CI / frontend-build (push) Successful in 25s
CI / backend-lint-and-test (push) Successful in 31s
Build images / build-web (push) Successful in 55s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 1m41s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m8s
CI / lint (push) Successful in 2s
CI / extension-version (push) Successful in 2s
Build images / sign-extension (push) Successful in 4s
Build images / build-agent (push) Successful in 6s
CI / frontend-build (push) Successful in 25s
CI / backend-lint-and-test (push) Successful in 31s
Build images / build-web (push) Successful in 55s
Build images / smoke-web (push) Skipped
Build images / build-ml (push) Successful in 1m41s
Build images / promote (push) Skipped
CI / integration (push) Successful in 2m8s
Milestone 422 step 2. `GET /api/system/workers` reports every lane joined to its live pool; `POST /api/system/workers/<name>` changes it. NO DOCKER SOCKET. Milestone 365 deferred "acting on the state" because restarting a dead worker needs a socket the web container deliberately does not have. That holds for restarting a CONTAINER; it does not hold for changing how much work a RUNNING worker does. celery's pool_grow / pool_shrink / add_consumer / cancel_consumer send a message over the Redis the app already uses, and the worker resizes itself. No new privilege, no new surface, and the security question that deferred this is never raised. PERSIST AND PUSH, in one call, in that order. pool_grow is not durable — a restart drops every lane to its env concurrency — so a UI that only pushed would lose the setting on the next deploy with nothing to show for it (lesson #4202). Storing alone would describe nothing until something restarted. A failed PUSH is not a failed setting: 200 with `applied: false` and a reason, so the UI says "saved, not yet live" rather than "that didn't work". Step 3's reconcile carries it when the lane answers again. PER-REPLICA DELTAS. `pool_grow(n, destination=[...])` adds n to EACH destination, so while `worker` runs `replicas: 2` a single delta from an aggregate is wrong for both. `slots` therefore means what CELERY_CONCURRENCY means — one process's pool — and each replica is driven to it from its OWN current size, so replicas that drifted apart converge rather than moving in lockstep. I wrote this wrong first: the docstring claimed per-replica while the code computed one delta from the max across replicas. LaneLiveState now carries `pools` per hostname and exposes `pool` as a property. A replica already at the target is sent nothing at all — the reachable fixed point step 3's periodic reconcile needs, or it re-issues a grow of zero every tick forever (lesson #4183). A replica that answered inspect but not stats is NAMED in the error rather than skipped silently, since otherwise it would run at a size the UI claims it does not. `present=False` is not "zero slots", it is "nothing answered" — kept distinct throughout, because step 3 skips an absent lane rather than correcting it. /workers now also reports pool size (from `insp.stats()`) and RESERVED count. Celery prefetches, so tasks that have left the Redis list but not started are invisible to LLEN: a lane can read depth 0 with thirty tasks held in worker memory. `pending` is depth + reserved. The UI is misleading without this and step 7's autoscaler would be simply wrong. Also kills the THIRD copy of the queue list: system_activity's _QUEUE_NAMES, whose own comment admitted the coupling ("must match celery_app.task_routes") and which sat alongside task_routes and the ROLE_NAMES copy step 1 collapsed. Now derived from LANES. The rendered order changes to lane grouping, which is the better shape for a lane-oriented UI. Separate blueprint rather than folding into system_activity, which states in its first line that it is read-only and answers a different question — its /workers is keyed on celery HOSTNAME and reports which nodes answered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LVjrnpQjRgHdvq95rASoiR
This commit is contained in:
@@ -0,0 +1,188 @@
|
||||
"""Changing a lane's slots on a running system (milestone 422 step 2).
|
||||
|
||||
The celery control calls are stubbed: what is being tested is the DELTA
|
||||
ARITHMETIC and the persist/push split, not that celery can resize its own
|
||||
pool. A test that asserted celery's behaviour would be testing celery.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
|
||||
from backend.app.services import worker_control as wc
|
||||
from backend.app.services.worker_lanes import LANES_BY_NAME
|
||||
|
||||
# --- what inspect reports ----------------------------------------------------
|
||||
|
||||
|
||||
class _Control:
|
||||
"""Records the control messages that were sent."""
|
||||
|
||||
def __init__(self):
|
||||
self.grew: list[tuple[int, list[str]]] = []
|
||||
self.shrank: list[tuple[int, list[str]]] = []
|
||||
self.added: list[tuple[str, list[str]]] = []
|
||||
self.cancelled: list[tuple[str, list[str]]] = []
|
||||
|
||||
def pool_grow(self, n, destination=None):
|
||||
self.grew.append((n, destination))
|
||||
|
||||
def pool_shrink(self, n, destination=None):
|
||||
self.shrank.append((n, destination))
|
||||
|
||||
def add_consumer(self, queue, destination=None):
|
||||
self.added.append((queue, destination))
|
||||
|
||||
def cancel_consumer(self, queue, destination=None):
|
||||
self.cancelled.append((queue, destination))
|
||||
|
||||
|
||||
def _stub_live(monkeypatch, lane_name, *, pools, present=True, reserved=0):
|
||||
state = wc.LaneLiveState(
|
||||
present=present,
|
||||
replicas=len(pools),
|
||||
hostnames=sorted(pools),
|
||||
pools=dict(pools),
|
||||
reserved=reserved,
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
wc, "inspect_lanes_sync",
|
||||
lambda: {name: (state if name == lane_name else wc.LaneLiveState())
|
||||
for name in LANES_BY_NAME},
|
||||
)
|
||||
return state
|
||||
|
||||
|
||||
def _stub_control(monkeypatch):
|
||||
control = _Control()
|
||||
|
||||
class _Celery:
|
||||
pass
|
||||
|
||||
celery = _Celery()
|
||||
celery.control = control
|
||||
import sys
|
||||
import types
|
||||
mod = types.ModuleType("backend.app.celery_app")
|
||||
mod.celery = celery
|
||||
monkeypatch.setitem(sys.modules, "backend.app.celery_app", mod)
|
||||
return control
|
||||
|
||||
|
||||
def test_pool_property_is_max_not_sum():
|
||||
"""`slots` means the pool size of ONE process, so the aggregate shown to
|
||||
the operator is the largest replica — not the total. A sum would report 8
|
||||
for two replicas of 4 and invite them to 'reduce it to 4', which would
|
||||
halve the lane."""
|
||||
state = wc.LaneLiveState(pools={"a": 4, "b": 4})
|
||||
assert state.pool == 4
|
||||
|
||||
|
||||
def test_pool_is_none_when_nothing_reported():
|
||||
"""Unknown, never zero — the distinction step 3's reconcile depends on."""
|
||||
assert wc.LaneLiveState(present=True).pool is None
|
||||
|
||||
|
||||
# --- the delta arithmetic ----------------------------------------------------
|
||||
|
||||
|
||||
def test_each_replica_gets_its_own_delta(monkeypatch):
|
||||
"""The bug this exists to prevent: one delta computed from an aggregate
|
||||
and applied to every replica. With replicas at 2 and 6 and a target of 4,
|
||||
a shared delta moves both the same way and leaves them at 4 and 8 — or 0
|
||||
and 4 — depending on which aggregate was used. Per-replica deltas
|
||||
converge both on 4.
|
||||
"""
|
||||
control = _stub_control(monkeypatch)
|
||||
_stub_live(monkeypatch, "worker", pools={"host-a": 2, "host-b": 6})
|
||||
|
||||
applied, err = wc.set_lane_slots_sync(LANES_BY_NAME["worker"], 4)
|
||||
|
||||
assert (applied, err) == (True, None)
|
||||
assert control.grew == [(2, ["host-a"])]
|
||||
assert control.shrank == [(2, ["host-b"])]
|
||||
|
||||
|
||||
def test_a_replica_already_at_the_target_is_sent_nothing(monkeypatch):
|
||||
"""The fixed point step 3's reconcile needs. An enforcer that re-issues a
|
||||
grow of zero every tick never converges and re-does its own work forever
|
||||
(lesson #4183)."""
|
||||
control = _stub_control(monkeypatch)
|
||||
_stub_live(monkeypatch, "worker", pools={"host-a": 4})
|
||||
|
||||
applied, err = wc.set_lane_slots_sync(LANES_BY_NAME["worker"], 4)
|
||||
|
||||
assert (applied, err) == (True, None)
|
||||
assert control.grew == []
|
||||
assert control.shrank == []
|
||||
|
||||
|
||||
def test_resizing_an_absent_lane_reports_rather_than_raises(monkeypatch):
|
||||
_stub_control(monkeypatch)
|
||||
_stub_live(monkeypatch, "worker", pools={}, present=False)
|
||||
|
||||
applied, err = wc.set_lane_slots_sync(LANES_BY_NAME["worker"], 4)
|
||||
|
||||
assert applied is False
|
||||
assert "not running" in err
|
||||
|
||||
|
||||
def test_a_replica_with_no_reported_pool_is_named_not_skipped_silently(
|
||||
monkeypatch,
|
||||
):
|
||||
"""Resize what can be resized, then say which could not. Silence would
|
||||
leave a replica running at a size the UI claims it is not."""
|
||||
control = _stub_control(monkeypatch)
|
||||
state = _stub_live(monkeypatch, "worker", pools={"host-a": 2})
|
||||
state.hostnames = ["host-a", "host-b"] # b answered inspect, not stats
|
||||
state.replicas = 2
|
||||
|
||||
applied, err = wc.set_lane_slots_sync(LANES_BY_NAME["worker"], 4)
|
||||
|
||||
assert control.grew == [(2, ["host-a"])]
|
||||
assert applied is False
|
||||
assert "host-b" in err
|
||||
|
||||
|
||||
# --- enabling and disabling --------------------------------------------------
|
||||
|
||||
|
||||
def test_disabling_cancels_consumers_rather_than_killing_the_worker(monkeypatch):
|
||||
"""A cancelled consumer keeps the process alive and answering inspect, so
|
||||
a disabled lane stays visible. A killed worker reads as ABSENT, which is
|
||||
the same signal as a crash — and milestone 365 exists precisely so those
|
||||
two do not look alike."""
|
||||
control = _stub_control(monkeypatch)
|
||||
_stub_live(monkeypatch, "ml", pools={"host-a": 1})
|
||||
|
||||
applied, err = wc.set_lane_enabled_sync(LANES_BY_NAME["ml"], False)
|
||||
|
||||
assert (applied, err) == (True, None)
|
||||
assert control.cancelled == [("ml", ["host-a"])]
|
||||
assert control.added == []
|
||||
|
||||
|
||||
def test_enabling_adds_a_consumer_for_every_queue_in_the_lane(monkeypatch):
|
||||
control = _stub_control(monkeypatch)
|
||||
_stub_live(monkeypatch, "worker", pools={"host-a": 1})
|
||||
|
||||
wc.set_lane_enabled_sync(LANES_BY_NAME["worker"], True)
|
||||
|
||||
assert [q for q, _ in control.added] == list(LANES_BY_NAME["worker"].queues)
|
||||
|
||||
|
||||
# --- lane identity -----------------------------------------------------------
|
||||
|
||||
|
||||
def test_queue_sets_map_back_to_their_lane_in_any_order():
|
||||
"""celery does not guarantee the order it lists a worker's queues in, so
|
||||
the lookup sorts. Unsorted, a lane would intermittently fail to match and
|
||||
read as absent."""
|
||||
lane = LANES_BY_NAME["worker"]
|
||||
assert wc._lane_for_queues(tuple(reversed(lane.queues))) is lane
|
||||
|
||||
|
||||
def test_an_unknown_queue_set_maps_to_no_lane():
|
||||
"""A deployment slicing CELERY_QUEUES differently has no lane row to
|
||||
control. Honest rather than an error — the roster still reports it."""
|
||||
assert wc._lane_for_queues(("something", "else")) is None
|
||||
Reference in New Issue
Block a user