Support for multiple broker connection options with RabbitMQ

[royalj]

Since Sensu 0.15 multiple broker connections with RabbitMQ have been supported to provide cluster failover without the need for a load balancer. It would be nice if this could be supported in the puppet module as well so we can start utilizing this feature.

[jlambert121]

@royalj would you be able to take a swing at a PR for this?

[royalj]

Ya, I'll have a go at it and see what I come up with.

[dblessing]

@jlambert121 i looked at this issue with @royalj and it appears it won't be so easy. Do you know why the module implements a custom type/provider for configuration when it could be a fairly simple template? I see no way to implement this without breaking changes. I'm not even sure this can be logically implemented within the type/provider. The configuration needs an array of multiple configuration fields (host, port, auth, etc)

[jlambert121]

@dblessing It uses a custom type/provider because that's how it was when I started working on it. I've considered replacing it with templates, just haven't ever. We aren't doing any complex logic so I'm not sure why it was implemented this way. @jamtur01 any insight?

[dblessing]

Ok, that sounds good. Would you accept a PR for moving just the config file to a template? It will likely cause breaking changes, but I think it would be logical and with some documentation we can alleviate too much pain.

[jamtur01]

The original design of those was @rodjek - he can probably shed the most light on it but my pennies worth is that templates actually become quite complex and hard to test at scale. Also remember you're only having this issue in one place. Might it not be better to try to fix this rather than introduce a whole new configuration method?

[jlambert121]

I've thought a few times about ripping out all of the types for checks and everything. The JSON files are nicely broken apart, where do you foresee issues? There also aren't any tests for the providers right now - not that that is a reason to rip it out.

[dblessing]

@jamtur01 The config is so dead simple I don't think we'll see any scaling issues. This is the RabbitMQ config - pretty straightforward.

{
  "rabbitmq": {
    "host": "sensu-mq1.example.com",
    "password": "secret",
    "vhost": "sensu",
    "port": 5672,
    "user": "sensu"
  }
}

I don't think changing the provider to accommodate multiple RabbitMQ is going to work. The issue is that you need an array of hashes. You can probably make it work but it's going to be a complete rewrite and add complexity.

{
  "rabbitmq": [
    {
      "host": "sensu-mq1.example.com",
      "password": "secret",
      "vhost": "sensu",
      "port": 5672,
      "user": "sensu"
    },
    {
      "host": "sensu-mq2.example.com",
      "password": "secret",
      "vhost": "sensu",
      "port": 5672,
      "user": "sensu"
    }
  ]
}

[jamtur01]

An array of hashes in the provider shouldn't be that hard IMHO - I'm on a plane this week so I'm happy to take a stab at it and I've pinged @rodjek to see if he's got thoughts/feelings too.

[dblessing]

Ok, take a shot at it @jamtur01. I still feel it's entirely over-engineered but if we can get support, we'll take it.

[jamtur01]

@dblessing I feel some of the providers probably are over-engineered but not all of them I guess - I just said to @jlambert121 that I am not adverse to replacing simpler ones with templates but wholesale replacement? Ick. Eww. And "please no". :) I also have bad memories of regressing to templates and losing the test-ability and introspection of native types/providers. Content => blah is not a solution - it's a hack. :P

[jamtur01]

I haven't had a chance to address this and it's unlikely I will. I'd be happy to take a patch replacing the provider with a template.

[agoddard]

@dblessing did you end up having a play with this?

[roobert]

Hi, I came here looking for this feature since it's now the recommended method to do HA Sensu.

I forked 2.0.0 (I tried forking master but was getting various errors) and adjusted the sensu::rabbitmq::config class to use a template if sensu::rabbitmq_hosts is set which should maintain backwards compatibility for existing deployments and allow people to define multiple servers if they need to.

In terms of implementation detail: the template first checks for unique configuration per-node and then falls back to checking for common values since it's assumed that when using rmq in a cluster configuration you'll have configured each server with the same certs, etc.

I'm not sure whether I should open a PR for this or not since it probably needs rebase'ing and I haven't added any tests or anything. Opinions welcome.

Cheers,

edit: link! https://github.com/roobert/sensu-puppet

[dalesit]

I've spun up the Vagrant instance, copied in the changes to the module, and added the lines

   use_embedded_ruby => true,
   rabbitmq_hosts    => ['localhost','puppet'],

to the sensu-server.pp file in tests.

Doing a puppet apply sensu-server.pp generates this error:

Error: Evaluation Error: Error while evaluating a Function Call, Failed to parse template sensu/rabbitmq.conf.erb:
  Filepath: /etc/puppet/modules/sensu/templates/rabbitmq.conf.erb
  Line: 5
  Detail: undefined method `has_key?' for "localhost":String
 at /etc/puppet/modules/sensu/manifests/rabbitmq/config.pp:125:18 on node sensu-server.example.com
Error: Evaluation Error: Error while evaluating a Function Call, Failed to parse template sensu/rabbitmq.conf.erb:
  Filepath: /etc/puppet/modules/sensu/templates/rabbitmq.conf.erb
  Line: 5
  Detail: undefined method `has_key?' for "localhost":String
 at /etc/puppet/modules/sensu/manifests/rabbitmq/config.pp:125:18 on node sensu-server.example.com

Any thoughts on where the error might be?

[roobert]

rabbitmq_hosts expects an (edit: array of hashes), see the template: https://github.com/roobert/sensu-puppet/blob/sensu_multi_rmq/templates/rabbitmq.conf.erb

Here's a (valid) example hiera config:

sensu::rabbitmq_hosts:
  - host: "sensu0.example.net"
    ssl: true
  - host: "sensu1.example.net"
    ssl: true
  - host: "sensu2.example.net"
    ssl: true

[cwjohnston]

@roobert I'm very interested in seeing rabbitmq host configurations as an array be supported by this module. Any chance you'll be opening a PR?

[roobert]

@cwjohnston: my patch would need tests adding and to be rebased against master which I'm hesitant to do before @jlambert121 / @jamtur01 comment on implementation. I'm also not confident that this is the best way or even a good way to implement this functionality, I just needed something that worked quickly. Since this issue has been open for over a year I don't want to do additional work on this unless I know it'll be worth while, i.e: merged into master.

[jlambert121]

@roobert without seeing the patch (there isn't a PR I missed is there?) I can't say for 100% certainty that I'd merge it, but I'm not opposed to the feature. If it isn't a breaking change and is written similar to the rest of the code base with tests, it should be an easy thing to get in. If it is a breaking change, we'd need to make it part of a 3.0 release.

[roobert]

@jlambert121, ah, I linked to my fork but not an actual diff, here's the changes I made to the 2.0.0 release: https://github.com/roobert/sensu-puppet/compare/bd58e836f7959a9475ffc9d0dcda8bfbf055987d...master

[jlambert121]

@roobert I totally missed the link to your fork, thanks for making it obvious for me.

I'm fine with this approach.

I wonder if it makes sense to just drop the sensu_rabbitmq_config type/provider so we don't have two paths for configuring the same file (if rabbitmq_hosts is unset it would be easy enough to build the single host into an array and deprecate those settings). I can do that refactor later too if you want.

[roobert]

@jlambert121 that makes sense to me. Feel free to refactor or whatever, I'm not precious. If you'd like me to make initial changes or anything then let me know how I can help and I should be able to get to it next week.

[dalesit]

I've made some changes based on Rob's fork which allows either providing a hash with settings, or just an array of servers utilising the same settings as set for a single host.

It is in the cluster_config branch of my fork below. I am in the process of updating the vagrant files to test with a cluster.

It uses a Boolean $rabbitmq_cluster to determine whether to use $rabbitmq_cluster_hosts as an array of servers, or $rabbitmq_cluster_custom as an array of hashes (using Rob's approach).

The $rabbitmq_cluster_hosts also works as an array of one.

https://github.com/dalesit/sensu-puppet/tree/cluster_config?files=1

[dalesit]

#485 raised - comments and suggestions welcome...

[roobert]

There's a bit more discussion on the ticket @dalesit referred to for those that are interested..

[dalesit]

Just merged in the latest changes from #490 into #485

[gbolo]

let's get these pulls merged. We really need support for multiple rabbitmq servers in the sensu puppet module

[dalesit]

I've merged in the latest changes from #494 and #498 into #485 - not sure why, but some of the travis builds utilising Ruby 2.1.6 and Puppet 4 failed, but doesn't appear to be on anything I have touched - has there been a backend change which is causing these builds to fail? Previous builds worked fine.

[dalesit]

@jlambert121 is there something holding up this PR that I could work on? It would be useful to get support for multiple rabbitmq servers into the module, and this issue has been open for ages now...

[jaxxstorm]

Deprecated in favour of #581