nagios-charmers team mailing list archive

[Peter Sabaini <peter.sabaini@xxxxxxxxxxxxx>]

Hi, some random comments from the first lines of the diff -- inline.

It seems because of https://bugs.launchpad.net/launchpad/+bug/604277 I don't see all of the diff in the UI. I'm afraid you might need to break up the MP


Diff comments:

> diff --git a/Developer_notes.md b/Developer_notes.md
> new file mode 100644
> index 0000000..4abb8e6
> --- /dev/null
> +++ b/Developer_notes.md

General note: while those notes offer good overview over the rewrite, not sure they should be merged into upstream though? If so, they should have some more context / introductory text

> @@ -0,0 +1,33 @@
> +Setup
> +=====
> +
> +As the original charm (nagios-charm) found on [1] is a mix of bash and python (2) scripting, this is an attempt to
> +change that into a responsive charm.
> +In order to break it down in manageable steps, the idea is the following:
> +
> +    1 - start with an empty reactive charm
> +    2 - add hooks to support the legacy hooks
> +    3 - from the reactive charm, call the legacy hooks
> +    4 - on a 'per hook' call, transform the hook to reactive (i.e. elliminate most/all @hook decorators) by moving hooks to an interface layer and only react to status changes

Wonder if you'll find a good hook -> reactive status mapping for all hooks, may end up needing to add some from scratch

> +
> +
> +Intended improvements
> +=====================
> +
> +There are several reasons to change this charm to reactive these are:
> +
> +    1 - Only update units that are changed in the relation-{changed,broken,update} hooks for monitoring
> +    2 - Make is simpler to maintain, as it will be python 3 only
> +    3 - Add support for nagios 4
> +    
> +ad 1. There are currently 2 reasons why the charm is as slow as it is. That is, for every addition and deletion of a relation, for every unit the complete configuration is rewritten twice! So this works down to an order of 2 * N^2, while it should be possible to reduce this to order N, by giving every unit its own configuration field.

Could you elaborate what a configuration field is in this context?

> +The other performance point, is the fact that every time the relation is touched, a complete copy of the configuration directory is made and the nagios process is restarted.
> +
> +TODO List
> +=========
> +
> +    * Ensure that all 'old' hooks are triggered at the right moment (without the duplication indicated above), still todo are:
> +        * website
> +        * start
> +        * stop
> +    * Port the exisiting unittest (amulet) to the new framework
> diff --git a/Makefile b/Makefile
> index 9d48829..a7a5937 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,28 +1,50 @@
> -#!/usr/bin/make
> -PYTHON := /usr/bin/python3
> -export PYTHONPATH := hooks
> -
> -default:
> -	echo Nothing to do
> -
> -# Primitive test runner. Someone please fix this.
> -test:
> -	tests/00-setup
> -	tests/100-deploy
> -	tests/10-initial-test
> -	tests/15-nrpe-test
> -	tests/20-ssl-test
> -	tests/21-monitors-interface-test
> -	tests/22-extraconfig-test
> -	tests/23-livestatus-test
> -	tests/24-pagerduty-test
> -
> -bin/charm_helpers_sync.py:
> -	@mkdir -p bin
> -	@bzr cat lp:charm-helpers/tools/charm_helpers_sync/charm_helpers_sync.py \
> -        > bin/charm_helpers_sync.py
> -
> -sync: bin/charm_helpers_sync.py
> -	@$(PYTHON) bin/charm_helpers_sync.py -c charm-helpers.yaml
> +ifndef JUJU_REPOSITORY
> +	JUJU_REPOSITORY := $(shell pwd)
> +	$(warning Warning JUJU_REPOSITORY was not set, defaulting to $(JUJU_REPOSITORY))

^^ those two lines begin with tabs but should have spaces, otherwise make will get confused

> +endif
>  
> +help:
> +	@echo "This project supports the following targets"
> +	@echo ""
> +	@echo " make help - show this text"
> +	@echo " make submodules - make sure that the submodules are up-to-date"
> +	@echo " make lint - run flake8"
> +	@echo " make test - run the unittests and lint"
> +	@echo " make unittest - run the tests defined in the unittest subdirectory"
> +	@echo " make functional - run the tests defined in the functional subdirectory"
> +	@echo " make release - build the charm"
> +	@echo " make clean - remove unneeded files"
> +	@echo ""
>  
> +submodules:
> +	@echo "Cloning submodules"
> +	@git submodule update --init --recursive
> +
> +lint:
> +	@echo "Running flake8"
> +	@cd src && tox -e lint
> +
> +test: unittest functional lint
> +
> +unittest:
> +	@cd src && tox -e unit
> +
> +functional: build
> +	@cd src && tox -e functional
> +
> +build: submodules
> +	@echo "Building charm to base directory $(JUJU_REPOSITORY)"
> +	@-git describe --tags > ./src/repo-info
> +	@LAYER_PATH=./layers INTERFACE_PATH=./interfaces\
> +		JUJU_REPOSITORY=$(JUJU_REPOSITORY) charm build ./src --force
> +
> +release: clean build
> +	@echo "Charm is built at $(JUJU_REPOSITORY)/builds"
> +
> +clean:
> +	@echo "Cleaning files"
> +	@if [ -d src/.tox ] ; then rm -r src/.tox ; fi
> +	@if [ -d src/.pytest_cache ] ; then rm -r src/.pytest_cache ; fi
> +
> +# The targets below don't depend on a file
> +.PHONY: lint test unittest functional build release clean help submodules


-- 
https://code.launchpad.net/~woutervb/nagios-charm/+git/nagios-charm/+merge/362668
Your team Nagios Charm developers is requested to review the proposed merge of ~woutervb/nagios-charm:reactive into nagios-charm:master.