Re: [PR] fix: handle PermissionError in init_log_folder for mounted filesystems [airflow]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #63878:
URL: https://github.com/apache/airflow/pull/63878#discussion_r3025338539


##########
airflow-core/src/airflow/logging_config.py:
##########
@@ -155,10 +155,19 @@ def configure_logging():
 
     base_log_folder = conf.get("logging", "base_log_folder")
 
-    return init_log_folder(
-        base_log_folder,
-        new_folder_permissions=new_folder_permissions,
-    )
+    try:
+        return init_log_folder(
+            base_log_folder,
+            new_folder_permissions=new_folder_permissions,
+        )
+    except PermissionError as e:
+        log.warning(
+            "Could not create log folder %s: %s. "
+            "Airflow will continue but logging to this directory may fail.",
+            base_log_folder,
+            e,
+        )
+        return None

Review Comment:
   The `try/except PermissionError` wrapper here is likely redundant: `init_log_folder()` already catches `PermissionError` and logs a warning (see `airflow/_shared/logging/structlog.py` / `airflow_shared/logging/structlog.py`). This block will never be hit, and it duplicates the warning message. Consider removing the wrapper, or broaden it to catch errors that `init_log_folder()` intentionally does not handle.
   ```suggestion
       return init_log_folder(
           base_log_folder,
           new_folder_permissions=new_folder_permissions,
       )
   ```



##########
shared/logging/tests/logging/test_structlog.py:
##########
@@ -512,6 +513,18 @@ def spy(et, ev, tb):
         sys.excepthook = original
 
 
+def test_init_log_folder_permission_error_logs_warning_and_continues(caplog):
+    from airflow_shared.logging.structlog import init_log_folder
+
+    with mock.patch.object(Path, "mkdir", side_effect=PermissionError("not allowed")):
+        with caplog.at_level(logging.WARNING):
+            init_log_folder("/tmp/blocked", 0o775)

Review Comment:
   This file-level comment says these tests avoid `caplog`, but this new test uses it. Either adjust the comment to note the exception, or capture the warning via the existing structlog testing helpers to keep the test style consistent within this file.



##########
shared/logging/tests/logging/test_structlog.py:
##########
@@ -512,6 +513,18 @@ def spy(et, ev, tb):
         sys.excepthook = original
 
 
+def test_init_log_folder_permission_error_logs_warning_and_continues(caplog):
+    from airflow_shared.logging.structlog import init_log_folder
+
+    with mock.patch.object(Path, "mkdir", side_effect=PermissionError("not allowed")):
+        with caplog.at_level(logging.WARNING):
+            init_log_folder("/tmp/blocked", 0o775)
+            continued_after_call = True
+
+    assert continued_after_call

Review Comment:
   `continued_after_call` is only used to assert the code path continued; since the assignment itself would never run if an exception was raised, this assertion is redundant. You can simplify the test by removing the flag and just asserting on the warning in `caplog` (or using a no-exception assertion pattern).
   ```suggestion
   
   ```



##########
shared/logging/src/airflow_shared/logging/structlog.py:
##########
@@ -689,9 +689,15 @@ def init_log_folder(directory: str | os.PathLike[str], new_folder_permissions: i
     user.
     """
     directory = Path(directory)
-    for parent in reversed(Path(directory).parents):
-        parent.mkdir(mode=new_folder_permissions, exist_ok=True)
-    directory.mkdir(mode=new_folder_permissions, exist_ok=True)
+    try:
+        directory.mkdir(mode=new_folder_permissions, parents=True, exist_ok=True)
+    except PermissionError as e:
+        log.warning(
+            "Could not create log folder %s: %s. "
+            "Airflow will continue but logging to this directory may fail.",
+            directory,
+            e,
+        )

Review Comment:
   `init_log_folder()` only catches `PermissionError`, but the PR description says it should also handle `OSError`. `Path.mkdir(..., parents=True)` can raise other `OSError` subclasses (e.g. read-only filesystem), which would still crash callers. Consider catching `(PermissionError, OSError)` here (or `OSError` if you want all filesystem failures handled uniformly) and logging the warning.



-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org