From 4aa11de6bf2a7d12bb4496eb9e7de6e396d046d9 Mon Sep 17 00:00:00 2001 From: Songmin Li Date: Sat, 9 Aug 2025 22:32:20 +0800 Subject: [PATCH 1/3] fix: podman-compose down should not stop the upstream dependencies. Say there is an app service depends on a db service, when we run `podman-compose down app`, the db service should not be stopped. Signed-off-by: Songmin Li --- ...op-wrong-dependents-on-compose-down.bugfix | 1 + podman_compose.py | 15 +++++- tests/unit/test_depends_on.py | 53 +++++++++++++++++++ 3 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 newsfragments/fix-stop-wrong-dependents-on-compose-down.bugfix create mode 100644 tests/unit/test_depends_on.py diff --git a/newsfragments/fix-stop-wrong-dependents-on-compose-down.bugfix b/newsfragments/fix-stop-wrong-dependents-on-compose-down.bugfix new file mode 100644 index 0000000..36fe51d --- /dev/null +++ b/newsfragments/fix-stop-wrong-dependents-on-compose-down.bugfix @@ -0,0 +1 @@ +Fix `podman-compose down service` stops wrong dependents diff --git a/podman_compose.py b/podman_compose.py index b4b24d5..10cae9e 100755 --- a/podman_compose.py +++ b/podman_compose.py @@ -1433,6 +1433,16 @@ def rec_deps( return deps +def calc_dependents(services: dict[str, Any]) -> None: + for name, srv in services.items(): + deps: set[ServiceDependency] = srv.get("_deps", set()) + for dep in deps: + if dep.name in services: + services[dep.name].setdefault("_dependents", set()).add( + ServiceDependency(name, dep.condition.value) + ) + + def flat_deps(services: dict[str, Any], with_extends: bool = False) -> None: """ create dependencies "_deps" or update it recursively for all services @@ -1470,6 +1480,8 @@ def flat_deps(services: dict[str, Any], with_extends: bool = False) -> None: for name, srv in services.items(): rec_deps(services, name) + calc_dependents(services) + ################### # Override and reset tags @@ -3028,10 +3040,11 @@ def get_excluded(compose: PodmanCompose, args: argparse.Namespace) -> set[str]: excluded = set() if args.services: excluded = set(compose.services) + dep_field = "_dependents" if args.command in ["down"] else "_deps" for service in args.services: # we need 'getattr' as compose_down_parse dose not configure 'no_deps' if service in compose.services and not getattr(args, "no_deps", False): - excluded -= set(x.name for x in compose.services[service]["_deps"]) + excluded -= set(x.name for x in compose.services[service].get(dep_field, set())) excluded.discard(service) log.debug("** excluding: %s", excluded) return excluded diff --git a/tests/unit/test_depends_on.py b/tests/unit/test_depends_on.py new file mode 100644 index 0000000..fff6e63 --- /dev/null +++ b/tests/unit/test_depends_on.py @@ -0,0 +1,53 @@ +import unittest +from typing import Any + +from parameterized import parameterized + +from podman_compose import flat_deps + + +class TestDependsOn(unittest.TestCase): + @parameterized.expand([ + ( + { + "service_a": {}, + "service_b": {"depends_on": {"service_a": {"condition": "healthy"}}}, + "service_c": {"depends_on": {"service_b": {"condition": "healthy"}}}, + }, + # dependencies + { + "service_a": set(), + "service_b": set(["service_a"]), + "service_c": set(["service_a", "service_b"]), + }, + # dependents + { + "service_a": set(["service_b", "service_c"]), + "service_b": set(["service_c"]), + "service_c": set(), + }, + ), + ]) + def test_flat_deps( + self, + services: dict[str, Any], + deps: dict[str, set[str]], + dependents: dict[str, set[str]], + ) -> None: + flat_deps(services) + self.assertEqual( + { + name: set([x.name for x in value.get("_deps", set())]) + for name, value in services.items() + }, + deps, + msg="Dependencies do not match", + ) + self.assertEqual( + { + name: set([x.name for x in value.get("_dependents", set())]) + for name, value in services.items() + }, + dependents, + msg="Dependents do not match", + ) From 967bced35b7b9286649120922673893cb8b0706a Mon Sep 17 00:00:00 2001 From: Songmin Li Date: Sun, 10 Aug 2025 01:58:31 +0800 Subject: [PATCH 2/3] Fix dependency field selection in get_excluded() function Previously, get_excluded() determined the dependency field based on args.command, but this caused issues during teardown actions where `podman-compose up` would pass `command=up` while needing dependents rather than dependencies. This change introduces an explicit dep_field parameter to eliminate the ambiguity and ensure correct dependency resolution. Signed-off-by: Songmin Li --- podman_compose.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/podman_compose.py b/podman_compose.py index 10cae9e..0d0957f 100755 --- a/podman_compose.py +++ b/podman_compose.py @@ -1438,7 +1438,7 @@ def calc_dependents(services: dict[str, Any]) -> None: deps: set[ServiceDependency] = srv.get("_deps", set()) for dep in deps: if dep.name in services: - services[dep.name].setdefault("_dependents", set()).add( + services[dep.name].setdefault(DependField.DEPENDENTS, set()).add( ServiceDependency(name, dep.condition.value) ) @@ -3036,11 +3036,19 @@ async def create_pods(compose: PodmanCompose) -> None: await compose.podman.run([], "pod", podman_args) -def get_excluded(compose: PodmanCompose, args: argparse.Namespace) -> set[str]: +class DependField(str, Enum): + DEPENDENCIES = "_deps" + DEPENDENTS = "_dependents" + + +def get_excluded( + compose: PodmanCompose, + args: argparse.Namespace, + dep_field: DependField = DependField.DEPENDENCIES, +) -> set[str]: excluded = set() if args.services: excluded = set(compose.services) - dep_field = "_dependents" if args.command in ["down"] else "_deps" for service in args.services: # we need 'getattr' as compose_down_parse dose not configure 'no_deps' if service in compose.services and not getattr(args, "no_deps", False): @@ -3335,7 +3343,7 @@ def get_volume_names(compose: PodmanCompose, cnt: dict) -> list[str]: @cmd_run(podman_compose, "down", "tear down entire stack") async def compose_down(compose: PodmanCompose, args: argparse.Namespace) -> None: - excluded = get_excluded(compose, args) + excluded = get_excluded(compose, args, DependField.DEPENDENTS) podman_args: list[str] = [] timeout_global = getattr(args, "timeout", None) containers = list(reversed(compose.containers)) From cd25efa7793fd5a6a65943e0bb91a841831b8d14 Mon Sep 17 00:00:00 2001 From: Songmin Li Date: Sun, 10 Aug 2025 22:35:05 +0800 Subject: [PATCH 3/3] Add integration test for compose down Signed-off-by: Songmin Li --- .../compose_down_behavior/__init__.py | 0 .../docker-compose_default.yaml | 12 +++ .../test_compose_down_behavior.py | 81 +++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 tests/integration/compose_down_behavior/__init__.py create mode 100644 tests/integration/compose_down_behavior/docker-compose_default.yaml create mode 100644 tests/integration/compose_down_behavior/test_compose_down_behavior.py diff --git a/tests/integration/compose_down_behavior/__init__.py b/tests/integration/compose_down_behavior/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/integration/compose_down_behavior/docker-compose_default.yaml b/tests/integration/compose_down_behavior/docker-compose_default.yaml new file mode 100644 index 0000000..8eb0ff1 --- /dev/null +++ b/tests/integration/compose_down_behavior/docker-compose_default.yaml @@ -0,0 +1,12 @@ +services: + app: + image: nopush/podman-compose-test + command: ["dumb-init", "/bin/busybox", "httpd", "-f", "-p", "8080"] + depends_on: + - db + db: + image: nopush/podman-compose-test + command: ["dumb-init", "/bin/busybox", "httpd", "-f", "-p", "8080"] + no_deps: + image: nopush/podman-compose-test + command: ["dumb-init", "/bin/busybox", "httpd", "-f", "-p", "8080"] diff --git a/tests/integration/compose_down_behavior/test_compose_down_behavior.py b/tests/integration/compose_down_behavior/test_compose_down_behavior.py new file mode 100644 index 0000000..81f1447 --- /dev/null +++ b/tests/integration/compose_down_behavior/test_compose_down_behavior.py @@ -0,0 +1,81 @@ +# SPDX-License-Identifier: GPL-2.0 + +import os +import unittest + +from parameterized import parameterized + +from tests.integration.test_utils import RunSubprocessMixin +from tests.integration.test_utils import podman_compose_path +from tests.integration.test_utils import test_path + + +def compose_yaml_path(scenario: str) -> str: + return os.path.join( + os.path.join(test_path(), "compose_down_behavior"), f"docker-compose_{scenario}.yaml" + ) + + +class TestComposeDownBehavior(unittest.TestCase, RunSubprocessMixin): + @parameterized.expand([ + ("default", ["down"], set()), + ( + "default", + ["down", "app"], + { + "compose_down_behavior_db_1", + "compose_down_behavior_no_deps_1", + }, + ), + ( + "default", + ["down", "db"], + { + "compose_down_behavior_no_deps_1", + }, + ), + ]) + def test_compose_down( + self, scenario: str, command_args: list[str], expect_containers: set[str] + ) -> None: + try: + self.run_subprocess_assert_returncode( + [podman_compose_path(), "-f", compose_yaml_path(scenario), "up", "-d"], + ) + + self.run_subprocess_assert_returncode( + [ + podman_compose_path(), + "-f", + compose_yaml_path(scenario), + *command_args, + ], + ) + + out, _ = self.run_subprocess_assert_returncode( + [ + podman_compose_path(), + "-f", + compose_yaml_path(scenario), + "ps", + "--format", + '{{ .Names }}', + ], + ) + + actual_containers = set() + for line in out.decode('utf-8').strip().split('\n'): + name = line.strip() + if name: + actual_containers.add(name) + + self.assertEqual(actual_containers, expect_containers) + finally: + self.run_subprocess_assert_returncode([ + podman_compose_path(), + "-f", + compose_yaml_path(scenario), + "down", + "-t", + "0", + ])