Andrushika commented on code in PR #74282: URL: https://github.com/apache/airflow/pull/74282#discussion_r4205174599
########## dev/breeze/src/airflow_breeze/utils/worktree_watcher.py: ########## @@ -0,0 +1,148 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Detached watcher that removes the Docker resources of a deleted Breeze worktree. + +Breeze starts it as a separate process with ``python -I <this file>``. The watcher must keep working +after the worktree is deleted, and that deletion can take Breeze's own virtualenv with it, so this +module only uses the standard library, imports everything up front and never imports +``airflow_breeze``. +""" + +from __future__ import annotations + +import argparse +import contextlib +import fcntl +import subprocess +import time +from pathlib import Path +from typing import IO + +OWNER_LABEL = "org.apache.airflow.breeze=true" +WORKTREE_LABEL = "org.apache.airflow.breeze.worktree" +HOST_LABEL = "org.apache.airflow.breeze.host" +DOCKER_TIMEOUT_SECONDS = 120 + + +def try_lock(lock_file: Path) -> IO[str] | None: + """Take the watcher lock, or return ``None`` when another process holds it.""" + lock_file.parent.mkdir(parents=True, exist_ok=True) + handle = lock_file.open("a") + try: + fcntl.flock(handle, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError: + handle.close() + return None + return handle + + +def is_watcher_running(lock_file: Path) -> bool: + handle = try_lock(lock_file) + if handle is None: + return True + handle.close() + return False + + +def is_worktree_missing(worktree: Path) -> bool: Review Comment: On macOS with Docker Desktop, the worktree path does not go away while its container is running, so this check never fires in the case the watcher is for. For example, with `breeze shell` running in a worktree, `rm -rf` of that worktree fails with "Permission denied" on `devel-common` and `providers-summary-docs` (the bind-mount sources), and both stay. `.git` is gone. The watcher kept polling for 2.5 minutes and the container kept running. After I stopped the container and removed the two directories, the watcher cleaned everything up within 20 seconds. Checking the link to the main repository instead of the path would cover this. It also covers a failed `git worktree remove --force`, which leaves a `.git` file whose gitdir is gone: ```python def is_worktree_missing(worktree: Path) -> bool: try: gitdir = (worktree / ".git").read_text().strip().removeprefix("gitdir:").strip() (worktree / gitdir).stat() except FileNotFoundError: return True except OSError: return False return False ``` ########## dev/breeze/src/airflow_breeze/utils/path_utils.py: ########## @@ -640,11 +641,49 @@ def cleanup_python_generated_files(): console_print("[info]Cleaned") +WORKTREE_ISOLATION_ENV = "BREEZE_WORKTREE_ISOLATION" +SUPPRESS_WORKTREE_ISOLATION_FILE = "suppress_worktree_isolation" + + +def get_shared_build_cache_path() -> Path: + """Return the ``.build`` directory of the main checkout, shared by all of its worktrees.""" + main_git_dir = get_main_git_dir_for_worktree() + return main_git_dir.parent / ".build" if main_git_dir else BUILD_CACHE_PATH + + +def is_worktree_isolation_enabled() -> bool: + value = os.environ.get(WORKTREE_ISOLATION_ENV, "").strip().lower() + if value in ("true", "1", "yes", "on"): + return True + if value in ("false", "0", "no", "off"): + return False + return not (get_shared_build_cache_path() / SUPPRESS_WORKTREE_ISOLATION_FILE).exists() + + +def get_isolated_worktree_path() -> Path | None: + """ + Return the path of the linked worktree that owns this checkout's Docker resources. + + ``None`` means the resources are shared: the main checkout, or a worktree with isolation disabled. + """ + if get_main_git_dir_for_worktree() is None or not is_worktree_isolation_enabled(): + return None + return AIRFLOW_ROOT_PATH.resolve() + + def get_default_project_name() -> str: - if get_main_git_dir_for_worktree() is None: + worktree = get_isolated_worktree_path() + if worktree is None: return "breeze" - name = re.sub(r"[^a-z0-9_-]", "-", AIRFLOW_ROOT_PATH.resolve().name.lower()) - return f"breeze-{name}" + name = re.sub(r"[^a-z0-9_-]", "-", worktree.name.lower()) + # Worktrees with the same directory name in different locations or clones must not share a project. + path_hash = hashlib.sha256(str(worktree).encode()).hexdigest()[:6] + return f"breeze-{name}-{path_hash}" + + +def get_host_id() -> str: + """Identify the machine whose filesystem decides whether a labelled worktree still exists.""" + return socket.gethostname() Review Comment: Minor: `socket.gethostname()` can change on macOS. If it does, the old resources look like another machine's, and the stale-worktree sweep skips them for good. For example, when `scutil --get HostName` is not set (the default), the name follows the LocalHostName, and macOS adds a number to it when another device on the network already uses the name. Mine already has a `-2` suffix. After such a rename, resources of a deleted worktree stay until someone runs `breeze down --all-worktrees`. Would a random id written once to a file under the user's home work here? ########## dev/breeze/src/airflow_breeze/utils/docker_command_utils.py: ########## @@ -827,12 +867,21 @@ def remove_stale_worktree_containers() -> None: 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): + container_id, worktree, host = (line.split("\t") + ["", ""])[:3] + # `docker ps` prints "<no value>" for containers created before the host label existed. + if _belongs_to_deleted_worktree(worktree, "" if host == "<no value>" else host): console_print(f"Removing container {container_id} for deleted worktree {worktree}") run_command(["docker", "rm", "--force", "--volumes", container_id], check=False) +def _belongs_to_deleted_worktree(worktree: str, host: str) -> bool: + # Another host's paths cannot be checked here. Resources created before the host label existed + # have no host and keep the local-path check. + if host and host != get_host_id(): + return False + return _worktree_is_missing(worktree) Review Comment: The sweep here and the watcher now each have their own "is this worktree deleted" check, and they already differ a bit (this one skips relative paths and warns on `OSError`). For example, a fix to the watcher's check for the leftover-directories case would not reach this sweep, which has the same problem. This module already imports `worktree_watcher`, so `_worktree_is_missing` could call `worktree_watcher.is_worktree_missing` and keep only the relative-path guard and the warning here. -- 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]
