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

Prototype vars and inventory dump performance tweaks #79687

Draft
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

nitzmahone
Copy link
Member

@nitzmahone nitzmahone commented Jan 6, 2023

SUMMARY

Demo elimination of unnecessary disk I/O that slows down host/group vars resolution and inventory dumps.

fixes #79664 (poorly; these changes are not production-ready but just show the issues and suggest fixes)

Running ansible-inventory with these changes against this inventory script with 100 groups of 500 hosts each (50k hosts total) reduces the inventory dump wall clock time by ~ an order of magnitude:

devel:

(ansible-dev39) [mdavis@mdavis-p1 TEMP_slowinv]$ time ansible-inventory -i ./inv.py --list --output /dev/null

real    0m50.638s
user    0m43.346s
sys     0m6.845s

this branch:

(ansible-dev39) [mdavis@mdavis-p1 TEMP_slowinv]$ time ansible-inventory -i ./inv.py --list --output /dev/null

real    0m5.495s
user    0m5.151s
sys     0m0.337s

Some notes about the rationale for the changes:

PluginLoader.all():

  • makes heavy use of globbing to find builtin and legacy plugins
  • does not cache path traversal info or missing paths, only source for loaded plugins
  • does not cache config defs for loaded plugins

host_group_vars vars plugin:

  • overuses os.path.realpath(), which is incredibly expensive; normalize paths earlier, or maybe not at all- penalty for duplicates is likely much less than the aggregate cost of the calls
  • does not cache missing paths/files

utils/path.py basedir() helper:

  • normalize with abspath() earlier or cache the normalized result; abspath calls are very expensive and occurring on every get_vars call

utils/vars.py _validate_mutable_mappings() helper:

  • this gets called millions of times during combine_vars; normalize things earlier or catch the immutability problems at the call sites- the isinstance calls are surprisingly expensive when called so frequently

vars/clean.py clean_facts():

  • excessive (and incomplete for collections!) use of PluginLoader.all() with connection plugins to get var prefixes to clean
  • cache the needed dynamic info

vars/plugins.py get_vars_from_path():

  • excessive use of PluginLoader.all() for vars plugins (cache findings)
  • excessive creation of vars plugins instances; builtin host_group_vars is basically stateless. Either cache instances or cache vars_plugins configs- the plugin setup and config is what's killing us when this is called millions of times.
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ansible-inventory, vars plugins

Demo elimination of problematic disk I/O that slows down host/group vars resolution and inventory dumps.

PluginLoader.all():
* makes heavy use of globbing to find builtin and legacy plugins
* does not cache path traversal info or missing paths, only source for loaded plugins
* does not cache config defs for loaded plugins

host_group_vars vars plugin:
* overuses `os.path.realpath()`, which is incredibly expensive; normalize paths earlier, or maybe not at all- penalty for duplicates is likely much less than the aggregate cost of the calls
* does not cache missing paths/files

utils/path.py basedir() helper:
* normalize with abspath() earlier or cache the normalized result; abspath calls are very expensive and occurring on every `get_vars` call

utils/vars.py _validate_mutable_mappings() helper:
* this gets called millions of times during combine_vars; normalize things earlier or catch the immutability problems at the call sites- the `isinstance` calls are surprisingly expensive when called so frequently

vars/clean.py clean_facts():
* excessive (and incomplete for collections!) use of PluginLoader.all() with connection plugins to get var prefixes to clean
* cache the needed dynamic info

vars/plugins.py get_vars_from_path():
* excessive use of PluginLoader.all() for vars plugins (cache findings)
* excessive creation of vars plugins instances; builtin host_group_vars is basically stateless. Either cache instances or cache vars_plugins configs- the plugin setup and config is what's killing us when this is called millions of times.
@nitzmahone nitzmahone marked this pull request as draft January 6, 2023 20:56
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.15 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. labels Jan 6, 2023
@bcoca
Copy link
Member

bcoca commented Jan 9, 2023

plugins now have option_definitions which is basically a cache savvy access to their config defs (which are cached in the ConfigManager), but only works for 'configurable' plugins currently (want to add modules to this to eventually reflect argspec). The issue is initializing automatically on plugin first load (or plugin class load?) and use the cache from then on.

@ansibot
Copy link
Contributor

ansibot commented Jan 9, 2023

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

lib/ansible/plugins/loader.py:933:5: E303: too many blank lines (3)
lib/ansible/plugins/vars/host_group_vars.py:68:1: E302: expected 2 blank lines, found 1
lib/ansible/plugins/vars/host_group_vars.py:95:21: E265: block comment should start with '# '
lib/ansible/vars/plugins.py:45:1: E302: expected 2 blank lines, found 1

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

lib/ansible/utils/vars.py:72:4: unreachable: Unreachable code

click here for bot help

@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Jan 10, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 18, 2023
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. has_issue labels Jul 12, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 10, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 24, 2023
s-hertel pushed a commit to s-hertel/ansible that referenced this pull request Dec 20, 2023
this is based on ansible#32609, but now
the cache is cleared when a new directory is added to the loader

* avoid calling glob.glob() when looking for py files in variable folders

* for each host listed in the inventory. improves performance by 30%.
  (in 2017, for unspecified example)

I added a global cache so all plugin loaders stay in sync like the other
caches, not sure if this is wanted.

ansible#79687 also identifies this as a hot code path.
s-hertel added a commit to s-hertel/ansible that referenced this pull request Dec 20, 2023
this is based on ansible#32609

* avoid calling glob.glob() when looking for py files in variable folders for each host listed in the inventory.

In addition, I added a global cache so all plugin loaders stay in sync
(not sure if this is wanted), and reset the cache when new directories
are added to the loader, like the other caches.

ansible#79687 also identifies this as a hot code path.

Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>
s-hertel added a commit to s-hertel/ansible that referenced this pull request Jan 31, 2024
this is based on ansible#32609

* avoid calling glob.glob() when looking for py files in variable folders for each host listed in the inventory.

In addition, I added a global cache so all plugin loaders stay in sync
(not sure if this is wanted), and reset the cache when new directories
are added to the loader, like the other caches.

ansible#79687 also identifies this as a hot code path.

Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.15 bug This issue/PR relates to a bug. has_issue needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slow inventory build
4 participants