diff options
Diffstat (limited to 'spec/lib/gitlab/database/load_balancing')
10 files changed, 557 insertions, 90 deletions
diff --git a/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb b/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb new file mode 100644 index 00000000000..ebbbafb855f --- /dev/null +++ b/spec/lib/gitlab/database/load_balancing/action_cable_callbacks_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::LoadBalancing::ActionCableCallbacks, :request_store do + describe '.wrapper' do + it 'uses primary and then releases the connection and clears the session' do + expect(Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer, :release_host) + expect(Gitlab::Database::LoadBalancing::Session).to receive(:clear_session) + + described_class.wrapper.call( + nil, + lambda do + expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).to eq(true) + end + ) + end + + context 'with an exception' do + it 'releases the connection and clears the session' do + expect(Gitlab::Database::LoadBalancing).to receive_message_chain(:proxy, :load_balancer, :release_host) + expect(Gitlab::Database::LoadBalancing::Session).to receive(:clear_session) + + expect do + described_class.wrapper.call(nil, lambda { raise 'test_exception' }) + end.to raise_error('test_exception') + end + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/configuration_spec.rb b/spec/lib/gitlab/database/load_balancing/configuration_spec.rb new file mode 100644 index 00000000000..6621e6276a5 --- /dev/null +++ b/spec/lib/gitlab/database/load_balancing/configuration_spec.rb @@ -0,0 +1,175 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::LoadBalancing::Configuration do + let(:model) do + config = ActiveRecord::DatabaseConfigurations::HashConfig + .new('main', 'test', configuration_hash) + + double(:model, connection_db_config: config) + end + + describe '.for_model' do + context 'when load balancing is not configured' do + let(:configuration_hash) { {} } + + it 'uses the default settings' do + config = described_class.for_model(model) + + expect(config.hosts).to eq([]) + expect(config.max_replication_difference).to eq(8.megabytes) + expect(config.max_replication_lag_time).to eq(60.0) + expect(config.replica_check_interval).to eq(60.0) + expect(config.service_discovery).to eq( + nameserver: 'localhost', + port: 8600, + record: nil, + record_type: 'A', + interval: 60, + disconnect_timeout: 120, + use_tcp: false + ) + expect(config.pool_size).to eq(Gitlab::Database.default_pool_size) + end + end + + context 'when load balancing is configured' do + let(:configuration_hash) do + { + pool: 4, + load_balancing: { + max_replication_difference: 1, + max_replication_lag_time: 2, + replica_check_interval: 3, + hosts: %w[foo bar], + discover: { + 'record' => 'foo.example.com' + } + } + } + end + + it 'uses the custom configuration settings' do + config = described_class.for_model(model) + + expect(config.hosts).to eq(%w[foo bar]) + expect(config.max_replication_difference).to eq(1) + expect(config.max_replication_lag_time).to eq(2.0) + expect(config.replica_check_interval).to eq(3.0) + expect(config.service_discovery).to eq( + nameserver: 'localhost', + port: 8600, + record: 'foo.example.com', + record_type: 'A', + interval: 60, + disconnect_timeout: 120, + use_tcp: false + ) + expect(config.pool_size).to eq(4) + end + end + + context 'when the load balancing configuration uses strings as the keys' do + let(:configuration_hash) do + { + pool: 4, + load_balancing: { + 'max_replication_difference' => 1, + 'max_replication_lag_time' => 2, + 'replica_check_interval' => 3, + 'hosts' => %w[foo bar], + 'discover' => { + 'record' => 'foo.example.com' + } + } + } + end + + it 'uses the custom configuration settings' do + config = described_class.for_model(model) + + expect(config.hosts).to eq(%w[foo bar]) + expect(config.max_replication_difference).to eq(1) + expect(config.max_replication_lag_time).to eq(2.0) + expect(config.replica_check_interval).to eq(3.0) + expect(config.service_discovery).to eq( + nameserver: 'localhost', + port: 8600, + record: 'foo.example.com', + record_type: 'A', + interval: 60, + disconnect_timeout: 120, + use_tcp: false + ) + expect(config.pool_size).to eq(4) + end + end + end + + describe '#load_balancing_enabled?' do + it 'returns true when hosts are configured' do + config = described_class.new(ActiveRecord::Base, %w[foo bar]) + + expect(config.load_balancing_enabled?).to eq(true) + end + + it 'returns true when a service discovery record is configured' do + config = described_class.new(ActiveRecord::Base) + config.service_discovery[:record] = 'foo' + + expect(config.load_balancing_enabled?).to eq(true) + end + + it 'returns false when no hosts are configured and service discovery is disabled' do + config = described_class.new(ActiveRecord::Base) + + expect(config.load_balancing_enabled?).to eq(false) + end + end + + describe '#service_discovery_enabled?' do + it 'returns true when a record is configured' do + config = described_class.new(ActiveRecord::Base) + config.service_discovery[:record] = 'foo' + + expect(config.service_discovery_enabled?).to eq(true) + end + + it 'returns false when no record is configured' do + config = described_class.new(ActiveRecord::Base) + + expect(config.service_discovery_enabled?).to eq(false) + end + end + + describe '#pool_size' do + context 'when a custom pool size is used' do + let(:configuration_hash) { { pool: 4 } } + + it 'always reads the value from the model configuration' do + config = described_class.new(model) + + expect(config.pool_size).to eq(4) + + # We can't modify `configuration_hash` as it's only used to populate the + # internal hash used by ActiveRecord; instead of it being used as-is. + allow(model.connection_db_config) + .to receive(:configuration_hash) + .and_return({ pool: 42 }) + + expect(config.pool_size).to eq(42) + end + end + + context 'when the pool size is nil' do + let(:configuration_hash) { {} } + + it 'returns the default pool size' do + config = described_class.new(model) + + expect(config.pool_size).to eq(Gitlab::Database.default_pool_size) + end + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb b/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb index 0ca99ec9acf..ba2f9485066 100644 --- a/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/connection_proxy_spec.rb @@ -3,7 +3,12 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do - let(:proxy) { described_class.new } + let(:proxy) do + config = Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base) + + described_class.new(Gitlab::Database::LoadBalancing::LoadBalancer.new(config)) + end describe '#select' do it 'performs a read' do @@ -35,9 +40,15 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do describe 'using a SELECT FOR UPDATE query' do it 'runs the query on the primary and sticks to it' do arel = double(:arel, locked: true) + session = Gitlab::Database::LoadBalancing::Session.new + + allow(Gitlab::Database::LoadBalancing::Session).to receive(:current) + .and_return(session) + + expect(session).to receive(:write!) expect(proxy).to receive(:write_using_load_balancer) - .with(:select_all, arel, 'foo', [], sticky: true) + .with(:select_all, arel, 'foo', []) proxy.select_all(arel, 'foo') end @@ -58,8 +69,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do Gitlab::Database::LoadBalancing::ConnectionProxy::STICKY_WRITES.each do |name| describe "#{name}" do it 'runs the query on the primary and sticks to it' do - expect(proxy).to receive(:write_using_load_balancer) - .with(name, 'foo', sticky: true) + session = Gitlab::Database::LoadBalancing::Session.new + + allow(Gitlab::Database::LoadBalancing::Session).to receive(:current) + .and_return(session) + + expect(session).to receive(:write!) + expect(proxy).to receive(:write_using_load_balancer).with(name, 'foo') proxy.send(name, 'foo') end @@ -108,7 +124,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do # We have an extra test for #transaction here to make sure that nested queries # are also sent to a primary. describe '#transaction' do - let(:session) { double(:session) } + let(:session) { Gitlab::Database::LoadBalancing::Session.new } before do allow(Gitlab::Database::LoadBalancing::Session).to receive(:current) @@ -192,7 +208,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do proxy.foo('foo') end - it 'properly forwards trailing hash arguments' do + it 'properly forwards keyword arguments' do allow(proxy.load_balancer).to receive(:read_write) expect(proxy).to receive(:write_using_load_balancer).and_call_original @@ -217,7 +233,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do proxy.foo('foo') end - it 'properly forwards trailing hash arguments' do + it 'properly forwards keyword arguments' do allow(proxy.load_balancer).to receive(:read) expect(proxy).to receive(:read_using_load_balancer).and_call_original @@ -297,20 +313,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::ConnectionProxy do .and_return(session) end - it 'uses but does not stick to the primary when sticking is disabled' do + it 'uses but does not stick to the primary' do expect(proxy.load_balancer).to receive(:read_write).and_yield(connection) expect(connection).to receive(:foo).with('foo') expect(session).not_to receive(:write!) proxy.write_using_load_balancer(:foo, 'foo') end - - it 'sticks to the primary when sticking is enabled' do - expect(proxy.load_balancer).to receive(:read_write).and_yield(connection) - expect(connection).to receive(:foo).with('foo') - expect(session).to receive(:write!) - - proxy.write_using_load_balancer(:foo, 'foo', sticky: true) - end end end diff --git a/spec/lib/gitlab/database/load_balancing/host_list_spec.rb b/spec/lib/gitlab/database/load_balancing/host_list_spec.rb index ad4ca18d5e6..9bb8116c434 100644 --- a/spec/lib/gitlab/database/load_balancing/host_list_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/host_list_spec.rb @@ -4,7 +4,12 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::HostList do let(:db_host) { ActiveRecord::Base.connection_pool.db_config.host } - let(:load_balancer) { double(:load_balancer) } + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) + ) + end + let(:host_count) { 2 } let(:hosts) { Array.new(host_count) { Gitlab::Database::LoadBalancing::Host.new(db_host, load_balancer, port: 5432) } } let(:host_list) { described_class.new(hosts) } diff --git a/spec/lib/gitlab/database/load_balancing/host_spec.rb b/spec/lib/gitlab/database/load_balancing/host_spec.rb index f42ac8be1bb..e2011692228 100644 --- a/spec/lib/gitlab/database/load_balancing/host_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/host_spec.rb @@ -3,7 +3,10 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::Host do - let(:load_balancer) { Gitlab::Database::LoadBalancing::LoadBalancer.new } + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer + .new(Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base)) + end let(:host) do Gitlab::Database::LoadBalancing::Host.new('localhost', load_balancer) @@ -274,7 +277,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do end it 'returns false when the data is not recent enough' do - diff = Gitlab::Database::LoadBalancing.max_replication_difference * 2 + diff = load_balancer.configuration.max_replication_difference * 2 expect(host) .to receive(:query_and_release) diff --git a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb index c647f5a8f5d..86fae14b961 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -5,7 +5,12 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do let(:conflict_error) { Class.new(RuntimeError) } let(:db_host) { ActiveRecord::Base.connection_pool.db_config.host } - let(:lb) { described_class.new([db_host, db_host]) } + let(:config) do + Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, [db_host, db_host]) + end + + let(:lb) { described_class.new(config) } let(:request_cache) { lb.send(:request_cache) } before do @@ -41,6 +46,19 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do top_error end + describe '#initialize' do + it 'ignores the hosts when the primary_only option is enabled' do + config = Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, [db_host]) + lb = described_class.new(config, primary_only: true) + hosts = lb.host_list.hosts + + expect(hosts.length).to eq(1) + expect(hosts.first) + .to be_instance_of(Gitlab::Database::LoadBalancing::PrimaryHost) + end + end + describe '#read' do it 'yields a connection for a read' do connection = double(:connection) @@ -121,6 +139,19 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do expect { |b| lb.read(&b) } .to yield_with_args(ActiveRecord::Base.retrieve_connection) end + + it 'uses the primary when the primary_only option is enabled' do + config = Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base) + lb = described_class.new(config, primary_only: true) + + # When no hosts are configured, we don't want to produce any warnings, as + # they aren't useful/too noisy. + expect(Gitlab::Database::LoadBalancing::Logger).not_to receive(:warn) + + expect { |b| lb.read(&b) } + .to yield_with_args(ActiveRecord::Base.retrieve_connection) + end end describe '#read_write' do @@ -152,8 +183,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end it 'does not create conflicts with other load balancers when caching hosts' do - lb1 = described_class.new([db_host, db_host], ActiveRecord::Base) - lb2 = described_class.new([db_host, db_host], Ci::CiDatabaseRecord) + ci_config = Gitlab::Database::LoadBalancing::Configuration + .new(Ci::CiDatabaseRecord, [db_host, db_host]) + + lb1 = described_class.new(config) + lb2 = described_class.new(ci_config) host1 = lb1.host host2 = lb2.host @@ -283,6 +317,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do expect(lb.connection_error?(error)).to eq(false) end + + it 'returns false for ActiveRecord errors without a cause' do + error = ActiveRecord::RecordNotUnique.new + + expect(lb.connection_error?(error)).to eq(false) + end end describe '#serialization_failure?' do diff --git a/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb b/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb new file mode 100644 index 00000000000..a0e63a7ee4e --- /dev/null +++ b/spec/lib/gitlab/database/load_balancing/primary_host_spec.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::LoadBalancing::PrimaryHost do + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) + ) + end + + let(:host) { Gitlab::Database::LoadBalancing::PrimaryHost.new(load_balancer) } + + describe '#connection' do + it 'returns a connection from the pool' do + expect(load_balancer.pool).to receive(:connection) + + host.connection + end + end + + describe '#release_connection' do + it 'does nothing' do + expect(host.release_connection).to be_nil + end + end + + describe '#enable_query_cache!' do + it 'does nothing' do + expect(host.enable_query_cache!).to be_nil + end + end + + describe '#disable_query_cache!' do + it 'does nothing' do + expect(host.disable_query_cache!).to be_nil + end + end + + describe '#query_cache_enabled' do + it 'delegates to the primary connection pool' do + expect(host.query_cache_enabled) + .to eq(load_balancer.pool.query_cache_enabled) + end + end + + describe '#disconnect!' do + it 'does nothing' do + expect(host.disconnect!).to be_nil + end + end + + describe '#offline!' do + it 'does nothing' do + expect(host.offline!).to be_nil + end + end + + describe '#online?' do + it 'returns true' do + expect(host.online?).to eq(true) + end + end + + describe '#primary_write_location' do + it 'returns the write location of the primary' do + expect(host.primary_write_location).to be_an_instance_of(String) + expect(host.primary_write_location).not_to be_empty + end + end + + describe '#caught_up?' do + it 'returns true' do + expect(host.caught_up?('foo')).to eq(true) + end + end + + describe '#database_replica_location' do + let(:connection) { double(:connection) } + + it 'returns the write ahead location of the replica', :aggregate_failures do + expect(host) + .to receive(:query_and_release) + .and_return({ 'location' => '0/D525E3A8' }) + + expect(host.database_replica_location).to be_an_instance_of(String) + end + + it 'returns nil when the database query returned no rows' do + expect(host).to receive(:query_and_release).and_return({}) + + expect(host.database_replica_location).to be_nil + end + + it 'returns nil when the database connection fails' do + allow(host).to receive(:connection).and_raise(PG::Error) + + expect(host.database_replica_location).to be_nil + end + end + + describe '#query_and_release' do + it 'executes a SQL query' do + results = host.query_and_release('SELECT 10 AS number') + + expect(results).to be_an_instance_of(Hash) + expect(results['number'].to_i).to eq(10) + end + + it 'releases the connection after running the query' do + expect(host) + .to receive(:release_connection) + .once + + host.query_and_release('SELECT 10 AS number') + end + + it 'returns an empty Hash in the event of an error' do + expect(host.connection) + .to receive(:select_all) + .and_raise(RuntimeError, 'kittens') + + expect(host.query_and_release('SELECT 10 AS number')).to eq({}) + end + end +end diff --git a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb index a27341a3324..e9bc465b1c7 100644 --- a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb @@ -3,13 +3,18 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do - let(:load_balancer) { Gitlab::Database::LoadBalancing::LoadBalancer.new([]) } + let(:load_balancer) do + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base) + ) + end + let(:service) do described_class.new( + load_balancer, nameserver: 'localhost', port: 8600, - record: 'foo', - load_balancer: load_balancer + record: 'foo' ) end @@ -26,11 +31,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do describe ':record_type' do subject do described_class.new( + load_balancer, nameserver: 'localhost', port: 8600, record: 'foo', - record_type: record_type, - load_balancer: load_balancer + record_type: record_type ) end @@ -69,18 +74,69 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do end describe '#perform_service_discovery' do - it 'reports exceptions to Sentry' do - error = StandardError.new + context 'without any failures' do + it 'runs once' do + expect(service) + .to receive(:refresh_if_necessary).once + + expect(service).not_to receive(:sleep) + + expect(Gitlab::ErrorTracking).not_to receive(:track_exception) + + service.perform_service_discovery + end + end + context 'with failures' do + before do + allow(Gitlab::ErrorTracking).to receive(:track_exception) + allow(service).to receive(:sleep) + end + + let(:valid_retry_sleep_duration) { satisfy { |val| described_class::RETRY_DELAY_RANGE.include?(val) } } + + it 'retries service discovery when under the retry limit' do + error = StandardError.new + + expect(service) + .to receive(:refresh_if_necessary) + .and_raise(error).exactly(described_class::MAX_DISCOVERY_RETRIES - 1).times.ordered + + expect(service) + .to receive(:sleep).with(valid_retry_sleep_duration) + .exactly(described_class::MAX_DISCOVERY_RETRIES - 1).times + + expect(service).to receive(:refresh_if_necessary).and_return(45).ordered + + expect(service.perform_service_discovery).to eq(45) + end + + it 'does not retry service discovery after exceeding the limit' do + error = StandardError.new + + expect(service) + .to receive(:refresh_if_necessary) + .and_raise(error).exactly(described_class::MAX_DISCOVERY_RETRIES).times + + expect(service) + .to receive(:sleep).with(valid_retry_sleep_duration) + .exactly(described_class::MAX_DISCOVERY_RETRIES).times + + service.perform_service_discovery + end - expect(service) - .to receive(:refresh_if_necessary) - .and_raise(error) + it 'reports exceptions to Sentry' do + error = StandardError.new + + expect(service) + .to receive(:refresh_if_necessary) + .and_raise(error).exactly(described_class::MAX_DISCOVERY_RETRIES).times - expect(Gitlab::ErrorTracking) - .to receive(:track_exception) - .with(error) + expect(Gitlab::ErrorTracking) + .to receive(:track_exception) + .with(error).exactly(described_class::MAX_DISCOVERY_RETRIES).times - service.perform_service_discovery + service.perform_service_discovery + end end end @@ -133,7 +189,10 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do let(:address_bar) { described_class::Address.new('bar') } let(:load_balancer) do - Gitlab::Database::LoadBalancing::LoadBalancer.new([address_foo]) + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, [address_foo]) + ) end before do @@ -166,11 +225,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do describe '#addresses_from_dns' do let(:service) do described_class.new( + load_balancer, nameserver: 'localhost', port: 8600, record: 'foo', - record_type: record_type, - load_balancer: load_balancer + record_type: record_type ) end @@ -224,6 +283,16 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do expect(service.addresses_from_dns).to eq([90, addresses]) end end + + context 'when the resolver returns an empty response' do + let(:packet) { double(:packet, answer: []) } + + let(:record_type) { 'A' } + + it 'raises EmptyDnsResponse' do + expect { service.addresses_from_dns }.to raise_error(Gitlab::Database::LoadBalancing::ServiceDiscovery::EmptyDnsResponse) + end + end end describe '#new_wait_time_for' do @@ -246,7 +315,10 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery do describe '#addresses_from_load_balancer' do let(:load_balancer) do - Gitlab::Database::LoadBalancing::LoadBalancer.new(%w[b a]) + Gitlab::Database::LoadBalancing::LoadBalancer.new( + Gitlab::Database::LoadBalancing::Configuration + .new(ActiveRecord::Base, %w[b a]) + ) end it 'returns the ordered host names of the load balancer' do diff --git a/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb index 54050a87af0..f683ade978a 100644 --- a/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sidekiq_client_middleware_spec.rb @@ -58,8 +58,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do it 'does not pass database locations', :aggregate_failures do run_middleware - expect(job['database_replica_location']).to be_nil - expect(job['database_write_location']).to be_nil + expect(job['wal_locations']).to be_nil end include_examples 'job data consistency' @@ -86,11 +85,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end it 'passes database_replica_location' do + expected_location = { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } + expect(load_balancer).to receive_message_chain(:host, "database_replica_location").and_return(location) run_middleware - expect(job['database_replica_location']).to eq(location) + expect(job['wal_locations']).to eq(expected_location) end include_examples 'job data consistency' @@ -102,40 +103,56 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end it 'passes primary write location', :aggregate_failures do + expected_location = { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } + expect(load_balancer).to receive(:primary_write_location).and_return(location) run_middleware - expect(job['database_write_location']).to eq(location) + expect(job['wal_locations']).to eq(expected_location) end include_examples 'job data consistency' end end - shared_examples_for 'database location was already provided' do |provided_database_location, other_location| - shared_examples_for 'does not set database location again' do |use_primary| - before do - allow(Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary?).and_return(use_primary) - end + context 'when worker cannot be constantized' do + let(:worker_class) { 'ActionMailer::MailDeliveryJob' } + let(:expected_consistency) { :always } - it 'does not set database locations again' do - run_middleware + include_examples 'does not pass database locations' + end - expect(job[provided_database_location]).to eq(old_location) - expect(job[other_location]).to be_nil - end - end + context 'when worker class does not include ApplicationWorker' do + let(:worker_class) { ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper } + let(:expected_consistency) { :always } + + include_examples 'does not pass database locations' + end + context 'database wal location was already provided' do let(:old_location) { '0/D525E3A8' } let(:new_location) { 'AB/12345' } - let(:job) { { "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", provided_database_location => old_location } } + let(:wal_locations) { { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => old_location } } + let(:job) { { "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'wal_locations' => wal_locations } } before do allow(load_balancer).to receive(:primary_write_location).and_return(new_location) allow(load_balancer).to receive(:database_replica_location).and_return(new_location) end + shared_examples_for 'does not set database location again' do |use_primary| + before do + allow(Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary?).and_return(use_primary) + end + + it 'does not set database locations again' do + run_middleware + + expect(job['wal_locations']).to eq(wal_locations) + end + end + context "when write was performed" do include_examples 'does not set database location again', true end @@ -145,28 +162,6 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqClientMiddleware do end end - context 'when worker cannot be constantized' do - let(:worker_class) { 'ActionMailer::MailDeliveryJob' } - let(:expected_consistency) { :always } - - include_examples 'does not pass database locations' - end - - context 'when worker class does not include ApplicationWorker' do - let(:worker_class) { ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper } - let(:expected_consistency) { :always } - - include_examples 'does not pass database locations' - end - - context 'database write location was already provided' do - include_examples 'database location was already provided', 'database_write_location', 'database_replica_location' - end - - context 'database replica location was already provided' do - include_examples 'database location was already provided', 'database_replica_location', 'database_write_location' - end - context 'when worker data consistency is :always' do include_context 'data consistency worker class', :always, :load_balancing_for_test_data_consistency_worker diff --git a/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb index 14f240cd159..9f23eb0094f 100644 --- a/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sidekiq_server_middleware_spec.rb @@ -62,9 +62,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do include_examples 'load balancing strategy', expected_strategy end - shared_examples_for 'replica is up to date' do |location, expected_strategy| + shared_examples_for 'replica is up to date' do |expected_strategy| + let(:location) {'0/D525E3A8' } + let(:wal_locations) { { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } } + it 'does not stick to the primary', :aggregate_failures do - expect(middleware).to receive(:replica_caught_up?).with(location).and_return(true) + expect(load_balancer).to receive(:select_up_to_date_host).with(location).and_return(true) run_middleware do expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).not_to be_truthy @@ -85,30 +88,40 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do include_examples 'stick to the primary', 'primary' end - context 'when database replica location is set' do - let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'database_replica_location' => '0/D525E3A8' } } + context 'when database wal location is set' do + let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'wal_locations' => wal_locations } } + + before do + allow(load_balancer).to receive(:select_up_to_date_host).with(location).and_return(true) + end + + it_behaves_like 'replica is up to date', 'replica' + end + + context 'when deduplication wal location is set' do + let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'dedup_wal_locations' => wal_locations } } before do - allow(middleware).to receive(:replica_caught_up?).and_return(true) + allow(load_balancer).to receive(:select_up_to_date_host).with(wal_locations[:main]).and_return(true) end - it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica' + it_behaves_like 'replica is up to date', 'replica' end - context 'when database primary location is set' do + context 'when legacy wal location is set' do let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e', 'database_write_location' => '0/D525E3A8' } } before do - allow(middleware).to receive(:replica_caught_up?).and_return(true) + allow(load_balancer).to receive(:select_up_to_date_host).with('0/D525E3A8').and_return(true) end - it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica' + it_behaves_like 'replica is up to date', 'replica' end context 'when database location is not set' do let(:job) { { 'job_id' => 'a180b47c-3fd6-41b8-81e9-34da61c3400e' } } - it_behaves_like 'stick to the primary', 'primary_no_wal' + include_examples 'stick to the primary', 'primary_no_wal' end end @@ -167,7 +180,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do replication_lag!(false) end - it_behaves_like 'replica is up to date', '0/D525E3A8', 'replica_retried' + include_examples 'replica is up to date', 'replica_retried' end end end @@ -178,7 +191,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware do context 'when replica is not up to date' do before do - allow(middleware).to receive(:replica_caught_up?).and_return(false) + allow(load_balancer).to receive(:select_up_to_date_host).and_return(false) end include_examples 'stick to the primary', 'primary' |