potiuk commented on code in PR #73836:
URL: https://github.com/apache/airflow/pull/73836#discussion_r4125636631
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -639,6 +638,11 @@ def perform_environment_checks(quiet: bool = False):
check_uv_version(quiet)
if not quiet:
console_print(f"[success]Host python version is {sys.version}[/]")
+ if cleanup_stale_worktrees:
Review Comment:
This runs from `perform_environment_checks()`, so every Docker-touching
command now goes through it: `shell`, `start-airflow`, each `testing` run, and
so on. But `_get_compose_resources()` (L989, L997) and the `stop`/`rm` calls in
`bring_compose_projects_down()` (L1039, L1046) all use `check=True`.
That turns routine races into failures of unrelated commands. For example, a
container from a parallel `run --rm` test run disappears between `ls` and
`inspect`, or `volume rm` hits a volume that's still attached. When that
happens, a plain `breeze shell` in another checkout exits with
`CalledProcessError`.
For the opportunistic sweep, could it be best-effort? Use `check=False`,
skip the IDs that failed and print one warning. The strict path can stay for an
explicit `breeze down`.
Related, and cheap: the `ls` only filters on
`label=com.docker.compose.project`, so each command inspects every compose
resource on the host, including non-breeze projects. Filtering on
`label=org.apache.airflow.breeze.worktree` would limit the `inspect` to our own
resources.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting
##########
scripts/ci/docker-compose/backend-postgres.yml:
##########
@@ -44,4 +47,7 @@ services:
restart: "on-failure"
volumes:
postgres-data-volume:
- name: "postgres${POSTGRES_VERSION}-db-volume"
+ labels:
+ org.apache.airflow.breeze: "true"
+ org.apache.airflow.breeze.worktree: "${BREEZE_WORKTREE_PATH:-}"
+ name: "${COMPOSE_PROJECT_NAME}-postgres${POSTGRES_VERSION}-db-volume"
Review Comment:
This renames the main checkout's volume from
`postgres${POSTGRES_VERSION}-db-volume` to
`breeze-postgres${POSTGRES_VERSION}-db-volume`. So everyone who upgrades breeze
gets an empty Postgres DB in their main checkout without being told. The old
volume stays behind. It's only pruned through the legacy
`com.docker.compose.project=breeze` filter, and only if the `breeze` project
created it.
Could we keep the old name when the project is `breeze`? Or print a one-time
notice when the old volume exists, with the command to remove it (or migrate
it)? At minimum it deserves a line in `04_troubleshooting.rst`.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting
##########
dev/breeze/src/airflow_breeze/utils/docker_command_utils.py:
##########
@@ -802,22 +806,61 @@ def pull_images_with_retries(
return all_pulled
+def remove_stale_worktree_containers() -> None:
+ result = run_command(
+ [
+ "docker",
+ "ps",
+ "--all",
+ "--filter",
+ "label=org.apache.airflow.breeze=true",
+ "--format",
+ '{{.ID}}\t{{.Label "org.apache.airflow.breeze.worktree"}}',
+ ],
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ console_print("[error]Unable to discover containers belonging to
deleted worktrees.[/]")
+ return
+ for line in result.stdout.splitlines():
+ container_id, _, worktree = line.partition("\t")
+ if _worktree_is_missing(worktree):
+ console_print(f"Removing container {container_id} for deleted
worktree {worktree}")
+ run_command(["docker", "rm", "--force", "--volumes",
container_id], check=False)
+
+
+def _worktree_is_missing(worktree: str) -> bool:
Review Comment:
Staleness is decided by whether the labelled path exists on the machine
running breeze. The commit message notes this assumes the daemon is local. It
isn't always:
- `DOCKER_HOST` can point at a remote daemon or a shared VM.
- Two clones on different hosts can use the same daemon.
- Breeze can be invoked from inside a container, where host paths don't
resolve.
In all of these, a path missing *here* makes us remove someone else's
running containers and DB volumes.
Could we also stamp a host identifier (e.g.
`org.apache.airflow.breeze.host`, set from the hostname or machine ID) next to
`org.apache.airflow.breeze.worktree`? The sweep would then only consider
resources whose host label matches the current host. It's one more label in the
compose files and one extra condition in the selector.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting
##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -1159,21 +1154,22 @@ def down(
all_projects: bool,
project_name: str | None,
):
- perform_environment_checks()
- brought_down, skipped = bring_all_compose_projects_down(
+ if all_projects and project_name:
+ raise click.UsageError("--all-projects and --project-name cannot be
used together.")
+ perform_environment_checks(cleanup_stale_worktrees=False)
+ brought_down = bring_compose_projects_down(
Review Comment:
The semantics of `breeze down` change quite a bit here. Before, a plain
`breeze down` brought down every `breeze*` project: `breeze-docs`,
`breeze-prek`, `breeze-airflow-test-*`, registry, release-management. That was
the "one-shot cleanup" intent of `KNOWN_DOCKER_COMPOSE_PROJECT_PREFIXES`. Now
it only brings down this checkout's default project plus stale worktrees, and
the rest needs `--all-projects`.
CI calls plain `breeze down` in
`scripts/ci/testing/run_integration_tests_with_retry.sh` between retries, and
in `.github/actions/migration_tests/action.yml`. The testing command downs its
own project first, so I don't expect breakage. But it's a user-visible change
that the PR description doesn't mention.
Could plain `breeze down` keep bringing down all of *this checkout's*
projects? That's everything labelled with the current worktree path (or with no
path, in the main checkout), plus stale ones. Then it would still be the
one-shot cleanup for "my" checkout.
---
Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
