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

Configurable and parallel gather facts #49399

Open
wants to merge 11 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@bcoca
Member

bcoca commented Dec 1, 2018

Use a configurable list of multiple facts modules in parallel for fact gathering stage.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

gather_facts

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 1, 2018

Hi @bcoca, thank you for submitting this pull-request!

click here for bot help

@ansible ansible deleted a comment from ansibot Dec 1, 2018

@bcoca bcoca changed the title from [WIP] Gather facts parallel to Configurable and parallel gather facts Dec 1, 2018

@ansibot ansibot added needs_revision and removed WIP labels Dec 1, 2018

@bcoca bcoca removed the needs_revision label Dec 3, 2018

@ansibot ansibot added the core_review label Dec 3, 2018

@sivel sivel removed needs_triage labels Dec 4, 2018

@bcoca bcoca requested a review from nitzmahone Dec 10, 2018

@ansibot ansibot added the stale_ci label Dec 10, 2018

@nitzmahone

Overall process looks OK to me. Only big-picture question is if the async stuff is really worth it; unless a particular module is really slow, it adds a lot of moving parts and significant overhead (which may actually end up slower than serial direct execution in many cases).

Probably also needs explicit tests added to the Windows setup integration tests under win_setup; the async stuff will probably work as-written, but we need to make sure...

Show resolved Hide resolved lib/ansible/plugins/action/gather_facts.py Outdated
mod_args = self._task.args.copy()
if fact_module != 'setup':
mod_args.pop('gather_subset', None)

This comment has been minimized.

@nitzmahone

nitzmahone Dec 10, 2018

Member

Should maybe add a warning here if gather_subset isn't 'all? Alternatively, can we sanely tell if it was the gather_subset passed by config/play vs one set explicitly by the module args (so we're not effectively creating a new reserved module arg)?

This comment has been minimized.

@bcoca

bcoca Dec 10, 2018

Member

i expect that we phase out both 'setup' and gather_subset since this new feature lets us break out the 'huge' setup into ansible_min_facts, ansible_network_facts, etc

But i'm fine with adding a warning here, if you really want it.

class ActionModule(ActionBase):
def _get_module_args(self, fact_module, task_vars):
mod_args = task_vars.get('ansible_facts_modules', {}).get(fact_module, {})

This comment has been minimized.

@nitzmahone

nitzmahone Dec 10, 2018

Member

Should this technically also go through the config path, passing task_vars as variables instead of directly to task_vars?

This comment has been minimized.

@bcoca

bcoca Dec 10, 2018

Member

I would, but modules/actions do not go through the config system and I had no plans to add them to it.

Show resolved Hide resolved lib/ansible/plugins/action/gather_facts.py Outdated
else:
time.sleep(0.5)
if skipped:

This comment has been minimized.

@nitzmahone

nitzmahone Dec 10, 2018

Member

Hrm, the potential to mix success + failed + skipped in the same result is ... weird (especially the latter two). Wondering how the default callbacks handle display in those cases. The only other sane thing I could think of though is that some skipped is a warning instead, but it's a corner case, regardless...

This comment has been minimized.

@bcoca

bcoca Dec 10, 2018

Member

i tried several combinations, we do give feedback to the user for each module attempted, but i'm still unsure what would display best. The current should be sufficient for users to give us feedback.

elif res.get('skipped', False):
skipped[module] = res.get('msg')
else:
result = combine_vars(result, {'ansible_facts': res.get('ansible_facts', {})})

This comment has been minimized.

@nitzmahone

nitzmahone Dec 10, 2018

Member

Are we OK silently masking collisions between facts modules (last writer wins)? I think I'm OK with that (again, pretty minor corner case), but maybe a warning would be better (then again, somebody's going to want merge_hash to work at some point, and ick).

This comment has been minimized.

@bcoca

bcoca Dec 10, 2018

Member

it works the same as currently when you execute them yourself, last one updating wins

@ansibot ansibot added needs_revision and removed core_review labels Dec 10, 2018

bcoca added some commits May 11, 2018

Configurable list of facts modules (#31783)
* configurable list of facts modules

 - allow for args dict for specific modules
 - add way to pass parameters
 - avoid facts poluting test
 - move to 'facts gathered' flag
 - add 'gathering' setting tests

(cherry picked from commit 95655fa)

bcoca added some commits Dec 10, 2018

@bcoca bcoca force-pushed the bcoca:gather_facts_parallel branch from 81d0736 to 8a17c2e Dec 10, 2018

@ansibot ansibot added needs_ci and removed needs_ci stale_ci labels Dec 10, 2018

bcoca added some commits Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment