fix(log2ram): keep journal protection enabled until disk migration

.github/scripts/tests/test_log2ram_setup.py

@@ -5,6 +5,7 @@ and interval the host already has."""
 import os
 from pathlib import Path
 import shutil
+import shlex
 import subprocess
 import tempfile
 import unittest
@@ -25,10 +26,176 @@ class KeepOnDiskTests(unittest.TestCase):
         (self.log / "journal/abc/system.journal").write_text("entries")
         (self.log / "pveproxy").mkdir()
         (self.log / "pveproxy/access.log").write_text("GET /")
+        self.config = self.root / "etc/log2ram.conf"
+        self.config.parent.mkdir()
+        self.config.write_text("SIZE=128M\nJOURNALD_AWARE=true\nNOTIFICATION=true\n")
+
+    def run_setup(self, edit_failure=False):
+        # Extract only the shared setup function, never an administrative caller.
+        body = SETUP.read_text().split("pmx_log2ram_setup() {", 1)[1].split("\n}\n", 1)[0]
+        body = body.replace("/etc/", str(self.root / "etc") + "/")
+        for directory in ("cron.d", "tmpfiles.d"):
+            (self.root / "etc" / directory).mkdir(exist_ok=True)
+        stubs = r'''
+translate() { printf '%s' "$1"; }
+msg_info() { :; }
+msg_ok() { :; }
+pmx_install_pkg() { :; }
+_pmx_log2ram_install_release() { :; }
+_pmx_log2ram_pbs_logrotate() { :; }
+_pmx_log2ram_remove_legacy() { :; }
+pmx_edit_file() { local file="$1"; shift; sed -i "$@" "$file"; }
+pmx_append_file() { cat >> "$1"; }
+pmx_write_file() { cat > "$1"; }
+pmx_remove_file() { rm -f "$1"; }
+pmx_apply_setting() { :; }
+systemctl() { printf '%s\n' "$*" >> "$SYSTEMCTL_TRACE"; }
+'''
+        if edit_failure:
+            # A failed configuration writer must be handled even by root runners.
+            stubs += "\npmx_edit_file() { return 1; }\n"
+        script = (stubs + "\npmx_log2ram_setup() {" + body + "\n}\n"
+                  + "pmx_log2ram_setup 256 3 false\n")
+        env = dict(os.environ, SYSTEMCTL_TRACE=str(self.root / "systemctl.trace"),
+                   PMX_L2R_TAG="1.7.2", PMX_L2R_SCRIPTS=str(KEEP.parent),
+                   PMX_L2R_REVERT_UNIT=str(self.root / "etc/revert.service"))
+        return subprocess.run(["bash", "-c", script], env=env, capture_output=True, text=True, timeout=10)
+
+    def test_setup_keeps_journal_aware_syncing_until_boot(self):
+        self.config.write_text("SIZE=128M\nJOURNALD_AWARE=false\nNOTIFICATION=true\n")
+        result = self.run_setup()
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertIn("JOURNALD_AWARE=true\n", self.config.read_text())
+        self.assertIn("SIZE=256M\n", self.config.read_text())
+        self.assertFalse((self.log / "journal").is_symlink())
+        self.assertEqual((self.root / "systemctl.trace").read_text(), "daemon-reload\n")
+
+    def test_boot_disables_journal_aware_syncing_after_relocation(self):
+        self.assertEqual(self.run_setup().returncode, 0)
+        result = self.run_script()
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertEqual(os.readlink(self.log / "journal"), "/var/hdd.journal")
+        self.assertEqual((self.root / "var/hdd.journal/abc/system.journal").read_text(), "entries")
+        self.assertIn("JOURNALD_AWARE=false\n", self.config.read_text())
+        self.assertIn("SIZE=256M\n", self.config.read_text())
+        # Reapplying the repair to an already-relocated installation is safe.
+        self.assertEqual(self.run_setup().returncode, 0)
+        self.assertEqual(self.run_script().returncode, 0)
+        self.assertIn("JOURNALD_AWARE=false\n", self.config.read_text())
+        self.assertEqual(sorted(p.name for p in (self.root / "var/hdd.journal").iterdir()), ["abc"])
+
+    def test_boot_migration_failure_is_not_ignored_by_startup(self):
+        self.assertEqual(self.run_setup().returncode, 0)
+        # A non-directory destination causes a real copy failure, without mounts.
+        conflict = self.root / "var/hdd.journal"
+        conflict.write_text("blocked")
+        unit = self.root / "etc/systemd/system/log2ram.service.d/proxmenux.conf"
+        command = next(line.split("=", 1)[1] for line in unit.read_text().splitlines()
+                       if line.startswith("ExecStartPre="))
+        result = subprocess.run(["bash", *shlex.split(command.lstrip("-"))],
+                                env=dict(os.environ, PMX_L2R_ROOT=str(self.root)),
+                                capture_output=True, text=True, timeout=10)
+        self.assertNotEqual(result.returncode, 0, result.stderr)
+        self.assertIn("JOURNALD_AWARE=true\n", self.config.read_text())
+        self.assertFalse(command.startswith("-"), "systemd must not ignore the helper's failure")
+        conflict.unlink()
+        self.assertEqual(self.run_script().returncode, 0)
+        self.assertIn("JOURNALD_AWARE=false\n", self.config.read_text())
+
+    def test_setup_adds_journal_aware_setting_when_missing(self):
+        self.config.write_text("SIZE=128M\n#JOURNALD_AWARE=false\n")
+        result = self.run_setup()
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertIn("\nJOURNALD_AWARE=true\n", self.config.read_text())
+        self.assertEqual(self.run_script().returncode, 0)
+        self.assertIn("\nJOURNALD_AWARE=false\n", self.config.read_text())
+
+    def check_missing_setting_without_final_newline(self, content):
+        self.config.write_text(content)
+        result = self.run_setup()
+        self.assertEqual(result.returncode, 0, result.stderr)
+        for expected in ("true", "false"):
+            with self.subTest(journal_aware=expected):
+                # Read shell semantics in a fresh Bash, not a substring match.
+                sourced = subprocess.run(
+                    ["bash", "--noprofile", "--norc", "-c",
+                     'unset JOURNALD_AWARE NOTIFICATION; source "$1"; '
+                     'printf "%s\\n%s\\n" "${JOURNALD_AWARE-UNSET}" "${NOTIFICATION-UNSET}"',
+                     "fixture", str(self.config)],
+                    capture_output=True, text=True, timeout=10)
+                self.assertEqual(sourced.returncode, 0, sourced.stderr)
+                self.assertEqual(sourced.stdout, expected + "\nfalse\n")
+
+            if expected == "true":
+                self.assertFalse((self.log / "journal").is_symlink())
+                self.assertEqual((self.log / "journal/abc/system.journal").read_text(), "entries")
+                result = self.run_script()
+                self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertEqual(os.readlink(self.log / "journal"), "/var/hdd.journal")
+        self.assertEqual((self.root / "var/hdd.journal/abc/system.journal").read_text(), "entries")
+
+    def test_missing_setting_after_comment_without_final_newline(self):
+        self.check_missing_setting_without_final_newline(
+            "SIZE=128M\nNOTIFICATION=false\n#JOURNALD_AWARE=false")
+
+    def test_missing_setting_after_assignment_without_final_newline(self):
+        self.check_missing_setting_without_final_newline("SIZE=128M\nNOTIFICATION=false")
+
+    def test_setup_reports_configuration_edit_failure(self):
+        self.config.write_text("SIZE=128M\nJOURNALD_AWARE=false\nNOTIFICATION=false\n")
+        result = self.run_setup(edit_failure=True)
+        self.assertNotEqual(result.returncode, 0, result.stderr)
+        self.assertIn("JOURNALD_AWARE=false\n", self.config.read_text())
+
+    def test_an_existing_link_with_an_unusable_disk_target_fails(self):
+        self.assertEqual(self.run_script().returncode, 0)
+        self.config.write_text("JOURNALD_AWARE=true\n")
+        disk = self.root / "var/hdd.journal"
+        shutil.rmtree(disk)
+        disk.write_text("blocked")
+        result = self.run_script()
+        self.assertNotEqual(result.returncode, 0, result.stderr)
+        self.assertEqual(self.config.read_text(), "JOURNALD_AWARE=true\n")
+
+    def test_an_unexpected_journal_link_does_not_complete_migration(self):
+        shutil.rmtree(self.log / "journal")
+        (self.log / "journal").symlink_to("pveproxy")
+        result = self.run_script()
+        self.assertNotEqual(result.returncode, 0, result.stderr)
+        self.assertEqual(os.readlink(self.log / "journal"), "pveproxy")
+        self.assertIn("JOURNALD_AWARE=true\n", self.config.read_text())
+
+    def test_a_missing_journal_with_an_unusable_disk_target_fails(self):
+        shutil.rmtree(self.log / "journal")
+        (self.root / "var/hdd.journal").write_text("blocked")
+        result = self.run_script()
+        self.assertNotEqual(result.returncode, 0, result.stderr)
+        self.assertIn("JOURNALD_AWARE=true\n", self.config.read_text())
+        self.assertFalse((self.log / "journal").is_symlink())
+
+    def test_failed_source_removal_does_not_complete_migration(self):
+        (self.root / "var/hdd.journal").mkdir()
+        result = subprocess.run(
+            ["bash", "-c", 'rm() { return 1; }; export -f rm; exec bash "$1"', "fixture", str(KEEP)],
+            env=dict(os.environ, PMX_L2R_ROOT=str(self.root)), capture_output=True, text=True, timeout=10)
+        self.assertNotEqual(result.returncode, 0, result.stderr)
+        self.assertIn("JOURNALD_AWARE=true\n", self.config.read_text())
+        self.assertTrue((self.log / "journal/abc/system.journal").is_file())
+
+    def test_boot_configuration_failure_is_reported_and_can_be_retried(self):
+        self.config.unlink()
+        self.config.mkdir()
+        result = self.run_script()
+        self.assertNotEqual(result.returncode, 0, result.stderr)
+        self.assertEqual(os.readlink(self.log / "journal"), "/var/hdd.journal")
+        self.config.rmdir()
+        self.config.write_text("JOURNALD_AWARE=true\n")
+        self.assertEqual(self.run_script().returncode, 0)
+        self.assertEqual(self.config.read_text(), "JOURNALD_AWARE=false\n")
 
     def run_script(self, revert=False):
         env = dict(os.environ, PMX_L2R_ROOT=str(self.root), PMX_L2R_REVERT="1" if revert else "0")
-        return subprocess.run(["bash", str(KEEP)], env=env, capture_output=True, text=True)
+        return subprocess.run(["bash", str(KEEP)], env=env, capture_output=True, text=True, timeout=10)
 
     def test_the_journal_moves_to_disk_behind_a_link(self):
         self.assertEqual(self.run_script().returncode, 0)
@@ -78,6 +245,7 @@ class KeepOnDiskTests(unittest.TestCase):
     def test_removing_log2ram_puts_the_folders_back(self):
         (self.log / "proxmox-backup").mkdir()
         self.run_script()
+        self.config.unlink()  # Revert must not require or rewrite Log2RAM settings.
         self.assertEqual(self.run_script(revert=True).returncode, 0)
         for name in ("journal", "proxmox-backup"):
             self.assertFalse((self.log / name).is_symlink(), name)
@@ -99,7 +267,7 @@ class KeptSettingsTests(unittest.TestCase):
             library = Path(folder) / "setup.sh"
             library.write_text(script)
             result = subprocess.run(["bash", "-c", f'source "{library}"; {function}'],
-                                    capture_output=True, text=True)
+                                    capture_output=True, text=True, timeout=10)
             return result.stdout.strip()
 
     def test_the_size_comes_from_the_installed_configuration(self):
@@ -114,6 +282,24 @@ class KeptSettingsTests(unittest.TestCase):
         cron = "MAILTO=\"\"\n0 */3 * * * root /usr/local/bin/log2ram write >/dev/null 2>&1\n"
         self.assertEqual(self.read("pmx_log2ram_current_hours", {"/etc/cron.d/log2ram": cron}), "3")
 
+    def test_both_modes_offer_a_repair_to_version_2_0_installs(self):
+        for filename, function, updater in (
+            ("auto_post_install.sh", "install_log2ram_auto", "_update_existing_log2ram_auto"),
+            ("customizable_post_install.sh", "configure_log2ram", "_update_existing_log2ram_custom"),
+        ):
+            with self.subTest(mode=filename), tempfile.TemporaryDirectory() as folder:
+                config = Path(folder) / "log2ram.conf"
+                config.write_text("JOURNALD_AWARE=false\n")
+                source = (ROOT / "scripts/post_install" / filename).read_text()
+                body = source.split(function + "() {", 1)[1].split("\n}\n", 1)[0]
+                body = body.replace("/etc/log2ram.conf", str(config))
+                script = ("pmx_journal_context() { :; }\n"
+                          + updater + "() { printf '%s\\n' \"$1\"; }\n"
+                          + function + "() {" + body + "\n}\n" + function)
+                result = subprocess.run(["bash", "-c", script], capture_output=True, text=True, timeout=10)
+                self.assertEqual(result.returncode, 0, result.stderr)
+                self.assertEqual(result.stdout.strip(), "2.1")
+
     def test_the_release_is_pinned_to_a_verified_commit(self):
         text = SETUP.read_text()
         self.assertIn('PMX_L2R_TAG="1.7.2"', text)

scripts/global/log2ram_keep_on_disk.sh

@@ -30,20 +30,21 @@ keep_on_disk() {
     local place="$LOG_DIR/$name" disk="$ROOT/var/hdd.$name"
 
     if [ -L "$place" ]; then
-        [ -d "$disk" ] || mkdir -p "$disk"
+        [ "$(readlink "$place")" = "/var/hdd.$name" ] || return 1
+        [ -d "$disk" ] || mkdir -p "$disk" || return 1
         return 0
     fi
     # The journal is always kept on disk; any other folder only where it exists.
     if [ ! -d "$place" ]; then
         [ "$name" = journal ] || return 0
-        mkdir -p "$disk"
+        mkdir -p "$disk" || return 1
         chown root:systemd-journal "$disk" 2>/dev/null || true
         chmod 2755 "$disk"
     elif [ ! -e "$disk" ]; then
         mv "$place" "$disk" || return 1
     else
         cp -a "$place/." "$disk/" || return 1
-        rm -rf "${place:?}"
+        rm -rf "${place:?}" || return 1
     fi
     ln -s "/var/hdd.$name" "$place"
 }
@@ -70,4 +71,8 @@ for name in $KEPT_ON_DISK; do
         keep_on_disk "$name" || status=1
     fi
 done
+# Only a completed boot migration makes it safe to skip journal rotation.
+if [ "$status" = 0 ] && [ "${PMX_L2R_REVERT:-0}" != 1 ]; then
+    sed -i 's/^JOURNALD_AWARE=.*/JOURNALD_AWARE=false/' "$ROOT/etc/log2ram.conf" || status=1
+fi
 exit "$status"

scripts/global/log2ram_setup.sh

@@ -112,19 +112,22 @@ pmx_log2ram_setup() {
     _pmx_log2ram_install_release || return 1
     msg_ok "$(translate "Log2RAM installed:") ${PMX_L2R_TAG}"
 
-    # The journal is not in RAM, so Log2RAM does not rotate it before a sync;
+    # Keep journal-aware syncing through shutdown: the running journal may
-    # and Proxmox has no mail command for its error notice.
+    # still be in RAM. The boot helper disables it only after relocation.
+    # Proxmox has no mail command for the error notice.
     pmx_edit_file /etc/log2ram.conf \
         -e "s/^SIZE=.*/SIZE=${size_mb}M/" \
-        -e "s/^JOURNALD_AWARE=.*/JOURNALD_AWARE=false/" \
+        -e "s/^JOURNALD_AWARE=.*/JOURNALD_AWARE=true/" \
-        -e "s/^#\?NOTIFICATION=.*/NOTIFICATION=false/"
+        -e "s/^#\?NOTIFICATION=.*/NOTIFICATION=false/" || return 1
+    grep -q '^JOURNALD_AWARE=' /etc/log2ram.conf || \
+        printf '\nJOURNALD_AWARE=true\n' | pmx_append_file /etc/log2ram.conf || return 1
     grep -q '^NOTIFICATION=' /etc/log2ram.conf || printf 'NOTIFICATION=false\n' | pmx_append_file /etc/log2ram.conf
 
     mkdir -p /etc/systemd/system/log2ram.service.d /etc/systemd/system/log2ram-daily.timer.d
     pmx_write_file /etc/systemd/system/log2ram.service.d/proxmenux.conf <<EOF
 # ProxMenux: the journal stays on disk, and a sync is on disk when it ends.
 [Service]
-ExecStartPre=-${PMX_L2R_SCRIPTS}/log2ram_keep_on_disk.sh
+ExecStartPre=${PMX_L2R_SCRIPTS}/log2ram_keep_on_disk.sh
 ExecReload=
 ExecReload=/usr/local/bin/log2ram write
 ExecReload=/bin/sync -f /var/hdd.log

scripts/post_install/auto_post_install.sh

@@ -973,7 +973,7 @@ _update_existing_log2ram_auto() {
 
 
 install_log2ram_auto() {
-    local FUNC_VERSION="2.0"
+    local FUNC_VERSION="2.1"
     local existing_log2ram_bin=""
     pmx_journal_context "install_log2ram_auto" "$FUNC_VERSION"
 

scripts/post_install/customizable_post_install.sh

@@ -3303,7 +3303,7 @@ _update_existing_log2ram_custom() {
 
 
 configure_log2ram() {
-    local FUNC_VERSION="2.0"
+    local FUNC_VERSION="2.1"
     local existing_log2ram_bin=""
     local watch=false
     pmx_journal_context "configure_log2ram" "$FUNC_VERSION"