diff options
Diffstat (limited to 'spec/lib/gitlab/database')
37 files changed, 766 insertions, 151 deletions
diff --git a/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb b/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb new file mode 100644 index 00000000000..adb0f45706d --- /dev/null +++ b/spec/lib/gitlab/database/async_indexes/index_destructor_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::AsyncIndexes::IndexDestructor do + include ExclusiveLeaseHelpers + + describe '#perform' do + subject { described_class.new(async_index) } + + let(:async_index) { create(:postgres_async_index, :with_drop) } + + let(:index_model) { Gitlab::Database::AsyncIndexes::PostgresAsyncIndex } + + let(:model) { Gitlab::Database.database_base_models[Gitlab::Database::PRIMARY_DATABASE_NAME] } + let(:connection) { model.connection } + + let!(:lease) { stub_exclusive_lease(lease_key, :uuid, timeout: lease_timeout) } + let(:lease_key) { "gitlab/database/async_indexes/index_destructor/#{Gitlab::Database::PRIMARY_DATABASE_NAME}" } + let(:lease_timeout) { described_class::TIMEOUT_PER_ACTION } + + before do + connection.add_index(async_index.table_name, 'id', name: async_index.name) + end + + around do |example| + Gitlab::Database::SharedModel.using_connection(connection) do + example.run + end + end + + context 'when the index does not exist' do + before do + connection.execute(async_index.definition) + end + + it 'skips index destruction' do + expect(connection).not_to receive(:execute).with(/DROP INDEX/) + + subject.perform + end + end + + it 'creates the index while controlling lock timeout' do + allow(connection).to receive(:execute).and_call_original + expect(connection).to receive(:execute).with("SET lock_timeout TO '60000ms'").and_call_original + expect(connection).to receive(:execute).with(async_index.definition).and_call_original + expect(connection).to receive(:execute) + .with("RESET idle_in_transaction_session_timeout; RESET lock_timeout") + .and_call_original + + subject.perform + end + + it 'removes the index preparation record from postgres_async_indexes' do + expect(async_index).to receive(:destroy).and_call_original + + expect { subject.perform }.to change { index_model.count }.by(-1) + end + + it 'skips logic if not able to acquire exclusive lease' do + expect(lease).to receive(:try_obtain).ordered.and_return(false) + expect(connection).not_to receive(:execute).with(/DROP INDEX/) + expect(async_index).not_to receive(:destroy) + + expect { subject.perform }.not_to change { index_model.count } + end + end +end 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 9ba3dad72b3..52f5e37eff2 100644 --- a/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/migration_helpers_spec.rb @@ -142,4 +142,42 @@ RSpec.describe Gitlab::Database::AsyncIndexes::MigrationHelpers do end end end + + describe '#prepare_async_index_removal' do + before do + connection.create_table(table_name) + connection.add_index(table_name, 'id', name: index_name) + end + + it 'creates the record for the async index removal' do + expect do + migration.prepare_async_index_removal(table_name, 'id', name: index_name) + end.to change { index_model.where(name: index_name).count }.by(1) + + record = index_model.find_by(name: index_name) + + expect(record.table_name).to eq(table_name) + expect(record.definition).to match(/DROP INDEX CONCURRENTLY "#{index_name}"/) + end + + context 'when the index does not exist' do + it 'does not create the record' do + connection.remove_index(table_name, 'id', name: index_name) + + expect do + migration.prepare_async_index_removal(table_name, 'id', name: index_name) + end.not_to change { index_model.where(name: index_name).count } + end + end + + context 'when the record already exists' do + it 'does attempt to create the record' do + create(:postgres_async_index, table_name: table_name, name: index_name) + + expect do + migration.prepare_async_index_removal(table_name, 'id', name: index_name) + end.not_to change { index_model.where(name: index_name).count } + end + end + end end diff --git a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb index 223730f87c0..806d57af4b3 100644 --- a/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb +++ b/spec/lib/gitlab/database/async_indexes/postgres_async_index_spec.rb @@ -16,4 +16,21 @@ RSpec.describe Gitlab::Database::AsyncIndexes::PostgresAsyncIndex, type: :model it { is_expected.to validate_presence_of(:definition) } it { is_expected.to validate_length_of(:definition).is_at_most(definition_limit) } end + + describe 'scopes' do + let!(:async_index_creation) { create(:postgres_async_index) } + let!(:async_index_destruction) { create(:postgres_async_index, :with_drop) } + + describe '.to_create' do + subject { described_class.to_create } + + it { is_expected.to contain_exactly(async_index_creation) } + end + + describe '.to_drop' do + subject { described_class.to_drop } + + it { is_expected.to contain_exactly(async_index_destruction) } + end + end end diff --git a/spec/lib/gitlab/database/async_indexes_spec.rb b/spec/lib/gitlab/database/async_indexes_spec.rb index 74e30ea2c4e..8a5509f892f 100644 --- a/spec/lib/gitlab/database/async_indexes_spec.rb +++ b/spec/lib/gitlab/database/async_indexes_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Gitlab::Database::AsyncIndexes do end it 'takes 2 pending indexes and creates those' do - Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.order(:id).limit(2).each do |index| + Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_create.order(:id).limit(2).each do |index| creator = double('index creator') expect(Gitlab::Database::AsyncIndexes::IndexCreator).to receive(:new).with(index).and_return(creator) expect(creator).to receive(:perform) @@ -20,4 +20,22 @@ RSpec.describe Gitlab::Database::AsyncIndexes do subject end end + + describe '.drop_pending_indexes!' do + subject { described_class.drop_pending_indexes! } + + before do + create_list(:postgres_async_index, 4, :with_drop) + end + + it 'takes 2 pending indexes and destroys those' do + Gitlab::Database::AsyncIndexes::PostgresAsyncIndex.to_drop.order(:id).limit(2).each do |index| + destructor = double('index destructor') + expect(Gitlab::Database::AsyncIndexes::IndexDestructor).to receive(:new).with(index).and_return(destructor) + expect(destructor).to receive(:perform) + end + + subject + end + end end diff --git a/spec/lib/gitlab/database/background_migration/batched_job_spec.rb b/spec/lib/gitlab/database/background_migration/batched_job_spec.rb index a7b3670da7c..32746a46308 100644 --- a/spec/lib/gitlab/database/background_migration/batched_job_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_job_spec.rb @@ -304,6 +304,13 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedJob, type: :model d it { expect(subject).to be_falsey } end + + context 'when the batch_size is 1' do + let(:job) { create(:batched_background_migration_job, :failed, batch_size: 1) } + let(:exception) { ActiveRecord::StatementTimeout.new } + + it { expect(subject).to be_falsey } + end end describe '#time_efficiency' do @@ -415,10 +422,18 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedJob, type: :model d end context 'when batch size is already 1' do - let!(:job) { create(:batched_background_migration_job, :failed, batch_size: 1) } + let!(:job) { create(:batched_background_migration_job, :failed, batch_size: 1, attempts: 3) } - it 'raises an exception' do - expect { job.split_and_retry! }.to raise_error 'Job cannot be split further' + it 'keeps the same batch size' do + job.split_and_retry! + + expect(job.reload.batch_size).to eq 1 + end + + it 'resets the number of attempts' do + job.split_and_retry! + + expect(job.attempts).to eq 0 end end diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb index b8ff78be333..4ef2e7f936b 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_runner_spec.rb @@ -15,8 +15,8 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end before do - allow(Gitlab::Database::BackgroundMigration::HealthStatus).to receive(:evaluate) - .and_return(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Normal) + normal_signal = instance_double(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Normal, stop?: false) + allow(Gitlab::Database::BackgroundMigration::HealthStatus).to receive(:evaluate).and_return([normal_signal]) end describe '#run_migration_job' do @@ -77,14 +77,14 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end it 'puts migration on hold on stop signal' do - expect(health_status).to receive(:evaluate).and_return(stop_signal) + expect(health_status).to receive(:evaluate).and_return([stop_signal]) expect { runner.run_migration_job(migration) }.to change { migration.on_hold? } .from(false).to(true) end it 'optimizes migration on normal signal' do - expect(health_status).to receive(:evaluate).and_return(normal_signal) + expect(health_status).to receive(:evaluate).and_return([normal_signal]) expect(migration).to receive(:optimize!) @@ -92,7 +92,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end it 'optimizes migration on no signal' do - expect(health_status).to receive(:evaluate).and_return(not_available_signal) + expect(health_status).to receive(:evaluate).and_return([not_available_signal]) expect(migration).to receive(:optimize!) @@ -100,7 +100,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end it 'optimizes migration on unknown signal' do - expect(health_status).to receive(:evaluate).and_return(unknown_signal) + expect(health_status).to receive(:evaluate).and_return([unknown_signal]) expect(migration).to receive(:optimize!) diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb index 55f607c0cb0..06c2bc32db3 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -307,7 +307,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end describe '#batch_class' do - let(:batch_class) { Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchingStrategy} + let(:batch_class) { Gitlab::BackgroundMigration::BatchingStrategies::PrimaryKeyBatchingStrategy } let(:batched_migration) { build(:batched_background_migration) } it 'returns the class of the batch strategy for the migration' do @@ -617,6 +617,49 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end end + describe '#progress' do + subject { migration.progress } + + context 'when the migration is finished' do + let(:migration) do + create(:batched_background_migration, :finished, total_tuple_count: 1).tap do |record| + create(:batched_background_migration_job, :succeeded, batched_migration: record, batch_size: 1) + end + end + + it 'returns 100' do + expect(subject).to be 100 + end + end + + context 'when the migration does not have jobs' do + let(:migration) { create(:batched_background_migration, :active) } + + it 'returns zero' do + expect(subject).to be 0 + end + end + + context 'when the `total_tuple_count` is zero' do + let(:migration) { create(:batched_background_migration, :active, total_tuple_count: 0) } + let!(:batched_job) { create(:batched_background_migration_job, :succeeded, batched_migration: migration) } + + it 'returns nil' do + expect(subject).to be nil + end + end + + context 'when migration has completed jobs' do + let(:migration) { create(:batched_background_migration, :active, total_tuple_count: 100) } + + let!(:batched_job) { create(:batched_background_migration_job, :succeeded, batched_migration: migration, batch_size: 8) } + + it 'calculates the progress' do + expect(subject).to be 8 + end + end + end + describe '.for_configuration' do let!(:attributes) do { diff --git a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb index 83c0275a870..983f482d464 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_wrapper_spec.rb @@ -38,10 +38,11 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' batch_column: 'id', sub_batch_size: 1, pause_ms: pause_ms, + job_arguments: active_migration.job_arguments, connection: connection) .and_return(job_instance) - expect(job_instance).to receive(:perform).with('id', 'other_id') + expect(job_instance).to receive(:perform).with(no_args) perform end @@ -49,7 +50,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' it 'updates the tracking record in the database' do test_metrics = { 'my_metrics' => 'some value' } - expect(job_instance).to receive(:perform).with('id', 'other_id') + expect(job_instance).to receive(:perform).with(no_args) expect(job_instance).to receive(:batch_metrics).and_return(test_metrics) freeze_time do @@ -78,7 +79,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' it 'increments attempts and updates other fields' do updated_metrics = { 'updated_metrics' => 'some_value' } - expect(job_instance).to receive(:perform).with('id', 'other_id') + expect(job_instance).to receive(:perform).with(no_args) expect(job_instance).to receive(:batch_metrics).and_return(updated_metrics) freeze_time do @@ -97,7 +98,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' context 'when the migration job does not raise an error' do it 'marks the tracking record as succeeded' do - expect(job_instance).to receive(:perform).with('id', 'other_id') + expect(job_instance).to receive(:perform).with(no_args) freeze_time do perform @@ -110,7 +111,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end it 'tracks metrics of the execution' do - expect(job_instance).to receive(:perform).with('id', 'other_id') + expect(job_instance).to receive(:perform).with(no_args) expect(metrics_tracker).to receive(:track).with(job_record) perform @@ -120,7 +121,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' context 'when the migration job raises an error' do shared_examples 'an error is raised' do |error_class| it 'marks the tracking record as failed' do - expect(job_instance).to receive(:perform).with('id', 'other_id').and_raise(error_class) + expect(job_instance).to receive(:perform).with(no_args).and_raise(error_class) freeze_time do expect { perform }.to raise_error(error_class) @@ -133,7 +134,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end it 'tracks metrics of the execution' do - expect(job_instance).to receive(:perform).with('id', 'other_id').and_raise(error_class) + expect(job_instance).to receive(:perform).with(no_args).and_raise(error_class) expect(metrics_tracker).to receive(:track).with(job_record) expect { perform }.to raise_error(error_class) @@ -147,6 +148,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' context 'when the batched background migration does not inherit from BatchedMigrationJob' do let(:job_class) { Class.new } + let(:job_instance) { job_class.new } it 'runs the job with the correct arguments' do expect(job_class).to receive(:new).with(no_args).and_return(job_instance) diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb b/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb index 21204814f17..db4383a79d4 100644 --- a/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb +++ b/spec/lib/gitlab/database/background_migration/health_status/indicators/autovacuum_active_on_table_spec.rb @@ -20,9 +20,9 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators:: swapout_view_for_table(:postgres_autovacuum_activity) end - let(:context) { Gitlab::Database::BackgroundMigration::HealthStatus::Context.new(tables) } let(:tables) { [table] } let(:table) { 'users' } + let(:context) { Gitlab::Database::BackgroundMigration::HealthStatus::Context.new(connection, tables) } context 'without autovacuum activity' do it 'returns Normal signal' do diff --git a/spec/lib/gitlab/database/background_migration/health_status/indicators/write_ahead_log_spec.rb b/spec/lib/gitlab/database/background_migration/health_status/indicators/write_ahead_log_spec.rb new file mode 100644 index 00000000000..650f11e3cd5 --- /dev/null +++ b/spec/lib/gitlab/database/background_migration/health_status/indicators/write_ahead_log_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus::Indicators::WriteAheadLog do + let(:connection) { Gitlab::Database.database_base_models[:main].connection } + + around do |example| + Gitlab::Database::SharedModel.using_connection(connection) do + example.run + end + end + + describe '#evaluate' do + let(:tables) { [table] } + let(:table) { 'users' } + let(:context) { Gitlab::Database::BackgroundMigration::HealthStatus::Context.new(connection, tables) } + + subject(:evaluate) { described_class.new(context).evaluate } + + it 'remembers the indicator class' do + expect(evaluate.indicator_class).to eq(described_class) + end + + it 'returns NoSignal signal in case the feature flag is disabled' do + stub_feature_flags(batched_migrations_health_status_wal: false) + + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::NotAvailable) + expect(evaluate.reason).to include('indicator disabled') + end + + it 'returns NoSignal signal when WAL archive queue can not be calculated' do + expect(connection).to receive(:execute).and_return([{ 'pending_wal_count' => nil }]) + + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::NotAvailable) + expect(evaluate.reason).to include('WAL archive queue can not be calculated') + end + + it 'uses primary database' do + expect(Gitlab::Database::LoadBalancing::Session.current).to receive(:use_primary).and_yield + + evaluate + end + + context 'when WAL archive queue size is below the limit' do + it 'returns Normal signal' do + expect(connection).to receive(:execute).and_return([{ 'pending_wal_count' => 1 }]) + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Normal) + expect(evaluate.reason).to include('WAL archive queue is within limit') + end + end + + context 'when WAL archive queue size is above the limit' do + it 'returns Stop signal' do + expect(connection).to receive(:execute).and_return([{ 'pending_wal_count' => 420 }]) + expect(evaluate).to be_a(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Stop) + expect(evaluate.reason).to include('WAL archive queue is too big') + end + end + end +end diff --git a/spec/lib/gitlab/database/background_migration/health_status_spec.rb b/spec/lib/gitlab/database/background_migration/health_status_spec.rb index 6d0430dcbbb..8bc04d80fa1 100644 --- a/spec/lib/gitlab/database/background_migration/health_status_spec.rb +++ b/spec/lib/gitlab/database/background_migration/health_status_spec.rb @@ -12,30 +12,47 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus do end describe '.evaluate' do - subject(:evaluate) { described_class.evaluate(migration, indicator_class) } + subject(:evaluate) { described_class.evaluate(migration, [autovacuum_indicator_class]) } let(:migration) { build(:batched_background_migration, :active) } - let(:health_status) { 'Gitlab::Database::BackgroundMigration::HealthStatus' } - let(:indicator_class) { class_double("#{health_status}::Indicators::AutovacuumActiveOnTable") } - let(:indicator) { instance_double("#{health_status}::Indicators::AutovacuumActiveOnTable") } + let(:health_status) { Gitlab::Database::BackgroundMigration::HealthStatus } + let(:autovacuum_indicator_class) { health_status::Indicators::AutovacuumActiveOnTable } + let(:wal_indicator_class) { health_status::Indicators::WriteAheadLog } + let(:autovacuum_indicator) { instance_double(autovacuum_indicator_class) } + let(:wal_indicator) { instance_double(wal_indicator_class) } before do - allow(indicator_class).to receive(:new).with(migration.health_context).and_return(indicator) + allow(autovacuum_indicator_class).to receive(:new).with(migration.health_context).and_return(autovacuum_indicator) end - it 'returns a signal' do + context 'with default indicators' do + subject(:evaluate) { described_class.evaluate(migration) } + + it 'returns a collection of signals' do + normal_signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) + not_available_signal = instance_double("#{health_status}::Signals::NotAvailable", log_info?: false) + + expect(autovacuum_indicator).to receive(:evaluate).and_return(normal_signal) + expect(wal_indicator_class).to receive(:new).with(migration.health_context).and_return(wal_indicator) + expect(wal_indicator).to receive(:evaluate).and_return(not_available_signal) + + expect(evaluate).to contain_exactly(normal_signal, not_available_signal) + end + end + + it 'returns a collection of signals' do signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) - expect(indicator).to receive(:evaluate).and_return(signal) + expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) - expect(evaluate).to eq(signal) + expect(evaluate).to contain_exactly(signal) end it 'logs interesting signals' do signal = instance_double("#{health_status}::Signals::Stop", log_info?: true) - expect(indicator).to receive(:evaluate).and_return(signal) + expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) expect(described_class).to receive(:log_signal).with(signal, migration) evaluate @@ -44,7 +61,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus do it 'does not log signals of no interest' do signal = instance_double("#{health_status}::Signals::Normal", log_info?: false) - expect(indicator).to receive(:evaluate).and_return(signal) + expect(autovacuum_indicator).to receive(:evaluate).and_return(signal) expect(described_class).not_to receive(:log_signal) evaluate @@ -54,7 +71,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus do let(:error) { RuntimeError.new('everything broken') } before do - expect(indicator).to receive(:evaluate).and_raise(error) + expect(autovacuum_indicator).to receive(:evaluate).and_raise(error) end it 'does not fail' do @@ -62,8 +79,10 @@ RSpec.describe Gitlab::Database::BackgroundMigration::HealthStatus do end it 'returns Unknown signal' do - expect(evaluate).to be_an_instance_of(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) - expect(evaluate.reason).to eq("unexpected error: everything broken (RuntimeError)") + signal = evaluate.first + + expect(signal).to be_an_instance_of(Gitlab::Database::BackgroundMigration::HealthStatus::Signals::Unknown) + expect(signal.reason).to eq("unexpected error: everything broken (RuntimeError)") end it 'reports the exception to error tracking' do diff --git a/spec/lib/gitlab/database/bulk_update_spec.rb b/spec/lib/gitlab/database/bulk_update_spec.rb index 08b4d50f83b..fa519cffd6b 100644 --- a/spec/lib/gitlab/database/bulk_update_spec.rb +++ b/spec/lib/gitlab/database/bulk_update_spec.rb @@ -91,7 +91,8 @@ RSpec.describe Gitlab::Database::BulkUpdate do .to eq(['MR a', 'Issue a', 'Issue b']) end - context 'validates prepared_statements support', :reestablished_active_record_base do + context 'validates prepared_statements support', :reestablished_active_record_base, + :suppress_gitlab_schemas_validate_connection do using RSpec::Parameterized::TableSyntax where(:prepared_statements) do 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 34eb64997c1..9c09253b24c 100644 --- a/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/load_balancer_spec.rb @@ -358,7 +358,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end it 'returns true for deeply wrapped/nested errors' do - top = twice_wrapped_exception(ActionView::Template::Error, ActiveRecord::StatementInvalid, ActiveRecord::ConnectionNotEstablished) + top = twice_wrapped_exception( + ActionView::Template::Error, + ActiveRecord::StatementInvalid, + ActiveRecord::ConnectionNotEstablished + ) expect(lb.connection_error?(top)).to eq(true) end @@ -404,7 +408,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end describe '#select_up_to_date_host' do - let(:location) { 'AB/12345'} + let(:location) { 'AB/12345' } let(:hosts) { lb.host_list.hosts } let(:set_host) { request_cache[described_class::CACHE_KEY] } @@ -455,7 +459,7 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do end it 'does not modify connection class pool' do - expect { with_replica_pool(5) { } }.not_to change { ActiveRecord::Base.connection_pool } + expect { with_replica_pool(5) {} }.not_to change { ActiveRecord::Base.connection_pool } end def with_replica_pool(*args) 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 b768d4ecea3..a1c141af537 100644 --- a/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/rack_middleware_spec.rb @@ -30,6 +30,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::RackMiddleware, :redis do expect(app).to receive(:call).with(env).and_return(10) + allow(ActiveSupport::Notifications).to receive(:instrument).and_call_original + expect(ActiveSupport::Notifications) .to receive(:instrument) .with('web_transaction_completed.load_balancing') diff --git a/spec/lib/gitlab/database/load_balancing/session_spec.rb b/spec/lib/gitlab/database/load_balancing/session_spec.rb index 74512f76fd4..05b44579c62 100644 --- a/spec/lib/gitlab/database/load_balancing/session_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/session_spec.rb @@ -132,7 +132,11 @@ RSpec.describe Gitlab::Database::LoadBalancing::Session do it 'does not prevent using primary if an exception is raised' do instance = described_class.new - instance.ignore_writes { raise ArgumentError } rescue ArgumentError + begin + instance.ignore_writes { raise ArgumentError } + rescue ArgumentError + nil + end instance.write! expect(instance).to be_using_primary 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 31be3963565..8053bd57bba 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 @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe Gitlab::Database::LoadBalancing::SidekiqServerMiddleware, :clean_gitlab_redis_queues do let(:middleware) { described_class.new } let(:worker) { worker_class.new } - let(:location) {'0/D525E3A8' } + let(:location) { '0/D525E3A8' } let(:wal_locations) { { Gitlab::Database::MAIN_DATABASE_NAME.to_sym => location } } let(:job) { { "retry" => 3, "job_id" => "a180b47c-3fd6-41b8-81e9-34da61c3400e", 'wal_locations' => wal_locations } } diff --git a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb index f3139bb1b4f..2ffb2c32c32 100644 --- a/spec/lib/gitlab/database/load_balancing/sticking_spec.rb +++ b/spec/lib/gitlab/database/load_balancing/sticking_spec.rb @@ -77,6 +77,8 @@ RSpec.describe Gitlab::Database::LoadBalancing::Sticking, :redis do 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) diff --git a/spec/lib/gitlab/database/load_balancing_spec.rb b/spec/lib/gitlab/database/load_balancing_spec.rb index f320fe0276f..76dfaa74ae6 100644 --- a/spec/lib/gitlab/database/load_balancing_spec.rb +++ b/spec/lib/gitlab/database/load_balancing_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::LoadBalancing do +RSpec.describe Gitlab::Database::LoadBalancing, :suppress_gitlab_schemas_validate_connection do describe '.base_models' do it 'returns the models to apply load balancing to' do models = described_class.base_models diff --git a/spec/lib/gitlab/database/lock_writes_manager_spec.rb b/spec/lib/gitlab/database/lock_writes_manager_spec.rb new file mode 100644 index 00000000000..eb527d492cf --- /dev/null +++ b/spec/lib/gitlab/database/lock_writes_manager_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::LockWritesManager do + let(:connection) { ApplicationRecord.connection } + let(:test_table) { '_test_table' } + let(:logger) { instance_double(Logger) } + + subject(:lock_writes_manager) do + described_class.new( + table_name: test_table, + connection: connection, + database_name: 'main', + logger: logger + ) + end + + before do + allow(logger).to receive(:info) + + connection.execute(<<~SQL) + CREATE TABLE #{test_table} (id integer NOT NULL, value integer NOT NULL DEFAULT 0); + + INSERT INTO #{test_table} (id, value) + VALUES (1, 1), (2, 2), (3, 3) + SQL + end + + describe '#lock_writes' do + it 'prevents any writes on the table' do + subject.lock_writes + + expect do + connection.execute("delete from #{test_table}") + end.to raise_error(ActiveRecord::StatementInvalid, /Table: "#{test_table}" is write protected/) + end + + it 'prevents truncating the table' do + subject.lock_writes + + expect do + connection.execute("truncate #{test_table}") + end.to raise_error(ActiveRecord::StatementInvalid, /Table: "#{test_table}" is write protected/) + end + + it 'adds 3 triggers to the ci schema tables on the main database' do + expect do + subject.lock_writes + end.to change { + number_of_triggers_on(connection, test_table) + }.by(3) # Triggers to block INSERT / UPDATE / DELETE + # Triggers on TRUNCATE are not added to the information_schema.triggers + # See https://www.postgresql.org/message-id/16934.1568989957%40sss.pgh.pa.us + end + + it 'logs the write locking' do + expect(logger).to receive(:info).with("Database: 'main', Table: '_test_table': Lock Writes") + + subject.lock_writes + end + + it 'retries again if it receives a statement_timeout a few number of times' do + error_message = "PG::QueryCanceled: ERROR: canceling statement due to statement timeout" + call_count = 0 + allow(connection).to receive(:execute) do |statement| + if statement.include?("CREATE TRIGGER") + call_count += 1 + raise(ActiveRecord::QueryCanceled, error_message) if call_count.even? + end + end + subject.lock_writes + end + + it 'raises the exception if it happened many times' do + error_message = "PG::QueryCanceled: ERROR: canceling statement due to statement timeout" + allow(connection).to receive(:execute) do |statement| + if statement.include?("CREATE TRIGGER") + raise(ActiveRecord::QueryCanceled, error_message) + end + end + + expect do + subject.lock_writes + end.to raise_error(ActiveRecord::QueryCanceled) + end + end + + describe '#unlock_writes' do + before do + subject.lock_writes + end + + it 'allows writing on the table again' do + subject.unlock_writes + + expect do + connection.execute("delete from #{test_table}") + end.not_to raise_error + end + + it 'removes the write protection triggers from the gitlab_main tables on the ci database' do + expect do + subject.unlock_writes + end.to change { + number_of_triggers_on(connection, test_table) + }.by(-3) # Triggers to block INSERT / UPDATE / DELETE + # Triggers on TRUNCATE are not added to the information_schema.triggers + # See https://www.postgresql.org/message-id/16934.1568989957%40sss.pgh.pa.us + end + + it 'logs the write unlocking' do + expect(logger).to receive(:info).with("Database: 'main', Table: '_test_table': Allow Writes") + + subject.unlock_writes + end + end + + def number_of_triggers_on(connection, table_name) + connection + .select_value("SELECT count(*) FROM information_schema.triggers WHERE event_object_table=$1", nil, [table_name]) + end +end diff --git a/spec/lib/gitlab/database/loose_foreign_keys_spec.rb b/spec/lib/gitlab/database/loose_foreign_keys_spec.rb index 87a3e0f81e4..ff99f681b0c 100644 --- a/spec/lib/gitlab/database/loose_foreign_keys_spec.rb +++ b/spec/lib/gitlab/database/loose_foreign_keys_spec.rb @@ -84,4 +84,32 @@ RSpec.describe Gitlab::Database::LooseForeignKeys do end end end + + describe '.definitions' do + subject(:definitions) { described_class.definitions } + + it 'contains at least all parent tables that have triggers' do + all_definition_parent_tables = definitions.map { |d| d.to_table }.to_set + + triggers_query = <<~SQL + SELECT event_object_table, trigger_name + FROM information_schema.triggers + WHERE trigger_name LIKE '%_loose_fk_trigger' + GROUP BY event_object_table, trigger_name + SQL + + all_triggers = ApplicationRecord.connection.execute(triggers_query) + + all_triggers.each do |trigger| + table = trigger['event_object_table'] + trigger_name = trigger['trigger_name'] + error_message = <<~END + Missing a loose foreign key definition for parent table: #{table} with trigger: #{trigger_name}. + Loose foreign key definitions must be added before triggers are added and triggers must be removed before removing the loose foreign key definition. + Read more at https://docs.gitlab.com/ee/development/database/loose_foreign_keys.html ." + END + expect(all_definition_parent_tables).to include(table), error_message + end + end + end end diff --git a/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb b/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb index 1009ec354c3..e43cfe0814e 100644 --- a/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/restrict_gitlab_schema_spec.rb @@ -5,6 +5,13 @@ require 'spec_helper' RSpec.describe Gitlab::Database::MigrationHelpers::RestrictGitlabSchema, query_analyzers: false, stub_feature_flags: false do let(:schema_class) { Class.new(Gitlab::Database::Migration[1.0]).include(described_class) } + # We keep only the GitlabSchemasValidateConnection analyzer running + around do |example| + Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection.with_suppressed(false) do + example.run + end + end + describe '#restrict_gitlab_migration' do it 'invalid schema raises exception' do expect { schema_class.restrict_gitlab_migration gitlab_schema: :gitlab_non_exisiting } diff --git a/spec/lib/gitlab/database/migration_helpers/v2_spec.rb b/spec/lib/gitlab/database/migration_helpers/v2_spec.rb index 5c054795697..2055dc33d48 100644 --- a/spec/lib/gitlab/database/migration_helpers/v2_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers/v2_spec.rb @@ -266,7 +266,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do let(:env) { { 'DISABLE_LOCK_RETRIES' => 'true' } } it 'sets the migration class name in the logs' do - model.with_lock_retries(env: env, logger: in_memory_logger) { } + model.with_lock_retries(env: env, logger: in_memory_logger) {} buffer.rewind expect(buffer.read).to include("\"class\":\"#{model.class}\"") @@ -280,7 +280,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do expect(Gitlab::Database::WithLockRetries).to receive(:new).and_return(with_lock_retries) expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: raise_on_exhaustion) - model.with_lock_retries(env: env, logger: in_memory_logger, raise_on_exhaustion: raise_on_exhaustion) { } + model.with_lock_retries(env: env, logger: in_memory_logger, raise_on_exhaustion: raise_on_exhaustion) {} end end @@ -289,7 +289,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do expect(Gitlab::Database::WithLockRetries).to receive(:new).and_return(with_lock_retries) expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false) - model.with_lock_retries(env: env, logger: in_memory_logger) { } + model.with_lock_retries(env: env, logger: in_memory_logger) {} end it 'defaults to disallowing subtransactions' do @@ -297,7 +297,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do expect(Gitlab::Database::WithLockRetries).to receive(:new).with(hash_including(allow_savepoints: false)).and_return(with_lock_retries) expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false) - model.with_lock_retries(env: env, logger: in_memory_logger) { } + model.with_lock_retries(env: env, logger: in_memory_logger) {} end context 'when in transaction' do @@ -323,7 +323,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers::V2 do end it 'raises an error' do - expect { model.with_lock_retries(env: env, logger: in_memory_logger) { } }.to raise_error /can not be run inside an already open transaction/ + expect { model.with_lock_retries(env: env, logger: in_memory_logger) {} }.to raise_error /can not be run inside an already open transaction/ end end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 3ccc3a17862..dd5ad40d8ef 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -15,7 +15,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end describe 'overridden dynamic model helpers' do - let(:test_table) { '__test_batching_table' } + let(:test_table) { '_test_batching_table' } before do model.connection.execute(<<~SQL) @@ -1022,6 +1022,40 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(Project.sum(:star_count)).to eq(2 * Project.count) end end + + context 'when the table is write-locked' do + let(:test_table) { '_test_table' } + let(:lock_writes_manager) do + Gitlab::Database::LockWritesManager.new( + table_name: test_table, + connection: model.connection, + database_name: 'main' + ) + end + + before do + model.connection.execute(<<~SQL) + CREATE TABLE #{test_table} (id integer NOT NULL, value integer NOT NULL DEFAULT 0); + + INSERT INTO #{test_table} (id, value) + VALUES (1, 1), (2, 2), (3, 3) + SQL + + lock_writes_manager.lock_writes + end + + it 'disables the write-lock trigger function' do + expect do + model.update_column_in_batches(test_table, :value, Arel.sql('1+1'), disable_lock_writes: true) + end.not_to raise_error + end + + it 'raises an error if it does not disable the trigger function' do + expect do + model.update_column_in_batches(test_table, :value, Arel.sql('1+1'), disable_lock_writes: false) + end.to raise_error(ActiveRecord::StatementInvalid, /Table: "#{test_table}" is write protected/) + end + end end context 'when running inside the transaction' do @@ -1080,6 +1114,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end it 'renames a column concurrently' do + expect(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection).to receive(:with_suppressed).and_yield + expect(model).to receive(:check_trigger_permissions!).with(:users) expect(model).to receive(:install_rename_triggers) @@ -1112,6 +1148,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do let(:connection) { ActiveRecord::Migration.connection } before do + expect(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection).to receive(:with_suppressed).and_yield expect(Gitlab::Database::UnidirectionalCopyTrigger).to receive(:on_table) .with(:users, connection: connection).and_return(copy_trigger) end @@ -1119,6 +1156,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do it 'copies the value to the new column using the type_cast_function', :aggregate_failures do expect(model).to receive(:copy_indexes).with(:users, :id, :new) expect(model).to receive(:add_not_null_constraint).with(:users, :new) + expect(model).to receive(:execute).with("SELECT set_config('lock_writes.users', 'false', true)") expect(model).to receive(:execute).with("UPDATE \"users\" SET \"new\" = cast_to_jsonb_with_default(\"users\".\"id\") WHERE \"users\".\"id\" >= #{user.id}") expect(copy_trigger).to receive(:create).with(:id, :new, trigger_name: nil) @@ -1165,6 +1203,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end it 'copies the default to the new column' do + expect(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection).to receive(:with_suppressed).and_yield + expect(model).to receive(:change_column_default) .with(:users, :new, old_column.default) @@ -1176,6 +1216,34 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end end + context 'when the table in the other database is write-locked' do + let(:test_table) { '_test_table' } + let(:lock_writes_manager) do + Gitlab::Database::LockWritesManager.new( + table_name: test_table, + connection: model.connection, + database_name: 'main' + ) + end + + before do + model.connection.execute(<<~SQL) + CREATE TABLE #{test_table} (id integer NOT NULL, value integer NOT NULL DEFAULT 0); + + INSERT INTO #{test_table} (id, value) + VALUES (1, 1), (2, 2), (3, 3) + SQL + + lock_writes_manager.lock_writes + end + + it 'does not raise an error when renaming the column' do + expect do + model.rename_column_concurrently(test_table, :value, :new_value) + end.not_to raise_error + end + end + context 'when the column to be renamed does not exist' do before do allow(model).to receive(:columns).and_return([]) @@ -1246,6 +1314,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end it 'reverses the operations of cleanup_concurrent_column_rename' do + expect(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection).to receive(:with_suppressed).and_yield + expect(model).to receive(:check_trigger_permissions!).with(:users) expect(model).to receive(:install_rename_triggers) @@ -1302,6 +1372,8 @@ RSpec.describe Gitlab::Database::MigrationHelpers do end it 'copies the default to the old column' do + expect(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection).to receive(:with_suppressed).and_yield + expect(model).to receive(:change_column_default) .with(:users, :old, new_column.default) @@ -2438,7 +2510,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do let(:env) { { 'DISABLE_LOCK_RETRIES' => 'true' } } it 'sets the migration class name in the logs' do - model.with_lock_retries(env: env, logger: in_memory_logger) { } + model.with_lock_retries(env: env, logger: in_memory_logger) {} buffer.rewind expect(buffer.read).to include("\"class\":\"#{model.class}\"") @@ -2452,7 +2524,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(Gitlab::Database::WithLockRetries).to receive(:new).and_return(with_lock_retries) expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: raise_on_exhaustion) - model.with_lock_retries(env: env, logger: in_memory_logger, raise_on_exhaustion: raise_on_exhaustion) { } + model.with_lock_retries(env: env, logger: in_memory_logger, raise_on_exhaustion: raise_on_exhaustion) {} end end @@ -2461,7 +2533,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(Gitlab::Database::WithLockRetries).to receive(:new).and_return(with_lock_retries) expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false) - model.with_lock_retries(env: env, logger: in_memory_logger) { } + model.with_lock_retries(env: env, logger: in_memory_logger) {} end it 'defaults to allowing subtransactions' do @@ -2470,7 +2542,7 @@ RSpec.describe Gitlab::Database::MigrationHelpers do expect(Gitlab::Database::WithLockRetries).to receive(:new).with(hash_including(allow_savepoints: true)).and_return(with_lock_retries) expect(with_lock_retries).to receive(:run).with(raise_on_exhaustion: false) - model.with_lock_retries(env: env, logger: in_memory_logger) { } + model.with_lock_retries(env: env, logger: in_memory_logger) {} end end diff --git a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb index c423340a572..f21f1ac5e52 100644 --- a/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/background_migration_helpers_spec.rb @@ -37,12 +37,6 @@ RSpec.describe Gitlab::Database::Migrations::BackgroundMigrationHelpers do freeze_time { example.run } end - before do - User.class_eval do - include EachBatch - end - end - it 'returns the final expected delay' do Sidekiq::Testing.fake! do final_delay = model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, batch_size: 2) diff --git a/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb b/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb index 5bfb2516ba1..a2f6e6b43ed 100644 --- a/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migrations/batched_background_migration_helpers_spec.rb @@ -15,12 +15,25 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d describe '#queue_batched_background_migration' do let(:pgclass_info) { instance_double('Gitlab::Database::PgClass', cardinality_estimate: 42) } + let(:job_class) do + Class.new(Gitlab::BackgroundMigration::BatchedMigrationJob) do + def self.name + 'MyJobClass' + end + end + end before do allow(Gitlab::Database::PgClass).to receive(:for_table).and_call_original expect(Gitlab::Database::QueryAnalyzers::RestrictAllowedSchemas).to receive(:require_dml_mode!) allow(migration).to receive(:transaction_open?).and_return(false) + + stub_const("Gitlab::Database::BackgroundMigration::BatchedMigration::JOB_CLASS_MODULE", '') + allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigration) do |batched_migration| + allow(batched_migration).to receive(:job_class) + .and_return(job_class) + end end context 'when such migration already exists' do @@ -42,7 +55,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d expect do migration.queue_batched_background_migration( - 'MyJobClass', + job_class.name, :projects, :id, [:id], [:id_convert_to_bigint], @@ -62,7 +75,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d expect do migration.queue_batched_background_migration( - 'MyJobClass', + job_class.name, :projects, :id, job_interval: 5.minutes, @@ -97,7 +110,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d it 'sets the job interval to the minimum value' do expect do - migration.queue_batched_background_migration('MyJobClass', :events, :id, job_interval: minimum_delay - 1.minute) + migration.queue_batched_background_migration(job_class.name, :events, :id, job_interval: minimum_delay - 1.minute) end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) created_migration = Gitlab::Database::BackgroundMigration::BatchedMigration.last @@ -107,26 +120,76 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d end context 'when additional arguments are passed to the method' do - it 'saves the arguments on the database record' do - expect do - migration.queue_batched_background_migration( - 'MyJobClass', - :projects, - :id, - 'my', - 'arguments', - job_interval: 5.minutes, - batch_max_value: 1000) - end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) + context 'when the job class provides job_arguments_count' do + context 'when defined job arguments for the job class does not match provided arguments' do + it 'raises an error' do + expect do + migration.queue_batched_background_migration( + job_class.name, + :projects, + :id, + 'my', + 'arguments', + job_interval: 2.minutes) + end.to raise_error(RuntimeError, /Wrong number of job arguments for MyJobClass \(given 2, expected 0\)/) + end + end - expect(Gitlab::Database::BackgroundMigration::BatchedMigration.last).to have_attributes( - job_class_name: 'MyJobClass', - table_name: 'projects', - column_name: 'id', - interval: 300, - min_value: 1, - max_value: 1000, - job_arguments: %w[my arguments]) + context 'when defined job arguments for the job class match provided arguments' do + let(:job_class) do + Class.new(Gitlab::BackgroundMigration::BatchedMigrationJob) do + def self.name + 'MyJobClass' + end + + job_arguments :foo, :bar + end + end + + it 'saves the arguments on the database record' do + expect do + migration.queue_batched_background_migration( + job_class.name, + :projects, + :id, + 'my', + 'arguments', + job_interval: 5.minutes, + batch_max_value: 1000) + end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) + + expect(Gitlab::Database::BackgroundMigration::BatchedMigration.last).to have_attributes( + job_class_name: 'MyJobClass', + table_name: 'projects', + column_name: 'id', + interval: 300, + min_value: 1, + max_value: 1000, + job_arguments: %w[my arguments]) + end + end + end + + context 'when the job class does not provide job_arguments_count' do + let(:job_class) do + Class.new do + def self.name + 'MyJobClass' + end + end + end + + it 'does not raise an error' do + expect do + migration.queue_batched_background_migration( + job_class.name, + :projects, + :id, + 'my', + 'arguments', + job_interval: 2.minutes) + end.not_to raise_error + end end end @@ -138,7 +201,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d it 'creates the record with the current max value' do expect do - migration.queue_batched_background_migration('MyJobClass', :events, :id, job_interval: 5.minutes) + migration.queue_batched_background_migration(job_class.name, :events, :id, job_interval: 5.minutes) end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) created_migration = Gitlab::Database::BackgroundMigration::BatchedMigration.last @@ -148,7 +211,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d it 'creates the record with an active status' do expect do - migration.queue_batched_background_migration('MyJobClass', :events, :id, job_interval: 5.minutes) + migration.queue_batched_background_migration(job_class.name, :events, :id, job_interval: 5.minutes) end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) expect(Gitlab::Database::BackgroundMigration::BatchedMigration.last).to be_active @@ -158,7 +221,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d context 'when the database is empty' do it 'sets the max value to the min value' do expect do - migration.queue_batched_background_migration('MyJobClass', :events, :id, job_interval: 5.minutes) + migration.queue_batched_background_migration(job_class.name, :events, :id, job_interval: 5.minutes) end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) created_migration = Gitlab::Database::BackgroundMigration::BatchedMigration.last @@ -168,7 +231,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d it 'creates the record with a finished status' do expect do - migration.queue_batched_background_migration('MyJobClass', :projects, :id, job_interval: 5.minutes) + migration.queue_batched_background_migration(job_class.name, :projects, :id, job_interval: 5.minutes) end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) expect(Gitlab::Database::BackgroundMigration::BatchedMigration.last).to be_finished @@ -181,7 +244,7 @@ RSpec.describe Gitlab::Database::Migrations::BatchedBackgroundMigrationHelpers d expect(migration).to receive(:gitlab_schema_from_context).and_return(:gitlab_ci) expect do - migration.queue_batched_background_migration('MyJobClass', :events, :id, job_interval: 5.minutes) + migration.queue_batched_background_migration(job_class.name, :events, :id, job_interval: 5.minutes) end.to change { Gitlab::Database::BackgroundMigration::BatchedMigration.count }.by(1) created_migration = Gitlab::Database::BackgroundMigration::BatchedMigration.last diff --git a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb index c31244060ec..3540a120b8f 100644 --- a/spec/lib/gitlab/database/migrations/instrumentation_spec.rb +++ b/spec/lib/gitlab/database/migrations/instrumentation_spec.rb @@ -122,7 +122,11 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do it 'records observations for all migrations' do subject.observe(version: migration_version, name: migration_name, connection: connection) {} - subject.observe(version: migration_version_2, name: migration_name_2, connection: connection) { raise 'something went wrong' } rescue nil + begin + subject.observe(version: migration_version_2, name: migration_name_2, connection: connection) { raise 'something went wrong' } + rescue StandardError + nil + end expect { load_observation(result_dir, migration_name) }.not_to raise_error expect { load_observation(result_dir, migration_name_2) }.not_to raise_error diff --git a/spec/lib/gitlab/database/migrations/lock_retry_mixin_spec.rb b/spec/lib/gitlab/database/migrations/lock_retry_mixin_spec.rb index 50ad77caaf1..6092d985ce8 100644 --- a/spec/lib/gitlab/database/migrations/lock_retry_mixin_spec.rb +++ b/spec/lib/gitlab/database/migrations/lock_retry_mixin_spec.rb @@ -83,10 +83,10 @@ RSpec.describe Gitlab::Database::Migrations::LockRetryMixin do context 'with transactions disabled' do let(:migration) { double('migration', enable_lock_retries?: false) } - let(:receiver) { double('receiver', use_transaction?: false)} + let(:receiver) { double('receiver', use_transaction?: false) } it 'calls super method' do - p = proc { } + p = proc {} expect(receiver).to receive(:ddl_transaction).with(migration, &p) @@ -95,11 +95,11 @@ RSpec.describe Gitlab::Database::Migrations::LockRetryMixin do end context 'with transactions enabled, but lock retries disabled' do - let(:receiver) { double('receiver', use_transaction?: true)} + let(:receiver) { double('receiver', use_transaction?: true) } let(:migration) { double('migration', enable_lock_retries?: false) } it 'calls super method' do - p = proc { } + p = proc {} expect(receiver).to receive(:ddl_transaction).with(migration, &p) @@ -108,12 +108,12 @@ RSpec.describe Gitlab::Database::Migrations::LockRetryMixin do end context 'with transactions enabled and lock retries enabled' do - let(:receiver) { double('receiver', use_transaction?: true)} + let(:receiver) { double('receiver', use_transaction?: true) } let(:migration) { double('migration', migration_connection: connection, enable_lock_retries?: true) } let(:connection) { ActiveRecord::Base.connection } it 'calls super method' do - p = proc { } + p = proc {} expect(receiver).not_to receive(:ddl_transaction) expect_next_instance_of(Gitlab::Database::WithLockRetries) do |retries| diff --git a/spec/lib/gitlab/database/migrations/runner_spec.rb b/spec/lib/gitlab/database/migrations/runner_spec.rb index e7f68e3e4a8..a37247ba0c6 100644 --- a/spec/lib/gitlab/database/migrations/runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/runner_spec.rb @@ -41,7 +41,7 @@ RSpec.describe Gitlab::Database::Migrations::Runner do allow(described_class).to receive(:migration_context).and_return(ctx) - names_this_branch = (applied_migrations_this_branch + pending_migrations).map { |m| "db/migrate/#{m.version}_#{m.name}.rb"} + names_this_branch = (applied_migrations_this_branch + pending_migrations).map { |m| "db/migrate/#{m.version}_#{m.name}.rb" } allow(described_class).to receive(:migration_file_names_this_branch).and_return(names_this_branch) end diff --git a/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb b/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb index f1f72d71e1a..9451a6bd34a 100644 --- a/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb +++ b/spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb @@ -18,7 +18,7 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez let(:connection) { ApplicationRecord.connection } - let(:table_name) { "_test_column_copying"} + let(:table_name) { "_test_column_copying" } before do connection.execute(<<~SQL) @@ -50,18 +50,16 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez context 'with jobs to run' do let(:migration_name) { 'TestBackgroundMigration' } - before do - migration.queue_batched_background_migration( - migration_name, table_name, :id, job_interval: 5.minutes, batch_size: 100 - ) - end - it 'samples jobs' do calls = [] define_background_migration(migration_name) do |*args| calls << args end + migration.queue_batched_background_migration(migration_name, table_name, :id, + job_interval: 5.minutes, + batch_size: 100) + described_class.new(result_dir: result_dir, connection: connection).run_jobs(for_duration: 3.minutes) expect(calls.count).to eq(10) # 1000 rows / batch size 100 = 10 @@ -70,6 +68,9 @@ RSpec.describe Gitlab::Database::Migrations::TestBatchedBackgroundRunner, :freez context 'with multiple jobs to run' do it 'runs all jobs created within the last 3 hours' do old_migration = define_background_migration(migration_name) + migration.queue_batched_background_migration(migration_name, table_name, :id, + job_interval: 5.minutes, + batch_size: 100) travel 4.hours 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 d8b06ee1a5d..04b9fba5b2f 100644 --- a/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb +++ b/spec/lib/gitlab/database/partitioning/sliding_list_strategy_spec.rb @@ -48,61 +48,43 @@ RSpec.describe Gitlab::Database::Partitioning::SlidingListStrategy do end describe '#validate_and_fix' do - context 'feature flag is disabled' do - before do - stub_feature_flags(fix_sliding_list_partitioning: false) - end + it 'does not call change_column_default if the partitioning in a valid state' do + expect(strategy.model.connection).not_to receive(:change_column_default) - it 'does not try to fix the default partition value' do - connection.change_column_default(model.table_name, strategy.partitioning_key, 3) - expect(strategy.model.connection).not_to receive(:change_column_default) - strategy.validate_and_fix - end + strategy.validate_and_fix end - context 'feature flag is enabled' do - before do - stub_feature_flags(fix_sliding_list_partitioning: true) - end - - it 'does not call change_column_default if the partitioning in a valid state' do - expect(strategy.model.connection).not_to receive(:change_column_default) - - strategy.validate_and_fix - end - - it 'calls change_column_default on partition_key with the most default partition number' do - connection.change_column_default(model.table_name, strategy.partitioning_key, 1) + it 'calls change_column_default on partition_key with the most default partition number' do + connection.change_column_default(model.table_name, strategy.partitioning_key, 1) - expect(Gitlab::AppLogger).to receive(:warn).with( - message: 'Fixed default value of sliding_list_strategy partitioning_key', - connection_name: 'main', - old_value: 1, - new_value: 2, - table_name: table_name, - column: strategy.partitioning_key - ) + expect(Gitlab::AppLogger).to receive(:warn).with( + message: 'Fixed default value of sliding_list_strategy partitioning_key', + connection_name: 'main', + old_value: 1, + new_value: 2, + table_name: table_name, + column: strategy.partitioning_key + ) - expect(strategy.model.connection).to receive(:change_column_default).with( - model.table_name, strategy.partitioning_key, 2 - ).and_call_original + expect(strategy.model.connection).to receive(:change_column_default).with( + model.table_name, strategy.partitioning_key, 2 + ).and_call_original - strategy.validate_and_fix - end + strategy.validate_and_fix + end - it 'does not change the default column if it has been changed in the meanwhile by another process' do - expect(strategy).to receive(:current_default_value).and_return(1, 2) + it 'does not change the default column if it has been changed in the meanwhile by another process' do + expect(strategy).to receive(:current_default_value).and_return(1, 2) - expect(strategy.model.connection).not_to receive(:change_column_default) + expect(strategy.model.connection).not_to receive(:change_column_default) - expect(Gitlab::AppLogger).to receive(:warn).with( - message: 'Table partitions or partition key default value have been changed by another process', - table_name: table_name, - default_value: 2 - ) + expect(Gitlab::AppLogger).to receive(:warn).with( + message: 'Table partitions or partition key default value have been changed by another process', + table_name: table_name, + default_value: 2 + ) - strategy.validate_and_fix - end + strategy.validate_and_fix end end diff --git a/spec/lib/gitlab/database/partitioning_spec.rb b/spec/lib/gitlab/database/partitioning_spec.rb index 7c69f639aab..36c8b0811fe 100644 --- a/spec/lib/gitlab/database/partitioning_spec.rb +++ b/spec/lib/gitlab/database/partitioning_spec.rb @@ -89,7 +89,7 @@ RSpec.describe Gitlab::Database::Partitioning do end it 'manages partitions for each given model' do - expect { described_class.sync_partitions(models)} + expect { described_class.sync_partitions(models) } .to change { find_partitions(table_names.first).size }.from(0) .and change { find_partitions(table_names.last).size }.from(0) end diff --git a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb index 5e8afc0102e..ddf5793049d 100644 --- a/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb +++ b/spec/lib/gitlab/database/query_analyzers/gitlab_schemas_validate_connection_spec.rb @@ -5,6 +5,13 @@ require 'spec_helper' RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection, query_analyzers: false do let(:analyzer) { described_class } + # We keep only the GitlabSchemasValidateConnection analyzer running + around do |example| + Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection.with_suppressed(false) do + example.run + end + end + context 'properly observes all queries', :request_store do using RSpec::Parameterized::TableSyntax @@ -61,6 +68,24 @@ RSpec.describe Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection end end + context "when analyzer is enabled for tests", :query_analyzers do + before do + skip_if_multiple_databases_not_setup + end + + it "throws an error when trying to access a table that belongs to the gitlab_main schema from the ci database" do + expect do + Ci::ApplicationRecord.connection.execute("select * from users limit 1") + end.to raise_error(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection::CrossSchemaAccessError) + end + + it "throws an error when trying to access a table that belongs to the gitlab_ci schema from the main database" do + expect do + ApplicationRecord.connection.execute("select * from ci_builds limit 1") + end.to raise_error(Gitlab::Database::QueryAnalyzers::GitlabSchemasValidateConnection::CrossSchemaAccessError) + end + end + def process_sql(model, sql) Gitlab::Database::QueryAnalyzer.instance.within([analyzer]) do # Skip load balancer and retrieve connection assigned to model diff --git a/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb index 34670696787..1bccdda3be1 100644 --- a/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb +++ b/spec/lib/gitlab/database/reindexing/grafana_notifier_spec.rb @@ -6,7 +6,7 @@ RSpec.describe Gitlab::Database::Reindexing::GrafanaNotifier do include Database::DatabaseHelpers let(:api_key) { "foo" } - let(:api_url) { "http://bar"} + let(:api_url) { "http://bar" } let(:additional_tag) { "some-tag" } let(:action) { create(:reindex_action) } diff --git a/spec/lib/gitlab/database/reindexing_spec.rb b/spec/lib/gitlab/database/reindexing_spec.rb index 976b9896dfa..495e953f993 100644 --- a/spec/lib/gitlab/database/reindexing_spec.rb +++ b/spec/lib/gitlab/database/reindexing_spec.rb @@ -46,6 +46,27 @@ RSpec.describe Gitlab::Database::Reindexing do end end + context 'when async index destruction is enabled' do + it 'executes async index destruction prior to any reindexing actions' do + stub_feature_flags(database_async_index_destruction: true) + + expect(Gitlab::Database::AsyncIndexes).to receive(:drop_pending_indexes!).ordered.exactly(databases_count).times + expect(described_class).to receive(:automatic_reindexing).ordered.exactly(databases_count).times + + described_class.invoke + end + end + + context 'when async index destruction is disabled' do + it 'does not execute async index destruction' do + stub_feature_flags(database_async_index_destruction: false) + + expect(Gitlab::Database::AsyncIndexes).not_to receive(:drop_pending_indexes!) + + described_class.invoke + end + end + context 'calls automatic reindexing' do it 'uses all candidate indexes' do expect(described_class).to receive(:automatic_reindexing).exactly(databases_count).times diff --git a/spec/lib/gitlab/database/shared_model_spec.rb b/spec/lib/gitlab/database/shared_model_spec.rb index c88edc17817..7e0ba3397d1 100644 --- a/spec/lib/gitlab/database/shared_model_spec.rb +++ b/spec/lib/gitlab/database/shared_model_spec.rb @@ -106,7 +106,7 @@ RSpec.describe Gitlab::Database::SharedModel do shared_model = shared_model_class.new - expect(shared_model.connection_db_config). to eq(described_class.connection_db_config) + expect(shared_model.connection_db_config).to eq(described_class.connection_db_config) end end end diff --git a/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb b/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb index 6c32fb3ca17..836332524a9 100644 --- a/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_outside_transaction_spec.rb @@ -232,14 +232,14 @@ RSpec.describe Gitlab::Database::WithLockRetriesOutsideTransaction do expect(connection).to receive(:execute).with('RESET idle_in_transaction_session_timeout; RESET lock_timeout').and_call_original expect(connection).to receive(:execute).with("SET lock_timeout TO '15ms'").and_call_original - subject.run { } + subject.run {} end it 'calls `sleep` after the first iteration fails, using the configured sleep time' do expect(subject).to receive(:run_block_with_lock_timeout).and_raise(ActiveRecord::LockWaitTimeout).twice expect(subject).to receive(:sleep).with(0.025) - subject.run { } + subject.run {} end end end diff --git a/spec/lib/gitlab/database/with_lock_retries_spec.rb b/spec/lib/gitlab/database/with_lock_retries_spec.rb index 6b35ccafabc..797a01c482d 100644 --- a/spec/lib/gitlab/database/with_lock_retries_spec.rb +++ b/spec/lib/gitlab/database/with_lock_retries_spec.rb @@ -248,14 +248,14 @@ RSpec.describe Gitlab::Database::WithLockRetries do expect(connection).to receive(:execute).with("SET LOCAL lock_timeout TO '15ms'").and_call_original expect(connection).to receive(:execute).with("RELEASE SAVEPOINT active_record_1", "TRANSACTION").and_call_original - subject.run { } + subject.run {} end it 'calls `sleep` after the first iteration fails, using the configured sleep time' do expect(subject).to receive(:run_block_with_lock_timeout).and_raise(ActiveRecord::LockWaitTimeout).twice expect(subject).to receive(:sleep).with(0.025) - subject.run { } + subject.run {} end end @@ -265,13 +265,13 @@ RSpec.describe Gitlab::Database::WithLockRetries do it 'prevents running inside already open transaction' do allow(connection).to receive(:transaction_open?).and_return(true) - expect { subject.run { } }.to raise_error(/should not run inside already open transaction/) + expect { subject.run {} }.to raise_error(/should not run inside already open transaction/) end it 'does not raise the error if not inside open transaction' do allow(connection).to receive(:transaction_open?).and_return(false) - expect { subject.run { } }.not_to raise_error + expect { subject.run {} }.not_to raise_error end end end |