diff --git a/devservices/commands/down.py b/devservices/commands/down.py index 1e74922..edc0320 100644 --- a/devservices/commands/down.py +++ b/devservices/commands/down.py @@ -22,6 +22,8 @@ from devservices.utils.console import Console from devservices.utils.console import Status from devservices.utils.dependencies import construct_dependency_graph +from devservices.utils.dependencies import DependencyNode +from devservices.utils.dependencies import DependencyType from devservices.utils.dependencies import get_non_shared_remote_dependencies from devservices.utils.dependencies import install_and_verify_dependencies from devservices.utils.dependencies import InstalledRemoteDependency @@ -234,7 +236,10 @@ def _get_dependent_service( ) # If the service we are trying to bring down is in the dependency graph of another service, # we should not bring it down - if service.name in dependency_graph.graph: + if ( + DependencyNode(name=service.name, dependency_type=DependencyType.SERVICE) + in dependency_graph.graph + ): return other_started_service return None diff --git a/devservices/commands/up.py b/devservices/commands/up.py index b8164cd..af7b58a 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -24,6 +24,8 @@ from devservices.utils.console import Console from devservices.utils.console import Status from devservices.utils.dependencies import construct_dependency_graph +from devservices.utils.dependencies import DependencyNode +from devservices.utils.dependencies import DependencyType from devservices.utils.dependencies import install_and_verify_dependencies from devservices.utils.dependencies import InstalledRemoteDependency from devservices.utils.docker import check_all_containers_healthy @@ -169,7 +171,12 @@ def _up( dependency_graph = construct_dependency_graph(service, modes=modes) starting_order = dependency_graph.get_starting_order() sorted_remote_dependencies = sorted( - remote_dependencies, key=lambda dep: starting_order.index(dep.service_name) + remote_dependencies, + key=lambda dep: starting_order.index( + DependencyNode( + name=dep.service_name, dependency_type=DependencyType.SERVICE + ) + ), ) # Pull all images in parallel status.info("Pulling images") diff --git a/devservices/utils/dependencies.py b/devservices/utils/dependencies.py index 4ee3c50..b885b89 100644 --- a/devservices/utils/dependencies.py +++ b/devservices/utils/dependencies.py @@ -10,6 +10,7 @@ from concurrent.futures import as_completed from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass +from enum import Enum from typing import TextIO from typing import TypeGuard @@ -53,51 +54,65 @@ ] +class DependencyType(str, Enum): + SERVICE = "service" + COMPOSE = "compose" + + +@dataclass(frozen=True, eq=True) +class DependencyNode: + name: str + dependency_type: DependencyType + + def __str__(self) -> str: + return self.name + + class DependencyGraph: def __init__(self) -> None: - self.graph: dict[str, set[str]] = dict() + self.graph: dict[DependencyNode, set[DependencyNode]] = dict() - def add_dependency(self, service_name: str) -> None: - if service_name not in self.graph: - self.graph[service_name] = set() + def add_node(self, node: DependencyNode) -> None: + if node not in self.graph: + self.graph[node] = set() - def add_edge(self, service_name: str, dependency_name: str) -> None: - # TODO: We should rename services that depend on themselves - if service_name == dependency_name: - return - if service_name not in self.graph: - self.add_dependency(service_name) - if dependency_name not in self.graph: - self.add_dependency(dependency_name) + def add_edge(self, from_node: DependencyNode, to_node: DependencyNode) -> None: + if from_node == to_node: + # TODO: Add a better exception + raise ValueError("Cannot add an edge from a node to itself") + if from_node not in self.graph: + self.add_node(from_node) + if to_node not in self.graph: + self.add_node(to_node) # TODO: Should we check for cycles here? - self.graph[service_name].add(dependency_name) + self.graph[from_node].add(to_node) - def topological_sort(self) -> list[str]: + def topological_sort(self) -> list[DependencyNode]: in_degree = {service_name: 0 for service_name in self.graph} - for service_name in self.graph.keys(): - for dependency in self.graph[service_name]: - in_degree[dependency] += 1 + for service_node in self.graph.keys(): + for dependency_node in self.graph[service_node]: + in_degree[dependency_node] += 1 queue = deque( [ - service_name - for service_name in self.graph - if in_degree[service_name] == 0 + dependency_node + for dependency_node in self.graph + if in_degree[dependency_node] == 0 ] ) topological_order = list() while queue: - service_name = queue.popleft() - topological_order.append(service_name) + service_node = queue.popleft() + topological_order.append(service_node) - for dependency in self.graph[service_name]: - in_degree[dependency] -= 1 - if in_degree[dependency] == 0: - queue.append(dependency) + for dependency_node in self.graph[service_node]: + in_degree[dependency_node] -= 1 + if in_degree[dependency_node] == 0: + queue.append(dependency_node) if len(topological_order) != len(self.graph): # TODO: Add a better exception @@ -105,7 +120,7 @@ def topological_sort(self) -> list[str]: return topological_order - def get_starting_order(self) -> list[str]: + def get_starting_order(self) -> list[DependencyNode]: return list(reversed(self.topological_sort())) @@ -729,7 +744,18 @@ def _construct_dependency_graph( # Skip the dependency if it's not in the modes (since it may not be installed and we don't care about it) if dependency_name not in service_mode_dependencies: continue - dependency_graph.add_edge(service_config.service_name, dependency_name) + dependency_graph.add_edge( + DependencyNode( + name=service_config.service_name, + dependency_type=DependencyType.SERVICE, + ), + DependencyNode( + name=dependency_name, + dependency_type=DependencyType.SERVICE + if _has_remote_config(dependency.remote) + else DependencyType.COMPOSE, + ), + ) if _has_remote_config(dependency.remote): dependency_config = get_remote_dependency_config(dependency.remote) _construct_dependency_graph(dependency_config, [dependency.remote.mode]) diff --git a/tests/utils/test_dependencies.py b/tests/utils/test_dependencies.py index 3830de5..9076ff4 100644 --- a/tests/utils/test_dependencies.py +++ b/tests/utils/test_dependencies.py @@ -22,6 +22,8 @@ from devservices.exceptions import InvalidDependencyConfigError from devservices.exceptions import ModeDoesNotExistError from devservices.utils.dependencies import construct_dependency_graph +from devservices.utils.dependencies import DependencyNode +from devservices.utils.dependencies import DependencyType from devservices.utils.dependencies import get_installed_remote_dependencies from devservices.utils.dependencies import get_non_shared_remote_dependencies from devservices.utils.dependencies import GitConfigManager @@ -2409,7 +2411,7 @@ def test_construct_dependency_graph_simple( dependency_service_repo_config = { "x-sentry-service-config": { "version": 0.1, - "service_name": "test-service", + "service_name": "dependency-1", "dependencies": { "dependency-1": { "description": "dependency-1", @@ -2457,11 +2459,30 @@ def test_construct_dependency_graph_simple( install_and_verify_dependencies(service) dependency_graph = construct_dependency_graph(service, ["default"]) assert dependency_graph.graph == { - "dependency-1": set(), - "test-service": {"dependency-1"}, + DependencyNode( + name="dependency-1", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="dependency-1", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="dependency-1", dependency_type=DependencyType.COMPOSE + ) + }, + DependencyNode( + name="test-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="dependency-1", dependency_type=DependencyType.SERVICE + ) + }, } - assert dependency_graph.get_starting_order() == ["dependency-1", "test-service"] + assert dependency_graph.get_starting_order() == [ + DependencyNode(name="dependency-1", dependency_type=DependencyType.COMPOSE), + DependencyNode(name="dependency-1", dependency_type=DependencyType.SERVICE), + DependencyNode(name="test-service", dependency_type=DependencyType.SERVICE), + ] def test_construct_dependency_graph_one_nested_dependency( @@ -2555,16 +2576,61 @@ def test_construct_dependency_graph_one_nested_dependency( install_and_verify_dependencies(service) dependency_graph = construct_dependency_graph(service, ["default"]) assert dependency_graph.graph == { - "child-service": set(), - "parent-service": {"child-service"}, - "grandparent-service": {"parent-service"}, + DependencyNode( + name="child-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="child-service", dependency_type=DependencyType.COMPOSE + ) + }, + DependencyNode( + name="parent-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="parent-service", dependency_type=DependencyType.COMPOSE + ), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ), + }, + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ), + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.COMPOSE + ), + }, } - assert dependency_graph.get_starting_order() == [ - "child-service", - "parent-service", - "grandparent-service", - ] + starting_order = dependency_graph.get_starting_order() + assert starting_order.index( + DependencyNode(name="child-service", dependency_type=DependencyType.SERVICE) + ) < starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ), "Child service should come before parent service in the starting order" + assert starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ) < starting_order.index( + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ) + ), "Parent service should come before grandparent service in the starting order" def test_construct_dependency_graph_shared_dependency( @@ -2666,16 +2732,218 @@ def test_construct_dependency_graph_shared_dependency( install_and_verify_dependencies(service) dependency_graph = construct_dependency_graph(service, ["default"]) assert dependency_graph.graph == { - "child-service": set(), - "parent-service": {"child-service"}, - "grandparent-service": {"parent-service", "child-service"}, + DependencyNode( + name="child-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="child-service", dependency_type=DependencyType.COMPOSE + ) + }, + DependencyNode( + name="parent-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="parent-service", dependency_type=DependencyType.COMPOSE + ), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ), + }, + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.COMPOSE + ), + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ), + }, } - assert dependency_graph.get_starting_order() == [ - "child-service", - "parent-service", - "grandparent-service", - ] + starting_order = dependency_graph.get_starting_order() + assert starting_order.index( + DependencyNode(name="child-service", dependency_type=DependencyType.SERVICE) + ) < starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ), "Child service should come before parent service in the starting order" + + assert starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ) < starting_order.index( + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ) + ), "Parent service should come before grandparent service in the starting order" + + +def test_construct_dependency_graph_non_self_reference( + tmp_path: Path, +) -> None: + parent_service_repo_path = tmp_path / "parent-service-repo" + child_service_repo_path = tmp_path / "child-service-repo" + create_mock_git_repo("blank_repo", parent_service_repo_path) + create_mock_git_repo("blank_repo", child_service_repo_path) + parent_service_repo_config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "parent-service", + "dependencies": { + "child-service": { + "description": "child-service", + "remote": { + "repo_name": "child-service", + "repo_link": f"file://{child_service_repo_path}", + "branch": "main", + }, + }, + "parent-service-container": { + "description": "parent-service-container", + }, + }, + "modes": {"default": ["child-service", "parent-service-container"]}, + }, + "services": { + "parent-service-container": { + "image": "parent-service-container", + }, + }, + } + child_service_repo_config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "child-service", + "dependencies": { + "child-service-container": { + "description": "child-service-container", + }, + }, + "modes": {"default": ["child-service-container"]}, + }, + "services": { + "child-service-container": { + "image": "child-service-container", + }, + }, + } + create_config_file(parent_service_repo_path, parent_service_repo_config) + create_config_file(child_service_repo_path, child_service_repo_config) + run_git_command(["add", "."], cwd=parent_service_repo_path) + run_git_command( + ["commit", "-m", "Add devservices config"], cwd=parent_service_repo_path + ) + run_git_command(["add", "."], cwd=child_service_repo_path) + run_git_command( + ["commit", "-m", "Add devservices config"], cwd=child_service_repo_path + ) + service = Service( + name="grandparent-service", + repo_path="/path/to/grandparent-service", + config=ServiceConfig( + version=0.1, + service_name="grandparent-service", + dependencies={ + "parent-service": Dependency( + description="parent-service", + remote=RemoteConfig( + repo_name="parent-service", + repo_link=f"file://{parent_service_repo_path}", + branch="main", + ), + ), + "grandparent-service-container": Dependency( + description="grandparent-service-container", + ), + }, + modes={ + "default": ["parent-service", "grandparent-service-container"], + }, + ), + ) + + with mock.patch( + "devservices.utils.dependencies.DEVSERVICES_DEPENDENCIES_CACHE_DIR", + str(tmp_path / "dependency-dir"), + ): + install_and_verify_dependencies(service) + dependency_graph = construct_dependency_graph(service, ["default"]) + assert dependency_graph.graph == { + DependencyNode( + name="child-service-container", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="child-service-container", + dependency_type=DependencyType.COMPOSE, + ) + }, + DependencyNode( + name="parent-service-container", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="parent-service-container", + dependency_type=DependencyType.COMPOSE, + ), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ), + }, + DependencyNode( + name="grandparent-service-container", + dependency_type=DependencyType.COMPOSE, + ): set(), + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="grandparent-service-container", + dependency_type=DependencyType.COMPOSE, + ), + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ), + }, + } + + starting_order = dependency_graph.get_starting_order() + assert starting_order.index( + DependencyNode(name="child-service", dependency_type=DependencyType.SERVICE) + ) < starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ), "Child service should come before parent service in the starting order" + + assert starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ) < starting_order.index( + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ) + ), "Parent service should come before grandparent service in the starting order" def test_construct_dependency_graph_complex( @@ -2839,14 +3107,85 @@ def test_construct_dependency_graph_complex( install_and_verify_dependencies(service) dependency_graph = construct_dependency_graph(service, ["default"]) assert dependency_graph.graph == { - "child-service": set(), - "parent-service": {"child-service"}, - "grandparent-service": {"parent-service"}, - "complex-service": {"grandparent-service", "child-service"}, + DependencyNode( + name="child-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="child-service", dependency_type=DependencyType.COMPOSE + ) + }, + DependencyNode( + name="parent-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="parent-service", dependency_type=DependencyType.COMPOSE + ), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ), + }, + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.COMPOSE + ), + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ), + }, + DependencyNode( + name="complex-service", dependency_type=DependencyType.COMPOSE + ): set(), + DependencyNode( + name="complex-service", dependency_type=DependencyType.SERVICE + ): { + DependencyNode( + name="complex-service", dependency_type=DependencyType.COMPOSE + ), + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ), + DependencyNode( + name="child-service", dependency_type=DependencyType.SERVICE + ), + }, } - assert dependency_graph.get_starting_order() == [ - "child-service", - "parent-service", - "grandparent-service", - "complex-service", - ] + + starting_order = dependency_graph.get_starting_order() + assert starting_order.index( + DependencyNode(name="child-service", dependency_type=DependencyType.SERVICE) + ) < starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ), "Child service should come before parent service in the starting order" + + assert starting_order.index( + DependencyNode( + name="parent-service", dependency_type=DependencyType.SERVICE + ) + ) < starting_order.index( + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ) + ), "Parent service should come before grandparent service in the starting order" + + assert starting_order.index( + DependencyNode( + name="complex-service", dependency_type=DependencyType.SERVICE + ) + ) > starting_order.index( + DependencyNode( + name="grandparent-service", dependency_type=DependencyType.SERVICE + ) + ), "Grandparent service should come before complex service in the starting order"