From 9db557e43114e6f26d39ab3537ad301b02334d22 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Mon, 8 Mar 2021 16:20:37 -0500 Subject: [PATCH] Nonfatal facts (#73804) continue with local facts vs at script error actually capture execution errors better error messages in general add more local facts tests fixes #52427 --- changelogs/fragments/local_facts_continue.yml | 2 + .../module_utils/facts/system/local.py | 50 ++++++++++++------- .../targets/facts_d/files/basdscript.fact | 3 ++ .../targets/facts_d/files/goodscript.fact | 3 ++ .../targets/facts_d/files/preferences.fact | 2 + .../targets/facts_d/files/unreadable.fact | 1 + .../targets/facts_d/tasks/main.yml | 48 ++++++++++-------- 7 files changed, 68 insertions(+), 41 deletions(-) create mode 100644 changelogs/fragments/local_facts_continue.yml create mode 100644 test/integration/targets/facts_d/files/basdscript.fact create mode 100644 test/integration/targets/facts_d/files/goodscript.fact create mode 100644 test/integration/targets/facts_d/files/preferences.fact create mode 100644 test/integration/targets/facts_d/files/unreadable.fact diff --git a/changelogs/fragments/local_facts_continue.yml b/changelogs/fragments/local_facts_continue.yml new file mode 100644 index 00000000000..eb9b5898f70 --- /dev/null +++ b/changelogs/fragments/local_facts_continue.yml @@ -0,0 +1,2 @@ +bugfixes: + - setup, don't give up on all local facts gathering if one script file fails. diff --git a/lib/ansible/module_utils/facts/system/local.py b/lib/ansible/module_utils/facts/system/local.py index fe33a3232fe..e80fca9eac2 100644 --- a/lib/ansible/module_utils/facts/system/local.py +++ b/lib/ansible/module_utils/facts/system/local.py @@ -21,12 +21,10 @@ import json import os import stat -from ansible.module_utils.six.moves import configparser -from ansible.module_utils.six.moves import StringIO - +from ansible.module_utils._text import to_text from ansible.module_utils.facts.utils import get_file_content - from ansible.module_utils.facts.collector import BaseFactCollector +from ansible.module_utils.six.moves import configparser, StringIO class LocalFactCollector(BaseFactCollector): @@ -46,36 +44,47 @@ class LocalFactCollector(BaseFactCollector): return local_facts local = {} + # go over .fact files, run executables, read rest, skip bad with warning and note for fn in sorted(glob.glob(fact_path + '/*.fact')): - # where it will sit under local facts + # use filename for key where it will sit under local facts fact_base = os.path.basename(fn).replace('.fact', '') if stat.S_IXUSR & os.stat(fn)[stat.ST_MODE]: - # run it - # try to read it as json first - # if that fails read it with ConfigParser - # if that fails, skip it + failed = None try: + # run it rc, out, err = module.run_command(fn) - except UnicodeError: - fact = 'error loading fact - output of running %s was not utf-8' % fn - local[fact_base] = fact - local_facts['local'] = local - module.warn(fact) - return local_facts + if rc != 0: + failed = 'Failure executing fact script (%s), rc: %s, err: %s' % (fn, rc, err) + except (IOError, OSError) as e: + failed = 'Could not execute fact script (%s): %s' % (fn, to_text(e)) + + if failed is not None: + local[fact_base] = failed + module.warn(failed) + continue else: + # ignores exceptions and returns empty out = get_file_content(fn, default='') - # load raw json - fact = 'loading %s' % fact_base + try: + # ensure we have unicode + out = to_text(out, errors='surrogate_or_strict') + except UnicodeError: + fact = 'error loading fact - output of running "%s" was not utf-8' % fn + local[fact_base] = fact + module.warn(fact) + continue + + # try to read it as json first try: fact = json.loads(out) except ValueError: - # load raw ini + # if that fails read it with ConfigParser cp = configparser.ConfigParser() try: cp.readfp(StringIO(out)) except configparser.Error: - fact = "error loading fact - please check content" + fact = "error loading facts as JSON or ini - please check content: %s" % fn module.warn(fact) else: fact = {} @@ -85,6 +94,9 @@ class LocalFactCollector(BaseFactCollector): for opt in cp.options(sect): val = cp.get(sect, opt) fact[sect][opt] = val + except Exception as e: + fact = "Failed to convert (%s) to JSON: %s" % (fn, to_text(e)) + module.warn(fact) local[fact_base] = fact diff --git a/test/integration/targets/facts_d/files/basdscript.fact b/test/integration/targets/facts_d/files/basdscript.fact new file mode 100644 index 00000000000..2bb8d868bd0 --- /dev/null +++ b/test/integration/targets/facts_d/files/basdscript.fact @@ -0,0 +1,3 @@ +#!/bin/sh + +exit 1 diff --git a/test/integration/targets/facts_d/files/goodscript.fact b/test/integration/targets/facts_d/files/goodscript.fact new file mode 100644 index 00000000000..6ee866cfb6b --- /dev/null +++ b/test/integration/targets/facts_d/files/goodscript.fact @@ -0,0 +1,3 @@ +#!/bin/sh + +echo '{"script_ran": true}' diff --git a/test/integration/targets/facts_d/files/preferences.fact b/test/integration/targets/facts_d/files/preferences.fact new file mode 100644 index 00000000000..c32583d4b28 --- /dev/null +++ b/test/integration/targets/facts_d/files/preferences.fact @@ -0,0 +1,2 @@ +[general] +bar=loaded diff --git a/test/integration/targets/facts_d/files/unreadable.fact b/test/integration/targets/facts_d/files/unreadable.fact new file mode 100644 index 00000000000..98f562be6b2 --- /dev/null +++ b/test/integration/targets/facts_d/files/unreadable.fact @@ -0,0 +1 @@ +wontbeseen=ever diff --git a/test/integration/targets/facts_d/tasks/main.yml b/test/integration/targets/facts_d/tasks/main.yml index ca23544fbe0..aadef4c693f 100644 --- a/test/integration/targets/facts_d/tasks/main.yml +++ b/test/integration/targets/facts_d/tasks/main.yml @@ -1,35 +1,36 @@ -# Test code for facts.d and setup filters # (c) 2014, James Tanner +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) -# This file is part of Ansible -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . +- name: prep for local facts tests + block: + - name: set factdir var + set_fact: fact_dir={{output_dir}}/facts.d -- set_fact: fact_dir={{output_dir}}/facts.d + - name: create fact dir + file: path={{ fact_dir }} state=directory -- file: path={{ fact_dir }} state=directory -- shell: echo "[general]" > {{ fact_dir }}/preferences.fact -- shell: echo "bar=loaded" >> {{ fact_dir }}/preferences.fact + - name: copy local facts test files + copy: src={{ item['name'] }}.fact dest={{ fact_dir }}/ mode={{ item['mode']|default(omit) }} + loop: + - name: preferences + - name: basdscript + mode: '0775' + - name: goodscript + mode: '0775' + - name: unreadable + mode: '0000' -- setup: +- name: force fact gather to get ansible_local + setup: fact_path: "{{ fact_dir | expanduser }}" filter: "*local*" register: setup_result -- debug: var=setup_result +- name: show gathering results if rerun with -vvv + debug: var=setup_result verbosity=3 -- assert: +- name: check for expected results from local facts + assert: that: - "'ansible_facts' in setup_result" - "'ansible_local' in setup_result.ansible_facts" @@ -39,3 +40,6 @@ - "'general' in setup_result.ansible_facts['ansible_local']['preferences']" - "'bar' in setup_result.ansible_facts['ansible_local']['preferences']['general']" - "setup_result.ansible_facts['ansible_local']['preferences']['general']['bar'] == 'loaded'" + - setup_result['ansible_facts']['ansible_local']['goodscript']['script_ran']|bool + - setup_result['ansible_facts']['ansible_local']['basdscript'].startswith("Failure executing fact script") + - setup_result['ansible_facts']['ansible_local']['unreadable'].startswith('error loading facts')