From 8a3f5d0f819ebd2b2059aa71a446e0af9335a31a Mon Sep 17 00:00:00 2001 From: Josh Patterson Date: Thu, 9 Jul 2026 16:55:30 -0400 Subject: [PATCH] Gate so-salt-minion-wait on real minion readiness The previous gate did not detect whether the restarted minion was back: systemctl is-active --quiet salt-minion \ && salt-call --local --timeout=5 --out=quiet test.ping Both halves are near-vacuous. `--local` sets file_client=local, so test.ping runs in a throwaway minion that never contacts the master and never inspects the running daemon; it only proves python and the module loader work. And the shipped unit is Type=notify with notify_systemd() called before the daemon imports salt.cli.daemons, so is-active goes true at process launch, not at connection. The script could return ready while the minion was still authenticating, which is the race it exists to prevent. Gate instead on the condition salt itself uses to log "Minion is ready to receive requests!", requiring both signals of the current daemon instance: 1. the pid-tagged ready line in the minion log. tune_in() emits it only after sync_connect_master() returns, i.e. the pub channel authenticated, the req channel connected, and _post_master_init() finished loading modules and compiling pillar. 2. that same pid holding an ESTABLISHED req connection to a master on 4506 plus a second (publish) connection to the same master IP. The publish port is absent from minion config -- the minion learns it from the master's auth reply -- so it is derived from the connection. Resolve the daemon pid from systemd (MainPID -> pgrep -P), never from /var/run/salt-minion.pid. salt_minion() runs the minion in a multiprocessing child; that child writes the pidfile, owns the sockets and logs the ready line, while MainPID is the parent. During a restart the pidfile still names the old child, whose own ready line is already in the log, so keying off it reports ready instantly. Children of the current MainPID exclude the old instance structurally, with no timing assumptions. Degrade deterministically rather than spinning to the timeout: if log_level_logfile does not emit INFO records the ready line can never appear, so detect that up front from the merged config and fall back to the socket check. log_level_logfile defaults to None (inherit log_level), so resolve the inheritance before deciding. If ss is unavailable, fall back to the log gate. If neither signal is usable, fail immediately with a clear message. Requiring master connectivity adds no new dependency: every path that applies salt.minion or a highstate does so without --local, so file_client=remote already required a reachable master to fetch salt:// files. No salt-call master round-trip is added; the daemon's own successful auth already proves the key is accepted. Also fix the comment above wait_for_salt_minion_ready, which attributed the script to common_sbin/common/tools/sbin (it is deployed by salt_sbin from salt/tools/sbin) and asserted a --no-block restart that appears nowhere in the repo. No state logic changed. --- salt/salt/minion/init.sls | 13 ++- salt/salt/tools/sbin/so-salt-minion-wait | 142 +++++++++++++++++++++-- 2 files changed, 138 insertions(+), 17 deletions(-) diff --git a/salt/salt/minion/init.sls b/salt/salt/minion/init.sls index a251aa633..035d9e7f8 100644 --- a/salt/salt/minion/init.sls +++ b/salt/salt/minion/init.sls @@ -131,11 +131,14 @@ salt_minion_service: {% endif %} - order: last -# block until the just-restarted salt-minion is back and can execute modules locally, so -# follow-on jobs and the next highstate iteration do not race the restart. onchanges + -# require on salt_minion_service catches every restart trigger uniformly because watch -# mod_watch results replace the service state's running entry. wait logic lives in -# /usr/sbin/so-salt-minion-wait (deployed by common_sbin from common/tools/sbin/). +# block until the just-restarted salt-minion daemon logs "Minion is ready to receive requests!" +# for the current instance, so follow-on jobs and the next highstate iteration do not race the +# restart. onchanges + require on salt_minion_service catches every restart trigger uniformly +# because watch mod_watch results replace the service state's running entry. wait logic lives in +# /usr/sbin/so-salt-minion-wait (deployed by salt_sbin from salt/tools/sbin/); it keys the ready +# line to the current daemon pid (resolved via systemd, not the pidfile) and corroborates with the +# master req/publish sockets. set_log_levels above enforces the log_level_logfile: info that the +# ready line depends on. wait_for_salt_minion_ready: cmd.run: - name: /usr/sbin/so-salt-minion-wait diff --git a/salt/salt/tools/sbin/so-salt-minion-wait b/salt/salt/tools/sbin/so-salt-minion-wait index a30c67e80..9fb2a41c7 100644 --- a/salt/salt/tools/sbin/so-salt-minion-wait +++ b/salt/salt/tools/sbin/so-salt-minion-wait @@ -5,31 +5,149 @@ # https://securityonion.net/license; you may not use this file except in compliance with the # Elastic License 2.0. -# Block until the local salt-minion service is back up and can execute modules locally. -# Invoked from the wait_for_salt_minion_ready state in salt/minion/init.sls after -# salt_minion_service fires its watch-driven mod_watch (a non-blocking systemctl restart), -# so follow-on jobs and the next highstate iteration do not race the in-flight restart. +# Block until the just-restarted salt-minion daemon reaches the point where salt itself logs +# "Minion is ready to receive requests!". Invoked from the wait_for_salt_minion_ready state in +# salt/minion/init.sls after salt_minion_service fires its watch-driven restart, so follow-on jobs +# and the next highstate iteration do not race it. +# +# Salt logs that line from Minion.tune_in() only after sync_connect_master() returns, which means +# the pub channel authenticated, the long-running req channel connected, and _post_master_init() +# finished loading modules and compiling pillar. Two signals reproduce that: +# +# 1. Primary the pid-tagged ready line in the minion log. Salt's log_fmt_logfile embeds +# [%(process)d] just before the message, so this is keyed to one daemon instance. +# 2. Corroborating that same pid holds an ESTABLISHED req connection to a master on 4506 plus a +# second (publish) connection to that same master IP on another port. The publish +# port is learned from the master's auth reply and is absent from minion config, +# so it is derived from the connection rather than read from config. +# +# The daemon pid is resolved from systemd, never from /var/run/salt-minion.pid. salt_minion() runs +# the real minion in a multiprocessing child; that child writes the pidfile, owns the sockets and +# logs the ready line, while systemd's MainPID is the parent. During a restart the pidfile can still +# name the OLD child, whose own ready line is already in the log -- matching it would report ready +# instantly. Children of the current MainPID structurally exclude the old instance. . /usr/sbin/so-common -# Initial sleep gives the systemctl restart (--no-block by default for salt-minion on -# >=3006.15) time to begin tearing down the old process before we probe for readiness. +set -u + INITIAL_SLEEP=3 TIMEOUT=120 -PING_TIMEOUT=5 +MASTER_PORT=4506 +LOG_TAIL_LINES=10000 +DEFAULT_LOG_FILE="/opt/so/log/salt/minion" +LOG_FILE="$DEFAULT_LOG_FILE" + +# Decide whether the ready line can ever appear. salt-call --local sets file_client=local, so this +# reads the merged config (honoring minion.d overrides) without contacting the master. salt defaults +# log_level_logfile to None, meaning it inherits log_level, so resolve that before deciding. +LOG_LEVEL_LOGFILE=$(salt-call --local --out=newline_values_only config.get log_level_logfile 2>/dev/null | head -n1) +case "${LOG_LEVEL_LOGFILE,,}" in + ""|none) LOG_LEVEL_LOGFILE=$(salt-call --local --out=newline_values_only config.get log_level 2>/dev/null | head -n1) ;; +esac + +case "${LOG_LEVEL_LOGFILE,,}" in + all|garbage|trace|debug|profile|info) USE_LOG_GATE=1 ;; + *) USE_LOG_GATE=0 ;; +esac + +if [ "$USE_LOG_GATE" -eq 1 ]; then + LOG_FILE=$(salt-call --local --out=newline_values_only config.get log_file 2>/dev/null | head -n1) + [ -z "$LOG_FILE" ] && LOG_FILE="$DEFAULT_LOG_FILE" + [ -d "$(dirname "$LOG_FILE")" ] || USE_LOG_GATE=0 +fi + +if command -v ss >/dev/null 2>&1; then + USE_SOCKET_GATE=1 +else + USE_SOCKET_GATE=0 +fi + +if [ "$USE_LOG_GATE" -eq 0 ] && [ "$USE_SOCKET_GATE" -eq 0 ]; then + echo "so-salt-minion-wait: no usable readiness signal (log_level_logfile='${LOG_LEVEL_LOGFILE:-unset}', ss not found)" >&2 + exit 1 +fi + +if [ "$USE_LOG_GATE" -eq 1 ] && [ "$USE_SOCKET_GATE" -eq 1 ]; then + echo "so-salt-minion-wait: gating on pid-tagged ready line in ${LOG_FILE} plus master sockets" +elif [ "$USE_LOG_GATE" -eq 1 ]; then + echo "so-salt-minion-wait: ss not found; gating on pid-tagged ready line in ${LOG_FILE} only" +else + echo "so-salt-minion-wait: INFO file logging unavailable (log_level_logfile='${LOG_LEVEL_LOGFILE:-unset}'); gating on master sockets only" +fi + +# Emit the pid(s) of the current daemon instance. systemd's MainPID is the parent keepalive process; +# its child runs tune_in. Fall back to MainPID when there is no child (--disable-keepalive path). +resolve_daemon_pids() { + local mainpid children + mainpid=$(systemctl show -p MainPID --value salt-minion 2>/dev/null) + if [ -z "$mainpid" ] || [ "$mainpid" = "0" ]; then + return 1 + fi + children=$(pgrep -P "$mainpid" 2>/dev/null) + printf '%s\n' "${children:-$mainpid}" +} + +# True iff the ready line tagged with this pid is in the current or most recently rotated log. +ready_logged() { + local pid=$1 f + for f in "$LOG_FILE" "$LOG_FILE.1"; do + [ -r "$f" ] || continue + if tail -n "$LOG_TAIL_LINES" "$f" 2>/dev/null | grep -Fq "[$pid] Minion is ready to receive requests!"; then + return 0 + fi + done + return 1 +} + +# True iff this pid holds an ESTABLISHED req connection to a master on MASTER_PORT and a second +# ESTABLISHED connection to that same master IP on another port. The trailing comma in "pid=N," +# keeps pid=123 from matching pid=1234. Grid comms are IPv4 (the unit's ExecStartPre gates on ip -4). +socket_ready() { + local pid=$1 mip master_ips + master_ips=$(ss -tnp state established "dport = :${MASTER_PORT}" 2>/dev/null \ + | grep -F "pid=${pid}," \ + | grep -oE "[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+:${MASTER_PORT}" \ + | sed "s/:${MASTER_PORT}\$//" \ + | sort -u) + [ -z "$master_ips" ] && return 1 + for mip in $master_ips; do + if ss -tnp state established "dst ${mip} and dport != :${MASTER_PORT}" 2>/dev/null | grep -qF "pid=${pid},"; then + return 0 + fi + done + return 1 +} + +instance_ready() { + local pid=$1 + if [ "$USE_LOG_GATE" -eq 1 ] && ! ready_logged "$pid"; then + return 1 + fi + if [ "$USE_SOCKET_GATE" -eq 1 ] && ! socket_ready "$pid"; then + return 1 + fi + return 0 +} sleep "$INITIAL_SLEEP" elapsed="$INITIAL_SLEEP" +pids="" while [ "$elapsed" -lt "$TIMEOUT" ]; do - if systemctl is-active --quiet salt-minion \ - && salt-call --local --timeout="$PING_TIMEOUT" --out=quiet test.ping >/dev/null 2>&1; then - echo "salt-minion ready after ${elapsed}s" - exit 0 + if pids=$(resolve_daemon_pids); then + # shellcheck disable=SC2086 + for pid in $pids; do + if instance_ready "$pid"; then + echo "salt-minion (pid ${pid}) ready after ${elapsed}s" + exit 0 + fi + done fi sleep 1 elapsed=$((elapsed + 1)) done -echo "salt-minion did not become ready within ${TIMEOUT}s" >&2 +mainpid=$(systemctl show -p MainPID --value salt-minion 2>/dev/null) +echo "salt-minion did not become ready within ${TIMEOUT}s (MainPID=${mainpid:-unknown}, candidate pids='${pids:-none}', log_gate=${USE_LOG_GATE}, socket_gate=${USE_SOCKET_GATE}, log_file=${LOG_FILE})" >&2 exit 1