Isolate the active-HighState stack per execution context (#63056)#69782
Isolate the active-HighState stack per execution context (#63056)#69782ggiesen wants to merge 5 commits into
Conversation
…3056) Concurrent state/orchestration renders shared a single class-level HighState.stack list. When the reactor renders orchestrations in parallel worker threads (it runs them inline, without forking), one render's push_active was visible to another, so HighState.get_active could return the wrong HighState in the middle of a render. This surfaced as an IndexError popping an empty pydsl render stack, or KeyError: '__env__' from a spuriously detected conflicting ID. Store the stack in a contextvars.ContextVar instead, mirroring salt.loader's loader_ctxvar, so each execution context (and therefore each reactor worker thread) gets its own stack. The pydsl top-file matches, previously cached in a module-level SLS_MATCHES global with the same sharing problem, are now cached per HighState instance. SSHHighState.push_active was a redundant override reaching for the removed class attribute, so it now inherits the context-backed base method. The conflicting-ID error formatter also uses .get() so a genuine conflict reports cleanly instead of raising KeyError.
…sh_active The saltstack#63056 refactor put the active-HighState stack accessors (push_active/pop_active/get_active/clear_active) on HighState. But SSHHighState subclasses BaseHighState, not HighState, and the salt-ssh state wrapper calls st_.push_active() on every run -- so every salt-ssh state execution raised "'SSHHighState' object has no attribute 'push_active'". Move the accessors to BaseHighState (their shared ancestor) so both HighState and SSHHighState resolve them and share the one context-isolated stack, restoring the original shared-stack semantics without an SSH-specific override. Add a regression test that exercises SSHHighState directly -- the original test only covered HighState, which is why the break reached CI.
|
@ggiesen Looks like we have some test failures |
|
The remaining red is |
This PR adds `import contextvars` to salt/state.py, which is imported at every salt-call startup (salt.minion -> salt.utils.state -> salt.state). A Python 3.6 target has no stdlib contextvars, so the thin ships a backport; that backport imports immutables, which imports typing_extensions -- not previously bundled -- so salt-call died with `ModuleNotFoundError: No module named 'typing_extensions'`. Bundle typing_extensions alongside the immutables the thin already ships for py3.6, mirroring the has_immutables pattern, and extend the get_tops tests to expect it. This is what integration/ssh/test_log.py (a python 3.6 container) was tripping on.
|
Correction to my earlier comment -- this is not a flake, my mistake. Fixed in cf523c4: bundle |
…xtensions bundle The previous commit bundled typing_extensions into the salt-ssh thin, but the thin ships the build host's (modern) typing_extensions, whose py3.8+ syntax is a SyntaxError on a Python 3.6 target -- and it shadows the target's own compatible typing_extensions, so importlib_metadata (imported at salt-call startup via salt._compat) crashed before reaching any of this PR's code. That broke salt-ssh on every py3.6 target (Rocky Linux 8, Amazon Linux 2). Reverted. Instead, guard "import contextvars" in salt/state.py. On targets where it can only be resolved through the thin's backport -- which drags in immutables/typing_extensions that may be missing (a py3.9 Debian target) or incompatible (py3.6) -- catch ImportError/SyntaxError and fall back to a shared class-level active-HighState stack. salt-ssh runs one execution per target, so the per-context isolation (saltstack#63056) is not needed there; on py3.7+ minions and the onedir master/minion, stdlib contextvars is used and the isolation is unchanged. Validated on live Python 3.6.8 (AlmaLinux 8): a modern typing_extensions is a SyntaxError on py3.6, and the guarded import catches both that and a missing typing_extensions, engaging the fallback without crashing. Adds a test for the fallback path.
|
Correction on my correction, @twangboy -- the typing_extensions-in-the-thin approach was wrong and I've reverted it. The thin ships the build host's modern Real fix in fe8ca91: revert the thin change and guard Validated this time on live Python 3.6.8 (AlmaLinux 8): a modern |
3 participants
What does this PR do?
Isolates the stack of active
HighStateobjects per execution context soconcurrent state/orchestration renders don't corrupt one another.
The stack lived on the class (
HighState.stack = []), shared by every thread.The reactor renders orchestrations in a
ThreadPool(reactor_worker_threads),running the runner inline in worker threads rather than forking. When two
reactor events fire close together, their orchestrations render concurrently in
different worker threads and each
push_active/pop_activemutates the sameshared list.
HighState.get_active()then returns the wrongHighStatein themiddle of a render, which shows up as:
IndexError: list index out of rangepopping an empty pydsl render stack(
renderers/pydsl.pyreadsget_render_stack()[-1]off the wrong HighState), andKeyError: '__env__'when a cross-contaminatedbuilding_highstateproduces aspurious conflicting-ID and the error formatter indexes a chunk that lacks
__env__.The fix stores the stack in a
contextvars.ContextVar, mirroringsalt.loader'sloader_ctxvar(which the failing traceback already runsthrough), so each execution context -- and therefore each reactor worker thread
-- gets its own stack. Three follow-on pieces make the isolation complete:
SLS_MATCHESglobalwith the same cross-run sharing problem; they're now cached per
HighStateinstance.
SSHHighState.push_activewas a redundant override that reached for theremoved class attribute; it now inherits the context-backed base method (so
salt-ssh pydsl states keep working).
.get()for__env__/__sls__, so agenuine conflict reports cleanly instead of raising
KeyError.Single-threaded behavior is unchanged: within one context the lazy per-context
list behaves exactly like the old shared list, and the canonical
push_active()...try/finally: pop_active()pattern keeps the stack balanced.What issues does this PR fix or reference?
Fixes #63056
Previous Behavior
Concurrent reactor orchestrations (or any concurrent renders that touch the
active-HighState stack) intermittently failed with
Rendering SLS ... failed, render error: '__env__'followed byIndexError: list index out of range.Events that arrived too close together dropped their orchestration.
New Behavior
Each render gets its own active-HighState stack and its own pydsl top-file
match cache, so concurrent renders no longer corrupt one another.
Merge requirements satisfied?
A deterministic two-thread regression test asserts each thread sees only the
HighStateit pushed. It fails against the pre-fix code(
{'A': 'B', 'B': 'B'}) and passes with the fix.Commits signed with GPG?
No