From d63380fa93dff921c69f7aaa31ff004864e4db13 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 23 Jan 2019 03:31:57 +0100 Subject: Refactor ProjectMigrate and ProjectRollback workers Moved to HashedStorage namespace, and added them to the `:hashed_storage` queue namespace --- app/models/project.rb | 8 ++-- app/workers/all_queues.yml | 4 +- .../hashed_storage/project_migrate_worker.rb | 48 +++++++++++++++++++++ .../hashed_storage/project_rollback_worker.rb | 47 ++++++++++++++++++++ .../project_migrate_hashed_storage_worker.rb | 43 ------------------- .../project_rollback_hashed_storage_worker.rb | 42 ------------------ spec/lib/gitlab/hashed_storage/migrator_spec.rb | 12 +++--- spec/models/project_spec.rb | 16 +++---- .../hashed_storage/project_migrate_worker_spec.rb | 48 +++++++++++++++++++++ .../hashed_storage/project_rollback_worker_spec.rb | 50 ++++++++++++++++++++++ .../project_migrate_hashed_storage_worker_spec.rb | 48 --------------------- .../project_rollback_hashed_storage_worker_spec.rb | 50 ---------------------- 12 files changed, 213 insertions(+), 203 deletions(-) create mode 100644 app/workers/hashed_storage/project_migrate_worker.rb create mode 100644 app/workers/hashed_storage/project_rollback_worker.rb delete mode 100644 app/workers/project_migrate_hashed_storage_worker.rb delete mode 100644 app/workers/project_rollback_hashed_storage_worker.rb create mode 100644 spec/workers/hashed_storage/project_migrate_worker_spec.rb create mode 100644 spec/workers/hashed_storage/project_rollback_worker_spec.rb delete mode 100644 spec/workers/project_migrate_hashed_storage_worker_spec.rb delete mode 100644 spec/workers/project_rollback_hashed_storage_worker_spec.rb diff --git a/app/models/project.rb b/app/models/project.rb index 400e86123f7..00592c108db 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1970,9 +1970,9 @@ class Project < ActiveRecord::Base return unless storage_upgradable? if git_transfer_in_progress? - ProjectMigrateHashedStorageWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id) + HashedStorage::ProjectMigrateWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id) else - ProjectMigrateHashedStorageWorker.perform_async(id) + HashedStorage::ProjectMigrateWorker.perform_async(id) end end @@ -1980,9 +1980,9 @@ class Project < ActiveRecord::Base return if legacy_storage? if git_transfer_in_progress? - ProjectRollbackHashedStorageWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id) + HashedStorage::ProjectRollbackWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id) else - ProjectRollbackHashedStorageWorker.perform_async(id) + HashedStorage::ProjectRollbackWorker.perform_async(id) end end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 82351c1334f..a5dce221883 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -47,6 +47,8 @@ - github_importer:github_import_stage_import_repository - hashed_storage:hashed_storage_migrator +- hashed_storage:hashed_storage_project_migrate +- hashed_storage:hashed_storage_project_rollback - mail_scheduler:mail_scheduler_issue_due - mail_scheduler:mail_scheduler_notification_service @@ -126,8 +128,6 @@ - project_cache - project_destroy - project_export -- project_migrate_hashed_storage -- project_rollback_hashed_storage - project_service - propagate_service_template - reactive_caching diff --git a/app/workers/hashed_storage/project_migrate_worker.rb b/app/workers/hashed_storage/project_migrate_worker.rb new file mode 100644 index 00000000000..05bf72f519a --- /dev/null +++ b/app/workers/hashed_storage/project_migrate_worker.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module HashedStorage + class ProjectMigrateWorker + include ApplicationWorker + + LEASE_TIMEOUT = 30.seconds.to_i + LEASE_KEY_SEGMENT = 'project_migrate_hashed_storage_worker'.freeze + + queue_namespace :hashed_storage + + # rubocop: disable CodeReuse/ActiveRecord + def perform(project_id, old_disk_path = nil) + uuid = lease_for(project_id).try_obtain + + if uuid + project = Project.find_by(id: project_id) + return if project.nil? || project.pending_delete? + + old_disk_path ||= project.disk_path + + ::Projects::HashedStorage::MigrationService.new(project, old_disk_path, logger: logger).execute + else + return false + end + + ensure + cancel_lease_for(project_id, uuid) if uuid + end + + # rubocop: enable CodeReuse/ActiveRecord + + def lease_for(project_id) + Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT) + end + + private + + def lease_key(project_id) + # we share the same lease key for both migration and rollback so they don't run simultaneously + "#{LEASE_KEY_SEGMENT}:#{project_id}" + end + + def cancel_lease_for(project_id, uuid) + Gitlab::ExclusiveLease.cancel(lease_key(project_id), uuid) + end + end +end diff --git a/app/workers/hashed_storage/project_rollback_worker.rb b/app/workers/hashed_storage/project_rollback_worker.rb new file mode 100644 index 00000000000..ace9fea98a6 --- /dev/null +++ b/app/workers/hashed_storage/project_rollback_worker.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module HashedStorage + class ProjectRollbackWorker + include ApplicationWorker + + LEASE_TIMEOUT = 30.seconds.to_i + + queue_namespace :hashed_storage + + # rubocop: disable CodeReuse/ActiveRecord + def perform(project_id, old_disk_path = nil) + uuid = lease_for(project_id).try_obtain + + if uuid + project = Project.find_by(id: project_id) + return if project.nil? || project.pending_delete? + + old_disk_path ||= project.disk_path + + ::Projects::HashedStorage::RollbackService.new(project, old_disk_path, logger: logger).execute + else + return false + end + + ensure + cancel_lease_for(project_id, uuid) if uuid + end + + # rubocop: enable CodeReuse/ActiveRecord + + def lease_for(project_id) + Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT) + end + + private + + def lease_key(project_id) + # we share the same lease key for both migration and rollback so they don't run simultaneously + "#{ProjectMigrateWorker::LEASE_KEY_SEGMENT}:#{project_id}" + end + + def cancel_lease_for(project_id, uuid) + Gitlab::ExclusiveLease.cancel(lease_key(project_id), uuid) + end + end +end diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb deleted file mode 100644 index 1c8f313e6e9..00000000000 --- a/app/workers/project_migrate_hashed_storage_worker.rb +++ /dev/null @@ -1,43 +0,0 @@ -# frozen_string_literal: true - -class ProjectMigrateHashedStorageWorker - include ApplicationWorker - - LEASE_TIMEOUT = 30.seconds.to_i - LEASE_KEY_SEGMENT = 'project_migrate_hashed_storage_worker'.freeze - - # rubocop: disable CodeReuse/ActiveRecord - def perform(project_id, old_disk_path = nil) - uuid = lease_for(project_id).try_obtain - - if uuid - project = Project.find_by(id: project_id) - return if project.nil? || project.pending_delete? - - old_disk_path ||= project.disk_path - - ::Projects::HashedStorage::MigrationService.new(project, old_disk_path, logger: logger).execute - else - return false - end - - ensure - cancel_lease_for(project_id, uuid) if uuid - end - # rubocop: enable CodeReuse/ActiveRecord - - def lease_for(project_id) - Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT) - end - - private - - def lease_key(project_id) - # we share the same lease key for both migration and rollback so they don't run simultaneously - "#{LEASE_KEY_SEGMENT}:#{project_id}" - end - - def cancel_lease_for(project_id, uuid) - Gitlab::ExclusiveLease.cancel(lease_key(project_id), uuid) - end -end diff --git a/app/workers/project_rollback_hashed_storage_worker.rb b/app/workers/project_rollback_hashed_storage_worker.rb deleted file mode 100644 index 38faffd2bfa..00000000000 --- a/app/workers/project_rollback_hashed_storage_worker.rb +++ /dev/null @@ -1,42 +0,0 @@ -# frozen_string_literal: true - -class ProjectRollbackHashedStorageWorker - include ApplicationWorker - - LEASE_TIMEOUT = 30.seconds.to_i - - # rubocop: disable CodeReuse/ActiveRecord - def perform(project_id, old_disk_path = nil) - uuid = lease_for(project_id).try_obtain - - if uuid - project = Project.find_by(id: project_id) - return if project.nil? || project.pending_delete? - - old_disk_path ||= project.disk_path - - ::Projects::HashedStorage::RollbackService.new(project, old_disk_path, logger: logger).execute - else - return false - end - - ensure - cancel_lease_for(project_id, uuid) if uuid - end - # rubocop: enable CodeReuse/ActiveRecord - - def lease_for(project_id) - Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT) - end - - private - - def lease_key(project_id) - # we share the same lease key for both migration and rollback so they don't run simultaneously - "#{ProjectMigrateHashedStorageWorker::LEASE_KEY_SEGMENT}:#{project_id}" - end - - def cancel_lease_for(project_id, uuid) - Gitlab::ExclusiveLease.cancel(lease_key(project_id), uuid) - end -end diff --git a/spec/lib/gitlab/hashed_storage/migrator_spec.rb b/spec/lib/gitlab/hashed_storage/migrator_spec.rb index 7c2582bb27a..000913f32bb 100644 --- a/spec/lib/gitlab/hashed_storage/migrator_spec.rb +++ b/spec/lib/gitlab/hashed_storage/migrator_spec.rb @@ -13,9 +13,9 @@ describe Gitlab::HashedStorage::Migrator do let(:projects) { create_list(:project, 2, :legacy_storage) } let(:ids) { projects.map(&:id) } - it 'enqueue jobs to ProjectMigrateHashedStorageWorker' do + it 'enqueue jobs to HashedStorage::ProjectMigrateWorker' do Sidekiq::Testing.fake! do - expect { subject.bulk_migrate(start: ids.min, finish: ids.max) }.to change(ProjectMigrateHashedStorageWorker.jobs, :size).by(2) + expect { subject.bulk_migrate(start: ids.min, finish: ids.max) }.to change(HashedStorage::ProjectMigrateWorker.jobs, :size).by(2) end end @@ -48,7 +48,7 @@ describe Gitlab::HashedStorage::Migrator do it 'enqueues project migration job' do Sidekiq::Testing.fake! do - expect { subject.migrate(project) }.to change(ProjectMigrateHashedStorageWorker.jobs, :size).by(1) + expect { subject.migrate(project) }.to change(HashedStorage::ProjectMigrateWorker.jobs, :size).by(1) end end @@ -79,7 +79,7 @@ describe Gitlab::HashedStorage::Migrator do it 'doesnt enqueue any migration job' do Sidekiq::Testing.fake! do - expect { subject.migrate(project) }.not_to change(ProjectMigrateHashedStorageWorker.jobs, :size) + expect { subject.migrate(project) }.not_to change(HashedStorage::ProjectMigrateWorker.jobs, :size) end end @@ -94,7 +94,7 @@ describe Gitlab::HashedStorage::Migrator do it 'enqueues project rollback job' do Sidekiq::Testing.fake! do - expect { subject.rollback(project) }.to change(ProjectRollbackHashedStorageWorker.jobs, :size).by(1) + expect { subject.rollback(project) }.to change(HashedStorage::ProjectRollbackWorker.jobs, :size).by(1) end end @@ -125,7 +125,7 @@ describe Gitlab::HashedStorage::Migrator do it 'doesnt enqueue any rollback job' do Sidekiq::Testing.fake! do - expect { subject.rollback(project) }.not_to change(ProjectRollbackHashedStorageWorker.jobs, :size) + expect { subject.rollback(project) }.not_to change(HashedStorage::ProjectRollbackWorker.jobs, :size) end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6dc89b352f3..b2392f9521f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3430,24 +3430,24 @@ describe Project do project.migrate_to_hashed_storage! end - it 'schedules ProjectMigrateHashedStorageWorker with delayed start when the project repo is in use' do + it 'schedules HashedStorage::ProjectMigrateWorker with delayed start when the project repo is in use' do Gitlab::ReferenceCounter.new(project.gl_repository(is_wiki: false)).increase - expect(ProjectMigrateHashedStorageWorker).to receive(:perform_in) + expect(HashedStorage::ProjectMigrateWorker).to receive(:perform_in) project.migrate_to_hashed_storage! end - it 'schedules ProjectMigrateHashedStorageWorker with delayed start when the wiki repo is in use' do + it 'schedules HashedStorage::ProjectMigrateWorker with delayed start when the wiki repo is in use' do Gitlab::ReferenceCounter.new(project.gl_repository(is_wiki: true)).increase - expect(ProjectMigrateHashedStorageWorker).to receive(:perform_in) + expect(HashedStorage::ProjectMigrateWorker).to receive(:perform_in) project.migrate_to_hashed_storage! end - it 'schedules ProjectMigrateHashedStorageWorker' do - expect(ProjectMigrateHashedStorageWorker).to receive(:perform_async).with(project.id) + it 'schedules HashedStorage::ProjectMigrateWorker' do + expect(HashedStorage::ProjectMigrateWorker).to receive(:perform_async).with(project.id) project.migrate_to_hashed_storage! end @@ -3541,7 +3541,7 @@ describe Project do project = create(:project, storage_version: 1, skip_disk_validation: true) Sidekiq::Testing.fake! do - expect { project.migrate_to_hashed_storage! }.to change(ProjectMigrateHashedStorageWorker.jobs, :size).by(1) + expect { project.migrate_to_hashed_storage! }.to change(HashedStorage::ProjectMigrateWorker.jobs, :size).by(1) end end end @@ -3566,7 +3566,7 @@ describe Project do it 'enqueues a job' do Sidekiq::Testing.fake! do - expect { project.rollback_to_legacy_storage! }.to change(ProjectRollbackHashedStorageWorker.jobs, :size).by(1) + expect { project.rollback_to_legacy_storage! }.to change(HashedStorage::ProjectRollbackWorker.jobs, :size).by(1) end end end diff --git a/spec/workers/hashed_storage/project_migrate_worker_spec.rb b/spec/workers/hashed_storage/project_migrate_worker_spec.rb new file mode 100644 index 00000000000..340e722aa7e --- /dev/null +++ b/spec/workers/hashed_storage/project_migrate_worker_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe HashedStorage::ProjectMigrateWorker, :clean_gitlab_redis_shared_state do + include ExclusiveLeaseHelpers + + describe '#perform' do + let(:project) { create(:project, :empty_repo, :legacy_storage) } + let(:lease_key) { "project_migrate_hashed_storage_worker:#{project.id}" } + let(:lease_timeout) { described_class::LEASE_TIMEOUT } + let(:migration_service) { ::Projects::HashedStorage::MigrationService } + + it 'skips when project no longer exists' do + expect(migration_service).not_to receive(:new) + + subject.perform(-1) + end + + it 'skips when project is pending delete' do + pending_delete_project = create(:project, :empty_repo, pending_delete: true) + + expect(migration_service).not_to receive(:new) + + subject.perform(pending_delete_project.id) + end + + it 'delegates migration to service class when we have exclusive lease' do + stub_exclusive_lease(lease_key, 'uuid', timeout: lease_timeout) + + service_spy = spy + + allow(migration_service) + .to receive(:new).with(project, project.full_path, logger: subject.logger) + .and_return(service_spy) + + subject.perform(project.id) + + expect(service_spy).to have_received(:execute) + end + + it 'skips when it cant acquire the exclusive lease' do + stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) + + expect(migration_service).not_to receive(:new) + + subject.perform(project.id) + end + end +end diff --git a/spec/workers/hashed_storage/project_rollback_worker_spec.rb b/spec/workers/hashed_storage/project_rollback_worker_spec.rb new file mode 100644 index 00000000000..d833553c0ec --- /dev/null +++ b/spec/workers/hashed_storage/project_rollback_worker_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe HashedStorage::ProjectRollbackWorker, :clean_gitlab_redis_shared_state do + include ExclusiveLeaseHelpers + + describe '#perform' do + let(:project) { create(:project, :empty_repo) } + let(:lease_key) { "project_migrate_hashed_storage_worker:#{project.id}" } + let(:lease_timeout) { described_class::LEASE_TIMEOUT } + let(:rollback_service) { ::Projects::HashedStorage::RollbackService } + + it 'skips when project no longer exists' do + expect(rollback_service).not_to receive(:new) + + subject.perform(-1) + end + + it 'skips when project is pending delete' do + pending_delete_project = create(:project, :empty_repo, pending_delete: true) + + expect(rollback_service).not_to receive(:new) + + subject.perform(pending_delete_project.id) + end + + it 'delegates rollback to service class when have exclusive lease' do + stub_exclusive_lease(lease_key, 'uuid', timeout: lease_timeout) + + service_spy = spy + + allow(rollback_service) + .to receive(:new).with(project, project.disk_path, logger: subject.logger) + .and_return(service_spy) + + subject.perform(project.id) + + expect(service_spy).to have_received(:execute) + end + + it 'skips when it cant acquire the exclusive lease' do + stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) + + expect(rollback_service).not_to receive(:new) + + subject.perform(project.id) + end + end +end diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb deleted file mode 100644 index 333eb6a0569..00000000000 --- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb +++ /dev/null @@ -1,48 +0,0 @@ -require 'spec_helper' - -describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do - include ExclusiveLeaseHelpers - - describe '#perform' do - let(:project) { create(:project, :empty_repo, :legacy_storage) } - let(:lease_key) { "project_migrate_hashed_storage_worker:#{project.id}" } - let(:lease_timeout) { described_class::LEASE_TIMEOUT } - let(:migration_service) { ::Projects::HashedStorage::MigrationService } - - it 'skips when project no longer exists' do - expect(migration_service).not_to receive(:new) - - subject.perform(-1) - end - - it 'skips when project is pending delete' do - pending_delete_project = create(:project, :empty_repo, pending_delete: true) - - expect(migration_service).not_to receive(:new) - - subject.perform(pending_delete_project.id) - end - - it 'delegates migration to service class when we have exclusive lease' do - stub_exclusive_lease(lease_key, 'uuid', timeout: lease_timeout) - - service_spy = spy - - allow(migration_service) - .to receive(:new).with(project, project.full_path, logger: subject.logger) - .and_return(service_spy) - - subject.perform(project.id) - - expect(service_spy).to have_received(:execute) - end - - it 'skips when it cant acquire the exclusive lease' do - stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) - - expect(migration_service).not_to receive(:new) - - subject.perform(project.id) - end - end -end diff --git a/spec/workers/project_rollback_hashed_storage_worker_spec.rb b/spec/workers/project_rollback_hashed_storage_worker_spec.rb deleted file mode 100644 index aed7493763d..00000000000 --- a/spec/workers/project_rollback_hashed_storage_worker_spec.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe ProjectRollbackHashedStorageWorker, :clean_gitlab_redis_shared_state do - include ExclusiveLeaseHelpers - - describe '#perform' do - let(:project) { create(:project, :empty_repo) } - let(:lease_key) { "project_migrate_hashed_storage_worker:#{project.id}" } - let(:lease_timeout) { described_class::LEASE_TIMEOUT } - let(:rollback_service) { ::Projects::HashedStorage::RollbackService } - - it 'skips when project no longer exists' do - expect(rollback_service).not_to receive(:new) - - subject.perform(-1) - end - - it 'skips when project is pending delete' do - pending_delete_project = create(:project, :empty_repo, pending_delete: true) - - expect(rollback_service).not_to receive(:new) - - subject.perform(pending_delete_project.id) - end - - it 'delegates rollback to service class when have exclusive lease' do - stub_exclusive_lease(lease_key, 'uuid', timeout: lease_timeout) - - service_spy = spy - - allow(rollback_service) - .to receive(:new).with(project, project.disk_path, logger: subject.logger) - .and_return(service_spy) - - subject.perform(project.id) - - expect(service_spy).to have_received(:execute) - end - - it 'skips when it cant acquire the exclusive lease' do - stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) - - expect(rollback_service).not_to receive(:new) - - subject.perform(project.id) - end - end -end -- cgit v1.2.1