Merge branch 'dm-dedicated-sidekiq-queues' into 'master'

Use a dedicated queue for each Sidekiq worker

See merge request gitlab-org/gitlab-ce!15882

12 files changed, 181 insertions, 23 deletions

diff --git a/spec/lib/gitlab/sidekiq_config_spec.rb b/spec/lib/gitlab/sidekiq_config_spec.rb
index 09f95be2213..0c66d764851 100644
--- a/spec/lib/gitlab/sidekiq_config_spec.rb
+++ b/spec/lib/gitlab/sidekiq_config_spec.rb
@@ -16,9 +16,30 @@ describe Gitlab::SidekiqConfig do
 
       expect(queues).to include('post_receive')
       expect(queues).to include('merge')
-      expect(queues).to include('cronjob')
+      expect(queues).to include('cronjob:stuck_import_jobs')
       expect(queues).to include('mailers')
       expect(queues).to include('default')
     end
   end
+
+  describe '.expand_queues' do
+    it 'expands queue namespaces to concrete queue names' do
+      queues = described_class.expand_queues(%w[cronjob])
+
+      expect(queues).to include('cronjob:stuck_import_jobs')
+      expect(queues).to include('cronjob:stuck_merge_jobs')
+    end
+
+    it 'lets concrete queue names pass through' do
+      queues = described_class.expand_queues(%w[post_receive])
+
+      expect(queues).to include('post_receive')
+    end
+
+    it 'lets unknown queues pass through' do
+      queues = described_class.expand_queues(%w[unknown])
+
+      expect(queues).to include('unknown')
+    end
+  end
 end
diff --git a/spec/lib/gitlab/sidekiq_versioning/manager_spec.rb b/spec/lib/gitlab/sidekiq_versioning/manager_spec.rb
new file mode 100644
index 00000000000..7debf70a16f
--- /dev/null
+++ b/spec/lib/gitlab/sidekiq_versioning/manager_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe Gitlab::SidekiqVersioning::Manager do
+  before do
+    Sidekiq::Manager.prepend described_class
+  end
+
+  describe '#initialize' do
+    it 'listens on all expanded queues' do
+      manager = Sidekiq::Manager.new(queues: %w[post_receive repository_fork cronjob unknown])
+
+      queues = manager.options[:queues]
+
+      expect(queues).to include('post_receive')
+      expect(queues).to include('repository_fork')
+      expect(queues).to include('cronjob')
+      expect(queues).to include('cronjob:stuck_import_jobs')
+      expect(queues).to include('cronjob:stuck_merge_jobs')
+      expect(queues).to include('unknown')
+    end
+  end
+end
diff --git a/spec/lib/gitlab/sidekiq_versioning_spec.rb b/spec/lib/gitlab/sidekiq_versioning_spec.rb
new file mode 100644
index 00000000000..fa6d42e730d
--- /dev/null
+++ b/spec/lib/gitlab/sidekiq_versioning_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+
+describe Gitlab::SidekiqVersioning, :sidekiq, :redis do
+  let(:foo_worker) do
+    Class.new do
+      def self.name
+        'FooWorker'
+      end
+
+      include ApplicationWorker
+    end
+  end
+
+  let(:bar_worker) do
+    Class.new do
+      def self.name
+        'BarWorker'
+      end
+
+      include ApplicationWorker
+    end
+  end
+
+  before do
+    allow(Gitlab::SidekiqConfig).to receive(:workers).and_return([foo_worker, bar_worker])
+    allow(Gitlab::SidekiqConfig).to receive(:worker_queues).and_return([foo_worker.queue, bar_worker.queue])
+  end
+
+  describe '.install!' do
+    it 'prepends SidekiqVersioning::Manager into Sidekiq::Manager' do
+      described_class.install!
+
+      expect(Sidekiq::Manager).to include(Gitlab::SidekiqVersioning::Manager)
+    end
+
+    it 'registers all versionless and versioned queues with Redis' do
+      described_class.install!
+
+      queues = Sidekiq::Queue.all.map(&:name)
+      expect(queues).to include('foo')
+      expect(queues).to include('bar')
+    end
+  end
+end
diff --git a/spec/rubocop/cop/include_sidekiq_worker_spec.rb b/spec/rubocop/cop/include_sidekiq_worker_spec.rb
new file mode 100644
index 00000000000..7f406535dda
--- /dev/null
+++ b/spec/rubocop/cop/include_sidekiq_worker_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/include_sidekiq_worker'
+
+describe RuboCop::Cop::IncludeSidekiqWorker do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'when `Sidekiq::Worker` is included' do
+    let(:source) { 'include Sidekiq::Worker' }
+    let(:correct_source) { 'include ApplicationWorker' }
+
+    it 'registers an offense ' do
+      inspect_source(cop, source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+        expect(cop.highlights).to eq(['Sidekiq::Worker'])
+      end
+    end
+
+    it 'autocorrects to the right version' do
+      autocorrected = autocorrect_source(cop, source)
+
+      expect(autocorrected).to eq(correct_source)
+    end
+  end
+end
diff --git a/spec/rubocop/cop/sidekiq_options_queue_spec.rb b/spec/rubocop/cop/sidekiq_options_queue_spec.rb
new file mode 100644
index 00000000000..a31de381631
--- /dev/null
+++ b/spec/rubocop/cop/sidekiq_options_queue_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/sidekiq_options_queue'
+
+describe RuboCop::Cop::SidekiqOptionsQueue do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense when `sidekiq_options` is used with the `queue` option' do
+    inspect_source(cop, 'sidekiq_options queue: "some_queue"')
+
+    aggregate_failures do
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.offenses.map(&:line)).to eq([1])
+      expect(cop.highlights).to eq(['queue: "some_queue"'])
+    end
+  end
+
+  it 'does not register an offense when `sidekiq_options` is used with another option' do
+    inspect_source(cop, 'sidekiq_options retry: false')
+
+    expect(cop.offenses).to be_empty
+  end
+end
diff --git a/spec/workers/concerns/application_worker_spec.rb b/spec/workers/concerns/application_worker_spec.rb
index 0145563e0ed..901d77178bc 100644
--- a/spec/workers/concerns/application_worker_spec.rb
+++ b/spec/workers/concerns/application_worker_spec.rb
@@ -17,6 +17,14 @@ describe ApplicationWorker do
     end
   end
 
+  describe '.queue_namespace' do
+    it 'sets the queue name based on the class name' do
+      worker.queue_namespace :some_namespace
+
+      expect(worker.queue).to eq('some_namespace:foo_bar_dummy')
+    end
+  end
+
   describe '.queue' do
     it 'returns the queue name' do
       worker.sidekiq_options queue: :some_queue
diff --git a/spec/workers/concerns/cluster_queue_spec.rb b/spec/workers/concerns/cluster_queue_spec.rb
index 5049886b55c..4118b9aa194 100644
--- a/spec/workers/concerns/cluster_queue_spec.rb
+++ b/spec/workers/concerns/cluster_queue_spec.rb
@@ -14,6 +14,6 @@ describe ClusterQueue do
 
   it 'sets a default pipelines queue automatically' do
     expect(worker.sidekiq_options['queue'])
-      .to eq :gcp_cluster
+      .to eq 'gcp_cluster:dummy'
   end
 end
diff --git a/spec/workers/concerns/cronjob_queue_spec.rb b/spec/workers/concerns/cronjob_queue_spec.rb
index 3ae1c5f54d8..c042a52f41f 100644
--- a/spec/workers/concerns/cronjob_queue_spec.rb
+++ b/spec/workers/concerns/cronjob_queue_spec.rb
@@ -13,7 +13,7 @@ describe CronjobQueue do
   end
 
   it 'sets the queue name of a worker' do
-    expect(worker.sidekiq_options['queue'].to_s).to eq('cronjob')
+    expect(worker.sidekiq_options['queue'].to_s).to eq('cronjob:dummy')
   end
 
   it 'disables retrying of failed jobs' do
diff --git a/spec/workers/concerns/gitlab/github_import/queue_spec.rb b/spec/workers/concerns/gitlab/github_import/queue_spec.rb
index 9c69ee32da1..a96f583aff7 100644
--- a/spec/workers/concerns/gitlab/github_import/queue_spec.rb
+++ b/spec/workers/concerns/gitlab/github_import/queue_spec.rb
@@ -11,6 +11,6 @@ describe Gitlab::GithubImport::Queue do
       include Gitlab::GithubImport::Queue
     end
 
-    expect(worker.sidekiq_options['queue']).to eq('github_importer')
+    expect(worker.sidekiq_options['queue']).to eq('github_importer:dummy')
   end
 end
diff --git a/spec/workers/concerns/pipeline_queue_spec.rb b/spec/workers/concerns/pipeline_queue_spec.rb
index dd911760948..a312b307fce 100644
--- a/spec/workers/concerns/pipeline_queue_spec.rb
+++ b/spec/workers/concerns/pipeline_queue_spec.rb
@@ -14,15 +14,6 @@ describe PipelineQueue do
 
   it 'sets a default pipelines queue automatically' do
     expect(worker.sidekiq_options['queue'])
-      .to eq 'pipeline_default'
-  end
-
-  describe '.enqueue_in' do
-    it 'sets a custom sidekiq queue with prefix and group' do
-      worker.enqueue_in(group: :processing)
-
-      expect(worker.sidekiq_options['queue'])
-        .to eq 'pipeline_processing'
-    end
+      .to eq 'pipeline_default:dummy'
   end
 end
diff --git a/spec/workers/concerns/repository_check_queue_spec.rb b/spec/workers/concerns/repository_check_queue_spec.rb
index fdbbfcc90a5..d2eeecfc9a8 100644
--- a/spec/workers/concerns/repository_check_queue_spec.rb
+++ b/spec/workers/concerns/repository_check_queue_spec.rb
@@ -13,7 +13,7 @@ describe RepositoryCheckQueue do
   end
 
   it 'sets the queue name of a worker' do
-    expect(worker.sidekiq_options['queue'].to_s).to eq('repository_check')
+    expect(worker.sidekiq_options['queue'].to_s).to eq('repository_check:dummy')
   end
 
   it 'disables retrying of failed jobs' do
diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb
index 7ee0a51a263..9e3b99b3502 100644
--- a/spec/workers/every_sidekiq_worker_spec.rb
+++ b/spec/workers/every_sidekiq_worker_spec.rb
@@ -1,21 +1,36 @@
 require 'spec_helper'
 
 describe 'Every Sidekiq worker' do
-  it 'includes ApplicationWorker' do
-    expect(Gitlab::SidekiqConfig.workers).to all(include(ApplicationWorker))
-  end
-
   it 'does not use the default queue' do
     expect(Gitlab::SidekiqConfig.workers.map(&:queue)).not_to include('default')
   end
 
   it 'uses the cronjob queue when the worker runs as a cronjob' do
-    expect(Gitlab::SidekiqConfig.cron_workers.map(&:queue)).to all(eq('cronjob'))
+    expect(Gitlab::SidekiqConfig.cron_workers.map(&:queue)).to all(start_with('cronjob:'))
+  end
+
+  it 'has its queue in app/workers/all_queues.yml', :aggregate_failures do
+    file_worker_queues = Gitlab::SidekiqConfig.worker_queues.to_set
+
+    worker_queues = Gitlab::SidekiqConfig.workers.map(&:queue).to_set
+    worker_queues << ActionMailer::DeliveryJob.queue_name
+    worker_queues << 'default'
+
+    missing_from_file = worker_queues - file_worker_queues
+    expect(missing_from_file).to be_empty, "expected #{missing_from_file.to_a.inspect} to be in app/workers/all_queues.yml"
+
+    unncessarily_in_file = file_worker_queues - worker_queues
+    expect(unncessarily_in_file).to be_empty, "expected #{unncessarily_in_file.to_a.inspect} not to be in app/workers/all_queues.yml"
   end
 
-  it 'defines the queue in the Sidekiq configuration file' do
-    config_queue_names = Gitlab::SidekiqConfig.config_queues.to_set
+  it 'has its queue or namespace in config/sidekiq_queues.yml', :aggregate_failures do
+    config_queues = Gitlab::SidekiqConfig.config_queues.to_set
+
+    Gitlab::SidekiqConfig.workers.each do |worker|
+      queue = worker.queue
+      queue_namespace = queue.split(':').first
 
-    expect(Gitlab::SidekiqConfig.worker_queues).to all(be_in(config_queue_names))
+      expect(config_queues).to include(queue).or(include(queue_namespace))
+    end
   end
 end