diff --git a/changelog/69659.fixed.md b/changelog/69659.fixed.md new file mode 100644 index 00000000000..9b63ecbc43e --- /dev/null +++ b/changelog/69659.fixed.md @@ -0,0 +1 @@ +Fixed a minion process leak where forked per-job worker processes would hang forever in the ``resolve_dns`` retry loop when the master hostname was unresolvable, accumulating over time and exhausting memory. Workers now fail fast on an unresolvable master (the ``retry_dns``/``retry_dns_count`` infinite-retry behavior of the main minion process is unchanged and still bounded opt-in via ``retry_dns_count``). diff --git a/doc/ref/configuration/minion.rst b/doc/ref/configuration/minion.rst index 17298548815..c690855460b 100644 --- a/doc/ref/configuration/minion.rst +++ b/doc/ref/configuration/minion.rst @@ -383,6 +383,12 @@ Set the number of attempts to perform when resolving the master hostname if name resolution fails. By default the minion will retry indefinitely. +This bounds the retry loop of the main minion process. Forked per-job worker +processes always fail fast on an unresolvable master (as if ``retry_dns_count`` +were ``0``) regardless of this setting, so a bad or removed master name cannot +cause hung worker processes to accumulate. An explicit ``retry_dns_count`` is +still honored inside workers. + .. code-block:: yaml retry_dns_count: 3 diff --git a/salt/minion.py b/salt/minion.py index 229ec76956d..b7cccfc99fb 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -114,6 +114,11 @@ # signal handler sets this so that a SIGTERM arriving while the minion is # stuck retrying master DNS resolution can shut the io_loop down promptly # instead of waiting for ``retry_dns`` seconds * forever. See #69466. +# +# NOTE: this is a main-process mechanism only. A forked per-job worker holds +# its own dead copy of this event that the parent can never set, so workers +# must not rely on it to escape the retry loop -- they bound resolve_dns via +# retry_dns_count instead (see Minion._target). _RESOLVE_DNS_ABORT = threading.Event() @@ -2582,6 +2587,14 @@ def ctx(self): def _target(cls, minion_instance, opts, data, connected, creds_map): if creds_map: salt.crypt.AsyncAuth.creds_map = creds_map + # A forked per-job worker is short-lived and must never sit in the + # unbounded resolve_dns() retry loop when the master is unresolvable + # (the parent's _RESOLVE_DNS_ABORT event does not cross the fork + # boundary, so a stuck worker cannot be aborted short of SIGKILL and + # would leak). Fail fast so the worker raises, exits, and is reaped + # normally. An explicit operator-set retry_dns_count is honored. + if opts.get("retry_dns_count") is None: + opts["retry_dns_count"] = 0 if not minion_instance: minion_instance = cls(opts, load_grains=False) minion_instance.connected = connected diff --git a/tests/pytests/unit/test_minion.py b/tests/pytests/unit/test_minion.py index 70831a9d5f4..738f239b82b 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -1037,6 +1037,73 @@ def trip_abort(): ) +def test_resolve_dns_count_zero_fails_fast(minion_opts): + """ + ``retry_dns_count: 0`` must make resolve_dns raise immediately on an + unresolvable master instead of entering the ``retry_dns`` sleep loop. + This is what lets forked workers fail fast rather than leak. + """ + minion_opts.update( + { + "ipv6": False, + "master": "dummy", + "master_port": "4555", + # A large interval so that if the retry loop is (wrongly) entered + # the test would visibly stall instead of passing by luck. + "retry_dns": 90, + "retry_dns_count": 0, + }, + ) + + dns_check = MagicMock(side_effect=SaltClientError("unresolvable")) + started = time.monotonic() + with patch("salt.utils.network.dns_check", dns_check): + with pytest.raises(SaltMasterUnresolvableError): + salt.minion.resolve_dns(minion_opts) + elapsed = time.monotonic() - started + + # With retry_dns_count == 0 the loop raises on its first iteration before + # any retry sleep, so the initial lookup is the only dns_check call and + # the call returns effectively immediately. + assert dns_check.call_count == 1 + assert elapsed < 5, ( + f"resolve_dns slept before failing with retry_dns_count=0 " + f"(elapsed={elapsed:.2f}s); it should fail fast." + ) + + +def test_target_bounds_worker_dns_retry(minion_opts): + """ + A forked per-job worker must not inherit an unbounded DNS retry loop. + Minion._target should force ``retry_dns_count`` to 0 when the operator + has not set it, and leave an explicit value untouched. + """ + data = {"jid": "20240101000000000000", "fun": "test.ping", "arg": []} + + # Stub out the actual job execution and the tornado StackContext wrapping + # so the test exercises only the opts mutation in _target's preamble, + # without requiring a fully initialized minion (functions/ctx). + def fake_stack_context(*args, **kwargs): + return contextlib.nullcontext() + + with patch.object(salt.minion.Minion, "_thread_return", MagicMock()), patch( + "salt.ext.tornado.stack_context.StackContext", fake_stack_context + ): + # Operator did not set retry_dns_count -> worker fails fast. + unset_opts = copy.deepcopy(minion_opts) + unset_opts["retry_dns_count"] = None + minion = salt.minion.Minion(unset_opts, load_grains=False) + salt.minion.Minion._target(minion, unset_opts, data, True, None) + assert unset_opts["retry_dns_count"] == 0 + + # Operator set an explicit bound -> _target leaves it alone. + explicit_opts = copy.deepcopy(minion_opts) + explicit_opts["retry_dns_count"] = 5 + minion = salt.minion.Minion(explicit_opts, load_grains=False) + salt.minion.Minion._target(minion, explicit_opts, data, True, None) + assert explicit_opts["retry_dns_count"] == 5 + + def test_minion_manager_stop_unblocks_resolve_dns_69466(minion_opts): """ Regression test for #69466.