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: support extra modules in sys.path #53167

Merged
merged 3 commits into from May 18, 2020
Merged

Conversation

aplanas
Copy link
Collaborator

@aplanas aplanas commented May 22, 2019

What does this PR do?

The LazyLoader class populate the sys.path array to add the path
that contains the Python module that will be executed. If before
this point sys.path gets extended (like when globally the extmods
utils directory in added in some places, like after a sync) the
code can find all the dependencies.

This patch adds a new parameter to the LazyLoader class,
extra_module_dirs, that will populate locally the sys.path before
executing the the module.

This can be used to guarantee that in utils (and other modules) will be
available to the modules and states in any circumstances and Python
process.

Tests written?

Yes

@aplanas aplanas requested a review from as a code owner May 22, 2019
@aplanas aplanas force-pushed the fix_loader branch 2 times, most recently from edfada4 to d844c9c Compare May 27, 2019
@waynew
Copy link
Contributor

@waynew waynew commented May 31, 2019

There's a test failure happening https://jenkinsci.saltstack.com/job/pr-kitchen-windows2016-py3/job/PR-53167/3/testReport/junit/integration.states.test_file/FileTest/test_file_managed_onchanges/ - I'm not sure if that's related to this PR or not :suspect:

Also looks like there's a merge conflict 😛

Otherwise, I think this is looking good.

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Jun 1, 2019

How this PR relates to #52001? Does it extend or replace it?

@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Jun 1, 2019

@max-arnold I think that is a replacement of this one. Maybe is a good idea to add a commit here to revert the other code.

@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Jun 3, 2019

@max-arnold I added the revert commit here

@waynew I rebased the code. Also I do not see the windows fail related with the code. Lets see if is still present after the rebase.

waynew
waynew approved these changes Jun 3, 2019
Copy link
Contributor

@waynew waynew left a comment

I think this is ready to go, assuming tests pass after the rebase 😛

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Jul 1, 2019

@aplanas Would this fix #52136 and #53666?

@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Jul 1, 2019

@max-arnold Not sure about salt-ssh (#53666), but this fix #52136. I will not be able to test #53666 for some time (out from the office for a while)

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Oct 6, 2019

@aplanas I'm not very familiar with the loader internals, but it seems like other module types also import salt.utils and could benefit from this improvement.

In other words, if there is a bug in utils and someone shipped a fix via saltutil.sync_utils, all module types should be able to pick it up.

Below is a list of functions in loader.py, which you might want to change too (I'm not 100% sure though):

auth
beacons
cache
call
eauth_tokens
executors
log_handlers
matchers
metaproxy
netapi
outputters
pkgdb
pkgfiles
queues
render
returners
serializers
ssh_wrapper
thorium
tops
wheels

What do you think about this idea?

@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Oct 7, 2019

@max-arnold you can be right.

I updated the LazyLoader constructors where there is an explicit call to make __util__ available, but I left the others untouched.

For example, in def auth, I cannot see that is the case:

return LazyLoader(
        _module_dirs(opts, 'auth'),
        opts,
        tag='auth',
        whitelist=whitelist,
        pack={'__salt__': minion_mods(opts)},
    )

I would expect that only __salt__ is available.

Can be that there is a different mechanism where __util__ becomes available in those cases, if so I would need a bigger pull request.

Can we do that later?

@max-arnold
Copy link
Contributor

@max-arnold max-arnold commented Oct 7, 2019

@aplanas

Can we do that later?

We can definitely do this later, and it looks like there will be enough time (a few months?) before the neon release. I'm just a community member and this was a suggestion, not a requirement.

@aplanas aplanas changed the base branch from develop to master Oct 14, 2019
@aplanas aplanas changed the base branch from master to develop Oct 14, 2019
@aplanas aplanas changed the base branch from develop to master Oct 14, 2019
@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Oct 14, 2019

Rebased on top of master.

Note that I dropped the commit that reverted #52001, as this is not in master. I did not backported the original #52001 commit either.

@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Jan 7, 2020

windows errors are related to utils.pycrypto: AttributeError: 'module' object has no attribute 'crypt'

waynew
waynew previously approved these changes Jan 7, 2020
Copy link
Contributor

@waynew waynew left a comment

I think this looks good, but we have some issues going on with our Windows test suite that we need to figure out. As you point out, I don't think they're related to the changes in this PR.

@waynew
Copy link
Contributor

@waynew waynew commented Jan 9, 2020

We had some timeouts happening, something going wonky with the infrastructure(?)

I've restarted the Windows tests - assuming they pass this should be good to merge in 👍

@waynew
Copy link
Contributor

@waynew waynew commented Feb 28, 2020

Hm. I'm a bit confused. I merged the latest master into this PR and yet there's still an issue with the crypt thing? I don't have time to look at this today but I'll stick it on my list.

@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Mar 2, 2020

I rebased, let me check the results.

@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented Mar 2, 2020

Yep. Ignore the lint, is not related with my code:

************* Module salt.returners.django_return
salt/returners/django_return.py:42: [E0611(no-name-in-module), ] No name 'dispatch' in module 'django'


Your code has been rated at 10.00/10

@waynew
Copy link
Contributor

@waynew waynew commented Apr 17, 2020

@aplanas looks like still some lint complaints 😕

I'm not sure if a rebase on master will sort this out, but if you're using pre-commit then it should also run lint checks. I feel like I ran into some weird issues with this particular kind of import (importing from collections, also abc's). I've set a # pylint: disable=no-name-in-module comment around the problematic line(s), but it seems like sometimes I've had issues with pylint ignoring my comments 🤷

If you have issues, please ping me in #salt on IRC or in the community Slack.

aplanas and others added 3 commits May 14, 2020
The LazyLoader class populate the sys.path array to add the path
that contains the Python module that will be executed.  If before
this point sys.path gets extended (like when globally the extmods
utils directory in added in some places, like after a sync) the
code can find all the dependencies.

This patch adds a new parameter to the LazyLoader class,
extra_module_dirs, that will populate locally the sys.path before
executing the the module.

This can be used to guarantee that in utils (and other modules) will be
available to the modules and states in any circumstances and Python
process.
Because we are mangling with importlib, we can find from time to
time an invalidation issue with sys.path_importer_cache, that
requires the removal of FileFinder that remain None for the
extra_module_dirs
@aplanas
Copy link
Collaborator Author

@aplanas aplanas commented May 15, 2020

no failing test, re-review?

dwoz
dwoz approved these changes May 18, 2020
@dwoz dwoz merged commit 6af00cb into saltstack:master May 18, 2020
27 checks passed
Core Open PRs automation moved this from In progress to Done May 18, 2020
@sagetherage sagetherage added this to Done in Sodium May 19, 2020
@aplanas aplanas deleted the fix_loader branch Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Sodium
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants