Sentinel: refactorize to use ::redis

[EmilienM]

In previous commits, usage of redis::sentinel made impossible to run
::redis class which is the Puppet way to run this module.

This patch keeps feature parity but clean-up the implementation to use
::redis and configure Sentinel with the same configuration file as
regular system.

The rspec tests show that we can use ::redis and have Redis Sentinel
configuration that is expected.

[cdent]

Does this still preserve the ability to run a sentinel without running a redis server? Separate configs and services was a goal of my original patches.

Also, does it ensure required write perms on conf file?

I'm in bed on my phone, otherwise Id check. Will check things out tomorrow.

[cdent]

Also with the exec cp removed, each time puppet is run sentinel's state, which is kept in the conf file, will be lost.

That or a similar mechanism must be in place. It is why sentinel+puppet is so messy.

[EmilienM]

@cdent yes, you can run sentinel without running a redis server thanks to "service_name" which will only manage redis-sentinel service.
Same for write perms on config file, look at unit tests I did.
Separate config is a bit hacky, same for doing a "cp" in a manifest. I never saw this before.
As far I understand, Sentinel is a service that runs a config, exactly like does Redis.
Look at the unit tests I wrote and you'll see you have the same result by using ::redis class with the right parameters.

[cdent]

Separate config is a bit hacky, same for doing a "cp" in a manifest. I never saw this before.
As far I understand, Sentinel is a service that runs a config, exactly like does Redis.

I'll do some testing on the other issues, but this problem with the cp is something we'll need to figure out. Maybe your changes cover it (I'll see in the testing) but for reference here is more information.

From the sentinel docs:

You only need to specify the masters to monitor, giving to each separated master (that may have any number of slaves) a different name. There is no need to specify slaves, which are auto-discovered. Sentinel will update the configuration automatically with additional informations about slaves (in order to retain the information in case of restart). The configuration is also rewritten every time a slave is promoted to master during a failover.

This means that the after the sentinel server boots up and starts looking at masters and learning about slaves it will change the config file. The next time puppet runs, because the config file is managed, those changes will be lost, and the service will be restarted, disrupting the sentinel's management of its own state.

Thus we end up with the hacky solution: cp with two files. The sentinel.conf.puppet is the managed file. Only when it changes (with intentional admin changes) is it copied to the actual sentinel configuration file (and the service restarted).

There may be a better solution, but it has to be something that allows sentinel to write to its own config without disruption.

[cdent]

I've started my testing, the initial problems to stand out following, using this simple test.pp:

class { 'redis':
  sentinel_enabled          => true,
  service_name              => 'redis-sentinel',
  sentinel_master_name      => 'cow',
  sentinel_master_host      => '10.0.0.2',
}

• On rpm-based systems the systemd unit file is pointed at /etc/redis-sentinel.conf which means that the changes caused by these puppet manifests are not seen upon service restart
• The /etc/redis.conf file is owned by root but the redis-sentinel process is running as redis, so it can't write its state (not that this matters because of the item above)
• There is a redis-sentinel running, but it is monitoring the wrong host.
• The generated redis.conf has the correct information in it, but it is not being used because of the unit file issues described above.

I haven't had a chance to check on deb-based systems but will do so shortly.

[cdent]

@EmilienM and I talked about this morning in IRC and made some progress on figuring out what needs to happen. There were three basic conclusions that we hope to address later today:

• The fact that sentinel writes to its own config needs to be addressed, but there may be a more puppetish way to do it than my exec resource.

• If sufficient configuration parameters are passed to the redis class a working sentinel will start.

• The extent of parameters required goes against the "reasonable defaults" attitude that this module has so additional refactoring will be required. It ought be possible to start a sentinel by saying:

  • I want a sentinel
  • I want it monitoring this master

  and not needing to pass in parameters which otherwise have defaults in params.pp

Another question @EmilienM: Using this style, how do I declare in one manifest that I want to run both a redis-server and redis-sentinel on the same host? If I add another class { 'redis': } I get a duplicate declaration error. We need to be able to run both.

[EmilienM]

I think we should keep your implementation and fix the resource duplication: #20