Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3006.x] Fix parallel states on spawning platforms #66997

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

lkubb
Copy link
Contributor

@lkubb lkubb commented Oct 24, 2024

What does this PR do?

  • Fixes a crash introduced in [3006.x] Fix parallel state execution with Salt-SSH #66517 (Name mangling rewrites __invocation_jid to _State__invocation_jid, causing a TypeError when instantiating the class instance with **init_kwargs). This patch is not part of any release as of now.
  • Fixes parallel runs of state modules which rely on global dunders like __env__ to be defined
  • Fixes parallel runs of state modules when the context dict contains unpicklable objects (e.g. when the match module has been invoked, which uses it to cache a LazyLoader instance)

What issues does this PR fix or reference?

Fixes: #66996
Fixes: #66999

Previous Behavior

  • Crashes with a TypeError because of the first issue (this makes parallel states not work at all on spawning platforms).
  • Fixing the previous, crashes with a NameError because __env__ is not defined.
  • Fixing the previous and having an unpicklable object in __context__, crashes with a TypeError, e.g. cannot pickle '_thread.RLock' object

New Behavior

Works as expected.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@lkubb lkubb requested a review from a team as a code owner October 24, 2024 12:35
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Fix parallel states on spawning platforms [3006.x] Fix parallel states on spawning platforms Oct 24, 2024
@lkubb lkubb force-pushed the fix/parallel-spawning-inject-globals branch from 3cf69dc to b707d24 Compare October 24, 2024 13:58
dmurphy18
dmurphy18 previously approved these changes Oct 24, 2024
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs failing tests resolved

This was introduced by saltstack#66517

`__invocation_jid` is rewriten to `_State__invocation_id`, making
`cls(**init_kwargs)` fail with a TypeError (unexpected argument
`__invocation_jid` (this behavior is called name mangling).
This ensures dunders like __env__ are defined in states running in
parallel on spawning platforms.

The __running__ dict can contain salt.utils.process.Process instances,
which are still picklable.
@lkubb lkubb force-pushed the fix/parallel-spawning-inject-globals branch from b707d24 to dd6d058 Compare October 24, 2024 19:04
@lkubb
Copy link
Contributor Author

lkubb commented Oct 24, 2024

I rebased onto the current branch with #66986 merged, should be solved now

@lkubb
Copy link
Contributor Author

lkubb commented Oct 24, 2024

Sorry for dismissing the review, I stumbled upon another related issue (#66999).

@dwoz dwoz merged commit fd7b0dc into saltstack:3006.x Oct 25, 2024
452 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test:full Run the full test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants