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

[WIP] Collection content loading #52194

Open
wants to merge 20 commits into
base: devel
from

Conversation

@nitzmahone
Copy link
Member

nitzmahone commented Feb 13, 2019

SUMMARY

Load content and plugins from installed Ansible collections.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

PluginLoader

ADDITIONAL INFORMATION

KNOWN ISSUES:

  • Plugin types not yet tested: become, cache, cliconf, doc_fragments, httpapi, Powershell module_utils, netconf, shell, strategy, terminal, vars
  • plugin_loader's all() method isn't collection-aware (so eg, ansible-doc can't see these yet)
  • error messaging on load failures varies widely (from nonexistent to meh)
  • inventory plugin NAME must currently be FQNS (eg mynamespace.mycollection.invpluginname)
  • Python modules must currently import ansible.module_utils.basic if they import any other module_utils (bug)
  • collection module_utils imports can't currently do non-granular Python module imports with from. eg, from ansible_collections.myns.mycoll.plugins.module_utils import module won't work, but from ansible_collections.myns.mycoll.plugins.module_utils.module import stuff will. (bug)
  • role deduplication does not (yet) take the fully-qualified role name into account, so something like
- roles: [myns.mycoll.role_a, otherns.othercoll.role_a]

will not execute the second role unless allow_duplicates is set (bug)

NON-DOCUMENTATION:
Uses the new installed_content_roots config value to load spec-compliant collection content (defaults to playbook-adjacent, then ~/.ansible/content, /usr/share/ansible/content in that order).

Structure underneath an installed content dir is a Python 3.x implicit namespace package, set up as a subpackage of ansible_collections (the root Python namespace for all this stuff).

Once content is installed, most plugin types can be referenced as usual in Ansible by namespace.collection_name.plugin_name (except module_utils, which require the fully-qualified ansible_collections.namespace.collection_name.plugins.module_utils.module_name syntax). eg:
playbook.yaml

...
tasks:
- mynamespace.mycollection.mymodule:
    args: #...
- mynamespace.othercollection.myaction:
    args: #...
- assert:
    that:
    - 'foo' | mynamespace.mycollection.myfiltername == 'foofiltered'
    - 'foo' is mynamespace.mycollection.mytest
    - lookup('mynamespace.mycollection.mylookup', 'foo') == 'looked up foo'

inventory_config.yaml

plugin: mynamespace.mycollection.myinventory_plugin
...

ansible.cfg

[defaults]
callback_whitelist=mynamespace.mycollection.mycallback

hosts

myhost ansible_connection=mynamespace.mycollection.myconnection_plugin

SYNTHETIC COLLECTIONS
Two "fake" collections are included, ansible.core and ansible.legacy. ansible.core can be used as a prefix to load any plugin shipped inside the Ansible distribution (but not legacy extension dirs like library, action_plugins, etc). ansible.legacy behaves the same as ansible.core, but will also fall back to loading items from the legacy extension dirs (in most cases- collection-hosted roles are special). eg

- ping: # execute the built-in `ping` module, unless it was overridden by something in a `library/` dir
- ansible.core.ping: # execute the built-in `ping` module, ignoring any `library` dirs
- ansible.legacy.ping: # same as `ping`

collections KEYWORD AND UNQUALIFIED MODULES/ACTIONS
By default, any unqualified action/module in a task (meaning no namespace + collection name prefix specified) is assumed to be a built-in module/action or loadable from a legacy library/action_plugins path (ie the way Ansible has always worked). This behavior can be altered by adding the collections keyword at the play/block/task level with an ordered list of collections to be searched for unqualified modules/actions. eg, in the following playbook:

- hosts: localhost
  collections:
  - ansible.core
  - myns.mycoll
  tasks:
  - mymodule:

when the action resolver is trying to find the implementation of mymodule, it will first check the built-in actions and modules, then the myns.mycoll collection (and since we used ansible.core, it will ignore any legacy extension library/ or action_plugins/ dirs outside the myns.mycoll collection).

If ansible.core or ansible.legacy does not appear in the collections list, ansible.legacy will be appended to the end of the list to ease adding collection search behavior to existing content.

ROLES AND COLLECTIONS

Roles do not inherit collections search paths configured by the play. Role content is assumed to have been authored to work with a specific set of plugins, so the calling playbook should not be able to alter those assumptions. Roles in a collection (ie roles under the collection's roles dir) will not load plugins from any legacy plugin dirs inside the role (eg library, action_plugins), nor from playbook-adjacent or configured role paths (by default, but this one can be changed if the legacy behavior is desired). Roles in a collection are configured with a default collection search path of:

- (the role's collection)
- ansible.core

Role collections search behavior can be further altered by using the collections keyword on a block or task, or by setting a list of collections to search in the role's meta/main.yml using the new collections role meta keyword. The current collection will always be prepended to the list specified, and ansible.core will always be appended to the end unless it or ansible.legacy is specified explicitly somewhere else in the list.

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 13, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/template/__init__.py:42:4: ansible-bad-import-from Import MutableMapping from ansible.module_utils.common._collections_compat instead of collections
lib/ansible/template/__init__.py:301:4: unexpected-special-method-signature The special method '__delitem__' expects 1 param(s), 0 was given

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/playbook/__init__.py:68:9: E303 too many blank lines (2)
lib/ansible/template/__init__.py:314:1: E303 too many blank lines (3)

click here for bot help

@alikins

This comment has been minimized.

Copy link
Contributor

alikins commented Feb 14, 2019

Uses the new installed_content_roots config value to load spec-compliant collection content (defaults to playbook adjacent dir "plugin_content", ~/.ansible/content, /usr/share/ansible/contentin that order- playbook-adjacent dir will eventually just becontent`).

Since 'content' was the term used before we the term 'collections' started being used, I would suggest changing the use of 'content' in paths to 'collections'.

For ex, in:

  • ~/.ansible/content -> ~/.ansible/collections
  • <some_playbook_project>/plugin_content -> <some_playbook_project>/collections
  • /usr/share/ansible/content -> /usr/share/ansible/collections

installed_content_roots -> installed_collection_roots (or 'collection_paths' or 'collection_dirs' etc)

That leaves 'content' open for future use, while reinforcing the idea that this support is for 'collections' specifically.

@alikins

This comment was marked as resolved.

Copy link
Contributor

alikins commented Feb 14, 2019

Will role loading be part of this pr or a separate pr?

alikins added a commit to alikins/collection_inspect that referenced this pull request Feb 14, 2019

@alikins

This comment was marked as resolved.

Copy link
Contributor

alikins commented Feb 14, 2019

Testing with the collection at https://github.com/alikins/collection_inspect / https://galaxy-dev.ansible.com/alikins/collection_inspect

My lookup plugin isnt found. Details at https://gist.github.com/alikins/8ab4544601eb5ab4817eb37d0c4e1695

< see gist for full out, truncated here for brevity>

  File "/home/adrian/src/ansible/lib/ansible/plugins/loader.py", line 374, in _find_plugin
    return self._find_fq_plugin(name, suffix)
  File "/home/adrian/src/ansible/lib/ansible/plugins/loader.py", line 319, in _find_fq_plugin
    plugin_content = pkgutil.get_data(package, resource)
  File "/usr/lib64/python3.6/pkgutil.py", line 616, in get_data
    spec = importlib.util.find_spec(package)
  File "/home/adrian/venvs/ansible-py3/lib64/python3.6/importlib/util.py", line 88, in find_spec
    parent = __import__(parent_name, fromlist=['__path__'])
ModuleNotFoundError: No module named 'a'

fatal: [localhost]: FAILED! => {
    "msg": "Unexpected failure during module execution.",
    "stdout": ""
}
@alikins

This comment was marked as resolved.

Copy link
Contributor

alikins commented Feb 14, 2019

and the a and namespace package init.py

ah, missed that.

@alikins

This comment was marked as resolved.

Copy link
Contributor

alikins commented Feb 14, 2019

and the a and namespace package init.py

Is there an example of this setup?

doesnt work:

  • ~/.ansible/content/alikins/collection_inspect/a/init.py
  • /home/adrian/.ansible/content/alikins/collection_inspect/plugins/a/init.py

Also tried moving my collection to ~/.ansible/content/a/alikins (full path ~/.ansible/content/a/alikins/collection_inspect) and get the same error.

With the ~/.ansible/content/a/alikins/collection_inspect setup
PYTHONPATH=~/.ansible/content/ pydoc a.alikins.collection_inspect.plugins.lookup
does work however.

(ansible-py3) [newswoop:F27:playbooks (master % u=)]$ PAGER=cat PYTHONPATH=~/.ansible/content/ pydoc a.alikins.collection_inspect.plugins.lookup
Help on package a.alikins.collection_inspect.plugins.lookup in a.alikins.collection_inspect.plugins:

NAME
    a.alikins.collection_inspect.plugins.lookup

PACKAGE CONTENTS
    collection_inspect

FILE
    /home/adrian/.ansible/content/a/alikins/collection_inspect/plugins/lookup/__init__.py
@nitzmahone

This comment was marked as resolved.

Copy link
Member Author

nitzmahone commented Feb 14, 2019

@alikins ah, I think I see the confusion. The root of the entire "installed" managed content dir should be the a package, with namespace + collection subpackages below. eg:

└── a
    ├── __init__.py
    └── mynamespace
        ├── aws
        │   ├── __init__.py
        │   └── plugins
        │       ├── __init__.py
        │       ├── inventory
        │       │   └── __init__.py
        │       ├── modules
        │       │   └── __init__.py
        │       └── module_utils
        │           └── __init__.py
        ├── __init__.py
        └── mycollection
            ├── __init__.py
            └── plugins
                ├── action
                │   └── __init__.py
                ├── callback
                │   └── __init__.py
                ├── connection
                │   └── __init__.py
                ├── filter
                │   └── __init__.py
                ├── __init__.py
                ├── inventory
                │   └── __init__.py
                ├── lookup
                │   └── __init__.py
                ├── modules
                │   └── __init__.py
                ├── module_utils
                │   └── __init__.py
                └── test
                    └── __init__.py

So the github root of a collection would just be the what's under the "mycollection" level, leaving mazer et al responsible for managing the structure above it. Make sense?

... and if you want to split a namespace between system/user/content-adjacent, the init under the a and namespace packages currently needs to include the extend_paths boilerplate above. I've already worked around that with dynamic packages, just haven't integrated the change into this tree yet.

I'm all for changing the top-level directory name to collections from content if @thaumos/@tima agree.

And yes, my plan is to include the role-loading in this branch as well once the plugin stuff gets to an MVPish state.

@alikins

This comment has been minimized.

Copy link
Contributor

alikins commented Feb 19, 2019

@nitzmahone

The root of the entire "installed" managed content dir should be the a package, with namespace + collection subpackages below.

I'm all for changing the top-level directory name to collections from content if @thaumos/@tima agree.

Maybe we can combine those two needs and rename the 'content' dir (ie, ~/.ansible/content) to something like 'ansible_collections'[1], and let 'ansible_collections' also be the python namespace root instead of the current 'a'. ?

[1] or 'ansiblecollections' or 'galaxy_collections' etc

@tima

This comment was marked as resolved.

Copy link
Contributor

tima commented Feb 19, 2019

@alikins what is the future use of content you have in mind? What is gained by reinforcing collections to the end user?

@samdoran samdoran removed the needs_triage label Feb 19, 2019

@thaumos

This comment has been minimized.

Copy link
Contributor

thaumos commented Feb 21, 2019

Yeah, let's change it to collections @nitzmahone. It is simple enough. No need to bikeshed on this.

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 27, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/plugins/loader.py:509:8: bare-except No exception type(s) specified
lib/ansible/template/__init__.py:42:4: ansible-bad-import-from Import MutableMapping from ansible.module_utils.common._collections_compat instead of collections
lib/ansible/template/__init__.py:303:4: unexpected-special-method-signature The special method '__delitem__' expects 1 param(s), 0 was given
lib/ansible/utils/collection_loader.py:163:21: bad-whitespace No space allowed before bracket                 exec (source, newmod.__dict__)                      ^

The test ansible-test sanity --test no-underscore-variable [explain] failed with 1 error:

lib/ansible/utils/collection_loader.py:195:28: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test pep8 [explain] failed with 8 errors:

lib/ansible/plugins/loader.py:310:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:346:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:509:9: E722 do not use bare 'except'
lib/ansible/template/__init__.py:316:1: E303 too many blank lines (3)
lib/ansible/utils/collection_loader.py:27:1: E302 expected 2 blank lines, found 1
lib/ansible/utils/collection_loader.py:50:5: E303 too many blank lines (2)
lib/ansible/utils/collection_loader.py:163:21: E211 whitespace before '('
lib/ansible/utils/collection_loader.py:186:5: E301 expected 1 blank line, found 0

click here for bot help

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 27, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/plugins/loader.py:510:8: bare-except No exception type(s) specified
lib/ansible/template/__init__.py:42:4: ansible-bad-import-from Import MutableMapping from ansible.module_utils.common._collections_compat instead of collections
lib/ansible/template/__init__.py:303:4: unexpected-special-method-signature The special method '__delitem__' expects 1 param(s), 0 was given
lib/ansible/utils/collection_loader.py:163:21: bad-whitespace No space allowed before bracket                 exec (source, newmod.__dict__)                      ^

The test ansible-test sanity --test no-underscore-variable [explain] failed with 1 error:

lib/ansible/utils/collection_loader.py:195:28: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test pep8 [explain] failed with 8 errors:

lib/ansible/plugins/loader.py:310:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:347:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:510:9: E722 do not use bare 'except'
lib/ansible/template/__init__.py:316:1: E303 too many blank lines (3)
lib/ansible/utils/collection_loader.py:27:1: E302 expected 2 blank lines, found 1
lib/ansible/utils/collection_loader.py:50:5: E303 too many blank lines (2)
lib/ansible/utils/collection_loader.py:163:21: E211 whitespace before '('
lib/ansible/utils/collection_loader.py:186:5: E301 expected 1 blank line, found 0

click here for bot help

@ansibot ansibot added the needs_rebase label Mar 5, 2019

@nitzmahone nitzmahone force-pushed the nitzmahone:collection_content_load branch from e8de76d to 4aba547 Mar 5, 2019

@ansibot ansibot removed the needs_rebase label Mar 5, 2019

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Mar 5, 2019

The test ansible-test sanity --test pylint [explain] failed with 9 errors:

lib/ansible/playbook/role/definition.py:48:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/playbook/role/definition.py:166:37: unneeded-not Consider changing "not 'ansible.legacy' in self._collection_list" to "'ansible.legacy' not in self._collection_list"
lib/ansible/playbook/role/include.py:46:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/playbook/role/include.py:50:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/plugins/loader.py:510:8: bare-except No exception type(s) specified
lib/ansible/template/__init__.py:42:4: ansible-bad-import-from Import MutableMapping from ansible.module_utils.common._collections_compat instead of collections
lib/ansible/template/__init__.py:303:4: unexpected-special-method-signature The special method '__delitem__' expects 1 param(s), 0 was given
lib/ansible/utils/collection_loader.py:167:21: bad-whitespace No space allowed before bracket                 exec (source, newmod.__dict__)                      ^
lib/ansible/utils/collection_loader.py:257:0: dangerous-default-value Dangerous default value [] as argument

The test ansible-test sanity --test no-underscore-variable [explain] failed with 1 error:

lib/ansible/utils/collection_loader.py:199:28: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test pep8 [explain] failed with 12 errors:

lib/ansible/playbook/helpers.py:388:161: E501 line too long (170 > 160 characters)
lib/ansible/playbook/role/definition.py:156:9: E303 too many blank lines (2)
lib/ansible/playbook/role/definition.py:166:38: E713 test for membership should be 'not in'
lib/ansible/playbook/role/include.py:47:161: E501 line too long (162 > 160 characters)
lib/ansible/plugins/loader.py:310:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:347:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:510:9: E722 do not use bare 'except'
lib/ansible/template/__init__.py:316:1: E303 too many blank lines (3)
lib/ansible/utils/collection_loader.py:31:1: E302 expected 2 blank lines, found 1
lib/ansible/utils/collection_loader.py:54:5: E303 too many blank lines (2)
lib/ansible/utils/collection_loader.py:167:21: E211 whitespace before '('
lib/ansible/utils/collection_loader.py:190:5: E301 expected 1 blank line, found 0

click here for bot help

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Mar 6, 2019

The test ansible-test sanity --test pylint [explain] failed with 9 errors:

lib/ansible/playbook/role/definition.py:48:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/playbook/role/definition.py:166:37: unneeded-not Consider changing "not 'ansible.legacy' in self._collection_list" to "'ansible.legacy' not in self._collection_list"
lib/ansible/playbook/role/include.py:46:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/playbook/role/include.py:50:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/plugins/loader.py:510:8: bare-except No exception type(s) specified
lib/ansible/template/__init__.py:42:4: ansible-bad-import-from Import MutableMapping from ansible.module_utils.common._collections_compat instead of collections
lib/ansible/template/__init__.py:328:4: unexpected-special-method-signature The special method '__delitem__' expects 1 param(s), 0 was given
lib/ansible/utils/collection_loader.py:176:21: bad-whitespace No space allowed before bracket                 exec (source, newmod.__dict__)                      ^
lib/ansible/utils/collection_loader.py:266:0: dangerous-default-value Dangerous default value [] as argument

The test ansible-test sanity --test no-underscore-variable [explain] failed with 2 errors:

lib/ansible/template/__init__.py:307:13: use `dummy` instead of `_` for a variable name
lib/ansible/utils/collection_loader.py:208:28: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test pep8 [explain] failed with 13 errors:

lib/ansible/playbook/helpers.py:388:161: E501 line too long (170 > 160 characters)
lib/ansible/playbook/role/definition.py:156:9: E303 too many blank lines (2)
lib/ansible/playbook/role/definition.py:166:38: E713 test for membership should be 'not in'
lib/ansible/playbook/role/include.py:47:161: E501 line too long (162 > 160 characters)
lib/ansible/plugins/loader.py:310:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:347:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:510:9: E722 do not use bare 'except'
lib/ansible/template/__init__.py:307:95: E226 missing whitespace around arithmetic operator
lib/ansible/template/__init__.py:341:1: E303 too many blank lines (3)
lib/ansible/utils/collection_loader.py:31:1: E302 expected 2 blank lines, found 1
lib/ansible/utils/collection_loader.py:63:5: E303 too many blank lines (2)
lib/ansible/utils/collection_loader.py:176:21: E211 whitespace before '('
lib/ansible/utils/collection_loader.py:199:5: E301 expected 1 blank line, found 0

click here for bot help

@alikins
Copy link
Contributor

alikins left a comment

misc notes

try:
# FIXME: this won't support full package import, but do we actually want it to?
module_info = pkgutil.get_data(package_name, resource_name)
except FileNotFoundError:

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

FileNotFoundError is py3 only

This comment has been minimized.

# FIXME: this won't support full package import, but do we actually want it to?
module_info = pkgutil.get_data(package_name, resource_name)
except FileNotFoundError:
# FIXME: implement package fallback code

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

Related idea: Adding a 'ansible_collection' style virtual namespace for python modules that may only exist on the remote end, something like 'ansible_remote'. The 'ansible_remote' py modules could be looked up in a remote cache and cache misses would fail in a way that indicates 'ok, send the anziballZ again but this time with everything'. Or the py module loader could look in some known location for pre distributed py modules[1]. Or both.

Would also take advantage of using collections and collection loader as a way to start separating the ideas of 'module_utils is stuff for ansible modules' and 'module_utils is stuff used by controller and remote'. It could still
be the same code, that lives in the same place, but would provide a place to split it in the future.

[1] somewhere between 'install an agent first' and 'lets send all the code everytime'

This comment has been minimized.

@nitzmahone

nitzmahone Mar 20, 2019

Author Member

I've actually got some similar things brewing, but without the need for them to be called out differently.

module_info = pkgutil.get_data(package_name, resource_name)
except FileNotFoundError:
# FIXME: implement package fallback code
raise AnsibleError('unable to load collection-hosted module_util {0}.{1}'.format(package_name, resource_name))

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

Would it be worthwhile including the filename/errno info from the FileNotFoundError in this error message? It would point to a tmp path most likely, but may make debug/troubleshooting easier.

This comment has been minimized.

@alikins

alikins Mar 20, 2019

Contributor
Suggested change
raise AnsibleError('unable to load collection-hosted module_util {0}.{1}'.format(package_name, resource_name))
raise AnsibleError('Unable to load collection-hosted module_util {0}.{1}, {2}'.format(package_name, resource_name, str(exc)))
def find_module(self, fullname, path=None):
# this loader is only concerned with items under the Ansible Collections namespace hierarchy, ignore others
if fullname.startswith('ansible_collections.') or fullname == 'ansible_collections':
return self

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

Could this do additional checking here, for ex, some of the path based checking currently in load_modules? Do we have enough info to verify that the paths where that module could live exist?

If we can detect that earlier, I think it may improve some of the error handling for import errors. And potentially avoid some cases of needing to load modules that are partial matches.

# TODO: tighten this up to subset Python identifier requirements
_collection_role_name_re = re.compile(r'^(\w+)\.(\w+)\.(\w+)$')

# FIXME: exception handling/error logging

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

Seems like there are (at least) two levels to loading an ansible plugin:

  • a python module loader step
  • finding/creating an instance of a plugin class

And since this code finds/loads code, but doesn't load/deserialize any of the collections content or metadata (like it's galaxy.yml, etc)

I think of collection_loader.AnsibleCollectionLoader more as of CollectionPythonModuleLoader. Or maybe CollectionPythonModuleImporter (python docs and PEP302 uses loader/importer interchangeably so no help there).

And ansible.plugins.loader.PluginLoader as more like a AnsiblePluginInstanceLoader
since it is finding a matching class from a loaded python module, instantiating it [1], and returning it.

Those are just names, but maybe it would make the distinction more clear in the future?
Previously, PluginLoader intertwined a module importer and the AnsiblePlugin instance creator, but
since this pr starts pulling the ideas apart maybe it is a good time to make the names more specific.

[1] excluding 'class_only'

def _collection_paths(self):
return self._playbook_paths + self._configured_paths

def set_playbook_paths(self, playbook_paths):

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

Is there any way to pass the playbook_paths to init and avoid changing this state after construction?

That would reduce some of the side effects and make AnsibleCollectionLoader slightly more immutable-ish.

In combo with set_collection_playbook_paths() and ansible.plugins.loader._configure_collection_loader() it seems like this could make the state of a AnsibleCollectionLoader instance a little unpredictable as the ansible process starts up.

# FIXME: exception handling/error logging
class AnsibleCollectionLoader(object):
def __init__(self):
self._configured_paths = C.config.get_config_value('INSTALLED_CONTENT_ROOTS')

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

Maybe pass the playbook paths and _configured_paths in to init as part of something like a AnsibleCollectionLoaderContext or something immutable?

# TODO: All of the following is initialization code It should be moved inside of an initialization
# function which is called at some point early in the ansible and ansible-playbook CLI startup.

_PLUGIN_FILTERS = _load_plugin_filter()

_configure_collection_loader()

This comment has been minimized.

@alikins

alikins Mar 7, 2019

Contributor

Instead of doing this at module load time, it may be more predictable to just call it early in the app entry points (possibly main or somewhere in cli/ ?). The ordering of when/how the ansible.* modules gets loaded can easily change and change collection loading behavior. Those sort of bugs can be very hard to debug.

Folks importing ansible code in other apps would need to add the call to _configure_collection_loader() to their startup if they want the collection loader.

This comment has been minimized.

@alikins

alikins Mar 8, 2019

Contributor

I think the places it would need to be called explicitly are:

  • cli app startup (bin/ansible? ansible.cli.*?)
  • ansiballZ startup
  • worker process startup? (not require, but possibly useful)

For worker process, I dont think they have to resetup the collection loader,
but if they did create a new AnsibleCollectionLoader (and replace the one in sys.metapath
for the process if needed), that would help isolate workers from potentially altering
the controllers collection loader.

For ansiballz, with an explicit _configure_collection_loader() and AnsibleCollectionLoader creation,
it could use a different config and potentially offer a place to add/tweak python module loading on the remote
side. However, the interaction between PluginLoader, module_common, and ansiballZ imports is already
very complicated, unless using CollectionLoader could simplify that it's probably not worth changing.

@alikins
Copy link
Contributor

alikins left a comment

more misc notes

self._configured_paths = []

# expand any placeholders in configured paths
self._configured_paths = [os.path.expanduser(p) for p in self._configured_paths]

This comment has been minimized.

@alikins

alikins Mar 8, 2019

Contributor

Wondering if it would make sense to change AnsibleCollectionLoader so that it only has to know about one content root (ContentRootLoader?). And adding a chain of responsibility style layer above (I'll call it ContentRootLoaderChain for now). ContentRootLoaderChain would have-a ordered list of ContentRootLoader's (one for each content root), and ContentRootLoaderChain would be responsible for searching until it finds what it needs.

In addition to the ContentRootLoader instance for each installed_content_root, the ContentRootLoaderChain
could add additional links to check. For example, maybe a cache layer. Could also have a ContentRootLoader instance for each of 'ansible.legacy' and 'ansible.core' that is added to the end of the chain.

Logic for scoping/priority/precedence could potentially be mostly located in ContentRootLoaderChain.

I think AnsibleContentLoader / ContentRootLoader would be simpler if they only had to know about one content root.

This comment has been minimized.

@alikins

alikins Mar 8, 2019

Contributor

Oh, and the playbook adjacent collection loader logic could be extracted to it's own class (PlaybookContentRootLoader?) and added to the ContentRootLoaderChain


synpkg_def = _SYNTHETIC_PACKAGES.get(fullname)

# FIXME: collapse as much of this back to on-demand as possible (maybe stub packages that get replaced when actually loaded?)

This comment has been minimized.

@alikins

alikins Mar 8, 2019

Contributor

Might be useful to extract each of the module constructing clauses below into their own methods.
ie, one for 'map', 'flatmap', and 'pkg_only'.

Among other things, that would make it easier to unit test each of those.

Suppose it could have a types.ModuleType subclass for these cases, but that may be overboard.

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Mar 8, 2019

The test ansible-test sanity --test pylint [explain] failed with 9 errors:

lib/ansible/playbook/role/definition.py:48:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/playbook/role/definition.py:166:37: unneeded-not Consider changing "not 'ansible.legacy' in self._collection_list" to "'ansible.legacy' not in self._collection_list"
lib/ansible/playbook/role/include.py:46:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/playbook/role/include.py:50:4: dangerous-default-value Dangerous default value [] as argument
lib/ansible/plugins/loader.py:510:8: bare-except No exception type(s) specified
lib/ansible/template/__init__.py:42:4: ansible-bad-import-from Import MutableMapping from ansible.module_utils.common._collections_compat instead of collections
lib/ansible/template/__init__.py:328:4: unexpected-special-method-signature The special method '__delitem__' expects 1 param(s), 0 was given
lib/ansible/utils/collection_loader.py:176:21: bad-whitespace No space allowed before bracket                 exec (source, newmod.__dict__)                      ^
lib/ansible/utils/collection_loader.py:266:0: dangerous-default-value Dangerous default value [] as argument

The test ansible-test sanity --test no-underscore-variable [explain] failed with 2 errors:

lib/ansible/template/__init__.py:307:13: use `dummy` instead of `_` for a variable name
lib/ansible/utils/collection_loader.py:208:28: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test pep8 [explain] failed with 13 errors:

lib/ansible/playbook/helpers.py:388:161: E501 line too long (170 > 160 characters)
lib/ansible/playbook/role/definition.py:156:9: E303 too many blank lines (2)
lib/ansible/playbook/role/definition.py:166:38: E713 test for membership should be 'not in'
lib/ansible/playbook/role/include.py:47:161: E501 line too long (162 > 160 characters)
lib/ansible/plugins/loader.py:310:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:347:9: E265 block comment should start with '# '
lib/ansible/plugins/loader.py:510:9: E722 do not use bare 'except'
lib/ansible/template/__init__.py:307:95: E226 missing whitespace around arithmetic operator
lib/ansible/template/__init__.py:341:1: E303 too many blank lines (3)
lib/ansible/utils/collection_loader.py:31:1: E302 expected 2 blank lines, found 1
lib/ansible/utils/collection_loader.py:63:5: E303 too many blank lines (2)
lib/ansible/utils/collection_loader.py:176:21: E211 whitespace before '('
lib/ansible/utils/collection_loader.py:199:5: E301 expected 1 blank line, found 0

click here for bot help

@nitzmahone nitzmahone force-pushed the nitzmahone:collection_content_load branch from cc74170 to 920a6a6 Mar 20, 2019

@alikins

This comment has been minimized.

Copy link
Contributor

alikins commented Mar 20, 2019

Seeing an error with a collection local module where the created anziballZ doesn't have
any of the 'ansible_collections' paths included.

I think this is the same cause as #52194 (comment)

alikins@e125713 has a change which fixes this for me

I see this by running one of the playbooks from my collection_inspect collection.

ansible-playbook -vvv playbooks/test_collection_inspect.yml

With the collect and test playbook from https://github.com/alikins/collection_inspect/

The task:

    - name: test module get_collection_inspect
      alikins.collection_inspect.get_collection_inspect:
        args:
      register: get_collection_inspect_results

Causes the remote module exec to fail with this output and stack trace:

TASK [test module get_collection_inspect] **************************************************************************************************************************************************************************
task path: /home/adrian/src/testcollections/collection_inspect/playbooks/test_collection_inspect.yml:41
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: adrian
<127.0.0.1> EXEC /bin/sh -c 'echo ~adrian && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260 `" && echo ansible-tmp-1553115636.641968-110865914951260="` echo /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260 `" ) && sleep 0'
Using module file /home/adrian/.ansible/content/ansible_collections/alikins/collection_inspect/plugins/modules/get_collection_inspect.py
<127.0.0.1> PUT /home/adrian/.ansible/tmp/ansible-local-25824nohtrpar/tmpc71r1spn TO /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/AnsiballZ_get_collection_inspect.py
<127.0.0.1> PUT /home/adrian/.ansible/tmp/ansible-local-25824nohtrpar/tmptlq6lt2s TO /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/args
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/ /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/AnsiballZ_get_collection_inspect.py /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/args && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/home/adrian/venvs/ansible-py3/bin/python /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/AnsiballZ_get_collection_inspect.py /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/args && sleep 0'
fatal: [localhost]: FAILED! => {
    "changed": false,
    "rc": 1
}

MSG:

MODULE FAILURE
See stdout/stderr for the exact error


MODULE_STDERR:

2019-03-20 17:00:36,723 6 D __main__ 25891 <module>:20 - sys.path: ['/home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260',
 '/home/adrian/src/ansible/lib',
 '/home/adrian/venvs/ansible-py3/lib64/python36.zip',
 '/home/adrian/venvs/ansible-py3/lib64/python3.6',
 '/home/adrian/venvs/ansible-py3/lib64/python3.6/lib-dynload',
 '/usr/lib64/python3.6',
 '/usr/lib/python3.6',
 '/home/adrian/venvs/ansible-py3/lib/python3.6/site-packages',
 '/home/adrian/src/color_debug',
 '/home/adrian/venvs/ansible-py3/lib/python3.6/site-packages/alog-0.9.13-py3.6.egg',
 '/home/adrian/src/mazer',
 '/home/adrian/venvs/ansible-py3/lib/python3.6/site-packages/yamlloader-0.5.2-py3.6.egg',
 '/home/adrian/venvs/ansible-py3/lib/python3.6/site-packages/alogging-0.2.1-py3.6.egg',
 '/home/adrian/venvs/ansible-py3/lib/python3.6/site-packages']
2019-03-20 17:00:36,723 7 D __main__ 25891 <module>:21 - sys.meta_path: [<class '_frozen_importlib.BuiltinImporter'>,
 <class '_frozen_importlib.FrozenImporter'>,
 <class '_frozen_importlib_external.PathFinder'>]
Traceback (most recent call last):
  File "/home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/AnsiballZ_get_collection_inspect.py", line 24, in <module>
    from ansible_collections.alikins.collection_inspect.plugins.module_utils.collection_inspect import get_dunders
ModuleNotFoundError: No module named 'ansible_collections'

The ansiballZ created at /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/ does not have any of the module_utils from ansible_collection included.

(ansible-py3) [newswoop:F27:collection_inspect (master % u=)]$ ls -la /home/adrian/.ansible/tmp/ansible-tmp-1553115636.641968-110865914951260/
total 288
drwx------.    2 adrian adrian   4096 Mar 20 17:00 .
drwx------. 2154 adrian adrian 278528 Mar 20 17:00 ..
-rwxrw-r--.    1 adrian adrian   1567 Mar 20 17:00 AnsiballZ_get_collection_inspect.py
-rwxrw-r--.    1 adrian adrian    616 Mar 20 17:00 args
@nitzmahone

This comment has been minimized.

Copy link
Member Author

nitzmahone commented Mar 20, 2019

@alikins If it's a "medium-style" module that doesn't include some form of import ansible.module_utils.basic, I haven't fixed that bug yet (tis the only time I've seen that error). Adding that import (plus an explicit sys.exit() or use of AnsibleModule and exit_json) fixed it for me.

@alikins
Copy link
Contributor

alikins left a comment

py3-ism, etc

module_info = pkgutil.get_data(package_name, resource_name)
except FileNotFoundError:
# FIXME: implement package fallback code
raise AnsibleError('unable to load collection-hosted module_util {0}.{1}'.format(package_name, resource_name))

This comment has been minimized.

@alikins

alikins Mar 20, 2019

Contributor
Suggested change
raise AnsibleError('unable to load collection-hosted module_util {0}.{1}'.format(package_name, resource_name))
raise AnsibleError('Unable to load collection-hosted module_util {0}.{1}, {2}'.format(package_name, resource_name, str(exc)))
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 20, 2019

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/collections/aliases:0:0: missing alias `shippable/posix/group[1-4]` or `unsupported`

The test ansible-test sanity --test no-underscore-variable [explain] failed with 2 errors:

lib/ansible/template/__init__.py:302:13: use `dummy` instead of `_` for a variable name
lib/ansible/utils/collection_loader.py:209:28: use `dummy` instead of `_` for a variable name

The test ansible-test sanity --test shebang [explain] failed with 10 errors:

test/integration/targets/collections/ansible_collections/testns/content_adj/plugins/modules/contentadjmodule.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_sys/ansible_collections/testns/coll_in_sys/plugins/modules/systestmodule.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_sys/ansible_collections/testns/testcoll/plugins/modules/maskedmodule.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_sys/ansible_collections/testns/testcoll/plugins/modules/testmodule.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/ping.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/testmodule.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_base_mu_granular_nested_import.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_leaf_mu_flat_import.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_leaf_mu_granular_import.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'
test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/modules/uses_leaf_mu_module_import_from.py:1:1: unexpected non-module shebang: b'#!/usr/bin/python'

click here for bot help

@ansibot ansibot added the python3 label Mar 20, 2019

@alikins

This comment has been minimized.

Copy link
Contributor

alikins commented Mar 20, 2019

@alikins If it's a "medium-style" module that doesn't include some form of import ansible.module_utils.basic, I haven't fixed that bug yet (tis the only time I've seen that error). Adding that import (plus an explicit sys.exit() or use of AnsibleModule and exit_json) fixed it for me.

The test module has a sys.exit(), so that doesn't seem to be enough.

If it is valid for a python module to not use 'ansible.module_utils.basic' is valid for a python module,
(it is afaik...) the fix below works.

diff --git lib/ansible/executor/module_common.py lib/ansible/executor/module_common.py
index 4294c71044..19396b7735 100644
--- lib/ansible/executor/module_common.py
+++ lib/ansible/executor/module_common.py
@@ -738,6 +738,10 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas
     elif b'from ansible.module_utils.' in b_module_data:
         module_style = 'new'
         module_substyle = 'python'
+    elif b'from ansible_collections.' in b_module_data:
+        # For cases where we use collection local module_utils only
+        module_style = 'new'
+        module_substyle = 'python'
     elif REPLACER_WINDOWS in b_module_data:
         module_style = 'new'
         module_substyle = 'powershell'

The elif clause could go a step further and check for 'module_utils' in the same line to match more specifically. Or a regex to catch the potentially multiline import statement, etc.

+    elif b'from ansible_collections.' in b_module_data and b'module_utils' in b_module_data
rename ansible.core to ansible.builtin
* and various sanity fixes
@alikins
Copy link
Contributor

alikins left a comment

Should remove the base except on find_plugin for collection cases for pre releases
at least, to try to surface any bugs that could raise exceptions.

May need some fiddling to avoid spamming about exceptions from the old loader since I
know that can happen for expected cases. Would be great to be able to get rid of that too though.

try:
return self.find_plugin(name, collection_list=collection_list) is not None
# TODO: limit this to only plugin load/resolution errors
except Exception:

This comment has been minimized.

@alikins

alikins Mar 20, 2019

Contributor

At least for alpha/beta releases and for cases where collections are involved, this bare exception should probably at least warn about failures. Otherwise this could silently ignore a lot of problems.

Maybe something like this (untested...)

        try:
            return self.find_plugin(name, collection_list=collection_list) is not None
        except Exception as ex:
            display.warn('find_plugin(name=%s, collection_list=%s) raised %s' % (name, collection_list, ex))

In my dev branches with logging setup, the following works a lot better for me, but less useful with stock ansible:

    def has_plugin(self, name, collection_list=None):
        ''' Checks if a plugin named name exists '''

        try:
            return self.find_plugin(name, collection_list=collection_list) is not None
        except Exception as ex:
            self.log.exception(ex)
            self.log.debug('find_plugin(name=%s, collection_list=%s) raised %s', name, collection_list, ex)
            pass
export ANSIBLE_GATHERING=explicit
export ANSIBLE_GATHER_SUBSET=minimal

ansible-playbook -i ../../inventory -i ./a.statichost.yml -v play.yml

This comment has been minimized.

@alikins

alikins Mar 20, 2019

Contributor

I think it would be useful to add another invocation of ansible-playbook to the tests, but with config/inventory setup so that nothing causes early plugin loading (ie, stuff like specifying a callback in the config, or the inventory plugin used in test/integration/targets/collections/a.statichost.yml).

The goal would be to catch any issues that occur if a collection loader hasn't been created early in the process. Based on some of the unexpected and hard to debug behavior seen in earlier versions, some tests for the 'no early plugin or collection loader' cases would be useful.

@alikins

This comment has been minimized.

Copy link
Contributor

alikins commented Mar 20, 2019

I'm sure it's on the TODO list already, but test cases for the rest of the plugin types (vars, netconf, cliconf, strategy, etc) would be good to include at some point.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 20, 2019

@nitzmahone This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@jctanner

This comment has been minimized.

Copy link
Member

jctanner commented Mar 21, 2019

bot_skip

nitzmahone added some commits Mar 21, 2019

less grabby error handler on has_plugin
* probably need to replace with a or harden callers
fix win_ping test
* disable module test with explicit file extension; might be able to support in some scenarios, but can't see any other tests that verify that behavior...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.