Yelp / sensu_handlers

Custom Sensu Handlers to support a multi-tenant environment, allowing checks themselves to emit the type of handler behavior they need in the event json
Apache License 2.0
75 stars 31 forks source link

flexibly manage handler dependencies for pagerduty and mailer #60

Closed fessyfoo closed 9 years ago

fessyfoo commented 9 years ago

unless you've packaged it yourself there's no rubygems-redphone debian package around.

make it possible for other to meet this dependency some other way.

in the interim default to installing via gem.

somic commented 9 years ago

Internally we once discussed the idea of using sensu_gem provider for this - https://github.com/sensu/sensu-puppet/blob/master/lib/puppet/provider/package/sensu_gem.rb

With the end goal of having all sensu handlers run on ruby that ships with sensu instead of using system ruby.

solarkennedy commented 9 years ago

Yea I think long term using the sensu_gem provider is much easier to handle, with the tradeoff that it has to come "from the internet". Let me think about this a bit.

jaxxstorm commented 9 years ago

@solarkennedy the sensu_gem provider supports a "source" attribute:

https://github.com/sensu/sensu-puppet/blob/master/lib/puppet/provider/package/sensu_gem.rb#L6

All we need to do is figure out we want to mirror gems. I guess rubygems-mirror is the easiest way

https://github.com/rubygems/rubygems-mirror

fessyfoo commented 9 years ago

As an open source package I think you need to allow others to decide how to manage these dependencies. the pattern in this PR picks a way that should work for most people, and allows for it to be overridden to any other mechanism or completely disabled.

somic commented 9 years ago

@jaxxstorm rubygems-mirror could be overkill for us - I think it mirrors all gems. gems repo is very simple, can be hosted on S3 (http://blog.honeybadger.io/stop-using-rubygemsorg-in-production/), and we need just a tiny set of gems. We could discuss internally what's the best option.

somic commented 9 years ago

I think in this PR it could be beneficial not to assume that there is always going to be only one gem dependency for pagerduty handler - in other words, support more than one gem as a dependency.

And we probably are going to face similar situation for all handlers - in which case what if parameter names were more generic so that they can be used for all handlers - like for example "dependencies" instead of "pagerduty_package" and "pagerduty_package_opts"?

solarkennedy commented 9 years ago

Can we just declare that these handlers are going to use the sensu ruby and install gems using the sensu_gem provider? I think in the end that is the least painful way for people to deploy these things. This will allow us to more easily deploy this to any distroy. Also fpm'ing into debs recursively sucks.

@fessyfoo if you agree on that approach, then lets do all the conversion at once + changing the shebangs and updating the readme, and I'll ensure the transition is smooth on our ops side.

fessyfoo commented 9 years ago

@somic re keeping the same dependencies interfaces consistently across classes. I agree. [ aside: I think I was temporarily confused about scoping issues when I made it explicit. ]

re: doing multiple dependencies via a single "dependencies" param. I'm trying to think how that would look. I was thinking using ensure_packages() but that requires them all to have the same params, and I think for people who want to pin version numbers they should be able to do that. maybe with create_resources(). That said we could keep it simple and if more complex overrides are needed users would be encouraged to disable this modules management of the packages and do it themselves.

somic commented 9 years ago

@fessyfoo Yes, I was also thinking of something like create_resources('package', $dependencies)

fessyfoo commented 9 years ago

@solarkennedy re: sensu_gem i see three ways to go there:

1.) require users of this module to set EMBEDDED_RUBY=true /etc/default/sensu 2.) have this module manage wether or not EMBEDED_RUBY=true is in /etc/default/sensu 3.) hack the handler shebang lines, so they always use the embedded ruby dir.

IMO none of those sound like good ideas.

1.) is a manual dependency and leads to module not working on first try. 2.) seems not properly in the scope of this module. 3.) feels wrong to hardcode usage of the embeded ruby into handlers. it also tightly couples the installation to using sensu_gems when I think we should make dependency management optional. some net sentiment seems to agree not to hardcode embeded ruby [ https://groups.google.com/forum/#!topic/sensu-users/4aAp1JyQwPw ]

Therefore I'm suggesting we do the simplest dependency management possible and just use gem provider and the system ruby. and let people override how they see fit.

will update the PR to reflect that when I get a moment.

thoughts?

fessyfoo commented 9 years ago

updated to use dependencies pattern per @somic's feedback.

sensu_gems left out per my current opinion, pending discussion.

fessyfoo commented 9 years ago

included changes from #56 as extra examples of how this pattern would work.

solarkennedy commented 9 years ago

I think I like the pattern, but I'm really leaning toward doing the embedded sensu ruby for Yelp, and these handlers in general. This hash thing looks flexible enough to handle either case.

@fessyfoo if you pushed this as-is, would you actually use the defaults you have set here? What do you actually intend to do in production?

fessyfoo commented 9 years ago

at the moment we're using system ruby, so we'd use these defaults. though we too were discussing switching our system over to use sensu embeded ruby.

since puppet-sensu has a flag for using the embeded ruby it might make sense for this module to as well.

when I get a minute I'll draft up a PR that takes a different more supportive dep management approach for comparison. with a use_embeded_ruby flag, a spot for encoding redhat support.

fessyfoo commented 9 years ago

note #61

solarkennedy commented 9 years ago

3 ("hacking" the shebang lines) isn't a hack to me, it is... tying together these handlers to the omnibus package so that whenever you call them they work, even if you haven't set your gempath correctly..

Let's tackle that separately though.

I like this hash-based flexibility, it give us at yelp at least a way to transition without bringing down our monitoring system. Can you clean it up a bit with docs and a bit of test?

fessyfoo commented 9 years ago

@solarkennedy yes. haven't gotten back to it yet. :/

fessyfoo commented 9 years ago

closing in favor of #67