bootstack-charmers team mailing list archive

[Zachary Zehring <zachary.zehring@xxxxxxxxxxxxx>]

Review: Needs Fixing

Added more inline comments. I did notice there aren't any unit tests for the new NRPE bits added to reactive/charm_juju_lint.py . Are there unit tests for that module in this project?

Diff comments:

> diff --git a/reactive/charm_juju_lint.py b/reactive/charm_juju_lint.py
> index c3ad41c..8e8b0b7 100644
> --- a/reactive/charm_juju_lint.py
> +++ b/reactive/charm_juju_lint.py
> @@ -16,62 +16,81 @@ this program.  If not, see <https://www.gnu.org/licenses/>.
>  """
>  
>  import json
> +import subprocess
>  from os import (
> -    getenv,
> -    mkdir,
> +    makedirs,
>      path,
>  )
> -import virtualenv
>  
>  from charmhelpers.core import hookenv
>  from charmhelpers.core.host import rsync
> -from charmhelpers.fetch.python import packages
> +from charmhelpers.contrib.charmsupport import nrpe
>  from charms.reactive import (
>      when,
>      when_not,
> +    when_any,
>      set_flag,
> +    clear_flag,
>  )
>  
>  USR_LIB = "/usr/lib/juju-lint"
>  VAR_LIB = "/var/lib/juju-lint"
> +# auto_lint.py write results to here, check_juju_lint_results.py check it

Not necessary. I'd put something like this in a doc string for the actual check file. Here seems kind of unimportant.

> +LINT_RESULTS_FILE = path.join(VAR_LIB, 'lint-results.txt')
> +NAGIOS_PLUGINS_DIR = '/usr/local/lib/nagios/plugins/'
> +# where auto_lint.py will be installed

Redundant, so I'd remove. Var name conveys it fine.

> +AUTO_LINT_SCRIPT_PATH = path.join(USR_LIB, 'auto_lint.py')
> +
> +
> +CONFIG = hookenv.config()
> +LINT_FREQUENCY = CONFIG.get("lint-frequency", 30)

Not sure why lint-frequency is getting it's own "default" value here rather than in the config.yaml. I guess if user unsets value themselves?

> +
> +
> +def get_nagios_plugin_path(name):
> +    """
> +    Given nagios script name, return full path.
> +
> +    check_xxx.py --> '/usr/local/lib/nagios/plugins/check_xxx.py'
> +    """
> +    return path.join(NAGIOS_PLUGINS_DIR, name)
>  
>  
>  @when_not("charm-juju-lint.installed")
>  def install_charm_juju_lint():
>      create_install_dirs()
> -    virtualenv.create_environment(USR_LIB)
> -    packages.pip_install("juju", venv=USR_LIB)
>      deploy_scripts()
> +    create_crontab()
>      hookenv.status_set("active", "Unit is ready")
>      set_flag("charm-juju-lint.installed")
>  
>  
>  def create_install_dirs():
> -    install_dirs = (USR_LIB, VAR_LIB)
> +    install_dirs = (USR_LIB, VAR_LIB, NAGIOS_PLUGINS_DIR)
>      for install_dir in install_dirs:
> -        if not path.isdir(install_dir):
> -            try:
> -                mkdir(install_dir)
> -            except OSError as error:
> -                print("{} directory creation failed: {}".format(
> -                    install_dir, error
> -                ))
> +        makedirs(install_dir, exist_ok=True)
>  
>  
>  def deploy_scripts():
> -    rsync(
> -        path.join(getenv("CHARM_DIR"), "scripts", "auto_lint.py"),
> -        path.join(USR_LIB, "auto_lint.py")
> -    )
> +    """
> +    Install scripts.
> +
> +    NOTE: trailing slash(/) here is important for rsync.
> +    which means all content in dir other than dir itself.
> +    """
> +    charm_dir = hookenv.charm_dir()
> +    scripts_dir = path.join(charm_dir, 'scripts/')
> +    rsync(path.join(scripts_dir, "auto_lint.py"), AUTO_LINT_SCRIPT_PATH)
> +    # install all files in scripts/plugins/ into nagios plugins dir
> +    plugins_dir = path.join(scripts_dir, 'plugins/')
> +    rsync(plugins_dir, NAGIOS_PLUGINS_DIR)
>  
>  
>  @when("charm-juju-lint.installed", "config.changed")
>  def verify_config():
> -    config = hookenv.config()
>      required = ("controller-endpoint", "controller-username",
>                  "controller-password", "controller-cacert", "model-uuid")
>      for option in required:
> -        if not config[option]:
> +        if not CONFIG.get(option, None):

Remove None arg from get() as it's redundant (would return None anyways with no args). 

Also, can simplify this one a lot. Can use some set logic (like make required list a set and see if that's friendly with CONFIG). Or can use any() builtin like: 

if any([option in CONFIG for option in required]):
    # do rest

>              hookenv.status_set("active", "Set model connection config values")
>              return
>      hookenv.status_set("active", "Unit is ready")
> @@ -83,15 +102,78 @@ def create_auto_lint_config():
>      "/var/lib/juju-lint/auto-lint-config.json" containing charm config options.
>      """
>      with open(path.join(VAR_LIB, "auto-lint-config.json"), "w") as fp:
> -        json.dump(hookenv.config(), fp)
> +        json.dump(CONFIG, fp)
>  
>  
>  @when("charm-juju-lint.installed", "config.changed")
>  def create_crontab():
>      """Create a crontab at "/etc/cron.d/juju-lint" that runs "auto_lint.py"."""
>      cron_job = "*/{} * * * * root {}\n".format(
> -        hookenv.config()["lint-frequency"],
> -        path.join(USR_LIB, "auto_lint.py")
> +        LINT_FREQUENCY,
> +        AUTO_LINT_SCRIPT_PATH,
>      )
>      with open("/etc/cron.d/juju-lint", "w") as fp:
>          fp.write(cron_job)
> +
> +
> +@when("nrpe-external-master.initial_config")
> +@when_any("config.changed.nagios_context", "config.changed.nagios_servicegroups")
> +def update_nrpe_config():
> +    """On relation config changed"""
> +    nrpe_client = nrpe.NRPE()
> +
> +    shortname = 'juju_lint_results'
> +    check_cmd = '{} --lint-results-file {} --lint-results-max-age {}'.format(

This command will have to be updated when LINT_FREQUENCY changes, correct? Probably need to add the config.changed.lint_frequency flag to the @when_any condition.

> +        get_nagios_plugin_path('check_juju_lint_results.py'),
> +        LINT_RESULTS_FILE,
> +        # allow extra 5 mins to generate lint-results.txt

Hmm may want to get Jeremy's thoughts on this. Kind of makes the frequency the user sets to be incorrect.

> +        LINT_FREQUENCY + 5,
> +    )
> +    nrpe_client.add_check(
> +        shortname=shortname,
> +        description='check juju lint results file genrated by juju-lint',
> +        check_cmd=check_cmd,
> +    )
> +    hookenv.log('check_{} added'.format(shortname))
> +    nrpe_client.write()
> +    set_flag("nrpe-external-master.configured")
> +
> +
> +def run_auto_lint():
> +    """Run auto_lint.py to generate lint results file"""
> +    try:
> +        subprocess.check_call([AUTO_LINT_SCRIPT_PATH])
> +    except subprocess.CalledProcessError as e:
> +        hookenv.log('fail to run auto_lint.py: {}'.format(e), 'ERROR')
> +        hookenv.status_set('blocked', 'fail to run {}'.format(AUTO_LINT_SCRIPT_PATH))
> +
> +
> +@when("nrpe-external-master.available")
> +@when_not("nrpe-external-master.initial_config")
> +def initial_nrpe_config():
> +    """
> +    On relation joined.
> +
> +    This hook will be called when relation joined, normally once only unless
> +    rejoin. At this point, check will began to run at about every 5 mins by
> +    nagios. However, the cron job interval is 30 mins by default and may be
> +    changed to even longer, e.g.: 24h. So cron job may not run yet, and check
> +    has no lint_results.txt file to check against, which will trigger CRITICAL
> +    alert in nagios by our design.
> +
> +    To avoid above fake alert, trigger auto_lint.py here if lint results file
> +    not found yet.
> +    """
> +    update_nrpe_config()
> +    if not path.isfile(LINT_RESULTS_FILE):
> +        run_auto_lint()
> +    set_flag("nrpe-external-master.initial_config")
> +
> +
> +@when_not("nrpe-external-master.available")
> +@when("nrpe-external-master.configured")
> +def remove_nrpe_config():
> +    """On relation depart"""
> +    nrpe_client = nrpe.NRPE()
> +    nrpe_client.remove_check(shortname='juju_lint_results')
> +    clear_flag('nrpe-external-master.configured')
> diff --git a/scripts/plugins/check_juju_lint_results.py b/scripts/plugins/check_juju_lint_results.py
> new file mode 100755
> index 0000000..8f963b2
> --- /dev/null
> +++ b/scripts/plugins/check_juju_lint_results.py
> @@ -0,0 +1,104 @@
> +#!/usr/bin/env python3
> +
> +"""
> +# check_juju_lint_results.py
> +
> +Nagios plugin script to check juju lint results.
> +
> +Copyright (C) 2020 Canonical Ltd.
> +Copyright (C) 2020 Joe Guo <joe.guo@xxxxxxxxxxxxx>
> +
> +This program is free software: you can redistribute it and/or modify it under
> +the terms of the GNU General Public License version 3, as published by the Free
> +Software Foundation.
> +
> +This program is distributed in the hope that it will be useful, but WITHOUT ANY
> +WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A
> +PARTICULAR PURPOSE.  See the GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License along with
> +this program.  If not, see <https://www.gnu.org/licenses/>.
> +"""
> +
> +import argparse
> +import sys
> +import os
> +from time import time
> +from os.path import join, isfile
> +
> +VAR_LIB = "/var/lib/juju-lint"
> +LINT_RESULTS_FILE = join(VAR_LIB, 'lint-results.txt')
> +
> +NAGIOS_STATUS_OK = 0
> +NAGIOS_STATUS_WARNING = 1
> +NAGIOS_STATUS_CRITICAL = 2
> +NAGIOS_STATUS_UNKNOWN = 3
> +
> +NAGIOS_STATUS = {
> +    NAGIOS_STATUS_OK: 'OK',
> +    NAGIOS_STATUS_WARNING: 'WARNING',
> +    NAGIOS_STATUS_CRITICAL: 'CRITICAL',
> +    NAGIOS_STATUS_UNKNOWN: 'UNKNOWN',
> +}
> +
> +
> +def nagios_exit(status, message):
> +    assert status in NAGIOS_STATUS, "Invalid Nagios status code"
> +    output = '{}: {}'.format(NAGIOS_STATUS[status], message)
> +    print(output)  # nagios requires print to stdout, no stderr
> +    sys.exit(status)
> +
> +
> +def main():
> +    parser = argparse.ArgumentParser(
> +        description='check juju lint results',
> +        # show default in help
> +        formatter_class=argparse.ArgumentDefaultsHelpFormatter)
> +
> +    parser.add_argument(
> +        '-f', '--lint-results-file',
> +        dest='lint_results_file',
> +        default=LINT_RESULTS_FILE,
> +        help='juju lint results file to check')
> +
> +    parser.add_argument(
> +        '-a', '--lint-results-max-age',
> +        dest='lint_results_max_age',
> +        type=int, default=60,
> +        help='juju lint results file max age in minutes, 0 to ignore')
> +
> +    args = parser.parse_args()
> +    lint_results_file = args.lint_results_file
> +
> +    # raise CRITICAL alert if lint results file missing
> +    # which implies cron job may stop working
> +    if not isfile(lint_results_file):
> +        message = 'lint results file not found: {}'.format(lint_results_file)
> +        nagios_exit(NAGIOS_STATUS_CRITICAL, message)
> +
> +    max_age = args.lint_results_max_age
> +    if max_age > 0:  # if <= 0, skip
> +        stat = os.stat(lint_results_file)
> +        age_sec = int(time() - stat.st_mtime)

You may want to check out datetime lib for dealing with time deltas. I've found it very nice to work with, and supports the usual +, -, >, < operations. Can help simplify this section. not a big deal though.

> +        max_age_sec = max_age * 60
> +        if age_sec > max_age_sec:
> +            message = 'lint results file {} is older than max age {} mins'.format(
> +                lint_results_file, max_age)
> +            nagios_exit(NAGIOS_STATUS_CRITICAL, message)
> +
> +    with open(lint_results_file) as txt:
> +        lint_results = txt.read()
> +
> +    if 'ERROR' in lint_results or 'CRITICAL' in lint_results:
> +        message = 'juju-lint errors\n{}'.format(lint_results)
> +        nagios_exit(NAGIOS_STATUS_CRITICAL, message)
> +    elif 'WARNING' in lint_results:
> +        message = 'juju-lint warnings\n{}'.format(lint_results)
> +        nagios_exit(NAGIOS_STATUS_WARNING, message)
> +    else:
> +        message = 'juju lint is happy\n{}'.format(lint_results)
> +        nagios_exit(NAGIOS_STATUS_OK, message)
> +
> +
> +if __name__ == "__main__":
> +    main()
> diff --git a/unit_tests/test_juju_lint.py b/unit_tests/test_juju_lint.py
> new file mode 100755
> index 0000000..3d76828
> --- /dev/null
> +++ b/unit_tests/test_juju_lint.py
> @@ -0,0 +1,62 @@
> +#!/usr/bin/env python3
> +
> +import unittest
> +import subprocess
> +from os.path import abspath, dirname, join
> +
> +CURRENT_DIR = dirname(abspath(__file__))
> +CHARM_DIR = dirname(CURRENT_DIR)
> +SCRIPT_DIR = join(CHARM_DIR, 'scripts')
> +LINT_RESULTS_DIR = join(CURRENT_DIR, 'lint_results')
> +CHECK_JUJU_LINT_RESULTS_PATH = join(SCRIPT_DIR, 'check_juju_lint_results.py')
> +
> +
> +def get_lint_results_file(name):
> +    """
> +    Get full file path from name.
> +
> +    ERROR.txt --> /path/to/unit_tests/lint_results/ERROR.txt
> +    """
> +    return join(LINT_RESULTS_DIR, name)
> +
> +
> +def get_check_exit_code(name, max_age=0):
> +    """
> +    Return check script exit code for a file name.
> +    """
> +
> +    cmd = [
> +        CHECK_JUJU_LINT_RESULTS_PATH,
> +        '--lint-results-file={}'.format(get_lint_results_file(name)),
> +        '--lint-results-max-age={}'.format(max_age),
> +    ]
> +    print(' '.join(cmd))
> +
> +    try:
> +        subprocess.check_call(cmd)

Not sure how I feel about doing actual subprocess calls in unit tests, mostly due to speed of the running tests. This would fit more as functional, albeit not necessarily charm functional but script functional). Not sure exactly we as a team handle this. I'd definitely keep this test case, but I wouldn't qualify these as unit tests. Maybe something to bring up with Jeremy.

> +    except subprocess.CalledProcessError as e:
> +        return e.returncode
> +    return 0
> +
> +
> +class CharmJujuLintTestCase(unittest.TestCase):
> +
> +    def test_info(self):
> +        self.assertEqual(get_check_exit_code('INFO.txt'), 0)
> +
> +    def test_warning(self):
> +        self.assertEqual(get_check_exit_code('WARNING.txt'), 1)
> +
> +    def test_error(self):
> +        self.assertEqual(get_check_exit_code('ERROR.txt'), 2)
> +
> +    def test_critial(self):
> +        self.assertEqual(get_check_exit_code('CRITICAL.txt'), 2)
> +
> +    def test_lint_results_file_missing(self):
> +        """If lint results file missing, raise CRITICAL alert"""
> +        self.assertEqual(get_check_exit_code('NOT-EXIST.txt'), 2)
> +
> +    def test_lint_results_file_max_age(self):
> +        """If lint results file too old, raise CRITICAL alert"""
> +        self.assertEqual(get_check_exit_code('INFO.txt', max_age=1), 2)


-- 
https://code.launchpad.net/~guoqiao/charm-juju-lint/+git/charm-juju-lint/+merge/377959
Your team Canonical BootStack Charmers is subscribed to branch charm-juju-lint:master.