Thread safe loader 2 #48598
Thread safe loader 2 #48598
Conversation
There was a race condition in the salt loader when injecting global values (e.g. "__pillar__" or "__salt__") into modules. One effect of this race condition was that in a setup with multiple threads, some threads may see pillar data intended for other threads or the pillar data seen by a thread might even change spuriously. There have been earlier attempts to fix this problem (saltstack#27937, saltstack#29397). These patches tried to fix the problem by storing the dictionary that keeps the relevant data in a thread-local variable and referencing this thread-local variable from the variables that are injected into the modules. These patches did not fix the problem completely because they only work when a module is loaded through a single loader instance only. When there is more than one loader, there is more than one thread-local variable and the variable injected into a module is changed to point to another thread-local variable when the module is loaded again. Thus, the problem resurfaced while working on saltstack#39670. This patch attempts to solve the problem from a slightly different angle, complementing the earlier patches: The value injected into the modules now is a proxy that internally uses a thread-local variable to decide to which object it points. This means that when loading a module again through a different loader (possibly passing different pillar data), the data is actually only changed in the thread in which the loader is used. Other threads are not affected by such a change. This means that it will work correctly in the current situation where loaders are possibly created by many different modules and these modules do not necessary know in which context they are executed. Thus it is much more flexible and reliable than the more explicit approach used by the two earlier patches. Unfortunately, the stand JSON and Msgpack serialization code cannot handle proxied objects, so they have to be unwrapped before passing them to that code. The salt.utils.json module has been modified to takes care of unwrapping objects that are proxied using the ThreadLocalProxy. The salt.utils.msgpack module has been added and basically provides the same functions as the salt.utils.json module, but for msgpack. Like the json module, it takes care of unwrapping proxies.
ghost
requested review from
|
Go Go Jenkins! |
salt/utils/msgpack.py
Outdated
|
|
||
| def pack(o, stream, **kwargs): | ||
| ''' | ||
| .. versionadded:: Oxygen |
salt/utils/msgpack.py
Outdated
|
|
||
| def packb(o, **kwargs): | ||
| ''' | ||
| .. versionadded:: Oxygen |
salt/utils/msgpack.py
Outdated
|
|
||
| def unpack(stream, **kwargs): | ||
| ''' | ||
| .. versionadded:: Oxygen |
salt/utils/msgpack.py
Outdated
|
|
||
| def unpackb(packed, **kwargs): | ||
| ''' | ||
| .. versionadded:: Oxygen |
salt/utils/thread_local_proxy.py
Outdated
| Proxy object that can reference different values depending on the current | ||
| thread of execution. | ||
|
|
||
| ..versionadded:: Oxygen |
|
Also, looks like there are some pylint warnings in salt/state.py https://jenkinsci.saltstack.com/job/pr-lint/job/PR-48598/1/warnings52Result/ |
mattp-
force-pushed
the
thread-safe-loader-2
branch
from
eefaa64 to
8fbb40b
Compare
salt/transport/zeromq.py
Outdated
| @@ -433,7 +433,14 @@ def __del__(self): | |||
| def connect(self): | |||
| if not self.auth.authenticated: | |||
| yield self.auth.authenticate() | |||
| self.publish_port = self.auth.creds['publish_port'] | |||
|
|
|||
There was a problem hiding this comment.
I'm confused about the relationship between loader thread-safety and these proposed changes to the transports.
|
Sorry, that accidentally snuck in. It's from my other PR that I put on
develop, not a feature branch. You can ignore.
…On Mon, Jul 16, 2018, 5:57 PM Mike Place ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In salt/transport/zeromq.py
<#48598 (comment)>:
> @@ -433,7 +433,14 @@ def __del__(self):
def connect(self):
if not self.auth.authenticated:
yield self.auth.authenticate()
- self.publish_port = self.auth.creds['publish_port']
+
I'm confused about the relationship between loader thread-safety and these
proposed changes to the transports.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#48598 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACtVkRPslxi3_2VhSmeGf2YrC2kBLr2ks5uHQxLgaJpZM4VQclz>
.
|
|
@isbm Does anybody on @saltstack/team-suse want to review this? We're getting close to merging this in. |
|
@mattp- Can you rebase against the head of develop so we have only the related changes? That should clear out those transport changes so we know exactly what we're dealing with here. |
|
@cachedout yep will do shortly. to be clear I haven't made any changes yet; though I did notice one incompatibility between how data.serializers.msgpack and data.utils.msgpack is handled that I need to look at more. |
mattp-
force-pushed
the
thread-safe-loader-2
branch
from
46d2b5e to
1215422
Compare
|
unassociated commit removed |
|
@mattp- Cool. We also need the issues raised by @gtmanfred resolved before we can move forward, please. |
|
Something isn't right here. The test suite is not running the tests. @mattp- Can you take a look at the console output from the test run? It's having trouble in test-daemon startup: https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-48598/5/console |
salt.serializers.msgpack indirectly calls it down the stack, alongside other backcompat managament.
|
@gtmanfred Re-review requested please. |
|
I spent some time looking at this today and I'm honestly not sure what's happening. It's failing in different ways on different platforms and, oddly enough, it seems to work just fine on my development machine. |
|
closing in favor of #50648 |
What does this PR do?
this is a version of #45782 with latest develop merged in. I've not made any changes other than resolving merge conflicts; the original jenkins results have aged out so I'm creating this to re-run. If I figure out a solution I'll send PR to @smarsching and close this.
What issues does this PR fix or reference?
Previous Behavior
Remove this section if not relevant
New Behavior
Remove this section if not relevant
Tests written?
Yes/No
Commits signed with GPG?
Yes/No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.