nagios-charmers team mailing list archive

[Peter Sabaini <peter.sabaini@xxxxxxxxxxxxx>]

Review: Needs Fixing

First pass -- some inline comments, mostly nitpicking. TBD: review tests

Please also check flake8 config. Fwiw tox didn't install flake8 for me, also the config checks for max-line-length=80. With the current setting of max-line-length=80 getting a few errors. Otoh the current recomm. for our projects is max-line-length=120, so could just bump that



Diff comments:

> diff --git a/src/actions/actions.py b/src/actions/actions.py
> new file mode 100755
> index 0000000..cb845dc
> --- /dev/null
> +++ b/src/actions/actions.py
> @@ -0,0 +1,56 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright 2016,2019 Canonical Ltd
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at
> +#
> +#  http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +import os
> +import sys
> +import subprocess
> +from charmhelpers.core.hookenv import action_set, action_fail, log

Style nit: group imports stdlib/3rd party, separate by newline
https://www.python.org/dev/peps/pep-0008/#imports

> +
> +
> +def clear_sel():
> +    """
> +    Action to clear the IPMI system event log, prints the content
> +    of the log before clearing it.
> +    """
> +    command = ['/usr/sbin/ipmi-sel', '--post-clear']
> +    try:
> +        output = subprocess.check_output(command)
> +        log("Action clear-sel completed, sel log cleared")

Suggest to log the output here as well

> +        action_set({"cleared_log": output})
> +    except subprocess.CalledProcessError as e:
> +        action_fail("Action failed with {}".format(e))
> +
> +
> +# A dictionary of all the defined actions to callables (which take
> +# parsed arguments).
> +ACTIONS = {"clear-sel": clear_sel}
> +
> +
> +def main(args):
> +    action_name = os.path.basename(args[0])
> +    try:
> +        action = ACTIONS[action_name]
> +    except KeyError:
> +        return "Action {} undefined".format(action_name)
> +    else:
> +        try:
> +            action(args)
> +        except Exception as e:
> +            action_fail(str(e))
> +
> +
> +if __name__ == "__main__":
> +    sys.exit(main(sys.argv))
> diff --git a/src/lib/utils/hwdiscovery.py b/src/lib/utils/hwdiscovery.py
> index 1827ac6..d68389e 100644
> --- a/src/lib/utils/hwdiscovery.py
> +++ b/src/lib/utils/hwdiscovery.py
> @@ -78,14 +81,10 @@ def _supports_mdadm():
>              for line in devices_raw.stdout.readlines():
>                  line = line.decode().strip()
>                  if devices_re.match(line):
> +                    hookenv.log("Software RAID devices found")

Maybe also log which devices have been found for troubleshooting etc.

>                      return True
> -            # FIXME: log
> -            # if devices_raw.stderr:
> -            #    print('STDERR: {}'.format(devices_raw.stderr
> -            #                              .readline().decode().strip()))
> -        except Exception:
> -            # log error
> -            pass
> +        except Exception as e:
> +            hookenv.log("mdadm scan failed with {}".format(e))
>      return False
>  
>  
> diff --git a/src/lib/utils/tooling.py b/src/lib/utils/tooling.py
> index 3b9b927..d826ff2 100644
> --- a/src/lib/utils/tooling.py
> +++ b/src/lib/utils/tooling.py
> @@ -29,26 +29,32 @@ TOOLING = {
>          'filename': 'check_mdadm.py',
>          'cronjob': 'cron_mdadm.py'
>      },
> +    'ipmi': {
> +        'filename': 'ipmi/check_ipmi_sensor',
> +        'sudoers': 'ipmi/check_ipmi_sensor_sudoer'
> +    },
>  }
>  
>  PLUGINS_DIR = '/usr/local/lib/nagios/plugins/'
>  TOOLS_DIR = '/usr/local/bin/'
>  CRONJOB_PATH = '/etc/cron.d/hw_health_{}'
> +SUDOERS_DIR = '/etc/sudoers.d'
>  
>  
>  def install_resource(tools, resource):

Suggest a docstring here and for other non-obvious and public funcs in this module

>      ntools = len(tools)
>      if ntools == 0:
>          return False
> -    elif 'mdadm' in tools:
> -        tools = [tool for tool in tools if tool != 'mdadm']
> -        ntools -= 1
> +    elif 'mdadm' or 'ipmi' in tools:

This might be a logic error as the or will be short-circuited
>>> 'mdadm' or 'x' in []
'mdadm'

Maybe better:
>>> 'mdadm' in tools or 'ipmi' in tools

Or with set() operations (also cf. below)

> +        tools = [tool for tool in tools if tool not in ['mdadm', 'ipmi']]
> +        ntools = len(tools)

This may be a matter of taste, but I'd find a set() operation more readable, e.g.:

>>> tool_set = set(tools)
>>> need_resource = tool_set - set(['ipmi', 'mdadm'])
>>> if not need_resource:
...     return True

>          if ntools == 0:
>              return True
>  
>      if not isinstance(resource, str) \
>              or not resource.endswith('.zip') \
>              or not os.path.exists(resource):
> +        hookenv.log("The resource 'tools' does not end with .zip or does not exist")
>          return False
>  
>      with tempfile.TemporaryDirectory() as tmpdir:
> diff --git a/src/reactive/hw_health.py b/src/reactive/hw_health.py
> index 1a1402f..cd8e770 100644
> --- a/src/reactive/hw_health.py
> +++ b/src/reactive/hw_health.py
> @@ -62,7 +62,17 @@ def install():
>              set_flag('hw-health.installed')
>              status.waiting('Preparing scripts installation')
>          else:
> -            status.blocked('Tools not found: {}'.format(', '.join(tools)))
> +            missing_tools = [tool for tool in tools if tool not in ['mdadm', 'ipmi']]

Consider using set ops similar to comment for L260 above

> +            status.blocked('Tools not found: {}'.format(', '.join(missing_tools)))
> +
> +
> +@when_not('ipmi-tools.installed')
> +def install_ipmi():
> +    # copy script from submodule location
> +    # add sudoers
> +    # install freeipmi
> +    # add nrpe check
> +    pass

Should this maybe log log.warning("not yet implemented")
or even
raise Exception("not yet implemented")?

>  
>  
>  @when('hw-health.installed')
> diff --git a/src/tests/unit/test_actions.py b/src/tests/unit/test_actions.py
> new file mode 100644
> index 0000000..45c4469
> --- /dev/null
> +++ b/src/tests/unit/test_actions.py
> @@ -0,0 +1,30 @@
> +# Copyright 2019 Canonical Ltd
> +#
> +# Licensed under the Apache License, Version 2.0 (the "License");
> +# you may not use this file except in compliance with the License.
> +# You may obtain a copy of the License at
> +#
> +#  http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.
> +
> +import unittest
> +import unittest.mock as mock
> +from actions.actions import clear_sel

Style nit: group imports stdlib/3rd party, separate by newline
https://www.python.org/dev/peps/pep-0008/#imports

> +
> +
> +class ClearSelTestCase(unittest.TestCase):
> +
> +    @mock.patch('subprocess.check_output')
> +    @mock.patch('subprocess.check_call')
> +    @mock.patch('charmhelpers.core.hookenv.log')
> +    def test_clear_sel(self, mock_log, mock_check_call, mock_subprocess):
> +        sel_output = "Unittest system event log output"
> +        mock_subprocess.return_value = sel_output
> +        mock_check_call.return_value = None
> +        clear_sel()
> +        mock_check_call.assert_called_once_with(['action-set', "cleared_log={}".format(sel_output)])


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