← Back to team overview

bootstack-charmers team mailing list archive

Re: [Merge] ~guoqiao/charm-juju-lint:dev into charm-juju-lint:master

 

Hi Zachary,
Thanks for the great comments again, all fixed, except 2 issues to discuss with Jeremy:

1. how to handle lint results max age
2. where to put the tests for check script

Another review appreciated:)

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

will remove, thanks.

> +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

will remove, thanks.

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

This is just a personal python habit that always uses dict.get instead of dict['key'], yes the extra default value may cause trouble, will remove.

> +
> +
> +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):

changed to use set, thanks.

>              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(

Good point, will add.

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

Yes, I was a bit unsure about this bit too. But I am also trying to avoid adding more extra options/files for this. We can discuss later.

> +        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)

Sure, I always use datetime module when I can. But here, os.stat returns unix timestamp, which is why I am using time().

> +        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)

Yes, I wasn't sure, either. We can discuss about the reasonable way.

> +    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.


References