Fix import logic trying to load incorrect module (#102)

Fix import logic trying to load incorrect module

SUMMARY

There is currently a bug in the import logic that manifests when two
Ansible modules try to load a python module from somewhere in
module_utils. If the python module shares the same name as the second
(or subsequent) Ansible modules, turbo mode will attempt to load the
python module instead of the Ansible module.
This fixes the logic by removing the meta_path modification, which was
the root of the problem. Instead, it keeps the sys.path modification as
before, but attempts to reload any ansible_collection package modules.
We can't reload every module as this would overwrite any shared state in
the module cache, defeating the whole purpose of turbo mode. By
reloading only the package modules we force it to reexamine its
contents, which should now be pointing to the new zip archive since we
changed the loader for the package module.
One caveat to this is that if shared state were being stored in a
package's __init__.py, it would be written over from the package reload.
This can be worked around by conditionally defining any shared state in
__init__.py with something like:
    try:
        state
    except NameError:
        state = {}

ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION


The underlying problem with the current implementation is that it adds a zipimporter to the meta_path for every loaded module in the ansible_collections namespace. A zipimporter only examines that last portion of the module fullname. This can be seen in a simple example. Assuming a zip archive that looks like:
foo.zip
|-- foo
    |-- __init__.py
    |-- a
    |   |-- __init__.py
    |   |-- c.py
    |-- b
        |-- __init__.py
        |-- c.py

Now, create a zipimporter for this, pointing to a subpackage and see that it loads the wrong module:
loader = zipimporter("foo.zip/foo/a")
mod = loader.load_module("foo.b.c")
assert mod.__name__ == "foo.b.c"  # True
assert mod.__file__ == "foo.zip/foo/a/c.py"  # True
By adding zipimporters to the meta_path, when we try to load a module that isn't yet in the module cache, we add finders that will load the wrong module. This is a problem, for example, when you have a collection that looks like:
collection/plugins/modules/a.py
collection/plugins/modules/b.py
collection/plugins/module_utils/b.py

If modules a and b both import b from module_utils, a playbook that first runs module a and then runs module b will fail with an error that it can't find main(), because in the second task it has loaded module_utils/b.py instead of modules/b.py.
It's not enough to just change the __path__ on a package module that has already been imported. This is only examined the first time a package is loaded. When the package is loaded any modules inside __path__ are added as attributes to the package module. Subsequent attempts to access a new module in that package that isn't in the module cache will fail because it is only checking package's current attributes. We have to reload the package modules to be able to load any new modules that may now be in that package namespace. As stated above, this does mean any shared state held on the package (__init__.py) will be overwritten.
I can't see a perfect solution for this, but I think this change is the best one.



Closes: ansible-collections/vmware.vmware_rest#308

Reviewed-by: Gonéri Le Bouder <[email protected]>
Reviewed-by: None <None>

changelogs/fragments/102-fix-incorrect-module-loading.yaml

@@ -0,0 +1,3 @@
+---
+bugfixes:
+  - fix turbo mode loading incorrect module (https://github.com/ansible-collections/cloud.common/pull/102).

plugins/module_utils/turbo/server.py

@@ -127,30 +127,29 @@ async def load(self):
 
             # resettle the loaded modules that were associated
             # with a different Ansiblez.
-            for path, module in tuple(sys.modules.items()):
+            for path, module in sorted(tuple(sys.modules.items())):
                 if path and module and path.startswith("ansible_collections"):
                     try:
                         prefix = sys.modules[path].__loader__.prefix
                     except AttributeError:
                         # Not from a zipimporter loader, skipping
                         continue
-                    py_path = self.ansiblez_path + os.sep + prefix
-                    my_loader = zipimporter(py_path)
-                    sys.meta_path.append(my_loader)
+                    # Reload package modules only, to pick up new modules from
+                    # packages that have been previously loaded.
                     if hasattr(sys.modules[path], "__path__"):
-                        sys.modules[path].__path__ = py_path
-
+                        py_path = self.ansiblez_path + os.sep + prefix
+                        my_loader = zipimporter(py_path)
+                        sys.modules[path].__loader__ = my_loader
+                        try:
+                            importlib.reload(sys.modules[path])
+                        except ModuleNotFoundError:
+                            pass
             # Finally, load the plugin class.
             self.module_class = importlib.import_module(self.module_path)
 
     async def unload(self):
         async with sys_path_lock:
             sys.path = [i for i in sys.path if i != self.ansiblez_path]
-            sys.meta_path = [
-                i
-                for i in sys.meta_path
-                if not (isinstance(i, zipimporter) and i.archive == self.ansiblez_path)
-            ]
 
     def create_profiler(self):
         if self.debug_mode:

plugins/module_utils/turbo_demo.py

@@ -0,0 +1 @@
+# This module is part of the test suite to check the import logic of turbo mode

plugins/modules/turbo_demo.py

@@ -65,6 +65,8 @@ def run_module():
 
 
 def main():
+    from ansible_collections.cloud.common.plugins.module_utils import turbo_demo
+
     run_module()
 
 

plugins/modules/turbo_import.py

@@ -0,0 +1,42 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+
+# Copyright: (C) 2022, Red Hat
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+DOCUMENTATION = r"""
+---
+module: turbo_import
+short_description: A demo module to test import logic for turbo mode
+version_added: "1.0.0"
+description:
+- "This module tests the import logic for turbo mode."
+author:
+- Mike Graves (@gravesm)
+"""
+
+EXAMPLES = r"""
+- name: Run the module
+  cloud.common.turbo_import:
+"""
+
+
+from ansible_collections.cloud.common.plugins.module_utils.turbo.module import (
+    AnsibleTurboModule as AnsibleModule,
+)
+
+
+def run_module():
+    module = AnsibleModule(argument_spec={})
+    module.collection_name = "cloud.common"
+    module.exit_json(changed=False)
+
+
+def main():
+    from ansible_collections.cloud.common.plugins.module_utils import turbo_demo
+
+    run_module()
+
+
+if __name__ == "__main__":
+    main()

tests/integration/targets/turbo_mode/tasks/main.yaml

@@ -1,3 +1,5 @@
+- cloud.common.turbo_import:
+
 - cloud.common.turbo_demo:
   with_sequence: count=10
   register: _result

tests/sanity/ignore-2.10.txt

@@ -66,3 +66,11 @@ tests/unit/plugins/module_utils/turbo/conftest.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/conftest.py metaclass-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py metaclass-boilerplate!skip
+plugins/modules/turbo_import.py compile-2.6!skip
+plugins/modules/turbo_import.py compile-2.7!skip
+plugins/modules/turbo_import.py compile-3.5!skip
+plugins/modules/turbo_import.py future-import-boilerplate!skip
+plugins/modules/turbo_import.py import-2.6!skip
+plugins/modules/turbo_import.py import-2.7!skip
+plugins/modules/turbo_import.py import-3.5!skip
+plugins/modules/turbo_import.py metaclass-boilerplate!skip

tests/sanity/ignore-2.11.txt

@@ -74,3 +74,11 @@ tests/unit/plugins/module_utils/turbo/conftest.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/conftest.py metaclass-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py metaclass-boilerplate!skip
+plugins/modules/turbo_import.py compile-2.6!skip
+plugins/modules/turbo_import.py compile-2.7!skip
+plugins/modules/turbo_import.py compile-3.5!skip
+plugins/modules/turbo_import.py future-import-boilerplate!skip
+plugins/modules/turbo_import.py import-2.6!skip
+plugins/modules/turbo_import.py import-2.7!skip
+plugins/modules/turbo_import.py import-3.5!skip
+plugins/modules/turbo_import.py metaclass-boilerplate!skip

tests/sanity/ignore-2.12.txt

@@ -50,3 +50,11 @@ tests/unit/plugins/module_utils/turbo/conftest.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/conftest.py metaclass-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py metaclass-boilerplate!skip
+plugins/modules/turbo_import.py compile-2.6!skip
+plugins/modules/turbo_import.py compile-2.7!skip
+plugins/modules/turbo_import.py compile-3.5!skip
+plugins/modules/turbo_import.py future-import-boilerplate!skip
+plugins/modules/turbo_import.py import-2.6!skip
+plugins/modules/turbo_import.py import-2.7!skip
+plugins/modules/turbo_import.py import-3.5!skip
+plugins/modules/turbo_import.py metaclass-boilerplate!skip

tests/sanity/ignore-2.13.txt

@@ -39,3 +39,9 @@ tests/unit/plugins/module_utils/turbo/conftest.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/conftest.py metaclass-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py metaclass-boilerplate!skip
+plugins/modules/turbo_import.py compile-2.7!skip
+plugins/modules/turbo_import.py compile-3.5!skip
+plugins/modules/turbo_import.py future-import-boilerplate!skip
+plugins/modules/turbo_import.py import-2.7!skip
+plugins/modules/turbo_import.py import-3.5!skip
+plugins/modules/turbo_import.py metaclass-boilerplate!skip

tests/sanity/ignore-2.9.txt

@@ -66,3 +66,11 @@ tests/unit/plugins/module_utils/turbo/conftest.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/conftest.py metaclass-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py future-import-boilerplate!skip
 tests/unit/plugins/module_utils/turbo/test_module.py metaclass-boilerplate!skip
+plugins/modules/turbo_import.py compile-2.6!skip
+plugins/modules/turbo_import.py compile-2.7!skip
+plugins/modules/turbo_import.py compile-3.5!skip
+plugins/modules/turbo_import.py future-import-boilerplate!skip
+plugins/modules/turbo_import.py import-2.6!skip
+plugins/modules/turbo_import.py import-2.7!skip
+plugins/modules/turbo_import.py import-3.5!skip
+plugins/modules/turbo_import.py metaclass-boilerplate!skip