add sentinel multi monitor

[basti-nis]

trafficstars

Pull Request (PR) description

After multi instance was added we should let redis-sentinel watch over these instances

This Pull Request (PR) add following features and fixes the following issues

• multi instance support for redis-sentinel

This will close following Issues: Close #319 Close #223

[basti-nis]

Hey guys, this is finally ready for review! Please have a look @ekohl or @bastelfreak !

Thank you!

[bastelfreak]

A lot of changes going on here and I'm not a redis expert. @ekohl you maybe?

[basti-nis]

I'm wondering if it should be a defined type instead (redis::sentinel::instance or something). It should concat all monitored instances. The reason I think about this is that it's becoming harder to monitor.

@ekohl Good point, will change it to that name! I'll go into the 2 suggestions in this week, hope we can merge it soon :)

[basti-nis]

I'm wondering if it should be a defined type instead (redis::sentinel::instance or something). It should concat all monitored instances. The reason I think about this is that it's becoming harder to monitor.

@ekohl In a past PR you mentioned, that we should not add concat to our dependencies and further more that i should do a loop in the template. Or do i get you wrong?

redis::sentinel::instance is missleading, because it is not for sentinel instances. This PR makes it possible that your sentinel can monitor multiple redis instances.

I think it's fine as it is. Not much has changed in the template, only a loop was added. Why do you think it's harder to monitor, i can't follow your thoughts. If you could explain it to me in more detail, i would adapt it.

Otherwise we would have a solution that has been in demand for a long time, which works very well and is already being used successfully in over 10 environments.

[ekohl]

In a past PR you mentioned, that we should not add concat to our dependencies and further more that i should do a loop in the template. Or do i get you wrong?

If it's all in the same class, concat is not needed. However, if you use a defined type I think it will be needed.

redis::sentinel::instance is missleading, because it is not for sentinel instances. This PR makes it possible that your sentinel can monitor multiple redis instances.

instance is just a name I suggested. It was more about a concept that you have something that repeats itself. A defined type can make it easier to see that.

I think it's fine as it is. Not much has changed in the template, only a loop was added. Why do you think it's harder to monitor, i can't follow your thoughts. If you could explain it to me in more detail, i would adapt it.

I think passing a complex hash to a class is a painful API that's harder to understand than calling multiple defined types. In the end it'll result in the same content on disk so it's more about a usability from a developers point of view.

Does that make sense and help to elaborate my view?

[basti-nis]

@ekohl

I think passing a complex hash to a class is a painful API that's harder to understand than calling multiple defined types. In the end it'll result in the same content on disk so it's more about a usability from a developers point of view.

Does that make sense and help to elaborate my view?

Ok, that makes sense. Thanks for your detailed answer!

If it's all in the same class, concat is not needed. However, if you use a defined type I think it will be needed.

instance is just a name I suggested. It was more about a concept that you have something that repeats itself. A defined type can make it easier to see that.

You mean i should write an defined resource, that we can do something like this?:

redis::sentinel::monitor { 'my_redis_cluster':
  redis_host       => '1.2.3.4',
  redis_port       => 6881,
  down_after       => 5000,
  failover_timeout => 12000,
  quorum           => 2,
  parallel_sync    => 1,
  auth_pass        => 'secret',
}

Sorry, i think i missunderstood you in the past.

[gizmo15]

Hi,

I'm facing the same need, is there any updates ?

Thanks

[basti-nis]

Hi @gizmo15 ,

sorry, i didn't have time to write the code in the past months. Actually my branch from my forked project works in our production environment without any problems almos a year now: https://github.com/basti-nis/puppet-redis/tree/feature/sentinel_multi_monitor

If you need a quick solution, give it a try.

I will look into it, to complete the code like @ekohl suggested it, in the next couple of days.

If anybody else have an idea, feel free to contribute :)

[gizmo15]

Hi, Thanks for you answer and your work! we will check that!

[vox-pupuli-tasks[bot]]

Dear @basti-nis, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks