From 6eaad63faeaf18658d5d1173a5c97c93f51144db Mon Sep 17 00:00:00 2001 From: Twangboy Date: Thu, 2 Jul 2026 10:53:37 -0600 Subject: [PATCH 1/2] Fail fast on unresolvable master DNS in forked minion workers A forked per-job worker inherits the minion's default retry_dns=30 / retry_dns_count=None settings. If the job it runs resolves the master DNS (e.g. status.ping_master or master-alive checks) while the master hostname is unresolvable, resolve_dns() enters its unbounded while-True retry loop and the worker never returns. The loop's abort hook (_RESOLVE_DNS_ABORT, #69466) cannot rescue it: the event is a module-level threading.Event and a forked child holds its own dead copy that the parent's SIGTERM handler can never set. The wedged worker is therefore only killable via SIGKILL, and since SubprocessList cleanup only reaps already-exited processes, hung workers accumulate. A minion pointed at an unresolvable master leaked hundreds of workers and ~10GB of RAM over a couple of days. Bound DNS retries in the worker preamble (Minion._target): when the operator has not set retry_dns_count, force it to 0 so resolve_dns raises SaltMasterUnresolvableError on the first attempt, the worker exits, and it is reaped normally. An explicit retry_dns_count is still honored. This only shortcuts DNS-resolution failure, never job execution time, so it cannot kill a legitimate long-running job. The main minion process is unchanged (still retries by default; bounded opt-in via retry_dns_count), mirroring the existing failover path that already forces retry_dns=0. --- changelog/69659.fixed.md | 1 + doc/ref/configuration/minion.rst | 6 +++ salt/minion.py | 13 ++++++ tests/pytests/unit/test_minion.py | 69 +++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+) create mode 100644 changelog/69659.fixed.md diff --git a/changelog/69659.fixed.md b/changelog/69659.fixed.md new file mode 100644 index 000000000000..9b63ecbc43e6 --- /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 17298548815c..c690855460be 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 229ec76956d4..b7cccfc99fb8 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 70831a9d5f40..089c6096e696 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -1037,6 +1037,75 @@ 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. From cadcfcb325986e3cb7b7d7ff5b31e1fc6b83ef90 Mon Sep 17 00:00:00 2001 From: Twangboy Date: Thu, 2 Jul 2026 11:10:54 -0600 Subject: [PATCH 2/2] Apply black formatting to test_minion.py pre-commit's black hook collapses the patch.object/patch context manager in test_target_bounds_worker_dns_retry onto black's preferred line layout. No functional change. --- tests/pytests/unit/test_minion.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/pytests/unit/test_minion.py b/tests/pytests/unit/test_minion.py index 089c6096e696..738f239b82bd 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -1086,9 +1086,7 @@ def test_target_bounds_worker_dns_retry(minion_opts): def fake_stack_context(*args, **kwargs): return contextlib.nullcontext() - with patch.object( - salt.minion.Minion, "_thread_return", MagicMock() - ), patch( + 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.