setup module, make missing sysctl minor issue (#81297)

Signed-off-by: Brian Coca <brian.coca+git@gmail.com>
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Abhijeet Kasurde <akasurde@redhat.com>

changelogs/fragments/sysctl_fact_fix.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - setup/gather_facts will now warn and skip missing ``sysctl`` instead of being a fatal error.

lib/ansible/module_utils/basic.py

@@ -1348,14 +1348,16 @@ class AnsibleModule(object):
         # and we don't want to break modules unnecessarily
         return None
 
-    def get_bin_path(self, arg, required=False, opt_dirs=None):
+    def get_bin_path(self, arg, required=False, opt_dirs=None, warning=None):
         '''
         Find system executable in PATH.
 
         :param arg: The executable to find.
-        :param required: if executable is not found and required is ``True``, fail_json
+        :param required: if the executable is not found and required is ``True``, fail_json
         :param opt_dirs: optional list of directories to search in addition to ``PATH``
-        :returns: if found return full path; otherwise return None
+        :param warning: optional message when arg not found, only works when required=False
+        :returns: if found return full path; otherwise return original arg, unless 'warning' then return None
+        :raises: Sysexit: if arg is not found and required=True (via fail_json)
         '''
 
         bin_path = None
@@ -1364,8 +1366,8 @@ class AnsibleModule(object):
         except ValueError as e:
             if required:
                 self.fail_json(msg=to_text(e))
-            else:
-                return bin_path
+            elif warning is not None:
+                self.module.warn("Unable to find %s, %s" % (arg, warning))
 
         return bin_path
 

lib/ansible/module_utils/common/process.py

@@ -12,13 +12,18 @@ from ansible.module_utils.common.warnings import deprecate
 def get_bin_path(arg, opt_dirs=None, required=None):
     '''
     Find system executable in PATH. Raises ValueError if the executable is not found.
-    Optional arguments:
-       - required:  [Deprecated] Before 2.10, if executable is not found and required is true it raises an Exception.
-                    In 2.10 and later, an Exception is always raised. This parameter will be removed in 2.21.
-       - opt_dirs:  optional list of directories to search in addition to PATH
+
+    :param arg: the executable to find
+    :type arg: string
+    :param opt_dirs: optional list of directories to search in addition to PATH
+    :type opt_dirs: list of strings
+    :param required: DEPRECATED. This parameter will be removed in 2.21
+    :type required: boolean
+    :returns: path to arg (should be abs path unless PATH or opt_dirs are relative paths)
+    :raises: ValueError: if arg is not found
+
     In addition to PATH and opt_dirs, this function also looks through /sbin, /usr/sbin and /usr/local/sbin. A lot of
     modules, especially for gathering facts, depend on this behaviour.
-    If found return full path, otherwise raise ValueError.
     '''
     if required is not None:
         deprecate(
@@ -27,26 +32,34 @@ def get_bin_path(arg, opt_dirs=None, required=None):
             collection_name="ansible.builtin",
         )
 
+    paths = []
+    sbin_paths = ['/sbin', '/usr/sbin', '/usr/local/sbin']
     opt_dirs = [] if opt_dirs is None else opt_dirs
 
-    sbin_paths = ['/sbin', '/usr/sbin', '/usr/local/sbin']
-    paths = []
+    # Construct possible paths with precedence
+    # passed in paths
     for d in opt_dirs:
         if d is not None and os.path.exists(d):
             paths.append(d)
+    # system configured paths
     paths += os.environ.get('PATH', '').split(os.pathsep)
-    bin_path = None
-    # mangle PATH to include /sbin dirs
+
+    # existing /sbin dirs, if not there already
     for p in sbin_paths:
         if p not in paths and os.path.exists(p):
             paths.append(p)
+
+    # Search for binary
+    bin_path = None
     for d in paths:
         if not d:
             continue
         path = os.path.join(d, arg)
         if os.path.exists(path) and not os.path.isdir(path) and is_executable(path):
+            # fist found wins
             bin_path = path
             break
+
     if bin_path is None:
         raise ValueError('Failed to find required executable "%s" in paths: %s' % (arg, os.pathsep.join(paths)))
 

lib/ansible/module_utils/facts/hardware/aix.py

@@ -129,7 +129,7 @@ class AIXHardware(Hardware):
         rc, out, err = self.module.run_command("/usr/sbin/lsattr -El sys0 -a fwversion")
         data = out.split()
         dmi_facts['firmware_version'] = data[1].strip('IBM,')
-        lsconf_path = self.module.get_bin_path("lsconf")
+        lsconf_path = self.module.get_bin_path("lsconf", warning="dmi facts skipped")
         if lsconf_path:
             rc, out, err = self.module.run_command(lsconf_path)
             if rc == 0 and out:
@@ -160,8 +160,9 @@ class AIXHardware(Hardware):
         """
 
         vgs_facts = {}
-        lsvg_path = self.module.get_bin_path("lsvg")
-        xargs_path = self.module.get_bin_path("xargs")
+        warn = "vgs facts skipped"
+        lsvg_path = self.module.get_bin_path("lsvg", warning=warn)
+        xargs_path = self.module.get_bin_path("xargs", warning=warn)
         cmd = "%s -o | %s %s -p" % (lsvg_path, xargs_path, lsvg_path)
         if lsvg_path and xargs_path:
             rc, out, err = self.module.run_command(cmd, use_unsafe_shell=True)
@@ -194,35 +195,36 @@ class AIXHardware(Hardware):
 
         # AIX does not have mtab but mount command is only source of info (or to use
         # api calls to get same info)
-        mount_path = self.module.get_bin_path('mount')
-        rc, mount_out, err = self.module.run_command(mount_path)
-        if mount_out:
-            for line in mount_out.split('\n'):
-                fields = line.split()
-                if len(fields) != 0 and fields[0] != 'node' and fields[0][0] != '-' and re.match('^/.*|^[a-zA-Z].*|^[0-9].*', fields[0]):
-                    if re.match('^/', fields[0]):
-                        # normal mount
-                        mount = fields[1]
-                        mount_info = {'mount': mount,
-                                      'device': fields[0],
-                                      'fstype': fields[2],
-                                      'options': fields[6],
-                                      'time': '%s %s %s' % (fields[3], fields[4], fields[5])}
-                        mount_info.update(get_mount_size(mount))
-                    else:
-                        # nfs or cifs based mount
-                        # in case of nfs if no mount options are provided on command line
-                        # add into fields empty string...
-                        if len(fields) < 8:
-                            fields.append("")
-
-                        mount_info = {'mount': fields[2],
-                                      'device': '%s:%s' % (fields[0], fields[1]),
-                                      'fstype': fields[3],
-                                      'options': fields[7],
-                                      'time': '%s %s %s' % (fields[4], fields[5], fields[6])}
-
-                    mounts.append(mount_info)
+        mount_path = self.module.get_bin_path('mount', warning="skipping mount facts")
+        if mount_path:
+            rc, mount_out, err = self.module.run_command(mount_path)
+            if mount_out:
+                for line in mount_out.split('\n'):
+                    fields = line.split()
+                    if len(fields) != 0 and fields[0] != 'node' and fields[0][0] != '-' and re.match('^/.*|^[a-zA-Z].*|^[0-9].*', fields[0]):
+                        if re.match('^/', fields[0]):
+                            # normal mount
+                            mount = fields[1]
+                            mount_info = {'mount': mount,
+                                          'device': fields[0],
+                                          'fstype': fields[2],
+                                          'options': fields[6],
+                                          'time': '%s %s %s' % (fields[3], fields[4], fields[5])}
+                            mount_info.update(get_mount_size(mount))
+                        else:
+                            # nfs or cifs based mount
+                            # in case of nfs if no mount options are provided on command line
+                            # add into fields empty string...
+                            if len(fields) < 8:
+                                fields.append("")
+
+                            mount_info = {'mount': fields[2],
+                                          'device': '%s:%s' % (fields[0], fields[1]),
+                                          'fstype': fields[3],
+                                          'options': fields[7],
+                                          'time': '%s %s %s' % (fields[4], fields[5], fields[6])}
+
+                        mounts.append(mount_info)
 
         mount_facts['mounts'] = mounts
 
@@ -232,30 +234,32 @@ class AIXHardware(Hardware):
         device_facts = {}
         device_facts['devices'] = {}
 
-        lsdev_cmd = self.module.get_bin_path('lsdev', True)
-        lsattr_cmd = self.module.get_bin_path('lsattr', True)
-        rc, out_lsdev, err = self.module.run_command(lsdev_cmd)
-
-        for line in out_lsdev.splitlines():
-            field = line.split()
-
-            device_attrs = {}
-            device_name = field[0]
-            device_state = field[1]
-            device_type = field[2:]
-            lsattr_cmd_args = [lsattr_cmd, '-E', '-l', device_name]
-            rc, out_lsattr, err = self.module.run_command(lsattr_cmd_args)
-            for attr in out_lsattr.splitlines():
-                attr_fields = attr.split()
-                attr_name = attr_fields[0]
-                attr_parameter = attr_fields[1]
-                device_attrs[attr_name] = attr_parameter
-
-            device_facts['devices'][device_name] = {
-                'state': device_state,
-                'type': ' '.join(device_type),
-                'attributes': device_attrs
-            }
+        warn = 'device facts are skipped'
+        lsdev_cmd = self.module.get_bin_path('lsdev', warning=warn)
+        lsattr_cmd = self.module.get_bin_path('lsattr', warning=warn)
+        if lsdev_cmd and lsattr_cmd:
+            rc, out_lsdev, err = self.module.run_command(lsdev_cmd)
+
+            for line in out_lsdev.splitlines():
+                field = line.split()
+
+                device_attrs = {}
+                device_name = field[0]
+                device_state = field[1]
+                device_type = field[2:]
+                lsattr_cmd_args = [lsattr_cmd, '-E', '-l', device_name]
+                rc, out_lsattr, err = self.module.run_command(lsattr_cmd_args)
+                for attr in out_lsattr.splitlines():
+                    attr_fields = attr.split()
+                    attr_name = attr_fields[0]
+                    attr_parameter = attr_fields[1]
+                    device_attrs[attr_name] = attr_parameter
+
+                device_facts['devices'][device_name] = {
+                    'state': device_state,
+                    'type': ' '.join(device_type),
+                    'attributes': device_attrs
+                }
 
         return device_facts
 

lib/ansible/module_utils/facts/hardware/darwin.py

@@ -94,43 +94,52 @@ class DarwinHardware(Hardware):
 
         total_used = 0
         page_size = 4096
-        vm_stat_command = self.module.get_bin_path('vm_stat')
+
+        vm_stat_command = self.module.get_bin_path(
+            'vm_stat',
+            warning='falling back to sysctl for memtotal_mb, default to 0 for memfree_mb'
+        )
         if vm_stat_command is None:
             return memory_facts
 
-        rc, out, err = self.module.run_command(vm_stat_command)
-        if rc == 0:
-            # Free = Total - (Wired + active + inactive)
-            # Get a generator of tuples from the command output so we can later
-            # turn it into a dictionary
-            memory_stats = (line.rstrip('.').split(':', 1) for line in out.splitlines())
-
-            # Strip extra left spaces from the value
-            memory_stats = dict((k, v.lstrip()) for k, v in memory_stats)
-
-            for k, v in memory_stats.items():
-                try:
-                    memory_stats[k] = int(v)
-                except ValueError:
-                    # Most values convert cleanly to integer values but if the field does
-                    # not convert to an integer, just leave it alone.
-                    pass
-
-            if memory_stats.get('Pages wired down'):
-                total_used += memory_stats['Pages wired down'] * page_size
-            if memory_stats.get('Pages active'):
-                total_used += memory_stats['Pages active'] * page_size
-            if memory_stats.get('Pages inactive'):
-                total_used += memory_stats['Pages inactive'] * page_size
-
-            memory_facts['memfree_mb'] = memory_facts['memtotal_mb'] - (total_used // 1024 // 1024)
+        if vm_stat_command:
+            rc, out, err = self.module.run_command(vm_stat_command)
+            if rc == 0:
+                # Free = Total - (Wired + active + inactive)
+                # Get a generator of tuples from the command output so we can later
+                # turn it into a dictionary
+                memory_stats = (line.rstrip('.').split(':', 1) for line in out.splitlines())
+
+                # Strip extra left spaces from the value
+                memory_stats = dict((k, v.lstrip()) for k, v in memory_stats)
+
+                for k, v in memory_stats.items():
+                    try:
+                        memory_stats[k] = int(v)
+                    except ValueError:
+                        # Most values convert cleanly to integer values but if the field does
+                        # not convert to an integer, just leave it alone.
+                        pass
+
+                if memory_stats.get('Pages wired down'):
+                    total_used += memory_stats['Pages wired down'] * page_size
+                if memory_stats.get('Pages active'):
+                    total_used += memory_stats['Pages active'] * page_size
+                if memory_stats.get('Pages inactive'):
+                    total_used += memory_stats['Pages inactive'] * page_size
+
+                memory_facts['memfree_mb'] = memory_facts['memtotal_mb'] - (total_used // 1024 // 1024)
 
         return memory_facts
 
     def get_uptime_facts(self):
+
         # On Darwin, the default format is annoying to parse.
         # Use -b to get the raw value and decode it.
-        sysctl_cmd = self.module.get_bin_path('sysctl')
+        sysctl_cmd = self.module.get_bin_path('sysctl', warning='skipping uptime facts')
+        if not sysctl_cmd:
+            return {}
+
         cmd = [sysctl_cmd, '-b', 'kern.boottime']
 
         # We need to get raw bytes, not UTF-8.

lib/ansible/module_utils/facts/hardware/freebsd.py

@@ -23,7 +23,6 @@ import time
 
 from ansible.module_utils.facts.hardware.base import Hardware, HardwareCollector
 from ansible.module_utils.facts.timeout import TimeoutError, timeout
-
 from ansible.module_utils.facts.utils import get_file_content, get_mount_size
 
 

lib/ansible/module_utils/facts/hardware/hpux.py

@@ -40,6 +40,9 @@ class HPUXHardware(Hardware):
     def populate(self, collected_facts=None):
         hardware_facts = {}
 
+        # TODO: very inefficient calls to machinfo,
+        # should just make one and then deal with finding the data (see facts/sysctl)
+        # but not going to change unless there is hp/ux for testing
         cpu_facts = self.get_cpu_facts(collected_facts=collected_facts)
         memory_facts = self.get_memory_facts()
         hw_facts = self.get_hw_facts()

lib/ansible/module_utils/facts/hardware/netbsd.py

@@ -161,7 +161,13 @@ class NetBSDHardware(Hardware):
 
     def get_uptime_facts(self):
         # On NetBSD, we need to call sysctl with -n to get this value as an int.
-        sysctl_cmd = self.module.get_bin_path('sysctl')
+        sysctl_cmd = self.module.get_bin_path(
+            'sysctl',
+            warning="skipping uptime facts"
+        )
+        if sysctl_cmd is None:
+            return {}
+
         cmd = [sysctl_cmd, '-n', 'kern.boottime']
 
         rc, out, err = self.module.run_command(cmd)

lib/ansible/module_utils/facts/hardware/openbsd.py

@@ -112,7 +112,13 @@ class OpenBSDHardware(Hardware):
 
     def get_uptime_facts(self):
         # On openbsd, we need to call it with -n to get this value as an int.
-        sysctl_cmd = self.module.get_bin_path('sysctl')
+        sysctl_cmd = self.module.get_bin_path(
+            'sysctl',
+            warning="skipping uptime facts"
+        )
+        if sysctl_cmd is None:
+            return {}
+
         cmd = [sysctl_cmd, '-n', 'kern.boottime']
 
         rc, out, err = self.module.run_command(cmd)

lib/ansible/module_utils/facts/hardware/sunos.py

@@ -172,7 +172,14 @@ class SunOSHardware(Hardware):
         rc, platform, err = self.module.run_command('/usr/bin/uname -i')
         platform_sbin = '/usr/platform/' + platform.rstrip() + '/sbin'
 
-        prtdiag_path = self.module.get_bin_path("prtdiag", opt_dirs=[platform_sbin])
+        prtdiag_path = self.module.get_bin_path(
+            "prtdiag",
+            opt_dirs=[platform_sbin],
+            warning="skipping dmi facts"
+        )
+        if prtdiag_path is None:
+            return dmi_facts
+
         rc, out, err = self.module.run_command(prtdiag_path)
         # rc returns 1
         if out:

lib/ansible/module_utils/facts/network/aix.py

@@ -31,21 +31,25 @@ class AIXNetwork(GenericBsdIfconfigNetwork):
     def get_default_interfaces(self, route_path):
         interface = dict(v4={}, v6={})
 
-        netstat_path = self.module.get_bin_path('netstat')
-
-        if netstat_path:
-            rc, out, err = self.module.run_command([netstat_path, '-nr'])
-
-            lines = out.splitlines()
-            for line in lines:
-                words = line.split()
-                if len(words) > 1 and words[0] == 'default':
-                    if '.' in words[1]:
-                        interface['v4']['gateway'] = words[1]
-                        interface['v4']['interface'] = words[5]
-                    elif ':' in words[1]:
-                        interface['v6']['gateway'] = words[1]
-                        interface['v6']['interface'] = words[5]
+        netstat_path = self.module.get_bin_path(
+            'netstat',
+            warning="skipping default interface facts"
+        )
+        if netstat_path is None:
+            return interface['v4'], interface['v6']
+
+        rc, out, err = self.module.run_command([netstat_path, '-nr'])
+
+        lines = out.splitlines()
+        for line in lines:
+            words = line.split()
+            if len(words) > 1 and words[0] == 'default':
+                if '.' in words[1]:
+                    interface['v4']['gateway'] = words[1]
+                    interface['v4']['interface'] = words[5]
+                elif ':' in words[1]:
+                    interface['v6']['gateway'] = words[1]
+                    interface['v6']['interface'] = words[5]
 
         return interface['v4'], interface['v6']
 
@@ -58,9 +62,7 @@ class AIXNetwork(GenericBsdIfconfigNetwork):
             all_ipv6_addresses=[],
         )
 
-        uname_rc = None
-        uname_out = None
-        uname_err = None
+        uname_rc = uname_out = uname_err = None
         uname_path = self.module.get_bin_path('uname')
         if uname_path:
             uname_rc, uname_out, uname_err = self.module.run_command([uname_path, '-W'])

lib/ansible/module_utils/facts/network/fc_wwn.py

@@ -47,7 +47,10 @@ class FcWwnInitiatorFactCollector(BaseFactCollector):
         elif sys.platform.startswith('sunos'):
             # on solaris 10 or solaris 11 should use `fcinfo hba-port`
             # TBD (not implemented): on solaris 9 use `prtconf -pv`
-            cmd = module.get_bin_path('fcinfo')
+            cmd = module.get_bin_path(
+                'fcinfo',
+                warning="skipping fibre wwn initiator facts"
+            )
             if cmd:
                 cmd = cmd + " hba-port"
                 rc, fcinfo_out, err = module.run_command(cmd)
@@ -59,8 +62,14 @@ class FcWwnInitiatorFactCollector(BaseFactCollector):
                             data = line.split(' ')
                             fc_facts['fibre_channel_wwn'].append(data[-1].rstrip())
         elif sys.platform.startswith('aix'):
-            cmd = module.get_bin_path('lsdev')
-            lscfg_cmd = module.get_bin_path('lscfg')
+            cmd = module.get_bin_path(
+                'lsdev',
+                warning="skipping fibre wwn initiator facts"
+            )
+            lscfg_cmd = module.get_bin_path(
+                'lscfg',
+                warning="skipping fibre wwn initiator facts"
+            )
             if cmd and lscfg_cmd:
                 # get list of available fibre-channel devices (fcs)
                 cmd = cmd + " -Cc adapter -l fcs*"
@@ -81,8 +90,15 @@ class FcWwnInitiatorFactCollector(BaseFactCollector):
                                         data = line.split('.')
                                         fc_facts['fibre_channel_wwn'].append(data[-1].rstrip())
         elif sys.platform.startswith('hp-ux'):
-            cmd = module.get_bin_path('ioscan')
-            fcmsu_cmd = module.get_bin_path('fcmsutil', opt_dirs=['/opt/fcms/bin'])
+            cmd = module.get_bin_path(
+                'ioscan',
+                warning="skipping fibre wwn initiator facts"
+            )
+            fcmsu_cmd = module.get_bin_path(
+                'fcmsutil',
+                opt_dirs=['/opt/fcms/bin'],
+                warning="skipping fibre wwn initiator facts"
+            )
             # go ahead if we have both commands available
             if cmd and fcmsu_cmd:
                 # ioscan / get list of available fibre-channel devices (fcd)

lib/ansible/module_utils/facts/network/generic_bsd.py

@@ -34,12 +34,18 @@ class GenericBsdIfconfigNetwork(Network):
 
     def populate(self, collected_facts=None):
         network_facts = {}
-        ifconfig_path = self.module.get_bin_path('ifconfig')
+        ifconfig_path = self.module.get_bin_path(
+            'ifconfig',
+            warning="skipping network facts"
+        )
 
         if ifconfig_path is None:
             return network_facts
 
-        route_path = self.module.get_bin_path('route')
+        route_path = self.module.get_bin_path(
+            'route',
+            warning="skipping network facts"
+        )
 
         if route_path is None:
             return network_facts

lib/ansible/module_utils/facts/network/hpux.py

@@ -29,7 +29,11 @@ class HPUXNetwork(Network):
 
     def populate(self, collected_facts=None):
         network_facts = {}
-        netstat_path = self.module.get_bin_path('netstat')
+        netstat_path = self.module.get_bin_path(
+            'netstat',
+            opt_dirs=['/usr/bin'],
+            warning="skipping network facts"
+        )
 
         if netstat_path is None:
             return network_facts
@@ -46,7 +50,15 @@ class HPUXNetwork(Network):
 
     def get_default_interfaces(self):
         default_interfaces = {}
-        rc, out, err = self.module.run_command("/usr/bin/netstat -nr")
+        netstat_path = self.module.get_bin_path(
+            'netstat',
+            opt_dirs=['/usr/bin'],
+            warning="skipping default interface facts"
+        )
+
+        if netstat_path is None:
+            return default_interfaces
+        rc, out, err = self.module.run_command("%s -nr" % netstat_path)
         lines = out.splitlines()
         for line in lines:
             words = line.split()
@@ -59,7 +71,15 @@ class HPUXNetwork(Network):
 
     def get_interfaces_info(self):
         interfaces = {}
-        rc, out, err = self.module.run_command("/usr/bin/netstat -niw")
+        netstat_path = self.module.get_bin_path(
+            'netstat',
+            opt_dirs=['/usr/bin'],
+            warning="skipping default interface info facts"
+        )
+
+        if netstat_path is None:
+            return interfaces
+        rc, out, err = self.module.run_command("%s -niw" % netstat_path)
         lines = out.splitlines()
         for line in lines:
             words = line.split()

lib/ansible/module_utils/facts/network/hurd.py

@@ -63,7 +63,10 @@ class HurdPfinetNetwork(Network):
     def populate(self, collected_facts=None):
         network_facts = {}
 
-        fsysopts_path = self.module.get_bin_path('fsysopts')
+        fsysopts_path = self.module.get_bin_path(
+            'fsysopts',
+            warning="skipping network facts"
+        )
         if fsysopts_path is None:
             return network_facts
 

lib/ansible/module_utils/facts/network/iscsi.py

@@ -21,7 +21,6 @@ import sys
 
 import ansible.module_utils.compat.typing as t
 
-from ansible.module_utils.common.process import get_bin_path
 from ansible.module_utils.facts.utils import get_file_content
 from ansible.module_utils.facts.network.base import NetworkCollector
 
@@ -80,9 +79,11 @@ class IscsiInitiatorNetworkCollector(NetworkCollector):
                     iscsi_facts['iscsi_iqn'] = line.split('=', 1)[1]
                     break
         elif sys.platform.startswith('aix'):
-            try:
-                cmd = get_bin_path('lsattr')
-            except ValueError:
+            cmd = module.get_bin_path(
+                'lsattr',
+                warning="skipping iscsi initiator facts"
+            )
+            if cmd is None:
                 return iscsi_facts
 
             cmd += " -E -l iscsi0"
@@ -92,10 +93,12 @@ class IscsiInitiatorNetworkCollector(NetworkCollector):
                 iscsi_facts['iscsi_iqn'] = line.split()[1].rstrip()
 
         elif sys.platform.startswith('hp-ux'):
-            # try to find it in the default PATH and opt_dirs
-            try:
-                cmd = get_bin_path('iscsiutil', opt_dirs=['/opt/iscsi/bin'])
-            except ValueError:
+            cmd = module.get_bin_path(
+                'iscsiutil',
+                opt_dirs=['/opt/iscsi/bin'],
+                warning="skipping iscsi initiator facts"
+            )
+            if cmd is None:
                 return iscsi_facts
 
             cmd += " -l"

lib/ansible/module_utils/facts/other/facter.py

@@ -22,8 +22,16 @@ class FacterFactCollector(BaseFactCollector):
                                                   namespace=namespace)
 
     def find_facter(self, module):
-        facter_path = module.get_bin_path('facter', opt_dirs=['/opt/puppetlabs/bin'])
-        cfacter_path = module.get_bin_path('cfacter', opt_dirs=['/opt/puppetlabs/bin'])
+        facter_path = module.get_bin_path(
+            'facter',
+            opt_dirs=['/opt/puppetlabs/bin'],
+            warning="falling back to cfacter to gather facter facts"
+        )
+        cfacter_path = module.get_bin_path(
+            'cfacter',
+            opt_dirs=['/opt/puppetlabs/bin'],
+            warning="skipping facter facts"
+        )
 
         # Prefer to use cfacter if available
         if cfacter_path is not None:
@@ -73,7 +81,6 @@ class FacterFactCollector(BaseFactCollector):
         try:
             facter_dict = json.loads(facter_output)
         except Exception:
-            # FIXME: maybe raise a FactCollectorError with some info attrs?
-            pass
+            module.warn("Failed to parse facter facts")
 
         return facter_dict

lib/ansible/module_utils/facts/other/ohai.py

@@ -36,10 +36,12 @@ class OhaiFactCollector(BaseFactCollector):
                                                 namespace=namespace)
 
     def find_ohai(self, module):
-        ohai_path = module.get_bin_path('ohai')
-        return ohai_path
+        return module.get_bin_path(
+            'ohai',
+            warning="skipping ohai facts"
+        )
 
-    def run_ohai(self, module, ohai_path,):
+    def run_ohai(self, module, ohai_path):
         rc, out, err = module.run_command(ohai_path)
         return rc, out, err
 
@@ -67,7 +69,6 @@ class OhaiFactCollector(BaseFactCollector):
         try:
             ohai_facts = json.loads(ohai_output)
         except Exception:
-            # FIXME: useful error, logging, something...
-            pass
+            module.warn("Failed to gather ohai facts")
 
         return ohai_facts

lib/ansible/module_utils/facts/sysctl.py

@@ -21,41 +21,43 @@ from ansible.module_utils.common.text.converters import to_text
 
 
 def get_sysctl(module, prefixes):
-    sysctl_cmd = module.get_bin_path('sysctl')
-    cmd = [sysctl_cmd]
-    cmd.extend(prefixes)
 
     sysctl = dict()
-
-    try:
-        rc, out, err = module.run_command(cmd)
-    except (IOError, OSError) as e:
-        module.warn('Unable to read sysctl: %s' % to_text(e))
-        rc = 1
-
-    if rc == 0:
-        key = ''
-        value = ''
-        for line in out.splitlines():
-            if not line.strip():
-                continue
-
-            if line.startswith(' '):
-                # handle multiline values, they will not have a starting key
-                # Add the newline back in so people can split on it to parse
-                # lines if they need to.
-                value += '\n' + line
-                continue
+    sysctl_cmd = module.get_bin_path('sysctl', warning='skipping sysctl based facts')
+    if sysctl_cmd is not None:
+
+        cmd = [sysctl_cmd]
+        cmd.extend(prefixes)
+
+        try:
+            rc, out, err = module.run_command(cmd)
+        except (IOError, OSError) as e:
+            module.warn('Unable to read sysctl: %s' % to_text(e))
+            rc = 1
+
+        if rc == 0:
+            key = ''
+            value = ''
+            for line in out.splitlines():
+                if not line.strip():
+                    continue
+
+                if line.startswith(' '):
+                    # handle multiline values, they will not have a starting key
+                    # Add the newline back in so people can split on it to parse
+                    # lines if they need to.
+                    value += '\n' + line
+                    continue
+
+                if key:
+                    sysctl[key] = value.strip()
+
+                try:
+                    (key, value) = re.split(r'\s?=\s?|: ', line, maxsplit=1)
+                except Exception as e:
+                    module.warn('Unable to split sysctl line (%s): %s' % (to_text(line), to_text(e)))
 
             if key:
                 sysctl[key] = value.strip()
 
-            try:
-                (key, value) = re.split(r'\s?=\s?|: ', line, maxsplit=1)
-            except Exception as e:
-                module.warn('Unable to split sysctl line (%s): %s' % (to_text(line), to_text(e)))
-
-        if key:
-            sysctl[key] = value.strip()
-
     return sysctl

test/units/module_utils/facts/network/test_fc_wwn.py

@@ -89,34 +89,28 @@ FCMSUTIL_OUT = """
 """
 
 
-def mock_get_bin_path(cmd, required=False, opt_dirs=None):
-    result = None
-    if cmd == 'lsdev':
-        result = '/usr/sbin/lsdev'
-    elif cmd == 'lscfg':
-        result = '/usr/sbin/lscfg'
-    elif cmd == 'fcinfo':
-        result = '/usr/sbin/fcinfo'
-    elif cmd == 'ioscan':
-        result = '/usr/bin/ioscan'
-    elif cmd == 'fcmsutil':
-        result = '/opt/fcms/bin/fcmsutil'
-    return result
+def mock_get_bin_path(cmd, required=False, opt_dirs=None, warning=None):
+    cmds = {
+        'lsdev': '/usr/sbin/lsdev',
+        'lscfg': '/usr/sbin/lscfg',
+        'fcinfo': '/usr/sbin/fcinfo',
+        'ioscan': '/usr/bin/ioscan',
+        'fcmsutil': '/opt/fcms/bin/fcmsutil',
+    }
+    return cmds.get(cmd, None)
 
 
 def mock_run_command(cmd):
     rc = 0
-    if 'lsdev' in cmd:
-        result = LSDEV_OUTPUT
-    elif 'lscfg' in cmd:
-        result = LSCFG_OUTPUT
-    elif 'fcinfo' in cmd:
-        result = FCINFO_OUTPUT
-    elif 'ioscan' in cmd:
-        result = IOSCAN_OUT
-    elif 'fcmsutil' in cmd:
-        result = FCMSUTIL_OUT
-    else:
+    COMMANDS = {
+        '/usr/sbin/lsdev': LSDEV_OUTPUT,
+        '/usr/sbin/lscfg': LSCFG_OUTPUT,
+        '/usr/sbin/fcinfo': FCINFO_OUTPUT,
+        '/usr/bin/ioscan': IOSCAN_OUT,
+        '/opt/fcms/bin/fcmsutil': FCMSUTIL_OUT,
+    }
+    result = COMMANDS.get(cmd.split()[0])
+    if result is None:
         rc = 1
         result = 'Error'
     return (rc, result, '')

test/units/module_utils/facts/network/test_generic_bsd.py

@@ -22,12 +22,12 @@ import unittest
 from ansible.module_utils.facts.network import generic_bsd
 
 
-def get_bin_path(command):
-    if command == 'ifconfig':
-        return 'fake/ifconfig'
-    elif command == 'route':
-        return 'fake/route'
-    return None
+def get_bin_path(command, warning=None):
+    cmds = {
+        'ifconfig': 'fake/ifconfig',
+        'route': 'fake/route',
+    }
+    return cmds.get(command, None)
 
 
 netbsd_ifconfig_a_out_7_1 = r'''

test/units/module_utils/facts/network/test_iscsi_get_initiator.py

@@ -41,13 +41,13 @@ def test_get_iscsi_info(mocker):
     inst = iscsi.IscsiInitiatorNetworkCollector()
 
     mocker.patch('sys.platform', 'aix6')
-    mocker.patch('ansible.module_utils.facts.network.iscsi.get_bin_path', return_value='/usr/sbin/lsattr')
+    mocker.patch.object(module, 'get_bin_path', return_value='/usr/sbin/lsattr')
     mocker.patch.object(module, 'run_command', return_value=(0, LSATTR_OUTPUT, ''))
     aix_iscsi_expected = {"iscsi_iqn": "iqn.localhost.hostid.7f000002"}
     assert aix_iscsi_expected == inst.collect(module=module)
 
     mocker.patch('sys.platform', 'hp-ux')
-    mocker.patch('ansible.module_utils.facts.network.iscsi.get_bin_path', return_value='/opt/iscsi/bin/iscsiutil')
+    mocker.patch.object(module, 'get_bin_path', return_value='/opt/iscsi/bin/iscsiutil')
     mocker.patch.object(module, 'run_command', return_value=(0, ISCSIUTIL_OUTPUT, ''))
     hpux_iscsi_expected = {"iscsi_iqn": " iqn.2001-04.com.hp.stor:svcio"}
     assert hpux_iscsi_expected == inst.collect(module=module)