From 95abc1d82e4c40832c802253107ad3f9aeebc68d Mon Sep 17 00:00:00 2001 From: Adrian Likins Date: Thu, 28 Sep 2017 10:36:22 -0400 Subject: [PATCH] Fix fact failures cause by ordering of collectors (#30777) * 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 --- lib/ansible/module_utils/facts/collector.py | 39 +++-- .../module_utils/facts/default_collectors.py | 134 ++++++++++-------- test/integration/inventory | 2 +- .../gathering_facts/test_gathering_facts.yml | 96 +++++++++++-- .../module_utils/facts/test_collector.py | 87 ++++++++++++ 5 files changed, 277 insertions(+), 81 deletions(-) diff --git a/lib/ansible/module_utils/facts/collector.py b/lib/ansible/module_utils/facts/collector.py index 4baf5bd338..232c506803 100644 --- a/lib/ansible/module_utils/facts/collector.py +++ b/lib/ansible/module_utils/facts/collector.py @@ -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 diff --git a/lib/ansible/module_utils/facts/default_collectors.py b/lib/ansible/module_utils/facts/default_collectors.py index ca53efcc89..997306307e 100644 --- a/lib/ansible/module_utils/facts/default_collectors.py +++ b/lib/ansible/module_utils/facts/default_collectors.py @@ -74,60 +74,82 @@ 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 +# 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, + EnvFactCollector, + SshPubKeyFactCollector, + UserFactCollector +] + +# virtual, this might also limit hardware/networking +_virtual = [ + VirtualCollector, + DragonFlyVirtualCollector, + FreeBSDVirtualCollector, + LinuxVirtualCollector, + OpenBSDVirtualCollector, + NetBSDVirtualCollector, + SunOSVirtualCollector, + HPUXVirtualCollector +] + +_hardware = [ + HardwareCollector, + AIXHardwareCollector, + DarwinHardwareCollector, + DragonFlyHardwareCollector, + FreeBSDHardwareCollector, + HPUXHardwareCollector, + HurdHardwareCollector, + LinuxHardwareCollector, + NetBSDHardwareCollector, + OpenBSDHardwareCollector, + SunOSHardwareCollector +] + +_network = [ + DnsFactCollector, + NetworkCollector, + AIXNetworkCollector, + DarwinNetworkCollector, + DragonFlyNetworkCollector, + FreeBSDNetworkCollector, + HPUXNetworkCollector, + HurdNetworkCollector, + LinuxNetworkCollector, + NetBSDNetworkCollector, + OpenBSDNetworkCollector, + SunOSNetworkCollector +] + +# other fact sources +_extra_facts = [ + LocalFactCollector, + FacterFactCollector, + OhaiFactCollector +] + # TODO: make config driven -collectors = [ApparmorFactCollector, - ChrootFactCollector, - CmdLineFactCollector, - DateTimeFactCollector, - DistributionFactCollector, - DnsFactCollector, - EnvFactCollector, - FipsFactCollector, - - HardwareCollector, - AIXHardwareCollector, - DarwinHardwareCollector, - DragonFlyHardwareCollector, - FreeBSDHardwareCollector, - HPUXHardwareCollector, - HurdHardwareCollector, - LinuxHardwareCollector, - NetBSDHardwareCollector, - OpenBSDHardwareCollector, - SunOSHardwareCollector, - LocalFactCollector, - LSBFactCollector, - - NetworkCollector, - AIXNetworkCollector, - DarwinNetworkCollector, - DragonFlyNetworkCollector, - FreeBSDNetworkCollector, - HPUXNetworkCollector, - HurdNetworkCollector, - LinuxNetworkCollector, - NetBSDNetworkCollector, - OpenBSDNetworkCollector, - SunOSNetworkCollector, - - PkgMgrFactCollector, - OpenBSDPkgMgrFactCollector, - PlatformFactCollector, - PythonFactCollector, - SelinuxFactCollector, - ServiceMgrFactCollector, - SshPubKeyFactCollector, - SystemCapabilitiesFactCollector, - UserFactCollector, - - VirtualCollector, - DragonFlyVirtualCollector, - FreeBSDVirtualCollector, - LinuxVirtualCollector, - OpenBSDVirtualCollector, - NetBSDVirtualCollector, - SunOSVirtualCollector, - HPUXVirtualCollector, - - FacterFactCollector, - OhaiFactCollector] +collectors = _base + _restrictive + _general + _virtual + _hardware + _network + _extra_facts diff --git a/test/integration/inventory b/test/integration/inventory index 3534b8cb8a..697a6a8ba0 100644 --- a/test/integration/inventory +++ b/test/integration/inventory @@ -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 diff --git a/test/integration/targets/gathering_facts/test_gathering_facts.yml b/test/integration/targets/gathering_facts/test_gathering_facts.yml index 271d44266c..6769bf9ce7 100644 --- a/test/integration/targets/gathering_facts/test_gathering_facts.yml +++ b/test/integration/targets/gathering_facts/test_gathering_facts.yml @@ -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' ] diff --git a/test/units/module_utils/facts/test_collector.py b/test/units/module_utils/facts/test_collector.py index 362a6ffcfe..4e2b0cef50 100644 --- a/test/units/module_utils/facts/test_collector.py +++ b/test/units/module_utils/facts/test_collector.py @@ -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()