Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an initscript for sentinel on apt-based systems #23

Merged
merged 2 commits into from
Jan 19, 2015

Conversation

cdent
Copy link
Contributor

@cdent cdent commented Jan 16, 2015

Until an initscript is available for redis-sentinel in its package[1]
we need to roll our own. This change adds that by creating a templated
initscript with the sentinel config file and service owner and group
as template variables that get filled in.

Testing this revealed some bugs and misfeatures that are also addressed
in this change:

  • apt systems want the redis and redis-sentinel processes to daemonize
    (systemd-based) rpm does not. This was not reflected in the params
    defaults, nor reflected in the sentinel configuration template. Now
    it is.
  • the package_name for redis-sentinel was not being passed in as a
    parameter, instead it was just using the params.pp default for
    redis server. When [1] is resolve, it will likely mean a different
    package for redis-sentinel so we need there to be a separate parameter
    for the sentinel package that gets installed. For now the name of
    the existing package is used.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775414

Until an initscript is available for redis-sentinel in its package[1]
we need to roll our own. This change adds that by creating a templated
initscript with the sentinel config file and service owner and group
as template variables that get filled in.

Testing this revealed some bugs and misfeatures that are also addressed
in this change:

* apt systems want the redis and redis-sentinel processes to daemonize
  (systemd-based) rpm does not. This was not reflected in the params
  defaults, nor reflected in the sentinel configuration template. Now
  it is.

* the package_name for redis-sentinel was not being passed in as a
  parameter, instead it was just using the params.pp default for
  redis server. When [1] is resolve, it will likely mean a different
  package for redis-sentinel so we need there to be a separate parameter
  for the sentinel package that gets installed. For now the name of
  the existing package is used.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775414
@cdent
Copy link
Contributor Author

cdent commented Jan 16, 2015

@antonlindstrom and @arioch as far as my testing goes on ubuntu 14.04 this seems to fix #18 as expected. It would probably be wise for other folk besides me to test it before merging it though.

As you'll see the changes cascaded a bit to requiring a few other fixes.

@cdent
Copy link
Contributor Author

cdent commented Jan 16, 2015

Please don't merge this yet, I left out a critical part: The initscript is not getting linked into the rc directories so the service won't launch on boot. I'll get that early tomorrow.

Also make sure that a daemonized sentinel properly produces
a pid file so the init script can stop the sentinel.
@cdent
Copy link
Contributor Author

cdent commented Jan 17, 2015

The second commit should fix the the lack of initscript, and also fixes pid file handling. I reckon this is ready for someone else to give it a try and if it passes muster, should be good to merge.

@antonlindstrom
Copy link

Looks, good, I'll do a test today.

Thank you so much, @cdent!

@antonlindstrom
Copy link

@cdent I can verify that this works as expected, thanks!

@arioch
Copy link
Contributor

arioch commented Jan 19, 2015

Thanks @antonlindstrom & @cdent!

arioch added a commit that referenced this pull request Jan 19, 2015
Add an initscript for sentinel on apt-based systems
@arioch arioch merged commit 7848590 into voxpupuli:master Jan 19, 2015
@cdent
Copy link
Contributor Author

cdent commented Jan 19, 2015

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants