Configurable and parallel gather facts #49399

bcoca · 2018-12-01T02:43:28Z

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

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

gather_facts

ansibot · 2018-12-01T02:47:54Z

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

click here for bot help

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...

lib/ansible/plugins/action/gather_facts.py

lib/ansible/modules/system/gather_facts.py

also deal with the other 'setup specific' arguments

uses smart connection mapping

based on using 'network appliance' specific connection plugins

nitzmahone

let's discuss whether the connection map should be exposed or not, otherwise LGTM

nitzmahone · 2019-03-05T23:23:02Z

lib/ansible/config/base.yml

@@ -1324,6 +1324,30 @@ ERROR_ON_MISSING_HANDLER:
  ini:
  - {key: error_on_missing_handler, section: defaults}
  type: boolean
+CONNECTION_FACTS_MODULES:


This is a good stopgap for networking until we have platform, but I assume this would be superseded by that. Do we want to expose it to env/ini knowing that we'll need to deprecate it whenever we get around to platform, or just keep it as an internal detail for stuff that's shipped in the box for now?

since we cannot supply 'maps' that way .. i chose not to .. at least until ansible.config.yml

I'm fine with deprecating this once platform is out, but i don't see it as either/or proposition.

we could create per transport config .. but that does not need to happen right now, I would wait for user feedback.

Requested changes made, but have not re-reviewed at a higher level.

trishnaguha · 2019-03-07T05:19:14Z

lib/ansible/config/base.yml

+    ios: ios_facts
+    iosxr: iosxr_facts
+    nxos: nxos_facts
+    vyos: vyos_facts


@bcoca Will you please add frr: frr_facts and junos: junos_facts as well?

trishnaguha

@bcoca Does this facts gathering enable us to import ansible_facts in modules like we currently have with collectors in module_utils/facts/compat.py?

bcoca · 2019-03-07T14:46:17Z

@trishnaguha i don't understand the question

trishnaguha · 2019-03-08T05:11:12Z

@bcoca If we do from ansible.module_utils.facts import ansible_facts and call the ansible_facts(module) API in modules, the facts system runs the same collector for the specific platform and gets facts on runtime for each task/module run and return that to the module <Does NOT override ansible_facts var>. This enables fetching information from the devices for idempotence without having to implement that directly in the module.
REF: https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/facts/compat.py#L49

I was wondering if we have similar functionality here which would just execute the facts module whenever ansible_facts API is called and pass that to the module. So we don't have to maintain platform facts collector in module_utils/facts/ if someone wants to use ansible_facts API.

bcoca · 2019-03-08T15:33:07Z

this is not touching the API, it just allows you to execute 1 or more facts modules

ansible deleted a comment from ansibot Dec 1, 2018

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

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Dec 1, 2018

bcoca removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 3, 2018

ansibot added the core_review In order to be merged, this PR must follow the core review workflow. label Dec 3, 2018

sivel removed needs_triage Needs a first human triage before being processed. labels Dec 4, 2018

bcoca requested a review from nitzmahone December 10, 2018 16:35

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 Dec 10, 2018

nitzmahone requested changes Dec 10, 2018

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Dec 10, 2018

bcoca force-pushed the gather_facts_parallel branch from 81d0736 to 8a17c2e Compare December 10, 2018 20:37

nitzmahone reviewed Dec 11, 2018

View reviewed changes

lib/ansible/plugins/action/gather_facts.py Outdated Show resolved Hide resolved

nitzmahone reviewed Dec 11, 2018

View reviewed changes

lib/ansible/plugins/action/gather_facts.py Outdated Show resolved Hide resolved

nitzmahone reviewed Dec 11, 2018

View reviewed changes

lib/ansible/modules/system/gather_facts.py Outdated Show resolved Hide resolved

bcoca added 13 commits February 28, 2019 12:18

notes about merging

296f52a

moved to 'parallel'

3d82df0

fix typo

d39172d

parity on result handling and aggregation

1c7edcc

nicer parallel default

2b08e1f

retrun var to match config, being list

1e4d988

also deal with the other 'setup specific' arguments

added support to automatically map network facts

2857bc8

uses smart connection mapping

allow for smart to be partial list

a1b847a

fix pop

986ce8b

allow gather_subset for network appliance facts

d14ab06

based on using 'network appliance' specific connection plugins

fix load name

6bee84a

fixed keyerror

9ed7874

updates per fb

accda64

bcoca force-pushed the gather_facts_parallel branch from 6a8999c to accda64 Compare February 28, 2019 17:23

nitzmahone reviewed Mar 5, 2019

View reviewed changes

trishnaguha reviewed Mar 7, 2019

View reviewed changes

This was referenced Mar 7, 2019

Update frr_facts module #53439

Merged

frr gather_facts backup #53444

Closed

expanded network map as per request

f9aea59

This comment has been minimized.

Sign in to view

restore comment hash

784c7ff

bcoca merged commit 8940732 into ansible:devel Mar 8, 2019

bcoca deleted the gather_facts_parallel branch March 8, 2019 18:08

nitzmahone mentioned this pull request Mar 12, 2019

[WIP] Inventory override of fact gathering action ansible/proposals#17

Closed

ansible locked and limited conversation to collaborators Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable and parallel gather facts #49399

Configurable and parallel gather facts #49399

bcoca commented Dec 1, 2018

ansibot commented Dec 1, 2018

nitzmahone left a comment

nitzmahone left a comment

nitzmahone Mar 5, 2019

bcoca Mar 6, 2019 •

edited

trishnaguha Mar 7, 2019

trishnaguha left a comment

bcoca commented Mar 7, 2019

This comment has been minimized.

trishnaguha commented Mar 8, 2019 •

edited

bcoca commented Mar 8, 2019

Configurable and parallel gather facts #49399

Configurable and parallel gather facts #49399

Conversation

bcoca commented Dec 1, 2018

ISSUE TYPE

COMPONENT NAME

ansibot commented Dec 1, 2018

nitzmahone left a comment

Choose a reason for hiding this comment

nitzmahone left a comment

Choose a reason for hiding this comment

nitzmahone Mar 5, 2019

Choose a reason for hiding this comment

bcoca Mar 6, 2019 • edited

Choose a reason for hiding this comment

trishnaguha Mar 7, 2019

Choose a reason for hiding this comment

trishnaguha left a comment

Choose a reason for hiding this comment

bcoca commented Mar 7, 2019

This comment has been minimized.

trishnaguha commented Mar 8, 2019 • edited

bcoca commented Mar 8, 2019

bcoca Mar 6, 2019 •

edited

trishnaguha commented Mar 8, 2019 •

edited