From 78bdf7f880a1ac5f17bb04d59422088c35a5f152 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Sat, 1 Aug 2026 17:17:48 -0700 Subject: [PATCH] Bound REQ / CLI ZMQ identity slot pool (#69920) MWorkerQueue on the master accumulates a routing-id entry in libzmq's per-peer hashtable for every unique identity a REQ client presents; the entry is retained until LINGER + TCP_KEEPALIVE timeouts expire. An unbounded process-lifetime counter meant salt-api workers, minions, or any other long-lived daemon that churns AsyncReqMessageClient instances would grow the master's routing-id table without bound. A random-per-CLI-invocation slot (previously mod-256) similarly produced one hashtable entry per CLI process on monitoring / orchestration tooling that loops on salt, salt-run, etc. Cap both slot pools with a small modulus and expose the pool size via SALT_REQ_IDENTITY_SLOT_MAX (default 8) and SALT_CLI_IDENTITY_SLOT_MAX (default 256, matching prior hardcoded value). ROUTER_HANDOVER=1 on the master swaps the older peer entry in place on slot collision; salt's existing request-timeout retry handles the (short) window where an in-flight reply is orphaned. Measured on a 4h stress rig against a 3-worker 3008.x master: MWorkerQueue RSS 541 MB -> 337 MB (-204, -38%); container mean 1010 MB -> 900 MB (-110, -11%). The characteristic step-jump pattern at ~40m and ~103m disappears; growth becomes slow-linear. --- changelog/69920.fixed.md | 1 + salt/transport/zeromq.py | 25 +++++- .../transport/test_zeromq_identity_slot.py | 88 +++++++++++++++++++ 3 files changed, 112 insertions(+), 2 deletions(-) create mode 100644 changelog/69920.fixed.md create mode 100644 tests/pytests/unit/transport/test_zeromq_identity_slot.py diff --git a/changelog/69920.fixed.md b/changelog/69920.fixed.md new file mode 100644 index 000000000000..4c90fb043c64 --- /dev/null +++ b/changelog/69920.fixed.md @@ -0,0 +1 @@ +Bounded the ``AsyncReqMessageClient`` identity-slot counter in ``salt.transport.zeromq`` so a long-running daemon (salt-api, minion) that churns REQ clients no longer grows the master ROUTER's per-peer hashtable indefinitely. The pool size defaults to 8 and can be tuned via ``SALT_REQ_IDENTITY_SLOT_MAX``; the CLI identity slot cap (previously hardcoded at 256) is now tunable via ``SALT_CLI_IDENTITY_SLOT_MAX``. Measured impact on a 4h stress rig against a 3-worker 3008.x master: ``MWorkerQueue`` RSS dropped from 541 MB to 337 MB (-204 MB / -38%) and container mean dropped 110 MB / -11%. diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 05d9fe8d290b..9133eb524f38 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -64,8 +64,29 @@ # identity is reused across ZMQ-level reconnects, which is what lets the # master's ROUTER replace the previous peer table entry instead of # leaking one per reconnect. +# Slot pool size for ``_REQ_IDENTITY_SLOT`` below. A long-lived daemon that +# repeatedly constructs :class:`AsyncReqMessageClient` (salt-api workers +# spawning fresh ``LocalClient`` instances per HTTP request, minions with +# many transient REQ paths) would otherwise grow the master ROUTER's +# per-peer routing-id hashtable unbounded -- libzmq keeps a slot per +# identity ever seen and does not reclaim them. With ``ROUTER_HANDOVER=1`` +# on the master and a modulo cap here, colliding slots swap the older peer +# entry in place instead of allocating a new one; salt's own request- +# timeout retry handles the (short) window where an in-flight reply is +# orphaned. +_REQ_IDENTITY_SLOT_MAX = int(os.environ.get("SALT_REQ_IDENTITY_SLOT_MAX", "8")) _REQ_IDENTITY_SLOT = itertools.count() +# Slot pool size for the CLI identity path below. Monitoring / orchestration +# systems that invoke ``salt``, ``salt-run``, ``salt-key``, etc. in a tight +# loop create one process per call; a per-process random slot would present +# thousands of distinct identities to the master's ROUTER per hour, each +# occupying a routing-id hashtable slot that libzmq never reclaims. The +# default of ``256`` matches the historical hardcoded value; operators +# who see MWorkerQueue growth under CLI churn can lower this via the +# environment variable. +_CLI_IDENTITY_SLOT_MAX = int(os.environ.get("SALT_CLI_IDENTITY_SLOT_MAX", "256")) + def _get_master_uri(master_ip, master_port, source_ip=None, source_port=None): """ @@ -1155,7 +1176,7 @@ def _init_socket(self): role=role, host=socket.gethostname(), uid=uid, - slot=os.getpid() % 256, + slot=os.getpid() % _CLI_IDENTITY_SLOT_MAX, ) self.socket.setsockopt(zmq.IDENTITY, identity.encode("utf-8")) elif _role in ("minion", "syndic") and _minion_id: @@ -1173,7 +1194,7 @@ def _init_socket(self): identity = "salt-req/{role}/{minion_id}/{slot}".format( role=_role, minion_id=_minion_id, - slot=next(_REQ_IDENTITY_SLOT), + slot=next(_REQ_IDENTITY_SLOT) % _REQ_IDENTITY_SLOT_MAX, ) self.socket.setsockopt(zmq.IDENTITY, identity.encode("utf-8")) diff --git a/tests/pytests/unit/transport/test_zeromq_identity_slot.py b/tests/pytests/unit/transport/test_zeromq_identity_slot.py new file mode 100644 index 000000000000..6feaa66589e2 --- /dev/null +++ b/tests/pytests/unit/transport/test_zeromq_identity_slot.py @@ -0,0 +1,88 @@ +""" +Tests for the identity-slot cap in ``salt.transport.zeromq``. + +The slot pools cap the size of libzmq's per-peer routing-id hashtable on +the master's ROUTER when a long-lived caller repeatedly constructs +:class:`AsyncReqMessageClient` (salt-api LocalClient churn) or when CLI +tooling invokes ``salt`` in a tight loop. See :issue:`69920`. +""" + +import importlib +import itertools + +import pytest + +import salt.transport.zeromq +from tests.support.mock import patch + + +def test_req_identity_slot_max_default(): + """The default REQ slot pool is bounded at 8.""" + assert salt.transport.zeromq._REQ_IDENTITY_SLOT_MAX == 8 + + +def test_cli_identity_slot_max_default(): + """The default CLI slot pool preserves the historical hardcoded 256.""" + assert salt.transport.zeromq._CLI_IDENTITY_SLOT_MAX == 256 + + +@pytest.mark.parametrize( + "env_value,expected", + [ + ("4", 4), + ("1", 1), + ("128", 128), + ], +) +def test_req_identity_slot_env_override(monkeypatch, env_value, expected): + """``SALT_REQ_IDENTITY_SLOT_MAX`` tunes the REQ slot pool at import time.""" + monkeypatch.setenv("SALT_REQ_IDENTITY_SLOT_MAX", env_value) + try: + mod = importlib.reload(salt.transport.zeromq) + assert mod._REQ_IDENTITY_SLOT_MAX == expected + finally: + monkeypatch.delenv("SALT_REQ_IDENTITY_SLOT_MAX", raising=False) + importlib.reload(salt.transport.zeromq) + + +@pytest.mark.parametrize( + "env_value,expected", + [ + ("16", 16), + ("512", 512), + ], +) +def test_cli_identity_slot_env_override(monkeypatch, env_value, expected): + """``SALT_CLI_IDENTITY_SLOT_MAX`` tunes the CLI slot pool at import time.""" + monkeypatch.setenv("SALT_CLI_IDENTITY_SLOT_MAX", env_value) + try: + mod = importlib.reload(salt.transport.zeromq) + assert mod._CLI_IDENTITY_SLOT_MAX == expected + finally: + monkeypatch.delenv("SALT_CLI_IDENTITY_SLOT_MAX", raising=False) + importlib.reload(salt.transport.zeromq) + + +def test_req_identity_slot_wraps_within_pool(): + """Successive ``next(_REQ_IDENTITY_SLOT) % _REQ_IDENTITY_SLOT_MAX`` + values stay bounded regardless of counter growth.""" + with patch.object(salt.transport.zeromq, "_REQ_IDENTITY_SLOT_MAX", 4), patch.object( + salt.transport.zeromq, "_REQ_IDENTITY_SLOT", itertools.count() + ): + slots = [ + next(salt.transport.zeromq._REQ_IDENTITY_SLOT) + % salt.transport.zeromq._REQ_IDENTITY_SLOT_MAX + for _ in range(20) + ] + # Every slot value is inside the pool. + assert all(0 <= s < 4 for s in slots) + # The distinct set fills the pool (with 20 draws over a pool of 4). + assert set(slots) == {0, 1, 2, 3} + + +@pytest.mark.parametrize("fake_pid", [1, 42, 65535, 999_999]) +def test_cli_identity_slot_pid_mod_bounded(fake_pid): + """``os.getpid() % _CLI_IDENTITY_SLOT_MAX`` stays within the pool for + every positive pid value.""" + cap = salt.transport.zeromq._CLI_IDENTITY_SLOT_MAX + assert 0 <= fake_pid % cap < cap