Add redis multi instance monitor support for redis-sentinel

[basti-nis]

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
• added protected_mode only for supported versions
• fix spec tests mentioned in PR fix broken unit tests #368

[basti-nis]

Sorry for the massive commits, got some problems with unit tests.
We should do PDK Convert in this module

[basti-nis]

Maybe anybody can make some suggestions to fix the unit tests!
Thx a lot!

[ekohl]

Do I understand it right that this will end up with a single Sentinel process that monitors 1 or more masters?

[ekohl]

Wouldn't it be better to define a type alias using a struct?

[basti-nis]

you mean something like this:
type Redis::MasterName = Struct[{

instead of this:
type Redis::MasterName = Hash[String, Struct[{

?

[basti-nis]

What do u think about this:

type Redis::MasterName = Struct[{
  name                   => String,
  redis_host             => Stdlib::Host,
  redis_port             => Stdlib::Port,
  quorum                 => Integer[1],
  down_after             => Integer[1],
  parallel_sync          => Integer[0],
  failover_timeout       => Integer[1],
  auth_pass              => Optional[String],
  notification_script    => Optional[Stdlib::Absolutepath],
  client_reconfig_script => Optional[Stdlib::Absolutepath],
}]

The puppet code will look like:

     master_name   => {
       'session_6381' => {
         name             => 'redis-session-instance-6381',
         redis_host       => $redis_master_ip,
         redis_port       => 6381,
         quorum           => 2,
         parallel_sync    => 1,
         down_after       => 5000,
         failover_timeout => 12000,
         auth_pass        => $redis_auth,
       },
       'cache_6380' => {
         name             => 'redis-cache-instance-6380',
         redis_host       => $redis_master_ip,
         redis_port       => 6381,
         quorum           => 2,
         parallel_sync    => 1,
         down_after       => 5000,
         failover_timeout => 12000,
         auth_pass        => $redis_auth,
       },

The template would be changed to:

<%- | 
      $name,
      $redis_host,
      $redis_port,
      $quorum,
      $down_after,
      $parallel_sync,
      $failover_timeout,
      $auth_pass,
      $notification_script,
      $client_reconfig_script,
| -%>

sentinel monitor <%= $name%> <%= $redis_host %> <%= $redis_port %> <%= $quorum %>
sentinel down-after-milliseconds <%= $name%> <%= $down_after %>
sentinel parallel-syncs <%= $name%> <%= $parallel_sync %>
sentinel failover-timeout <%= $name%> <%= $failover_timeout %>
<% if $auth_pass { -%>
sentinel auth-pass <%= $name%> <%= $auth_pass %>
<% } -%>
<% if $notification_script { -%>
sentinel notification-script <%= $name%> <%= $notification_script %>
<% } -%>
<% if $client_reconfig_script { -%>
sentinel client-reconfig-script <%= $name%> <%= $client_reconfig_script %>
<% } -%>

Do you mean something like that?

[ekohl]

I didn't realize you already had a struct. I was mostly thinking about the documentation, since you specify all keys. Would it be better to document the type instead?

My suggestion would be to generate the reference (rake strings:generate:reference) and see how they both look like. See what makes the most sense.

[basti-nis]

I've changed the variable name to make clear, that we have an breaking change with this feature.
The handling is a little bit more user friendly.

And i run puppet strings to document the data type.

[basti-nis]

Do I understand it right that this will end up with a single Sentinel process that monitors 1 or more masters?

Yes, you are right!
I think, there is no need to create multiple instances of redis-sentinel when we can monitor 1 or more masters with one instance.
Makes more sense for me 🤔

[basti-nis]

I've successfully tested my changes on different systems.
Updated the unit test file for sentinel.

PS: sorry for the massive commits! i'm not that familiar with unit tests

Rdy 4 review!

[basti-nis]

@ekohl
pls review again

i have removed concat and use the map function, like you mentioned.

with this PR the parameter protected_mode is useless, because i implement a workaround to get all checks passed.
i would only write the config if you explicitly disabled it, if you want that i implement my idea in this PR just give me a sign.

i would do something like:

<% if @protected_mode != true -%>
protected-mode no
<% end -%>

Let me know about it

[basti-nis]

i will refork and create a new PR with changes for multi monitor support