Add opt-in memory-headroom backpressure for EventPublisher + MWorkerQueue - #70053
Draft
dwoz wants to merge 2 commits into
Draft
Add opt-in memory-headroom backpressure for EventPublisher + MWorkerQueue#70053dwoz wants to merge 2 commits into
dwoz wants to merge 2 commits into
Conversation
…ueue Mirrors the minion-side pattern in saltstack#70038 (minion_memory_headroom) so operators running the master under a tight cgroup limit (VMSP small = 1 GiB, k8s memory.limit, etc.) can bound in-flight work per subprocess before OOM. Four new master config options: event_publisher_memory_headroom (str/int, default None) event_publisher_memory_max (str/int, default None) mworker_queue_memory_headroom (str/int, default None) mworker_queue_memory_max (str/int, default None) Plus a shared knob controlling the sample cadence: event_publisher_memory_check_interval (float, default 0.5) Value semantics match saltstack#70038 exactly: * headroom: percentage string ("5%") or absolute size ("500M", "5G", int bytes). * max: absolute size / int bytes override for the reference "total memory available". * Reference-total precedence: max opt > cgroup v2 > cgroup v1 > psutil.virtual_memory().total. When neither opt in a pair is set, no check runs and behavior is byte-for-byte identical to the previous unbounded default. Opt-in only, per the LTS convention (saltstack#69443 auth_retries, saltstack#69597 gpg_decrypt). Enforcement: * MasterPubServerChannel._publish_daemon creates an asyncio.Event gate (initially set = permit) and a PeriodicCallback that toggles the gate every event_publisher_memory_check_interval seconds based on has_memory_headroom(). publish_payload awaits the gate before dispatching a new event. Backpressure propagates upstream via the puller's existing inline await. * MWorkerQueue.zmq_device_pooled caches the check result for the same interval and, when the check fails, skips recv_multipart on the ROUTER socket for that poll iteration. Messages stay in ZMQ's ROUTER queue; once RCVHWM fills, peer sends block per ZMQ semantics. Worker responses (DEALER -> ROUTER) are always drained so in-flight work can complete. * MWorkerQueue.zmq_device (non-pooled) is a C-level zmq.device(zmq.QUEUE, ...) proxy with no Python hook point; setting the opts here logs a warning at start-up and has no effect. Helpers extracted to a new module salt/utils/memory.py so the master paths here and the minion path in saltstack#70038 can converge on the same implementation. When saltstack#70038 lands the minion's private helpers can be refactored to import from salt.utils.memory in a follow-up. Docs at doc/ref/configuration/master.rst. Tests to follow in a subsequent commit on this branch.
Covers the new salt.utils.memory helpers and the two enforcement paths introduced in the preceding commit: * tests/pytests/unit/utils/test_memory.py (new): 51 cases spanning parse_size, parse_headroom, _read_cgroup_file, _parse_self_cgroup, _detect_cgroup_memory, resolve_memory_reference precedence chain, and the full has_memory_headroom matrix (both opts unset -> True; psutil missing -> True; over-limit -> False + WARNING log; bogus headroom -> 5% fallback; exceptions swallowed). Mirrors the fixture shape from saltstack#70038's tests/pytests/unit/test_minion_memory_headroom.py. * tests/pytests/unit/channel/test_server.py: 6 cases covering the MasterPubServerChannel._ep_memory_gate contract -- no-opt is a permit, set-from-start is a no-op, cleared blocks publish_payload, mid-await set releases it, and the PeriodicCallback closure toggles the gate based on has_memory_headroom's return value. * tests/pytests/unit/transport/test_zeromq.py: 7 cases covering MWorkerQueue -- the non-pooled zmq_device warning fires (and only fires) when an opt is set; pooled zmq_device_pooled skips ROUTER recv_multipart when has_memory_headroom is False, always drains DEALER responses (worker replies must flow), admits when headroom is OK, caches the check per event_publisher_memory_check_interval, and never calls the check on the default (unset-opts) path.
Contributor
Author
|
Tests added in c67ccad (branch 64 new cases across 3 files:
Local run: Coverage gaps to note:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Companion to minion-side #70038: same shape, same opt naming, same reference-memory resolution precedence — applied to the master's
EventPublisherand pooledMWorkerQueuesubprocesses so operators running the master under a tight cgroup limit (VMSP small = 1 GiB, k8s memory.limit) can bound in-flight work before OOM.New master options
event_publisher_memory_headroom"5%"/"500M"/ int bytesNoneevent_publisher_memory_max"5G"/ int bytesNonemworker_queue_memory_headroom"5%"/"500M"/ int bytesNonemworker_queue_memory_max"5G"/ int bytesNoneevent_publisher_memory_check_interval0.5Reference-memory precedence (identical to #70038):
*_memory_maxoptmemory.max/memory.currentmemory.limit_in_bytes/memory.usage_in_bytespsutil.virtual_memory().total/.usedWhen neither opt in a pair is set, no check runs and behavior is byte-for-byte identical to the previous unbounded default. Opt-in only per the LTS convention.
Enforcement
EventPublisher —
MasterPubServerChannel._publish_daemonstarts anasyncio.Eventgate and aPeriodicCallbackthat toggles the gate everyevent_publisher_memory_check_intervalseconds based onhas_memory_headroom().publish_payloadawaits the gate before dispatching a new event. Backpressure propagates upstream viaTCPPuller's inlineawait payload_handler(...)— producer processes (MWorkers, salt CLI, salt-run) block on their outboundsend().MWorkerQueue (pooled) —
zmq_device_pooled's Python poll loop caches the check for the same interval and, when the check fails, skipsrecv_multiparton the ROUTER socket for that poll iteration. Messages stay in ZMQ's ROUTER queue; onceRCVHWMfills, peersend()blocks per ZMQ semantics. Worker responses (DEALER → ROUTER) are always drained so in-flight work can complete.MWorkerQueue (non-pooled) —
zmq_deviceis a C-levelzmq.device(zmq.QUEUE, ...)proxy with no Python hook point. Setting the opts here logs a warning at start-up and has no effect. Requiresworker_poolsto be set.Coherence with #70038
Helpers are extracted into a new module
salt/utils/memory.pyso the master paths here and the minion path in #70038 can converge on the same code:parse_size(value)— "5G" / "500M" / int → bytesparse_headroom(value, reference)— "5%" / size → bytes_read_cgroup_file,_parse_self_cgroup,_detect_cgroup_memoryresolve_memory_reference(max_opt)— precedence chainhas_memory_headroom(opts, headroom_opt_key, max_opt_key, subject=None)— the actual check, parameterized on opt keys so both master (EP / MWQ) and minion (existing) can call itWhen #70038 lands, its private methods on
Minioncan be refactored in a follow-up commit to import fromsalt.utils.memory.Test plan
salt/utils/memory.py— parser edge cases, cgroup v1/v2 detection with synthetic tmp_path cgroupfs (mirror patterns from Add opt-in cgroup-aware minion_memory_headroom / minion_memory_max (#69884) #70038's tests)MasterPubServerChannel.publish_payload— gate blocks when headroom fails, unblocks when it passes, no-op when opt unsetzmq_device_pooled— skipsrecv_multipartwhen headroom check fails; drains worker responses regardless