New class to call docker api, check-container-logs.rb features added

rockandska commented 6 years ago

Pull Request Checklist

General

[X] Update Changelog following the conventions laid out on Our CHANGELOG Guidelines
[ ] Update README with any necessary configuration snippets
[ ] Binstubs are created if needed
[X] RuboCop passes (didn't touch this offense "sensu-plugins-docker.gemspec:7:1: C: Block has too many lines. [36/25]")
[ ] Existing tests pass (didn't touch this offense "sensu-plugins-docker.gemspec:7:1: C: Block has too many lines. [36/25]")

Purpose

completely suppress the need to have docker-cli on host ( "docker ps" is not executed anymore )
defined a class to be used in all docker checks / metrics
check-container-logs.rb: have the possibility to analyze all logs without the need to give a container name + extras
metrics-docker-container.rb : add friendly name option
Maybe correct #21 if i understood it well

Known Compatibility Issues

The default host is not 127.0.0.1:2375 anymore but is defined by the arg provided to DockerApi.new first (:config[docker_host]), then by DOCKER_URL env then DOCKER_HOST env then /var/run/docker.sock. Maybe some people used 127.0.0.1:2375, but i think than most of people use /var/run/docker.sock
check-container-logs.rb : don't check logs generated with a tty anymore. logs are reversed for the check (most recent first)

Personal comments

First ruby dev First pull request Sorry if it is not done the way it should be, feel free to guide me if i need to correct some things.

Best regards

majormoses commented 6 years ago

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly.

majormoses commented 6 years ago

It looks like some of your changes were made for a newer version of rubocop than what we are testing with. You should either update the gemspec for the dependency or revert those changes to fix CI.

majormoses commented 6 years ago

Hmm I wonder about the performance implications of analyzing all containers I do not think this is a good idea, if we do we should minimally warn the user to be careful to avoid not bricking sensu.

majormoses commented 6 years ago

for something this we definitely need a test a testing artifact

eheydrick commented 6 years ago

Can you rebase to resolve the conflict with sensu-plugins-docker.gemspec. Thanks again for contributing to the project @rockandska!

rockandska commented 6 years ago

Hi,

Rubocop version update or rebase (@majormoses) : ok, got it. I will revert it to Rubocop 0.40 on my laptop and will revert my changes due to Rubocop 0.50.
Negate stdout with proc (@eheydrick) : comments on code review
Test artifact (@majormoses) : for this PR or only for check-container-logs.rb ?
Check all docker logs inside check-container-logs.rb (@majormoses) :
- It is not a breaking change and will not affect the current user of this plugin.
- It will continue to work at it is today, but check all containers if you didn't provided a container name. Indeed, the users should be careful than the check interval is less than the time took to analyse the logs
- Indeed, i didn't test it in production env, just with a few containers on my laptop.
- Which kind of test artifact would you like to see ? results based on which metrics ? ( cpu ? memory ? io ? )

majormoses commented 6 years ago

The default docker host is not 127.0.0.1:2375 anymore this makes it a breaking change. Any time you modify the default options like this it will be a breaking change as it no longer did what it was supposed to do before upgrading. If the user wants to keep the old behavior they need to update their check definition. If it breaks less than 1% of users it is still a breaking change.

Regarding testing artifacts:

as you refactored all the checks I'd like to see a testing artifact for each to ensure that they are still functional.
regarding the logs I think we should minimally output a warning if you grab logs for all containers. I would opt for something along these lines message 'collecting logs from all containers is dangerous and could lead to sensu client hanging depending on volume of logs. This is not recommended for production environments' if we want we could add a flag to silence this message but I am torn on that.

rockandska commented 6 years ago

I rebase my branch , and made some cleanup too. Not sure to have done all the things you would like to see implemented like the warning message ( @majormoses : didn't really understand if you would like juste a warning message show up or trigger the sensu warning)

I tried to write a little artifact test script too, i think it is clear and add more test is easy. You could find it here

The default docker uri was revert to don't introduce breaking changes at this point.

PS: someone already complain about it in issue, but it is really sad than the options / default are not the same for all scripts and will be great if in the next major version it will be standardized.

Feel free to ask me to undo or update this PR if there is some changes that need to be done.

Regards,

majormoses commented 6 years ago

I was fine with there being a breaking change as long as it is called out in changelog appropriately and versioned (by maintainers after accepting contribution prior to release) to ensure it does not break responsible users who pin to at least the major and read the changelog before upgrading. Also it looks like there is an unreleased breaking change. I do think there is a lot of value in making all the checks/metrics the same and this seems like a reasonable time to do it. Up to you I can either accept it as is or we can modify the defaults.

rockandska commented 6 years ago

Ok for the breaking changes. So, if it is a perfect moment to do it, let me a few days more please and i will try to push new commits to :

add a message for 'all containers'
standardized the 'docker host' , 'container name' options (-H and -n ?), other options in mind ?
let the default docker host be defined by DockerAPi Class in this order : arg passed to DockerAPI || ENV['DOCKER_URL'] || ENV['DOCKER_HOST'] || '/var/run/docker.sock'

If you have some comments about the options to be standardized or default docker host, feel free.

Thanks

majormoses commented 6 years ago

I will review this when I have some time(hopefully tonight), looks like you need to rebase.

rockandska commented 6 years ago

Rebase done. :smile: Thanks

rockandska commented 6 years ago

I've updated my test_artifact to reflect the changes in options and test 3 type of container ( running, offline, non_exist ). I didn't pay attention earlier, but it seems that bin/check-container-logs.rb was previously returning an 'OK' even if the container didn't exist, now it return a critical.

PS: sorry for this chaotic PR...

majormoses commented 6 years ago

@rockandska no worries, you are what makes the open source world go round. Can you call that out in the CHANGELOG as a Breaking Change?

majormoses commented 6 years ago

Sorry this fell off my radar, let look at the comments and see if we are ready to rebase and merge.

majormoses commented 6 years ago

@rockandska if I can get you to rebase one more time and get something in the readme about the docker host precedence I am :+1: on merging and releasing this.

Apologies again for letting this fall off my radar I had some medical complications around that time and I am still playing catch up on all the old PRs I reviewed.

rockandska commented 6 years ago

Apologies again for letting this fall off my radar I had some medical complications around that time and I am still playing catch up on all the old PRs I reviewed.

Please, don't apologize @majormoses , your health before everything else and i wish you than everything is back to normal :)

I've done the rebase and add a description to the README, hope i've done everything that was necessary.

Regards,

majormoses commented 6 years ago

released: https://rubygems.org/gems/sensu-plugins-docker/versions/3.0.0

majormoses commented 6 years ago

Thanks for all your great work

sensu-plugins / sensu-plugins-docker