Fix wrong retcodes from concurrent jobs on a threaded minion#69755
Open
ggiesen wants to merge 1 commit into
Open
Fix wrong retcodes from concurrent jobs on a threaded minion#69755ggiesen wants to merge 1 commit into
ggiesen wants to merge 1 commit into
Conversation
With multiprocessing:False every job runs against one shared minion_instance. _thread_return calls gen_modules() at the top of every job, which rebuilds all loaders and rebinds minion_instance.functions to a brand-new LazyLoader whose __context__ is a fresh, empty dict. A module writes its retcode into the __context__ of the loader generation it ran under (e.g. cmdmod via NamedLoaderContext), but _thread_return read the retcode back from minion_instance.functions at completion. When a sibling job's gen_modules() rebound functions between a job's write and its read, the read landed on the new empty context and returned EX_OK, so a failed command was delivered to the master as a success. Make each job own the loader it wrote its retcode into: - gen_modules() now returns the loader generation it built. - _thread_return / _thread_multi_return capture that return value and use it for the function lookup, the retcode reset, and the retcode read, instead of re-reading the shared minion_instance.functions attribute. - _execute_job_function takes the captured loader (defaulting to self.functions) and uses it for the lookup and the retcode reset. - sys.reload_modules is bound to a new _reload_modules wrapper that reloads and returns None, so the reload job stays serializable on the wire (a LazyLoader is not). - The proxy and delta-proxy thread_return / thread_multi_return copies capture minion_instance.functions once per job and use it consistently, closing the same write/read desync there. Adds a direct-altitude regression test that drives the real Minion._thread_return with the poison sibling reload forced into the write->read window and asserts a failed job is delivered as failed (and a passing job as passing), plus a test that sys.reload_modules still returns a serializable None. Fixes saltstack#61830
ggiesen
force-pushed
the
fix-61830-retcode-race
branch
from
2497131 to
37e192b
Compare
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
3 participants
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?
Makes a threaded minion (
multiprocessing: False) report the correct retcode for concurrent jobs. Each job now reads its retcode from the loader generation it actually ran under, instead of from a shared attribute that a sibling job can swap out mid-job.What issues does this PR fix or reference?
Fixes #61830
Fixes #63117
Previous Behavior
With
multiprocessing: False, each job runs as a thread against the one sharedMinioninstance.Minion._thread_returncallsminion_instance.gen_modules()at the top of every job, which rebuilds and rebinds the sharedminion_instance.functions(the per-job rebuild added in9f1fe42b3cto reduceos.forkraces). A module writes its retcode into the loader generation it ran under (__context__["retcode"]), but_thread_returnreads the retcode back fromminion_instance.functionsat completion. If a concurrent job'sgen_modules()rebindsminion_instance.functionsbetween the write and the read, the read lands on a fresh, empty context and defaults toEX_OK-- so a failed job is reported as succeeded (and, symmetrically, a passing job can inherit a sibling's failure).multiprocessing: Trueavoids it because each job is a separate process. This is #61830, and described independently in #63117.New Behavior
gen_modules()returns the execution-module loader it built, and_thread_return/_thread_multi_returncapture that returned loader and thread it through_execute_job_function, so the func lookup, theretcodereset, the module's write, and the read all resolve to the one loader that job owns. A concurrentgen_modules()rebindingminion_instance.functionscan no longer steal the read. The same capture is applied to the metaproxy and deltaproxy copies of these methods.Because
gen_modulesis also bound as thesys.reload_modulesexecution function and a loader is not serializable over the wire,sys.reload_modulesis rebound to a thin_reload_moduleswrapper that reloads and returnsNone, preserving the published wire contract.This is a targeted correctness fix; it deliberately keeps the per-job
gen_modules()rebuild. Removing that rebuild is a real but separate performance improvement (measured at ~21 ms/job, so it matters most for high-frequency light jobs) that requires isolating__context__oncontextvarsfirst. Tracked separately in #69756 with the before/after measurements.Merge requirements satisfied?
changelog/61830.fixed.mdtests/pytests/unit/test_minion.py: a direct-altitude test drives the real_thread_returnwith a forced siblinggen_modulesrebind in the write->read window and asserts a failed job (retcode 42) is delivered failed while the rebound shared loader is empty, plus the inverse (a passing job stays success); and a test thatsys.reload_modulesreturnsNoneand is wire-serializable (whilegen_modules()itself returns a non-serializable loader -- why the wrapper exists). Both pin-proofed against baselineminion.py.Commits signed with GPG?
No