Skip to content

Add swarm checks#21

Merged
timdaman merged 8 commits intotimdaman:masterfrom
wanderu:add-swarm-checks
Oct 27, 2017
Merged

Add swarm checks#21
timdaman merged 8 commits intotimdaman:masterfrom
wanderu:add-swarm-checks

Conversation

@mattwwarren
Copy link
Copy Markdown
Contributor

Closes #20

@timdaman
Copy link
Copy Markdown
Owner

Matt, thanks for the PR. Sorry for the slow response, I was traveling over the weekend.

After my initial look there a few things that do stand out.

First Travis checks are failing.
https://travis-ci.org/timdaman/check_docker/builds/275623783

Secondly I see you added multiple 'url' functions. I would rather expand the existing 'get_url' to add a return code as that would reduce the number of functions overall. To that end I created a branch which does this.
https://github.com/timdaman/check_docker/tree/return-status
What do you think of adapting your code to work with this?

Finally the documentation needs to be updated. I can take care of that if you find that annoying.

@mattwwarren
Copy link
Copy Markdown
Contributor Author

Apologies about the failing tests. I have docker running locally so I think it found the socket file. I will get those fixed up. FWIW, test_ensure_new_version fails for me locally even on master.

At a glance, your branch looks good. I will merge into mine and get that fixed up. Docs are easy and very important. I'll do my best to add to them.

Thanks!

@timdaman
Copy link
Copy Markdown
Owner

test_ensure_new_version is used as a goof check for myself. I have found myself accidentally tring to publish two releases with the same version on PyPi. You can set version = "1.0.5" to clear the alert.

@timdaman
Copy link
Copy Markdown
Owner

timdaman commented Oct 4, 2017

Matt, do you have plans to complete this PR. I think it would be able valuable improvement. Let me know if there is something I can do to help.

@mattwwarren
Copy link
Copy Markdown
Contributor Author

Hey Tim, yes, sorry. I keep saying I'm going to get this squared away as a Friday task and then Fridays run away from me.

@timdaman
Copy link
Copy Markdown
Owner

timdaman commented Oct 6, 2017

No worries, it is holiday weekend around my parts so I won't be looking at it until Wednesday or Thursday at the earliest.

@timdaman
Copy link
Copy Markdown
Owner

Matt, my week has been a zoo. Literally I wasn't/won't be home any night this week. As a result I won't be doing a deep review until this weekend but I did notice one thing.

Is it true you think the swarm checks should be exclusive, i.e. not other checks should be performed when checking swarm status and service status? That makes sense as the arguments would be for different kinds of things, containers Vs. services Vs. node status.

Looking at perform_checks() it certainly shows that style of logic but the argument parsing does not observe the same restriction. It would be better if the user got a clear message if they used incompatible options together rather than some arguments just being silently ignored.

I am also considering if, perhaps, it makes more sense to break swarm logic apart into it's own check as it really very different. What do you think? There would be check_docker & check_swarm?

@mattwwarren
Copy link
Copy Markdown
Contributor Author

No worries, Tim! My week has also been very crazy.

It is my opinion that an nrpe check should output results or statistics about only a single metric at a time. By overloading a check, the recipient of any alert has to try to parse the output and doing that at 2 or 3am is never a good time.

I agree that perhaps my separation was not done well but it is also likely I did it subconsciously.

That being said, I like your idea that it might be best to split the functions into different commands.

@timdaman
Copy link
Copy Markdown
Owner

I spent some time this weekend and split out the swarm, checks into their own script. How does this look?
https://github.com/timdaman/check_docker/tree/check_swarm

Still need to update documentation
@timdaman
Copy link
Copy Markdown
Owner

Nudge nudge. I can merge this but I figured you may want to pull it in and update your PR so you get some credit. It was your suggestion and I would hate to obscure that.

@mattwwarren
Copy link
Copy Markdown
Contributor Author

👍 I was on vacation for the last week and this didn't quite make it into my list of things to review at 30k feet. I have merged and pushed back into this PR. Good stuff. Thank you!

@timdaman timdaman merged commit 1ae45a9 into timdaman:master Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants