diff options
Diffstat (limited to 'spec/lib/gitlab/database')
19 files changed, 1087 insertions, 556 deletions
diff --git a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb index 309bbf1e3f0..c5a20b5ef3d 100644 --- a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb @@ -141,6 +141,14 @@ RSpec.describe Gitlab::Database::AsyncIndexes::MigrationHelpers, feature_categor expect { migration.prepare_async_index(table_name, 'id') }.not_to raise_error end end + + context 'when the target table does not exist' do + it 'raises an error' do + expect { migration.prepare_async_index(:non_existent_table, 'id') }.to( + raise_error("Table non_existent_table does not exist") + ) + end + end end describe '#prepare_async_index_from_sql' do diff --git a/spec/lib/gitlab/database/click_house_client_spec.rb b/spec/lib/gitlab/database/click_house_client_spec.rb index 50086795b2b..6e63ae56557 100644 --- a/spec/lib/gitlab/database/click_house_client_spec.rb +++ b/spec/lib/gitlab/database/click_house_client_spec.rb @@ -2,98 +2,135 @@ require 'spec_helper' -RSpec.describe 'ClickHouse::Client', feature_category: :database do - context 'when click_house spec tag is not added' do - it 'does not have any ClickHouse databases configured' do - databases = ClickHouse::Client.configuration.databases +RSpec.describe 'ClickHouse::Client', :click_house, feature_category: :database do + it 'has a ClickHouse database configured' do + databases = ClickHouse::Client.configuration.databases - expect(databases).to be_empty - end + expect(databases).not_to be_empty end - describe 'when click_house spec tag is added', :click_house do - it 'has a ClickHouse database configured' do - databases = ClickHouse::Client.configuration.databases - - expect(databases).not_to be_empty - end + it 'does not return data via `execute` method' do + result = ClickHouse::Client.execute("SELECT 1 AS value", :main) - it 'does not return data via `execute` method' do - result = ClickHouse::Client.execute("SELECT 1 AS value", :main) + # does not return data, just true if successful. Otherwise error. + expect(result).to eq(true) + end - # does not return data, just true if successful. Otherwise error. - expect(result).to eq(true) - end + describe 'data manipulation' do + describe 'inserting' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project) } + + let_it_be(:author1) { create(:user).tap { |u| project.add_developer(u) } } + let_it_be(:author2) { create(:user).tap { |u| project.add_developer(u) } } + + let_it_be(:issue1) { create(:issue, project: project) } + let_it_be(:issue2) { create(:issue, project: project) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + let_it_be(:event1) { create(:event, :created, target: issue1, author: author1) } + let_it_be(:event2) { create(:event, :closed, target: issue2, author: author2) } + let_it_be(:event3) { create(:event, :merged, target: merge_request, author: author1) } + + let(:events) { [event1, event2, event3] } + + def format_row(event) + path = event.project.reload.project_namespace.traversal_ids.join('/') + + action = Event.actions[event.action] + [ + event.id, + "'#{path}/'", + event.author_id, + event.target_id, + "'#{event.target_type}'", + action, + event.created_at.to_f, + event.updated_at.to_f + ].join(',') + end - describe 'data manipulation' do - describe 'inserting' do - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project) } - - let_it_be(:author1) { create(:user).tap { |u| project.add_developer(u) } } - let_it_be(:author2) { create(:user).tap { |u| project.add_developer(u) } } - - let_it_be(:issue1) { create(:issue, project: project) } - let_it_be(:issue2) { create(:issue, project: project) } - let_it_be(:merge_request) { create(:merge_request, source_project: project) } - - let_it_be(:event1) { create(:event, :created, target: issue1, author: author1) } - let_it_be(:event2) { create(:event, :closed, target: issue2, author: author2) } - let_it_be(:event3) { create(:event, :merged, target: merge_request, author: author1) } - - let(:events) { [event1, event2, event3] } - - def format_row(event) - path = event.project.reload.project_namespace.traversal_ids.join('/') - - action = Event.actions[event.action] - [ - event.id, - "'#{path}/'", - event.author_id, - event.target_id, - "'#{event.target_type}'", - action, - event.created_at.to_f, - event.updated_at.to_f - ].join(',') + describe 'RSpec hooks' do + it 'ensures that tables are empty' do + results = ClickHouse::Client.select('SELECT * FROM events', :main) + expect(results).to be_empty end - describe 'RSpec hooks' do - it 'ensures that tables are empty' do - results = ClickHouse::Client.select('SELECT * FROM events', :main) - expect(results).to be_empty + it 'inserts data from CSV' do + time = Time.current.utc + Tempfile.open(['test', '.csv.gz']) do |f| + csv = "id,path,created_at\n10,1/2/,#{time.to_f}\n20,1/,#{time.to_f}" + File.binwrite(f.path, ActiveSupport::Gzip.compress(csv)) + + ClickHouse::Client.insert_csv('INSERT INTO events (id, path, created_at) FORMAT CSV', File.open(f.path), + :main) end - end - it 'inserts and modifies data' do - insert_query = <<~SQL - INSERT INTO events - (id, path, author_id, target_id, target_type, action, created_at, updated_at) - VALUES - (#{format_row(event1)}), - (#{format_row(event2)}), - (#{format_row(event3)}) - SQL + results = ClickHouse::Client.select('SELECT id, path, created_at FROM events ORDER BY id', :main) - ClickHouse::Client.execute(insert_query, :main) + expect(results).to match([ + { 'id' => 10, 'path' => '1/2/', 'created_at' => be_within(0.1.seconds).of(time) }, + { 'id' => 20, 'path' => '1/', 'created_at' => be_within(0.1.seconds).of(time) } + ]) + end + end - results = ClickHouse::Client.select('SELECT * FROM events ORDER BY id', :main) - expect(results.size).to eq(3) + it 'inserts and modifies data' do + insert_query = <<~SQL + INSERT INTO events + (id, path, author_id, target_id, target_type, action, created_at, updated_at) + VALUES + (#{format_row(event1)}), + (#{format_row(event2)}), + (#{format_row(event3)}) + SQL + + ClickHouse::Client.execute(insert_query, :main) + + results = ClickHouse::Client.select('SELECT * FROM events ORDER BY id', :main) + expect(results.size).to eq(3) + + last = results.last + expect(last).to match(a_hash_including( + 'id' => event3.id, + 'author_id' => event3.author_id, + 'created_at' => be_within(0.05).of(event3.created_at), + 'target_type' => event3.target_type + )) + + delete_query = ClickHouse::Client::Query.new( + raw_query: 'DELETE FROM events WHERE id = {id:UInt64}', + placeholders: { id: event3.id } + ) + + ClickHouse::Client.execute(delete_query, :main) + + select_query = ClickHouse::Client::Query.new( + raw_query: 'SELECT * FROM events WHERE id = {id:UInt64}', + placeholders: { id: event3.id } + ) + + results = ClickHouse::Client.select(select_query, :main) + expect(results).to be_empty + end + end + end - last = results.last - expect(last).to match(a_hash_including( - 'id' => event3.id, - 'author_id' => event3.author_id, - 'created_at' => be_within(0.05).of(event3.created_at), - 'target_type' => event3.target_type - )) + describe 'logging' do + let(:query_string) { "SELECT * FROM events WHERE id IN (4, 5, 6)" } - ClickHouse::Client.execute("DELETE FROM events WHERE id = #{event3.id}", :main) + context 'on dev and test environments' do + it 'logs the un-redacted query' do + expect(ClickHouse::Client.configuration.logger).to receive(:info).with({ + query: query_string, + correlation_id: a_kind_of(String) + }) - results = ClickHouse::Client.select("SELECT * FROM events WHERE id = #{event3.id}", :main) - expect(results).to be_empty - end + ClickHouse::Client.select(query_string, :main) + end + + it 'has a ClickHouse logger' do + expect(ClickHouse::Client.configuration.logger).to be_a(ClickHouse::Logger) end end end diff --git a/spec/lib/gitlab/database/gitlab_schema_spec.rb b/spec/lib/gitlab/database/gitlab_schema_spec.rb index 14ff1a462e3..e402014df90 100644 --- a/spec/lib/gitlab/database/gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/gitlab_schema_spec.rb @@ -148,7 +148,7 @@ RSpec.describe Gitlab::Database::GitlabSchema, feature_category: :database do subject { described_class.table_schemas!(tables) } it 'returns the matched schemas' do - expect(subject).to match_array %i[gitlab_main_cell gitlab_main gitlab_ci].to_set + expect(subject).to match_array %i[gitlab_main_cell gitlab_ci].to_set end context 'when one of the tables does not have a matching table schema' do diff --git a/spec/lib/gitlab/database/load_balancing/host_spec.rb b/spec/lib/gitlab/database/load_balancing/host_spec.rb index 5ef6d9173c4..89cecaff075 100644 --- a/spec/lib/gitlab/database/load_balancing/host_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/host_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing::Host do +RSpec.describe Gitlab::Database::LoadBalancing::Host, feature_category: :database do let(:load_balancer) do Gitlab::Database::LoadBalancing::LoadBalancer .new(Gitlab::Database::LoadBalancing::Configuration.new(ActiveRecord::Base)) @@ -124,13 +124,36 @@ RSpec.describe Gitlab::Database::LoadBalancing::Host do end it 'refreshes the status' do - expect(Gitlab::Database::LoadBalancing::Logger).to receive(:info) - .with(hash_including(event: :host_online)) - .and_call_original - expect(host).to be_online end + context 'and the host was previously online' do + # Hosts are online by default + + it 'does not log the online event' do + expect(Gitlab::Database::LoadBalancing::Logger) + .not_to receive(:info) + .with(hash_including(event: :host_online)) + + expect(host).to be_online + end + end + + context 'and the host was previously offline' do + before do + host.offline! + end + + it 'logs the online event' do + expect(Gitlab::Database::LoadBalancing::Logger) + .to receive(:info) + .with(hash_including(event: :host_online)) + .and_call_original + + expect(host).to be_online + end + end + context 'and replica is not up to date' do before do expect(host).to receive(:replica_is_up_to_date?).and_return(false) 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 26c8969efd8..c975f5b5ee4 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -469,25 +469,58 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store, fe context 'when none of the replicas are caught up' do before do - expect(hosts).to all(receive(:caught_up?).with(location).and_return(false)) + expect(hosts[0]).to receive(:caught_up?).with(location).and_return(false) + expect(hosts[1]).to receive(:caught_up?).with(location).and_return(false) end - it 'returns false and does not update the host thread-local variable' do - expect(subject).to be false + it 'returns NONE_CAUGHT_UP and does not update the host thread-local variable' do + expect(subject).to eq(described_class::NONE_CAUGHT_UP) expect(set_host).to be_nil end + + it 'notifies caught_up_replica_pick.load_balancing with result false' do + expect(ActiveSupport::Notifications).to receive(:instrument) + .with('caught_up_replica_pick.load_balancing', { result: false }) + + subject + end end - context 'when any of the replicas is caught up' do + context 'when any replica is caught up' do before do - # `allow` for non-caught up host, because we may not even check it, if will find the caught up one earlier - allow(hosts[0]).to receive(:caught_up?).with(location).and_return(false) + expect(hosts[0]).to receive(:caught_up?).with(location).and_return(true) + expect(hosts[1]).to receive(:caught_up?).with(location).and_return(false) + end + + it 'returns ANY_CAUGHT_UP and sets host thread-local variable' do + expect(subject).to eq(described_class::ANY_CAUGHT_UP) + expect(set_host).to eq(hosts[0]) + end + + it 'notifies caught_up_replica_pick.load_balancing with result true' do + expect(ActiveSupport::Notifications).to receive(:instrument) + .with('caught_up_replica_pick.load_balancing', { result: true }) + + subject + end + end + + context 'when all of the replicas is caught up' do + before do + expect(hosts[0]).to receive(:caught_up?).with(location).and_return(true) expect(hosts[1]).to receive(:caught_up?).with(location).and_return(true) end - it 'returns true and sets host thread-local variable' do - expect(subject).to be true - expect(set_host).to eq(hosts[1]) + it 'returns ALL_CAUGHT_UP and sets host thread-local variable' do + expect(subject).to eq(described_class::ALL_CAUGHT_UP) + expect(set_host).to be_in([hosts[0], hosts[1]]) + end + + it 'notifies caught_up_replica_pick.load_balancing with result true' do + expect(ActiveSupport::Notifications).to receive(:instrument) + .with('caught_up_replica_pick.load_balancing', { result: true }) + + subject end end end diff --git a/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb b/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb index 713bff5feea..863b1fb099b 100644 --- a/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do let(:app) { double(:app) } let(:middleware) { described_class.new(app) } let(:warden_user) { double(:warden, user: double(:user, id: 42)) } - let(:single_sticking_object) { Set.new([[ActiveRecord::Base.sticking, :user, 42]]) } + let(:single_sticking_object) { Set.new([[ActiveRecord::Base.sticking, :user, 99]]) } let(:multiple_sticking_objects) do Set.new([ [ActiveRecord::Base.sticking, :user, 42], @@ -25,7 +25,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do expect(middleware).to receive(:clear).twice - expect(middleware).to receive(:unstick_or_continue_sticking).with(env) + expect(middleware).to receive(:find_caught_up_replica).with(env) expect(middleware).to receive(:stick_if_necessary).with(env) expect(app).to receive(:call).with(env).and_return(10) @@ -41,12 +41,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do end end - describe '#unstick_or_continue_sticking' do + describe '#find_caught_up_replica' do it 'does not stick if no namespace and identifier could be found' do expect(ApplicationRecord.sticking) - .not_to receive(:unstick_or_continue_sticking) + .not_to receive(:find_caught_up_replica) - middleware.unstick_or_continue_sticking({}) + middleware.find_caught_up_replica({}) end it 'sticks to the primary if a warden user is found' do @@ -54,94 +54,125 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do Gitlab::Database::LoadBalancing.base_models.each do |model| expect(model.sticking) - .to receive(:unstick_or_continue_sticking) + .to receive(:find_caught_up_replica) .with(:user, 42) end - middleware.unstick_or_continue_sticking(env) + middleware.find_caught_up_replica(env) end it 'sticks to the primary if a sticking namespace and identifier is found' do env = { described_class::STICK_OBJECT => single_sticking_object } expect(ApplicationRecord.sticking) - .to receive(:unstick_or_continue_sticking) - .with(:user, 42) + .to receive(:find_caught_up_replica) + .with(:user, 99) - middleware.unstick_or_continue_sticking(env) + middleware.find_caught_up_replica(env) end it 'sticks to the primary if multiple sticking namespaces and identifiers were found' do env = { described_class::STICK_OBJECT => multiple_sticking_objects } expect(ApplicationRecord.sticking) - .to receive(:unstick_or_continue_sticking) + .to receive(:find_caught_up_replica) .with(:user, 42) .ordered expect(ApplicationRecord.sticking) - .to receive(:unstick_or_continue_sticking) + .to receive(:find_caught_up_replica) .with(:runner, '123456789') .ordered expect(ApplicationRecord.sticking) - .to receive(:unstick_or_continue_sticking) + .to receive(:find_caught_up_replica) .with(:runner, '1234') .ordered - middleware.unstick_or_continue_sticking(env) + middleware.find_caught_up_replica(env) end end describe '#stick_if_necessary' do - it 'does not stick to the primary if not necessary' do - expect(ApplicationRecord.sticking) - .not_to receive(:stick_if_necessary) - - middleware.stick_if_necessary({}) + let(:env) { { 'warden' => warden, described_class::STICK_OBJECT => stick_object }.compact } + let(:stick_object) { nil } + let(:write_performed) { true } + let(:warden) { warden_user } + + before do + allow(::Gitlab::Database::LoadBalancing::Session.current).to receive(:performed_write?) + .and_return(write_performed) end - it 'sticks to the primary if a warden user is found' do - env = { 'warden' => warden_user } + subject { middleware.stick_if_necessary(env) } + it 'sticks to the primary for the user' do Gitlab::Database::LoadBalancing.base_models.each do |model| expect(model.sticking) - .to receive(:stick_if_necessary) + .to receive(:stick) .with(:user, 42) end - middleware.stick_if_necessary(env) + subject end - it 'sticks to the primary if a a single sticking object is found' do - env = { described_class::STICK_OBJECT => single_sticking_object } + context 'when no write was performed' do + let(:write_performed) { false } - expect(ApplicationRecord.sticking) - .to receive(:stick_if_necessary) - .with(:user, 42) + it 'does not stick to the primary' do + expect(ApplicationRecord.sticking) + .not_to receive(:stick) - middleware.stick_if_necessary(env) + subject + end end - it 'sticks to the primary if multiple sticking namespaces and identifiers were found' do - env = { described_class::STICK_OBJECT => multiple_sticking_objects } + context 'when there is no user in the env' do + let(:warden) { nil } - expect(ApplicationRecord.sticking) - .to receive(:stick_if_necessary) - .with(:user, 42) - .ordered + context 'when there is an explicit single sticking object in the env' do + let(:stick_object) { single_sticking_object } - expect(ApplicationRecord.sticking) - .to receive(:stick_if_necessary) - .with(:runner, '123456789') - .ordered + it 'sticks to the single sticking object' do + expect(ApplicationRecord.sticking) + .to receive(:stick) + .with(:user, 99) - expect(ApplicationRecord.sticking) - .to receive(:stick_if_necessary) - .with(:runner, '1234') - .ordered + subject + end + end + + context 'when there is multiple explicit sticking objects' do + let(:stick_object) { multiple_sticking_objects } + + it 'sticks to the sticking objects' do + expect(ApplicationRecord.sticking) + .to receive(:stick) + .with(:user, 42) + .ordered - middleware.stick_if_necessary(env) + expect(ApplicationRecord.sticking) + .to receive(:stick) + .with(:runner, '123456789') + .ordered + + expect(ApplicationRecord.sticking) + .to receive(:stick) + .with(:runner, '1234') + .ordered + + subject + end + end + + context 'when there no explicit sticking objects' do + it 'does not stick to the primary' do + expect(ApplicationRecord.sticking) + .not_to receive(:stick) + + subject + end + 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 789919d2a51..7197b99fe33 100644 --- a/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/service_discovery_spec.rb @@ -15,7 +15,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego load_balancer, nameserver: 'localhost', port: 8600, - record: 'foo' + record: 'foo', + disconnect_timeout: 1 # Short disconnect timeout to keep tests fast ) end @@ -192,6 +193,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego end describe '#replace_hosts' do + before do + stub_env('LOAD_BALANCER_PARALLEL_DISCONNECT', 'true') + allow(service) + .to receive(:load_balancer) + .and_return(load_balancer) + end + let(:address_foo) { described_class::Address.new('foo') } let(:address_bar) { described_class::Address.new('bar') } @@ -202,19 +210,13 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego ) end - before do - allow(service) - .to receive(:load_balancer) - .and_return(load_balancer) - end - it 'replaces the hosts of the load balancer' do service.replace_hosts([address_bar]) expect(load_balancer.host_list.host_names_and_ports).to eq([['bar', nil]]) end - it 'disconnects the old connections' do + it 'disconnects the old connections gracefully if possible' do host = load_balancer.host_list.hosts.first allow(service) @@ -222,11 +224,59 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego .and_return(2) expect(host) - .to receive(:disconnect!) - .with(timeout: 2) + .to receive(:try_disconnect).and_return(true) + + expect(host).not_to receive(:force_disconnect!) service.replace_hosts([address_bar]) end + + it 'disconnects the old connections forcefully if necessary' do + host = load_balancer.host_list.hosts.first + + allow(service) + .to receive(:disconnect_timeout) + .and_return(2) + + expect(host) + .to receive(:try_disconnect).and_return(false) + + expect(host).to receive(:force_disconnect!) + + service.replace_hosts([address_bar]) + end + + context 'without old hosts' do + before do + allow(load_balancer.host_list).to receive(:hosts).and_return([]) + end + + it 'does not log any load balancing event' do + expect(::Gitlab::Database::LoadBalancing::Logger).not_to receive(:info) + + service.replace_hosts([address_foo, address_bar]) + end + end + + context 'when LOAD_BALANCER_PARALLEL_DISCONNECT is false' do + before do + stub_env('LOAD_BALANCER_PARALLEL_DISCONNECT', 'false') + end + + it 'disconnects them sequentially' do + host = load_balancer.host_list.hosts.first + + allow(service) + .to receive(:disconnect_timeout) + .and_return(2) + + expect(host) + .to receive(:disconnect!) + .with(timeout: 2) + + service.replace_hosts([address_bar]) + end + end end describe '#addresses_from_dns' do @@ -475,4 +525,61 @@ RSpec.describe Gitlab::Database::LoadBalancing::ServiceDiscovery, feature_catego end end end + + context 'with service discovery connected to a real load balancer' do + let(:database_address) do + host, port = ApplicationRecord.connection_pool.db_config.configuration_hash.fetch(:host, :port) + described_class::Address.new(host, port) + end + + before do + # set up the load balancer to point to the test postgres instance with three seperate conections + allow(service).to receive(:addresses_from_dns) + .and_return([Gitlab::Database::LoadBalancing::Resolver::FAR_FUTURE_TTL, + [database_address, database_address, database_address]]) + .once + + service.perform_service_discovery + end + + it 'configures service discovery with three replicas' do + expect(service.load_balancer.host_list.hosts.count).to eq(3) + end + + it 'swaps the hosts out gracefully when not contended' do + expect(service.load_balancer.host_list.hosts.count).to eq(3) + + host = service.load_balancer.host_list.next + + # Check out and use a connection from a host so that there is something to clean up + host.pool.with_connection do |connection| + expect { connection.execute('select 1') }.not_to raise_error + end + + allow(service).to receive(:addresses_from_dns).and_return([Gitlab::Database::LoadBalancing::Resolver::FAR_FUTURE_TTL, []]) + + service.load_balancer.host_list.hosts.each do |h| + # Expect that the host gets gracefully disconnected + expect(h).not_to receive(:force_disconnect!) + end + + expect { service.perform_service_discovery }.to change { host.pool.stat[:connections] }.from(1).to(0) + end + + it 'swaps the hosts out forcefully when contended' do + host = service.load_balancer.host_list.next + + # Check out a connection and leave it checked out (simulate a web request) + connection = host.pool.checkout + connection.execute('select 1') + + # Expect that the connection is forcefully checked in + expect(host).to receive(:force_disconnect!).and_call_original + expect(connection).to receive(:steal!).and_call_original + + allow(service).to receive(:addresses_from_dns).and_return([Gitlab::Database::LoadBalancing::Resolver::FAR_FUTURE_TTL, []]) + + service.perform_service_discovery + end + end end 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 7703b5680c2..aaca544ef80 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 @@ -8,6 +8,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ let(:location) { '0/D525E3A8' } let(:wal_locations) { { Gitlab::Database::MAIN_DATABASE_NAME.to_s => location } } let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'wal_locations' => wal_locations } } + let(:any_caught_up) { Gitlab::Database::LoadBalancing::LoadBalancer::ANY_CAUGHT_UP } + let(:all_caught_up) { Gitlab::Database::LoadBalancing::LoadBalancer::ALL_CAUGHT_UP } + let(:none_caught_up) { Gitlab::Database::LoadBalancing::LoadBalancer::NONE_CAUGHT_UP } before do skip_feature_flags_yaml_validation @@ -67,7 +70,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ expect(ActiveRecord::Base.load_balancer) .to receive(:select_up_to_date_host) .with(location) - .and_return(true) + .and_return(any_caught_up) run_middleware do expect(Gitlab::Database::LoadBalancing::Session.current.use_primary?).not_to be_truthy @@ -86,7 +89,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ allow(lb) .to receive(:select_up_to_date_host) .with(location) - .and_return(true) + .and_return(any_caught_up) end end @@ -100,7 +103,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ allow(ActiveRecord::Base.load_balancer) .to receive(:select_up_to_date_host) .with(wal_locations[:main]) - .and_return(true) + .and_return(any_caught_up) end it_behaves_like 'replica is up to date', 'replica' @@ -133,7 +136,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ context 'when replica is up to date' do before do Gitlab::Database::LoadBalancing.each_load_balancer do |lb| - allow(lb).to receive(:select_up_to_date_host).and_return(true) + allow(lb).to receive(:select_up_to_date_host).and_return(any_caught_up) end end @@ -147,7 +150,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ context 'when replica is not up to date' do before do Gitlab::Database::LoadBalancing.each_load_balancer do |lb| - allow(lb).to receive(:select_up_to_date_host).and_return(false, true) + allow(lb).to receive(:select_up_to_date_host).and_return(none_caught_up, any_caught_up) end end @@ -161,7 +164,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ context 'when replica is never not up to date' do before do Gitlab::Database::LoadBalancing.each_load_balancer do |lb| - allow(lb).to receive(:select_up_to_date_host).and_return(false, false) + allow(lb).to receive(:select_up_to_date_host).and_return(none_caught_up, none_caught_up) end end @@ -267,7 +270,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ context 'when replica is not up to date' do before do Gitlab::Database::LoadBalancing.each_load_balancer do |lb| - allow(lb).to receive(:select_up_to_date_host).and_return(false) + allow(lb).to receive(:select_up_to_date_host).and_return(none_caught_up) end end @@ -282,7 +285,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ .to eq(true) end - it 'returns true when all load balancers are in sync' do + it 'returns true when all load balancers are in sync for some replicas' do locations = {} Gitlab::Database::LoadBalancing.each_load_balancer do |lb| @@ -291,7 +294,23 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ expect(lb) .to receive(:select_up_to_date_host) .with('foo') - .and_return(true) + .and_return(any_caught_up) + end + + expect(middleware.send(:databases_in_sync?, locations)) + .to eq(true) + end + + it 'returns true when all load balancers are in sync for all replicas' do + locations = {} + + Gitlab::Database::LoadBalancing.each_load_balancer do |lb| + locations[lb.name] = 'foo' + + expect(lb) + .to receive(:select_up_to_date_host) + .with('foo') + .and_return(all_caught_up) end expect(middleware.send(:databases_in_sync?, locations)) @@ -307,7 +326,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ allow(lb) .to receive(:select_up_to_date_host) .with('foo') - .and_return(false) + .and_return(none_caught_up) end expect(middleware.send(:databases_in_sync?, locations)) @@ -324,7 +343,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ allow(lb) .to receive(:select_up_to_date_host) .with('foo') - .and_return(false) + .and_return(none_caught_up) end expect(middleware.send(:databases_in_sync?, locations)) @@ -346,8 +365,9 @@ RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_ end def replication_lag!(exists) + caught_up = exists ? none_caught_up : all_caught_up Gitlab::Database::LoadBalancing.each_load_balancer do |lb| - allow(lb).to receive(:select_up_to_date_host).and_return(!exists) + allow(lb).to receive(:select_up_to_date_host).and_return(caught_up) end end end diff --git a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb index ff31a5cd6cb..8c2901c3b89 100644 --- a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb @@ -3,327 +3,142 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do - let(:sticking) do - described_class.new(ActiveRecord::Base.load_balancer) - end + let(:load_balancer) { ActiveRecord::Base.load_balancer } + let(:primary_write_location) { 'the-primary-lsn' } + let(:last_write_location) { 'the-last-write-lsn' } - after do - Gitlab::Database::LoadBalancing::Session.clear_session + let(:sticking) do + described_class.new(load_balancer) end - shared_examples 'sticking' do - before do - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_write_location) - .and_return('foo') - end - - it 'sticks an entity to the primary', :aggregate_failures do - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_only?) - .and_return(false) - - ids.each do |id| - expect(sticking) - .to receive(:set_write_location_for) - .with(:user, id, 'foo') - end + let(:redis) { instance_double(::Gitlab::Redis::MultiStore) } - expect(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:use_primary!) - - subject - end + before do + allow(::Gitlab::Redis::DbLoadBalancing).to receive(:with).and_yield(redis) - it 'does not update the write location when no replicas are used' do - expect(sticking).not_to receive(:set_write_location_for) + allow(ActiveRecord::Base.load_balancer) + .to receive(:primary_write_location) + .and_return(primary_write_location) - subject - end + allow(redis).to receive(:get) + .with("database-load-balancing/write-location/#{load_balancer.name}/user/42") + .and_return(last_write_location) end - shared_examples 'tracking status in redis' do - describe '#stick_or_unstick_request' do - it 'sticks or unsticks a single object and updates the Rack environment' do - expect(sticking) - .to receive(:unstick_or_continue_sticking) - .with(:user, 42) - - env = {} - - sticking.stick_or_unstick_request(env, :user, 42) - - expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a) - .to eq([[sticking, :user, 42]]) - end - - it 'sticks or unsticks multiple objects and updates the Rack environment' do - expect(sticking) - .to receive(:unstick_or_continue_sticking) - .with(:user, 42) - .ordered - - expect(sticking) - .to receive(:unstick_or_continue_sticking) - .with(:runner, '123456789') - .ordered - - env = {} - - sticking.stick_or_unstick_request(env, :user, 42) - sticking.stick_or_unstick_request(env, :runner, '123456789') - - expect(env[Gitlab::Database::LoadBalancing::RackMiddleware::STICK_OBJECT].to_a).to eq( - [ - [sticking, :user, 42], - [sticking, :runner, - '123456789'] - ]) - end - end - - describe '#stick_if_necessary' do - it 'does not stick if no write was performed' do - allow(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:performed_write?) - .and_return(false) - - expect(sticking).not_to receive(:stick) - - sticking.stick_if_necessary(:user, 42) - end - - it 'sticks to the primary if a write was performed' do - allow(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:performed_write?) - .and_return(true) - - expect(sticking) - .to receive(:stick) - .with(:user, 42) + after do + Gitlab::Database::LoadBalancing::Session.clear_session + end - sticking.stick_if_necessary(:user, 42) - end + describe '#find_caught_up_replica' do + before do + allow(ActiveSupport::Notifications).to receive(:instrument).and_call_original end - describe '#all_caught_up?' do - let(:lb) { ActiveRecord::Base.load_balancer } - let(:last_write_location) { 'foo' } - - before do - allow(ActiveSupport::Notifications).to receive(:instrument).and_call_original - - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return(last_write_location) - end - - context 'when no write location could be found' do - let(:last_write_location) { nil } - - it 'returns true' do - expect(lb).not_to receive(:select_up_to_date_host) - - expect(sticking.all_caught_up?(:user, 42)).to eq(true) - end - end - - context 'when all secondaries have caught up' do - before do - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) - end - - it 'returns true, and unsticks' do - expect(sticking) - .to receive(:unstick) - .with(:user, 42) - - expect(sticking.all_caught_up?(:user, 42)).to eq(true) - end + context 'when no write location could be found' do + let(:last_write_location) { nil } - it 'notifies with the proper event payload' do - expect(ActiveSupport::Notifications) - .to receive(:instrument) - .with('caught_up_replica_pick.load_balancing', { result: true }) - .and_call_original + it 'returns true' do + expect(load_balancer).not_to receive(:select_up_to_date_host) - sticking.all_caught_up?(:user, 42) - end + expect(sticking.find_caught_up_replica(:user, 42)).to eq(true) end - context 'when the secondaries have not yet caught up' do - before do - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) - end - - it 'returns false' do - expect(sticking.all_caught_up?(:user, 42)).to eq(false) - end + context 'when use_primary_on_empty_location is true' do + it 'returns false, does not unstick and calls use_primary!' do + expect(load_balancer).not_to receive(:select_up_to_date_host) - it 'notifies with the proper event payload' do - expect(ActiveSupport::Notifications) - .to receive(:instrument) - .with('caught_up_replica_pick.load_balancing', { result: false }) - .and_call_original + expect(redis).not_to receive(:del) + expect(::Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary!) - sticking.all_caught_up?(:user, 42) + expect(sticking.find_caught_up_replica(:user, 42, use_primary_on_empty_location: true)).to eq(false) end end end - describe '#unstick_or_continue_sticking' do - let(:lb) { ActiveRecord::Base.load_balancer } - - it 'simply returns if no write location could be found' do - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return(nil) - - expect(lb).not_to receive(:select_up_to_date_host) - - sticking.unstick_or_continue_sticking(:user, 42) - end - - it 'unsticks if all secondaries have caught up' do - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return('foo') + context 'when all replicas have caught up' do + it 'returns true and unsticks' do + expect(load_balancer).to receive(:select_up_to_date_host).with(last_write_location) + .and_return(::Gitlab::Database::LoadBalancing::LoadBalancer::ALL_CAUGHT_UP) - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(true) + expect(redis) + .to receive(:del) + .with("database-load-balancing/write-location/#{load_balancer.name}/user/42") - expect(sticking) - .to receive(:unstick) - .with(:user, 42) - - sticking.unstick_or_continue_sticking(:user, 42) + expect(sticking.find_caught_up_replica(:user, 42)).to eq(true) end + end - it 'continues using the primary if the secondaries have not yet caught up' do - allow(sticking) - .to receive(:last_write_location_for) - .with(:user, 42) - .and_return('foo') - - allow(lb).to receive(:select_up_to_date_host).with('foo').and_return(false) - - expect(Gitlab::Database::LoadBalancing::Session.current) - .to receive(:use_primary!) + context 'when only some of the replicas have caught up' do + it 'returns true and does not unstick' do + expect(load_balancer).to receive(:select_up_to_date_host).with(last_write_location) + .and_return(::Gitlab::Database::LoadBalancing::LoadBalancer::ANY_CAUGHT_UP) - sticking.unstick_or_continue_sticking(:user, 42) - end - end + expect(redis).not_to receive(:del) - describe '#stick' do - it_behaves_like 'sticking' do - let(:ids) { [42] } - subject { sticking.stick(:user, ids.first) } + expect(sticking.find_caught_up_replica(:user, 42)).to eq(true) end end - describe '#bulk_stick' do - it_behaves_like 'sticking' do - let(:ids) { [42, 43] } - subject { sticking.bulk_stick(:user, ids) } + context 'when none of the replicas have caught up' do + before do + allow(load_balancer).to receive(:select_up_to_date_host).with(last_write_location) + .and_return(::Gitlab::Database::LoadBalancing::LoadBalancer::NONE_CAUGHT_UP) end - end - - describe '#mark_primary_write_location' do - it 'updates the write location with the load balancer' do - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_write_location) - .and_return('foo') - allow(ActiveRecord::Base.load_balancer) - .to receive(:primary_only?) - .and_return(false) + it 'returns false, does not unstick and calls use_primary!' do + expect(redis).not_to receive(:del) + expect(::Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary!) - expect(sticking) - .to receive(:set_write_location_for) - .with(:user, 42, 'foo') - - sticking.mark_primary_write_location(:user, 42) + expect(sticking.find_caught_up_replica(:user, 42)).to eq(false) end - it 'does nothing when no replicas are used' do - expect(sticking).not_to receive(:set_write_location_for) + context 'when use_primary_on_failure is false' do + it 'does not call use_primary!' do + expect(redis).not_to receive(:del) + expect(::Gitlab::Database::LoadBalancing::Session.current).not_to receive(:use_primary!) - sticking.mark_primary_write_location(:user, 42) + expect(sticking.find_caught_up_replica(:user, 42, use_primary_on_failure: false)).to eq(false) + end end end + end - describe '#unstick' do - it 'removes the sticking data from Redis' do - sticking.set_write_location_for(:user, 4, 'foo') - sticking.unstick(:user, 4) + shared_examples 'sticking' do + it 'sticks an entity to the primary', :aggregate_failures do + allow(ActiveRecord::Base.load_balancer) + .to receive(:primary_only?) + .and_return(false) - expect(sticking.last_write_location_for(:user, 4)).to be_nil + ids.each do |id| + expect(redis) + .to receive(:set) + .with("database-load-balancing/write-location/#{load_balancer.name}/user/#{id}", 'the-primary-lsn', ex: 30) end - end - describe '#last_write_location_for' do - it 'returns the last WAL write location for a user' do - sticking.set_write_location_for(:user, 4, 'foo') + expect(Gitlab::Database::LoadBalancing::Session.current) + .to receive(:use_primary!) - expect(sticking.last_write_location_for(:user, 4)).to eq('foo') - end + subject end - describe '#select_caught_up_replicas' do - let(:lb) { ActiveRecord::Base.load_balancer } - - context 'with no write location' do - before do - allow(sticking) - .to receive(:last_write_location_for) - .with(:project, 42) - .and_return(nil) - end - - it 'returns false and does not try to find caught up hosts' do - expect(lb).not_to receive(:select_up_to_date_host) - expect(sticking.select_caught_up_replicas(:project, 42)).to be false - end - end - - context 'with write location' do - before do - allow(sticking) - .to receive(:last_write_location_for) - .with(:project, 42) - .and_return('foo') - end + it 'does not update the write location when no replicas are used' do + expect(sticking).not_to receive(:set_write_location_for) - it 'returns true, selects hosts, and unsticks if any secondary has caught up' do - expect(lb).to receive(:select_up_to_date_host).and_return(true) - expect(sticking) - .to receive(:unstick) - .with(:project, 42) - expect(sticking.select_caught_up_replicas(:project, 42)).to be true - end - end + subject end end - context 'with multi-store feature flags turned on' do - it_behaves_like 'tracking status in redis' - end - - context 'when both multi-store feature flags are off' do - before do - stub_feature_flags(use_primary_and_secondary_stores_for_db_load_balancing: false) - stub_feature_flags(use_primary_store_as_default_for_db_load_balancing: false) + describe '#stick' do + it_behaves_like 'sticking' do + let(:ids) { [42] } + subject { sticking.stick(:user, ids.first) } end - - it_behaves_like 'tracking status in redis' end - describe '#redis_key_for' do - it 'returns a String' do - expect(sticking.redis_key_for(:user, 42)) - .to eq('database-load-balancing/write-location/main/user/42') + describe '#bulk_stick' do + it_behaves_like 'sticking' do + let(:ids) { [42, 43] } + subject { sticking.bulk_stick(:user, ids) } end end end diff --git a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb index 0b25389c667..a12e0909dc2 100644 --- a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb +++ b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb @@ -107,6 +107,8 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do before do observe + rescue Exception # rubocop:disable Lint/RescueException + # ignore (we expect this exception) end it 'records a valid observation', :aggregate_failures do diff --git a/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb b/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb index 6cac7abb703..2fa4c9e562f 100644 --- a/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb +++ b/spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb @@ -14,14 +14,17 @@ RSpec.describe 'cross-database foreign keys' do 'gitlab_subscriptions.hosted_plan_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422012 'group_import_states.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/421210 'identities.saml_provider_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422010 - 'project_authorizations.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422044 + 'issues.author_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422154 + 'issues.closed_by_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422154 + 'issues.updated_by_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422154 + 'issue_assignees.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422154 'merge_requests.assignee_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422080 'merge_requests.updated_by_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422080 'merge_requests.merge_user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422080 'merge_requests.author_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422080 + 'project_authorizations.user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/422044 'projects.creator_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/421844 'projects.marked_for_deletion_by_user_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/421844 - 'routes.namespace_id', # https://gitlab.com/gitlab-org/gitlab/-/issues/420869 'user_group_callouts.user_id' # https://gitlab.com/gitlab-org/gitlab/-/issues/421287 ] end diff --git a/spec/lib/gitlab/database/no_overrides_for_through_associations_spec.rb b/spec/lib/gitlab/database/no_overrides_for_through_associations_spec.rb new file mode 100644 index 00000000000..ca7b6c8aa98 --- /dev/null +++ b/spec/lib/gitlab/database/no_overrides_for_through_associations_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'overridden has_many :through associations', :eager_load, feature_category: :database do + let!(:allowed_overrides) do + [ + # https://gitlab.com/gitlab-org/gitlab/-/issues/424851 + override_class.new(:assignees, 'app/models/concerns/deprecated_assignee.rb'), + # https://gitlab.com/gitlab-org/gitlab/-/issues/424852 + override_class.new(:authorized_projects, 'app/models/user.rb'), + # https://gitlab.com/gitlab-org/gitlab/-/issues/424853 + override_class.new(:project, 'app/models/incident_management/issuable_escalation_status.rb'), + # https://gitlab.com/gitlab-org/gitlab/-/issues/424854 + override_class.new(:remediations, 'ee/app/models/vulnerabilities/finding.rb') + ] + end + + let!(:override_class) do + Struct.new(:method_name, :file_path, :association_class) do + def initialize(method_name, file_path, association_class = nil) + super(method_name, file_path, association_class) + end + + def ==(other) + full_source_path, short_path = + file_path.length > other.file_path.length ? [file_path, other.file_path] : [other.file_path, file_path] + method_name == other.method_name && full_source_path.include?(short_path) + end + + def association_type_name + if association_class == ActiveRecord::Associations::HasOneThroughAssociation + 'has_one through:' + else + 'has_many through:' + end + end + end + end + + let!(:documentation_link) do + 'https://docs.gitlab.com/ee/development/gotchas.html#do-not-override-has_many-through-or-has_one-through-associations' + end + + it 'onlies have allowed list of overridden has_many/has_one :through associations', :aggregate_failures do + overridden_associations.each do |overriden_method| + expect(allowed_override?(overriden_method)).to be_truthy, + "Found an overridden #{overriden_method.association_type_name} association " \ + "named `#{overriden_method.method_name}`, in #{overriden_method.file_path}, which isn't allowed. " \ + "Overriding such associations can have dangerous impacts, see: #{documentation_link}" + end + end + + private + + def allowed_override?(overriden_method) + allowed_overrides.find do |override| + override == overriden_method + end + end + + def overridden_associations + ApplicationRecord.descendants.reject(&:abstract_class?).each_with_object([]) do |klass, array| + through_reflections = klass.reflect_on_all_associations.select do |assoc| + assoc.is_a?(ActiveRecord::Reflection::ThroughReflection) + end + + overridden_methods = through_reflections + .map { |association| [association.association_class, association.name] } + .map { |association_class, method_name| [method_name, source_location(klass, method_name), association_class] } + .reject { |_, source_location, _| source_location.include?('activerecord-') } + + array << override_class.new(*overridden_methods.flatten) if overridden_methods.any? + end + end + + def source_location(klass, method_name) + klass.instance_method(method_name).source_location.first + end +end diff --git a/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb index f415e892818..79c2c9e32d2 100644 --- a/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/ci_sliding_list_strategy_spec.rb @@ -175,4 +175,30 @@ RSpec.describe Gitlab::Database::Partitioning::CiSlidingListStrategy, feature_ca end.not_to raise_error end end + + describe 'attributes' do + let(:partitioning_key) { :partition } + let(:next_partition_if) { -> { true } } + let(:detach_partition_if) { -> { false } } + let(:analyze_interval) { 1.week } + + subject(:strategy) do + described_class.new( + model, partitioning_key, + next_partition_if: next_partition_if, + detach_partition_if: detach_partition_if, + analyze_interval: analyze_interval + ) + end + + specify do + expect(strategy).to have_attributes({ + model: model, + partitioning_key: partitioning_key, + next_partition_if: next_partition_if, + detach_partition_if: detach_partition_if, + analyze_interval: analyze_interval + }) + end + end end diff --git a/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb index 50115a6f3dd..3afa338fdf7 100644 --- a/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/monthly_strategy_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do +RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy, feature_category: :database do let(:connection) { ActiveRecord::Base.connection } describe '#current_partitions' do @@ -273,4 +273,32 @@ RSpec.describe Gitlab::Database::Partitioning::MonthlyStrategy do end end end + + describe 'attributes' do + let(:partitioning_key) { :partition } + let(:retain_non_empty_partitions) { true } + let(:retain_for) { 12.months } + let(:analyze_interval) { 1.week } + let(:model) { class_double(ApplicationRecord, table_name: table_name, connection: connection) } + let(:table_name) { :_test_partitioned_test } + + subject(:strategy) do + described_class.new( + model, partitioning_key, + retain_for: retain_for, + retain_non_empty_partitions: retain_non_empty_partitions, + analyze_interval: analyze_interval + ) + end + + specify do + expect(strategy).to have_attributes({ + model: model, + partitioning_key: partitioning_key, + retain_for: retain_for, + retain_non_empty_partitions: retain_non_empty_partitions, + analyze_interval: analyze_interval + }) + end + end end diff --git a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb index eac4a162879..c41228777ca 100644 --- a/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb +++ b/spec/lib/gitlab/database/partitioning/partition_manager_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::Partitioning::PartitionManager do +RSpec.describe Gitlab::Database::Partitioning::PartitionManager, feature_category: :database do + include ActiveSupport::Testing::TimeHelpers include Database::PartitioningHelpers include ExclusiveLeaseHelpers @@ -15,7 +16,7 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do let(:connection) { ActiveRecord::Base.connection } let(:table) { partitioned_table_name } let(:partitioning_strategy) do - double(missing_partitions: partitions, extra_partitions: [], after_adding_partitions: nil) + double(missing_partitions: partitions, extra_partitions: [], after_adding_partitions: nil, analyze_interval: nil) end let(:partitions) do @@ -125,7 +126,7 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do let(:connection) { ActiveRecord::Base.connection } let(:table) { :_test_foo } let(:partitioning_strategy) do - double(extra_partitions: extra_partitions, missing_partitions: [], after_adding_partitions: nil) + double(extra_partitions: extra_partitions, missing_partitions: [], after_adding_partitions: nil, analyze_interval: nil) end before do @@ -256,6 +257,154 @@ RSpec.describe Gitlab::Database::Partitioning::PartitionManager do end end + describe 'analyze partitioned table' do + let(:analyze) { true } + let(:analyze_table) { partitioned_table_name } + let(:analyze_partition) { "#{partitioned_table_name}_1" } + let(:analyze_regex) { /ANALYZE VERBOSE "#{analyze_table}"/ } + let(:analyze_interval) { 1.week } + let(:connection) { my_model.connection } + let(:create_partition) { true } + let(:my_model) do + interval = analyze_interval + Class.new(ApplicationRecord) do + include PartitionedTable + + partitioned_by :partition_id, + strategy: :ci_sliding_list, + next_partition_if: proc { false }, + detach_partition_if: proc { false }, + analyze_interval: interval + end + end + + shared_examples_for 'run only once analyze within interval' do + specify do + control = ActiveRecord::QueryRecorder.new { described_class.new(my_model, connection: connection).sync_partitions(analyze: analyze) } + expect(control.occurrences).to include(analyze_regex) + + control = ActiveRecord::QueryRecorder.new { described_class.new(my_model, connection: connection).sync_partitions(analyze: analyze) } + expect(control.occurrences).not_to include(analyze_regex) + + travel_to((analyze_interval * 2).since) do + control = ActiveRecord::QueryRecorder.new { described_class.new(my_model, connection: connection).sync_partitions(analyze: analyze) } + expect(control.occurrences).to include(analyze_regex) + end + end + end + + shared_examples_for 'not to run the analyze at all' do + specify do + control = ActiveRecord::QueryRecorder.new { described_class.new(my_model, connection: connection).sync_partitions(analyze: analyze) } + expect(control.occurrences).not_to include(analyze_regex) + + control = ActiveRecord::QueryRecorder.new { described_class.new(my_model, connection: connection).sync_partitions(analyze: analyze) } + expect(control.occurrences).not_to include(analyze_regex) + + travel_to((analyze_interval * 2).since) do + control = ActiveRecord::QueryRecorder.new { described_class.new(my_model, connection: connection).sync_partitions(analyze: analyze) } + expect(control.occurrences).not_to include(analyze_regex) + end + end + end + + before do + my_model.table_name = partitioned_table_name + + connection.execute(<<~SQL) + CREATE TABLE #{analyze_table}(id serial) PARTITION BY LIST (id); + SQL + + connection.execute(<<~SQL) if create_partition + CREATE TABLE IF NOT EXISTS #{analyze_partition} PARTITION OF #{analyze_table} FOR VALUES IN (1); + SQL + + allow(connection).to receive(:select_value).and_return(nil, Time.current, Time.current) + end + + context 'when feature flag database_analyze_on_partitioned_tables is enabled' do + before do + stub_feature_flags(database_analyze_on_partitioned_tables: true) + end + + it_behaves_like 'run only once analyze within interval' + + context 'when analyze is false' do + let(:analyze) { false } + + it_behaves_like 'not to run the analyze at all' + end + + context 'when model does not set analyze_interval' do + let(:my_model) do + Class.new(ApplicationRecord) do + include PartitionedTable + + partitioned_by :partition_id, + strategy: :ci_sliding_list, + next_partition_if: proc { false }, + detach_partition_if: proc { false } + end + end + + it_behaves_like 'not to run the analyze at all' + end + + context 'when no partition is created' do + let(:create_partition) { false } + + it_behaves_like 'run only once analyze within interval' + end + end + + context 'when feature flag database_analyze_on_partitioned_tables is disabled' do + before do + stub_feature_flags(database_analyze_on_partitioned_tables: false) + end + + it_behaves_like 'not to run the analyze at all' + + context 'when analyze is false' do + let(:analyze) { false } + + it_behaves_like 'not to run the analyze at all' + end + + context 'when model does not set analyze_interval' do + let(:my_model) do + Class.new(ApplicationRecord) do + include PartitionedTable + + partitioned_by :partition_id, + strategy: :ci_sliding_list, + next_partition_if: proc { false }, + detach_partition_if: proc { false } + end + end + + it_behaves_like 'not to run the analyze at all' + end + + context 'when no partition is created' do + let(:create_partition) { false } + + it_behaves_like 'not to run the analyze at all' + end + end + end + + describe 'strategies that support analyze_interval' do + [ + ::Gitlab::Database::Partitioning::MonthlyStrategy, + ::Gitlab::Database::Partitioning::SlidingListStrategy, + ::Gitlab::Database::Partitioning::CiSlidingListStrategy + ].each do |klass| + specify "#{klass} supports analyze_interval" do + expect(klass).to be_method_defined(:analyze_interval) + end + end + end + context 'creating and then detaching partitions for a table' do let(:connection) { ActiveRecord::Base.connection } let(:my_model) do diff --git a/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb b/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb index 5b6967c2d14..ac4d345271e 100644 --- a/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb @@ -290,4 +290,30 @@ RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy, feature_cate expect(partition_3_model.partition).to eq(3) end end + + describe 'attributes' do + let(:partitioning_key) { :partition } + let(:next_partition_if) { -> { puts "next_partition_if" } } + let(:detach_partition_if) { -> { puts "detach_partition_if" } } + let(:analyze_interval) { 1.week } + + subject(:strategy) do + described_class.new( + model, partitioning_key, + next_partition_if: next_partition_if, + detach_partition_if: detach_partition_if, + analyze_interval: analyze_interval + ) + end + + specify do + expect(strategy).to have_attributes({ + model: model, + partitioning_key: partitioning_key, + next_partition_if: next_partition_if, + detach_partition_if: detach_partition_if, + analyze_interval: analyze_interval + }) + end + end end diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb index a1ae75ac916..e53e0cb8def 100644 --- a/spec/lib/gitlab/database/partitioning_spec.rb +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -8,6 +8,10 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do let(:main_connection) { ApplicationRecord.connection } + before do + stub_feature_flags(disallow_database_ddl_feature_flags: false) + end + around do |example| previously_registered_models = described_class.registered_models.dup described_class.instance_variable_set(:@registered_models, Set.new) @@ -32,7 +36,7 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do describe '.sync_partitions_ignore_db_error' do it 'calls sync_partitions' do - expect(described_class).to receive(:sync_partitions) + expect(described_class).to receive(:sync_partitions).with(analyze: false) described_class.sync_partitions_ignore_db_error end @@ -100,6 +104,55 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do .and change { find_partitions(table_names.last).size }.from(0) end + context 'for analyze' do + let(:analyze_regex) { /ANALYZE VERBOSE / } + let(:analyze) { true } + + shared_examples_for 'not running analyze' do + specify do + control = ActiveRecord::QueryRecorder.new { described_class.sync_partitions(analyze: analyze) } + expect(control.occurrences).not_to include(analyze_regex) + end + end + + context 'when analyze_interval is not set' do + it_behaves_like 'not running analyze' + + context 'when analyze is set to false' do + it_behaves_like 'not running analyze' + end + end + + context 'when analyze_interval is set' do + let(:models) do + [ + Class.new(ApplicationRecord) do + include PartitionedTable + + self.table_name = :_test_partitioning_test1 + partitioned_by :created_at, strategy: :monthly, analyze_interval: 1.week + end, + Class.new(Gitlab::Database::Partitioning::TableWithoutModel).tap do |klass| + klass.table_name = :_test_partitioning_test2 + klass.partitioned_by(:created_at, strategy: :monthly, analyze_interval: 1.week) + klass.limit_connection_names = %i[main] + end + ] + end + + it 'runs analyze' do + control = ActiveRecord::QueryRecorder.new { described_class.sync_partitions(models, analyze: analyze) } + expect(control.occurrences).to include(analyze_regex) + end + + context 'analyze is false' do + let(:analyze) { false } + + it_behaves_like 'not running analyze' + end + end + end + context 'with multiple databases' do it 'creates partitions in each database' do skip_if_shared_database(:ci) @@ -165,11 +218,11 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do execute_on_each_database("DROP TABLE IF EXISTS #{table_name}") execute_on_each_database(<<~SQL) - CREATE TABLE #{table_name} ( - id serial not null, - created_at timestamptz not null, - PRIMARY KEY (id, created_at)) - PARTITION BY RANGE (created_at); + CREATE TABLE #{table_name} ( + id serial not null, + created_at timestamptz not null, + PRIMARY KEY (id, created_at)) + PARTITION BY RANGE (created_at); SQL end end @@ -204,6 +257,20 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do described_class.sync_partitions(models) end end + + context 'when disallow_database_ddl_feature_flags feature flag is enabled' do + before do + described_class.register_models(models) + stub_feature_flags(disallow_database_ddl_feature_flags: true) + end + + it 'skips sync_partitions' do + expect(described_class::PartitionManager).not_to receive(:new) + expect(described_class).to receive(:sync_partitions).and_call_original + + described_class.sync_partitions(models) + end + end end describe '.report_metrics' do @@ -277,6 +344,18 @@ RSpec.describe Gitlab::Database::Partitioning, feature_category: :database do end end + context 'when the feature disallow DDL feature flags is enabled' do + before do + stub_feature_flags(disallow_database_ddl_feature_flags: true) + end + + it 'does not call the DetachedPartitionDropper' do + expect(Gitlab::Database::Partitioning::DetachedPartitionDropper).not_to receive(:new) + + described_class.drop_detached_partitions + end + end + def table_exists?(table_name) table_oid(table_name).present? end diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index 851fc7ea3cd..441f6476abe 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -6,6 +6,10 @@ RSpec.describe Gitlab::Database::Reindexing, feature_category: :database, time_t include ExclusiveLeaseHelpers include Database::DatabaseHelpers + before do + stub_feature_flags(disallow_database_ddl_feature_flags: false) + end + describe '.invoke' do let(:databases) { Gitlab::Database.database_base_models_with_gitlab_shared } let(:databases_count) { databases.count } @@ -44,6 +48,14 @@ RSpec.describe Gitlab::Database::Reindexing, feature_category: :database, time_t described_class.invoke end + + it 'does not execute async index creation when disable ddl flag is enabled' do + stub_feature_flags(disallow_database_ddl_feature_flags: true) + + expect(Gitlab::Database::AsyncIndexes).not_to receive(:create_pending_indexes!) + + described_class.invoke + end end it 'executes async index destruction prior to any reindexing actions' do @@ -86,6 +98,14 @@ RSpec.describe Gitlab::Database::Reindexing, feature_category: :database, time_t described_class.invoke end + + it 'does not execute async index creation when disable ddl flag is enabled' do + stub_feature_flags(disallow_database_ddl_feature_flags: true) + + expect(Gitlab::Database::AsyncIndexes).not_to receive(:validate_pending_entries!) + + described_class.invoke + end end end diff --git a/spec/lib/gitlab/database/tables_truncate_spec.rb b/spec/lib/gitlab/database/tables_truncate_spec.rb index 04bec50088d..e41c7d34378 100644 --- a/spec/lib/gitlab/database/tables_truncate_spec.rb +++ b/spec/lib/gitlab/database/tables_truncate_spec.rb @@ -9,6 +9,7 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba let(:min_batch_size) { 1 } let(:main_connection) { ApplicationRecord.connection } let(:ci_connection) { Ci::ApplicationRecord.connection } + let(:logger) { instance_double(Logger) } # Main Database let(:main_db_main_item_model) { table("_test_gitlab_main_items", database: "main") } @@ -32,8 +33,123 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba table("gitlab_partitions_dynamic._test_gitlab_hook_logs_202201", database: "ci") end + before do + skip_if_shared_database(:ci) + + # Creating some test tables on the main database + main_tables_sql = <<~SQL + CREATE TABLE _test_gitlab_main_items (id serial NOT NULL PRIMARY KEY); + + CREATE TABLE _test_gitlab_main_references ( + id serial NOT NULL PRIMARY KEY, + item_id BIGINT NOT NULL, + CONSTRAINT fk_constrained_1 FOREIGN KEY(item_id) REFERENCES _test_gitlab_main_items(id) + ); + + CREATE TABLE _test_gitlab_hook_logs ( + id bigserial not null, + created_at timestamptz not null, + item_id BIGINT NOT NULL, + PRIMARY KEY (id, created_at), + CONSTRAINT fk_constrained_1 FOREIGN KEY(item_id) REFERENCES _test_gitlab_main_items(id) + ) PARTITION BY RANGE(created_at); + + CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_202201 + PARTITION OF _test_gitlab_hook_logs + FOR VALUES FROM ('20220101') TO ('20220131'); + + CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_202202 + PARTITION OF _test_gitlab_hook_logs + FOR VALUES FROM ('20220201') TO ('20220228'); + + ALTER TABLE _test_gitlab_hook_logs DETACH PARTITION gitlab_partitions_dynamic._test_gitlab_hook_logs_202201; + SQL + + execute_on_each_database(main_tables_sql) + + ci_tables_sql = <<~SQL + CREATE TABLE _test_gitlab_ci_items (id serial NOT NULL PRIMARY KEY); + + CREATE TABLE _test_gitlab_ci_references ( + id serial NOT NULL PRIMARY KEY, + item_id BIGINT NOT NULL, + CONSTRAINT fk_constrained_1 FOREIGN KEY(item_id) REFERENCES _test_gitlab_ci_items(id) + ); + SQL + + execute_on_each_database(ci_tables_sql) + + internal_tables_sql = <<~SQL + CREATE TABLE _test_gitlab_shared_items (id serial NOT NULL PRIMARY KEY); + SQL + + execute_on_each_database(internal_tables_sql) + + # Filling the tables + 5.times do |i| + # Main Database + main_db_main_item_model.create!(id: i) + main_db_main_reference_model.create!(item_id: i) + main_db_ci_item_model.create!(id: i) + main_db_ci_reference_model.create!(item_id: i) + main_db_shared_item_model.create!(id: i) + main_db_partitioned_item.create!(item_id: i, created_at: '2022-02-02 02:00') + main_db_partitioned_item_detached.create!(item_id: i, created_at: '2022-01-01 01:00') + # CI Database + ci_db_main_item_model.create!(id: i) + ci_db_main_reference_model.create!(item_id: i) + ci_db_ci_item_model.create!(id: i) + ci_db_ci_reference_model.create!(item_id: i) + ci_db_shared_item_model.create!(id: i) + ci_db_partitioned_item.create!(item_id: i, created_at: '2022-02-02 02:00') + ci_db_partitioned_item_detached.create!(item_id: i, created_at: '2022-01-01 01:00') + end + + Gitlab::Database::SharedModel.using_connection(main_connection) do + Postgresql::DetachedPartition.create!( + table_name: '_test_gitlab_hook_logs_202201', + drop_after: Time.current + ) + end + + Gitlab::Database::SharedModel.using_connection(ci_connection) do + Postgresql::DetachedPartition.create!( + table_name: '_test_gitlab_hook_logs_202201', + drop_after: Time.current + ) + end + + allow(Gitlab::Database::GitlabSchema).to receive(:tables_to_schema).and_return( + { + "_test_gitlab_main_items" => :gitlab_main, + "_test_gitlab_main_references" => :gitlab_main, + "_test_gitlab_hook_logs" => :gitlab_main, + "_test_gitlab_ci_items" => :gitlab_ci, + "_test_gitlab_ci_references" => :gitlab_ci, + "_test_gitlab_shared_items" => :gitlab_shared, + "_test_gitlab_geo_items" => :gitlab_geo + } + ) + + allow(Gitlab::Database::GitlabSchema).to receive(:views_and_tables_to_schema).and_return( + { + "_test_gitlab_main_items" => :gitlab_main, + "_test_gitlab_main_references" => :gitlab_main, + "_test_gitlab_hook_logs" => :gitlab_main, + "_test_gitlab_ci_items" => :gitlab_ci, + "_test_gitlab_ci_references" => :gitlab_ci, + "_test_gitlab_shared_items" => :gitlab_shared, + "_test_gitlab_geo_items" => :gitlab_geo, + "detached_partitions" => :gitlab_shared, + "postgres_foreign_keys" => :gitlab_shared, + "postgres_partitions" => :gitlab_shared + } + ) + + allow(logger).to receive(:info).with(any_args) + end + shared_examples 'truncating legacy tables on a database' do - let(:logger) { instance_double(Logger) } let(:dry_run) { false } let(:until_table) { nil } @@ -47,122 +163,6 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba ).execute end - before do - skip_if_shared_database(:ci) - - # Creating some test tables on the main database - main_tables_sql = <<~SQL - CREATE TABLE _test_gitlab_main_items (id serial NOT NULL PRIMARY KEY); - - CREATE TABLE _test_gitlab_main_references ( - id serial NOT NULL PRIMARY KEY, - item_id BIGINT NOT NULL, - CONSTRAINT fk_constrained_1 FOREIGN KEY(item_id) REFERENCES _test_gitlab_main_items(id) - ); - - CREATE TABLE _test_gitlab_hook_logs ( - id bigserial not null, - created_at timestamptz not null, - item_id BIGINT NOT NULL, - PRIMARY KEY (id, created_at), - CONSTRAINT fk_constrained_1 FOREIGN KEY(item_id) REFERENCES _test_gitlab_main_items(id) - ) PARTITION BY RANGE(created_at); - - CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_202201 - PARTITION OF _test_gitlab_hook_logs - FOR VALUES FROM ('20220101') TO ('20220131'); - - CREATE TABLE gitlab_partitions_dynamic._test_gitlab_hook_logs_202202 - PARTITION OF _test_gitlab_hook_logs - FOR VALUES FROM ('20220201') TO ('20220228'); - - ALTER TABLE _test_gitlab_hook_logs DETACH PARTITION gitlab_partitions_dynamic._test_gitlab_hook_logs_202201; - SQL - - execute_on_each_database(main_tables_sql) - - ci_tables_sql = <<~SQL - CREATE TABLE _test_gitlab_ci_items (id serial NOT NULL PRIMARY KEY); - - CREATE TABLE _test_gitlab_ci_references ( - id serial NOT NULL PRIMARY KEY, - item_id BIGINT NOT NULL, - CONSTRAINT fk_constrained_1 FOREIGN KEY(item_id) REFERENCES _test_gitlab_ci_items(id) - ); - SQL - - execute_on_each_database(ci_tables_sql) - - internal_tables_sql = <<~SQL - CREATE TABLE _test_gitlab_shared_items (id serial NOT NULL PRIMARY KEY); - SQL - - execute_on_each_database(internal_tables_sql) - - # Filling the tables - 5.times do |i| - # Main Database - main_db_main_item_model.create!(id: i) - main_db_main_reference_model.create!(item_id: i) - main_db_ci_item_model.create!(id: i) - main_db_ci_reference_model.create!(item_id: i) - main_db_shared_item_model.create!(id: i) - main_db_partitioned_item.create!(item_id: i, created_at: '2022-02-02 02:00') - main_db_partitioned_item_detached.create!(item_id: i, created_at: '2022-01-01 01:00') - # CI Database - ci_db_main_item_model.create!(id: i) - ci_db_main_reference_model.create!(item_id: i) - ci_db_ci_item_model.create!(id: i) - ci_db_ci_reference_model.create!(item_id: i) - ci_db_shared_item_model.create!(id: i) - ci_db_partitioned_item.create!(item_id: i, created_at: '2022-02-02 02:00') - ci_db_partitioned_item_detached.create!(item_id: i, created_at: '2022-01-01 01:00') - end - - Gitlab::Database::SharedModel.using_connection(main_connection) do - Postgresql::DetachedPartition.create!( - table_name: '_test_gitlab_hook_logs_202201', - drop_after: Time.current - ) - end - - Gitlab::Database::SharedModel.using_connection(ci_connection) do - Postgresql::DetachedPartition.create!( - table_name: '_test_gitlab_hook_logs_202201', - drop_after: Time.current - ) - end - - allow(Gitlab::Database::GitlabSchema).to receive(:tables_to_schema).and_return( - { - "_test_gitlab_main_items" => :gitlab_main, - "_test_gitlab_main_references" => :gitlab_main, - "_test_gitlab_hook_logs" => :gitlab_main, - "_test_gitlab_ci_items" => :gitlab_ci, - "_test_gitlab_ci_references" => :gitlab_ci, - "_test_gitlab_shared_items" => :gitlab_shared, - "_test_gitlab_geo_items" => :gitlab_geo - } - ) - - allow(Gitlab::Database::GitlabSchema).to receive(:views_and_tables_to_schema).and_return( - { - "_test_gitlab_main_items" => :gitlab_main, - "_test_gitlab_main_references" => :gitlab_main, - "_test_gitlab_hook_logs" => :gitlab_main, - "_test_gitlab_ci_items" => :gitlab_ci, - "_test_gitlab_ci_references" => :gitlab_ci, - "_test_gitlab_shared_items" => :gitlab_shared, - "_test_gitlab_geo_items" => :gitlab_geo, - "detached_partitions" => :gitlab_shared, - "postgres_foreign_keys" => :gitlab_shared, - "postgres_partitions" => :gitlab_shared - } - ) - - allow(logger).to receive(:info).with(any_args) - end - context 'when the truncated tables are not locked for writes' do it 'raises an error that the tables are not locked for writes' do error_message = /is not locked for writes. Run the rake task gitlab:db:lock_writes first/ @@ -348,6 +348,50 @@ RSpec.describe Gitlab::Database::TablesTruncate, :reestablished_active_record_ba end end + describe '#needs_truncation?' do + let(:database_name) { 'ci' } + + subject { described_class.new(database_name: database_name).needs_truncation? } + + context 'when running in a single database mode' do + before do + skip_if_multiple_databases_are_setup(:ci) + end + + it { is_expected.to eq(false) } + end + + context 'when running in a multiple database mode' do + before do + skip_if_shared_database(:ci) + end + + context 'with main data in ci database' do + it { is_expected.to eq(true) } + end + + context 'with no main data in ci datatabase' do + before do + # Remove 'main' data in ci database + ci_connection.truncate_tables([:_test_gitlab_main_items, :_test_gitlab_main_references]) + end + + it { is_expected.to eq(false) } + + it 'supresses some QueryAnalyzers' do + expect( + Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection + ).to receive(:with_suppressed).and_call_original + expect( + Gitlab::Database::QueryAnalyzers::Ci::PartitioningRoutingAnalyzer + ).to receive(:with_suppressed).and_call_original + + subject + end + end + end + end + def geo_configured? !!ActiveRecord::Base.configurations.configs_for(env_name: Rails.env, name: 'geo') end |