Ability to start dedicated Sidekiq processes
This adds the ability to start Sidekiq processes for specific queues. See https://gitlab.com/gitlab-org/gitlab-ee/issues/1282 for more information.
Merge request reports
Activity
cc @pcarranza
Mentioned in issue #1282 (closed)
- Resolved by Yorick Peterse
- lib/gitlab/sidekiq_cluster.rb 0 → 100644
1 require 'open3' 2 3 module Gitlab 4 module SidekiqCluster 5 # The signals that should terminate both the master and workers. 6 TERMINATE_SIGNALS = %i(INT TERM) 7 8 # The signals that should simply be forwarded to the workers. 9 FORWARD_SIGNALS = %i(TTIN USR1 USR2 HUP) These were taken from https://github.com/mperham/sidekiq/wiki/Signals
- Resolved by Yorick Peterse
@marin From a packaging perspective, what are your requests when adding a new process to monitor by runit? For example: the script writes to a PID file if told to do so, but I'd like to know if there are more requests.
Edited by Yorick Peterse@yorickpeterse FYI, @marin is out for a few days. As far as I can tell, I think if we follow the conventions of the current runit processes, I think we'll be okay. Maybe @twk3 can chime in here too.
- Resolved by Yorick Peterse
Added 1 commit:
- 7aa95d04 - Ability to start dedicated Sidekiq processes
@stanhu Therein lies the problem: I'm not really aware of what those patterns/standards are other than processes not detaching themselves.
Added 1 commit:
- 9ceecb99 - Ability to start dedicated Sidekiq processes
added 710 commits
-
9ceecb99...05610a23 - 709 commits from branch
master
- c12ceebe - Ability to start dedicated Sidekiq processes
-
9ceecb99...05610a23 - 709 commits from branch
added 1 commit
- b137e5ba - Ability to start dedicated Sidekiq processes
@yorickpeterse I am having a bit of a problem following what you are trying to achieve (and ask). So I will write what I think is happening and we can get to the common ground from there:
The idea here is to be able to start up a process on demand and letting the supervisor handle the process from there on?
You are asking whether you need to do anything special for the supervisor? If that is the question, the answer is no. Supervisor watches the processes and daemonizes them automatically. If the process gets killed or stops, it will try to restart it and it will do that till the concept of time changes.
One question I have for you is, this is supposed to allow us to quickly spin up additional background workers and not as a replacement for the "main" sidekiq?
@marin The idea is to provide a tool that lets us start extra Sidekiq workers that only process a given set of queues. For GitLab.com we have a hack that uses tmux, but this of course doesn't work very pleasantly. These extra workers would be used to process important queues so processing timings are not affected by other queues.
To make this as easy as possible we came up with the idea of having some sort of wrapper script. This script would take a set of arguments (e.g. the queues) and spawn a number of
sidekiq
processes with the right settings. This is whatsidekiq-cluster
in this MR does. This can lead to a setup wheresidekiq
processes everything as usual, andsidekiq-cluster
starts two processes that processprocess_commit
andpost_receive
respectively (thus we'd have 3sidekiq
processes).One question I have for you is, this is supposed to allow us to quickly spin up additional background workers and not as a replacement for the "main" sidekiq?
It would not replace the main
sidekiq
process, instead it would complement it. This does mean that instead of something likesudo gitlab-ctl restart sidekiq
we'd also have to runsudo gitlab-ctl restart sidekiq-cluster
. For Omnibus this also means storing some configuration settings (e.g. the queues to use) somewhere, otherwiserestart
& friends won't know what settings to use. This could be something as simple as just a string passed tosidekiq-cluster
.Does that clear things up a bit?
Edited by Yorick Peterse@yorickpeterse Cool, thanks for explaining.
added 259 commits
-
b137e5ba...9e32bd5d - 258 commits from branch
master
- 0f73385a - Ability to start dedicated Sidekiq processes
-
b137e5ba...9e32bd5d - 258 commits from branch
@marin OK, so is there anything extra we need in this MR to make packaging this up as easy as possible in Omnbius? If not I'll start adding documentation for how this thing works.
@yorickpeterse No, I think this will Just Work (TM). If we need something extra, you will be pinged.
I just need an issue from you in omnibus-gitlab with parts of this discussion in how you see this being used so we can assign people on it.
added 1 commit
- 9660d2f0 - Ability to start dedicated Sidekiq processes
added 1 commit
- 56122dba - Ability to start dedicated Sidekiq processes
assigned to @rspeicher
assigned to @smcgivern
@yorickpeterse does this need to be ~"Pick into Stable" ?
@smcgivern Shoot I forgot to add that label, yes.
- Resolved by Yorick Peterse
- Resolved by Yorick Peterse
@yorickpeterse this looks good to me! Neither of my questions are important; if this needs to be in 8.15, please add ~"Pick into Stable" and merge!
assigned to @yorickpeterse
added 855 commits
-
56122dba...cb6ffd9c - 854 commits from branch
master
- 160fc3f9 - Ability to start dedicated Sidekiq processes
-
56122dba...cb6ffd9c - 854 commits from branch
enabled an automatic merge when the build for 160fc3f9 succeeds
added 1 commit
- b588a0c4 - Ability to start dedicated Sidekiq processes
enabled an automatic merge when the build for b588a0c4 succeeds
mentioned in commit 7ff93264
mentioned in commit omnibus-gitlab@5ddacac4
mentioned in merge request omnibus-gitlab!1180 (merged)
mentioned in commit 450865f9
added Enterprise Edition label
- lib/gitlab/sidekiq_cluster.rb 0 → 100644
62 # queues to use for a single process. 63 # 64 # Returns an Array containing the threads monitoring each process. 65 def self.start(queues, env) 66 queues.map { |pair| start_sidekiq(pair, env) } 67 end 68 69 # Starts a Sidekiq process that processes _only_ the given queues. 70 def self.start_sidekiq(queues, env) 71 switches = queues.map { |q| "-q #{q},1" } 72 73 Open3.popen3({ 'ENABLE_SIDEKIQ_CLUSTER' => '1' }, 74 'bundle', 75 'exec', 76 'sidekiq', 77 "-c #{queues.length + 1}", @yorickpeterse I'm going waaaaaay back in time here, so I'm OK if you don't remember, but: do you remember why this is
queues.length + 1
?I see !922 (comment 19158646) but that doesn't explain why, just that it was the case anyway.
For each queue group, the concurrency factor will be set to
min(number of queues, N)
.Which isn't correct, but now I'm not sure whether I should be fixing the docs or the code. If you don't remember, I'll go with fixing the docs as the code has been this way for a long time.
@smcgivern I think this may have just been done so we have an additional thread for other work, just in case. However, I am not sure if that is actually needed in the first place.
@yorickpeterse thanks. In that case I'll play it safe and just amend the documentation; it might be unnecessary, but if we don't know, it's best not to change anything.
mentioned in merge request !23408 (merged)