diff --git a/devservices/commands/up.py b/devservices/commands/up.py index 8de02bc..07ddf8c 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -146,16 +146,38 @@ def _up( mode_dependencies=mode_dependencies, ) - cmd_outputs = [] + containers_to_check = [] with concurrent.futures.ThreadPoolExecutor() as dependency_executor: futures = [ dependency_executor.submit(_bring_up_dependency, cmd, current_env, status) for cmd in docker_compose_commands ] for future in concurrent.futures.as_completed(futures): - cmd_outputs.append(future.result()) + _ = future.result() - check_all_containers_healthy(status) + for cmd in docker_compose_commands: + try: + container_names = subprocess.check_output( + [ + "docker", + "compose", + "-p", + cmd.project_name, + "-f", + cmd.config_path, + "ps", + "--format", + "{{.Name}}", + ], + text=True, + ).splitlines() + containers_to_check.extend(container_names) + except subprocess.CalledProcessError: + status.failure( + f"Failed to get containers to healthcheck for {cmd.project_name}" + ) + exit(1) + check_all_containers_healthy(status, containers_to_check) def _create_devservices_network() -> None: diff --git a/devservices/utils/docker.py b/devservices/utils/docker.py index e6ef6bc..e6435c3 100644 --- a/devservices/utils/docker.py +++ b/devservices/utils/docker.py @@ -4,7 +4,6 @@ import subprocess import time -from devservices.constants import DEVSERVICES_ORCHESTRATOR_LABEL from devservices.constants import HEALTHCHECK_INTERVAL from devservices.constants import HEALTHCHECK_TIMEOUT from devservices.exceptions import DockerDaemonNotRunningError @@ -25,9 +24,8 @@ def check_docker_daemon_running() -> None: raise DockerDaemonNotRunningError from e -def check_all_containers_healthy(status: Status) -> None: +def check_all_containers_healthy(status: Status, containers: list[str]) -> None: """Ensures all containers are healthy.""" - containers = get_matching_containers(DEVSERVICES_ORCHESTRATOR_LABEL) with concurrent.futures.ThreadPoolExecutor() as healthcheck_executor: futures = [ healthcheck_executor.submit(wait_for_healthy, container, status) @@ -65,7 +63,10 @@ def wait_for_healthy(container_name: str, status: Status) -> None: stderr=e.stderr, ) from e - if result == "healthy" or result == "unknown": + if result == "healthy": + return + elif result == "unknown": + status.warning(f"Container {container_name} does not have a healthcheck.") return # If not healthy, wait and try again @@ -87,8 +88,7 @@ def get_matching_containers(label: str) -> list[str]: [ "docker", "ps", - "--format", - "{{.Names}}", + "-q", "--filter", f"label={label}", ], diff --git a/tests/commands/test_up.py b/tests/commands/test_up.py index 9ec63d7..77b359c 100644 --- a/tests/commands/test_up.py +++ b/tests/commands/test_up.py @@ -33,7 +33,12 @@ @mock.patch("devservices.utils.state.State.update_started_service") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch("devservices.commands.up.check_all_containers_healthy") +@mock.patch( + "devservices.commands.up.subprocess.check_output", + return_value="clickhouse\nredis\n", +) def test_up_simple( + mock_subprocess_check_output: mock.Mock, mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_started_service: mock.Mock, @@ -240,7 +245,111 @@ def test_up_error( @mock.patch("devservices.utils.state.State.update_started_service") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch("devservices.commands.up.check_all_containers_healthy") +@mock.patch( + "devservices.commands.up.subprocess.check_output", + side_effect=[ + "clickhouse\nredis\n", + subprocess.CalledProcessError(returncode=1, cmd="docker compose ps"), + ], +) +def test_up_docker_compose_container_lookup_error( + mock_subprocess_check_output: mock.Mock, + mock_check_all_containers_healthy: mock.Mock, + mock_create_devservices_network: mock.Mock, + mock_update_started_service: mock.Mock, + mock_run: mock.Mock, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + with mock.patch( + "devservices.commands.up.DEVSERVICES_DEPENDENCIES_CACHE_DIR", + str(tmp_path / "dependency-dir"), + ): + config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "redis": {"description": "Redis"}, + "clickhouse": {"description": "Clickhouse"}, + }, + "modes": {"default": ["redis", "clickhouse"]}, + }, + "services": { + "redis": {"image": "redis:6.2.14-alpine"}, + "clickhouse": { + "image": "altinity/clickhouse-server:23.8.11.29.altinitystable" + }, + }, + } + + service_path = tmp_path / "example-service" + create_config_file(service_path, config) + os.chdir(service_path) + + args = Namespace(service_name=None, debug=False, mode="default") + + with pytest.raises(SystemExit): + up(args) + + # Ensure the DEVSERVICES_DEPENDENCIES_CACHE_DIR_KEY is set and is relative + env_vars = mock_run.call_args[1]["env"] + assert ( + env_vars[DEVSERVICES_DEPENDENCIES_CACHE_DIR_KEY] + == f"../dependency-dir/{DEPENDENCY_CONFIG_VERSION}" + ) + + mock_create_devservices_network.assert_called_once() + + mock_run.assert_called_with( + [ + "docker", + "compose", + "-p", + "example-service", + "-f", + f"{service_path}/{DEVSERVICES_DIR_NAME}/{CONFIG_FILE_NAME}", + "up", + "clickhouse", + "redis", + "-d", + ], + check=True, + capture_output=True, + text=True, + env=mock.ANY, + ) + + mock_update_started_service.assert_not_called() + mock_check_all_containers_healthy.assert_not_called() + captured = capsys.readouterr() + assert "Retrieving dependencies" in captured.out.strip() + assert "Starting 'example-service' in mode: 'default'" in captured.out.strip() + assert "Starting clickhouse" in captured.out.strip() + assert "Starting redis" in captured.out.strip() + assert ( + "Failed to get containers to healthcheck for example-service" + in captured.out.strip() + ) + + +@mock.patch( + "devservices.utils.docker_compose.subprocess.run", + return_value=subprocess.CompletedProcess( + args=["docker", "compose", "config", "--services"], + returncode=0, + stdout="clickhouse\nredis\n", + ), +) +@mock.patch("devservices.utils.state.State.update_started_service") +@mock.patch("devservices.commands.up._create_devservices_network") +@mock.patch("devservices.commands.up.check_all_containers_healthy") +@mock.patch( + "devservices.commands.up.subprocess.check_output", + return_value="clickhouse\nredis\n", +) def test_up_mode_simple( + mock_subprocess_check_output: mock.Mock, mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_started_service: mock.Mock, diff --git a/tests/utils/test_docker.py b/tests/utils/test_docker.py index c4110c9..d4be95f 100644 --- a/tests/utils/test_docker.py +++ b/tests/utils/test_docker.py @@ -53,8 +53,7 @@ def test_get_matching_containers( [ "docker", "ps", - "--format", - "{{.Names}}", + "-q", "--filter", f"label={DEVSERVICES_ORCHESTRATOR_LABEL}", ], @@ -91,8 +90,7 @@ def test_get_matching_containers_error( [ "docker", "ps", - "--format", - "{{.Names}}", + "-q", "--filter", f"label={DEVSERVICES_ORCHESTRATOR_LABEL}", ], @@ -337,16 +335,12 @@ def test_wait_for_healthy_healthcheck_failed(mock_check_output: mock.Mock) -> No @mock.patch("devservices.utils.docker.subprocess.check_output") -@mock.patch( - "devservices.utils.docker.get_matching_containers", - return_value=["container1", "container2"], -) def test_check_all_containers_healthy_success( - mock_get_matching_containers: mock.Mock, mock_check_output: mock.Mock + mock_check_output: mock.Mock, ) -> None: mock_status = mock.Mock() mock_check_output.side_effect = ["healthy", "healthy"] - check_all_containers_healthy(mock_status) + check_all_containers_healthy(mock_status, ["container1", "container2"]) mock_check_output.assert_has_calls( [ mock.call( @@ -377,19 +371,15 @@ def test_check_all_containers_healthy_success( @mock.patch("devservices.utils.docker.subprocess.check_output") -@mock.patch( - "devservices.utils.docker.get_matching_containers", - return_value=["container1", "container2"], -) def test_check_all_containers_healthy_failure( - mock_get_matching_containers: mock.Mock, mock_check_output: mock.Mock + mock_check_output: mock.Mock, ) -> None: mock_status = mock.Mock() mock_check_output.side_effect = ["healthy", "unhealthy", "unhealthy"] with mock.patch("devservices.utils.docker.HEALTHCHECK_TIMEOUT", 2), mock.patch( "devservices.utils.docker.HEALTHCHECK_INTERVAL", 1 ): - check_all_containers_healthy(mock_status) + check_all_containers_healthy(mock_status, ["container1", "container2"]) mock_check_output.assert_has_calls( [ mock.call(