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

Already on GitHub? Sign in to your account

Use runit to start / stop statsd process. Much more reliable than previous init scripts. #19

Merged
merged 4 commits into from Jun 15, 2013

Conversation

Projects
None yet
4 participants

I know that it's kind of big change, but it seems pretty straightforward and useful. Here is why:

We use AMS image ( amazon ami ) and I noticed that after fresh install when chef-client is running as part of bootstrap process ( not the same thing when you bootstrap with knife ) statsd process was missing.. After looking a little more, I noticed error message in the log "sudo: sorry, you must have a tty to run sudo" which makes perfect sense when you statsd init script is invoked by the main init process.

I was thinking to do a quick fix and replace sudo -u statsd -- with su statsd -c "command". I even created a branch for this, but quickly realized that it wasn't such a great idea because that would impose some additional constrains like the need to have real shell assigned for statsd use ( that's easy ) but the trickiest part was to monitor statsd process id, so it could be stopped easliy. When you use su -c , you can't easily parse out process id by using ps aux command, because there are multiple processes with this name.

And then it hit me - why reinvent the wheel and deal with process babysitting and using wired & to send process to the background if we could use excellent runit cookbook to do just that. So I sat down and rewrote it with runit support.

I hope you will consider the change.

Owner

hectcastro commented Jun 7, 2013

I'll take a closer look at this over the weekend.

Owner

hectcastro commented Jun 10, 2013

This change seems like it would break backward compatibility in at least one way. I'm not opposed to merging runit support, but I am slightly opposed to requiring runit for RHEL users that want to use this cookbook.

Pinging @ChrisLundquist, since he added the RHEL support initially. Perhaps he can propose other alternatives.

Can you explain a little more about your compatibility concerns?

As stated on runit cookbook page - "This cookbook does not use runit to replace system init, nor are there plans to do so."

Runit it's just a nice little tool to allow process supervision and it can run side by side with initV or upstart (or could replace those if you wish). It's just our use case would be perfect for it and should work across platforms.

I do use this modified cookbook with runit on REHL based box ( amazon ) and it works just fine as opposed to the provided init script which is sort of half baked..

We could also bump major release version, make it 2.0 or something so it wont break previous versions, since chef environments usually pinned to particular versions.

Owner

hectcastro commented Jun 11, 2013

Can you explain a little more about your compatibility concerns?

As stated on runit cookbook page - "This cookbook does not use runit to replace system init, nor are there plans to do so."

Personally, I don't have much experience with runit. That said, I like the ideas behind it.

My concerns are around how people without runit experience will be able to interact with statsd after using this cookbook.

For example, a modern Ubuntu user may be familiar with Upstart and expect to do:

$ stop statsd

If we switch to runit, they may execute that command, receive an error, realize that the cookbook uses runit, and eventually figure out to issue:

$ sv stop statsd

I'm not saying this is difficult, just unfamiliar.

Do you have any suggestions around this?

Runit it's just a nice little tool to allow process supervision and it can run side by side with initV or upstart (or could replace those if you wish). It's just our use case would be perfect for it and should work across platforms.

I added test-kitchen support to your branch locally, and it does appear to work on both Ubuntu and CentOS.

I do use this modified cookbook with runit on REHL based box ( amazon ) and it works just fine as opposed to the provided init script which is sort of half baked..

We could also bump major release version, make it 2.0 or something so it wont break previous versions, since chef environments usually pinned to particular versions.

Contributor

ChrisLundquist commented Jun 11, 2013

Sorry for the slow response. I'm fine with it. Any complex run_list pulls it down along with apt / yum.

Ahh, I see, you're nuking the init stuff entirely.
Normally people switch on "init_style" as a compromise.
At least, that was once the convention http://tickets.opscode.com/browse/COOK-1002

/cc @bbg-cookbooks
I don't know if you're running this still.

Not all services on Ubuntu provide upstart startup scripts, some still use systemV init.d versions.

Most people should be familiar with both "start service" (upstart) or "/etc/init.d/statsd start" || "service statsd start" (system V)

The nice thing about runit is that it creates wrapper inside /etc/init.d so you don't need to know anything about sv command, you just start and stop it with /etc/init.d/statsd stop|start ( or service command )

@hectcastro hectcastro commented on the diff Jun 12, 2013

recipes/default.rb
- frequency "daily"
- rotate 7
- create "644 root root"
-end
-
-service "statsd" do
- case node["platform"]
- when "ubuntu"
- if node["platform_version"].to_f >= 9.10
- provider Chef::Provider::Service::Upstart
- end
- #restart_command "sudo service statsd stop && sudo service statsd start"
- end
- action [ :enable, :start ]
- supports :start => true, :stop => true, :restart => true, :status => true
+runit_service "statsd" do
@hectcastro

hectcastro Jun 12, 2013

Owner

Is there a way that you can preserve the usage of node["statsd"]["log_file"]?

@gansbrest

gansbrest Jun 12, 2013

You can only specify log directory. Most cookbooks which use runit provide default logging template ( like we did ) and use default_logger option which causes it to go to /var/log/service_name dir by convention.

I think we should keep it this way as well and drop this log_file attribute.

Owner

hectcastro commented Jun 12, 2013

Not all services on Ubuntu provide upstart startup scripts, some still use systemV init.d versions.

Most people should be familiar with both "start service" (upstart) or "/etc/init.d/statsd start" || "service statsd start" (system V)

The nice thing about runit is that it creates wrapper inside /etc/init.d so you don't need to know anything about sv command, you just start and stop it with /etc/init.d/statsd stop|start ( or service command )

This last part about /etc/init.d eases a lot of my concerns. I made some comments on the commits. If those issues can be addressed, I'll merge.

@hectcastro hectcastro commented on the diff Jun 12, 2013

attributes/default.rb
@@ -9,6 +9,7 @@
default["statsd"]["graphite_port"] = 2003
default["statsd"]["delete_timers"] = false
default["statsd"]["delete_gauges"] = false
+default["statsd"]["username"] = "statsd"
@hectcastro

hectcastro Jun 12, 2013

Owner

Can you add this to the README?

@hectcastro hectcastro commented on the diff Jun 12, 2013

recipes/default.rb
@@ -1,11 +1,11 @@
include_recipe "git"
include_recipe "nodejs"
-include_recipe "logrotate"
@hectcastro

hectcastro Jun 12, 2013

Owner

Just to confirm, runit handles log rotation?

@gansbrest

gansbrest Jun 12, 2013

svlogd does a pretty good job timestamping lines, and it also rotates logs automatically without needing to stop and start the daemon.

I added changes you asked for, lets merge this in while it's still fresh in our heads )

@hectcastro hectcastro added a commit that referenced this pull request Jun 15, 2013

@hectcastro hectcastro Merge pull request #19 from gansbrest/use_runit
Use runit to start / stop statsd process. Much more reliable than previous init scripts.
2c99a41

@hectcastro hectcastro merged commit 2c99a41 into hectcastro:master Jun 15, 2013

1 check failed

default The Travis CI build failed
Details
Owner

hectcastro commented Jun 15, 2013

Thanks, everyone. @gansbrest give it a spin!

Great, don't forget to bump cookbook version?

While I support the use of runit, it seems the current implementation of the runit service in this cookbook is broken. I'm getting the following error when trying to include statsd in my recipe:

[2013-06-17T23:06:56+00:00] FATAL: Chef::Exceptions::ResourceNotFound: resource git[/usr/share/statsd] is configured to notify resource runit_service[statsd] with action restart, but runit_service[statsd] cannot be found in the resource collection. git[/usr/share/statsd] is defined in /var/cache/chef/cookbooks/statsd/recipes/default.rb:5:infrom_file'`

I have already verified that I have depends "runit" in my own metadata and even tried forking this cookbook and moving the order of the statements around to define the service before the notify statement, all to no avail.

Running Chef 10.24.0 on Ubuntu Precise (12.04).

Any clues? The guys on #chef could not figure it out either.

Contributor

ChrisLundquist commented Jun 18, 2013

@rafaelmagu I haven't looked in detail, but since you noted chef 10 I bet this is a chef 10/11 version issue. Chef 10 should find https://github.com/hectcastro/chef-statsd/blob/master/recipes/default.rb#L41 in compilation though. You could try changing https://github.com/hectcastro/chef-statsd/blob/master/recipes/default.rb#L42 to action :nothing and putting it at the top of the file to see if chef 10 picks it up then.

Tried that, still failed. It's as if Chef doesn't pick up on the service definition during compilation. Had to revert to using 0.1.4.

Owner

hectcastro commented Jun 18, 2013

I created an issue here to track: #20

Is there a way u could provide access to ur server, or some sample server running chef 10 setup. I could take a look there.

@hectcastro: will submit further info there.

@gansbrest: server access won't be possible, sorry. Will try my best to help with the diagnosis providing logging output and testing configurations.

@fredrick fredrick pushed a commit to Dwolla/chef-statsd that referenced this pull request Jul 21, 2014

@hectcastro hectcastro Merge pull request #19 from icebourg/PR-mysqlconfig
Add attributes for dashboard MySQL configuration.
9b579ce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment