Add sidekiq wrappers. #1689

eugenk · 2016-05-11T20:48:40Z

These are the most lightweight Sidekiq wrappers I can create.

Usage: bin/sidekiq_{default,hets,sequential} [options]
    -w, --workdir=MANDATORY          Set work directory

They create a pid file and a log file in the Workdir. The pid file is actually needed for the executable bin/sidekiqctl. It listens to the signals

TERM to terminate after at most 15 seconds (timeout parameter)
USR1 to terminate as soon as all running jobs have been processed
USR2 to reopen the log file

as described in the corresponding Sidekiq wiki page

On my laptop, sidekiq_hets takes 0.5 seconds to start up while the other two take 0.006 seconds to start up (i.e. to get to the exec command).

This is already deployed on tc.

This pull request is part of #1646.

houndci-bot · 2016-05-11T20:48:44Z

bin/sidekiq_wrapper_base.rb

+
+RAILS_ROOT = File.expand_path('../..', __FILE__)
+SIDEKIQ_EXECUTABLE = File.join(RAILS_ROOT, 'bin/sidekiq')
+SIDEKIQ_BASE_ARGUMENTS = %W(#{SIDEKIQ_EXECUTABLE}


Freeze mutable objects assigned to constants.

eugenk · 2016-05-11T20:51:45Z

Oh, and feel free to create a patch on the deployed instances to make a more specific shebang line, as you did with git/bin/git-shell

jelmd · 2016-05-11T21:03:36Z

bin/sidekiq_wrapper_base.rb

+  OptionParser.new do |opts|
+    opts.banner = "Usage: #{filename} [options]"
+
+    opts.on('-wMANDATORY', '--workdir=MANDATORY', 'Set work directory') do |w|


making options mandatory is almost in any case bad design. Use CDW if not given.

Done with f5bf00c

jelmd · 2016-05-11T21:10:28Z

Don't understand, why there need to be 3 files for the same? purpose.
Which one should be taken?
The timeout should be a param as well, so that it can be changed, when more knowledge has been collected...

houndci-bot · 2016-05-11T21:21:52Z

bin/sidekiq_wrapper_base.rb

+SIDEKIQ_EXECUTABLE = File.join(RAILS_ROOT, 'bin/sidekiq').freeze
+SIDEKIQ_BASE_ARGUMENTS =
+  %W(#{SIDEKIQ_EXECUTABLE}
+  --environment #{ENV['RAILS_ENV'] || 'production'}).freeze


Align the elements of an array literal if they span more than one line.

eugenk · 2016-05-11T21:25:36Z

Don't understand, why there need to be 3 files for the same? purpose.
Which one should be taken?

We need three different sidekiq instances each with different parameters. For each of these instances, I created a wrapper so that the init script can be very short. The wrappers are subject to change because the parameters may need some tweaking in the future (queue priorities, concurrency).

The timeout should be a param as well, so that it can be changed, when more knowledge has been collected...

Done with 5798b5c

eugenk · 2016-05-11T21:26:54Z

Which one should be taken?

All three sidekiq instances need to be started in parallel.

jelmd · 2016-05-12T00:53:55Z

No - this is a no go. This way the application logic is pushed back to the starter script which is wrong, buys us nothing (especially because Linux has no contract filesystem for processes).

As said, the init script starts one process and this one does, whatever is needed and daemonizes itself. It should provide required flexibility by accepting related options (if not given, choose reasonable defaults). And this one is the one where to send the signals. It than can intercept, xlate or broadcast to forked children or do whatever the documentation says ;-)

BTW: Since all the files do more or less the same, have the same purpose, they belong IMHO into a single file. See it as a Starter class, which has certain properties/different starter methods ...

eugenk · 2016-05-12T08:49:22Z

Alright, now there's only one wrapper to start and stop all instances of sidekiq. It delegates the above-mentioned signals to them.

jelmd · 2016-05-12T13:53:14Z

Okidoki, I'll try it ... :)

jelmd · 2016-05-15T10:00:50Z

bin/sidekiq_wrapper

+            "Set Rails environment (default: #{options[:environment]})") do |e|
+      options[:environment] = e
+    end
+
    opts.on('-wMANDATORY', '--workdir=MANDATORY',
            'Set work directory (default: CWD)') do |w|


BTW: dict lookup MANDATORY!

Also as a general rule of thumb: In synopsis, usage infos the option value names should indicate its "type", e.g. -w directory, -t seconds. The description should just include '(mandatory)' or '(optional)' if needed. Usually it is not, because the default value should be mentioned and than it is clear, that it is 'optional'. For arg-less options the name option indicates, that it is _option_al and thus doesn't need to be explicitly mentioned. So only in very rare cases or combination of options may require, that 'mandatory' appears in documentation.

The MANDATORY is Ruby's optparse's way to say that if -w is used, then a value must be specified. This does not say that you have to use -w at all.

Admins usually have no notion of ill/senseless optparse documentation. However, they know [indirectly] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html by reading man pages and usage synopsis from native Unix utilities ...

jelmd · 2016-05-15T10:56:59Z

The service (i.e. sidekiq_wrapper) does not daemonize itself, but every service always should after it has validated config[s] and runability (started needed workers) successfully. If not, it should exit wqith a proper exit code, which let the "restarter" determine, whether it makes sense to try to restart, whether there is a general error (e.g. missing disk ;-)) or config error (and thus needs to inform the user), etc..

Something like http://daemons.rubyforge.org/Daemonize.html may help ... ?

eugenk · 2016-05-15T10:59:34Z

Okay. I will implement it, but not today. Preumably on tuesday.

eugenk · 2016-05-18T13:56:04Z

You can now daemonise the process with a separate script (here with all parameters exemplary specified):

bin/sidekiq_wrapper_daemon start -- -e production -w /tmp/sidekiq -t 15
bin/sidekiq_wrapper_daemon stop

This script calls bin/sidekiq_wrapper and then daemonises itself. I couldn't get it to work with only one script.

eugenk · 2016-05-18T14:01:56Z

Oh, and the daemon process puts a pidfile of itself to $RAILS_ROOT/tmp/pids. Unfortunately, the daemons gem is not quite compatible to the options parser, so I can't get the -w to work for the daemon easily. I will try again, but I can't guarantee that it will work.

eugenk · 2016-06-06T13:40:19Z

We need to wait for spechub/Hets#1644 to get this running.

eugenk · 2016-08-10T11:19:46Z

@jelmd: spechub/Hets#1644 has been merged, so with the next version of Hets, you can specify a pidfile with hets -X -P path_to_pidfile.
Please do so in the svcadm scripts and post in this issue where (all) the pidfiles are located.

houndci-bot reviewed May 11, 2016
View reviewed changes

eugenk assigned jelmd May 11, 2016

jelmd reviewed May 11, 2016
View reviewed changes

houndci-bot reviewed May 11, 2016
View reviewed changes

eugenk force-pushed the 1646-services_on_ontohub_org branch from 7d8bc0c to 5798b5c Compare May 11, 2016 21:23

jelmd reviewed May 15, 2016
View reviewed changes

eugenk force-pushed the 1646-services_on_ontohub_org branch 3 times, most recently from 181fad7 to 2a5c102 Compare May 18, 2016 13:50

eugenk mentioned this pull request Jun 13, 2016

Write PID file in server mode. spechub/Hets#1648

Merged

ebolloff added this to the improve stability milestone Aug 2, 2016

eugenk force-pushed the 1646-services_on_ontohub_org branch from 2641319 to 28f711f Compare August 10, 2016 07:12

eugenk added 4 commits August 13, 2016 14:07

Add sidekiq wrappers.

de780cf

Freeze mutable objects assigned to constants.

f4cfa26

Use CWD if no workdir is given.

9dd516a

Make timeout a parameter.

e096b8b

eugenk added 11 commits August 13, 2016 14:07

Only have one sidekiq wrapper for all processes.

93f48bd

Make the environment a parameter.

3e17b8d

Use full name as hash key.

2a78ce2

Use daemons gem to daemonize sidekiq_wrapper.

15890c0

Improve synopsis.

5b067f7

Adjust eye config to svcadm.

c6fa541

Specify pidfiles per process.

2d414f7

Fix eye config.

a462156

Fix pids

c3b28de

Stop other processes when one died.

32030a6

Exit with proper exit code.

0f4089c

eugenk force-pushed the 1646-services_on_ontohub_org branch from 28f711f to 0f4089c Compare August 13, 2016 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sidekiq wrappers. #1689

Add sidekiq wrappers. #1689

eugenk commented May 11, 2016 •

edited

houndci-bot May 11, 2016

eugenk commented May 11, 2016

jelmd May 11, 2016

eugenk May 11, 2016

jelmd commented May 11, 2016

houndci-bot May 11, 2016

eugenk commented May 11, 2016

eugenk commented May 11, 2016

jelmd commented May 12, 2016

eugenk commented May 12, 2016

jelmd commented May 12, 2016

jelmd May 15, 2016

eugenk May 15, 2016

jelmd May 15, 2016

jelmd commented May 15, 2016

eugenk commented May 15, 2016

eugenk commented May 18, 2016 •

edited

eugenk commented May 18, 2016 •

edited

eugenk commented Jun 6, 2016

eugenk commented Aug 10, 2016

Add sidekiq wrappers. #1689

Are you sure you want to change the base?

Add sidekiq wrappers. #1689

Conversation

eugenk commented May 11, 2016 • edited

houndci-bot May 11, 2016

Choose a reason for hiding this comment

eugenk commented May 11, 2016

jelmd May 11, 2016

Choose a reason for hiding this comment

eugenk May 11, 2016

Choose a reason for hiding this comment

jelmd commented May 11, 2016

houndci-bot May 11, 2016

Choose a reason for hiding this comment

eugenk commented May 11, 2016

eugenk commented May 11, 2016

jelmd commented May 12, 2016

eugenk commented May 12, 2016

jelmd commented May 12, 2016

jelmd May 15, 2016

Choose a reason for hiding this comment

eugenk May 15, 2016

Choose a reason for hiding this comment

jelmd May 15, 2016

Choose a reason for hiding this comment

jelmd commented May 15, 2016

eugenk commented May 15, 2016

eugenk commented May 18, 2016 • edited

eugenk commented May 18, 2016 • edited

eugenk commented Jun 6, 2016

eugenk commented Aug 10, 2016

eugenk commented May 11, 2016 •

edited

eugenk commented May 18, 2016 •

edited

eugenk commented May 18, 2016 •

edited