diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-05-19 18:44:42 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-05-19 18:44:42 +0300 |
commit | 4555e1b21c365ed8303ffb7a3325d773c9b8bf31 (patch) | |
tree | 5423a1c7516cffe36384133ade12572cf709398d /spec/lib/gitlab/sidekiq_config | |
parent | e570267f2f6b326480d284e0164a6464ba4081bc (diff) |
Add latest changes from gitlab-org/gitlab@13-12-stable-eev13.12.0-rc42
Diffstat (limited to 'spec/lib/gitlab/sidekiq_config')
-rw-r--r-- | spec/lib/gitlab/sidekiq_config/worker_matcher_spec.rb | 82 | ||||
-rw-r--r-- | spec/lib/gitlab/sidekiq_config/worker_router_spec.rb | 212 | ||||
-rw-r--r-- | spec/lib/gitlab/sidekiq_config/worker_spec.rb | 3 |
3 files changed, 261 insertions, 36 deletions
diff --git a/spec/lib/gitlab/sidekiq_config/worker_matcher_spec.rb b/spec/lib/gitlab/sidekiq_config/worker_matcher_spec.rb index 75e9c8c100b..dfe9358f70b 100644 --- a/spec/lib/gitlab/sidekiq_config/worker_matcher_spec.rb +++ b/spec/lib/gitlab/sidekiq_config/worker_matcher_spec.rb @@ -11,6 +11,7 @@ RSpec.describe Gitlab::SidekiqConfig::WorkerMatcher do [ { name: 'a', + worker_name: 'WorkerA', feature_category: :category_a, has_external_dependencies: false, urgency: :low, @@ -19,6 +20,7 @@ RSpec.describe Gitlab::SidekiqConfig::WorkerMatcher do }, { name: 'a:2', + worker_name: 'WorkerA2', feature_category: :category_a, has_external_dependencies: false, urgency: :high, @@ -27,6 +29,7 @@ RSpec.describe Gitlab::SidekiqConfig::WorkerMatcher do }, { name: 'b', + worker_name: 'WorkerB', feature_category: :category_b, has_external_dependencies: true, urgency: :high, @@ -35,6 +38,7 @@ RSpec.describe Gitlab::SidekiqConfig::WorkerMatcher do }, { name: 'c', + worker_name: 'WorkerC', feature_category: :category_c, has_external_dependencies: false, urgency: :throttled, @@ -46,56 +50,62 @@ RSpec.describe Gitlab::SidekiqConfig::WorkerMatcher do context 'with valid input' do where(:query, :expected_metadatas) do + # worker_name + 'worker_name=WorkerA' | %w(WorkerA) + 'worker_name=WorkerA2' | %w(WorkerA2) + 'worker_name=WorkerB|worker_name=WorkerD' | %w(WorkerB) + 'worker_name!=WorkerA' | %w(WorkerA2 WorkerB WorkerC) + # feature_category - 'feature_category=category_a' | %w(a a:2) - 'feature_category=category_a,category_c' | %w(a a:2 c) - 'feature_category=category_a|feature_category=category_c' | %w(a a:2 c) - 'feature_category!=category_a' | %w(b c) + 'feature_category=category_a' | %w(WorkerA WorkerA2) + 'feature_category=category_a,category_c' | %w(WorkerA WorkerA2 WorkerC) + 'feature_category=category_a|feature_category=category_c' | %w(WorkerA WorkerA2 WorkerC) + 'feature_category!=category_a' | %w(WorkerB WorkerC) # has_external_dependencies - 'has_external_dependencies=true' | %w(b) - 'has_external_dependencies=false' | %w(a a:2 c) - 'has_external_dependencies=true,false' | %w(a a:2 b c) - 'has_external_dependencies=true|has_external_dependencies=false' | %w(a a:2 b c) - 'has_external_dependencies!=true' | %w(a a:2 c) + 'has_external_dependencies=true' | %w(WorkerB) + 'has_external_dependencies=false' | %w(WorkerA WorkerA2 WorkerC) + 'has_external_dependencies=true,false' | %w(WorkerA WorkerA2 WorkerB WorkerC) + 'has_external_dependencies=true|has_external_dependencies=false' | %w(WorkerA WorkerA2 WorkerB WorkerC) + 'has_external_dependencies!=true' | %w(WorkerA WorkerA2 WorkerC) # urgency - 'urgency=high' | %w(a:2 b) - 'urgency=low' | %w(a) - 'urgency=high,low,throttled' | %w(a a:2 b c) - 'urgency=low|urgency=throttled' | %w(a c) - 'urgency!=high' | %w(a c) + 'urgency=high' | %w(WorkerA2 WorkerB) + 'urgency=low' | %w(WorkerA) + 'urgency=high,low,throttled' | %w(WorkerA WorkerA2 WorkerB WorkerC) + 'urgency=low|urgency=throttled' | %w(WorkerA WorkerC) + 'urgency!=high' | %w(WorkerA WorkerC) # name - 'name=a' | %w(a) - 'name=a,b' | %w(a b) - 'name=a,a:2|name=b' | %w(a a:2 b) - 'name!=a,a:2' | %w(b c) + 'name=a' | %w(WorkerA) + 'name=a,b' | %w(WorkerA WorkerB) + 'name=a,a:2|name=b' | %w(WorkerA WorkerA2 WorkerB) + 'name!=a,a:2' | %w(WorkerB WorkerC) # resource_boundary - 'resource_boundary=memory' | %w(b c) - 'resource_boundary=memory,cpu' | %w(a b c) - 'resource_boundary=memory|resource_boundary=cpu' | %w(a b c) - 'resource_boundary!=memory,cpu' | %w(a:2) + 'resource_boundary=memory' | %w(WorkerB WorkerC) + 'resource_boundary=memory,cpu' | %w(WorkerA WorkerB WorkerC) + 'resource_boundary=memory|resource_boundary=cpu' | %w(WorkerA WorkerB WorkerC) + 'resource_boundary!=memory,cpu' | %w(WorkerA2) # tags - 'tags=no_disk_io' | %w(a b) - 'tags=no_disk_io,git_access' | %w(a a:2 b) - 'tags=no_disk_io|tags=git_access' | %w(a a:2 b) - 'tags=no_disk_io&tags=git_access' | %w(a) - 'tags!=no_disk_io' | %w(a:2 c) - 'tags!=no_disk_io,git_access' | %w(c) + 'tags=no_disk_io' | %w(WorkerA WorkerB) + 'tags=no_disk_io,git_access' | %w(WorkerA WorkerA2 WorkerB) + 'tags=no_disk_io|tags=git_access' | %w(WorkerA WorkerA2 WorkerB) + 'tags=no_disk_io&tags=git_access' | %w(WorkerA) + 'tags!=no_disk_io' | %w(WorkerA2 WorkerC) + 'tags!=no_disk_io,git_access' | %w(WorkerC) 'tags=unknown_tag' | [] - 'tags!=no_disk_io' | %w(a:2 c) - 'tags!=no_disk_io,git_access' | %w(c) - 'tags!=unknown_tag' | %w(a a:2 b c) + 'tags!=no_disk_io' | %w(WorkerA2 WorkerC) + 'tags!=no_disk_io,git_access' | %w(WorkerC) + 'tags!=unknown_tag' | %w(WorkerA WorkerA2 WorkerB WorkerC) # combinations - 'feature_category=category_a&urgency=high' | %w(a:2) - 'feature_category=category_a&urgency=high|feature_category=category_c' | %w(a:2 c) + 'feature_category=category_a&urgency=high' | %w(WorkerA2) + 'feature_category=category_a&urgency=high|feature_category=category_c' | %w(WorkerA2 WorkerC) # Match all - '*' | %w(a a:2 b c) + '*' | %w(WorkerA WorkerA2 WorkerB WorkerC) end with_them do @@ -103,7 +113,7 @@ RSpec.describe Gitlab::SidekiqConfig::WorkerMatcher do matched_metadatas = worker_metadatas.select do |metadata| described_class.new(query).match?(metadata) end - expect(matched_metadatas.map { |m| m[:name] }).to match_array(expected_metadatas) + expect(matched_metadatas.map { |m| m[:worker_name] }).to match_array(expected_metadatas) end end end @@ -113,7 +123,7 @@ RSpec.describe Gitlab::SidekiqConfig::WorkerMatcher do 'feature_category="category_a"' | described_class::InvalidTerm 'feature_category=' | described_class::InvalidTerm 'feature_category~category_a' | described_class::InvalidTerm - 'worker_name=a' | described_class::UnknownPredicate + 'invalid_term=a' | described_class::UnknownPredicate end with_them do diff --git a/spec/lib/gitlab/sidekiq_config/worker_router_spec.rb b/spec/lib/gitlab/sidekiq_config/worker_router_spec.rb new file mode 100644 index 00000000000..687e35813b1 --- /dev/null +++ b/spec/lib/gitlab/sidekiq_config/worker_router_spec.rb @@ -0,0 +1,212 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'rspec-parameterized' + +RSpec.describe Gitlab::SidekiqConfig::WorkerRouter do + describe '.queue_name_from_worker_name' do + using RSpec::Parameterized::TableSyntax + + def create_worker(name, namespace = nil) + Class.new.tap do |worker| + worker.define_singleton_method(:name) { name } + worker.define_singleton_method(:queue_namespace) { namespace } + end + end + + where(:worker, :expected_name) do + create_worker('PagesWorker') | 'pages' + create_worker('PipelineNotificationWorker') | 'pipeline_notification' + create_worker('PostReceive') | 'post_receive' + create_worker('PostReceive', :git) | 'git:post_receive' + create_worker('PipelineHooksWorker', :pipeline_hooks) | 'pipeline_hooks:pipeline_hooks' + create_worker('Gitlab::JiraImport::AdvanceStageWorker') | 'jira_import_advance_stage' + create_worker('Gitlab::PhabricatorImport::ImportTasksWorker', :importer) | 'importer:phabricator_import_import_tasks' + end + + with_them do + it 'generates a valid queue name from worker name' do + expect(described_class.queue_name_from_worker_name(worker)).to eql(expected_name) + end + end + end + + shared_context 'router examples setup' do + using RSpec::Parameterized::TableSyntax + + let(:worker) do + Class.new do + def self.name + 'Gitlab::Foo::BarWorker' + end + + include ApplicationWorker + + feature_category :feature_a + urgency :low + worker_resource_boundary :cpu + tags :expensive + end + end + + where(:routing_rules, :expected_queue) do + # Default, no configuration + [] | 'foo_bar' + # Does not match, fallback to the named queue + [ + ['feature_category=feature_b|urgency=high', 'queue_a'], + ['resource_boundary=memory', 'queue_b'], + ['tags=cheap', 'queue_c'] + ] | 'foo_bar' + # Match a nil queue, fallback to named queue + [ + ['feature_category=feature_b|urgency=high', 'queue_a'], + ['resource_boundary=cpu', nil], + ['tags=cheap', 'queue_c'] + ] | 'foo_bar' + # Match an empty string, fallback to named queue + [ + ['feature_category=feature_b|urgency=high', 'queue_a'], + ['resource_boundary=cpu', ''], + ['tags=cheap', 'queue_c'] + ] | 'foo_bar' + # Match the first rule + [ + ['feature_category=feature_a|urgency=high', 'queue_a'], + ['resource_boundary=cpu', 'queue_b'], + ['tags=cheap', 'queue_c'] + ] | 'queue_a' + # Match the first rule 2 + [ + ['feature_category=feature_b|urgency=low', 'queue_a'], + ['resource_boundary=cpu', 'queue_b'], + ['tags=cheap', 'queue_c'] + ] | 'queue_a' + # Match the third rule + [ + ['feature_category=feature_b|urgency=high', 'queue_a'], + ['resource_boundary=memory', 'queue_b'], + ['tags=expensive', 'queue_c'] + ] | 'queue_c' + # Match all, first match wins + [ + ['feature_category=feature_a|urgency=low', 'queue_a'], + ['resource_boundary=cpu', 'queue_b'], + ['tags=expensive', 'queue_c'] + ] | 'queue_a' + # Match the same rule multiple times, the first match wins + [ + ['feature_category=feature_a', 'queue_a'], + ['feature_category=feature_a', 'queue_b'], + ['feature_category=feature_a', 'queue_c'] + ] | 'queue_a' + # Match wildcard + [ + ['feature_category=feature_b|urgency=high', 'queue_a'], + ['resource_boundary=memory', 'queue_b'], + ['tags=cheap', 'queue_c'], + ['*', 'default'] + ] | 'default' + # Match wildcard at the top of the chain. It makes the following rules useless + [ + ['*', 'queue_foo'], + ['feature_category=feature_a|urgency=low', 'queue_a'], + ['resource_boundary=cpu', 'queue_b'], + ['tags=expensive', 'queue_c'] + ] | 'queue_foo' + end + end + + describe '.global' do + before do + described_class.remove_instance_variable(:@global_worker_router) if described_class.instance_variable_defined?(:@global_worker_router) + end + + after do + described_class.remove_instance_variable(:@global_worker_router) + end + + context 'valid routing rules' do + include_context 'router examples setup' + + with_them do + before do + stub_config(sidekiq: { routing_rules: routing_rules }) + end + + it 'routes the worker to the correct queue' do + expect(described_class.global.route(worker)).to eql(expected_queue) + end + end + end + + context 'invalid routing rules' do + let(:worker) do + Class.new do + def self.name + 'Gitlab::Foo::BarWorker' + end + + include ApplicationWorker + end + end + + before do + stub_config(sidekiq: { routing_rules: routing_rules }) + end + + context 'invalid routing rules format' do + let(:routing_rules) { ['feature_category=a'] } + + it 'captures the error and falls back to an empty route' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with(be_a(described_class::InvalidRoutingRuleError)) + + expect(described_class.global.route(worker)).to eql('foo_bar') + end + end + + context 'invalid predicate' do + let(:routing_rules) { [['invalid_term=a', 'queue_a']] } + + it 'captures the error and falls back to an empty route' do + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with( + be_a(Gitlab::SidekiqConfig::WorkerMatcher::UnknownPredicate) + ) + + expect(described_class.global.route(worker)).to eql('foo_bar') + end + end + end + end + + describe '#route' do + context 'valid routing rules' do + include_context 'router examples setup' + + with_them do + it 'routes the worker to the correct queue' do + router = described_class.new(routing_rules) + + expect(router.route(worker)).to eql(expected_queue) + end + end + end + + context 'invalid routing rules' do + it 'raises an exception' do + expect { described_class.new(nil) }.to raise_error(described_class::InvalidRoutingRuleError) + expect { described_class.new(['feature_category=a']) }.to raise_error(described_class::InvalidRoutingRuleError) + expect { described_class.new([['feature_category=a', 'queue_a', 'queue_b']]) }.to raise_error(described_class::InvalidRoutingRuleError) + expect do + described_class.new( + [ + ['feature_category=a', 'queue_b'], + ['feature_category=b'] + ] + ) + end.to raise_error(described_class::InvalidRoutingRuleError) + expect { described_class.new([['invalid_term=a', 'queue_a']]) }.to raise_error(Gitlab::SidekiqConfig::WorkerMatcher::UnknownPredicate) + end + end + end +end diff --git a/spec/lib/gitlab/sidekiq_config/worker_spec.rb b/spec/lib/gitlab/sidekiq_config/worker_spec.rb index 05987f95b33..0c43c33ff8c 100644 --- a/spec/lib/gitlab/sidekiq_config/worker_spec.rb +++ b/spec/lib/gitlab/sidekiq_config/worker_spec.rb @@ -6,6 +6,7 @@ RSpec.describe Gitlab::SidekiqConfig::Worker do def create_worker(queue:, **attributes) namespace = queue.include?(':') && queue.split(':').first inner_worker = double( + name: attributes[:worker_name] || 'Foo::BarWorker', queue: queue, queue_namespace: namespace, get_feature_category: attributes[:feature_category], @@ -87,6 +88,7 @@ RSpec.describe Gitlab::SidekiqConfig::Worker do describe 'YAML encoding' do it 'encodes the worker in YAML as a hash of the queue' do attributes_a = { + worker_name: 'WorkerA', feature_category: :source_code_management, has_external_dependencies: false, urgency: :low, @@ -97,6 +99,7 @@ RSpec.describe Gitlab::SidekiqConfig::Worker do } attributes_b = { + worker_name: 'WorkerB', feature_category: :not_owned, has_external_dependencies: true, urgency: :high, |