diff --git a/bin/smart-restart.sh b/bin/smart-restart.sh index 9e35962..a7712dc 100755 --- a/bin/smart-restart.sh +++ b/bin/smart-restart.sh @@ -33,10 +33,25 @@ if [[ -z "${NEEDS_RESTARTING_COMMAND:-}" ]] && [[ "$(readlink -f /usr/bin/dnf 2> NEEDS_RESTARTING_COMMAND="dnf5 needs-restarting --skip-file-locks" fi NEEDS_RESTARTING_COMMAND="${NEEDS_RESTARTING_COMMAND:-/usr/bin/needs-restarting}" +RPM_COMMAND="${RPM_COMMAND:-rpm}" IS_TESTING=${IS_TESTING:-} DEBUG=${DEBUG:-} +# The guards below (live-process filter, reboot-class withholding) compensate +# for dnf5's needs-restarting regressions. dnf4's and yum's needs-restarting +# derive services from running processes and withhold reboot-class services +# themselves, so their output never needs the guards - keep the legacy (AL2, +# AL2023) code paths unchanged and enable the guards only on dnf5 stacks. +# Override with ENFORCE_RESTART_SAFETY_GUARDS=1/0. +if [[ -z "${ENFORCE_RESTART_SAFETY_GUARDS:-}" ]]; then + if [[ "$NEEDS_RESTARTING_COMMAND" == *dnf5* ]]; then + ENFORCE_RESTART_SAFETY_GUARDS=1 + else + ENFORCE_RESTART_SAFETY_GUARDS=0 + fi +fi + SERVICES=() BLOCKED_SERVICES=() PRE_RESTART_HEALTHY=0 @@ -54,6 +69,69 @@ assert_root() { return 0 } +# dnf5's `needs-restarting -s` reports any unit with ActiveState "active", +# which includes completed oneshot units in "active (exited)" state, e.g. +# cloud-config.service after boot. Those have no processes that could be +# running outdated code, and "restarting" them re-executes their one-time +# (typically boot-time) action instead of reloading anything; re-running +# cloud-config.service deadlocks under cloud-init >= 25.3's single-process +# architecture. dnf4's needs-restarting never reported process-less units. +# Keep only services with live processes (SubState "running"). This guard +# can be dropped once the dnf5-side fix is merged and deployed: +# https://github.com/rpm-software-management/dnf5 (needs-restarting +# SubState filter). +# Note: "--property=X" without "--value" plus prefix stripping, because +# "--value" requires systemd >= 230 and AL2 ships 219. +service_has_live_processes() { + local sub_state + sub_state=$($SYSCTL_COMMAND show --property=SubState "$1" 2>/dev/null) + [[ "${sub_state#SubState=}" == "running" ]] +} + +# dnf4's needs-restarting refuses to recommend restarting services defined +# by reboot-class packages: units whose unit file belongs to an installed +# package providing one of the NEED_REBOOT names are withheld from the -s +# output and printed to stderr as requiring a reboot instead. dnf5 dropped +# that guard, so it happily lists e.g. dbus-broker.service after a dbus +# update - and blindly restarting that mid-transaction can sever the +# session driving the update. Enforce the same rule here: resolve which +# installed packages provide the reboot-class names (same provides-based +# matching as dnf4), and withhold services whose unit file they own. +# Only the rpmdb is read (no dnf metadata, no locks), which is safe from +# inside the transaction hook. +REBOOT_CLASS_PACKAGES="" +assemble_reboot_class_packages() { + # dnf4's NEED_REBOOT list plus dnf5's CORE_PACKAGE_NAMES additions + # (https://access.redhat.com/solutions/27943) + local reboot_class_provides=(kernel kernel-core kernel-PAE kernel-rt kernel-smp kernel-xen + linux-firmware microcode_ctl dbus dbus-broker dbus-daemon + glibc hal systemd udev gnutls openssl-libs) + local name pkgs + for name in "${reboot_class_provides[@]}"; do + pkgs=$($RPM_COMMAND -q --whatprovides --queryformat '%{NAME}\n' "$name" 2>/dev/null) || continue + REBOOT_CLASS_PACKAGES+="${pkgs}"$'\n' + done + REBOOT_CLASS_PACKAGES=$(sort -u <<<"$REBOOT_CLASS_PACKAGES") +} + +service_requires_reboot() { + [[ -n "$REBOOT_CLASS_PACKAGES" ]] || assemble_reboot_class_packages + + local fragment_path owner + fragment_path=$($SYSCTL_COMMAND show --property=FragmentPath "$1" 2>/dev/null) + fragment_path="${fragment_path#FragmentPath=}" + [[ -n "$fragment_path" ]] || return 1 + owner=$($RPM_COMMAND -qf --queryformat '%{NAME}\n' "$fragment_path" 2>/dev/null) || return 1 + grep -qFx "$owner" <<<"$REBOOT_CLASS_PACKAGES" +} + +# Combined guard used wherever needs-restarting -s output is consumed. +# On dnf4/yum stacks this always passes, keeping legacy behavior identical. +service_is_restart_candidate() { + [[ "$ENFORCE_RESTART_SAFETY_GUARDS" == 1 ]] || return 0 + service_has_live_processes "$1" && ! service_requires_reboot "$1" +} + assemble_service_list() { # shellcheck disable=SC2207 local all_services=($($NEEDS_RESTARTING_COMMAND -s | xargs)) @@ -65,15 +143,29 @@ assemble_service_list() { DBG "Blocked services: ${BLOCKED_SERVICES[*]}" DBG "All services: ${all_services[*]}" + local reboot_required_services=() for SERVICE in "${all_services[@]}"; do grep -qF "${SERVICE}" <<<"${BLOCKED_SERVICES[*]}" if [[ $? -eq 0 ]]; then DBG "Ignoring ${SERVICE}" + elif [[ "$ENFORCE_RESTART_SAFETY_GUARDS" == 1 ]] && ! service_has_live_processes "${SERVICE}"; then + DBG "Ignoring ${SERVICE} (no running processes)" + elif [[ "$ENFORCE_RESTART_SAFETY_GUARDS" == 1 ]] && service_requires_reboot "${SERVICE}"; then + DBG "Ignoring ${SERVICE} (owned by a reboot-class package)" + reboot_required_services+=("${SERVICE}") else SERVICES+=("${SERVICE}") DBG "Adding ${SERVICE}" fi done + + # Same warning dnf4's needs-restarting prints when it withholds these + if [[ ${#reboot_required_services[@]} != 0 ]]; then + >&2 echo "Warning: The following services should not be restarted but require a reboot:" + for SERVICE in "${reboot_required_services[@]}"; do + >&2 echo " ${SERVICE}" + done + fi } execute_pre_hooks() { @@ -165,7 +257,7 @@ generate_reboot_hint_marker() { local post_restart_services=$($NEEDS_RESTARTING_COMMAND -s | xargs) local failed_services=() for SERVICE in $post_restart_services; do - if ! grep -qF "$SERVICE" <<<"${BLOCKED_SERVICES[*]}"; then + if ! grep -qF "$SERVICE" <<<"${BLOCKED_SERVICES[*]}" && service_is_restart_candidate "$SERVICE"; then DBG "$SERVICE not denylisted. Service restart required" failed_services+=("${SERVICE}") reboot_hint=1 diff --git a/tests/mocks/needs-restarting b/tests/mocks/needs-restarting index 4a6b874..3c59904 100755 --- a/tests/mocks/needs-restarting +++ b/tests/mocks/needs-restarting @@ -14,7 +14,17 @@ fi if [[ "$1" == "-s" ]]; then - if [[ $NEED_RESTART_2 == 1 ]]; then + if [[ $NEED_RESTART_2_ONESHOT == 1 ]]; then + echo "dummy.service" + echo "dummy2.service" + echo "oneshot.service" + elif [[ $NEED_RESTART_2_DBUS == 1 ]]; then + echo "dummy.service" + echo "dummy2.service" + echo "dbus-broker.service" + elif [[ $NEED_RESTART_DBUS == 1 ]]; then + echo "dbus-broker.service" + elif [[ $NEED_RESTART_2 == 1 ]]; then echo "dummy.service" echo "dummy2.service" elif [[ $NEED_RESTART_1 == 1 ]]; then diff --git a/tests/mocks/rpm b/tests/mocks/rpm new file mode 100755 index 0000000..56b7abc --- /dev/null +++ b/tests/mocks/rpm @@ -0,0 +1,47 @@ +#!/usr/bin/env bash + +# Mock for the rpm invocations smart-restart makes: +# rpm -q --whatprovides --queryformat '%{NAME}\n' +# rpm -qf --queryformat '%{NAME}\n' +# Models a system where dbus-broker provides the dbus names, glibc and +# systemd exist, and every other reboot-class name has no provider. +# Unit files under a path containing "dbus-broker" belong to dbus-broker; +# everything else belongs to dummy-pkg. + +DBG() { [[ "$DEBUG" != "" ]] && >&2 echo "$@"; } + +DBG "rpm-mock invoked with: ${*}" + +NAME="${*: -1}" + +if [[ "$*" == *"--whatprovides"* ]]; then + case "$NAME" in + dbus|dbus-broker|dbus-daemon) + echo "dbus-broker" + ;; + glibc) + echo "glibc" + ;; + systemd) + echo "systemd" + ;; + *) + echo "no package provides $NAME" + exit 1 + ;; + esac + exit 0 +elif [[ "$1" == "-qf" ]]; then + case "$NAME" in + *dbus-broker*) + echo "dbus-broker" + ;; + *) + echo "dummy-pkg" + ;; + esac + exit 0 +fi + +DBG "rpm-mock called incorrectly: (No params: $# -> $*)" +exit 98 diff --git a/tests/mocks/systemctl b/tests/mocks/systemctl index 86402ec..9bc4a07 100755 --- a/tests/mocks/systemctl +++ b/tests/mocks/systemctl @@ -54,7 +54,9 @@ if [[ "$1" == "daemon-reexec" ]]; then DBG "systemd daemon-reexec called incorrectly: (No params: $# -> $@)" exit 1 elif [[ "$1" == "restart" ]]; then - if [[ $SYS_EXPECT_2 == 1 ]]; then + if [[ $SYS_EXPECT_3 == 1 ]]; then + [[ $# == 4 ]] && [[ "$2" == "dummy.service" ]] && [[ "$3" == "dummy2.service" ]] && [[ "$4" == "oneshot.service" ]] && exit 0 + elif [[ $SYS_EXPECT_2 == 1 ]]; then [[ $# == 3 ]] && [[ "$2" == "dummy.service" ]] && [[ "$3" == "dummy2.service" ]] && exit 0 elif [[ $SYS_EXPECT_1 == 1 ]]; then [[ $# == 2 ]] && [[ "$2" == "dummy2.service" ]] && exit 0 @@ -63,6 +65,25 @@ elif [[ "$1" == "restart" ]]; then else exit 98 fi +elif [[ "$1" == "show" ]]; then + # smart-restart queries (no --value; AL2's systemd 219 lacks it): + # systemctl show --property=SubState -> SubState=... + # systemctl show --property=FragmentPath -> FragmentPath=... + # Units listed in SYS_ONESHOT_UNITS (space separated) report "exited", + # like a completed Type=oneshot service; everything else is "running". + UNIT="${*: -1}" + if [[ "$*" == *"--property=FragmentPath"* ]]; then + echo "FragmentPath=/usr/lib/systemd/system/${UNIT}" + exit 0 + fi + for u in $SYS_ONESHOT_UNITS; do + if [[ "$u" == "$UNIT" ]]; then + echo "SubState=exited" + exit 0 + fi + done + echo "SubState=running" + exit 0 elif [[ "$1" == "status" ]]; then if [[ $SYS_RESTART_FAILED == "1" ]]; then echo "$SERVICE_STATUS_FAILED" diff --git a/tests/setup_test b/tests/setup_test index b2258ba..1eb1fb0 100644 --- a/tests/setup_test +++ b/tests/setup_test @@ -20,6 +20,10 @@ UUT=../bin/smart-restart.sh export CONF_PATH="$(pwd)"/conf export NEEDS_RESTARTING_COMMAND="$(pwd)"/mocks/needs-restarting export SYSCTL_COMMAND="$(pwd)"/mocks/systemctl +export RPM_COMMAND="$(pwd)"/mocks/rpm +# The mock needs-restarting is not a dnf5 subcommand, so auto-detection +# would disable the dnf5 guards; enable them explicitly for the tests. +export ENFORCE_RESTART_SAFETY_GUARDS=1 export IS_TESTING="1" export REBOOT_HINT_PATH="$(pwd)" @@ -34,6 +38,7 @@ echo -e "=========================== ${YELLOW}$TEST_NAME${COLOR_OFF} started "== reset_test_environment() { SERVICES=() BLOCKED_SERVICES=() + REBOOT_CLASS_PACKAGES="" PRE_RESTART_HEALTHY="0" POST_RESTART_HEALTHY="0" echo "" > conf/default-denylist diff --git a/tests/test-reboot-hint.sh b/tests/test-reboot-hint.sh index 796f2a4..5734b99 100755 --- a/tests/test-reboot-hint.sh +++ b/tests/test-reboot-hint.sh @@ -162,9 +162,59 @@ function reboot_hint_on_unhealthy_service() { } +function no_reboot_hint_on_stale_oneshot() { + DESCRIPTION="No reboot hint when only a completed oneshot service remains stale" + reset_test_environment + local -i retval=0 + + NEED_RESTART_2=1 assemble_service_list + SYS_RESTART_FAILED=0 count_pre_restart_health + + # After the restarts, needs-restarting still reports oneshot.service + # ("active (exited)", never restarted by us). That must not be treated + # as a failed restart requiring a reboot. + OS_VERSION="2023" SYS_RESTART_FAILED=0 NEED_RESTART_1=1 SYS_ONESHOT_UNITS="dummy2.service" \ + generate_reboot_hint_marker || retval=$? + + if [[ $retval -eq 0 && ! -f "$(pwd)/reboot-hint-marker" ]]; then + PASSED "$DESCRIPTION" + else + FAILED "$DESCRIPTION (error: $retval)" + fi + rm -rf "$(pwd)/reboot-hint-marker" +} + + +function no_reboot_hint_on_stale_reboot_class_service() { + DESCRIPTION="No reboot hint when only a withheld reboot-class service remains stale" + reset_test_environment + local -i retval=0 + + NEED_RESTART_2=1 assemble_service_list + SYS_RESTART_FAILED=0 count_pre_restart_health + + # After the restarts, needs-restarting still reports dbus-broker.service + # (withheld from restarting on purpose). AL2023's dnf4 hid it from the -s + # output entirely, so it never raised the hint there; same result here. + # The reboot recommendation for such packages is handled by the + # --reboothint path, which deliberately ignores them. + OS_VERSION="2023" SYS_RESTART_FAILED=0 NEED_RESTART_DBUS=1 \ + generate_reboot_hint_marker || retval=$? + + if [[ $retval -eq 0 && ! -f "$(pwd)/reboot-hint-marker" ]]; then + PASSED "$DESCRIPTION" + else + FAILED "$DESCRIPTION (error: $retval)" + fi + rm -rf "$(pwd)/reboot-hint-marker" +} + + restart_successful restart_failed kernel_update userspace_update userspace_and_kernel_update reboot_hint_on_unhealthy_service +no_reboot_hint_on_stale_oneshot +no_reboot_hint_on_stale_reboot_class_service diff --git a/tests/test-restart.sh b/tests/test-restart.sh index e43e28c..02a59e1 100755 --- a/tests/test-restart.sh +++ b/tests/test-restart.sh @@ -153,6 +153,94 @@ function denylist_systemd_failed() { +function completed_oneshot_not_restarted() { + DESCRIPTION="Completed oneshot service is filtered from restart list" + reset_test_environment + local -i retval=0 + + # needs-restarting reports dummy.service, dummy2.service and + # oneshot.service; the latter is "active (exited)" (SubState "exited") + # and must not be restarted. + NEED_RESTART_2_ONESHOT="1" SYS_ONESHOT_UNITS="oneshot.service" assemble_service_list + count_pre_restart_health + # SYS_EXPECT_2 makes the systemctl mock only accept a restart of exactly + # dummy.service dummy2.service, proving oneshot.service was dropped. + SYS_EXPECT_2="1" SYS_RESTART_FAILED="0" restart_services || retval=$? + + if [[ $retval == 0 ]]; then + PASSED "$DESCRIPTION" + else + FAILED "$DESCRIPTION (error: $retval)" + fi + PRE_RESTART_HEALTHY="0" + POST_RESTART_HEALTHY="0" +} + + + +function reboot_class_service_not_restarted() { + DESCRIPTION="Reboot-class service is withheld from restart list" + reset_test_environment + local -i retval=0 + + # needs-restarting reports dummy.service, dummy2.service and + # dbus-broker.service; the rpm mock owns the latter's unit file via + # dbus-broker, which provides a reboot-class name, so it must be + # withheld (dnf4 behavior). + NEED_RESTART_2_DBUS="1" assemble_service_list 2>/dev/null + count_pre_restart_health + # SYS_EXPECT_2 makes the systemctl mock only accept a restart of exactly + # dummy.service dummy2.service, proving dbus-broker.service was dropped. + SYS_EXPECT_2="1" SYS_RESTART_FAILED="0" restart_services || retval=$? + + if [[ $retval == 0 ]]; then + PASSED "$DESCRIPTION" + else + FAILED "$DESCRIPTION (error: $retval)" + fi + PRE_RESTART_HEALTHY="0" + POST_RESTART_HEALTHY="0" +} + +function reboot_class_service_warns() { + DESCRIPTION="Withheld reboot-class service prints dnf4-style warning to stderr" + reset_test_environment + local warning + + warning=$(NEED_RESTART_2_DBUS="1" assemble_service_list 2>&1 >/dev/null) + + if grep -q "should not be restarted but require a reboot" <<<"$warning" \ + && grep -q "dbus-broker.service" <<<"$warning"; then + PASSED "$DESCRIPTION" + else + FAILED "$DESCRIPTION (stderr: $warning)" + fi +} + +function legacy_stack_keeps_dnf4_behavior() { + DESCRIPTION="Guards disabled (dnf4/yum stack): list is restarted verbatim" + reset_test_environment + local -i retval=0 + + # On AL2/AL2023 the guards are off (dnf4 already applies them itself); + # everything needs-restarting printed must be restarted unchanged, even + # units our mocks would classify as oneshot. + ENFORCE_RESTART_SAFETY_GUARDS=0 NEED_RESTART_2_ONESHOT="1" SYS_ONESHOT_UNITS="oneshot.service" \ + assemble_service_list + count_pre_restart_health + SYS_EXPECT_3="1" SYS_RESTART_FAILED="0" restart_services || retval=$? + + if [[ $retval == 0 ]]; then + PASSED "$DESCRIPTION" + else + FAILED "$DESCRIPTION (error: $retval)" + fi + PRE_RESTART_HEALTHY="0" + POST_RESTART_HEALTHY="0" +} + + + restart_successful restart_failed all_services_denylisted @@ -161,3 +249,7 @@ denylist_systemd denylist_systemd_failed health_check_pass health_check_fail +completed_oneshot_not_restarted +reboot_class_service_not_restarted +reboot_class_service_warns +legacy_stack_keeps_dnf4_behavior