nagios-charmers team mailing list archive

[Joel Sing <joel.sing@xxxxxxxxxxxxx>]

Readability/standards comments inline.

Please also get a peer review for the functionality change.

Diff comments:

> diff --git a/src/files/check_mdadm.py b/src/files/check_mdadm.py
> index 741865e..b2c8b4a 100644
> --- a/src/files/check_mdadm.py
> +++ b/src/files/check_mdadm.py
> @@ -23,14 +25,15 @@ def get_devices():
>          except Exception as e:
>              # log error
>              pass
> -    return
> +    return set()
>  
>  
> -def main():
> +def parse_output():
>      devices = get_devices()
>      if len(devices) == 0:
> -        print('WARNING: unexpectedly checked no devices')
> -        return 1
> +        msg = 'WARNING: unexpectedly checked no devices'
> +        rc = 1
> +        return (msg, rc)

Seems like msg and rc gain little here, when you could do:

return ('WARNING: ...', 1)

I also wonder if a simple Result object would not be an even better option, which would allow a default return value and class members for readability.

>  
>      mdadm_detail = ['/sbin/mdadm', '--detail']
>      mdadm_detail.extend(sorted(devices))
> @@ -75,8 +80,8 @@ def main():
>  
>      rc = devices_details_raw.wait()
>      if rc:
> -        print('WARNING: mdadm returned exit status {}'.format(int(rc)))
> -        return 1
> +        msg = 'WARNING: mdadm returned exit status {}'.format(int(rc))
> +        return (msg, 1)

This is inconsistent with the 'rc = 1; return (msg, rc)' approach used earlier (FTR I prefer this, but it should be consistent regardless)

>  
>      parts = []
>      msg = []
> @@ -88,22 +93,30 @@ def main():
>          else:
>              parts.append('{} ok'.format(device))
>  
> -        for status in sorted(devices_stats[device]['stats']):
> -            parts.append('{}[{}]'
> -                         .format(status,
> -                                 devices_stats[device]['stats'][status]))
> -            if status == 'Failed' \
> -                    and devices_stats[device]['stats'][status] > 0:
> -                critical = True
> +        if devices_stats[device]['stats'].get('Failed', 0) > 0:
> +            critical = True
> +            for status in sorted(devices_stats[device]['stats']):
> +                parts.append('{}[{}]'

This seems excessively wrapped.

> +                             .format(status,
> +                                     devices_stats[device]['stats'][status]))
>          msg.append(', '.join(parts))
> +        parts = []
>  
>      msg = '; '.join(msg)
>      if critical:
> -        print('CRITICAL: {}'.format(msg))
> -        return 2
> +        msg_tmpl = 'CRITICAL: {}'
> +        rc = 2
>      else:
> -        print('OK: {}'.format(msg))
> -        return 0
> +        msg_tmpl = 'OK: {}'
> +        rc = 0

Similar thing here - if you used an object you could automatically prefix OK/WARNING/CRITICAL and have it determine the return code based on that...

> +    return (msg_tmpl.format(msg),

This is wrapped unnecessarily.

> +            rc)
> +
> +
> +def main():
> +    msg, rc = parse_output()
> +    print(msg)
> +    sys.exit(rc)
>  
>  
>  if __name__ == '__main__':
> diff --git a/src/lib/utils/tooling.py b/src/lib/utils/tooling.py
> index 387324f..3fe4fc6 100644
> --- a/src/lib/utils/tooling.py
> +++ b/src/lib/utils/tooling.py
> @@ -8,7 +8,8 @@ from charmhelpers.core.hookenv import (
>      INFO
>  )
>  # from charms.layer import snap
> -from shutil import copy2
> +# from shutil import copy2

Remove commented imports/code.

> +import shutil
>  import subprocess
>  
>  TOOLING = {
> @@ -41,6 +42,9 @@ def install_tools(tools, method=None):
>                  except Exception as error:
>                      log('Snap install error: {}'.format(error),
>                          ERROR)
> +        elif tool == 'mdadm':
> +            # XXX(aluria): mdadm is assumed to be installed by default

A TODO would be preferable over a XXX - i.e. what actually needs to be fixed here?

> +            count += 1
>  
>      return len(tools) >= count > 0
>  
> diff --git a/src/tests/unit/test_check_mdadm.py b/src/tests/unit/test_check_mdadm.py
> new file mode 100644
> index 0000000..e9d36bf
> --- /dev/null
> +++ b/src/tests/unit/test_check_mdadm.py
> @@ -0,0 +1,119 @@
> +import mock
> +import unittest
> +import io
> +import os
> +import sys
> +
> +sys.path.append('files')
> +import check_mdadm
> +
> +
> +class TestCheckMegaCLI(unittest.TestCase):
> +    @mock.patch('os.path.exists')
> +    @mock.patch('subprocess.Popen')
> +    def test_get_devices_mdadm_exists(self, mdadm_details, path_exists):
> +        class Test_Popen(object):
> +            def __init__(cls):
> +                data = b'ARRAY /dev/md0 metadata=1.2 name=node00:0'
> +                data += b' UUID=dfaf7413:7551b000:56dd7442:5b020adb\n'
> +                data += b'ARRAY /dev/md1 metadata=1.2 name=node01:0'
> +                data += b' UUID=dfaf7413:7551b000:56dd7442:5b020adc\n'

Not a huge deal here, but be aware that Python strings are immutable, so repeated concatenation actually results in reallocation/copying.

Either a multi-line string (with """) or a test data file could be used instead.

> +                cls.stdout = io.BufferedReader(io.BytesIO(data))
> +
> +        mdadm_details.return_value = Test_Popen()
> +        path_exists.return_value = True
> +
> +        real = check_mdadm.get_devices()

Nit: actual vs expected would be probably be more idiomatic.

> +        expected = set(['/dev/md0', '/dev/md1'])
> +        self.assertEqual(real, expected)
> +
> +    @mock.patch('os.path.exists')
> +    def test_get_devices_mdadm_notfound(self, path_exists):
> +        path_exists.return_value = False
> +        real = check_mdadm.get_devices()
> +        expected = set()
> +        self.assertEqual(real, expected)
> +
> +    @mock.patch('os.path.exists')
> +    @mock.patch('subprocess.Popen')
> +    def test_get_devices_mdadm_exception(self, mdadm_details, path_exists):
> +        path_exists.return_value = True
> +        mdadm_details.side_effect = Exception
> +        real = check_mdadm.get_devices()
> +        expected = set()
> +        self.assertEqual(real, expected)
> +
> +    @mock.patch('check_mdadm.get_devices')
> +    @mock.patch('subprocess.Popen')
> +    def test_parse_output_ok(self, mdadm_details, devices):
> +        class Test_Popen(object):
> +            def __init__(cls):
> +                test_output = os.path.join(os.getcwd(),
> +                                           'tests',
> +                                           'hw-health-samples',
> +                                           'mdadm.output')
> +                cls.stdout = io.FileIO(test_output)
> +                cls.wait = lambda: 0
> +
> +        devices.return_value = set(['/dev/md0', '/dev/md1', '/dev/md2'])
> +        mdadm_details.return_value = Test_Popen()
> +        real = check_mdadm.parse_output()
> +        expected = ('OK: /dev/md0 ok; /dev/md1 ok; /dev/md3 ok; /dev/md2 ok',
> +                    0)
> +        self.assertEqual(real, expected)
> +
> +    @mock.patch('check_mdadm.get_devices')
> +    @mock.patch('subprocess.Popen')
> +    def test_parse_output_wait_warning(self, mdadm_details, devices):
> +        class Test_Popen(object):
> +            def __init__(cls):
> +                test_output = os.path.join(os.getcwd(),
> +                                           'tests',
> +                                           'hw-health-samples',
> +                                           'mdadm.output')
> +                cls.stdout = io.FileIO(test_output)
> +                cls.wait = lambda: 2
> +
> +        devices.return_value = set(['/dev/md0', '/dev/md1', '/dev/md2'])
> +        mdadm_details.return_value = Test_Popen()
> +        real = check_mdadm.parse_output()
> +        expected = ('WARNING: mdadm returned exit status 2',
> +                    1)
> +        self.assertEqual(real, expected)
> +
> +    @mock.patch('check_mdadm.get_devices')
> +    def test_parse_output_nodevices(self, devices):
> +        devices.return_value = set()
> +        real = check_mdadm.parse_output()
> +        expected = ('WARNING: unexpectedly checked no devices', 1)
> +        self.assertEqual(real, expected)
> +
> +    @mock.patch('check_mdadm.get_devices')
> +    @mock.patch('subprocess.Popen')
> +    def test_parse_output_mdadm_warning(self, mdadm_details, devices):
> +        devices.return_value = set(['/dev/md0', '/dev/md1', '/dev/md2'])
> +        mdadm_details.side_effect = Exception('ugly error')
> +        real = check_mdadm.parse_output()
> +        expected = ('WARNING: error executing mdadm: ugly error', 1)
> +        self.assertEqual(real, expected)
> +
> +    @mock.patch('check_mdadm.get_devices')
> +    @mock.patch('subprocess.Popen')
> +    def test_parse_output_degraded(self, mdadm_details, devices):
> +        class Test_Popen(object):
> +            def __init__(cls):
> +                test_output = os.path.join(os.getcwd(),
> +                                           'tests',
> +                                           'hw-health-samples',
> +                                           'mdadm.output.critical')
> +                cls.stdout = io.FileIO(test_output)
> +                cls.wait = lambda: 0
> +
> +        devices.return_value = set(['/dev/md0', '/dev/md1', '/dev/md2'])
> +        mdadm_details.return_value = Test_Popen()
> +        real = check_mdadm.parse_output()
> +        expected = ('CRITICAL: /dev/md0 ok; /dev/md1 degraded, Active[1],'
> +                    ' Failed[1], Spare[0], Working[1]; /dev/md3 ok;'
> +                    ' /dev/md2 ok',
> +                    2)
> +        self.assertEqual(real, expected)


-- 
https://code.launchpad.net/~aluria/hw-health-charm/+git/hw-health-charm/+merge/361504
Your team Nagios Charm developers is requested to review the proposed merge of ~aluria/hw-health-charm/+git/hw-health-charm:unittests-engrot6 into hw-health-charm:master.