bootstack-charmers team mailing list archive
-
bootstack-charmers team
-
Mailing list archive
-
Message #00003
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