Commit 73218620 authored by Stan Hu's avatar Stan Hu

Limit sidekiq-cluster concurrency to a maximum of 50 by default

By default, sidekiq-cluster will allocate N threads for N queues specified
on the command line, which in turn will require N + 5 Redis connections.
Especially when used with the `--negate` flag, this can lead to using hundreds
of Redis connections, which can lead to performance issues.

This change also adds a `-m N` option to cap the number of threads. For each
queue group, the concurrency factor will be set to min(number of queues, N).

Closes #7374
parent b8f2f711
......@@ -111,3 +111,20 @@ For multiple processes of all queues (except "process_commit" and "post_receive"
```bash
sidekiq-cluster process_commit,post_receive process_commit,post_receive --negate
```
## Limiting Concurrency
By default, `sidekiq-cluster` will spin up extra Sidekiq processes that use
one thread per queue up to a maximum of 50. If you wish to change the cap, use
the `-m N` option. For example, this would cap the maximum number of threads to 1:
```bash
sidekiq-cluster process_commit,post_receive -m 1
```
For each queue group, the concurrency factor will be set to min(number of
queues, N). Setting the value to 0 will disable the limit.
Note that each thread requires a Redis connection, so adding threads may
increase Redis latency and potentially cause client timeouts. See the [Sidekiq
documentation about Redis](https://github.com/mperham/sidekiq/wiki/Using-Redis) for more details.
---
title: Limit sidekiq-cluster concurrency to a maximum of 50
merge_request: 7025
author:
type: performance
......@@ -62,16 +62,16 @@ module Gitlab
# directory - The directory of the Rails application.
#
# Returns an Array containing the PIDs of the started processes.
def self.start(queues, env, directory = Dir.pwd, dryrun: false)
queues.map { |pair| start_sidekiq(pair, env, directory, dryrun: dryrun) }
def self.start(queues, env, directory = Dir.pwd, max_concurrency = 50, dryrun: false)
queues.map { |pair| start_sidekiq(pair, env, directory, max_concurrency, dryrun: dryrun) }
end
# Starts a Sidekiq process that processes _only_ the given queues.
#
# Returns the PID of the started process.
def self.start_sidekiq(queues, env, directory = Dir.pwd, dryrun: false)
def self.start_sidekiq(queues, env, directory = Dir.pwd, max_concurrency = 50, dryrun: false)
cmd = %w[bundle exec sidekiq]
cmd << "-c #{queues.length + 1}"
cmd << "-c #{self.concurrency(queues, max_concurrency)}"
cmd << "-e#{env}"
cmd << "-gqueues: #{queues.join(', ')}"
cmd << "-r#{directory}"
......@@ -97,6 +97,14 @@ module Gitlab
pid
end
def self.concurrency(queues, max_concurrency)
if max_concurrency.positive?
[queues.length + 1, max_concurrency].min
else
queues.length + 1
end
end
# Waits for the given process to complete using a separate thread.
def self.wait_async(pid)
Thread.new do
......
......@@ -8,6 +8,8 @@ module Gitlab
CommandError = Class.new(StandardError)
def initialize(log_output = STDERR)
# As recommended by https://github.com/mperham/sidekiq/wiki/Advanced-Options#concurrency
@max_concurrency = 50
@environment = ENV['RAILS_ENV'] || 'development'
@pid = nil
@interval = 5
......@@ -45,7 +47,7 @@ module Gitlab
@logger.info("Starting cluster with #{queue_groups.length} processes")
@processes = SidekiqCluster.start(queue_groups, @environment, @rails_path, dryrun: @dryrun)
@processes = SidekiqCluster.start(queue_groups, @environment, @rails_path, @max_concurrency, dryrun: @dryrun)
return if @dryrun
......@@ -94,6 +96,10 @@ module Gitlab
abort opt.to_s
end
opt.on('-m', '--max-concurrency INT', 'Maximum threads to use with Sidekiq (default: 50, 0 to disable)') do |int|
@max_concurrency = int.to_i
end
opt.on('-e', '--environment ENV', 'The application environment') do |env|
@environment = env
end
......
......@@ -19,7 +19,7 @@ describe Gitlab::SidekiqCluster::CLI do
it 'starts the Sidekiq workers' do
expect(Gitlab::SidekiqCluster).to receive(:start)
.with([['foo']], 'test', Dir.pwd, dryrun: false)
.with([['foo']], 'test', Dir.pwd, 50, dryrun: false)
.and_return([])
cli.run(%w(foo))
......@@ -29,18 +29,29 @@ describe Gitlab::SidekiqCluster::CLI do
it 'starts Sidekiq workers for all queues in all_queues.yml except the ones in argv' do
expect(Gitlab::SidekiqConfig).to receive(:worker_queues).and_return(['baz'])
expect(Gitlab::SidekiqCluster).to receive(:start)
.with([['baz']], 'test', Dir.pwd, dryrun: false)
.with([['baz']], 'test', Dir.pwd, 50, dryrun: false)
.and_return([])
cli.run(%w(foo -n))
end
end
context 'with --max-concurrency flag' do
it 'starts Sidekiq workers for specified queues with a max concurrency' do
expect(Gitlab::SidekiqConfig).to receive(:worker_queues).and_return(%w(foo bar baz))
expect(Gitlab::SidekiqCluster).to receive(:start)
.with([%w(foo bar baz), %w(solo)], 'test', Dir.pwd, 2, dryrun: false)
.and_return([])
cli.run(%w(foo,bar,baz solo -m 2))
end
end
context 'queue namespace expansion' do
it 'starts Sidekiq workers for all queues in all_queues.yml with a namespace in argv' do
expect(Gitlab::SidekiqConfig).to receive(:worker_queues).and_return(['cronjob:foo', 'cronjob:bar'])
expect(Gitlab::SidekiqCluster).to receive(:start)
.with([['cronjob', 'cronjob:foo', 'cronjob:bar']], 'test', Dir.pwd, dryrun: false)
.with([['cronjob', 'cronjob:foo', 'cronjob:bar']], 'test', Dir.pwd, 50, dryrun: false)
.and_return([])
cli.run(%w(cronjob))
......
......@@ -58,12 +58,22 @@ describe Gitlab::SidekiqCluster do
describe '.start' do
it 'starts Sidekiq with the given queues and environment' do
expect(described_class).to receive(:start_sidekiq)
.ordered.with(%w(foo), :production, 'foo/bar', dryrun: false)
.ordered.with(%w(foo), :production, 'foo/bar', 50, dryrun: false)
expect(described_class).to receive(:start_sidekiq)
.ordered.with(%w(bar baz), :production, 'foo/bar', dryrun: false)
.ordered.with(%w(bar baz), :production, 'foo/bar', 50, dryrun: false)
described_class.start([%w(foo), %w(bar baz)], :production, 'foo/bar')
described_class.start([%w(foo), %w(bar baz)], :production, 'foo/bar', 50)
end
it 'starts Sidekiq with capped concurrency limits for each queue' do
expect(described_class).to receive(:start_sidekiq)
.ordered.with(%w(foo bar baz), :production, 'foo/bar', 2, dryrun: false)
expect(described_class).to receive(:start_sidekiq)
.ordered.with(%w(solo), :production, 'foo/bar', 2, dryrun: false)
described_class.start([%w(foo bar baz), %w(solo)], :production, 'foo/bar', 2)
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment