removes MySQL monitoring from Holland plugin #84
Conversation
|
#85 was merged with another change, which means this would require a rebase. Is the existing mysql plugin trustworthy enough for checking holland? I believe part of the reason for adding the mysql checks into here was so that it would verify with respect to the credentials set in the holland config rather than the defaults. I am torn between re-purposing this or adding a new more robust Holland log plugin that accepts multiple providers and maybe other options? Either way, I'll add a few line-specific comments |
| except: | ||
| print "status error unable to write to tracking file" | ||
| sys.exit(1) | ||
| else: | ||
| tracking.close() | ||
|
|
||
|
|
||
| # Finally check SQL | ||
| sql = MySQL(backupset) |
There was a problem hiding this comment.
This and the whole MySQL class could be removed.
|
If the credentials don't work, or mysql isn't online, you'll get an error in the error log when Holland tries to run, and the existing checks will say what the reason for the failure is. You have successfully monitored Holland. Currently, if MySQL is stopped, it will generate an alert, implying Holland has failed because MySQL is offline. This is not correct; if I start MySQL again the alert will clear and Holland will either run fine when it should next run (i.e. this alert was a false positive) or Holland will not run, causing additional monitoring alerts to be triggered. I'll rebase off of #85 if this functionality is desired; if it's not accepted, I don't see much point in rebasing it. I am not prepared to rewrite the plugin so that it can load the defaults file, but not the password section of /root/.my.cnf, but also including it's own password. That just feels too hack-ey. I could also see a case for trying to connect with the Holland username/password (to check credentials) without passing the defaults file at all, since we just want to test that the password works. This does leave you with a weird dichotomy where two of the stats (age and errors) are reactive, while two of the checks are proactive. It's my preference to have one or the other and let the MySQL plugin check the others. In regards to it being trustworthy, it runs the same mysqladmin commands with root that the Holland check already was, so far as I see. |
|
Additionally, I'm not convinced #85 should have been accepted If there is an exception trying to run the subprocess when a config file exists, rather than returning false, it now returns true. This is a change in behavior, as previously an exception would cause return false. In fact, exceptions for the same call if there is not a config file generate a false return as well. Unless I'm misreading this, it seems like a completely unexpected behavior. |
|
Just one comment on the exception returning true with the options file existing. I really just copied the same logic as the mysql status check over to the mysql ping check. If the exception should return false, then the status check should probably do the same. |
|
I hadn't noticed the mysql ping check having the same logic, but I don't see why it should be different. If your credentials throw an exception, saying MySQL is OK doesn't make any sense in that context either, to my mind |
|
mckraken: did you want to go with what we discussed in person, where the sql check is removed but the login check is retained? And then I think just changing the one I commented on to return false instead will put us on the same page and I'll just close mine |
|
rillip3, I'm just getting back to this. I still think there's value in the MySQL checks here. As we talked about, the credentials check has value here, the one we're not so sure about is the status/ping check. The use case I'm seeing a little more now is the pointing of Holland to remote hosts or DBaaS instances. Where do you see putting the monitoring for that case? It would make sense to put that MySQL status monitoring in the same place you are running Holland from, probably, so I'd say why not just leave it in here. |
|
I'm no longer clear on what the preferred solution is or what is needed, and this has merge conflicts since being introduced anyway. Closing this request. |
Address Issue #83 by removing MySQL checks from Holland. It is instead recommended you use the MySQL plugin in this same repository for monitoring MySQL status