guan404ming commented on code in PR #74227:
URL: https://github.com/apache/airflow/pull/74227#discussion_r4184383049
##########
task-sdk/src/airflow/sdk/coordinators/executable/coordinator.py:
##########
@@ -284,22 +284,51 @@ def _walk_executables(
children = list(item.iterdir())
except OSError:
continue
- yield from _walk_executables(children, seen_dirs)
- elif stat.S_ISREG(st.st_mode) and os.access(item, os.X_OK):
+ yield from _walk_bundle_files(children, seen_dirs)
+ elif stat.S_ISREG(st.st_mode):
yield item
+def _mark_executable(path: pathlib.Path) -> bool:
+ """
+ Add the execute bit for each read bit already set on *path*, if missing.
+
+ An object-store Dag bundle (for example ``S3DagBundle``) has no concept of
+ file permissions, so a bundle synced from one always loses its execute bit.
+ By the time this is called the file has already passed the footer-magic and
+ binary_sha256 checks in :func:`_read_bundle_metadata`, so marking it
+ executable is safe.
+ """
+ try:
+ mode = path.stat().st_mode
+ except OSError as exc:
+ log.debug("Cannot stat bundle file; skipping", path=str(path),
error=str(exc))
+ return False
+
+ wanted = mode | ((mode & 0o444) >> 2)
+ if wanted == mode:
+ return True
+ try:
+ path.chmod(wanted)
+ except OSError as exc:
+ log.debug("Cannot set executable bit on bundle; skipping",
path=str(path), error=str(exc))
+ return False
+ return True
+
+
@attrs.define
class _Bundle(ResolvedBundle):
@classmethod
def find(cls, roots: Sequence[pathlib.Path], dag_id: str) -> Self:
log.debug("Finding executable bundles recursively", roots=roots)
rejected: list[tuple[pathlib.Path, str]] = []
- for p in _find_executables(roots):
+ for p in _find_bundle_files(roots):
if (metadata := _read_bundle_metadata(p)) is None:
continue
if dag_id not in _dag_ids(metadata):
continue
+ if not _mark_executable(p):
Review Comment:
Would it be a bit safer to chmod only the bundle we return? Right now
rejected bundles get marked too.
##########
task-sdk/src/airflow/sdk/coordinators/executable/coordinator.py:
##########
@@ -284,22 +284,51 @@ def _walk_executables(
children = list(item.iterdir())
except OSError:
continue
- yield from _walk_executables(children, seen_dirs)
- elif stat.S_ISREG(st.st_mode) and os.access(item, os.X_OK):
+ yield from _walk_bundle_files(children, seen_dirs)
+ elif stat.S_ISREG(st.st_mode):
yield item
+def _mark_executable(path: pathlib.Path) -> bool:
+ """
+ Add the execute bit for each read bit already set on *path*, if missing.
+
+ An object-store Dag bundle (for example ``S3DagBundle``) has no concept of
+ file permissions, so a bundle synced from one always loses its execute bit.
+ By the time this is called the file has already passed the footer-magic and
+ binary_sha256 checks in :func:`_read_bundle_metadata`, so marking it
+ executable is safe.
+ """
+ try:
+ mode = path.stat().st_mode
+ except OSError as exc:
+ log.debug("Cannot stat bundle file; skipping", path=str(path),
error=str(exc))
+ return False
+
+ wanted = mode | ((mode & 0o444) >> 2)
+ if wanted == mode:
Review Comment:
I think a `0o744` bundle on a read-only mount would now be skipped;
returning early when `os.access(path, os.X_OK)` holds could avoid that.
##########
task-sdk/src/airflow/sdk/coordinators/executable/coordinator.py:
##########
@@ -284,22 +284,51 @@ def _walk_executables(
children = list(item.iterdir())
except OSError:
continue
- yield from _walk_executables(children, seen_dirs)
- elif stat.S_ISREG(st.st_mode) and os.access(item, os.X_OK):
+ yield from _walk_bundle_files(children, seen_dirs)
+ elif stat.S_ISREG(st.st_mode):
yield item
+def _mark_executable(path: pathlib.Path) -> bool:
+ """
+ Add the execute bit for each read bit already set on *path*, if missing.
+
+ An object-store Dag bundle (for example ``S3DagBundle``) has no concept of
+ file permissions, so a bundle synced from one always loses its execute bit.
+ By the time this is called the file has already passed the footer-magic and
+ binary_sha256 checks in :func:`_read_bundle_metadata`, so marking it
+ executable is safe.
+ """
+ try:
+ mode = path.stat().st_mode
+ except OSError as exc:
+ log.debug("Cannot stat bundle file; skipping", path=str(path),
error=str(exc))
+ return False
+
+ wanted = mode | ((mode & 0o444) >> 2)
+ if wanted == mode:
+ return True
+ try:
+ path.chmod(wanted)
+ except OSError as exc:
+ log.debug("Cannot set executable bit on bundle; skipping",
path=str(path), error=str(exc))
Review Comment:
It might help to record the chmod failure in `rejected`, so users see the
real reason instead of "cannot find executable bundle".
--
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]
