RequestContext

[jacksontj]

_Please DO NOT immediately merge_

While investigating #23373 we realized that the reactor problems we were seeing were actually to do with the fact that the loader isn't threadsafe (due to globals injection). Since the direction we are going is more concurrency-- we really need to have a robust solution for making injected variables thread/coroutine safe.

Another system that has something similar is flask's request context). In flask if you want access to the request object, you simply import the object and it will be your version (for your given context). This simple access makes the data storage concurrency a non-issue.

The implementation here is a RequestContext (based on tornado's stack_context). This RequestContext will do all of the bookkeeping of which coroutine/thread is currently executing-- and will switch between the values for each one.

To maintain compatibility with the globals injection (__salt__) I've also created NamespacedDictWrapper which simply looks like the global dict but under the hood calls the appropriate namespace within RequestContext.

[s0undt3ch]

Great usage of tornado's stack context! 👍

[s0undt3ch]

Pitty that Jenkins is not too happy about it...

[jacksontj]

Well this is more of a PoC. At this point it mostly works-- but I haven't
run through to make sure it works everywhere :)

The downside with the current implementation is that there is a single
shared global base-- so its only kinda threadsafe (granted most cases,
but the test suite is the one where it doesn't quite work). Because of that
I'd like to get rid of the magic global version and change it into more
of a class-factory-- so we can make an object per thing (grains, pillar,
etc.) and potentially per master/minion. Or at least make the global values
reset once the parent instance was killed-- still haven't quite sorted out
the specifics.

I mostly wanted to get something out to get some feedback on the API etc. :)

On Sat, Oct 10, 2015 at 9:06 AM, Pedro Algarvio notifications@github.com
wrote:

Pitty that Jenkins is not too happy about it...

—
Reply to this email directly or view it on GitHub
#27843 (comment).

[cachedout]

This looks really elegant. We could definitely make wider use of this! 👍

[jacksontj]

After thinking about it over the weekend, I think I know how to clean this up-- should be able to update the PR in the next day or two.

[cachedout]

Sweet! Looking forward to it. :]

[DmitryKuzmenko]

@jacksontj great approach, I've found a lot of new stuff to learn here. I'm not sure just about tornado's request context, but maybe I didn't got the idea yet. I've slightly updated my version of the fix. If you'd have a time to take a quick look, it would be great to hear your opinion!

[jacksontj]

@DmitryKuzmenko That works for the threading case, but it doesn't work with coroutines (AFAIK) since the context is per thread not per coroutine.

[DmitryKuzmenko]

@jacksontj sounds great. Thank you for figuring it out and for your work! Can't wait to see the final revision. Let me know if I can help.

[igorwidlinski]

So, I've been pulling my hair all day long while trying to start 5 ec2 instances using salt-cloud with the cloud map file, something like this:

salt-cloud -m /etc/salt/cloud.maps.d/dev.staging.map -P

On the master we have a reactor to sync grains:

reactor:
  - 'minion_start':
    - /srv/salt-common/reactor/sync_grains.sls

contents of /srv/salt-common/reactor/sync_grains.sls:

sync_modules:
  local.saltutil.sync_modules:
    - tgt: {{ data['id'] }}

sync_grains:
  local.saltutil.sync_grains:
    - tgt: {{ data['id'] }}

We have custom grain that does not get synced on some minions. Its totally random which minions do not get synced. On the server side I can see the master gets the minion_start event, starts some jobs, but some minions never get the commends to do the sync. Most of the time one minion will sync, while the reminder 4 won't, but it varies.

Anyways, I know this is not a proper place for this, but it seems like it might be related to this bug. Maybe someone can confirm that these are related?

[DmitryKuzmenko]

@igorwidlinski any issue with getting wrong (old or mixed or empty) pillar and/or grains data is 99% related to this. We just have to wait some time while it will be fixed.

[igorwidlinski]

Thanks @DmitryKuzmenko ! Glad its being looked into.

[jacksontj]

At this point I have it working in the threaded minion case, now on to the reactor!

[jacksontj]

That last commit fixes the threaded reactor cases (since we only use the threadpool for runner/wheel).

[cachedout]

This is really clever. I love how clean this is.

[cachedout]

This looks great. I really like it. I'm going to clean up the lint errors in another PR and then get this one merged along with it. Thanks as always for your hard work here, @jacksontj

[cachedout]

#28249 includes these commits but with minor lint fixes. Once tests complete, it will be merged and this PR will be closed as a result.

[jacksontj]

@cachedout Great, glad to get this merged in. I just pushed an additional test case, mind pulling that in as well?

[cachedout]

Will do. Thanks.

[cachedout]

@jacksontj Doesn't look like the linter liked that change at all. Could you have a peek when the test finishes?

[jacksontj]

Yea, will do :)

On Fri, Oct 23, 2015 at 8:36 AM, Mike Place notifications@github.com
wrote:

@jacksontj https://github.com/jacksontj Doesn't look like the linter
liked that change at all. Could you have a peek when the test finishes?

—
Reply to this email directly or view it on GitHub
#27843 (comment).

[jacksontj]

That should fix the last of the pylint errors.

[jacksontj]

@cachedout There we go, failures should be unrelated-- especially since they aren't even consistent across platform.

[s0undt3ch]

And lint has also been fixed in current develop branch

[cachedout]

Merged! Thanks all.

[DanyC97]

@jacksontj awesome work!!

@cachedout on which version/ build will this PR pop? (if you can share a tip of how i can keep track on which versions commits lands that will be great :) )

[cachedout]

@DanyC97 This will be released in Salt Boron, which definietely wlill not be out before the end of the year I'm afraid. We're working on getting more of our release schedule online and but as a general rule, code in develop will apply to the next major release of Salt and code in an already release branch (like 2015.8) will apply to the next minor release of Salt, which for us will be 2015.8.2.

[DanyC97]

@cachedout yes i'm aware of the develop branch = major and already branch = minor but what is not clear is the cadence of releases.
Although your naming version follows a pattern - year.month in all the discussions, the periodic table elements name is mentioned and that creates a super confusion as is not known if next next 2 elements will fall in next year or not.

Following the project i can see the cadence is ~2 major/ year but what will be nice to have is a timeline - same like OpenStack has - i.e Carbon = 2016.3.x (MArch 16- soft deadline) - 2016.4 (April 16 - hard deadline) or Nitrogen = 2016.9.x - 2016.11.x

[paclat]

That does indeed seem like the same issue. So this should be in 2015.8.2???

[cachedout]

@paclat I don't think we're going to get it into 2015.8.2 since these changes are fairly substantial. It will be in the next major release, however.

[themalkolm]

This is still happening in 2015.8.7. So what is the first release with this patch? Only 2016.3? Was it confirmed that it fixes the issues?