fix: require authoritative container snapshot before reconciling
test / stage-firecracker-inputs (pull_request) Successful in 5s
tracker-policy-pr / check-pr (pull_request) Successful in 12s
test / integration-docker (pull_request) Successful in 33s
test / unit (pull_request) Successful in 39s
lint / lint (push) Successful in 49s
test / build-infra (pull_request) Successful in 3m23s
test / integration-firecracker (pull_request) Successful in 1m42s
test / coverage (pull_request) Failing after 1m26s
test / publish-infra (pull_request) Has been skipped
test / stage-firecracker-inputs (pull_request) Successful in 5s
tracker-policy-pr / check-pr (pull_request) Successful in 12s
test / integration-docker (pull_request) Successful in 33s
test / unit (pull_request) Successful in 39s
lint / lint (push) Successful in 49s
test / build-infra (pull_request) Successful in 3m23s
test / integration-firecracker (pull_request) Successful in 1m42s
test / coverage (pull_request) Failing after 1m26s
test / publish-infra (pull_request) Has been skipped
A failed container list or a partial per-container inspect were previously indistinguishable from a legitimately empty/partial live set, so reconciliation could silently unregister healthy bottles. - enumerate_active() now raises EnumerationError instead of returning [] when `container list` fails - Add inspect_container_network_ip() to util, which returns None on inspect failure (vs "" for "no DHCP address yet"), so live_source_ips can tell the two apart - live_source_ips() raises EnumerationError on either failure mode; register_agent() catches it and skips reconciliation, same as OrchestratorClientError - Update tests: rename test_empty_when_the_cli_fails to test_raises_when_the_cli_fails, update patching to the new function, add coverage for list failure, per-container inspect failure, and the launch-not-blocked path
This commit is contained in:
@@ -41,7 +41,7 @@ from ...orchestrator.client import OrchestratorClient, OrchestratorClientError
|
|||||||
from ...orchestrator.registration import registration_inputs
|
from ...orchestrator.registration import registration_inputs
|
||||||
from ..docker.gateway_provision import deprovision_git_gate, provision_git_gate
|
from ..docker.gateway_provision import deprovision_git_gate, provision_git_gate
|
||||||
from . import util as container_mod
|
from . import util as container_mod
|
||||||
from .enumerate import CONTAINER_NAME_PREFIX, enumerate_active
|
from .enumerate import CONTAINER_NAME_PREFIX, EnumerationError, enumerate_active
|
||||||
from .gateway import GATEWAY_NETWORK
|
from .gateway import GATEWAY_NETWORK
|
||||||
from .gateway_provision import AppleGatewayTransport
|
from .gateway_provision import AppleGatewayTransport
|
||||||
from .infra import MacosInfraService, OrchestratorStartError
|
from .infra import MacosInfraService, OrchestratorStartError
|
||||||
@@ -98,14 +98,21 @@ def live_source_ips(network: str) -> list[str]:
|
|||||||
The reconciliation input: the orchestrator lives inside the infra
|
The reconciliation input: the orchestrator lives inside the infra
|
||||||
container and cannot enumerate the host's containers, so the host has to
|
container and cannot enumerate the host's containers, so the host has to
|
||||||
tell it which bottles are actually up. Containers that have not been
|
tell it which bottles are actually up. Containers that have not been
|
||||||
assigned an address yet contribute nothing (the read is non-fatal) — the
|
assigned an address yet contribute nothing — the reap's grace window, not
|
||||||
reap's grace window, not this list, is what protects an in-flight
|
this list, is what protects an in-flight launch.
|
||||||
launch."""
|
|
||||||
|
Raises `EnumerationError` when the live set cannot be determined
|
||||||
|
authoritatively: either the container listing fails or any individual
|
||||||
|
inspect fails. Callers must skip reconciliation in that case to avoid
|
||||||
|
unregistering healthy bottles."""
|
||||||
ips: list[str] = []
|
ips: list[str] = []
|
||||||
for agent in enumerate_active():
|
for agent in enumerate_active():
|
||||||
ip = container_mod.try_container_ipv4_on_network(
|
name = f"{CONTAINER_NAME_PREFIX}{agent.slug}"
|
||||||
f"{CONTAINER_NAME_PREFIX}{agent.slug}", network,
|
ip = container_mod.inspect_container_network_ip(name, network)
|
||||||
)
|
if ip is None:
|
||||||
|
raise EnumerationError(
|
||||||
|
f"container inspect {name!r} failed; live set is not authoritative"
|
||||||
|
)
|
||||||
if ip:
|
if ip:
|
||||||
ips.append(ip)
|
ips.append(ip)
|
||||||
return ips
|
return ips
|
||||||
@@ -133,7 +140,7 @@ def register_agent(
|
|||||||
# reconciliation failure must not block an otherwise-fine launch.
|
# reconciliation failure must not block an otherwise-fine launch.
|
||||||
try:
|
try:
|
||||||
client.reconcile(live_source_ips(endpoint.network))
|
client.reconcile(live_source_ips(endpoint.network))
|
||||||
except OrchestratorClientError as e:
|
except (OrchestratorClientError, EnumerationError) as e:
|
||||||
info(f"registry reconciliation skipped: {e}")
|
info(f"registry reconciliation skipped: {e}")
|
||||||
inputs = registration_inputs(egress_plan)
|
inputs = registration_inputs(egress_plan)
|
||||||
reg = client.register_bottle(
|
reg = client.register_bottle(
|
||||||
|
|||||||
@@ -19,6 +19,10 @@ CONTAINER_NAME_PREFIX = "bot-bottle-"
|
|||||||
_INFRA_NAMES = frozenset({INFRA_NAME})
|
_INFRA_NAMES = frozenset({INFRA_NAME})
|
||||||
|
|
||||||
|
|
||||||
|
class EnumerationError(RuntimeError):
|
||||||
|
"""container list failed; the resulting live set is not authoritative."""
|
||||||
|
|
||||||
|
|
||||||
def enumerate_active() -> list[ActiveAgent]:
|
def enumerate_active() -> list[ActiveAgent]:
|
||||||
result = subprocess.run(
|
result = subprocess.run(
|
||||||
["container", "list", "--quiet"],
|
["container", "list", "--quiet"],
|
||||||
@@ -27,7 +31,10 @@ def enumerate_active() -> list[ActiveAgent]:
|
|||||||
check=False,
|
check=False,
|
||||||
)
|
)
|
||||||
if result.returncode != 0:
|
if result.returncode != 0:
|
||||||
return []
|
raise EnumerationError(
|
||||||
|
f"container list failed: "
|
||||||
|
f"{(result.stderr or '').strip() or '<no stderr>'}"
|
||||||
|
)
|
||||||
out: list[ActiveAgent] = []
|
out: list[ActiveAgent] = []
|
||||||
for name in sorted(line.strip() for line in result.stdout.splitlines()):
|
for name in sorted(line.strip() for line in result.stdout.splitlines()):
|
||||||
if not name.startswith(CONTAINER_NAME_PREFIX) or name in _INFRA_NAMES:
|
if not name.startswith(CONTAINER_NAME_PREFIX) or name in _INFRA_NAMES:
|
||||||
|
|||||||
@@ -572,6 +572,41 @@ def try_container_ipv4_on_network(name: str, network: str) -> str:
|
|||||||
return ""
|
return ""
|
||||||
|
|
||||||
|
|
||||||
|
def inspect_container_network_ip(name: str, network: str) -> str | None:
|
||||||
|
"""IP of `name` on `network`, distinguishing inspect failure from "not yet".
|
||||||
|
|
||||||
|
Returns:
|
||||||
|
- the IP string when the container has one on `network`
|
||||||
|
- "" when inspect succeeds but no address is assigned yet (in-flight DHCP)
|
||||||
|
- None when the inspect command itself fails (authoritative list impossible)
|
||||||
|
"""
|
||||||
|
result = subprocess.run(
|
||||||
|
[_CONTAINER, "inspect", name],
|
||||||
|
capture_output=True, text=True, check=False,
|
||||||
|
)
|
||||||
|
if result.returncode != 0:
|
||||||
|
return None
|
||||||
|
try:
|
||||||
|
data = json.loads(result.stdout or "[]")
|
||||||
|
except json.JSONDecodeError:
|
||||||
|
return None
|
||||||
|
if isinstance(data, list):
|
||||||
|
data = data[0] if data else {}
|
||||||
|
if not isinstance(data, dict):
|
||||||
|
return None
|
||||||
|
status = data.get("status")
|
||||||
|
networks = status.get("networks") if isinstance(status, dict) else None
|
||||||
|
if not isinstance(networks, list):
|
||||||
|
return ""
|
||||||
|
for entry in networks:
|
||||||
|
if not isinstance(entry, dict) or entry.get("network") != network:
|
||||||
|
continue
|
||||||
|
raw = entry.get("ipv4Address")
|
||||||
|
if isinstance(raw, str) and raw:
|
||||||
|
return raw.split("/", 1)[0]
|
||||||
|
return ""
|
||||||
|
|
||||||
|
|
||||||
def wait_container_ipv4_on_network(
|
def wait_container_ipv4_on_network(
|
||||||
name: str, network: str, *, timeout: float = 15.0, poll: float = 0.25,
|
name: str, network: str, *, timeout: float = 15.0, poll: float = 0.25,
|
||||||
) -> str:
|
) -> str:
|
||||||
|
|||||||
@@ -147,7 +147,7 @@ class TestLiveSourceIps(unittest.TestCase):
|
|||||||
def test_maps_slugs_to_container_addresses(self) -> None:
|
def test_maps_slugs_to_container_addresses(self) -> None:
|
||||||
from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips
|
from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips
|
||||||
with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \
|
with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \
|
||||||
patch(f"{_MOD}.container_mod.try_container_ipv4_on_network",
|
patch(f"{_MOD}.container_mod.inspect_container_network_ip",
|
||||||
side_effect=["10.0.0.1", "10.0.0.2"]) as ip:
|
side_effect=["10.0.0.1", "10.0.0.2"]) as ip:
|
||||||
got = live_source_ips("net0")
|
got = live_source_ips("net0")
|
||||||
self.assertEqual(["10.0.0.1", "10.0.0.2"], got)
|
self.assertEqual(["10.0.0.1", "10.0.0.2"], got)
|
||||||
@@ -158,10 +158,30 @@ class TestLiveSourceIps(unittest.TestCase):
|
|||||||
nothing — the reap's grace window, not this list, protects it."""
|
nothing — the reap's grace window, not this list, protects it."""
|
||||||
from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips
|
from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips
|
||||||
with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \
|
with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \
|
||||||
patch(f"{_MOD}.container_mod.try_container_ipv4_on_network",
|
patch(f"{_MOD}.container_mod.inspect_container_network_ip",
|
||||||
side_effect=["", "10.0.0.2"]):
|
side_effect=["", "10.0.0.2"]):
|
||||||
self.assertEqual(["10.0.0.2"], live_source_ips("net0"))
|
self.assertEqual(["10.0.0.2"], live_source_ips("net0"))
|
||||||
|
|
||||||
|
def test_container_list_failure_raises(self) -> None:
|
||||||
|
"""If container list fails, the live set is not authoritative and
|
||||||
|
reconciliation must be skipped."""
|
||||||
|
from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips
|
||||||
|
from bot_bottle.backend.macos_container.enumerate import EnumerationError
|
||||||
|
with patch(f"{_MOD}.enumerate_active",
|
||||||
|
side_effect=EnumerationError("container list failed")):
|
||||||
|
with self.assertRaises(EnumerationError):
|
||||||
|
live_source_ips("net0")
|
||||||
|
|
||||||
|
def test_per_container_inspect_failure_raises(self) -> None:
|
||||||
|
"""If any individual inspect fails, the live set is not authoritative."""
|
||||||
|
from bot_bottle.backend.macos_container.consolidated_launch import live_source_ips
|
||||||
|
from bot_bottle.backend.macos_container.enumerate import EnumerationError
|
||||||
|
with patch(f"{_MOD}.enumerate_active", return_value=self._agents("a", "b")), \
|
||||||
|
patch(f"{_MOD}.container_mod.inspect_container_network_ip",
|
||||||
|
side_effect=["10.0.0.1", None]):
|
||||||
|
with self.assertRaises(EnumerationError):
|
||||||
|
live_source_ips("net0")
|
||||||
|
|
||||||
|
|
||||||
class TestRegisterAgentReconciles(unittest.TestCase):
|
class TestRegisterAgentReconciles(unittest.TestCase):
|
||||||
"""Registration self-heals the registry first: an orphan row at a recycled
|
"""Registration self-heals the registry first: an orphan row at a recycled
|
||||||
@@ -201,3 +221,18 @@ class TestRegisterAgentReconciles(unittest.TestCase):
|
|||||||
client.reconcile.side_effect = OrchestratorClientError("unreachable")
|
client.reconcile.side_effect = OrchestratorClientError("unreachable")
|
||||||
self._register(client)
|
self._register(client)
|
||||||
client.register_bottle.assert_called_once()
|
client.register_bottle.assert_called_once()
|
||||||
|
|
||||||
|
def test_enumeration_error_does_not_block_the_launch(self) -> None:
|
||||||
|
"""A partial container listing must not abort the launch — skip
|
||||||
|
reconciliation and proceed, just as with an unreachable orchestrator."""
|
||||||
|
from bot_bottle.backend.macos_container.enumerate import EnumerationError
|
||||||
|
client = _client()
|
||||||
|
with patch(f"{_MOD}.OrchestratorClient", return_value=client), \
|
||||||
|
patch(f"{_MOD}.provision_git_gate"), \
|
||||||
|
patch(f"{_MOD}.live_source_ips",
|
||||||
|
side_effect=EnumerationError("container list failed")):
|
||||||
|
register_agent(
|
||||||
|
_egress_plan(), _git_plan(),
|
||||||
|
source_ip="10.0.0.7", endpoint=_endpoint(),
|
||||||
|
)
|
||||||
|
client.register_bottle.assert_called_once()
|
||||||
|
|||||||
@@ -66,8 +66,10 @@ class TestMacosContainerEnumerate(unittest.TestCase):
|
|||||||
agents = self._enumerate("bot-bottle-mac-infra\nbot-bottle-dev-abc\n")
|
agents = self._enumerate("bot-bottle-mac-infra\nbot-bottle-dev-abc\n")
|
||||||
self.assertEqual(["dev-abc"], [a.slug for a in agents])
|
self.assertEqual(["dev-abc"], [a.slug for a in agents])
|
||||||
|
|
||||||
def test_empty_when_the_cli_fails(self):
|
def test_raises_when_the_cli_fails(self):
|
||||||
self.assertEqual([], self._enumerate("", returncode=1))
|
from bot_bottle.backend.macos_container.enumerate import EnumerationError
|
||||||
|
with self.assertRaises(EnumerationError):
|
||||||
|
self._enumerate("", returncode=1)
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
|
|||||||
Reference in New Issue
Block a user