Code review comment for lp://qastaging/~brad-marshall/charms/trusty/ceilometer/add-nrpe-checks

Revision history for this message
Liam Young (gnuoy) wrote :

Thank you for the mp, the nagios checks are sorely needed. It looks fine, there just a few things it would be good to get fixed up.

The list of services that comprise a ceilometer deployment are already compiled as part of the service context in ceilometer_utils.py. This includes logic to adjust the list depending on what Openstack release is being deployed. I think this mechanism should be used rather than defining a new list directly in update_nrpe_config()

That being said, it looks like the existing charm has a list of new icehouse packages which are being added to the package list in ceilometer_utils.py but the corresponding services are not being added to the CONFIG_FILES OrderedDict. This means that the ceilometer-alarm* and ceilometer-agent-notification services are not going to be restarted on changes to ceilometer.conf

So, what I think is needed is:
1) Steal the services() method from nova-cloud-controller and use that to get a list of services in update_nrpe_config()
2) Define a list of ICEHOUSE_SERVICES (probably exactly the same as ICEHOUSE_PACKAGES) and conditionally add (depending on ostack release) to the CEILOMETER_CONF service in register_configs():
    if (get_os_codename_install_source(config('openstack-origin'))
            >= 'icehouse'):
        CONFIG_FILES[CEILOMETER_CONF]['services'] = CONFIG_FILES[CEILOMETER_CONF]['services'] + ICEHOUSE_SERVICES

review: Needs Fixing

« Back to merge proposal