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

Loader Contexts #58853

Merged
merged 12 commits into from
Dec 6, 2020
Merged

Loader Contexts #58853

merged 12 commits into from
Dec 6, 2020

Conversation

dwoz
Copy link
Contributor

@dwoz dwoz commented Oct 30, 2020

What does this PR do?

Adds loader contexts so that multiple loaders can't stomp on each other.

What issues does this PR fix or reference?

Prior to these changes salt's 'magic attributes' (__utils__, __salt__, __opts__, ect..). Would be added as global attributes to a s loaded module's namespace. This had the un-intended side effect of allowing a method call from one loader to change something in another loader. This can be particularly problematic for __context__ and __opts__. This can also cause problems for loaders added with unique configs.

There is also an addition of a new pack_self attribute to Salt's LazyLoader class. This allows the loader to expose itself as a magic dunder without creating circular references on in the loader's pack dictionary. Those circular references are the cause of some memory leaks. The pack_self argument should be used to avoid memory leaks.

Previous Behavior

Due to the global nature of Salt's dunder methods, calls from one loader could interfere with other loaders.

New Behavior

Loaders use contextvars to provide a context that is specific the the loader that loads the function being called.

Merge requirements satisfied?

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

  • Docs - More needed
  • Changelog - Still Needed
  • Tests written/updated - More needed

@dwoz dwoz requested a review from a team as a code owner October 30, 2020 22:08
@dwoz dwoz requested review from cmcmarrow and removed request for a team October 30, 2020 22:08
@dwoz dwoz force-pushed the loader_fixes branch 11 times, most recently from ef25e7b to 91493e6 Compare November 1, 2020 21:45
Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

The new requirement should be added to requirements/static/pkg since it will become a hard dependency, in fact, it should actually be added to requirements/base.txt since its a hard dependency and applies to all platforms.

Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

There are still a lot of print debug statements, but you should know that already.
Left some questions.

salt/loader.py Outdated Show resolved Hide resolved
tests/unit/modules/test_boto3_elasticsearch.py Outdated Show resolved Hide resolved
tests/unit/modules/test_boto_secgroup.py Outdated Show resolved Hide resolved
@dwoz dwoz force-pushed the loader_fixes branch 5 times, most recently from 76e3a76 to ee93d39 Compare November 5, 2020 23:08
s0undt3ch
s0undt3ch previously approved these changes Nov 8, 2020
garethgreenaway
garethgreenaway previously approved these changes Dec 3, 2020
Copy link
Contributor

@oeuftete oeuftete left a comment

Choose a reason for hiding this comment

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

Approving based on others', and that I know this fixes #54045.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants