[Plugin] Refactor add_blockdev_cmd into add_device_cmd

Initially, the design behind `add_blockdev_cmd()` made the assumption
that there would (eventually) be separate methods for different kinds of
devices. On review, this is less practical and less maintainable than a
single method, `add_device_cmd()` paired with improved functionality for
handling device lists.

As such, rework `add_blockdev_cmd()` into `add_device_cmd()` and with
it, change the logic on how device lists are sourced. The `devices` dict
handed to plugins now has a top-level separation between device types,
in this revision putting `block` and `fibre` devices under `storage` in
the `devices` dict.

In using `add_device_cmd()`, plugins may now specify either `block`,
`fibre`, or `storage` for both. In addition, any devices passed via the
`devices` parameter that aren't keys in the devices dict are returned,
meaning a plugin may specify a list of devices and add `storage` to
include all storage devices for command iteration.

This change should allow easier addition of network device iteration.

Signed-off-by: Jake Hunsaker <jhunsake@redhat.com>

8 files changed, 106 insertions, 51 deletions

diff --git a/sos/report/__init__.py b/sos/report/__init__.py
index 8402f717..ab25fe91 100644
--- a/sos/report/__init__.py
+++ b/sos/report/__init__.py
@@ -434,8 +434,10 @@ class SoSReport(SoSComponent):
 
     def _get_hardware_devices(self):
         self.devices = {
-            'block': self.get_block_devs(),
-            'fibre': self.get_fibre_devs()
+            'storage': {
+                'block': self.get_block_devs(),
+                'fibre': self.get_fibre_devs()
+            }
         }
         # TODO: enumerate network devices, preferably with devtype info
 
@@ -474,7 +476,7 @@ class SoSReport(SoSComponent):
         """Enumerate a list of fibrechannel devices on this system so that
         plugins can iterate over them
 
-        These devices are used by add_fibredev_cmd() in the Plugin class.
+        These devices are used by add_device_cmd() in the Plugin class.
         """
         try:
             devs = []
@@ -496,16 +498,15 @@ class SoSReport(SoSComponent):
         """Enumerate a list of block devices on this system so that plugins
         can iterate over them
 
-        These devices are used by add_blockdev_cmd() in the Plugin class.
+        These devices are used by add_device_cmd() in the Plugin class.
         """
         try:
-            device_list = os.listdir('/sys/block')
+            device_list = ["/dev/%s" % d for d in os.listdir('/sys/block')]
             loop_devices = sos_get_command_output('losetup --all --noheadings')
             real_loop_devices = []
             if loop_devices['status'] == 0:
                 for loop_dev in loop_devices['output'].splitlines():
                     loop_device = loop_dev.split()[0].replace(':', '')
-                    loop_device = loop_device.replace('/dev/', '')
                     real_loop_devices.append(loop_device)
             ghost_loop_devs = [dev for dev in device_list
                                if dev.startswith("loop")
@@ -1566,7 +1567,7 @@ class SoSReport(SoSComponent):
         self.report_md.add_list('profiles', self.opts.profiles)
         self.report_md.add_section('devices')
         for key, value in self.devices.items():
-            self.report_md.devices.add_list(key, value)
+            self.report_md.devices.add_field(key, value)
         self.report_md.add_list('enabled_plugins', self.opts.enable_plugins)
         self.report_md.add_list('disabled_plugins', self.opts.skip_plugins)
         self.report_md.add_section('plugins')
diff --git a/sos/report/plugins/__init__.py b/sos/report/plugins/__init__.py
index eed656ae..7e832acd 100644
--- a/sos/report/plugins/__init__.py
+++ b/sos/report/plugins/__init__.py
@@ -13,7 +13,8 @@
 from sos.utilities import (sos_get_command_output, import_module, grep,
                            fileobj, tail, is_executable, TIMEOUT_DEFAULT,
                            path_exists, path_isdir, path_isfile, path_islink,
-                           listdir, path_join, bold, file_is_binary)
+                           listdir, path_join, bold, file_is_binary,
+                           recursive_dict_values_by_key)
 
 from sos.archive import P_FILE
 import os
@@ -1867,23 +1868,24 @@ class Plugin():
                         'tags': _spec_tags
                     })
 
-    def add_blockdev_cmd(self, cmds, devices='block', timeout=None,
-                         sizelimit=None, chroot=True, runat=None, env=None,
-                         binary=False, prepend_path=None, whitelist=[],
-                         blacklist=[], tags=[], priority=10):
-        """Run a command or list of commands against storage-related devices.
+    def add_device_cmd(self, cmds, devices, timeout=None, sizelimit=None,
+                       chroot=True, runat=None, env=None, binary=False,
+                       prepend_path=None, whitelist=[], blacklist=[], tags=[],
+                       priority=10):
+        """Run a command or list of commands against devices discovered during
+        sos initialization.
 
         Any commands specified by cmd will be iterated over the list of the
         specified devices. Commands passed to this should include a '%(dev)s'
         variable for substitution.
 
-        :param cmds: The command(s) to run against the list of devices
-        :type cmds: ``str`` or a ``list`` of strings
+        :param cmds:    The command(s) to run against the list of devices
+        :type cmds:     ``str`` or a ``list`` of strings
 
-        :param devices: The device paths to run `cmd` against. If set to
-                        `block` or `fibre`, the commands will be run against
-                        the matching list of discovered devices
-        :type devices: ``str`` or a ``list`` of device paths
+        :param devices: The device paths to run `cmd` against. This should be
+                        either a list of devices/device paths or a key in the
+                        devices dict discovered by sos during initialization.
+        :type devices:  ``str`` or a ``list`` of devices or device paths.
 
         :param timeout: Timeout in seconds to allow each `cmd` to run
         :type timeout: ``int``
@@ -1914,51 +1916,49 @@ class Plugin():
                           item(s)
         :type blacklist: ``list`` of ``str``
         """
+
         _dev_tags = []
         if isinstance(tags, str):
             tags = [tags]
-        if devices == 'block':
-            prepend_path = prepend_path or '/dev/'
-            devices = self.devices['block']
-            _dev_tags.append('block')
-        if devices == 'fibre':
-            devices = self.devices['fibre']
-            _dev_tags.append('fibre')
+        if isinstance(devices, str):
+            devices = [devices]
+
+        _devs = recursive_dict_values_by_key(self.devices, devices)
+
+        if whitelist:
+            if isinstance(whitelist, str):
+                whitelist = [whitelist]
+
+            _devs = [d for d in _devs if
+                     any(re.match("(.*)?%s" % wl, d) for wl in whitelist)]
+
+        if blacklist:
+            if isinstance(blacklist, str):
+                blacklist = [blacklist]
+
+            _devs = [d for d in _devs if not
+                     any(re.match("(.*)?%s" % bl, d) for bl in blacklist)]
+
         _dev_tags.extend(tags)
-        self._add_device_cmd(cmds, devices, timeout=timeout,
+        self._add_device_cmd(cmds, _devs, timeout=timeout,
                              sizelimit=sizelimit, chroot=chroot, runat=runat,
                              env=env, binary=binary, prepend_path=prepend_path,
-                             whitelist=whitelist, blacklist=blacklist,
                              tags=_dev_tags, priority=priority)
 
     def _add_device_cmd(self, cmds, devices, timeout=None, sizelimit=None,
                         chroot=True, runat=None, env=None, binary=False,
-                        prepend_path=None, whitelist=[], blacklist=[],
-                        tags=[], priority=10):
+                        prepend_path=None, tags=[], priority=10):
         """Run a command against all specified devices on the system.
         """
         if isinstance(cmds, str):
             cmds = [cmds]
         if isinstance(devices, str):
             devices = [devices]
-        if isinstance(whitelist, str):
-            whitelist = [whitelist]
-        if isinstance(blacklist, str):
-            blacklist = [blacklist]
         sizelimit = sizelimit or self.get_option('log_size')
         for cmd in cmds:
             for device in devices:
-                _dev_ok = True
                 _dev_tags = [device]
                 _dev_tags.extend(tags)
-                if whitelist:
-                    if not any(re.match(wl, device) for wl in whitelist):
-                        _dev_ok = False
-                if blacklist:
-                    if any(re.match(blist, device) for blist in blacklist):
-                        _dev_ok = False
-                if not _dev_ok:
-                    continue
                 if prepend_path:
                     device = self.path_join(prepend_path, device)
                 _cmd = cmd % {'dev': device}
diff --git a/sos/report/plugins/ata.py b/sos/report/plugins/ata.py
index 23f73338..03a80566 100644
--- a/sos/report/plugins/ata.py
+++ b/sos/report/plugins/ata.py
@@ -27,7 +27,8 @@ class Ata(Plugin, IndependentPlugin):
             "smartctl -l scterc %(dev)s",
             "smartctl -l scterc %(dev)s -j"
         ]
-        self.add_blockdev_cmd(cmd_list, whitelist=['sd.*', 'hd.*'])
+        self.add_device_cmd(cmd_list, devices='block',
+                            whitelist=['sd.*', 'hd.*'])
 
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/block.py b/sos/report/plugins/block.py
index 23ca69a2..1ac084c8 100644
--- a/sos/report/plugins/block.py
+++ b/sos/report/plugins/block.py
@@ -56,7 +56,7 @@ class Block(Plugin, IndependentPlugin):
             "udevadm info %(dev)s",
             "udevadm info -a %(dev)s"
         ]
-        self.add_blockdev_cmd(cmds, blacklist='ram.*')
+        self.add_device_cmd(cmds, devices='block', blacklist='ram.*')
 
         lsblk = self.collect_cmd_output("lsblk -f -a -l")
         # for LUKS devices, collect cryptsetup luksDump
diff --git a/sos/report/plugins/fibrechannel.py b/sos/report/plugins/fibrechannel.py
index cd21d5da..535af137 100644
--- a/sos/report/plugins/fibrechannel.py
+++ b/sos/report/plugins/fibrechannel.py
@@ -29,7 +29,7 @@ class Fibrechannel(Plugin, RedHatPlugin):
     ]
 
     def setup(self):
-        self.add_blockdev_cmd("udevadm info -a %(dev)s", devices='fibre')
+        self.add_device_cmd("udevadm info -a %(dev)s", devices='fibre')
         if self.get_option('debug'):
             self.add_copy_spec(self.debug_paths)
 
diff --git a/sos/report/plugins/nvme.py b/sos/report/plugins/nvme.py
index ecd1e493..7ed59f7a 100644
--- a/sos/report/plugins/nvme.py
+++ b/sos/report/plugins/nvme.py
@@ -36,6 +36,6 @@ class Nvme(Plugin, IndependentPlugin):
             "nvme error-log %(dev)s",
             "nvme show-regs %(dev)s"
         ]
-        self.add_blockdev_cmd(cmds, whitelist='nvme.*')
+        self.add_device_cmd(cmds, devices='block', whitelist='nvme.*')
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/report/plugins/scsi.py b/sos/report/plugins/scsi.py
index f913f457..ec62dd78 100644
--- a/sos/report/plugins/scsi.py
+++ b/sos/report/plugins/scsi.py
@@ -61,14 +61,13 @@ class Scsi(Plugin, IndependentPlugin):
         ])
 
         scsi_hosts = glob("/sys/class/scsi_host/*")
-        self.add_blockdev_cmd("udevadm info -a %(dev)s", devices=scsi_hosts,
-                              prepend_path='/sys/class/scsi_host')
+        self.add_device_cmd("udevadm info -a %(dev)s", devices=scsi_hosts)
 
-        self.add_blockdev_cmd([
+        self.add_device_cmd([
             "sg_persist --in -k -d %(dev)s",
             "sg_persist --in -r -d %(dev)s",
             "sg_persist --in -s -d %(dev)s",
             "sg_inq %(dev)s"
-        ], whitelist=['sd.*'])
+        ], devices='block', whitelist=['sd.*'])
 
 # vim: set et ts=4 sw=4 :
diff --git a/sos/utilities.py b/sos/utilities.py
index 7aee016e..2046c8fd 100644
--- a/sos/utilities.py
+++ b/sos/utilities.py
@@ -325,6 +325,60 @@ def bold(text):
     return '\033[1m' + text + '\033[0m'
 
 
+def recursive_dict_values_by_key(dobj, keys=[]):
+    """Recursively compile all elements of a potentially nested dict by a set
+    of keys. If a given key is a dict within ``dobj``, then _all_ elements
+    within that dict, regardless of child keys, will be returned.
+
+    For example, if a Plugin searches the devices dict for the 'storage' key,
+    then all storage devices under the that dict (e.g. block, fibre, etc...)
+    will be returned. However, if the Plugin specifies 'block' via ``keys``,
+    then only the block devices within the devices['storage'] dict will be
+    returned.
+
+    Any elements passed here that are _not_ keys within the dict or any nested
+    dicts will also be returned.
+
+    :param dobj:    The 'top-level' dict to intially search by
+    :type dobj:     ``dict``
+
+    :param keys:    Which keys to compile elements from within ``dobj``. If no
+                    keys are given, all nested elements are returned
+    :param keys:    ``list`` of ``str``
+
+    :returns:       All elements within the dict and any nested dicts
+    :rtype:         ``list``
+    """
+    _items = []
+    _filt = []
+    _items.extend(keys)
+    if isinstance(dobj, dict):
+        for k, v in dobj.items():
+            _filt.append(k)
+            # get everything below this key, including nested dicts
+            if not keys or k in keys:
+                _items.extend(recursive_dict_values_by_key(v))
+            # recurse into this dict only for dict keys that match what
+            # we're looking for
+            elif isinstance(v, dict):
+                try:
+                    # this will return a nested list, extract it
+                    _items.extend(
+                        recursive_dict_values_by_key(
+                            v[key] for key in keys if key in v
+                        )[0]
+                    )
+                except IndexError:
+                    # none of the keys given exist in the nested dict
+                    pass
+                _filt.extend(v.keys())
+
+    else:
+        _items.extend(dobj)
+
+    return [d for d in _items if d not in _filt]
+
+
 class FakeReader():
     """Used as a replacement AsyncReader for when we are writing directly to
     disk, and allows us to keep more simplified flows for executing,