New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add an nrpe-external-master relation, with a basic check (systemd service is running) #74

Merged
merged 3 commits into from Feb 4, 2017

Conversation

Projects
None yet
4 participants
@axinojolais
Copy link
Contributor

axinojolais commented Jan 25, 2017

@mbruzek
Copy link
Contributor

mbruzek left a comment

Thanks for the contribution, I honestly appreciate it.

We do have to worry about add and remove relations so having checks for removing the relation are needed here.

hostname = nrpe.get_nagios_hostname()
current_unit = nrpe.get_nagios_unit_name()
nrpe_setup = nrpe.NRPE(hostname=hostname, primary=False)
nrpe.add_init_service_checks(nrpe_setup, services, current_unit)

This comment has been minimized.

@mbruzek

mbruzek Jan 25, 2017

Contributor

It looks like this method has a very permissive reactive clause. Is this add idempotent? Will multiple adds work?

This comment has been minimized.

@axinojolais

axinojolais Jan 26, 2017

Contributor

A "very permissive reactive clause" ? AIUI, it will only fire when the config option "nagios_context" or "nagios_servicegroups" is changed, which shouldn't happen all that often. Is that not the case ?



@when('etcd.installed')
@when('nrpe-external-master.available')

This comment has been minimized.

@mbruzek

mbruzek Jan 25, 2017

Contributor

You also need to include logic to remove the nrpe check when the relationship to etcd is removed/destroyed.

This comment has been minimized.

@axinojolais

axinojolais Jan 26, 2017

Contributor

Indeed, working on that.

@@ -25,6 +25,7 @@
from charmhelpers.core import host
from charmhelpers.fetch import apt_update
from charmhelpers.fetch import apt_install
from charmhelpers.contrib.charmsupport import nrpe

This comment has been minimized.

@mbruzek

mbruzek Jan 25, 2017

Contributor

Is this library provided by the nrpe interface?

This comment has been minimized.

@axinojolais

axinojolais Jan 26, 2017

Contributor

No, it's in charmhelpers, which is provided by hmmm the basic layer I think ? I'm not sure I understand your question.

This comment has been minimized.

@marcoceppi

marcoceppi Feb 3, 2017

Member

👍 this is included in charmhelpers

@axinojolais axinojolais force-pushed the axinojolais:nrpe branch from 022444b to 4dbf606 Jan 26, 2017

@axinojolais axinojolais force-pushed the axinojolais:nrpe branch from 4dbf606 to cd77f98 Jan 26, 2017

@axinojolais

This comment has been minimized.

Copy link
Contributor

axinojolais commented Jan 26, 2017

OK, the check will get removed upon nrpe-external-master departure with the last commit.

@lazypower

This comment has been minimized.

Copy link
Contributor

lazypower commented Feb 1, 2017

+1 LGTM

Thanks for the submission @axinojolais. If you don't get to the re-sync i'll try to pull it down and rebase for you. Thanks again!

@axinojolais

This comment has been minimized.

Copy link
Contributor

axinojolais commented Feb 2, 2017

@mbruzek @chuckbutler I just resolved the (trivial) conflict, this should be good to merge :)

Thanks

@lazypower lazypower merged commit dbce301 into juju-solutions:master Feb 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lazypower

This comment has been minimized.

Copy link
Contributor

lazypower commented Feb 4, 2017

Thanks for the contribution! This will ship with the next release of the etcd charm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment