nagios-charmers team mailing list archive

[Joel Sing <joel.sing@xxxxxxxxxxxxx>]

Review: Needs Fixing

Generally looks good, handful of things to address.

Diff comments:

> diff --git a/src/lib/hwhealth/classes.py b/src/lib/hwhealth/classes.py
> new file mode 100644
> index 0000000..1884059
> --- /dev/null
> +++ b/src/lib/hwhealth/classes.py

Seems like this file should be tools.py or similar, so that it is indicative of what it actually contains (classes is highly generic).

> @@ -0,0 +1,298 @@
> +import os
> +import shutil
> +import subprocess
> +from zipfile import ZipFile, BadZipFile
> +from tempfile import TemporaryDirectory
> +from charmhelpers import fetch

Imports should be split into three groups (or two in this case) - standard library, third-party and library specific - https://www.python.org/dev/peps/pep-0008/#imports

In this case, it basically means adding a blank line above this one (since they're already grouped and sorted).

> +from charmhelpers.core import hookenv
> +from charmhelpers.core.hookenv import status_set
> +from charmhelpers.contrib.charmsupport import nrpe
> +
> +
> +class Tool():
> +    '''

For documentation strings, the first line should be the summary, followed by the rest - also """ is normally used instead of ''':

https://www.python.org/dev/peps/pep-0008/#documentation-strings

"""An abstract class that represents a "tool".

The idea is to delegate ...
"""

> +    This is an abstract class representing a "tool".
> +    The idea is to delegate install/configure duties to specific per-tool
> +    classes instead of handling multiple exceptional cases in a main loop
> +    during the install hook.
> +    Every tool should implement its own internal logic regarding how to be
> +    installed, configured, and removed.
> +    '''
> +    NRPE_PLUGINS_DIR = '/usr/local/lib/nagios/plugins'

I would recommend either sorting these, or grouping them, to aid readability/maintainability.

> +    NRPE_PLUGINS_MODE = 0o100755
> +    NRPE_PLUGINS_UID = 0
> +    NRPE_PLUGINS_GID = 0
> +    TOOLS_DIR = '/usr/local/bin'
> +    TOOLS_MODE = 0o100755
> +    TOOLS_UID = 0
> +    TOOLS_GID = 0
> +    CROND_DIR = '/etc/cron.d'
> +    CRONJOB_SCRIPT_MODE = 0o100755
> +    CRONJOB_SCRIPT_UID = 0
> +    CRONJOB_SCRIPT_GID = 0
> +    SUDOERS_DIR = '/etc/sudoers.d'
> +    SUDOERS_MODE = 0o100440
> +    SUDOERS_UID = 0
> +    SUDOERS_GID = 0
> +
> +    def __init__(self, shortname, nrpe_opts=''):
> +        self._nagios_hostname = nrpe.get_nagios_hostname()
> +        self._nrpe_opts = nrpe_opts
> +        self._shortname = shortname
> +        self._files_dir = os.path.join(hookenv.charm_dir(),
> +                                       'files',
> +                                       self._shortname)
> +        self._nrpe_script = 'check_{}'.format(shortname)
> +
> +    def _install_nrpe_plugin(self):
> +        src = os.path.join(self._files_dir, self._nrpe_script)
> +        dst = shutil.copy(src, self.NRPE_PLUGINS_DIR)
> +        os.chmod(dst, self.NRPE_PLUGINS_MODE)
> +        os.chown(dst, uid=self.NRPE_PLUGINS_UID, gid=self.NRPE_PLUGINS_GID)
> +        hookenv.log('NRPE script for tool [{}] installed at as {}'
> +                    ''.format(self._shortname, dst),
> +                    hookenv.DEBUG)
> +        return dst
> +
> +    def _remove_nrpe_plugin(self):
> +        plugin_path = os.path.join(self.NRPE_PLUGINS_DIR, self._nrpe_script)
> +        os.remove(plugin_path)
> +        hookenv.log('deleted NRPE script for tool [{}]'
> +                    ''.format(self._shortname),
> +                    hookenv.DEBUG)
> +
> +    def configure_nrpe_check(self):
> +        nrpe_setup = nrpe.NRPE(hostname=self._nagios_hostname, primary=False)
> +        cmd = ' '.join([self._nrpe_script, self._nrpe_opts])
> +        nrpe_setup.add_check(
> +            shortname=self._shortname,
> +            description='{} Hardware Health',
> +            check_cmd=cmd
> +        )
> +        nrpe_setup.write()
> +        hookenv.log('configured NRPE check for tool [{}]'

Nit: Given the indent style used for nrpe_setup.add_check above, I'd be inclined to use it here as well:

hookenv.log(
    'configured NRPE check for tool [{}]'.format(self._shortname),
    hookenv.DEBUG
)

Obviously if you change it here you should do the same elsewhere.

> +                    ''.format(self._shortname),
> +                    hookenv.DEBUG)
> +
> +    def remove_nrpe_check(self):
> +        nrpe_setup = nrpe.NRPE(hostname=self._nagios_hostname, primary=False)
> +        cmd = ' '.join([self._nrpe_script, self._nrpe_opts])
> +        nrpe_setup.remove_check(
> +            shortname=self._shortname,
> +            check_cmd=cmd
> +        )
> +        nrpe_setup.write()
> +        hookenv.log('removed NRPE check for tool [{}]'
> +                    ''.format(self._shortname),
> +                    hookenv.DEBUG)
> +
> +    def install(self):
> +        self._install_nrpe_plugin()
> +        hookenv.log('Installed tool [{}]'.format(self._shortname))
> +
> +    def remove(self):
> +        self.remove_nrpe_check()
> +        self._remove_nrpe_plugin()
> +        hookenv.log('Removed tool [{}]'.format(self._shortname))
> +
> +
> +class VendorTool(Tool):
> +    '''

Same here re doc comments:

"""A class representing a Vendor tool that is shipped via a resource.

A Vendor tool is a statically linked ...
"""

> +    A Vendor tool is a statically linked binary we ship via a resource.
> +    Since it generally requires root privileges to run, we also deploy a
> +    cronjob that runs as root and saves the tool output in a temporary file
> +    that nrpe can read (as nagios user).
> +    '''
> +    def __init__(self, shortname):
> +        super().__init__(shortname=shortname)
> +        self._cronjob_script = 'check_{}.sh'.format(shortname)
> +        self._nrpe_script = 'check_{}.py'.format(shortname)
> +
> +    def install(self):
> +        self._install_from_resource()
> +        # TODO(aluria): install vendor utilities via snaps
> +        # https://forum.snapcraft.io/t/request-for-classic-confinement-sas2ircu/9023
> +        # Snap interface for hw tooling is not supported

This comment seems strangely/awkwardly wrapped.

> +        # in strict mode (confined). Classic mode
> +        # (unconfined snaps) need approval
> +        # Install the cronjob that generates the temp output
> +        self._install_cronjob()
> +        super().install()
> +
> +    def remove(self):
> +        self._remove_cronjob()
> +        self._remove_binary()
> +        super().remove()
> +
> +    def _install_from_resource(self):
> +        resource = hookenv.resource_get('tools')
> +        if resource:

Nit: To make this more readable, I'd be inclined to invert this:

if not resource:
  hookenv.log('Missing Juju resource ...')
  status_set('blocked', 'Missing Juju resource: tools')
  return

This way you drop an indent level, rather than having an else that is 30+ lines from the if.

> +            hookenv.log('Installing tool [{}] from resource'
> +                        ''.format(self._shortname),
> +                        hookenv.DEBUG)
> +            status_set('waiting', 'Installing from attached resource')
> +            # Move in from a temp directory to be atomic
> +            with TemporaryDirectory() as tmpdir:
> +                try:
> +                    with ZipFile(resource, 'r') as zipfile:
> +                        tmpfile = zipfile.extract(self._shortname, tmpdir)
> +                        # We could just use self.TOOLS_DIR as a destination
> +                        # here, but shutil.move refuses to overwrite the
> +                        # destination file unless it receives a full path
> +                        dst = shutil.move(tmpfile,

The wrapping here is awkward - you should be able to unwrap all of this to a single line that is less than 120 chars (especially if you change the 'if resource' above). If you don't want to do that then I'd suggest something like:

dst = os.path.join(self.TOOLS_DIR, self._shortname)
shutil.move(tmpfile, dst)

> +                                          os.path.join(self.TOOLS_DIR,
> +                                                       self._shortname))
> +                        os.chmod(dst, self.TOOLS_MODE)
> +                        os.chown(dst, uid=self.TOOLS_UID, gid=self.TOOLS_GID)
> +
> +                except BadZipFile as error:
> +                    hookenv.log('BadZipFile: {}'.format(error), hookenv.ERROR)
> +
> +                except PermissionError as error:
> +                    hookenv.log('Unable to unzip tool {} from the provided '
> +                                'resource: {}'.format(self._shortname, error),
> +                                hookenv.ERROR)
> +                except KeyError as error:
> +                    hookenv.log('Tool {} not found in the provided resource: '
> +                                '{}'.format(self._shortname, error),
> +                                hookenv.ERROR)
> +                    status_set('blocked',
> +                               'Tool {} not found'.format(self._shortname))
> +
> +        else:
> +            hookenv.log(
> +                'Missing Juju resource: tools - alternative method is not '
> +                'available yet', hookenv.ERROR)
> +            status_set('blocked', 'Missing Juju resource: tools')
> +
> +    def _remove_binary(self):
> +        binary_path = os.path.join(self.TOOLS_DIR, self._shortname)
> +        os.remove(binary_path)
> +        hookenv.log('Removed binary tool {}'.format(binary_path),
> +                    hookenv.DEBUG)
> +
> +    def _install_cronjob(self):
> +        # Copy the cronjob script to the nagios plugins directory
> +        src = os.path.join(self._files_dir, self._cronjob_script)
> +        dst = shutil.copy(src, self.NRPE_PLUGINS_DIR)
> +        os.chmod(dst, self.CRONJOB_SCRIPT_MODE)
> +        os.chown(dst,
> +                 uid=self.CRONJOB_SCRIPT_UID,
> +                 gid=self.CRONJOB_SCRIPT_GID)
> +        hookenv.log('Cronjob script [{}] copied to {}'
> +                    ''.format(self._cronjob_script, self.NRPE_PLUGINS_DIR),
> +                    hookenv.DEBUG)
> +        # Run it once to generate the temp file, otherwise the nrpe check might
> +        # fail at first
> +        subprocess.call(dst)
> +        # Now generate the cronjob file
> +        cronjob_line = '*/5 * * * * root {}\n'.format(dst)
> +        crond_file = os.path.join(self.CROND_DIR, self._shortname)
> +        with open(crond_file, 'w') as crond_fd:
> +            crond_fd.write(cronjob_line)
> +            hookenv.log('Cronjob configured at {}'.format(crond_file),
> +                        hookenv.DEBUG)
> +
> +        return dst
> +
> +    def _remove_cronjob(self):
> +        crond_file = os.path.join(self.CROND_DIR, self._shortname)
> +        cronjob_script = os.path.join(self.NRPE_PLUGINS_DIR,
> +                                      self._cronjob_script)
> +        os.remove(crond_file)
> +        os.remove(cronjob_script)
> +        hookenv.log('Removed cronjob files [{}, {}]'
> +                    ''.format(crond_file, cronjob_script),
> +                    hookenv.DEBUG)
> +
> +
> +class Sas3IrcuTool(VendorTool):

It would probably be worth adding doc comments for this class the the next two (even just summaries).

> +    def __init__(self):
> +        super().__init__(shortname='sas3ircu')
> +
> +
> +class Sas2IrcuTool(VendorTool):
> +    def __init__(self):
> +        super().__init__(shortname='sas2ircu')
> +
> +
> +class MegaCLITool(VendorTool):
> +    def __init__(self):
> +        super().__init__(shortname='megacli')
> +
> +
> +class MdadmTool(VendorTool):
> +    '''

Same as earlier:

"""Summary goes here.

Our mdadm check kind of ...
"""

> +    Our mdadm check kind of behaves like a VendorTool for the purpose of
> +    installation as it has a cronjob + check script
> +    '''
> +    def __init__(self):
> +        shortname = 'mdadm'
> +        super().__init__(shortname=shortname)
> +        self._cronjob_script = 'cron_{}.py'.format(shortname)
> +        self._nrpe_script = 'check_{}.py'.format(shortname)
> +
> +    def install(self):
> +        # mdadm should already be installed, but let's check
> +        fetch.apt_install(['mdadm'], fatal=True)
> +        self._install_cronjob()
> +        # No vendor binary to install
> +        Tool.install(self)
> +
> +    def remove(self):
> +        self._remove_cronjob()
> +        # There's no vendor binary to remove
> +        Tool.remove(self)
> +
> +
> +class IpmiTool(Tool):
> +    '''

Same here.

> +    The IPMI check does not rely on a vendor binary, it only uses freeipmi
> +    (available via the main repos) and a nrpe check that is imported as a git
> +    submodule
> +    '''
> +    def __init__(self, nrpe_opts=''):
> +        super().__init__(shortname='ipmi',
> +                         nrpe_opts=hookenv.config('ipmi_check_options'))
> +        self._nrpe_script = 'check_ipmi_sensor'
> +        self._sudoer_file = 'check_ipmi_sensor.sudoer'
> +
> +    def install(self):
> +        # Install the sudoer file
> +        self._install_sudoer()
> +        super().install()
> +
> +    def remove(self):
> +        self._remove_sudoer()
> +        super().remove()
> +
> +    def _install_sudoer(self):
> +        '''

Same here (or turn it into a straight comment:

# User nagios ...
#

> +        User nagios is normally not able to run ipmi-sel and ipmi-sensors, so
> +        we need to add a sudoer entry for it
> +        '''
> +        src = os.path.join(self._files_dir, self._sudoer_file)
> +        dst = shutil.copy(src, self.SUDOERS_DIR)
> +        os.chmod(dst, self.SUDOERS_MODE)
> +        os.chown(dst, uid=self.SUDOERS_UID, gid=self.SUDOERS_GID)
> +        hookenv.log('sudoer file for tool [{}] installed at {}'
> +                    ''.format(self._shortname, dst),
> +                    hookenv.DEBUG)
> +        return dst
> +
> +    def _remove_sudoer(self):
> +        sudoer_path = os.path.join(self.SUDOERS_DIR, self._sudoer_file)
> +        os.remove(sudoer_path)
> +        hookenv.log('deleted sudoer file'.format(sudoer_path), hookenv.DEBUG)
> +
> +
> +class HPTool(VendorTool):

Again, probably worth adding summary doc comments here and below.

> +    # ['hponcfg', 'hp-health', 'hpssacli']
> +    def __init__(self):
> +        raise NotImplementedError
> +
> +
> +class HPETool(VendorTool):
> +    # ['hponcfg', 'ilorest']
> +    def __init__(self):
> +        raise NotImplementedError
> diff --git a/src/lib/hwhealth/hwdiscovery.py b/src/lib/hwhealth/hwdiscovery.py
> index 480fea2..ba949ef 100644
> --- a/src/lib/hwhealth/hwdiscovery.py
> +++ b/src/lib/hwhealth/hwdiscovery.py
> @@ -4,20 +4,38 @@ import os
>  import re
>  import subprocess
>  
> -# import sys
> +from hwhealth import classes
>  from charmhelpers.core import host, hookenv
>  
>  
> -def get_tools():
> +def get_tools(manufacturer='auto'):
> +    if manufacturer == 'test':
> +        # Return all possible tools to aid testing
> +        return [
> +            classes.MegaCLITool(),

I'd sort this list by class name to aid in maintainability/readability.

Also, this code confirms what I noted earlier re the file name - it would be more readable to have tools.MegaCLI() here, than classes.MegaCLITool(). I'd consider renaming the file and dropping the 'Tool' suffix for all actual implementations (i.e. leave Tool and VendorTool as is).

> +            classes.Sas2IrcuTool(),
> +            classes.Sas3IrcuTool(),
> +            # classes.HPETool(), # NotImplementedYet
> +            # classes.HPTool(),  # NotImplementedYet
> +            classes.MdadmTool(),
> +            classes.IpmiTool(),
> +        ]
> +    elif manufacturer == 'auto':
> +        return _get_tools()
> +    else:
> +        raise NotImplementedError
> +
> +
> +def _get_tools():
>      tools = set()
>  
>      # MegaRAID SAS
>      if len(glob.glob('/sys/bus/pci/drivers/megaraid_sas/00*')) > 0:
> -        tools.add('megacli')
> +        tools.add(classes.MegaCLITool())
>  
>      # Huawei LSI SAS card
>      if len(glob.glob('/sys/bus/pci/drivers/mpt2sas/00*')) > 0:
> -        tools.add('sas2ircu')
> +        tools.add(classes.Sas2IrcuTool())
>  
>      # New Huawei drivers, but also Supermicro LSI SAS cards
>      pci_devs = glob.glob('/sys/bus/pci/drivers/mpt3sas/00*')
> diff --git a/src/tests/unit/test_hwdiscovery.py b/src/tests/unit/test_hwdiscovery.py
> index 06b5f95..9c184b7 100644
> --- a/src/tests/unit/test_hwdiscovery.py
> +++ b/src/tests/unit/test_hwdiscovery.py
> @@ -9,50 +9,79 @@ sys.path.append('lib/hwhealth')
>  import hwdiscovery  # noqa: E402
>  
>  
> -class TestHWDiscovery(unittest.TestCase):
> -    @mock.patch('hwdiscovery._supports_mdadm')
> -    @mock.patch('hwdiscovery.dmidecode')
> -    @mock.patch('glob.glob')
> -    @mock.patch('charmhelpers.core.hookenv.config')
> -    def test_get_tools_megacli(self, mock_hookenv, driver, dmidecode, supports_mdadm):
> +class TestGetTools(unittest.TestCase):
> +    def setUp(self):
> +        patchers = list()
> +        # Mock all Tool classes
> +        hwhealth_classes = [
> +            'HPETool',

Please sort.

> +            'HPTool',
> +            'MdadmTool',
> +            'IpmiTool',
> +            'Sas2IrcuTool',
> +            'Sas3IrcuTool',
> +            'MegaCLITool'
> +        ]
> +        for hwhealth_class in hwhealth_classes:
> +            patchers.append(mock.patch('hwdiscovery.classes.{}'
> +                                       ''.format(hwhealth_class),
> +                                       autospec=True))
> +
> +        # Provide test defaults that should make get_tools detect no tool
> +        # support. Specific hardware will be mocked in the specific tests
> +
> +        # Mock the hookenv; disable IPMI support
> +        def config_return_values(value):
> +            if value == 'enable_ipmi':

return value != 'enable_ipmi'

> +                return False
> +            else:
> +                return True
> +
> +        patchers.append(mock.patch('hwdiscovery.hookenv.config',
> +                                   autospec=True,
> +                                   side_effect=config_return_values))
> +        # Disable mdadm support
> +        patchers.append(mock.patch('hwdiscovery._supports_mdadm',
> +                                   return_value=False))
> +        # Assume a dummy 'unittest' hardware model
> +        patchers.append(mock.patch('hwdiscovery.dmidecode',
> +                                   return_value='unittest'))
> +
> +        # Start all mocks
> +        for patcher in patchers:
> +            patcher.start()
> +
> +        # Clean up all mocks at the end of the testrun
> +        self.addCleanup(mock.patch.stopall)
> +
> +    @mock.patch('hwdiscovery.glob.glob')
> +    def test_get_tools_megacli(self, driver):
>          def side_effect_glob(value):
>              if value == '/sys/bus/pci/drivers/megaraid_sas/00*':
>                  return ['/...megaraid']
>              else:
>                  return []
>  
> -        supports_mdadm.return_value = False
> -        dmidecode.return_value = 'unittest'
>          driver.side_effect = side_effect_glob
> -        mock_hookenv.return_value = True
>          real = hwdiscovery.get_tools()
> -        expected = set(['megacli', 'ipmi'])
> +        expected = set([hwdiscovery.classes.MegaCLITool()])
>          self.assertEqual(real, expected)
>  
> -    @mock.patch('hwdiscovery._supports_mdadm')
> -    @mock.patch('hwdiscovery.dmidecode')
> -    @mock.patch('glob.glob')
> -    @mock.patch('charmhelpers.core.hookenv.config')
> -    def test_get_tools_sas2ircu(self, mock_hookenv, driver, dmidecode, supports_mdadm):
> +    @mock.patch('hwdiscovery.glob.glob')
> +    def test_get_tools_sas2ircu(self, driver):
>          def side_effect_glob(value):
>              if value == '/sys/bus/pci/drivers/mpt2sas/00*':
>                  return ['/...mp2sas']
>              else:
>                  return []
>  
> -        supports_mdadm.return_value = False
> -        dmidecode.return_value = 'unittest'
> -        mock_hookenv.return_value = True
>          driver.side_effect = side_effect_glob
>          real = hwdiscovery.get_tools()
> -        expected = set(['sas2ircu', 'ipmi'])
> +        expected = set([hwdiscovery.classes.Sas2IrcuTool()])
>          self.assertEqual(real, expected)
>  
> -    @mock.patch('hwdiscovery._supports_mdadm')
> -    @mock.patch('hwdiscovery.dmidecode')
> -    @mock.patch('glob.glob')
> -    @mock.patch('charmhelpers.core.hookenv.config')
> -    def test_get_tools_sas3ircu(self, mock_hookenv, driver, dmidecode, supports_mdadm):
> +    @mock.patch('hwdiscovery.glob.glob')
> +    def test_get_tools_sas3ircu(self, driver):
>          def side_effect_glob(value):
>              if value not in ('/sys/bus/pci/drivers/megaraid_sas/00*',
>                               '/sys/bus/pci/drivers/mpt2sas/00*'):


-- 
https://code.launchpad.net/~aieri/hw-health-charm/+git/hw-health-charm/+merge/364757
Your team Nagios Charm developers is subscribed to branch hw-health-charm:oo-rewrite-integration.