Stop writing to config dir log file on supervised install (#146675)

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

homeassistant/bootstrap.py

@@ -616,34 +616,44 @@ async def async_enable_logging(
         ),
     )
 
-    # Log errors to a file if we have write access to file or config dir
+    logger = logging.getLogger()
+    logger.setLevel(logging.INFO if verbose else logging.WARNING)
+
     if log_file is None:
-        err_log_path = hass.config.path(ERROR_LOG_FILENAME)
+        default_log_path = hass.config.path(ERROR_LOG_FILENAME)
+        if "SUPERVISOR" in os.environ:
+            _LOGGER.info("Running in Supervisor, not logging to file")
+            # Rename the default log file if it exists, since previous versions created
+            # it even on Supervisor
+            if os.path.isfile(default_log_path):
+                with contextlib.suppress(OSError):
+                    os.rename(default_log_path, f"{default_log_path}.old")
+            err_log_path = None
+        else:
+            err_log_path = default_log_path
     else:
         err_log_path = os.path.abspath(log_file)
 
-    err_path_exists = os.path.isfile(err_log_path)
-    err_dir = os.path.dirname(err_log_path)
+    if err_log_path:
+        err_path_exists = os.path.isfile(err_log_path)
+        err_dir = os.path.dirname(err_log_path)
 
-    # Check if we can write to the error log if it exists or that
-    # we can create files in the containing directory if not.
-    if (err_path_exists and os.access(err_log_path, os.W_OK)) or (
-        not err_path_exists and os.access(err_dir, os.W_OK)
-    ):
-        err_handler = await hass.async_add_executor_job(
-            _create_log_file, err_log_path, log_rotate_days
-        )
+        # Check if we can write to the error log if it exists or that
+        # we can create files in the containing directory if not.
+        if (err_path_exists and os.access(err_log_path, os.W_OK)) or (
+            not err_path_exists and os.access(err_dir, os.W_OK)
+        ):
+            err_handler = await hass.async_add_executor_job(
+                _create_log_file, err_log_path, log_rotate_days
+            )
 
-        err_handler.setFormatter(logging.Formatter(fmt, datefmt=FORMAT_DATETIME))
+            err_handler.setFormatter(logging.Formatter(fmt, datefmt=FORMAT_DATETIME))
+            logger.addHandler(err_handler)
 
-        logger = logging.getLogger()
-        logger.addHandler(err_handler)
-        logger.setLevel(logging.INFO if verbose else logging.WARNING)
-
-        # Save the log file location for access by other components.
-        hass.data[DATA_LOGGING] = err_log_path
-    else:
-        _LOGGER.error("Unable to set up error log %s (access denied)", err_log_path)
+            # Save the log file location for access by other components.
+            hass.data[DATA_LOGGING] = err_log_path
+        else:
+            _LOGGER.error("Unable to set up error log %s (access denied)", err_log_path)
 
     async_activate_log_queue_handler(hass)
 

tests/test_bootstrap.py

@@ -38,6 +38,17 @@ from .common import (
 
 VERSION_PATH = os.path.join(get_test_config_dir(), config_util.VERSION_FILE)
 
+CONFIG_LOG_FILE = get_test_config_dir("home-assistant.log")
+ARG_LOG_FILE = "test.log"
+
+
+def cleanup_log_files() -> None:
+    """Remove all log files."""
+    for f in glob.glob(f"{CONFIG_LOG_FILE}*"):
+        os.remove(f)
+    for f in glob.glob(f"{ARG_LOG_FILE}*"):
+        os.remove(f)
+
 
 @pytest.fixture(autouse=True)
 def disable_installed_check() -> Generator[None]:
@@ -85,16 +96,11 @@ async def test_async_enable_logging(
     hass: HomeAssistant, caplog: pytest.LogCaptureFixture
 ) -> None:
     """Test to ensure logging is migrated to the queue handlers."""
-    config_log_file_pattern = get_test_config_dir("home-assistant.log*")
-    arg_log_file_pattern = "test.log*"
 
     # Ensure we start with a clean slate
-    for f in glob.glob(arg_log_file_pattern):
-        os.remove(f)
-    for f in glob.glob(config_log_file_pattern):
-        os.remove(f)
-    assert len(glob.glob(config_log_file_pattern)) == 0
-    assert len(glob.glob(arg_log_file_pattern)) == 0
+    cleanup_log_files()
+    assert len(glob.glob(CONFIG_LOG_FILE)) == 0
+    assert len(glob.glob(ARG_LOG_FILE)) == 0
 
     with (
         patch("logging.getLogger"),
@@ -108,7 +114,7 @@ async def test_async_enable_logging(
     ):
         await bootstrap.async_enable_logging(hass)
         mock_async_activate_log_queue_handler.assert_called_once()
-        assert len(glob.glob(config_log_file_pattern)) > 0
+        assert len(glob.glob(CONFIG_LOG_FILE)) > 0
 
         mock_async_activate_log_queue_handler.reset_mock()
         await bootstrap.async_enable_logging(
@@ -117,14 +123,61 @@ async def test_async_enable_logging(
             log_file="test.log",
         )
         mock_async_activate_log_queue_handler.assert_called_once()
-        assert len(glob.glob(arg_log_file_pattern)) > 0
+        assert len(glob.glob(ARG_LOG_FILE)) > 0
 
     assert "Error rolling over log file" in caplog.text
 
-    for f in glob.glob(arg_log_file_pattern):
-        os.remove(f)
-    for f in glob.glob(config_log_file_pattern):
-        os.remove(f)
+    cleanup_log_files()
+
+
+async def test_async_enable_logging_supervisor(
+    hass: HomeAssistant, caplog: pytest.LogCaptureFixture
+) -> None:
+    """Test to ensure the default log file is not created on Supervisor installations."""
+
+    # Ensure we start with a clean slate
+    cleanup_log_files()
+    assert len(glob.glob(CONFIG_LOG_FILE)) == 0
+    assert len(glob.glob(ARG_LOG_FILE)) == 0
+
+    with (
+        patch.dict(os.environ, {"SUPERVISOR": "1"}),
+        patch(
+            "homeassistant.bootstrap.async_activate_log_queue_handler"
+        ) as mock_async_activate_log_queue_handler,
+        patch("logging.getLogger"),
+    ):
+        await bootstrap.async_enable_logging(hass)
+        assert len(glob.glob(CONFIG_LOG_FILE)) == 0
+        mock_async_activate_log_queue_handler.assert_called_once()
+        mock_async_activate_log_queue_handler.reset_mock()
+
+        # Check that if the log file exists, it is renamed
+        def write_log_file():
+            with open(
+                get_test_config_dir("home-assistant.log"), "w", encoding="utf8"
+            ) as f:
+                f.write("test")
+
+        await hass.async_add_executor_job(write_log_file)
+        assert len(glob.glob(CONFIG_LOG_FILE)) == 1
+        assert len(glob.glob(f"{CONFIG_LOG_FILE}.old")) == 0
+        await bootstrap.async_enable_logging(hass)
+        assert len(glob.glob(CONFIG_LOG_FILE)) == 0
+        assert len(glob.glob(f"{CONFIG_LOG_FILE}.old")) == 1
+        mock_async_activate_log_queue_handler.assert_called_once()
+        mock_async_activate_log_queue_handler.reset_mock()
+
+        await bootstrap.async_enable_logging(
+            hass,
+            log_rotate_days=5,
+            log_file="test.log",
+        )
+        mock_async_activate_log_queue_handler.assert_called_once()
+        # Even on Supervisor, the log file should be created if it is explicitly specified
+        assert len(glob.glob(ARG_LOG_FILE)) > 0
+
+    cleanup_log_files()
 
 
 async def test_load_hassio(hass: HomeAssistant) -> None: