Thread safe loader 2018.3.3 #50648
Thread safe loader 2018.3.3 #50648
Conversation
ghost
requested review from
|
.oO(Wonders how to review 1,419 files now) Is that possible you cherry-pick or rebase that instead? |
|
@isbm: Sure, I will create a proper PR with my changes neatly squashed into one commit as soon as I have figured out what's causing the problems. I started this try out on the 2018.3.3 branch, but then decided to switch develop because it seemed like Jenkins was not running the tests at all and I though that it might work with the develop branch. Seems like even with that branch, the tests do not run at all in the Ubuntu 16.04 test environment. Incidentally, I also use a Ubuntu 16.04 VM for my test environment and the fact that I am having funny problems in that environment was the main reason why I created this PR in the first place (to have a playground where I can test various approaches without having to worry about a working test environment). |
|
👍 👍 |
|
Really glad to see this, @smarsching. We want to be very focused on getting this in as soon as we can. I sincerely apologize for the ridiculous delay on this. It's our fault and we appreciate you sticking with us. :) |
|
Just so that you know: I found the solution to why my changes caused so many problems and an elegant fix for these problems. Now, I have to clean up my branch (it got really messy from trying so many different things), and the I will force-push this changes to the branch referenced by this PR. I will keep the branch based against 2018.3.3 (as I need a patch for that version for internal use anyway), but from that point it should be pretty straight forward of porting the changes to the develop branch. I cannot promise that I will get this done today (its already 7 pm around here), but if not you can expect to have the cleaned up version by tomorrow. |
|
@smarsching that is very cool. Looking forward for your final PR! |
This commit introduces salt.utils.msgpack modifies all places in the code that either use json or msgpack to use salt.utils.json or salt.utils.msgpack respectively. While this change itself does not have any effect, it is important to allow for centrally dealing with objects that cannot be directly serialied via json or msgpack.
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 and salt.utils.msgpack modules have been modified to take care of unwrapping objects that are proxied using the ThreadLocalProxy.
smarsching
force-pushed
the
thread-safe-loader-2018.3.3
branch
from
8531ab2 to
c00d9a4
Compare
|
From my point of view, the code is complete, but Jenkins did not run the tests (most likely because now it is again based on 2018.3.3). Therefore, I created a new PR #50655 that is now based on develop. Let's see whether the tests run now or whether there are still some problems. |
When accessing msgpack.Unpacker, we cannot use salt.utils.msgpack, but we do not have to either.
This is a new version of #48598 that is now based against the 2018.3.3 branch. I made a few modifications that might fix (at least some of) the test failures we see in the original PR.
I only created this PR so that I can run Jenkins on my changes. When testing changes locally, I get sporadic errors that do not really seem to be related to my changes (the same code sometimes runs and sometimes fails). I want to debug things in a well-defined environment so that I can concentrate on what effects certain changes have...
I do not intend this PR to be merged and will close it in favor of #48598 once I have finished my tests.