diff options
-rw-r--r-- | changelogs/unreleased/58739-hashed-storage-prevent-a-migration-and-rollback-running-at-the-same-time.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/hashed_storage/migrator.rb | 18 | ||||
-rw-r--r-- | lib/tasks/gitlab/storage.rake | 15 | ||||
-rw-r--r-- | spec/lib/gitlab/hashed_storage/migrator_spec.rb | 52 | ||||
-rw-r--r-- | spec/tasks/gitlab/storage_rake_spec.rb | 108 |
5 files changed, 167 insertions, 31 deletions
diff --git a/changelogs/unreleased/58739-hashed-storage-prevent-a-migration-and-rollback-running-at-the-same-time.yml b/changelogs/unreleased/58739-hashed-storage-prevent-a-migration-and-rollback-running-at-the-same-time.yml new file mode 100644 index 00000000000..765a991bb6a --- /dev/null +++ b/changelogs/unreleased/58739-hashed-storage-prevent-a-migration-and-rollback-running-at-the-same-time.yml @@ -0,0 +1,5 @@ +--- +title: 'Hashed Storage: Prevent a migration and rollback running at the same time' +merge_request: 25976 +author: +type: changed diff --git a/lib/gitlab/hashed_storage/migrator.rb b/lib/gitlab/hashed_storage/migrator.rb index 7046b4e2a43..f5368d41629 100644 --- a/lib/gitlab/hashed_storage/migrator.rb +++ b/lib/gitlab/hashed_storage/migrator.rb @@ -81,8 +81,26 @@ module Gitlab Rails.logger.error("#{err.message} rolling-back storage of #{project.full_path} (ID=#{project.id}), trace - #{err.backtrace}") end + # Returns whether we have any pending storage migration + # + def migration_pending? + any_non_empty_queue?(::HashedStorage::MigratorWorker, ::HashedStorage::ProjectMigrateWorker) + end + + # Returns whether we have any pending storage rollback + # + def rollback_pending? + any_non_empty_queue?(::HashedStorage::RollbackerWorker, ::HashedStorage::ProjectRollbackWorker) + end + private + def any_non_empty_queue?(*workers) + workers.any? do |worker| + worker.jobs.any? + end + end + # rubocop: disable CodeReuse/ActiveRecord def build_relation(start, finish) relation = Project diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake index a2136ce1b92..954f827f716 100644 --- a/lib/tasks/gitlab/storage.rake +++ b/lib/tasks/gitlab/storage.rake @@ -11,6 +11,13 @@ namespace :gitlab do storage_migrator = Gitlab::HashedStorage::Migrator.new helper = Gitlab::HashedStorage::RakeHelper + if storage_migrator.rollback_pending? + warn "There is already a rollback operation in progress, " \ + "running a migration at the same time may have unexpected consequences." + + next + end + if helper.range_single_item? project = Project.with_unmigrated_storage.find_by(id: helper.range_from) @@ -56,6 +63,13 @@ namespace :gitlab do storage_migrator = Gitlab::HashedStorage::Migrator.new helper = Gitlab::HashedStorage::RakeHelper + if storage_migrator.migration_pending? + warn "There is already a migration operation in progress, " \ + "running a rollback at the same time may have unexpected consequences." + + next + end + if helper.range_single_item? project = Project.with_storage_feature(:repository).find_by(id: helper.range_from) @@ -82,7 +96,6 @@ namespace :gitlab do print "Enqueuing rollback of #{hashed_projects_count} projects in batches of #{helper.batch_size}" helper.project_id_batches_rollback do |start, finish| - puts "Start: #{start} FINISH: #{finish}" storage_migrator.bulk_schedule_rollback(start: start, finish: finish) print '.' diff --git a/spec/lib/gitlab/hashed_storage/migrator_spec.rb b/spec/lib/gitlab/hashed_storage/migrator_spec.rb index 6154b3e2f76..d03a74ac9eb 100644 --- a/spec/lib/gitlab/hashed_storage/migrator_spec.rb +++ b/spec/lib/gitlab/hashed_storage/migrator_spec.rb @@ -1,6 +1,8 @@ +# frozen_string_literal: true + require 'spec_helper' -describe Gitlab::HashedStorage::Migrator do +describe Gitlab::HashedStorage::Migrator, :sidekiq do describe '#bulk_schedule_migration' do it 'schedules job to HashedStorage::MigratorWorker' do Sidekiq::Testing.fake! do @@ -182,4 +184,52 @@ describe Gitlab::HashedStorage::Migrator do end end end + + describe 'migration_pending?' do + set(:project) { create(:project, :empty_repo) } + + it 'returns true when there are MigratorWorker jobs scheduled' do + Sidekiq::Testing.fake! do + ::HashedStorage::MigratorWorker.perform_async(1, 5) + + expect(subject.migration_pending?).to be_truthy + end + end + + it 'returns true when there are ProjectMigrateWorker jobs scheduled' do + Sidekiq::Testing.fake! do + ::HashedStorage::ProjectMigrateWorker.perform_async(1) + + expect(subject.migration_pending?).to be_truthy + end + end + + it 'returns false when queues are empty' do + expect(subject.migration_pending?).to be_falsey + end + end + + describe 'rollback_pending?' do + set(:project) { create(:project, :empty_repo) } + + it 'returns true when there are RollbackerWorker jobs scheduled' do + Sidekiq::Testing.fake! do + ::HashedStorage::RollbackerWorker.perform_async(1, 5) + + expect(subject.rollback_pending?).to be_truthy + end + end + + it 'returns true when there are jobs scheduled' do + Sidekiq::Testing.fake! do + ::HashedStorage::ProjectRollbackWorker.perform_async(1) + + expect(subject.rollback_pending?).to be_truthy + end + end + + it 'returns false when queues are empty' do + expect(subject.rollback_pending?).to be_falsey + end + end end diff --git a/spec/tasks/gitlab/storage_rake_spec.rb b/spec/tasks/gitlab/storage_rake_spec.rb index 6b50670c3c0..736809eee5b 100644 --- a/spec/tasks/gitlab/storage_rake_spec.rb +++ b/spec/tasks/gitlab/storage_rake_spec.rb @@ -1,6 +1,6 @@ require 'rake_helper' -describe 'rake gitlab:storage:*' do +describe 'rake gitlab:storage:*', :sidekiq do before do Rake.application.rake_require 'tasks/gitlab/storage' @@ -43,9 +43,7 @@ describe 'rake gitlab:storage:*' do end end - describe 'gitlab:storage:migrate_to_hashed' do - let(:task) { 'gitlab:storage:migrate_to_hashed' } - + shared_examples "make sure database is writable" do context 'read-only database' do it 'does nothing' do expect(Gitlab::Database).to receive(:read_only?).and_return(true) @@ -55,48 +53,68 @@ describe 'rake gitlab:storage:*' do expect { run_rake_task(task) }.to output(/This task requires database write access. Exiting./).to_stderr end end + end - context '0 legacy projects' do - it 'does nothing' do - expect(::HashedStorage::MigratorWorker).not_to receive(:perform_async) + shared_examples "handles custom BATCH env var" do |worker_klass| + context 'in batches of 1' do + before do + stub_env('BATCH' => 1) + end + + it "enqueues one #{worker_klass} per project" do + projects.each do |project| + expect(worker_klass).to receive(:perform_async).with(project.id, project.id) + end run_rake_task(task) end end - context '3 legacy projects' do - let(:projects) { create_list(:project, 3, :legacy_storage) } + context 'in batches of 2' do + before do + stub_env('BATCH' => 2) + end - context 'in batches of 1' do - before do - stub_env('BATCH' => 1) + it "enqueues one #{worker_klass} per 2 projects" do + projects.map(&:id).sort.each_slice(2) do |first, last| + last ||= first + expect(worker_klass).to receive(:perform_async).with(first, last) end - it 'enqueues one HashedStorage::MigratorWorker per project' do - projects.each do |project| - expect(::HashedStorage::MigratorWorker).to receive(:perform_async).with(project.id, project.id) - end - - run_rake_task(task) - end + run_rake_task(task) end + end + end - context 'in batches of 2' do - before do - stub_env('BATCH' => 2) - end + describe 'gitlab:storage:migrate_to_hashed' do + let(:task) { 'gitlab:storage:migrate_to_hashed' } - it 'enqueues one HashedStorage::MigratorWorker per 2 projects' do - projects.map(&:id).sort.each_slice(2) do |first, last| - last ||= first - expect(::HashedStorage::MigratorWorker).to receive(:perform_async).with(first, last) - end + context 'with rollback already scheduled' do + it 'does nothing' do + Sidekiq::Testing.fake! do + ::HashedStorage::RollbackerWorker.perform_async(1, 5) + + expect(Project).not_to receive(:with_unmigrated_storage) - run_rake_task(task) + expect { run_rake_task(task) }.to output(/There is already a rollback operation in progress/).to_stderr end end end + context 'with 0 legacy projects' do + it 'does nothing' do + expect(::HashedStorage::MigratorWorker).not_to receive(:perform_async) + + run_rake_task(task) + end + end + + context 'with 3 legacy projects' do + let(:projects) { create_list(:project, 3, :legacy_storage) } + + it_behaves_like "handles custom BATCH env var", ::HashedStorage::MigratorWorker + end + context 'with same id in range' do it 'displays message when project cant be found' do stub_env('ID_FROM', 99999) @@ -123,6 +141,38 @@ describe 'rake gitlab:storage:*' do end end + describe 'gitlab:storage:rollback_to_legacy' do + let(:task) { 'gitlab:storage:rollback_to_legacy' } + + it_behaves_like 'make sure database is writable' + + context 'with migration already scheduled' do + it 'does nothing' do + Sidekiq::Testing.fake! do + ::HashedStorage::MigratorWorker.perform_async(1, 5) + + expect(Project).not_to receive(:with_unmigrated_storage) + + expect { run_rake_task(task) }.to output(/There is already a migration operation in progress/).to_stderr + end + end + end + + context 'with 0 hashed projects' do + it 'does nothing' do + expect(::HashedStorage::RollbackerWorker).not_to receive(:perform_async) + + run_rake_task(task) + end + end + + context 'with 3 hashed projects' do + let(:projects) { create_list(:project, 3) } + + it_behaves_like "handles custom BATCH env var", ::HashedStorage::RollbackerWorker + end + end + describe 'gitlab:storage:legacy_projects' do it_behaves_like 'rake entities summary', 'projects', 'Legacy' do let(:task) { 'gitlab:storage:legacy_projects' } |