Only consider real kernel module trees when detecting the latest kernel

[Cybis320]

Problem

should_reboot() picked the newest installed kernel with:

latest="$(ls /lib/modules/ | sort -V | tail -1)"

Not every entry in /lib/modules/ is a kernel version directory. On Ubuntu 24.04 with the NVIDIA driver packages installed, a literal kernel/ directory exists there and is owned by dpkg:

$ ls /lib/modules/
6.14.0-37-generic  6.17.0-23-generic  6.17.0-35-generic
6.17.0-40-generic  7.0.0-28-generic   kernel

$ dpkg -S /lib/modules/kernel
linux-modules-nvidia-595-open-6.17.0-40-generic,
linux-modules-nvidia-595-open-7.0.0-28-generic: /lib/modules/kernel

Since k sorts after digits, sort -V | tail -1 returns kernel, so the comparison on the next line always reports a mismatch. Observed on a live station with nothing actually pending (no /var/run/reboot-required, running kernel already the newest installed):

Jul 26 05:00:02 rms_updater: Kernel mismatch: running 7.0.0-28-generic, installed kernel

/var/run/reboot-required masks this on Ubuntu, so the fallback only misfires when that file is absent, i.e. exactly on the RPi OS / Debian systems the fallback exists to serve. On a machine with working passwordless sudo shutdown the consequence is one spurious reboot, after which do_reboot() stamps kernel into the stamp file, last_target == latest holds forever, and the loop guard permanently disables the fallback. One unnecessary reboot, then silent loss of the feature.

Deleting /lib/modules/kernel is not a fix — dpkg owns it and it returns on the next NVIDIA driver update.

Fix

Consider only entries that are version-shaped and contain a modules.dep, which depmod writes into every genuinely installed kernel's directory. sort -V ordering is unchanged, and the loop-guard/stamp logic is untouched — it behaves correctly once latest is right.

The modules.dep guard also filters half-removed kernels, which are version-shaped and would otherwise trigger the same spurious reboot. Against a directory containing both kernel/ and a purged-kernel leftover:

filter	result
current (`ls	sort -V
^[0-9] only	7.1.0-1-generic (the leftover)
^[0-9] + modules.dep	7.0.0-28-generic

Testing

• bash -n passes; shellcheck reports nothing new (the two existing SC2155/SC2106 warnings are elsewhere in the file and pre-existing).
• The repo has no shell-script test harness, so should_reboot() was exercised out-of-line with /lib/modules, /var/run/reboot-required and uname redirected at a fixture directory containing the layout above:
  • reboot-required present -> returns 0
  • running == newest installed -> returns 1 (this is the reported bug; previously returned 0)
  • running older than newest -> returns 0 with target 7.0.0-28-generic
  • same, with the stamp already holding that kernel -> returns 1 (loop guard intact)
  • empty /lib/modules -> returns 1

[dvida]

Reviewed this carefully — the diagnosis is right, the fix is a strict improvement, and the writeup made it easy to verify. What I checked, then one thing I think should go in before merge.

Verified

• bash -n passes on bbc4c07.
• Reproduced the reported behaviour on a fixture containing kernel/, three real kernel trees and a version-shaped purged leftover: old code → kernel, this branch → 7.0.0-28-generic. Matches your table.
• Empty or missing /lib/modules: the glob doesn't match, d becomes the literal *, the ^[0-9] test drops it, latest is empty, should_reboot returns 1. Also quieter than the old ls, which wrote to stderr.
• Safe under the script's set -Eeuo pipefail: every test is guarded by || continue, and the pipeline ends in tail -1, which always exits 0.
• modules.dep is a good sentinel — depmod writes it from the kernel package postinst, and if it were ever missing the failure direction is a missed reboot rather than a spurious one.

I could not reproduce /lib/modules/kernel on my box (no linux-modules-nvidia-* installed), so I'm taking the dpkg -S output at face value. The filter is the right defence regardless of that specific cause.

Finding 1 (major): flavour mixing reproduces the same bug

/lib/modules/ has one directory per installed flavour, not per version, and sort -V | tail -1 crosses flavours happily:

6.12.34+rpt-rpi-v8   vs  6.12.34+rpt-rpi-2712   ->  ...-rpi-v8     wins
7.0.0-28-generic     vs  7.0.0-28-lowlatency    ->  ...-lowlatency wins

Both entries survive the new ^[0-9] + modules.dep filter — they're genuine, fully installed kernels, so neither guard applies.

This lands on exactly the platform the fallback exists to serve. Raspberry Pi OS 64-bit ships both linux-image-rpi-v8 and linux-image-rpi-2712 on the same image, so a Pi 5 runs ...-rpi-2712 while latest resolves to ...-rpi-v8: permanent mismatch → one spurious reboot → do_reboot() stamps ...-rpi-v8 → loop guard matches forever → fallback silently dead. Same consequence chain you describe in the problem statement, reached from a different direction, and on RPi OS there's no /var/run/reboot-required to mask it. 32-bit Pi OS is worse — rpi-v6/v7/v7l/v8 can all be installed at once.

The fix is to also require the candidate to share the running kernel's flavour. This parse handles every naming scheme I could find:

uname -r	flavour
7.0.0-28-generic	generic
6.8.0-51-generic-64k	generic-64k
6.1.0-37-arm64	arm64
6.5.0-1010-raspi	raspi
6.12.34+rpt-rpi-2712	rpi-2712
6.6.51-v8+	v8+

Finding 2 (minor): inequality where ordering is meant

"$running" != "$latest" treats any difference as "newer kernel pending", so an older latest also triggers a reboot. Reachable if the running kernel's modules tree has been purged, and via the flavour skew above. Worth making it "only reboot if latest sorts strictly after running" while we're in here.

Finding 3 (nit): the loop can be half the size

Putting both tests in the glob is equivalent — same results on all my fixtures, including the no-match case:

for f in /lib/modules/[0-9]*/modules.dep; do
    [[ -f "$f" ]] || continue
    d="${f%/modules.dep}"; echo "${d##*/}"
done

The [[ -f "$f" ]] guard is still needed since nullglob isn't set.

Suggested block

All three rolled together. Tested under set -Eeuo pipefail against fixtures for: Pi 5 with both flavours plus kernel/ plus a purged leftover; Ubuntu generic+lowlatency; running kernel purged leaving only an older tree; and empty /lib/modules. All gave the expected verdict.

        # Fallback: compare running kernel to latest installed (works on RPi OS / Debian).
        local running latest last_target flavour d f
        running="$(uname -r)"

        # /lib/modules/ is not a clean list of pending kernels. The NVIDIA driver packages
        # own a literal /lib/modules/kernel/, purged kernels leave a version-shaped
        # directory behind, and every installed *flavour* gets its own tree (Raspberry Pi
        # OS ships both rpi-v8 and rpi-2712; Ubuntu can have generic and lowlatency).
        # Only a same-flavour, fully installed kernel is a valid reboot target, so keep
        # entries that are version-shaped, carry a modules.dep (written by depmod for
        # every installed kernel), and end in the running kernel's flavour.
        flavour=""
        [[ "$running" =~ ^[0-9][^-]*(-[0-9]+)?-(.+)$ ]] && flavour="${BASH_REMATCH[2]}"
        latest="$(for f in /lib/modules/[0-9]*/modules.dep; do
                      [[ -f "$f" ]] || continue
                      d="${f%/modules.dep}"; d="${d##*/}"
                      [[ -z "$flavour" || "$d" == *"-$flavour" ]] || continue
                      echo "$d"
                  done | sort -V | tail -1)"

        # Only reboot when the candidate is strictly newer, so a purged running-kernel
        # tree cannot trigger a "downgrade" reboot.
        if [[ -n "$latest" && "$running" != "$latest" \
              && "$(printf '%s\n%s\n' "$running" "$latest" | sort -V | tail -1)" == "$latest" ]]; then

Everything below that if — loop guard, stamp, REBOOT_KERNEL_TARGET — stays as you have it.

If you take Finding 1, please extend the out-of-line test matrix with: both rpi-v8 and rpi-2712 present while running rpi-2712 → returns 1; generic and lowlatency present while running generic → returns 1; and only an older tree present while running a newer kernel → returns 1.

[dvida]

Pushed the three fixes to this branch as 5f86803 — flavour match, strictly-newer guard, and the glob simplification. Your ^[0-9] + modules.dep reasoning is unchanged, both filters just moved into the glob pattern.

The flavour one is the reason I pushed rather than just commenting: on a Pi 5 the old and new code behave identically, because 6.12.34+rpt-rpi-v8 and 6.12.34+rpt-rpi-2712 are both real kernels with a real modules.dep. Pi OS 64-bit ships both images, sort -V puts v8 on top, and a station running rpi-2712 reports a mismatch on every run — same chain you traced, and RPi OS is where there's no reboot-required file to hide it.

Verified

bash -n passes. Still no shellcheck here, so I can't confirm your "nothing new" claim independently.

Since the repo has no shell test harness and Tests/ is Python-only, I didn't want to invent a convention inside a one-file fix, so the harness stayed out of the commit. It's below if you want it in — it extracts should_reboot() with awk, rewrites the two absolute paths at a fixture tree, and stubs uname/log_message. 16 cases, all passing, including your original five with unchanged expectations:

--- cases from the PR description (expectations unchanged) ---
ok   reboot-required present                              rc=0 target=[]
ok   running == newest installed (the reported bug)       rc=1 target=[]
ok   running older than newest                            rc=0 target=[7.0.0-28-generic]
ok   same, stamp already holds the target (loop guard)    rc=1 target=[]
ok   empty /lib/modules                                   rc=1 target=[]
ok   missing /lib/modules                                 rc=1 target=[]
--- Finding 1: flavour mixing ---
ok   Pi 5: running rpi-2712, newer rpi-v8 present         rc=1 target=[]
ok   Pi 4: running rpi-v8, rpi-2712 also installed        rc=1 target=[]
ok   Pi 5: genuine same-flavour upgrade pending           rc=0 target=[6.12.34+rpt-rpi-2712]
ok   Ubuntu: running generic, lowlatency installed        rc=1 target=[]
ok   flavour suffix with a dash (generic-64k)             rc=1 target=[]
ok   old RPi naming (6.6.51-v8+)                          rc=0 target=[6.6.62-v8+]
--- Finding 2: only reboot for a strictly newer kernel ---
ok   running kernel tree purged, only older left          rc=1 target=[]
ok   older and newer both present, running older          rc=0 target=[7.0.0-28-generic]
--- reboot mode passthrough ---
ok   --reboot always                                      rc=0 target=[]
ok   no reboot requested                                  rc=1 target=[]

all 16 passed

Against the real /lib/modules on my Ubuntu box (running 7.0.0-28-generic, newest installed 7.0.0-28-generic): returns 1, no target flagged.

Two things worth a second pair of eyes, since I can't test either:

• The flavour parse is ^[0-9][^-]*(-[0-9]+)?-(.+)$, group 2. It gives generic, generic-64k, arm64, raspi, rpi-2712, v8+ for the naming schemes I could find. If uname -r doesn't match at all, flavour is empty and the filter is skipped, so it degrades to the behaviour in your original commit rather than dropping every candidate.
• Could you confirm on the station that originally logged installed kernel that the fallback now stays quiet? That's the one case I have no way to reproduce — I have no linux-modules-nvidia-* installed, so no /lib/modules/kernel to test against.

Test harness

#!/bin/bash
# Out-of-line harness for should_reboot() in GRMSUpdater.sh.
# Extracts the function, redirects /lib/modules, /var/run/reboot-required and uname
# at fixtures, and asserts the return value plus the flagged kernel target.

set -Eeuo pipefail

SRC="/home/dvida/source/RMS/Scripts/MultiCamLinux/GRMSUpdater.sh"
WORK="$(mktemp -d)"
trap 'rm -rf "$WORK"' EXIT

MODDIR="$WORK/lib/modules"
FLAGFILE="$WORK/reboot-required"

# Pull out should_reboot() and rewrite the two absolute paths to point at the fixtures.
awk '/^should_reboot\(\) \{/,/^\}/' "$SRC" \
    | sed -e "s#/lib/modules#$MODDIR#g" -e "s#/var/run/reboot-required#$FLAGFILE#g" \
    > "$WORK/fn.sh"
grep -q 'should_reboot' "$WORK/fn.sh" || { echo "FAIL: could not extract should_reboot()"; exit 1; }
# shellcheck disable=SC1090
source "$WORK/fn.sh"

RUNNING=""
uname() { [[ "${1:-}" == "-r" ]] && echo "$RUNNING" || command uname "$@"; }
log_message() { LOG="$*"; }

REBOOT_MODE="if-needed"
REBOOT_STAMP_FILE="$WORK/stamp"
REBOOT_KERNEL_TARGET=""

fails=0
pass=0

# make_fixture <mods-with-modules.dep...> -- <bare-dirs...>
make_fixture() {
    rm -rf "$MODDIR"; mkdir -p "$MODDIR"
    local bare=0
    for e in "$@"; do
        if [[ "$e" == "--" ]]; then bare=1; continue; fi
        mkdir -p "$MODDIR/$e"
        (( bare )) || : > "$MODDIR/$e/modules.dep"
    done
}

# check <name> <running> <expected-rc> <expected-target>
check() {
    local name="$1" running="$2" want_rc="$3" want_target="$4" rc=0
    RUNNING="$running"; LOG=""
    should_reboot || rc=$?
    if [[ "$rc" == "$want_rc" && "${REBOOT_KERNEL_TARGET:-}" == "$want_target" ]]; then
        printf 'ok   %-52s rc=%s target=[%s]\n' "$name" "$rc" "${REBOOT_KERNEL_TARGET:-}"
        (( ++pass ))
    else
        printf 'FAIL %-52s rc=%s (want %s) target=[%s] (want [%s]) log=%s\n' \
            "$name" "$rc" "$want_rc" "${REBOOT_KERNEL_TARGET:-}" "$want_target" "$LOG"
        (( ++fails ))
    fi
}

rm -f "$REBOOT_STAMP_FILE" "$FLAGFILE"

echo "--- cases from the PR description (expectations unchanged) ---"
make_fixture 6.14.0-37-generic 6.17.0-40-generic 7.0.0-28-generic -- kernel 7.1.0-1-generic
: > "$FLAGFILE"
check "reboot-required present" 7.0.0-28-generic 0 ""
rm -f "$FLAGFILE"
check "running == newest installed (the reported bug)" 7.0.0-28-generic 1 ""
check "running older than newest" 6.14.0-37-generic 0 "7.0.0-28-generic"
echo "7.0.0-28-generic" > "$REBOOT_STAMP_FILE"
check "same, stamp already holds the target (loop guard)" 6.14.0-37-generic 1 ""
rm -f "$REBOOT_STAMP_FILE"
make_fixture
check "empty /lib/modules" 7.0.0-28-generic 1 ""
rm -rf "$MODDIR"
check "missing /lib/modules" 7.0.0-28-generic 1 ""

echo "--- Finding 1: flavour mixing ---"
make_fixture 6.12.34+rpt-rpi-v8 6.12.34+rpt-rpi-2712 6.12.25+rpt-rpi-2712 -- kernel
check "Pi 5: running rpi-2712, newer rpi-v8 present" 6.12.34+rpt-rpi-2712 1 ""
check "Pi 4: running rpi-v8, rpi-2712 also installed" 6.12.34+rpt-rpi-v8 1 ""
check "Pi 5: genuine same-flavour upgrade pending" 6.12.25+rpt-rpi-2712 0 "6.12.34+rpt-rpi-2712"
make_fixture 7.0.0-28-generic 7.0.0-28-lowlatency -- kernel
check "Ubuntu: running generic, lowlatency installed" 7.0.0-28-generic 1 ""
make_fixture 6.8.0-51-generic-64k 6.8.0-51-generic
check "flavour suffix with a dash (generic-64k)" 6.8.0-51-generic-64k 1 ""
make_fixture 6.6.51-v8+ 6.6.62-v8+
check "old RPi naming (6.6.51-v8+)" 6.6.51-v8+ 0 "6.6.62-v8+"

echo "--- Finding 2: only reboot for a strictly newer kernel ---"
make_fixture 6.14.0-37-generic
check "running kernel tree purged, only older left" 7.0.0-28-generic 1 ""
make_fixture 6.14.0-37-generic 7.0.0-28-generic
check "older and newer both present, running older" 6.14.0-37-generic 0 "7.0.0-28-generic"

echo "--- reboot mode passthrough ---"
REBOOT_MODE="always"; check "--reboot always" 7.0.0-28-generic 0 ""
REBOOT_MODE="none";   check "no reboot requested" 6.14.0-37-generic 1 ""
REBOOT_MODE="if-needed"

echo
if (( fails )); then echo "$fails FAILED, $pass passed"; exit 1; fi
echo "all $pass passed"

[dvida]

@Cybis320 can do run a quick test on your end?

[Cybis320]

Thanks Denis — merged your 5f86803 into flux-empirical-lm for live testing. Scope correction on what the fleet actually exercises: the Pi stations reboot daily after processing, so the should_reboot() fallback is moot there — the real test is the Ubuntu machines on this branch, where the fallback is the only reboot path. That covers the case you asked about: they're the NVIDIA-equipped boxes with the literal /lib/modules/kernel directory (the original bug), and after a cycle or two I'll confirm the fallback stays quiet on them.

The Pi flavour scenario (rpi-v8 vs rpi-2712 both installed) therefore stays desk-review only from my side — your harness is the coverage there.

On the flavour parse — I went through the naming schemes I could find and it holds for generic, generic-64k, raspi, rpi-2712, v8+, and degrades to the pre-flavour behaviour when uname -r doesn't match the pattern. One edge worth knowing about: Debian-packaged Pi kernels use ABI-in-name versioning like 6.1.0-rpi7-rpi-v8. There the regex captures rpi7-rpi-v8 as the flavour, so a same-flavour ABI bump (rpi7 → rpi8) produces a candidate whose parsed flavour differs and gets filtered — the reboot nudge goes quiet for that upgrade. That's the conservative failure direction (missed nudge, never a loop), and Pi OS's own +rpt naming parses fine, so I think it's acceptable as-is — flagging it so it's a known trade-off rather than a surprise if a Debian-kernel station shows up.

Will report the Ubuntu-machine results here.