diff --git a/src/agent_env/store/image_store/local_registry_image_store.py b/src/agent_env/store/image_store/local_registry_image_store.py index e3f06cc9..b9f91868 100644 --- a/src/agent_env/store/image_store/local_registry_image_store.py +++ b/src/agent_env/store/image_store/local_registry_image_store.py @@ -10,6 +10,13 @@ from agent_env.store.image_store.oci_registry_credentials import OciRegistryCredentials _REGISTRY_CONTAINER = "agentenv-registry" +_DEFAULT_PORT = 5000 + + +def _container_name(port: int) -> str: + """``agentenv-registry`` on the default port, ``agentenv-registry-`` on any other, so + registries on different ports coexist and each restarts with its own images.""" + return _REGISTRY_CONTAINER if port == _DEFAULT_PORT else f"{_REGISTRY_CONTAINER}-{port}" class LocalRegistryImageStore(OciRegistryImageStore): @@ -34,37 +41,34 @@ def ensure_repository(self, repository: str) -> None: """A local registry creates repositories on push, so this instead lazily brings up the backing ``registry:2`` — idempotently, and safe to call from many concurrent pushes. A no-op when something already serves the registry at ``registry_host`` (a registry you - run yourself, or one a concurrent push already started); it never removes a container - that might be serving. Called from the image-push path, so no caller manages it.""" - port = urlsplit(f"//{self.registry_host}").port or 5000 + run yourself, or one a concurrent push already started); it never removes a container. + Called from the image-push path, so no caller manages it.""" + port = urlsplit(f"//{self.registry_host}").port or _DEFAULT_PORT if self._registry_listening(port): return - run = self._run_registry(port) + name = _container_name(port) + run = self._run_registry(name, port) if run.returncode != 0: # `docker run` creates the named container atomically, so a failure is either a name # conflict (our container already exists) or something unrelated — disambiguate on # the existing container instead of blindly retrying, so a real error isn't masked. - existing_port = self._container_configured_port(_REGISTRY_CONTAINER) + existing_port = self._container_configured_port(name) if existing_port is None: # No such container: the failure is real — the port is taken by another process, # the image pull failed, or the daemon is down. Surface it now, don't wait 30s. raise RuntimeError(f"could not start the local registry: {run.stderr.strip()}") if existing_port != port: - # A stale container of ours on a different port (registry_host changed since it - # was created). It isn't serving our port — the check above proved that — so - # replacing it is safe. - subprocess.run(["docker", "rm", "-f", _REGISTRY_CONTAINER], capture_output=True, text=True) - run = self._run_registry(port) - if run.returncode != 0: - raise RuntimeError(f"could not start the local registry: {run.stderr.strip()}") - else: - # A concurrent push won the race, or a stopped container of ours is on our port: - # (re)start it (a no-op if it's already running) and surface a genuine failure. - start = subprocess.run(["docker", "start", _REGISTRY_CONTAINER], capture_output=True, text=True) - if start.returncode != 0: - raise RuntimeError( - f"local registry container exists but could not be started: {start.stderr.strip()}" - ) + # Only an older version's agentenv-registry, which it ran on any port; refuse rather than remove it. + raise RuntimeError( + f"{name} is using port {existing_port}, not {port}: " + f"point registry_host at port {existing_port}, or remove the container" + ) + # A concurrent push won the race, or our stopped container is on this port: (re)start it. + start = subprocess.run(["docker", "start", name], capture_output=True, text=True) + if start.returncode != 0: + raise RuntimeError( + f"local registry container exists but could not be started: {start.stderr.strip()}" + ) deadline = time.time() + 30 while time.time() < deadline: if self._registry_listening(port): @@ -73,9 +77,9 @@ def ensure_repository(self, repository: str) -> None: raise RuntimeError(f"the local registry started but did not answer /v2/ on port {port} within 30s") @staticmethod - def _run_registry(port: int) -> subprocess.CompletedProcess: + def _run_registry(name: str, port: int) -> subprocess.CompletedProcess: return subprocess.run( - ["docker", "run", "-d", "--restart", "unless-stopped", "--name", _REGISTRY_CONTAINER, + ["docker", "run", "-d", "--restart", "unless-stopped", "--name", name, "-p", f"127.0.0.1:{port}:5000", "registry:2"], capture_output=True, text=True, ) diff --git a/tst/unit/store/local_registry_image_store_test.py b/tst/unit/store/local_registry_image_store_test.py new file mode 100644 index 00000000..0712e96d --- /dev/null +++ b/tst/unit/store/local_registry_image_store_test.py @@ -0,0 +1,95 @@ +"""LocalRegistryImageStore's lazy registry bring-up, with docker and the /v2/ ping faked.""" + +import subprocess + +import pytest + +from agent_env.store.image_store import local_registry_image_store +from agent_env.store.image_store.local_registry_image_store import LocalRegistryImageStore + + +class _Docker: + """Answers the docker commands bring-up issues and records each one.""" + + def __init__(self, *, run_rc=0, existing_port=None, start_rc=0): + self.run_rc, self.existing_port, self.start_rc = run_rc, existing_port, start_rc + self.calls: list[list[str]] = [] + + def __call__(self, argv, **kwargs): + self.calls.append(argv) + command = argv[1] + if command == "run": + return subprocess.CompletedProcess(argv, self.run_rc, "", "Conflict" if self.run_rc else "") + if command == "inspect": + found = self.existing_port is not None + return subprocess.CompletedProcess(argv, 0 if found else 1, str(self.existing_port or ""), "") + if command == "start": + return subprocess.CompletedProcess(argv, self.start_rc, "", "boom" if self.start_rc else "") + raise AssertionError(f"unexpected docker call: {argv}") + + +@pytest.fixture +def docker(monkeypatch): + def install(**kwargs): + fake = _Docker(**kwargs) + monkeypatch.setattr(local_registry_image_store.subprocess, "run", fake) + return fake + return install + + +@pytest.fixture +def listening(monkeypatch): + """The registry answers only after bring-up has run a docker command.""" + def install(fake): + monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: bool(fake.calls))) + monkeypatch.setattr(local_registry_image_store.time, "sleep", lambda seconds: None) + return install + + +def test_the_default_port_runs_agentenv_registry(docker, listening): + fake = docker() + listening(fake) + LocalRegistryImageStore("localhost:5000").ensure_repository("r") + assert fake.calls == [["docker", "run", "-d", "--restart", "unless-stopped", "--name", "agentenv-registry", + "-p", "127.0.0.1:5000:5000", "registry:2"]] + + +def test_another_port_runs_its_own_container(docker, listening): + fake = docker() + listening(fake) + LocalRegistryImageStore("localhost:5001").ensure_repository("r") + assert fake.calls == [["docker", "run", "-d", "--restart", "unless-stopped", "--name", "agentenv-registry-5001", + "-p", "127.0.0.1:5001:5000", "registry:2"]] + + +def test_a_serving_registry_is_left_alone(docker, monkeypatch): + fake = docker() + monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: True)) + LocalRegistryImageStore("localhost:5000").ensure_repository("r") + assert fake.calls == [] + + +@pytest.mark.parametrize("port, name", [(5000, "agentenv-registry"), (5001, "agentenv-registry-5001")]) +def test_a_stopped_container_on_this_port_is_started(docker, listening, port, name): + fake = docker(run_rc=125, existing_port=port) + listening(fake) + LocalRegistryImageStore(f"localhost:{port}").ensure_repository("r") + assert [call[1] for call in fake.calls] == ["run", "inspect", "start"] + assert fake.calls[1][-1] == name + assert fake.calls[-1] == ["docker", "start", name] + + +def test_an_older_agentenv_registry_on_another_port_is_refused_not_removed(docker, monkeypatch): + fake = docker(run_rc=125, existing_port=5001) + monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: False)) + with pytest.raises(RuntimeError, match="agentenv-registry is using port 5001, not 5000"): + LocalRegistryImageStore("localhost:5000").ensure_repository("r") + assert [call[1] for call in fake.calls] == ["run", "inspect"] + + +def test_a_failed_run_with_no_container_surfaces_dockers_error(docker, monkeypatch): + fake = docker(run_rc=125) + monkeypatch.setattr(LocalRegistryImageStore, "_registry_listening", staticmethod(lambda port: False)) + with pytest.raises(RuntimeError, match="could not start the local registry: Conflict"): + LocalRegistryImageStore("localhost:5000").ensure_repository("r") + assert all(call[1] != "rm" for call in fake.calls)