Fix concurrent state.* runs when queue=True and JID sorts high#69826
Open
dwoz wants to merge 1 commit into
Open
Fix concurrent state.* runs when queue=True and JID sorts high#69826dwoz wants to merge 1 commit into
dwoz wants to merge 1 commit into
Conversation
`state.apply queue=True` (and every `state.*` function that honors the `queue` argument) is documented to guarantee at most one `state.*` execution per minion at a time. In practice, when several state runs were published to the same minion in rapid succession, more than one could dispatch concurrently to the `proc/` directory instead of serializing through `queues/<master>/state_queue/`. The root cause was the JID string comparison in `salt.utils.state.check_prior_running_states`: it only treated *strictly older* JIDs as blocking, so any concurrently running state whose JID sorted higher than the current one was silently ignored. The result was two active proc entries whenever `_check_queue` for a new job happened to run before the peer subprocess's proc file was on disk. `check_prior_running_states` now blocks on any real running state.* process (non-zero PID) regardless of JID ordering, while queued placeholders (`pid == 0`, produced by scanning the state_queue / job_queue directories) continue to use the FIFO comparison so the state-queue processor can dequeue the oldest queued JID without deadlocking on its younger siblings. Fixes saltstack#69825
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
1 participant
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.
What does this PR do?
Fixes issue #69825.
state.apply queue=True(and everystate.*module function thathonors the
queueargument via_check_queue) is documented toguarantee at most one
state.*execution per minion at a time. Inpractice, when several state runs were published to the same minion
in rapid succession, more than one could dispatch concurrently to the
proc/directory instead of serializing throughqueues/<master>/state_queue/.Root cause
salt.utils.state.check_prior_running_statesfiltered blocking jobswith
str(data_jid) < str(jid)-- only treating strictly older JIDsas blockers. Any concurrently running state whose JID sorted higher
than the current one was silently ignored, so the second (higher-JID)
subprocess entering
_check_queuesaw an empty active-list, passedthe queue check, and dispatched in parallel.
Fix
check_prior_running_statesnow blocks on any real runningstate.* process (non-zero PID) regardless of JID ordering. Queued
placeholder entries (
pid == 0, produced by scanning the state_queue /job_queue directories inside the function itself) keep the FIFO JID
comparison so the state-queue processor can safely dequeue the oldest
queued JID without deadlocking on its younger queued siblings.
Tests
Added two unit regression tests in
tests/pytests/unit/modules/state/test_state.py::TestCheckPriorRunningStates:test_check_prior_running_states_blocks_on_higher_jid_running--asserts a running state.* job with a higher JID blocks the current job.
test_check_prior_running_states_ignores_higher_jid_queued_placeholder--asserts queued placeholders keep FIFO semantics (only older placeholders
block; younger ones do not, so the queue processor still makes progress).
The pre-existing
test_state_queue_basicintegration test continues topass, confirming the queue processor is not deadlocked by the fix.
Fixes
Fixes #69825
Notes
The reporter also mentioned a residual race between the queue-lock
check and the proc-file write in
salt.minion._thread_return. This PRdoes not close that microsecond window -- the JID-comparison bug
addressed here is orders of magnitude wider than that window, and
closing it fully requires a reservation-marker written inside the
queue-lock critical section (a larger refactor left for a follow-up).