From 4692a92c19e66e1b4a459a57f31be4dee5a7b39e Mon Sep 17 00:00:00 2001 From: didericis Date: Mon, 13 Jul 2026 17:00:18 -0400 Subject: [PATCH] =?UTF-8?q?feat(orchestrator):=20slice=205=20=E2=80=94=20b?= =?UTF-8?q?uild=20the=20consolidated=20sidecar=20bundle=20image=20(#352)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Answers "where do we build the consolidated sidecar": nowhere, until now. * sidecar.py — `Sidecar.ensure_built()` (default no-op) + `DockerSidecar` now defaults its image to the real bundle (`bot-bottle-sidecars`) and `ensure_built()` builds it from `Dockerfile.sidecars` when `docker image inspect` shows it's missing (no-op when present or when no dockerfile is configured, e.g. a pre-pulled image). `image_exists()` added. * service.py — `ensure_sidecar()` now builds then runs. * __main__.py — `--sidecar` runs the consolidated bundle (build-if-missing). Scope note: this builds + launches the bundle *container*; making the running instance functional across bottles needs the per-bottle, source-IP-keyed multi-tenant config + registration/reload, and routing agent bottles to it — the next slices (added to PRD 0070's roadmap). Tests: unit (docker mocked) — image_exists, ensure_built builds when missing / no-op when present / no-op without a dockerfile / raises on build failure; ensure_sidecar builds-then-runs; integration (gated, no heavy build) — image_exists reflects real docker state. Full suite green (only pre-existing /bin/sleep errors). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01WBMWTEtQdJ4W5UrWuLHCck --- bot_bottle/orchestrator/__main__.py | 9 +-- bot_bottle/orchestrator/service.py | 5 +- bot_bottle/orchestrator/sidecar.py | 61 +++++++++++++++++-- .../test_orchestrator_docker_sidecar_build.py | 36 +++++++++++ tests/unit/test_orchestrator_service.py | 7 ++- tests/unit/test_orchestrator_sidecar.py | 50 +++++++++++++++ 6 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 tests/integration/test_orchestrator_docker_sidecar_build.py diff --git a/bot_bottle/orchestrator/__main__.py b/bot_bottle/orchestrator/__main__.py index acac5c9..a323afe 100644 --- a/bot_bottle/orchestrator/__main__.py +++ b/bot_bottle/orchestrator/__main__.py @@ -38,8 +38,8 @@ def main(argv: list[str] | None = None) -> int: help="launch broker: 'stub' records requests; 'docker' runs containers", ) parser.add_argument( - "--sidecar-image", default=None, - help="if set, run one consolidated per-host sidecar from this image", + "--sidecar", action="store_true", + help="run one consolidated per-host sidecar bundle (build-if-missing)", ) args = parser.parse_args(argv) @@ -51,10 +51,11 @@ def main(argv: list[str] | None = None) -> int: # anything; 'docker' runs real containers (firecracker drops in later). secret = secrets.token_bytes(32) broker: LaunchBroker = DockerBroker(secret) if args.broker == "docker" else StubBroker(secret) - sidecar: Sidecar | None = DockerSidecar(args.sidecar_image) if args.sidecar_image else None + sidecar: Sidecar | None = DockerSidecar() if args.sidecar else None orchestrator = Orchestrator(registry, broker, secret, sidecar) - # One persistent per-host sidecar, shared by every bottle (idempotent). + # One persistent per-host sidecar, shared by every bottle: build the + # bundle image if missing, then bring the singleton up (idempotent). if sidecar is not None: orchestrator.ensure_sidecar() log.info("consolidated sidecar ensured", context={"name": sidecar.name}) diff --git a/bot_bottle/orchestrator/service.py b/bot_bottle/orchestrator/service.py index 01667f3..3b2aca7 100644 --- a/bot_bottle/orchestrator/service.py +++ b/bot_bottle/orchestrator/service.py @@ -83,9 +83,10 @@ class Orchestrator: # --- consolidated sidecar ---------------------------------------------- def ensure_sidecar(self) -> None: - """Bring the single per-host sidecar up (idempotent). No-op when no - sidecar is configured.""" + """Ensure the single per-host sidecar is built and up (idempotent). + No-op when no sidecar is configured.""" if self._sidecar is not None: + self._sidecar.ensure_built() self._sidecar.ensure_running() def sidecar_status(self) -> dict[str, object]: diff --git a/bot_bottle/orchestrator/sidecar.py b/bot_bottle/orchestrator/sidecar.py index 4def186..5eeb18a 100644 --- a/bot_bottle/orchestrator/sidecar.py +++ b/bot_bottle/orchestrator/sidecar.py @@ -18,15 +18,25 @@ launches never spawn N sidecars. from __future__ import annotations import abc +import os +from pathlib import Path from ..docker_cmd import run_docker SIDECAR_NAME = "bot-bottle-orch-sidecar" SIDECAR_LABEL = "bot-bottle-orch-sidecar=1" +# The real sidecar-bundle image + its Dockerfile. Kept as a local constant +# rather than imported from backend.docker.sidecar_bundle, which would drag +# the whole backend layer into the lean orchestrator (see #359); unify when +# that lands. Env override matches the backend's BOT_BOTTLE_SIDECAR_IMAGE. +SIDECAR_BUNDLE_IMAGE = os.environ.get("BOT_BOTTLE_SIDECAR_IMAGE", "bot-bottle-sidecars:latest") +SIDECAR_DOCKERFILE = "Dockerfile.sidecars" +_REPO_ROOT = Path(__file__).resolve().parents[2] + class SidecarError(Exception): - """The shared sidecar failed to start/stop (non-zero `docker` exit).""" + """The shared sidecar failed to build/start/stop (non-zero `docker` exit).""" class Sidecar(abc.ABC): @@ -34,10 +44,16 @@ class Sidecar(abc.ABC): name: str + def ensure_built(self) -> None: + """Ensure the sidecar's image / rootfs exists, building it if needed. + Default: nothing to build (e.g. a stub or a pre-pulled image).""" + return + @abc.abstractmethod def ensure_running(self) -> None: """Start the sidecar if it isn't already up. Idempotent: a no-op - when it's already running (that's the whole point — one per host).""" + when it's already running (that's the whole point — one per host). + Assumes the image exists — call `ensure_built()` first.""" @abc.abstractmethod def is_running(self) -> bool: @@ -49,11 +65,43 @@ class Sidecar(abc.ABC): class DockerSidecar(Sidecar): - """The consolidated sidecar as a single, fixed-name Docker container.""" + """The consolidated sidecar as a single, fixed-name Docker container. - def __init__(self, image_ref: str, *, name: str = SIDECAR_NAME) -> None: + `image_ref` defaults to the real sidecar-bundle image; `ensure_built` + builds it from `Dockerfile.sidecars` when it's missing. (Note: slice 5 + builds + launches the bundle container; wiring its per-bottle, + source-IP-keyed config is a later slice — see PRD 0070.)""" + + def __init__( + self, + image_ref: str = SIDECAR_BUNDLE_IMAGE, + *, + name: str = SIDECAR_NAME, + build_context: Path | None = None, + dockerfile: str | None = SIDECAR_DOCKERFILE, + ) -> None: self.image_ref = image_ref self.name = name + self._build_context = build_context or _REPO_ROOT + self._dockerfile = dockerfile + + def image_exists(self) -> bool: + return run_docker(["docker", "image", "inspect", self.image_ref]).returncode == 0 + + def ensure_built(self) -> None: + """Build the bundle image from its Dockerfile when it's missing. + No-op when the image is already present, or when no dockerfile is + configured (e.g. a pre-pulled image).""" + if self._dockerfile is None or self.image_exists(): + return + proc = run_docker([ + "docker", "build", + "-t", self.image_ref, + "-f", str(self._build_context / self._dockerfile), + str(self._build_context), + ]) + if proc.returncode != 0: + raise SidecarError(f"sidecar image build failed: {proc.stderr.strip()}") def is_running(self) -> bool: proc = run_docker([ @@ -85,4 +133,7 @@ class DockerSidecar(Sidecar): raise SidecarError(f"sidecar failed to stop: {proc.stderr.strip()}") -__all__ = ["Sidecar", "DockerSidecar", "SidecarError", "SIDECAR_NAME", "SIDECAR_LABEL"] +__all__ = [ + "Sidecar", "DockerSidecar", "SidecarError", + "SIDECAR_NAME", "SIDECAR_LABEL", "SIDECAR_BUNDLE_IMAGE", +] diff --git a/tests/integration/test_orchestrator_docker_sidecar_build.py b/tests/integration/test_orchestrator_docker_sidecar_build.py new file mode 100644 index 0000000..e677100 --- /dev/null +++ b/tests/integration/test_orchestrator_docker_sidecar_build.py @@ -0,0 +1,36 @@ +"""Integration: DockerSidecar.image_exists reflects real docker state. + +Gated on a reachable Docker daemon. Deliberately does NOT build the full +sidecar bundle (a heavy, slow image build) — it exercises the `image_exists` +primitive that `ensure_built` gates on, against a tiny pulled image and a +name that can't exist. +""" + +from __future__ import annotations + +import subprocess +import unittest + +from bot_bottle.orchestrator.sidecar import DockerSidecar +from tests._docker import skip_unless_docker + +IMAGE = "busybox" + + +@skip_unless_docker() +class TestDockerSidecarImageExists(unittest.TestCase): + def test_image_exists_true_for_present_false_for_absent(self) -> None: + # Ensure the tiny image is present (build_if_missing is disabled here + # so this never triggers a Dockerfile.sidecars build). + subprocess.run( + ["docker", "pull", IMAGE], + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False, + ) + self.assertTrue(DockerSidecar(IMAGE, dockerfile=None).image_exists()) + self.assertFalse( + DockerSidecar("bot-bottle-nonexistent:doesnotexist", dockerfile=None).image_exists() + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_orchestrator_service.py b/tests/unit/test_orchestrator_service.py index f6bcd82..d02927c 100644 --- a/tests/unit/test_orchestrator_service.py +++ b/tests/unit/test_orchestrator_service.py @@ -30,8 +30,12 @@ class _FakeSidecar(Sidecar): def __init__(self) -> None: self.name = "fake-sidecar" self.ensured = 0 + self.built = 0 self._running = False + def ensure_built(self) -> None: + self.built += 1 + def ensure_running(self) -> None: self.ensured += 1 self._running = True @@ -100,7 +104,8 @@ class TestOrchestrator(unittest.TestCase): orch.sidecar_status(), ) orch.ensure_sidecar() - self.assertEqual(1, sc.ensured) + self.assertEqual(1, sc.built) # ensure_sidecar builds first, + self.assertEqual(1, sc.ensured) # then runs self.assertEqual( {"configured": True, "name": "fake-sidecar", "running": True}, orch.sidecar_status(), diff --git a/tests/unit/test_orchestrator_sidecar.py b/tests/unit/test_orchestrator_sidecar.py index 4992193..30fd294 100644 --- a/tests/unit/test_orchestrator_sidecar.py +++ b/tests/unit/test_orchestrator_sidecar.py @@ -75,5 +75,55 @@ class TestDockerSidecar(unittest.TestCase): self.sc.stop() +class TestDockerSidecarBuild(unittest.TestCase): + def setUp(self) -> None: + self.sc = DockerSidecar() # defaults to the real bundle image + dockerfile + + def test_image_exists_reads_docker_inspect(self) -> None: + with patch(_RUN_DOCKER, return_value=_proc(returncode=0)): + self.assertTrue(self.sc.image_exists()) + with patch(_RUN_DOCKER, return_value=_proc(returncode=1)): + self.assertFalse(self.sc.image_exists()) + + def test_ensure_built_is_noop_when_image_present(self) -> None: + with patch(_RUN_DOCKER, return_value=_proc(returncode=0)) as m: + self.sc.ensure_built() + self.assertEqual(1, m.call_count) # only the image-inspect probe + + def test_ensure_built_builds_from_dockerfile_when_missing(self) -> None: + calls: list[list[str]] = [] + + def fake(argv: list[str]) -> Mock: + calls.append(argv) + if argv[:3] == ["docker", "image", "inspect"]: + return _proc(returncode=1) # missing + return _proc() + + with patch(_RUN_DOCKER, side_effect=fake): + self.sc.ensure_built() + + builds = [c for c in calls if c[:2] == ["docker", "build"]] + self.assertEqual(1, len(builds)) + self.assertIn(self.sc.image_ref, builds[0]) + self.assertIn("-f", builds[0]) + self.assertTrue(any(a.endswith("Dockerfile.sidecars") for a in builds[0])) + + def test_ensure_built_noop_when_no_dockerfile(self) -> None: + sc = DockerSidecar("busybox", dockerfile=None) + with patch(_RUN_DOCKER) as m: + sc.ensure_built() + m.assert_not_called() + + def test_ensure_built_raises_on_build_failure(self) -> None: + def fake(argv: list[str]) -> Mock: + if argv[:3] == ["docker", "image", "inspect"]: + return _proc(returncode=1) + return _proc(returncode=1, stderr="build boom") + + with patch(_RUN_DOCKER, side_effect=fake): + with self.assertRaises(SidecarError): + self.sc.ensure_built() + + if __name__ == "__main__": unittest.main()