diff --git a/devservices/commands/down.py b/devservices/commands/down.py index 0381e2c..880c93a 100644 --- a/devservices/commands/down.py +++ b/devservices/commands/down.py @@ -76,14 +76,20 @@ def down(args: Namespace) -> None: modes = service.config.modes state = State() - started_services = state.get_service_entries(StateTables.STARTED_SERVICES) - if service.name not in started_services: + starting_services = set(state.get_service_entries(StateTables.STARTING_SERVICES)) + started_services = set(state.get_service_entries(StateTables.STARTED_SERVICES)) + active_services = starting_services.union(started_services) + if service.name not in active_services: console.warning(f"{service.name} is not running") exit(0) - active_modes = state.get_active_modes_for_service( + active_starting_modes = state.get_active_modes_for_service( + service.name, StateTables.STARTING_SERVICES + ) + active_started_modes = state.get_active_modes_for_service( service.name, StateTables.STARTED_SERVICES ) + active_modes = active_starting_modes or active_started_modes mode_dependencies = set() for active_mode in active_modes: active_mode_dependencies = modes.get(active_mode, []) @@ -111,13 +117,20 @@ def down(args: Namespace) -> None: # Check if any service depends on the service we are trying to bring down # TODO: We should also take into account the active modes of the other services (this is not trivial to do) - other_started_services = set(started_services).difference({service.name}) + other_started_services = active_services.difference({service.name}) dependent_service_name = None for other_started_service in other_started_services: other_service = find_matching_service(other_started_service) - other_service_active_modes = state.get_active_modes_for_service( + other_service_active_starting_modes = state.get_active_modes_for_service( + other_service.name, StateTables.STARTING_SERVICES + ) + other_service_active_started_modes = state.get_active_modes_for_service( other_service.name, StateTables.STARTED_SERVICES ) + other_service_active_modes = ( + other_service_active_starting_modes + or other_service_active_started_modes + ) dependency_graph = construct_dependency_graph( other_service, other_service_active_modes ) @@ -140,7 +153,7 @@ def down(args: Namespace) -> None: ) # TODO: We should factor in healthchecks here before marking service as not running - state = State() + state.remove_service_entry(service.name, StateTables.STARTING_SERVICES) state.remove_service_entry(service.name, StateTables.STARTED_SERVICES) if dependent_service_name is None: console.success(f"{service.name} stopped") diff --git a/devservices/commands/up.py b/devservices/commands/up.py index 31035fc..72c48e7 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -79,6 +79,8 @@ def up(args: Namespace) -> None: modes = service.config.modes mode = args.mode + state = State() + with Status( lambda: console.warning(f"Starting '{service.name}' in mode: '{mode}'"), lambda: console.success(f"{service.name} started"), @@ -100,6 +102,8 @@ def up(args: Namespace) -> None: except subprocess.CalledProcessError: # Network already exists, ignore the error pass + # Add the service to the starting services table + state.update_service_entry(service.name, mode, StateTables.STARTING_SERVICES) try: mode_dependencies = modes[mode] _up(service, [mode], remote_dependencies, mode_dependencies, status) @@ -108,7 +112,7 @@ def up(args: Namespace) -> None: status.failure(f"Failed to start {service.name}: {dce.stderr}") exit(1) # TODO: We should factor in healthchecks here before marking service as running - state = State() + state.remove_service_entry(service.name, StateTables.STARTING_SERVICES) state.update_service_entry(service.name, mode, StateTables.STARTED_SERVICES) diff --git a/devservices/utils/dependencies.py b/devservices/utils/dependencies.py index 7c3496e..0be2fb6 100644 --- a/devservices/utils/dependencies.py +++ b/devservices/utils/dependencies.py @@ -263,12 +263,14 @@ def get_non_shared_remote_dependencies( service_to_stop: Service, remote_dependencies: set[InstalledRemoteDependency] ) -> set[InstalledRemoteDependency]: state = State() - started_services = state.get_service_entries(StateTables.STARTED_SERVICES) + starting_services = set(state.get_service_entries(StateTables.STARTING_SERVICES)) + started_services = set(state.get_service_entries(StateTables.STARTED_SERVICES)) + active_services = starting_services.union(started_services) # We don't care about the remote dependencies of the service we are stopping - started_services.remove(service_to_stop.name) + active_services.remove(service_to_stop.name) other_running_remote_dependencies: set[InstalledRemoteDependency] = set() base_running_service_names: set[str] = set() - for started_service_name in started_services: + for started_service_name in active_services: started_service = find_matching_service(started_service_name) for dependency_name in service_to_stop.config.dependencies.keys(): if dependency_name == started_service.config.service_name: diff --git a/tests/commands/test_down.py b/tests/commands/test_down.py index 8f41fc3..a9fe0f4 100644 --- a/tests/commands/test_down.py +++ b/tests/commands/test_down.py @@ -37,7 +37,96 @@ ), ) @mock.patch("devservices.utils.state.State.remove_service_entry") -def test_down_simple( +def test_down_starting( + mock_remove_service_entry: mock.Mock, + mock_run: mock.Mock, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + with mock.patch( + "devservices.commands.down.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) + + with mock.patch( + "devservices.utils.state.STATE_DB_FILE", str(tmp_path / "state") + ): + state = State() + state.update_service_entry( + "example-service", "default", StateTables.STARTING_SERVICES + ) + down(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_run.assert_called_with( + [ + "docker", + "compose", + "-p", + "example-service", + "-f", + f"{service_path}/{DEVSERVICES_DIR_NAME}/{CONFIG_FILE_NAME}", + "stop", + "clickhouse", + "redis", + ], + check=True, + capture_output=True, + text=True, + env=mock.ANY, + ) + + mock_remove_service_entry.assert_has_calls( + [ + mock.call("example-service", StateTables.STARTING_SERVICES), + mock.call("example-service", StateTables.STARTED_SERVICES), + ] + ) + + captured = capsys.readouterr() + assert "Stopping clickhouse" in captured.out.strip() + assert "Stopping redis" 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.remove_service_entry") +def test_down_started( mock_remove_service_entry: mock.Mock, mock_run: mock.Mock, tmp_path: Path, @@ -105,8 +194,11 @@ def test_down_simple( env=mock.ANY, ) - mock_remove_service_entry.assert_called_with( - "example-service", StateTables.STARTED_SERVICES + mock_remove_service_entry.assert_has_calls( + [ + mock.call("example-service", StateTables.STARTING_SERVICES), + mock.call("example-service", StateTables.STARTED_SERVICES), + ] ) captured = capsys.readouterr() @@ -269,8 +361,11 @@ def test_down_mode_simple( env=mock.ANY, ) - mock_remove_service_entry.assert_called_with( - "example-service", StateTables.STARTED_SERVICES + mock_remove_service_entry.assert_has_calls( + [ + mock.call("example-service", StateTables.STARTING_SERVICES), + mock.call("example-service", StateTables.STARTED_SERVICES), + ] ) captured = capsys.readouterr() @@ -440,8 +535,11 @@ def test_down_overlapping_services( ) # example-service should be stopped - mock_remove_service_entry.assert_called_with( - "example-service", StateTables.STARTED_SERVICES + mock_remove_service_entry.assert_has_calls( + [ + mock.call("example-service", StateTables.STARTING_SERVICES), + mock.call("example-service", StateTables.STARTED_SERVICES), + ] ) @@ -603,8 +701,11 @@ def test_down_does_not_stop_service_being_used_by_another_service( mock_bring_down_dependency.assert_not_called() # example-service should be stopped - mock_remove_service_entry.assert_called_with( - "example-service", StateTables.STARTED_SERVICES + mock_remove_service_entry.assert_has_calls( + [ + mock.call("example-service", StateTables.STARTING_SERVICES), + mock.call("example-service", StateTables.STARTED_SERVICES), + ] ) @@ -773,8 +874,11 @@ def test_down_does_not_stop_nested_service_being_used_by_another_service( mock_bring_down_dependency.assert_not_called() # child-service should be stopped - mock_remove_service_entry.assert_called_with( - "child-service", StateTables.STARTED_SERVICES + mock_remove_service_entry.assert_has_calls( + [ + mock.call("child-service", StateTables.STARTING_SERVICES), + mock.call("child-service", StateTables.STARTED_SERVICES), + ] ) @@ -889,6 +993,9 @@ def test_down_overlapping_non_remote_services( ) # example-service should be stopped - mock_remove_service_entry.assert_called_with( - "example-service", StateTables.STARTED_SERVICES + mock_remove_service_entry.assert_has_calls( + [ + mock.call("example-service", StateTables.STARTING_SERVICES), + mock.call("example-service", StateTables.STARTED_SERVICES), + ] ) diff --git a/tests/commands/test_up.py b/tests/commands/test_up.py index e2801e6..174586c 100644 --- a/tests/commands/test_up.py +++ b/tests/commands/test_up.py @@ -24,6 +24,7 @@ from testing.utils import run_git_command +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch("devservices.commands.up.check_all_containers_healthy") @@ -36,6 +37,7 @@ def test_up_simple( mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: @@ -121,9 +123,16 @@ def test_up_simple( ] ) - mock_update_service_entry.assert_called_with( - "example-service", "default", StateTables.STARTED_SERVICES + mock_update_service_entry.assert_has_calls( + [ + mock.call("example-service", "default", StateTables.STARTING_SERVICES), + mock.call("example-service", "default", StateTables.STARTED_SERVICES), + ] ) + mock_remove_service_entry.assert_called_once_with( + "example-service", StateTables.STARTING_SERVICES + ) + mock_check_all_containers_healthy.assert_called_once() captured = capsys.readouterr() assert "Retrieving dependencies" in captured.out.strip() @@ -132,6 +141,7 @@ def test_up_simple( assert "Starting redis" in captured.out.strip() +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch("devservices.commands.up.check_all_containers_healthy") @@ -141,6 +151,7 @@ def test_up_dependency_error( mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, capsys: pytest.CaptureFixture[str], tmp_path: Path, ) -> None: @@ -184,6 +195,7 @@ def test_up_dependency_error( assert "DependencyError: example-repo (link) on branch" in captured.out.strip() mock_update_service_entry.assert_not_called() + mock_remove_service_entry.assert_not_called() mock_subprocess_check_output.assert_not_called() @@ -196,9 +208,11 @@ def test_up_dependency_error( assert "Starting redis" not in captured.out.strip() +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") def test_up_no_config_file( mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, capsys: pytest.CaptureFixture[str], tmp_path: Path, ) -> None: @@ -218,8 +232,10 @@ def test_up_no_config_file( ) mock_update_service_entry.assert_not_called() + mock_remove_service_entry.assert_not_called() +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch("devservices.commands.up.check_all_containers_healthy") @@ -236,6 +252,7 @@ def test_up_error( mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, capsys: pytest.CaptureFixture[str], tmp_path: Path, ) -> None: @@ -287,7 +304,10 @@ def test_up_error( "Failed to start example-service: Docker Compose error" in captured.out.strip() ) - mock_update_service_entry.assert_not_called() + mock_update_service_entry.assert_called_once_with( + "example-service", "default", StateTables.STARTING_SERVICES + ) + mock_remove_service_entry.assert_not_called() assert "Retrieving dependencies" in captured.out.strip() assert "Starting 'example-service' in mode: 'default'" in captured.out.strip() @@ -295,6 +315,7 @@ def test_up_error( assert "Starting redis" not in captured.out.strip() +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch("devservices.commands.up.check_all_containers_healthy") @@ -307,6 +328,7 @@ def test_up_docker_compose_container_lookup_error( mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: @@ -398,7 +420,11 @@ def test_up_docker_compose_container_lookup_error( ] ) - mock_update_service_entry.assert_not_called() + mock_update_service_entry.assert_called_once_with( + "example-service", "default", StateTables.STARTING_SERVICES + ) + mock_remove_service_entry.assert_not_called() + mock_check_all_containers_healthy.assert_not_called() captured = capsys.readouterr() assert "Retrieving dependencies" in captured.out.strip() @@ -411,6 +437,7 @@ def test_up_docker_compose_container_lookup_error( ) +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch( @@ -430,6 +457,7 @@ def test_up_docker_compose_container_healthcheck_failed( mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: @@ -516,7 +544,11 @@ def test_up_docker_compose_container_healthcheck_failed( ] ) - mock_update_service_entry.assert_not_called() + mock_update_service_entry.assert_called_once_with( + "example-service", "default", StateTables.STARTING_SERVICES + ) + mock_remove_service_entry.assert_not_called() + mock_check_all_containers_healthy.assert_called_once() captured = capsys.readouterr() assert "Retrieving dependencies" in captured.out.strip() @@ -529,6 +561,7 @@ def test_up_docker_compose_container_healthcheck_failed( ) +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") @mock.patch("devservices.commands.up._create_devservices_network") @mock.patch("devservices.commands.up.check_all_containers_healthy") @@ -541,6 +574,7 @@ def test_up_mode_simple( mock_check_all_containers_healthy: mock.Mock, mock_create_devservices_network: mock.Mock, mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: @@ -629,8 +663,14 @@ def test_up_mode_simple( ] ) - mock_update_service_entry.assert_called_with( - "example-service", "test", StateTables.STARTED_SERVICES + mock_update_service_entry.assert_has_calls( + [ + mock.call("example-service", "test", StateTables.STARTING_SERVICES), + mock.call("example-service", "test", StateTables.STARTED_SERVICES), + ] + ) + mock_remove_service_entry.assert_called_once_with( + "example-service", StateTables.STARTING_SERVICES ) mock_check_all_containers_healthy.assert_called_once() captured = capsys.readouterr() @@ -639,11 +679,13 @@ def test_up_mode_simple( assert "Starting redis" in captured.out.strip() +@mock.patch("devservices.utils.state.State.remove_service_entry") @mock.patch("devservices.utils.state.State.update_service_entry") @mock.patch("devservices.commands.up.check_all_containers_healthy") def test_up_mode_does_not_exist( mock_check_all_containers_healthy: mock.Mock, mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: @@ -703,6 +745,8 @@ def test_up_mode_does_not_exist( ) mock_update_service_entry.assert_not_called() + mock_remove_service_entry.assert_not_called() + mock_check_all_containers_healthy.assert_not_called() captured = capsys.readouterr()