RFC: Remove daemonization and logging control?

[mperham]

IME people using -d to daemonize almost always don't know the "proper" way to supervise and control Sidekiq or other long-running services. I'd like to remove the -d, -P and -L CLI arguments as they are Unix legacy and we need to entice users to learn the newer, better ways.

1. Don't daemonize, start Sidekiq with a process supervisor like systemd, upstart, foreman, etc.
2. Log only to STDOUT, the entity starting Sidekiq can control where STDOUT redirects to, if any.
3. PID files are a legacy of double forking and have no reason to exist anymore.

Please chime in if you have a use case this might break.

[t27duck]

I'd still like the ability to have a pid file. This may be a case of me being "old", but I've typically always worked in pids and sending signals to processes through pids.

Personally, I set up my supervisors to monitor pids, again this is probably a result of me just doing things "the old way".

The rest of the proposed changes I'm alright with.

[mperham]

@t27duck What do you use to supervise Sidekiq? Typically you can get the PID from the supervisor, with systemctl status sidekiq or similar. This is a decent overview of why PID files are broken (apologies if you are already familiar):

https://stackoverflow.com/a/25933330/1494519

[t27duck]

Yeah, admittedly I use systemd and just give it the location of the PID file to use.

My problem is I haven't gotten around to "fixing" them to do it the right way, mainly due to "they work fine for me as-is" today.

Figure it's worth a shot to keep me from having to redo my configs. :D

[mperham]

Reframe it as "simplifying" your configs and it makes more sense. :-) systemd's PIDFile= option is really necessary to support services which only support old-style double forking.

[renchap]

+1 for this!

Regarding logging, I would love to be able to have Sidekiq output logs in a formatted format (be it JSON or anything else, or make it modular). This would be really helpful when you want to analyse them. I know it is possible to create a custom logger, but this should be in core imo.

[mperham]

Note to readers: I also wrote this blog post over four years ago about how daemonization is a terrible thing. This proposal has been a long time coming.

https://www.mikeperham.com/2014/09/22/dont-daemonize-your-daemons/

[mperham]

@renchap I would consider inclusion in core if you want to submit a PR.

[mperham]

If PID files are removed, we break the quiet/shutdown features of sidekiqctl. I'll have to remove that too; sidekiqctl always implied that you were using sidekiq in :daemon mode.

[t27duck]

Would there be a replacement/equivalent for telling sidekiq to stop collecting new jobs due to a soon-to-be shutdown?

[Tensho]

@t27duck I guess for systemd it should look like:

$ systemctl kill -s SIGTSTP sidekiq

[jbielick]

Sounds great. Good separation of concerns and some sensible changes that I’ve been interested in from time to time after years of using sidekiq.

[ndbroadbent]

Hello, I just wanted to let you know about one use-case that this will affect, and I wanted to see if there might be a better way for me to accomplish the same goal.

Some context: I accidentally deployed a change that caused my Sidekiq workers to crash on boot. (Fortunately I had zero downtime and the deploy was rolled back automatically.) This was an error that happened in a Sidekiq.configure_server block. My test suite was only running the Sidekiq jobs inline, so unfortunately my CI build was passing. I had also forgotten to restart Spring, so when I restarted Sidekiq in development, it wasn't loading the new changes. This issue would have been caught if I had deployed to staging first, but my local tests and the passing test suite had given me a false sense of confidence.

I've now added an actual end-to-end worker test to my CI build, where I start a real Sidekiq process, and run a real job through Redis. Here's a gist that contains my "sanity check" script, and also a WorkerHealthCheckJob that just writes some data to a temporary file.

I'm using the --pidfile, --logfile, and --daemon flags, because that makes it a bit easier to start a test process, and then kill it once the test is finished.

Now that these flags are being removed, I think I can just run Sidekiq in the background and get the PID from $!:

sidekiq  &
SIDEKIQ_PID=$(echo $!)

So that's not too much of a problem! I just wanted to let you know about this use-case and see if you might have any feedback or suggestions about end-to-end testing in CI.

UPDATE: This works totally fine: https://gist.github.com/ndbroadbent/ea32c513f43917a844903760831ec9d3
(It's also nicer to have all of the log output in stdout.)

UPDATE 2: Sorry I spoke too soon, I think this worked locally on my Mac, but I couldn't get it to work on CI. (I think it's something to do with the TTY.) I could only get this to work by using the --pidfile, --logfile, and --daemon flags. Here's the latest iteration of my bash script.

[mperham]

An alternative is to use a system("bundle exec sidekiq") call to start Sidekiq in a separate thread and monitor its stdout. Your test thread can wait for the server to boot, send a job and wait for the server to output done for that JID. It's not easy but possible.

You need nohup <cmd> & to spin off an orphan process. Otherwise when you close your terminal, any & processes from that terminal will also close.

[ndbroadbent]

Thanks, that's a much better idea! The bash script was definitely a bit clunky, so I decided to do this inside an RSpec test. I call Sidekiq::Testing.disable! so that it sends the job to Redis, and then I start a sidekiq process via PTY.spawn, and wait for some expected output: https://gist.github.com/ndbroadbent/93b56b183141ff395b8ef1497f0f4e4d

I was initially using expect (like this), but I need to see the full output from Sidekiq if the test fails. Learned a lot about pty and expect today, so this has been fun.

Thanks for the idea, and this is much better!

[Tensho]

@ndbroadbent Just curious, if it's a problem with Sidekiq configuration isn't it easier to cover it with unit test?

[ndbroadbent]

@Tensho This was coming from config/initializers/sidekiq.rb, where I set up a Sidekiq.configure_server block to configure the Sidekiq server. It was just a single of line of code but it ended up crashing the worker. This configure_server block wasn't ever called when running the inline jobs, and I'm actually not too sure how to call it in a test. But that could also be a good idea.

But I do think that this kind of end-to-end integration test is really useful, so you can be sure that the worker is able to boot and process a job from a real Redis connection.

I do the same thing for puma, to make sure that my Docker images can boot the Rails server and pass a simple health check by rendering a Rails view. This has also been a really helpful safety guard.