diff options
Diffstat (limited to 'spec/lib/gitlab/database/background_migration')
5 files changed, 495 insertions, 30 deletions
diff --git a/spec/lib/gitlab/database/background_migration/batch_optimizer_spec.rb b/spec/lib/gitlab/database/background_migration/batch_optimizer_spec.rb new file mode 100644 index 00000000000..95863ce3765 --- /dev/null +++ b/spec/lib/gitlab/database/background_migration/batch_optimizer_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Database::BackgroundMigration::BatchOptimizer do + describe '#optimize' do + subject { described_class.new(migration, number_of_jobs: number_of_jobs, ema_alpha: ema_alpha).optimize! } + + let(:migration) { create(:batched_background_migration, batch_size: batch_size, sub_batch_size: 100, interval: 120) } + + let(:batch_size) { 10_000 } + + let_it_be(:number_of_jobs) { 5 } + let_it_be(:ema_alpha) { 0.4 } + + let_it_be(:target_efficiency) { described_class::TARGET_EFFICIENCY.max } + + def mock_efficiency(eff) + expect(migration).to receive(:smoothed_time_efficiency).with(number_of_jobs: number_of_jobs, alpha: ema_alpha).and_return(eff) + end + + it 'with unknown time efficiency, it keeps the batch size' do + mock_efficiency(nil) + + expect { subject }.not_to change { migration.reload.batch_size } + end + + it 'with a time efficiency of 95%, it keeps the batch size' do + mock_efficiency(0.95) + + expect { subject }.not_to change { migration.reload.batch_size } + end + + it 'with a time efficiency of 90%, it keeps the batch size' do + mock_efficiency(0.9) + + expect { subject }.not_to change { migration.reload.batch_size } + end + + it 'with a time efficiency of 85%, it increases the batch size' do + time_efficiency = 0.85 + + mock_efficiency(time_efficiency) + + new_batch_size = ((target_efficiency / time_efficiency) * batch_size).to_i + + expect { subject }.to change { migration.reload.batch_size }.from(batch_size).to(new_batch_size) + end + + it 'with a time efficiency of 110%, it decreases the batch size' do + time_efficiency = 1.1 + + mock_efficiency(time_efficiency) + + new_batch_size = ((target_efficiency / time_efficiency) * batch_size).to_i + + expect { subject }.to change { migration.reload.batch_size }.from(batch_size).to(new_batch_size) + end + + context 'reaching the upper limit for an increase' do + it 'caps the batch size multiplier at 20% when increasing' do + time_efficiency = 0.1 # this would result in a factor of 10 if not limited + + mock_efficiency(time_efficiency) + + new_batch_size = (1.2 * batch_size).to_i + + expect { subject }.to change { migration.reload.batch_size }.from(batch_size).to(new_batch_size) + end + + it 'does not limit the decrease multiplier' do + time_efficiency = 10 + + mock_efficiency(time_efficiency) + + new_batch_size = (0.1 * batch_size).to_i + + expect { subject }.to change { migration.reload.batch_size }.from(batch_size).to(new_batch_size) + end + end + + context 'reaching the upper limit for the batch size' do + let(:batch_size) { 1_950_000 } + + it 'caps the batch size at 10M' do + mock_efficiency(0.7) + + expect { subject }.to change { migration.reload.batch_size }.to(2_000_000) + end + end + + context 'reaching the lower limit for the batch size' do + let(:batch_size) { 1_050 } + + it 'caps the batch size at 1k' do + mock_efficiency(1.1) + + expect { subject }.to change { migration.reload.batch_size }.to(1_000) + end + 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 1020aafcf08..78e0b7627e9 100644 --- a/spec/lib/gitlab/database/background_migration/batched_job_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_job_spec.rb @@ -9,6 +9,42 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedJob, type: :model d it { is_expected.to belong_to(:batched_migration).with_foreign_key(:batched_background_migration_id) } end + describe 'scopes' do + let_it_be(:fixed_time) { Time.new(2021, 04, 27, 10, 00, 00, 00) } + + let_it_be(:pending_job) { create(:batched_background_migration_job, status: :pending, updated_at: fixed_time) } + let_it_be(:running_job) { create(:batched_background_migration_job, status: :running, updated_at: fixed_time) } + let_it_be(:stuck_job) { create(:batched_background_migration_job, status: :pending, updated_at: fixed_time - described_class::STUCK_JOBS_TIMEOUT) } + let_it_be(:failed_job) { create(:batched_background_migration_job, status: :failed, attempts: 1) } + + before_all do + create(:batched_background_migration_job, status: :failed, attempts: described_class::MAX_ATTEMPTS) + create(:batched_background_migration_job, status: :succeeded) + end + + before do + travel_to fixed_time + end + + describe '.active' do + it 'returns active jobs' do + expect(described_class.active).to contain_exactly(pending_job, running_job, stuck_job) + end + end + + describe '.stuck' do + it 'returns stuck jobs' do + expect(described_class.stuck).to contain_exactly(stuck_job) + end + end + + describe '.retriable' do + it 'returns retriable jobs' do + expect(described_class.retriable).to contain_exactly(failed_job, stuck_job) + end + end + end + describe 'delegated batched_migration attributes' do let(:batched_job) { build(:batched_background_migration_job) } let(:batched_migration) { batched_job.batched_migration } @@ -47,4 +83,55 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedJob, type: :model d end end end + + describe '#time_efficiency' do + subject { job.time_efficiency } + + let(:migration) { build(:batched_background_migration, interval: 120.seconds) } + let(:job) { build(:batched_background_migration_job, status: :succeeded, batched_migration: migration) } + + context 'when job has not yet succeeded' do + let(:job) { build(:batched_background_migration_job, status: :running) } + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'when finished_at is not set' do + it 'returns nil' do + job.started_at = Time.zone.now + + expect(subject).to be_nil + end + end + + context 'when started_at is not set' do + it 'returns nil' do + job.finished_at = Time.zone.now + + expect(subject).to be_nil + end + end + + context 'when job has finished' do + it 'returns ratio of duration to interval, here: 0.5' do + freeze_time do + job.started_at = Time.zone.now - migration.interval / 2 + job.finished_at = Time.zone.now + + expect(subject).to eq(0.5) + end + end + + it 'returns ratio of duration to interval, here: 1' do + freeze_time do + job.started_at = Time.zone.now - migration.interval + job.finished_at = Time.zone.now + + expect(subject).to eq(1) + end + end + end + 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 7d0e10b62c6..9f0493ab0d7 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 @@ -17,9 +17,9 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end it 'marks the migration as finished' do - relation = Gitlab::Database::BackgroundMigration::BatchedMigration.finished.where(id: migration.id) + runner.run_migration_job(migration) - expect { runner.run_migration_job(migration) }.to change { relation.count }.by(1) + expect(migration.reload).to be_finished end end @@ -50,6 +50,15 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do batch_size: migration.batch_size, sub_batch_size: migration.sub_batch_size) end + + it 'optimizes the migration after executing the job' do + migration.update!(min_value: event1.id, max_value: event2.id) + + expect(migration_wrapper).to receive(:perform).ordered + expect(migration).to receive(:optimize!).ordered + + runner.run_migration_job(migration) + end end context 'when the batch maximum exceeds the migration maximum' do @@ -83,7 +92,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do let!(:event3) { create(:event) } let!(:migration) do - create(:batched_background_migration, :active, batch_size: 2, min_value: event1.id, max_value: event3.id) + create(:batched_background_migration, :active, batch_size: 2, min_value: event1.id, max_value: event2.id) end let!(:previous_job) do @@ -92,14 +101,24 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do min_value: event1.id, max_value: event2.id, batch_size: 2, - sub_batch_size: 1) + sub_batch_size: 1, + status: :succeeded + ) end let(:job_relation) do Gitlab::Database::BackgroundMigration::BatchedJob.where(batched_background_migration_id: migration.id) end + context 'when the migration has no batches remaining' do + it_behaves_like 'it has completed the migration' + end + context 'when the migration has batches to process' do + before do + migration.update!(max_value: event3.id) + end + it 'runs the migration job for the next batch' do expect(migration_wrapper).to receive(:perform) do |job_record| expect(job_record).to eq(job_relation.last) @@ -123,17 +142,82 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do end end - context 'when the migration has no batches remaining' do + context 'when migration has failed jobs' do before do - create(:batched_background_migration_job, - batched_migration: migration, - min_value: event3.id, - max_value: event3.id, - batch_size: 2, - sub_batch_size: 1) + previous_job.update!(status: :failed) end - it_behaves_like 'it has completed the migration' + it 'retries the failed job' do + expect(migration_wrapper).to receive(:perform) do |job_record| + expect(job_record).to eq(previous_job) + end + + expect { runner.run_migration_job(migration) }.to change { job_relation.count }.by(0) + end + + context 'when failed job has reached the maximum number of attempts' do + before do + previous_job.update!(attempts: Gitlab::Database::BackgroundMigration::BatchedJob::MAX_ATTEMPTS) + end + + it 'marks the migration as failed' do + expect(migration_wrapper).not_to receive(:perform) + + expect { runner.run_migration_job(migration) }.to change { job_relation.count }.by(0) + + expect(migration).to be_failed + end + end + end + + context 'when migration has stuck jobs' do + before do + previous_job.update!(status: :running, updated_at: 1.hour.ago - Gitlab::Database::BackgroundMigration::BatchedJob::STUCK_JOBS_TIMEOUT) + end + + it 'retries the stuck job' do + expect(migration_wrapper).to receive(:perform) do |job_record| + expect(job_record).to eq(previous_job) + end + + expect { runner.run_migration_job(migration.reload) }.to change { job_relation.count }.by(0) + end + end + + context 'when migration has possible stuck jobs' do + before do + previous_job.update!(status: :running, updated_at: 1.hour.from_now - Gitlab::Database::BackgroundMigration::BatchedJob::STUCK_JOBS_TIMEOUT) + end + + it 'keeps the migration active' do + expect(migration_wrapper).not_to receive(:perform) + + expect { runner.run_migration_job(migration) }.to change { job_relation.count }.by(0) + + expect(migration.reload).to be_active + end + end + + context 'when the migration has batches to process and failed jobs' do + before do + migration.update!(max_value: event3.id) + previous_job.update!(status: :failed) + end + + it 'runs next batch then retries the failed job' do + expect(migration_wrapper).to receive(:perform) do |job_record| + expect(job_record).to eq(job_relation.last) + job_record.update!(status: :succeeded) + end + + expect { runner.run_migration_job(migration) }.to change { job_relation.count }.by(1) + + expect(migration_wrapper).to receive(:perform) do |job_record| + expect(job_record).to eq(previous_job) + end + + expect { runner.run_migration_job(migration.reload) }.to change { job_relation.count }.by(0) + end end end end @@ -180,10 +264,12 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationRunner do it 'runs all jobs inline until finishing the migration' do expect(migration_wrapper).to receive(:perform) do |job_record| expect(job_record).to eq(job_relation.first) + job_record.update!(status: :succeeded) end expect(migration_wrapper).to receive(:perform) do |job_record| expect(job_record).to eq(job_relation.last) + job_record.update!(status: :succeeded) end expect { runner.run_entire_migration(migration) }.to change { job_relation.count }.by(2) 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 261e23d0745..43e34325419 100644 --- a/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb +++ b/spec/lib/gitlab/database/background_migration/batched_migration_spec.rb @@ -119,7 +119,13 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m end describe '#create_batched_job!' do - let(:batched_migration) { create(:batched_background_migration) } + let(:batched_migration) do + create(:batched_background_migration, + batch_size: 999, + sub_batch_size: 99, + pause_ms: 250 + ) + end it 'creates a batched_job with the correct batch configuration' do batched_job = batched_migration.create_batched_job!(1, 5) @@ -128,7 +134,9 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m min_value: 1, max_value: 5, batch_size: batched_migration.batch_size, - sub_batch_size: batched_migration.sub_batch_size) + sub_batch_size: batched_migration.sub_batch_size, + pause_ms: 250 + ) end end @@ -196,6 +204,22 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m it_behaves_like 'an attr_writer that demodulizes assigned class names', :batch_class_name end + describe '#migrated_tuple_count' do + subject { batched_migration.migrated_tuple_count } + + let(:batched_migration) { create(:batched_background_migration) } + + before do + create_list(:batched_background_migration_job, 5, status: :succeeded, batch_size: 1_000, batched_migration: batched_migration) + create_list(:batched_background_migration_job, 1, status: :running, batch_size: 1_000, batched_migration: batched_migration) + create_list(:batched_background_migration_job, 1, status: :failed, batch_size: 1_000, batched_migration: batched_migration) + end + + it 'sums the batch_size of succeeded jobs' do + expect(subject).to eq(5_000) + end + end + describe '#prometheus_labels' do let(:batched_migration) { create(:batched_background_migration, job_class_name: 'TestMigration', table_name: 'foo', column_name: 'bar') } @@ -208,4 +232,96 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m expect(batched_migration.prometheus_labels).to eq(labels) end end + + describe '#smoothed_time_efficiency' do + let(:migration) { create(:batched_background_migration, interval: 120.seconds) } + let(:end_time) { Time.zone.now } + + around do |example| + freeze_time do + example.run + end + end + + let(:common_attrs) do + { + status: :succeeded, + batched_migration: migration, + finished_at: end_time + } + end + + context 'when there are not enough jobs' do + subject { migration.smoothed_time_efficiency(number_of_jobs: 10) } + + it 'returns nil' do + create_list(:batched_background_migration_job, 9, **common_attrs) + + expect(subject).to be_nil + end + end + + context 'when there are enough jobs' do + subject { migration.smoothed_time_efficiency(number_of_jobs: number_of_jobs) } + + let!(:jobs) { create_list(:batched_background_migration_job, number_of_jobs, **common_attrs.merge(batched_migration: migration)) } + let(:number_of_jobs) { 10 } + + before do + expect(migration).to receive_message_chain(:batched_jobs, :successful_in_execution_order, :reverse_order, :limit).with(no_args).with(no_args).with(number_of_jobs).and_return(jobs) + end + + def mock_efficiencies(*effs) + effs.each_with_index do |eff, i| + expect(jobs[i]).to receive(:time_efficiency).and_return(eff) + end + end + + context 'example 1: increasing trend, but only recently crossed threshold' do + it 'returns the smoothed time efficiency' do + mock_efficiencies(1.1, 1, 0.95, 0.9, 0.8, 0.95, 0.9, 0.8, 0.9, 0.95) + + expect(subject).to be_within(0.05).of(0.95) + end + end + + context 'example 2: increasing trend, crossed threshold a while ago' do + it 'returns the smoothed time efficiency' do + mock_efficiencies(1.2, 1.1, 1, 1, 1.1, 1, 0.95, 0.9, 0.95, 0.9) + + expect(subject).to be_within(0.05).of(1.1) + end + end + + context 'example 3: decreasing trend, but only recently crossed threshold' do + it 'returns the smoothed time efficiency' do + mock_efficiencies(0.9, 0.95, 1, 1.2, 1.1, 1.2, 1.1, 1.0, 1.1, 1.0) + + expect(subject).to be_within(0.05).of(1.0) + end + end + + context 'example 4: latest run spiked' do + it 'returns the smoothed time efficiency' do + mock_efficiencies(1.2, 0.9, 0.8, 0.9, 0.95, 0.9, 0.92, 0.9, 0.95, 0.9) + + expect(subject).to be_within(0.02).of(0.96) + end + end + end + end + + describe '#optimize!' do + subject { batched_migration.optimize! } + + let(:batched_migration) { create(:batched_background_migration) } + let(:optimizer) { instance_double('Gitlab::Database::BackgroundMigration::BatchOptimizer') } + + it 'calls the BatchOptimizer' do + expect(Gitlab::Database::BackgroundMigration::BatchOptimizer).to receive(:new).with(batched_migration).and_return(optimizer) + expect(optimizer).to receive(:optimize!) + + subject + end + end end 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 00d13f23d36..c1183a15e37 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 @@ -7,9 +7,16 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' let(:job_class) { Gitlab::BackgroundMigration::CopyColumnUsingBackgroundMigrationJob } + let_it_be(:pause_ms) { 250 } let_it_be(:active_migration) { create(:batched_background_migration, :active, job_arguments: [:id, :other_id]) } - let!(:job_record) { create(:batched_background_migration_job, batched_migration: active_migration) } + let!(:job_record) do + create(:batched_background_migration_job, + batched_migration: active_migration, + pause_ms: pause_ms + ) + end + let(:job_instance) { double('job instance', batch_metrics: {}) } before do @@ -17,7 +24,7 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end it 'runs the migration job' do - expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, 'id', 'other_id') + expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') subject end @@ -42,6 +49,42 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end end + context 'when running a job that failed previously' do + let!(:job_record) do + create(:batched_background_migration_job, + batched_migration: active_migration, + pause_ms: pause_ms, + attempts: 1, + status: :failed, + finished_at: 1.hour.ago, + metrics: { 'my_metrics' => 'some_value' } + ) + end + + it 'increments attempts and updates other fields' do + updated_metrics = { 'updated_metrics' => 'some_value' } + + expect(job_instance).to receive(:perform) + expect(job_instance).to receive(:batch_metrics).and_return(updated_metrics) + + expect(job_record).to receive(:update!).with( + hash_including(attempts: 2, status: :running, finished_at: nil, metrics: {}) + ).and_call_original + + freeze_time do + subject + + job_record.reload + + expect(job_record).not_to be_failed + expect(job_record.attempts).to eq(2) + expect(job_record.started_at).to eq(Time.current) + expect(job_record.finished_at).to eq(Time.current) + expect(job_record.metrics).to eq(updated_metrics) + end + end + end + context 'reporting prometheus metrics' do let(:labels) { job_record.batched_migration.prometheus_labels } @@ -61,12 +104,26 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' subject end + it 'reports interval' do + expect(described_class.metrics[:gauge_interval]).to receive(:set).with(labels, job_record.batched_migration.interval) + + subject + end + it 'reports updated tuples (currently based on batch_size)' do expect(described_class.metrics[:counter_updated_tuples]).to receive(:increment).with(labels, job_record.batch_size) subject end + it 'reports migrated tuples' do + count = double + expect(job_record.batched_migration).to receive(:migrated_tuple_count).and_return(count) + expect(described_class.metrics[:gauge_migrated_tuples]).to receive(:set).with(labels, count) + + subject + end + it 'reports summary of query timings' do metrics = { 'timings' => { 'update_all' => [1, 2, 3, 4, 5] } } @@ -82,14 +139,26 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' subject end - it 'reports time efficiency' do + it 'reports job duration' do freeze_time do expect(Time).to receive(:current).and_return(Time.zone.now - 5.seconds).ordered - expect(Time).to receive(:current).and_return(Time.zone.now).ordered + allow(Time).to receive(:current).and_call_original + + expect(described_class.metrics[:gauge_job_duration]).to receive(:set).with(labels, 5.seconds) - ratio = 5 / job_record.batched_migration.interval.to_f + subject + end + end - expect(described_class.metrics[:histogram_time_efficiency]).to receive(:observe).with(labels, ratio) + it 'reports the total tuple count for the migration' do + expect(described_class.metrics[:gauge_total_tuple_count]).to receive(:set).with(labels, job_record.batched_migration.total_tuple_count) + + subject + end + + it 'reports last updated at timestamp' do + freeze_time do + expect(described_class.metrics[:gauge_last_update_time]).to receive(:set).with(labels, Time.current.to_i) subject end @@ -98,7 +167,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(1, 10, 'events', 'id', 1, 'id', 'other_id') + expect(job_instance).to receive(:perform).with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') freeze_time do subject @@ -112,19 +181,24 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigrationWrapper, ' end context 'when the migration job raises an error' do - it 'marks the tracking record as failed before raising the error' do - expect(job_instance).to receive(:perform) - .with(1, 10, 'events', 'id', 1, 'id', 'other_id') - .and_raise(RuntimeError, 'Something broke!') + shared_examples 'an error is raised' do |error_class| + it 'marks the tracking record as failed' do + expect(job_instance).to receive(:perform) + .with(1, 10, 'events', 'id', 1, pause_ms, 'id', 'other_id') + .and_raise(error_class) - freeze_time do - expect { subject }.to raise_error(RuntimeError, 'Something broke!') + freeze_time do + expect { subject }.to raise_error(error_class) - reloaded_job_record = job_record.reload + reloaded_job_record = job_record.reload - expect(reloaded_job_record).to be_failed - expect(reloaded_job_record.finished_at).to eq(Time.current) + expect(reloaded_job_record).to be_failed + expect(reloaded_job_record.finished_at).to eq(Time.current) + end end end + + it_behaves_like 'an error is raised', RuntimeError.new('Something broke!') + it_behaves_like 'an error is raised', SignalException.new('SIGTERM') end end |