Merge branch 'sh-limit-sidekiq-cluster-concurrency' into 'master'

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

Closes #7374

See merge request gitlab-org/gitlab-ee!7032

doc/administration/operations/extra_sidekiq_processes.md

@@ -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.

ee/changelogs/unreleased/sh-limit-sidekiq-cluster-concurrency.yml

+---
+title: Limit sidekiq-cluster concurrency to a maximum of 50
+merge_request: 7025
+author:
+type: performance

ee/lib/gitlab/sidekiq_cluster.rb

@@ -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

ee/lib/gitlab/sidekiq_cluster/cli.rb

@@ -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

ee/spec/lib/gitlab/sidekiq_cluster/cli_spec.rb

@@ -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))

ee/spec/lib/gitlab/sidekiq_cluster_spec.rb

@@ -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