From fcab7de2ac28cc4ee8a309e05b661a906c045efd Mon Sep 17 00:00:00 2001 From: Mike Graves Date: Thu, 27 Jan 2022 09:42:47 -0500 Subject: [PATCH 1/4] Fix import logic trying to load incorrect module 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 = {} --- plugins/module_utils/turbo/server.py | 19 ++++----- plugins/module_utils/turbo_demo.py | 1 + plugins/modules/turbo_demo.py | 1 + plugins/modules/turbo_import.py | 41 +++++++++++++++++++ .../targets/turbo_mode/tasks/main.yaml | 2 + tests/sanity/ignore-2.10.txt | 8 ++++ tests/sanity/ignore-2.11.txt | 8 ++++ tests/sanity/ignore-2.12.txt | 8 ++++ tests/sanity/ignore-2.13.txt | 6 +++ tests/sanity/ignore-2.9.txt | 8 ++++ 10 files changed, 92 insertions(+), 10 deletions(-) create mode 100644 plugins/module_utils/turbo_demo.py create mode 100644 plugins/modules/turbo_import.py diff --git a/plugins/module_utils/turbo/server.py b/plugins/module_utils/turbo/server.py index c2c8c4a..770b599 100644 --- a/plugins/module_utils/turbo/server.py +++ b/plugins/module_utils/turbo/server.py @@ -134,23 +134,22 @@ async def load(self): 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: diff --git a/plugins/module_utils/turbo_demo.py b/plugins/module_utils/turbo_demo.py new file mode 100644 index 0000000..1a14f07 --- /dev/null +++ b/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 diff --git a/plugins/modules/turbo_demo.py b/plugins/modules/turbo_demo.py index 14b1dcb..3d8aa33 100644 --- a/plugins/modules/turbo_demo.py +++ b/plugins/modules/turbo_demo.py @@ -65,6 +65,7 @@ def run_module(): def main(): + from ansible_collections.cloud.common.plugins.module_utils import turbo_demo run_module() diff --git a/plugins/modules/turbo_import.py b/plugins/modules/turbo_import.py new file mode 100644 index 0000000..adbc322 --- /dev/null +++ b/plugins/modules/turbo_import.py @@ -0,0 +1,41 @@ +#!/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() diff --git a/tests/integration/targets/turbo_mode/tasks/main.yaml b/tests/integration/targets/turbo_mode/tasks/main.yaml index d5c00c3..5be80f6 100644 --- a/tests/integration/targets/turbo_mode/tasks/main.yaml +++ b/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 diff --git a/tests/sanity/ignore-2.10.txt b/tests/sanity/ignore-2.10.txt index b5126a0..4f8d3f1 100644 --- a/tests/sanity/ignore-2.10.txt +++ b/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 diff --git a/tests/sanity/ignore-2.11.txt b/tests/sanity/ignore-2.11.txt index b882739..3afe6f8 100644 --- a/tests/sanity/ignore-2.11.txt +++ b/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 diff --git a/tests/sanity/ignore-2.12.txt b/tests/sanity/ignore-2.12.txt index 59133ed..cca703b 100644 --- a/tests/sanity/ignore-2.12.txt +++ b/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 diff --git a/tests/sanity/ignore-2.13.txt b/tests/sanity/ignore-2.13.txt index b27ed13..fe7e251 100644 --- a/tests/sanity/ignore-2.13.txt +++ b/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 diff --git a/tests/sanity/ignore-2.9.txt b/tests/sanity/ignore-2.9.txt index b5126a0..4f8d3f1 100644 --- a/tests/sanity/ignore-2.9.txt +++ b/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 From d33ff29dbe8e6494d937dfa5f4c3d9ab8e649133 Mon Sep 17 00:00:00 2001 From: Mike Graves Date: Thu, 27 Jan 2022 11:43:38 -0500 Subject: [PATCH 2/4] formatting --- plugins/modules/turbo_demo.py | 1 + plugins/modules/turbo_import.py | 1 + 2 files changed, 2 insertions(+) diff --git a/plugins/modules/turbo_demo.py b/plugins/modules/turbo_demo.py index 3d8aa33..30093b5 100644 --- a/plugins/modules/turbo_demo.py +++ b/plugins/modules/turbo_demo.py @@ -66,6 +66,7 @@ def run_module(): def main(): from ansible_collections.cloud.common.plugins.module_utils import turbo_demo + run_module() diff --git a/plugins/modules/turbo_import.py b/plugins/modules/turbo_import.py index adbc322..92e0fbf 100644 --- a/plugins/modules/turbo_import.py +++ b/plugins/modules/turbo_import.py @@ -34,6 +34,7 @@ def run_module(): def main(): from ansible_collections.cloud.common.plugins.module_utils import turbo_demo + run_module() From 54b8afa2c6ae4dfe344c78fa99022154975329ac Mon Sep 17 00:00:00 2001 From: Mike Graves Date: Thu, 27 Jan 2022 13:23:10 -0500 Subject: [PATCH 3/4] Add changelog fragment --- changelogs/fragments/102-fix-incorrect-module-loading.yaml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelogs/fragments/102-fix-incorrect-module-loading.yaml diff --git a/changelogs/fragments/102-fix-incorrect-module-loading.yaml b/changelogs/fragments/102-fix-incorrect-module-loading.yaml new file mode 100644 index 0000000..c547888 --- /dev/null +++ b/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). From 37b41182dc35e09fcb975e9c76ff34f3144f9217 Mon Sep 17 00:00:00 2001 From: Mike Graves Date: Tue, 1 Feb 2022 15:15:07 -0500 Subject: [PATCH 4/4] Sort the modules before reloading The modules need to be reloaded within a package namespace from top to bottom. Trying to reload a module before its parent package loader has been adjusted leads to a FileNotFoundError. --- plugins/module_utils/turbo/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/module_utils/turbo/server.py b/plugins/module_utils/turbo/server.py index 770b599..c035a20 100644 --- a/plugins/module_utils/turbo/server.py +++ b/plugins/module_utils/turbo/server.py @@ -127,7 +127,7 @@ 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