-
Notifications
You must be signed in to change notification settings - Fork 77
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 nrpe check tests for manila-ganesha charm #806
Add nrpe check tests for manila-ganesha charm #806
Conversation
- Check if the custom plugins are being installed - Check if the custom cronjobs are being installed - Check if the NRPE services are being added. - The tests will not be ran if nrpe does not exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see inline; also note that the PR description needs filling out too. Thanks.
logging.warn("Skipping as nrpe is not deployed.") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use self.skipTest(...)
to skip a test.
try: | ||
zaza.model.get_application("nrpe") | ||
except KeyError: | ||
logging.warn("Skipping as nrpe is not deployed.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use self.skipTest(...)
to skip a test.
zaza.model.get_application("nrpe") | ||
except KeyError: | ||
logging.warn("Skipping as nrpe is not deployed.") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use self.skipTest(...)
to skip a test.
cmds = [] | ||
for check_name in plugins: | ||
cmds.append(f'ls /usr/local/lib/nagios/plugins/{check_name}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nicer (and more efficient) to use a list comprehension here:
cmds = [f'ls ... {check_name}' for check_name in plugins]
cmds = [] | ||
for check_name in plugins: | ||
cmds.append(f'ls /etc/cron.d/nagios-check_{check_name}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, please think of a list comprehension here.
cmds = [] | ||
for check_name in services: | ||
cmds.append( | ||
'egrep -oh /usr/local.* /etc/nagios/nrpe.d/' | ||
'check_{}.cfg'.format(check_name) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
for attempt in tenacity.Retrying( | ||
wait=tenacity.wait_fixed(20), | ||
stop=tenacity.stop_after_attempt(2), | ||
reraise=True | ||
): | ||
with attempt: | ||
ret = generic_utils.check_commands_on_units(cmds, units) | ||
self.assertIsNone(ret, msg=ret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is repeated 3 times; please consider a refactor to pull this into a helper function.
Thanks for the comments. I have refactored the code and updated the description of the PR, please have a look again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; that's a really nice refactor of the original patch and it is much easier to understand now. Thanks.
] | ||
|
||
commands = [ | ||
f"ls /usr/local/lib/nagios/plugins/{plugin}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the py35 test run:
zaza/openstack/charm_tests/manila_ganesha/tests.py:88:56: E999 SyntaxError: invalid syntax
Py3.5.sadly doesn't support f-strings. You'll need to use .format(...)
here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for the hints!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline; unfortunately f-strings are not compatible with Python 3.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for the update to fix the pep8 error.
- added 3 basic checks to check: nfs connection, nfs services, and nfs shares - updated test bundles to include nrpe and nrpe-external-master for functional tests. func-test-pr: openstack-charmers/zaza-openstack-tests#806 Closes-bug: #1925975 Change-Id: I4d3736300f75b9811f4f6525b5454c7e495ef566
* Update charm-manila-ganesha from branch 'master' to 65ca408318ac637ef4787e61d5ec48f4d4477502 - Added NRPE checks for manila-ganesha charm. - added 3 basic checks to check: nfs connection, nfs services, and nfs shares - updated test bundles to include nrpe and nrpe-external-master for functional tests. func-test-pr: openstack-charmers/zaza-openstack-tests#806 Closes-bug: #1925975 Change-Id: I4d3736300f75b9811f4f6525b5454c7e495ef566
This PR adds nrpe check tests for manila-ganesha charm. It's accompanied by the new features proposed in https://review.opendev.org/c/openstack/charm-manila-ganesha/+/848219