cmueller/ansible
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix fact failures cause by ordering of collectors (#30777)

Browse source 
* Fix fact failures cause by ordering of collectors

Some fact collectors need info collected by other facts.
(for ex, service_mgr needs to know 'ansible_system').
This info is passed to the Collector.collect method via
the 'collected_facts' info.

But, the order the fact collectors were running in is
not a set order, so collectors like service_mgr could
run before the PlatformFactCollect ('ansible_system', etc),
so the 'ansible_system' fact would not exist yet. 

Depending on the collector and the deps, this can result
in incorrect behavior and wrong or missing facts.

To make the ordering of the collectors more consistent
and predictable, the code that builds that list is now
driven by the order of collectors in default_collectors.py,
and the rest of the code tries to preserve it.

* Flip the loops when building collector names

iterate over the ordered default_collectors list
selecting them for the final list in order instead
of driving it from the unordered collector_names set.

This lets the list returned by select_collector_classes
to stay in the same order as default_collectors.collectors

For collectors that have implicit deps on other fact collectors,
the default collectors can be ordered to include those early.

* default_collectors.py now uses a handful of sub lists of
collectors that can be ordered in default_collectors.collectors.

fixes #30753
fixes #30623
This commit is contained in:
Adrian Likins 2017-09-28 10:36:22 -04:00 committed by GitHub
parent c5971047a4
commit 95abc1d82e
5 changed files with 277 additions and 81 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

39
lib/ansible/module_utils/facts/collector.py
View file

@ -203,6 +203,28 @@ def build_fact_id_to_collector_map(collectors_for_platform):
return fact_id_to_collector_map, aliases_map
def select_collector_classes(collector_names, all_fact_subsets, all_collector_classes):
# TODO: can be a set()
seen_collector_classes = []
selected_collector_classes = []
for candidate_collector_class in all_collector_classes:
candidate_collector_name = candidate_collector_class.name
if candidate_collector_name not in collector_names:
continue
collector_classes = all_fact_subsets.get(candidate_collector_name, [])
for collector_class in collector_classes:
if collector_class not in seen_collector_classes:
selected_collector_classes.append(collector_class)
seen_collector_classes.append(collector_class)
return selected_collector_classes
def collector_classes_from_gather_subset(all_collector_classes=None,
valid_subsets=None,
minimal_gather_subset=None,
@ -248,19 +270,8 @@ def collector_classes_from_gather_subset(all_collector_classes=None,
aliases_map=aliases_map,
platform_info=platform_info)
# TODO: can be a set()
seen_collector_classes = []
selected_collector_classes = []
for collector_name in collector_names:
collector_classes = all_fact_subsets.get(collector_name, [])
# TODO? log/warn if we dont find an implementation of a fact_id?
for collector_class in collector_classes:
if collector_class not in seen_collector_classes:
selected_collector_classes.append(collector_class)
seen_collector_classes.append(collector_class)
selected_collector_classes = select_collector_classes(collector_names,
all_fact_subsets,
all_collector_classes)
return selected_collector_classes

80
lib/ansible/module_utils/facts/default_collectors.py
View file

@ -74,16 +74,48 @@ from ansible.module_utils.facts.virtual.netbsd import NetBSDVirtualCollector
from ansible.module_utils.facts.virtual.openbsd import OpenBSDVirtualCollector
from ansible.module_utils.facts.virtual.sunos import SunOSVirtualCollector
# TODO: make config driven
collectors = [ApparmorFactCollector,
# these should always be first due to most other facts depending on them
_base = [
PlatformFactCollector,
DistributionFactCollector,
LSBFactCollector
]
# These restrict what is possible in others
_restrictive = [
SelinuxFactCollector,
ApparmorFactCollector,
ChrootFactCollector,
FipsFactCollector
]
# general info, not required but probably useful for other facts
_general = [
PythonFactCollector,
SystemCapabilitiesFactCollector,
PkgMgrFactCollector,
OpenBSDPkgMgrFactCollector,
ServiceMgrFactCollector,
CmdLineFactCollector,
DateTimeFactCollector,
DistributionFactCollector,
DnsFactCollector,
EnvFactCollector,
FipsFactCollector,
SshPubKeyFactCollector,
UserFactCollector
]
# virtual, this might also limit hardware/networking
_virtual = [
VirtualCollector,
DragonFlyVirtualCollector,
FreeBSDVirtualCollector,
LinuxVirtualCollector,
OpenBSDVirtualCollector,
NetBSDVirtualCollector,
SunOSVirtualCollector,
HPUXVirtualCollector
]
_hardware = [
HardwareCollector,
AIXHardwareCollector,
DarwinHardwareCollector,
@ -94,10 +126,11 @@ collectors = [ApparmorFactCollector,
LinuxHardwareCollector,
NetBSDHardwareCollector,
OpenBSDHardwareCollector,
SunOSHardwareCollector,
LocalFactCollector,
LSBFactCollector,
SunOSHardwareCollector
]
_network = [
DnsFactCollector,
NetworkCollector,
AIXNetworkCollector,
DarwinNetworkCollector,
@ -108,26 +141,15 @@ collectors = [ApparmorFactCollector,
LinuxNetworkCollector,
NetBSDNetworkCollector,
OpenBSDNetworkCollector,
SunOSNetworkCollector,
PkgMgrFactCollector,
OpenBSDPkgMgrFactCollector,
PlatformFactCollector,
PythonFactCollector,
SelinuxFactCollector,
ServiceMgrFactCollector,
SshPubKeyFactCollector,
SystemCapabilitiesFactCollector,
UserFactCollector,
VirtualCollector,
DragonFlyVirtualCollector,
FreeBSDVirtualCollector,
LinuxVirtualCollector,
OpenBSDVirtualCollector,
NetBSDVirtualCollector,
SunOSVirtualCollector,
HPUXVirtualCollector,
SunOSNetworkCollector
]
# other fact sources
_extra_facts = [
LocalFactCollector,
FacterFactCollector,
OhaiFactCollector]
OhaiFactCollector
]
# TODO: make config driven
collectors = _base + _restrictive + _general + _virtual + _hardware + _network + _extra_facts

2
test/integration/inventory
View file

@ -5,7 +5,7 @@ testhost2 ansible_ssh_host=127.0.0.1 ansible_connection=local
testhost3 ansible_ssh_host=127.0.0.3
testhost4 ansible_ssh_host=127.0.0.4
# For testing fact gathering
facthost[0:20] ansible_host=127.0.0.1 ansible_connection=local
facthost[0:25] ansible_host=127.0.0.1 ansible_connection=local
[binary_modules]
testhost_binary_modules ansible_host=127.0.0.1 ansible_connection=local

96
test/integration/targets/gathering_facts/test_gathering_facts.yml
View file

@ -11,6 +11,82 @@
- "!hardware"
register: not_hardware_facts
- name: min and network test for platform added
hosts: facthost21
tags: [ 'fact_network' ]
connection: local
gather_subset: "!all,network"
gather_facts: yes
tasks:
- name: Test that retrieving network facts works and gets prereqs from platform and distribution
assert:
that:
- 'ansible_default_ipv4|default("UNDEF") != "UNDEF"'
- 'ansible_interfaces|default("UNDEF") != "UNDEF"'
# these are true for linux, but maybe not for other os
- 'ansible_system|default("UNDEF") != "UNDEF"'
- 'ansible_distribution|default("UNDEF") != "UNDEF"'
# we dont really require these but they are in the min set
# - 'ansible_virtualization_role|default("UNDEF") == "UNDEF"'
# - 'ansible_user_id|default("UNDEF") == "UNDEF"'
# - 'ansible_env|default("UNDEF") == "UNDEF"'
# - 'ansible_selinux|default("UNDEF") == "UNDEF"'
# - 'ansible_pkg_mgr|default("UNDEF") == "UNDEF"'
- name: min and hardware test for platform added
hosts: facthost22
tags: [ 'fact_hardware' ]
connection: local
gather_subset: "hardware"
gather_facts: yes
tasks:
- name: debug stuff
debug:
var: hostvars['facthost22']
# we should also collect platform, but not distribution
- name: Test that retrieving hardware facts works and gets prereqs from platform and distribution
when: ansible_system|default("UNDEF") == "Linux"
assert:
# LinuxHardwareCollector requires 'platform' facts
that:
- 'ansible_memory_mb|default("UNDEF") != "UNDEF"'
- 'ansible_default_ipv4|default("UNDEF") == "UNDEF"'
- 'ansible_interfaces|default("UNDEF") == "UNDEF"'
# these are true for linux, but maybe not for other os
# hardware requires 'platform'
- 'ansible_system|default("UNDEF") != "UNDEF"'
- 'ansible_machine|default("UNDEF") != "UNDEF"'
# hardware does not require 'distribution' but it is min set
# - 'ansible_distribution|default("UNDEF") == "UNDEF"'
# we dont really require these but they are in the min set
# - 'ansible_virtualization_role|default("UNDEF") == "UNDEF"'
# - 'ansible_user_id|default("UNDEF") == "UNDEF"'
# - 'ansible_env|default("UNDEF") == "UNDEF"'
# - 'ansible_selinux|default("UNDEF") == "UNDEF"'
# - 'ansible_pkg_mgr|default("UNDEF") == "UNDEF"'
- name: min and service_mgr test for platform added
hosts: facthost23
tags: [ 'fact_service_mgr' ]
connection: local
gather_subset: "!all,service_mgr"
gather_facts: yes
tasks:
- name: Test that retrieving service_mgr facts works and gets prereqs from platform and distribution
assert:
that:
- 'ansible_service_mgr|default("UNDEF") != "UNDEF"'
- 'ansible_default_ipv4|default("UNDEF") == "UNDEF"'
- 'ansible_interfaces|default("UNDEF") == "UNDEF"'
# these are true for linux, but maybe not for other os
- 'ansible_system|default("UNDEF") != "UNDEF"'
- 'ansible_distribution|default("UNDEF") != "UNDEF"'
# we dont really require these but they are in the min set
# - 'ansible_virtualization_role|default("UNDEF") == "UNDEF"'
# - 'ansible_user_id|default("UNDEF") == "UNDEF"'
# - 'ansible_env|default("UNDEF") == "UNDEF"'
# - 'ansible_selinux|default("UNDEF") == "UNDEF"'
# - 'ansible_pkg_mgr|default("UNDEF") == "UNDEF"'
- hosts: facthost0
tags: [ 'fact_min' ]
@ -18,8 +94,8 @@
gather_subset: "all"
gather_facts: yes
tasks:
- setup:
register: facts
#- setup:
# register: facts
- name: Test that retrieving all facts works
assert:
that:
@ -36,7 +112,7 @@
tasks:
- setup:
filter: "*env*"
register: facts_results
register: fact_results
- name: Test that retrieving all facts filtered to env works
assert:
@ -53,7 +129,7 @@
tasks:
- setup:
filter: "ansible_user_id"
register: facts_results
register: fact_results
- name: Test that retrieving all facts filtered to specific fact ansible_user_id works
assert:
@ -72,7 +148,7 @@
tasks:
- setup:
filter: "*"
register: facts
register: fact_results
- name: Test that retrieving all facts filtered to splat
assert:
@ -89,7 +165,7 @@
tasks:
- setup:
filter: ""
register: facts
register: fact_results
- name: Test that retrieving all facts filtered to empty filter_spec works
assert:
@ -119,16 +195,16 @@
- hosts: facthost2
tags: [ 'fact_network' ]
connection: local
gather_subset: "network"
gather_subset: "!all,!min,network"
gather_facts: yes
tasks:
- name: Test that retrieving network facts work
assert:
that:
- 'ansible_user_id|default("UNDEF_MIN") != "UNDEF_MIN"'
- 'ansible_user_id|default("UNDEF") == "UNDEF"'
- 'ansible_interfaces|default("UNDEF_NET") != "UNDEF_NET"'
- 'ansible_mounts|default("UNDEF_MOUNT") == "UNDEF_MOUNT"'
- 'ansible_virtualization_role|default("UNDEF_VIRT") == "UNDEF_VIRT"'
- 'ansible_mounts|default("UNDEF") == "UNDEF"'
- 'ansible_virtualization_role|default("UNDEF") == "UNDEF"'
- hosts: facthost3
tags: [ 'fact_hardware' ]

87
test/units/module_utils/facts/test_collector.py
View file

@ -20,6 +20,8 @@
from __future__ import (absolute_import, division)
__metaclass__ = type
from collections import defaultdict
# for testing
from ansible.compat.tests import unittest
@ -28,6 +30,91 @@ from ansible.module_utils.facts import collector
from ansible.module_utils.facts import default_collectors
class TestFindCollectorsForPlatform(unittest.TestCase):
def test(self):
compat_platforms = [{'system': 'Generic'}]
res = collector.find_collectors_for_platform(default_collectors.collectors,
compat_platforms)
for coll_class in res:
self.assertIn(coll_class._platform, ('Generic'))
def test_linux(self):
compat_platforms = [{'system': 'Linux'}]
res = collector.find_collectors_for_platform(default_collectors.collectors,
compat_platforms)
for coll_class in res:
self.assertIn(coll_class._platform, ('Linux'))
def test_linux_or_generic(self):
compat_platforms = [{'system': 'Generic'}, {'system': 'Linux'}]
res = collector.find_collectors_for_platform(default_collectors.collectors,
compat_platforms)
for coll_class in res:
self.assertIn(coll_class._platform, ('Generic', 'Linux'))
class TestSelectCollectorNames(unittest.TestCase):
def test(self):
collector_names = set(['distribution', 'all_ipv4_addresses',
'local', 'pkg_mgr'])
all_fact_subsets = self._all_fact_subsets()
all_collector_classes = self._all_collector_classes()
res = collector.select_collector_classes(collector_names,
all_fact_subsets,
all_collector_classes)
expected = [default_collectors.DistributionFactCollector,
default_collectors.PkgMgrFactCollector]
self.assertEqual(res, expected)
def test_reverse(self):
collector_names = set(['distribution', 'all_ipv4_addresses',
'local', 'pkg_mgr'])
all_fact_subsets = self._all_fact_subsets()
all_collector_classes = self._all_collector_classes()
all_collector_classes.reverse()
res = collector.select_collector_classes(collector_names,
all_fact_subsets,
all_collector_classes)
expected = [default_collectors.PkgMgrFactCollector,
default_collectors.DistributionFactCollector]
self.assertEqual(res, expected)
def test_default_collectors(self):
platform_info = {'system': 'Generic'}
compat_platforms = [platform_info]
collectors_for_platform = collector.find_collectors_for_platform(default_collectors.collectors,
compat_platforms)
all_fact_subsets, aliases_map = collector.build_fact_id_to_collector_map(collectors_for_platform)
all_valid_subsets = frozenset(all_fact_subsets.keys())
collector_names = collector.get_collector_names(valid_subsets=all_valid_subsets,
aliases_map=aliases_map,
platform_info=platform_info)
collector.select_collector_classes(collector_names,
all_fact_subsets,
default_collectors.collectors)
def _all_collector_classes(self):
return [default_collectors.DistributionFactCollector,
default_collectors.PkgMgrFactCollector,
default_collectors.LinuxNetworkCollector]
def _all_fact_subsets(self, data=None):
all_fact_subsets = defaultdict(list)
_data = {'pkg_mgr': [default_collectors.PkgMgrFactCollector],
'distribution': [default_collectors.DistributionFactCollector],
'network': [default_collectors.LinuxNetworkCollector]}
data = data or _data
for key, value in data.items():
all_fact_subsets[key] = value
return all_fact_subsets
class TestGetCollectorNames(unittest.TestCase):
def test_none(self):
res = collector.get_collector_names()