From ff2ca3569e704bb26c770ba5c28a888789d27230 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sat, 22 Dec 2018 14:54:33 +0100 Subject: [PATCH 01/10] Rake task for storage rollback --- lib/tasks/gitlab/storage.rake | 45 +++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake index f9ce3e1d338..b759a2dad3a 100644 --- a/lib/tasks/gitlab/storage.rake +++ b/lib/tasks/gitlab/storage.rake @@ -45,6 +45,51 @@ namespace :gitlab do puts ' Done!' end + desc 'GitLab | Storage | Rollback existing projects to Legacy Storage' + task rollback_to_legacy: :environment do + if Gitlab::Database.read_only? + warn 'This task requires database write access. Exiting.' + + next + end + + storage_migrator = Gitlab::HashedStorage::Migrator.new + helper = Gitlab::HashedStorage::RakeHelper + + if helper.range_single_item? + project = Project.with_storage_feature(:repository).find_by(id: helper.range_from) + + unless project + warn "There are no projects that can be rolledback with ID=#{helper.range_from}" + + next + end + + puts "Enqueueing storage rollback of #{project.full_path} (ID=#{project.id})..." + storage_migrator.rollback(project) + + next + end + + hashed_projects_count = Project.with_storage_feature(:repository).count + + if hashed_projects_count == 0 + warn 'There are no projects that can have storage rolledback. Nothing to do!' + + next + end + + print "Enqueuing rollback of #{hashed_projects_count} projects in batches of #{helper.batch_size}" + + helper.project_id_batches do |start, finish| + storage_migrator.bulk_schedule(start: start, finish: finish, operation: :rollback) + + print '.' + end + + puts ' Done!' + end + desc 'Gitlab | Storage | Summary of existing projects using Legacy Storage' task legacy_projects: :environment do helper = Gitlab::HashedStorage::RakeHelper From 1592b5830f7b2847dff02ef2c66b745cdc60565a Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Thu, 17 Jan 2019 02:57:35 +0100 Subject: [PATCH 02/10] Adds Rollback functionality to HashedStorage migration We are adding sidekiq workers and service classes to allow to rollback a hashed storage migration. There are some refactoring involved as well as part of the code can be reused by both the migration and the rollback logic. --- app/models/project.rb | 10 ++ .../rollback_attachments_service.rb | 53 +++++++++ .../rollback_repository_service.rb | 52 +++++++++ .../hashed_storage/rollback_service.rb | 37 +++++++ app/workers/all_queues.yml | 1 + .../project_rollback_hashed_storage_worker.rb | 42 ++++++++ ...rage-migration-safer-and-more-inviting.yml | 5 + config/sidekiq_queues.yml | 1 + lib/gitlab/hashed_storage/migrator.rb | 9 +- .../gitlab/hashed_storage/migrator_spec.rb | 46 ++++++++ spec/models/project_spec.rb | 38 +++++++ .../rollback_repository_service_spec.rb | 101 ++++++++++++++++++ .../hashed_storage/rollback_service_spec.rb | 57 ++++++++++ ...ect_rollback_hashed_storage_worker_spec.rb | 50 +++++++++ 14 files changed, 501 insertions(+), 1 deletion(-) create mode 100644 app/services/projects/hashed_storage/rollback_attachments_service.rb create mode 100644 app/services/projects/hashed_storage/rollback_repository_service.rb create mode 100644 app/services/projects/hashed_storage/rollback_service.rb create mode 100644 app/workers/project_rollback_hashed_storage_worker.rb create mode 100644 changelogs/unreleased/53966-make-hashed-storage-migration-safer-and-more-inviting.yml create mode 100644 spec/services/projects/hashed_storage/rollback_repository_service_spec.rb create mode 100644 spec/services/projects/hashed_storage/rollback_service_spec.rb create mode 100644 spec/workers/project_rollback_hashed_storage_worker_spec.rb diff --git a/app/models/project.rb b/app/models/project.rb index 47baf899222..400e86123f7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1976,6 +1976,16 @@ class Project < ActiveRecord::Base end end + def rollback_to_legacy_storage! + return if legacy_storage? + + if git_transfer_in_progress? + ProjectRollbackHashedStorageWorker.perform_in(Gitlab::ReferenceCounter::REFERENCE_EXPIRE_TIME, id) + else + ProjectRollbackHashedStorageWorker.perform_async(id) + end + end + def git_transfer_in_progress? repo_reference_count > 0 || wiki_reference_count > 0 end diff --git a/app/services/projects/hashed_storage/rollback_attachments_service.rb b/app/services/projects/hashed_storage/rollback_attachments_service.rb new file mode 100644 index 00000000000..5183609ab85 --- /dev/null +++ b/app/services/projects/hashed_storage/rollback_attachments_service.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module Projects + module HashedStorage + AttachmentRollbackError = Class.new(StandardError) + + class RollbackAttachmentsService < BaseService + attr_reader :logger, :old_disk_path + + def initialize(project, logger: nil) + @project = project + @logger = logger || Rails.logger + end + + def execute + origin = FileUploader.absolute_base_dir(project) + project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository] + target = FileUploader.absolute_base_dir(project) + + result = move_folder!(origin, target) + project.save! + + if result && block_given? + yield + end + + result + end + + private + + def move_folder!(old_path, new_path) + unless File.directory?(old_path) + logger.info("Skipped attachments rollback from '#{old_path}' to '#{new_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})") + return true + end + + if File.exist?(new_path) + logger.error("Cannot rollback attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") + raise AttachmentRollbackError, "Target path '#{new_path}' already exist" + end + + # Create hashed storage base path folder + FileUtils.mkdir_p(File.dirname(new_path)) + + FileUtils.mv(old_path, new_path) + logger.info("Rolledback project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") + + true + end + end + end +end diff --git a/app/services/projects/hashed_storage/rollback_repository_service.rb b/app/services/projects/hashed_storage/rollback_repository_service.rb new file mode 100644 index 00000000000..b57d05c50ca --- /dev/null +++ b/app/services/projects/hashed_storage/rollback_repository_service.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Projects + module HashedStorage + class RollbackRepositoryService < BaseRepositoryService + def execute + try_to_set_repository_read_only! + + @old_storage_version = project.storage_version + project.storage_version = nil + project.ensure_storage_path_exists + + @new_disk_path = project.disk_path + + result = move_repository(old_disk_path, new_disk_path) + + if move_wiki + result &&= move_repository("#{old_wiki_disk_path}", "#{new_disk_path}.wiki") + end + + if result + project.write_repository_config + project.track_project_repository + else + rollback_folder_move + project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository] + end + + project.repository_read_only = false + project.save! + + if result && block_given? + yield + end + + result + end + + private + + def try_to_set_repository_read_only! + # Mitigate any push operation to start during migration + unless project.set_repository_read_only! + migration_error = "Target repository '#{old_disk_path}' cannot be made read-only as there is a git transfer in progress" + logger.error migration_error + + raise RepositoryRollbackError, migration_error + end + end + end + end +end diff --git a/app/services/projects/hashed_storage/rollback_service.rb b/app/services/projects/hashed_storage/rollback_service.rb new file mode 100644 index 00000000000..25767f5de5e --- /dev/null +++ b/app/services/projects/hashed_storage/rollback_service.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Projects + module HashedStorage + class RollbackService < BaseService + attr_reader :logger, :old_disk_path + + def initialize(project, old_disk_path, logger: nil) + @project = project + @old_disk_path = old_disk_path + @logger = logger || Rails.logger + end + + def execute + # Rollback attachments from Hashed Storage to Legacy + if project.hashed_storage?(:attachments) + return false unless rollback_attachments + end + + # Rollback repository from Hashed Storage to Legacy + if project.hashed_storage?(:repository) + rollback_repository + end + end + + private + + def rollback_attachments + HashedStorage::RollbackAttachmentsService.new(project, logger: logger).execute + end + + def rollback_repository + HashedStorage::RollbackRepositoryService.new(project, old_disk_path, logger: logger).execute + end + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 337f39b2091..82351c1334f 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -127,6 +127,7 @@ - project_destroy - project_export - project_migrate_hashed_storage +- project_rollback_hashed_storage - project_service - propagate_service_template - reactive_caching diff --git a/app/workers/project_rollback_hashed_storage_worker.rb b/app/workers/project_rollback_hashed_storage_worker.rb new file mode 100644 index 00000000000..38faffd2bfa --- /dev/null +++ b/app/workers/project_rollback_hashed_storage_worker.rb @@ -0,0 +1,42 @@ +# 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/changelogs/unreleased/53966-make-hashed-storage-migration-safer-and-more-inviting.yml b/changelogs/unreleased/53966-make-hashed-storage-migration-safer-and-more-inviting.yml new file mode 100644 index 00000000000..556a238ff7d --- /dev/null +++ b/changelogs/unreleased/53966-make-hashed-storage-migration-safer-and-more-inviting.yml @@ -0,0 +1,5 @@ +--- +title: Hashed Storage rollback mechanism +merge_request: 23955 +author: +type: added diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 1f40363e126..cef123b86ae 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -68,6 +68,7 @@ - [background_migration, 1] - [gcp_cluster, 1] - [project_migrate_hashed_storage, 1] + - [project_rollback_hashed_storage, 1] - [hashed_storage, 1] - [pages_domain_verification, 1] - [object_storage_upload, 1] diff --git a/lib/gitlab/hashed_storage/migrator.rb b/lib/gitlab/hashed_storage/migrator.rb index bf463077dcc..73ed57ea584 100644 --- a/lib/gitlab/hashed_storage/migrator.rb +++ b/lib/gitlab/hashed_storage/migrator.rb @@ -45,8 +45,15 @@ module Gitlab Rails.logger.error("#{err.message} migrating storage of #{project.full_path} (ID=#{project.id}), trace - #{err.backtrace}") end + # Flag a project to be rolled-back to Legacy Storage + # + # @param [Project] project that will be rolled-back def rollback(project) - # TODO: implement rollback strategy + Rails.logger.info "Starting storage rollback of #{project.full_path} (ID=#{project.id})..." + + project.rollback_to_legacy_storage! + rescue => err + Rails.logger.error("#{err.message} rolling-back storage of #{project.full_path} (ID=#{project.id}), trace - #{err.backtrace}") end private diff --git a/spec/lib/gitlab/hashed_storage/migrator_spec.rb b/spec/lib/gitlab/hashed_storage/migrator_spec.rb index 3942f168ceb..7c2582bb27a 100644 --- a/spec/lib/gitlab/hashed_storage/migrator_spec.rb +++ b/spec/lib/gitlab/hashed_storage/migrator_spec.rb @@ -88,4 +88,50 @@ describe Gitlab::HashedStorage::Migrator do end end end + + describe '#rollback' do + let(:project) { create(:project, :empty_repo) } + + it 'enqueues project rollback job' do + Sidekiq::Testing.fake! do + expect { subject.rollback(project) }.to change(ProjectRollbackHashedStorageWorker.jobs, :size).by(1) + end + end + + it 'rescues and log exceptions' do + allow(project).to receive(:rollback_to_hashed_storage!).and_raise(StandardError) + + expect { subject.rollback(project) }.not_to raise_error + end + + it 'rolls-back project storage' do + perform_enqueued_jobs do + subject.rollback(project) + end + + expect(project.reload.legacy_storage?).to be_truthy + end + + it 'has rolled-back project set as writable' do + perform_enqueued_jobs do + subject.rollback(project) + end + + expect(project.reload.repository_read_only?).to be_falsey + end + + context 'when project is already on legacy storage' do + let(:project) { create(:project, :legacy_storage, :empty_repo) } + + it 'doesnt enqueue any rollback job' do + Sidekiq::Testing.fake! do + expect { subject.rollback(project) }.not_to change(ProjectRollbackHashedStorageWorker.jobs, :size) + end + end + + it 'returns false' do + expect(subject.rollback(project)).to be_falsey + end + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9fb0d04ca9e..6dc89b352f3 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3452,6 +3452,20 @@ describe Project do project.migrate_to_hashed_storage! end end + + describe '#rollback_to_legacy_storage!' do + let(:project) { create(:project, :empty_repo, :legacy_storage) } + + it 'returns nil' do + expect(project.rollback_to_legacy_storage!).to be_nil + end + + it 'does not run validations' do + expect(project).not_to receive(:valid?) + + project.rollback_to_legacy_storage! + end + end end context 'hashed storage' do @@ -3532,6 +3546,30 @@ describe Project do end end end + + describe '#rollback_to_legacy_storage!' do + let(:project) { create(:project, :repository, skip_disk_validation: true) } + + it 'returns true' do + expect(project.rollback_to_legacy_storage!).to be_truthy + end + + it 'does not run validations' do + expect(project).not_to receive(:valid?) + + project.rollback_to_legacy_storage! + end + + it 'does not flag as read-only' do + expect { project.rollback_to_legacy_storage! }.not_to change { project.repository_read_only } + end + + it 'enqueues a job' do + Sidekiq::Testing.fake! do + expect { project.rollback_to_legacy_storage! }.to change(ProjectRollbackHashedStorageWorker.jobs, :size).by(1) + end + end + end end describe '#gl_repository' do diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb new file mode 100644 index 00000000000..c9d0a085d21 --- /dev/null +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis_shared_state do + include GitHelpers + + let(:gitlab_shell) { Gitlab::Shell.new } + let(:project) { create(:project, :repository, :wiki_repo, storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository]) } + let(:legacy_storage) { Storage::LegacyProject.new(project) } + let(:hashed_storage) { Storage::HashedProject.new(project) } + + subject(:service) { described_class.new(project, project.disk_path) } + + describe '#execute' do + let(:old_disk_path) { hashed_storage.disk_path } + let(:new_disk_path) { legacy_storage.disk_path } + + before do + allow(service).to receive(:gitlab_shell) { gitlab_shell } + end + + context 'repository lock' do + it 'tries to lock the repository' do + expect(service).to receive(:try_to_set_repository_read_only!) + + service.execute + end + + it 'fails when a git operation is in progress' do + allow(project).to receive(:repo_reference_count) { 1 } + + expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryRollbackError) + end + end + + context 'when succeeds' do + it 'renames project and wiki repositories' do + service.execute + + expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy + expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy + end + + it 'updates project to be legacy and not read-only' do + service.execute + + expect(project.legacy_storage?).to be_truthy + expect(project.repository_read_only).to be_falsey + end + + it 'move operation is called for both repositories' do + expect_move_repository(old_disk_path, new_disk_path) + expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki") + + service.execute + end + + it 'writes project full path to .git/config' do + service.execute + + rugged_config = rugged_repo(project.repository).config['gitlab.fullpath'] + + expect(rugged_config).to eq project.full_path + end + end + + context 'when one move fails' do + it 'rollsback repositories to original name' do + allow(service).to receive(:move_repository).and_call_original + allow(service).to receive(:move_repository).with(old_disk_path, new_disk_path).once { false } # will disable first move only + + expect(service).to receive(:rollback_folder_move).and_call_original + + service.execute + + expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey + expect(project.repository_read_only?).to be_falsey + end + + context 'when rollback fails' do + before do + legacy_storage.ensure_storage_path_exists + gitlab_shell.mv_repository(project.repository_storage, old_disk_path, new_disk_path) + end + + it 'does not try to move nil repository over existing' do + expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, old_disk_path, new_disk_path) + expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki") + + service.execute + end + end + end + + def expect_move_repository(from_name, to_name) + expect(gitlab_shell).to receive(:mv_repository).with(project.repository_storage, from_name, to_name).and_call_original + end + end +end diff --git a/spec/services/projects/hashed_storage/rollback_service_spec.rb b/spec/services/projects/hashed_storage/rollback_service_spec.rb new file mode 100644 index 00000000000..427d1535559 --- /dev/null +++ b/spec/services/projects/hashed_storage/rollback_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::HashedStorage::RollbackService do + let(:project) { create(:project, :empty_repo, :wiki_repo) } + let(:logger) { double } + + subject(:service) { described_class.new(project, project.full_path, logger: logger) } + + describe '#execute' do + context 'attachments rollback' do + let(:attachments_service_class) { Projects::HashedStorage::RollbackAttachmentsService } + let(:attachments_service) { attachments_service_class.new(project, logger: logger) } + + it 'delegates rollback to Projects::HashedStorage::RollbackAttachmentsService' do + expect(attachments_service_class).to receive(:new) + .with(project, logger: logger) + .and_return(attachments_service) + expect(attachments_service).to receive(:execute) + + service.execute + end + + it 'does not delegate rollback if repository is in legacy storage already' do + project.storage_version = nil + expect(attachments_service_class).not_to receive(:new) + + service.execute + end + end + + context 'repository rollback' do + let(:repository_service_class) { Projects::HashedStorage::RollbackRepositoryService } + let(:repository_service) { repository_service_class.new(project, project.full_path, logger: logger) } + + it 'delegates rollback to RollbackRepositoryService' do + project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository] + + expect(repository_service_class).to receive(:new) + .with(project, project.full_path, logger: logger) + .and_return(repository_service) + expect(repository_service).to receive(:execute) + + service.execute + end + + it 'does not delegate rollback if repository is in legacy storage already' do + project.storage_version = nil + + expect(repository_service_class).not_to receive(:new) + + service.execute + end + 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 new file mode 100644 index 00000000000..aed7493763d --- /dev/null +++ b/spec/workers/project_rollback_hashed_storage_worker_spec.rb @@ -0,0 +1,50 @@ +# 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 From d63380fa93dff921c69f7aaa31ff004864e4db13 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 23 Jan 2019 03:31:57 +0100 Subject: [PATCH 03/10] 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 ---------------- .../gitlab/hashed_storage/migrator_spec.rb | 12 ++--- spec/models/project_spec.rb | 16 +++---- .../project_migrate_worker_spec.rb} | 2 +- .../project_rollback_worker_spec.rb} | 2 +- 10 files changed, 117 insertions(+), 107 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 rename spec/workers/{project_migrate_hashed_storage_worker_spec.rb => hashed_storage/project_migrate_worker_spec.rb} (94%) rename spec/workers/{project_rollback_hashed_storage_worker_spec.rb => hashed_storage/project_rollback_worker_spec.rb} (94%) 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/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/hashed_storage/project_migrate_worker_spec.rb similarity index 94% rename from spec/workers/project_migrate_hashed_storage_worker_spec.rb rename to spec/workers/hashed_storage/project_migrate_worker_spec.rb index 333eb6a0569..340e722aa7e 100644 --- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb +++ b/spec/workers/hashed_storage/project_migrate_worker_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do +describe HashedStorage::ProjectMigrateWorker, :clean_gitlab_redis_shared_state do include ExclusiveLeaseHelpers describe '#perform' do diff --git a/spec/workers/project_rollback_hashed_storage_worker_spec.rb b/spec/workers/hashed_storage/project_rollback_worker_spec.rb similarity index 94% rename from spec/workers/project_rollback_hashed_storage_worker_spec.rb rename to spec/workers/hashed_storage/project_rollback_worker_spec.rb index aed7493763d..d833553c0ec 100644 --- a/spec/workers/project_rollback_hashed_storage_worker_spec.rb +++ b/spec/workers/hashed_storage/project_rollback_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe ProjectRollbackHashedStorageWorker, :clean_gitlab_redis_shared_state do +describe HashedStorage::ProjectRollbackWorker, :clean_gitlab_redis_shared_state do include ExclusiveLeaseHelpers describe '#perform' do From fc0ff92807620c36d01f23eb0d7d88b02cb141c1 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 23 Jan 2019 03:40:05 +0100 Subject: [PATCH 04/10] Added Rollbacker workers and support on the rake task Rollback is done similar to Migration for the Hashed Storage. It also shares the same ExclusiveLease key to prevent both happening at the same time. All Hashed Storage related workers now share the same queue namespace which allows for assigning dedicated workers easily. --- .../migrate_attachments_service.rb | 21 +++- .../migrate_repository_service.rb | 2 +- .../rollback_attachments_service.rb | 28 ++++- .../rollback_repository_service.rb | 2 +- app/workers/all_queues.yml | 1 + .../hashed_storage/project_migrate_worker.rb | 4 +- .../hashed_storage/project_rollback_worker.rb | 4 +- .../hashed_storage/rollbacker_worker.rb | 16 +++ lib/gitlab/hashed_storage/migrator.rb | 27 ++++- lib/gitlab/hashed_storage/rake_helper.rb | 12 ++- lib/tasks/gitlab/storage.rake | 9 +- .../gitlab/hashed_storage/migrator_spec.rb | 60 +++++++++-- .../migrate_attachments_service_spec.rb | 2 + .../rollback_attachments_service_spec.rb | 100 ++++++++++++++++++ .../rollback_repository_service_spec.rb | 2 +- .../hashed_storage/rollbacker_worker_spec.rb | 27 +++++ 16 files changed, 292 insertions(+), 25 deletions(-) create mode 100644 app/workers/hashed_storage/rollbacker_worker.rb create mode 100644 spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb create mode 100644 spec/workers/hashed_storage/rollbacker_worker_spec.rb diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 03e0685d2cd..0df1e4ee130 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -5,13 +5,21 @@ module Projects AttachmentMigrationError = Class.new(StandardError) class MigrateAttachmentsService < BaseService - attr_reader :logger, :old_disk_path, :new_disk_path + # Returns the disk_path value before the execution + # This is used in EE for Geo + attr_reader :old_disk_path + + # Returns the diks_path value after the execution + # This is used in EE for Geo + attr_reader :new_disk_path + + # Returns the logger currently in use + attr_reader :logger def initialize(project, old_disk_path, logger: nil) @project = project @logger = logger || Rails.logger @old_disk_path = old_disk_path - @new_disk_path = project.disk_path @skipped = false end @@ -23,6 +31,8 @@ module Projects project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] target = FileUploader.absolute_base_dir(project) + @new_disk_path = project.disk_path + result = move_folder!(origin, target) project.save! @@ -33,6 +43,10 @@ module Projects result end + # Return whether this operation was skipped or not + # This is used in EE for Geo to decide if an event will be triggered or not + # + # @return [Boolean] true if skipped of false otherwise def skipped? @skipped end @@ -43,12 +57,13 @@ module Projects unless File.directory?(old_path) logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})") @skipped = true + return true end if File.exist?(new_path) logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") - raise AttachmentMigrationError, "Target path '#{new_path}' already exist" + raise AttachmentMigrationError, "Target path '#{new_path}' already exists" end # Create hashed storage base path folder diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index 9c672283c7e..a45d8ace2df 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -15,7 +15,7 @@ module Projects result = move_repository(old_disk_path, new_disk_path) if move_wiki - result &&= move_repository("#{old_wiki_disk_path}", "#{new_disk_path}.wiki") + result &&= move_repository(old_wiki_disk_path, "#{new_disk_path}.wiki") end if result diff --git a/app/services/projects/hashed_storage/rollback_attachments_service.rb b/app/services/projects/hashed_storage/rollback_attachments_service.rb index 5183609ab85..8d0de27d30b 100644 --- a/app/services/projects/hashed_storage/rollback_attachments_service.rb +++ b/app/services/projects/hashed_storage/rollback_attachments_service.rb @@ -5,11 +5,21 @@ module Projects AttachmentRollbackError = Class.new(StandardError) class RollbackAttachmentsService < BaseService - attr_reader :logger, :old_disk_path + # Returns the disk_path value before the execution + # This is used in EE for Geo + attr_reader :old_disk_path + + # Returns the diks_path value after the execution + # This is used in EE for Geo + attr_reader :new_disk_path + + # Returns the logger currently in use + attr_reader :logger def initialize(project, logger: nil) @project = project @logger = logger || Rails.logger + @old_disk_path = project.disk_path end def execute @@ -17,6 +27,8 @@ module Projects project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository] target = FileUploader.absolute_base_dir(project) + @new_disk_path = FileUploader.base_dir(project) + result = move_folder!(origin, target) project.save! @@ -27,24 +39,34 @@ module Projects result end + # Return whether this operation was skipped or not + # This is used in EE for Geo to decide if an event will be triggered or not + # + # @return [Boolean] true if skipped of false otherwise + def skipped? + @skipped + end + private def move_folder!(old_path, new_path) unless File.directory?(old_path) logger.info("Skipped attachments rollback from '#{old_path}' to '#{new_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})") + @skipped = true + return true end if File.exist?(new_path) logger.error("Cannot rollback attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") - raise AttachmentRollbackError, "Target path '#{new_path}' already exist" + raise AttachmentRollbackError, "Target path '#{new_path}' already exists" end # Create hashed storage base path folder FileUtils.mkdir_p(File.dirname(new_path)) FileUtils.mv(old_path, new_path) - logger.info("Rolledback project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") + logger.info("Rolled project attachments back from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") true end diff --git a/app/services/projects/hashed_storage/rollback_repository_service.rb b/app/services/projects/hashed_storage/rollback_repository_service.rb index b57d05c50ca..46956ed72c7 100644 --- a/app/services/projects/hashed_storage/rollback_repository_service.rb +++ b/app/services/projects/hashed_storage/rollback_repository_service.rb @@ -15,7 +15,7 @@ module Projects result = move_repository(old_disk_path, new_disk_path) if move_wiki - result &&= move_repository("#{old_wiki_disk_path}", "#{new_disk_path}.wiki") + result &&= move_repository(old_wiki_disk_path, "#{new_disk_path}.wiki") end if result diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index a5dce221883..d86f654dd44 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -47,6 +47,7 @@ - github_importer:github_import_stage_import_repository - hashed_storage:hashed_storage_migrator +- hashed_storage:hashed_storage_rollbacker - hashed_storage:hashed_storage_project_migrate - hashed_storage:hashed_storage_project_rollback diff --git a/app/workers/hashed_storage/project_migrate_worker.rb b/app/workers/hashed_storage/project_migrate_worker.rb index 05bf72f519a..d516929129f 100644 --- a/app/workers/hashed_storage/project_migrate_worker.rb +++ b/app/workers/hashed_storage/project_migrate_worker.rb @@ -14,8 +14,8 @@ module HashedStorage uuid = lease_for(project_id).try_obtain if uuid - project = Project.find_by(id: project_id) - return if project.nil? || project.pending_delete? + project = Project.without_deleted.find_by(id: project_id) + return unless project old_disk_path ||= project.disk_path diff --git a/app/workers/hashed_storage/project_rollback_worker.rb b/app/workers/hashed_storage/project_rollback_worker.rb index ace9fea98a6..c6ac76a1674 100644 --- a/app/workers/hashed_storage/project_rollback_worker.rb +++ b/app/workers/hashed_storage/project_rollback_worker.rb @@ -13,8 +13,8 @@ module HashedStorage uuid = lease_for(project_id).try_obtain if uuid - project = Project.find_by(id: project_id) - return if project.nil? || project.pending_delete? + project = Project.without_deleted.find_by(id: project_id) + return unless project old_disk_path ||= project.disk_path diff --git a/app/workers/hashed_storage/rollbacker_worker.rb b/app/workers/hashed_storage/rollbacker_worker.rb new file mode 100644 index 00000000000..a4da8443787 --- /dev/null +++ b/app/workers/hashed_storage/rollbacker_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module HashedStorage + class RollbackerWorker + include ApplicationWorker + + queue_namespace :hashed_storage + + # @param [Integer] start initial ID of the batch + # @param [Integer] finish last ID of the batch + def perform(start, finish) + migrator = Gitlab::HashedStorage::Migrator.new + migrator.bulk_rollback(start: start, finish: finish) + end + end +end diff --git a/lib/gitlab/hashed_storage/migrator.rb b/lib/gitlab/hashed_storage/migrator.rb index 73ed57ea584..7046b4e2a43 100644 --- a/lib/gitlab/hashed_storage/migrator.rb +++ b/lib/gitlab/hashed_storage/migrator.rb @@ -13,10 +13,18 @@ module Gitlab # # @param [Integer] start first project id for the range # @param [Integer] finish last project id for the range - def bulk_schedule(start:, finish:) + def bulk_schedule_migration(start:, finish:) ::HashedStorage::MigratorWorker.perform_async(start, finish) end + # Schedule a range of projects to be bulk rolledback with #bulk_rollback asynchronously + # + # @param [Integer] start first project id for the range + # @param [Integer] finish last project id for the range + def bulk_schedule_rollback(start:, finish:) + ::HashedStorage::RollbackerWorker.perform_async(start, finish) + end + # Start migration of projects from specified range # # Flagging a project to be migrated is a synchronous action @@ -34,6 +42,23 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord + # Start rollback of projects from specified range + # + # Flagging a project to be rolled back is a synchronous action + # but the rollback runs through async jobs + # + # @param [Integer] start first project id for the range + # @param [Integer] finish last project id for the range + # rubocop: disable CodeReuse/ActiveRecord + def bulk_rollback(start:, finish:) + projects = build_relation(start, finish) + + projects.with_route.find_each(batch_size: BATCH_SIZE) do |project| + rollback(project) + end + end + # rubocop: enable CodeReuse/ActiveRecord + # Flag a project to be migrated to Hashed Storage # # @param [Project] project that will be migrated diff --git a/lib/gitlab/hashed_storage/rake_helper.rb b/lib/gitlab/hashed_storage/rake_helper.rb index 38f552fab03..87a31a37e3f 100644 --- a/lib/gitlab/hashed_storage/rake_helper.rb +++ b/lib/gitlab/hashed_storage/rake_helper.rb @@ -24,7 +24,7 @@ module Gitlab end # rubocop: disable CodeReuse/ActiveRecord - def self.project_id_batches(&block) + def self.project_id_batches_migration(&block) Project.with_unmigrated_storage.in_batches(of: batch_size, start: range_from, finish: range_to) do |relation| # rubocop: disable Cop/InBatches ids = relation.pluck(:id) @@ -33,6 +33,16 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord + # rubocop: disable CodeReuse/ActiveRecord + def self.project_id_batches_rollback(&block) + Project.with_storage_feature(:repository).in_batches(of: batch_size, start: range_from, finish: range_to) do |relation| # rubocop: disable Cop/InBatches + ids = relation.pluck(:id) + + yield ids.min, ids.max + end + end + # rubocop: enable CodeReuse/ActiveRecord + # rubocop: disable CodeReuse/ActiveRecord def self.legacy_attachments_relation Upload.joins(<<~SQL).where('projects.storage_version < :version OR projects.storage_version IS NULL', version: Project::HASHED_STORAGE_FEATURES[:attachments]) diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake index b759a2dad3a..a2136ce1b92 100644 --- a/lib/tasks/gitlab/storage.rake +++ b/lib/tasks/gitlab/storage.rake @@ -36,8 +36,8 @@ namespace :gitlab do print "Enqueuing migration of #{legacy_projects_count} projects in batches of #{helper.batch_size}" - helper.project_id_batches do |start, finish| - storage_migrator.bulk_schedule(start: start, finish: finish) + helper.project_id_batches_migration do |start, finish| + storage_migrator.bulk_schedule_migration(start: start, finish: finish) print '.' end @@ -81,8 +81,9 @@ namespace :gitlab do print "Enqueuing rollback of #{hashed_projects_count} projects in batches of #{helper.batch_size}" - helper.project_id_batches do |start, finish| - storage_migrator.bulk_schedule(start: start, finish: finish, operation: :rollback) + helper.project_id_batches_rollback do |start, finish| + puts "Start: #{start} FINISH: #{finish}" + storage_migrator.bulk_schedule_rollback(start: start, finish: finish) print '.' end diff --git a/spec/lib/gitlab/hashed_storage/migrator_spec.rb b/spec/lib/gitlab/hashed_storage/migrator_spec.rb index 000913f32bb..6154b3e2f76 100644 --- a/spec/lib/gitlab/hashed_storage/migrator_spec.rb +++ b/spec/lib/gitlab/hashed_storage/migrator_spec.rb @@ -1,16 +1,24 @@ require 'spec_helper' describe Gitlab::HashedStorage::Migrator do - describe '#bulk_schedule' do - it 'schedules job to StorageMigratorWorker' do + describe '#bulk_schedule_migration' do + it 'schedules job to HashedStorage::MigratorWorker' do Sidekiq::Testing.fake! do - expect { subject.bulk_schedule(start: 1, finish: 5) }.to change(HashedStorage::MigratorWorker.jobs, :size).by(1) + expect { subject.bulk_schedule_migration(start: 1, finish: 5) }.to change(HashedStorage::MigratorWorker.jobs, :size).by(1) + end + end + end + + describe '#bulk_schedule_rollback' do + it 'schedules job to HashedStorage::RollbackerWorker' do + Sidekiq::Testing.fake! do + expect { subject.bulk_schedule_rollback(start: 1, finish: 5) }.to change(HashedStorage::RollbackerWorker.jobs, :size).by(1) end end end describe '#bulk_migrate' do - let(:projects) { create_list(:project, 2, :legacy_storage) } + let(:projects) { create_list(:project, 2, :legacy_storage, :empty_repo) } let(:ids) { projects.map(&:id) } it 'enqueue jobs to HashedStorage::ProjectMigrateWorker' do @@ -32,13 +40,53 @@ describe Gitlab::HashedStorage::Migrator do subject.bulk_migrate(start: ids.min, finish: ids.max) end - it 'has migrated projects set as writable' do + it 'has all projects migrated and set as writable' do perform_enqueued_jobs do subject.bulk_migrate(start: ids.min, finish: ids.max) end projects.each do |project| - expect(project.reload.repository_read_only?).to be_falsey + project.reload + + expect(project.hashed_storage?(:repository)).to be_truthy + expect(project.repository_read_only?).to be_falsey + end + end + end + + describe '#bulk_rollback' do + let(:projects) { create_list(:project, 2, :empty_repo) } + let(:ids) { projects.map(&:id) } + + it 'enqueue jobs to HashedStorage::ProjectRollbackWorker' do + Sidekiq::Testing.fake! do + expect { subject.bulk_rollback(start: ids.min, finish: ids.max) }.to change(HashedStorage::ProjectRollbackWorker.jobs, :size).by(2) + end + end + + it 'rescues and log exceptions' do + allow_any_instance_of(Project).to receive(:rollback_to_legacy_storage!).and_raise(StandardError) + expect { subject.bulk_rollback(start: ids.min, finish: ids.max) }.not_to raise_error + end + + it 'delegates each project in specified range to #rollback' do + projects.each do |project| + expect(subject).to receive(:rollback).with(project) + end + + subject.bulk_rollback(start: ids.min, finish: ids.max) + end + + it 'has all projects rolledback and set as writable' do + perform_enqueued_jobs do + subject.bulk_rollback(start: ids.min, finish: ids.max) + end + + projects.each do |project| + project.reload + + expect(project.legacy_storage?).to be_truthy + expect(project.repository_read_only?).to be_falsey end end end diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb index 61dbb57ec08..d51988c6f42 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -86,6 +86,8 @@ describe Projects::HashedStorage::MigrateAttachmentsService do context '#new_disk_path' do it 'returns new disk_path for project' do + service.execute + expect(service.new_disk_path).to eq(project.disk_path) end end diff --git a/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb new file mode 100644 index 00000000000..9e18428f412 --- /dev/null +++ b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::HashedStorage::RollbackAttachmentsService do + subject(:service) { described_class.new(project, logger: nil) } + + let(:project) { create(:project, :repository, skip_disk_validation: true) } + let(:legacy_storage) { Storage::LegacyProject.new(project) } + let(:hashed_storage) { Storage::HashedProject.new(project) } + + let!(:upload) { Upload.find_by(path: file_uploader.upload_path) } + let(:file_uploader) { build(:file_uploader, project: project) } + let(:old_disk_path) { File.join(base_path(hashed_storage), upload.path) } + let(:new_disk_path) { File.join(base_path(legacy_storage), upload.path) } + + context '#execute' do + context 'when succeeds' do + it 'moves attachments to legacy storage layout' do + expect(File.file?(old_disk_path)).to be_truthy + expect(File.file?(new_disk_path)).to be_falsey + expect(File.exist?(base_path(hashed_storage))).to be_truthy + expect(File.exist?(base_path(legacy_storage))).to be_falsey + expect(FileUtils).to receive(:mv).with(base_path(hashed_storage), base_path(legacy_storage)).and_call_original + + service.execute + + expect(File.exist?(base_path(legacy_storage))).to be_truthy + expect(File.exist?(base_path(hashed_storage))).to be_falsey + expect(File.file?(old_disk_path)).to be_falsey + expect(File.file?(new_disk_path)).to be_truthy + end + + it 'returns true' do + expect(service.execute).to be_truthy + end + + it 'sets skipped to false' do + service.execute + + expect(service.skipped?).to be_falsey + end + end + + context 'when original folder does not exist anymore' do + before do + FileUtils.rm_rf(base_path(hashed_storage)) + end + + it 'skips moving folders and go to next' do + expect(FileUtils).not_to receive(:mv).with(base_path(hashed_storage), base_path(legacy_storage)) + + service.execute + + expect(File.exist?(base_path(legacy_storage))).to be_falsey + expect(File.file?(new_disk_path)).to be_falsey + end + + it 'returns true' do + expect(service.execute).to be_truthy + end + + it 'sets skipped to true' do + service.execute + + expect(service.skipped?).to be_truthy + end + end + + context 'when target folder already exists' do + before do + FileUtils.mkdir_p(base_path(legacy_storage)) + end + + it 'raises AttachmentRollbackError' do + expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) + + expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentRollbackError) + end + end + end + + context '#old_disk_path' do + it 'returns old disk_path for project' do + expect(service.old_disk_path).to eq(project.disk_path) + end + end + + context '#new_disk_path' do + it 'returns new disk_path for project' do + service.execute + + expect(service.new_disk_path).to eq(project.full_path) + end + end + + def base_path(storage) + File.join(FileUploader.root, storage.disk_path) + end +end diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index c9d0a085d21..7c3a243b3ca 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -66,7 +66,7 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis end context 'when one move fails' do - it 'rollsback repositories to original name' do + it 'rolls repositories back to original name' do allow(service).to receive(:move_repository).and_call_original allow(service).to receive(:move_repository).with(old_disk_path, new_disk_path).once { false } # will disable first move only diff --git a/spec/workers/hashed_storage/rollbacker_worker_spec.rb b/spec/workers/hashed_storage/rollbacker_worker_spec.rb new file mode 100644 index 00000000000..4055f380978 --- /dev/null +++ b/spec/workers/hashed_storage/rollbacker_worker_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe HashedStorage::RollbackerWorker do + subject(:worker) { described_class.new } + let(:projects) { create_list(:project, 2, :empty_repo) } + let(:ids) { projects.map(&:id) } + + describe '#perform' do + it 'delegates to MigratorService' do + expect_any_instance_of(Gitlab::HashedStorage::Migrator).to receive(:bulk_rollback).with(start: 5, finish: 10) + + worker.perform(5, 10) + end + + it 'rollsback projects in the specified range' do + perform_enqueued_jobs do + worker.perform(ids.min, ids.max) + end + + projects.each do |project| + expect(project.reload.legacy_storage?).to be_truthy + end + end + end +end From 4bae61005dd835c7c8bf3ce911328a8e8af86a93 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 22 Feb 2019 09:12:06 +0100 Subject: [PATCH 05/10] Edge case: upgrade/downgrade when repository doesn't exist This change takes the case where repository doesn't exist on disk and make it ignore but succeed the step, increasing or decreasing the version depending on the operation. --- .../projects/hashed_storage/base_repository_service.rb | 3 ++- .../hashed_storage/migrate_repository_service_spec.rb | 10 ++++++++++ .../hashed_storage/rollback_repository_service_spec.rb | 10 ++++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/app/services/projects/hashed_storage/base_repository_service.rb b/app/services/projects/hashed_storage/base_repository_service.rb index 761c81d776f..00a6af5238e 100644 --- a/app/services/projects/hashed_storage/base_repository_service.rb +++ b/app/services/projects/hashed_storage/base_repository_service.rb @@ -38,7 +38,8 @@ module Projects # project was not originally empty. if !from_exists && !to_exists logger.warn "Can't find a repository on either source or target paths for #{project.full_path} (ID=#{project.id}) ..." - return false + + return true elsif !from_exists # Repository have been moved already. return true diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index 0772dc4b85b..cd56337420b 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -32,6 +32,16 @@ describe Projects::HashedStorage::MigrateRepositoryService do end end + context 'when repository doesnt exist on disk' do + let(:project) { create(:project, :legacy_storage) } + + it 'skips the disk change but increase the version' do + service.execute + + expect(project.hashed_storage?(:repository)).to be_truthy + end + end + context 'when succeeds' do it 'renames project and wiki repositories' do service.execute diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index 7c3a243b3ca..daac220f710 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -34,6 +34,16 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis end end + context 'when repository doesnt exist on disk' do + let(:project) { create(:project) } + + it 'skips the disk change but decrease the version' do + service.execute + + expect(project.legacy_storage?).to be_truthy + end + end + context 'when succeeds' do it 'renames project and wiki repositories' do service.execute From 7c8920c9dc6449f9d9a76dac619e6829398ec7db Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 27 Feb 2019 06:08:34 +0100 Subject: [PATCH 06/10] Extract common logic to HashedStorage::BaseWorker New class contains the ExclusiveLease specifics that is shared among both the Migration and Rollback workers. --- .../hashed_storage/base_repository_service.rb | 2 ++ app/workers/hashed_storage/base_worker.rb | 21 ++++++++++++ .../hashed_storage/project_migrate_worker.rb | 34 ++++--------------- .../hashed_storage/project_rollback_worker.rb | 33 ++++-------------- 4 files changed, 35 insertions(+), 55 deletions(-) create mode 100644 app/workers/hashed_storage/base_worker.rb diff --git a/app/services/projects/hashed_storage/base_repository_service.rb b/app/services/projects/hashed_storage/base_repository_service.rb index 00a6af5238e..a682df35d4c 100644 --- a/app/services/projects/hashed_storage/base_repository_service.rb +++ b/app/services/projects/hashed_storage/base_repository_service.rb @@ -39,6 +39,8 @@ module Projects if !from_exists && !to_exists logger.warn "Can't find a repository on either source or target paths for #{project.full_path} (ID=#{project.id}) ..." + # We return true so we still reflect the change in the database. + # Next time the repository is (re)created it will be under the new storage layout return true elsif !from_exists # Repository have been moved already. diff --git a/app/workers/hashed_storage/base_worker.rb b/app/workers/hashed_storage/base_worker.rb new file mode 100644 index 00000000000..816e0504db6 --- /dev/null +++ b/app/workers/hashed_storage/base_worker.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module HashedStorage + class BaseWorker + include ExclusiveLeaseGuard + + LEASE_TIMEOUT = 30.seconds.to_i + LEASE_KEY_SEGMENT = 'project_migrate_hashed_storage_worker'.freeze + + protected + + def lease_key + # we share the same lease key for both migration and rollback so they don't run simultaneously + "#{LEASE_KEY_SEGMENT}:#{project_id}" + end + + def lease_timeout + LEASE_TIMEOUT + end + end +end diff --git a/app/workers/hashed_storage/project_migrate_worker.rb b/app/workers/hashed_storage/project_migrate_worker.rb index d516929129f..f00a459a097 100644 --- a/app/workers/hashed_storage/project_migrate_worker.rb +++ b/app/workers/hashed_storage/project_migrate_worker.rb @@ -1,48 +1,26 @@ # frozen_string_literal: true module HashedStorage - class ProjectMigrateWorker + class ProjectMigrateWorker < BaseWorker include ApplicationWorker - LEASE_TIMEOUT = 30.seconds.to_i - LEASE_KEY_SEGMENT = 'project_migrate_hashed_storage_worker'.freeze - queue_namespace :hashed_storage + attr_reader :project_id + # rubocop: disable CodeReuse/ActiveRecord def perform(project_id, old_disk_path = nil) - uuid = lease_for(project_id).try_obtain + @project_id = project_id # we need to set this in order to create the lease_key - if uuid + try_obtain_lease do project = Project.without_deleted.find_by(id: project_id) - return unless project + break unless project 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 index c6ac76a1674..55e1d7ab23e 100644 --- a/app/workers/hashed_storage/project_rollback_worker.rb +++ b/app/workers/hashed_storage/project_rollback_worker.rb @@ -1,47 +1,26 @@ # frozen_string_literal: true module HashedStorage - class ProjectRollbackWorker + class ProjectRollbackWorker < BaseWorker include ApplicationWorker - LEASE_TIMEOUT = 30.seconds.to_i - queue_namespace :hashed_storage + attr_reader :project_id + # rubocop: disable CodeReuse/ActiveRecord def perform(project_id, old_disk_path = nil) - uuid = lease_for(project_id).try_obtain + @project_id = project_id # we need to set this in order to create the lease_key - if uuid + try_obtain_lease do project = Project.without_deleted.find_by(id: project_id) - return unless project + break unless project 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 From 264394f6d39c57e6863742ae0bf7c300156185ad Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 1 Mar 2019 07:57:11 +0100 Subject: [PATCH 07/10] Extract and simplify more code into BaseRepositoryService` `try_to_set_repository_read_only!` is now on the Base class. Simplified Exception classes from 2 to 1 with a more descriptive name. --- .../hashed_storage/base_repository_service.rb | 17 ++++++++++++----- .../migrate_repository_service.rb | 12 ------------ .../rollback_repository_service.rb | 12 ------------ .../migrate_repository_service_spec.rb | 2 +- .../rollback_repository_service_spec.rb | 2 +- 5 files changed, 14 insertions(+), 31 deletions(-) diff --git a/app/services/projects/hashed_storage/base_repository_service.rb b/app/services/projects/hashed_storage/base_repository_service.rb index a682df35d4c..f97a28b8c3b 100644 --- a/app/services/projects/hashed_storage/base_repository_service.rb +++ b/app/services/projects/hashed_storage/base_repository_service.rb @@ -2,11 +2,8 @@ module Projects module HashedStorage - # Returned when there is an error with the Hashed Storage migration - RepositoryMigrationError = Class.new(StandardError) - - # Returned when there is an error with the Hashed Storage rollback - RepositoryRollbackError = Class.new(StandardError) + # Returned when repository can't be made read-only because there is already a git transfer in progress + RepositoryInUseError = Class.new(StandardError) class BaseRepositoryService < BaseService include Gitlab::ShellAdapter @@ -55,6 +52,16 @@ module Projects move_repository(new_disk_path, old_disk_path) move_repository("#{new_disk_path}.wiki", old_wiki_disk_path) end + + def try_to_set_repository_read_only! + # Mitigate any push operation to start during migration + unless project.set_repository_read_only! + migration_error = "Target repository '#{old_disk_path}' cannot be made read-only as there is a git transfer in progress" + logger.error migration_error + + raise RepositoryInUseError, migration_error + end + end end end end diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index a45d8ace2df..5afa8732c0a 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -35,18 +35,6 @@ module Projects result end - - private - - def try_to_set_repository_read_only! - # Mitigate any push operation to start during migration - unless project.set_repository_read_only! - migration_error = "Target repository '#{old_disk_path}' cannot be made read-only as there is a git transfer in progress" - logger.error migration_error - - raise RepositoryMigrationError, migration_error - end - end end end end diff --git a/app/services/projects/hashed_storage/rollback_repository_service.rb b/app/services/projects/hashed_storage/rollback_repository_service.rb index 46956ed72c7..b5c971c70a5 100644 --- a/app/services/projects/hashed_storage/rollback_repository_service.rb +++ b/app/services/projects/hashed_storage/rollback_repository_service.rb @@ -35,18 +35,6 @@ module Projects result end - - private - - def try_to_set_repository_read_only! - # Mitigate any push operation to start during migration - unless project.set_repository_read_only! - migration_error = "Target repository '#{old_disk_path}' cannot be made read-only as there is a git transfer in progress" - logger.error migration_error - - raise RepositoryRollbackError, migration_error - end - end end end end diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index cd56337420b..e77e2198439 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -28,7 +28,7 @@ describe Projects::HashedStorage::MigrateRepositoryService do it 'fails when a git operation is in progress' do allow(project).to receive(:repo_reference_count) { 1 } - expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryMigrationError) + expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryInUseError) end end diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index daac220f710..41927934501 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -30,7 +30,7 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis it 'fails when a git operation is in progress' do allow(project).to receive(:repo_reference_count) { 1 } - expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryRollbackError) + expect { service.execute }.to raise_error(Projects::HashedStorage::RepositoryInUseError) end end From b91a005162515cfe398d90772ed66ecd92e17531 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 1 Mar 2019 07:58:05 +0100 Subject: [PATCH 08/10] Extract duplicated code into BaseAttachmentService Exceptions were also simplified from 2 to 1. --- .../hashed_storage/base_attachment_service.rb | 51 +++++++++++++++++++ .../migrate_attachments_service.rb | 50 ++---------------- .../rollback_attachments_service.rb | 47 +---------------- .../migrate_attachments_service_spec.rb | 4 +- .../rollback_attachments_service_spec.rb | 4 +- 5 files changed, 59 insertions(+), 97 deletions(-) create mode 100644 app/services/projects/hashed_storage/base_attachment_service.rb diff --git a/app/services/projects/hashed_storage/base_attachment_service.rb b/app/services/projects/hashed_storage/base_attachment_service.rb new file mode 100644 index 00000000000..828ab616bab --- /dev/null +++ b/app/services/projects/hashed_storage/base_attachment_service.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Projects + module HashedStorage + AttachmentMigrationError = Class.new(StandardError) + + AttachmentCannotMoveError = Class.new(StandardError) + + class BaseAttachmentService < BaseService + # Returns the disk_path value before the execution + attr_reader :old_disk_path + + # Returns the disk_path value after the execution + attr_reader :new_disk_path + + # Returns the logger currently in use + attr_reader :logger + + # Return whether this operation was skipped or not + # + # @return [Boolean] true if skipped of false otherwise + def skipped? + @skipped + end + + protected + + def move_folder!(old_path, new_path) + unless File.directory?(old_path) + logger.info("Skipped attachments move from '#{old_path}' to '#{new_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})") + @skipped = true + + return true + end + + if File.exist?(new_path) + logger.error("Cannot move attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") + raise AttachmentCannotMoveError, "Target path '#{new_path}' already exists" + end + + # Create base path folder on the new storage layout + FileUtils.mkdir_p(File.dirname(new_path)) + + FileUtils.mv(old_path, new_path) + logger.info("Project attachments moved from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") + + true + end + end + end +end diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 0df1e4ee130..2b8b7e459e0 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -2,20 +2,7 @@ module Projects module HashedStorage - AttachmentMigrationError = Class.new(StandardError) - - class MigrateAttachmentsService < BaseService - # Returns the disk_path value before the execution - # This is used in EE for Geo - attr_reader :old_disk_path - - # Returns the diks_path value after the execution - # This is used in EE for Geo - attr_reader :new_disk_path - - # Returns the logger currently in use - attr_reader :logger - + class MigrateAttachmentsService < BaseAttachmentService def initialize(project, old_disk_path, logger: nil) @project = project @logger = logger || Rails.logger @@ -25,7 +12,8 @@ module Projects def execute origin = FileUploader.absolute_base_dir(project) - # It's possible that old_disk_path does not match project.disk_path. For example, that happens when we rename a project + # It's possible that old_disk_path does not match project.disk_path. + # For example, that happens when we rename a project origin.sub!(/#{Regexp.escape(project.full_path)}\z/, old_disk_path) project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] @@ -42,38 +30,6 @@ module Projects result end - - # Return whether this operation was skipped or not - # This is used in EE for Geo to decide if an event will be triggered or not - # - # @return [Boolean] true if skipped of false otherwise - def skipped? - @skipped - end - - private - - def move_folder!(old_path, new_path) - unless File.directory?(old_path) - logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})") - @skipped = true - - return true - end - - if File.exist?(new_path) - logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") - raise AttachmentMigrationError, "Target path '#{new_path}' already exists" - end - - # Create hashed storage base path folder - FileUtils.mkdir_p(File.dirname(new_path)) - - FileUtils.mv(old_path, new_path) - logger.info("Migrated project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") - - true - end end end end diff --git a/app/services/projects/hashed_storage/rollback_attachments_service.rb b/app/services/projects/hashed_storage/rollback_attachments_service.rb index 8d0de27d30b..c35b8787dee 100644 --- a/app/services/projects/hashed_storage/rollback_attachments_service.rb +++ b/app/services/projects/hashed_storage/rollback_attachments_service.rb @@ -2,20 +2,7 @@ module Projects module HashedStorage - AttachmentRollbackError = Class.new(StandardError) - - class RollbackAttachmentsService < BaseService - # Returns the disk_path value before the execution - # This is used in EE for Geo - attr_reader :old_disk_path - - # Returns the diks_path value after the execution - # This is used in EE for Geo - attr_reader :new_disk_path - - # Returns the logger currently in use - attr_reader :logger - + class RollbackAttachmentsService < BaseAttachmentService def initialize(project, logger: nil) @project = project @logger = logger || Rails.logger @@ -38,38 +25,6 @@ module Projects result end - - # Return whether this operation was skipped or not - # This is used in EE for Geo to decide if an event will be triggered or not - # - # @return [Boolean] true if skipped of false otherwise - def skipped? - @skipped - end - - private - - def move_folder!(old_path, new_path) - unless File.directory?(old_path) - logger.info("Skipped attachments rollback from '#{old_path}' to '#{new_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})") - @skipped = true - - return true - end - - if File.exist?(new_path) - logger.error("Cannot rollback attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") - raise AttachmentRollbackError, "Target path '#{new_path}' already exists" - end - - # Create hashed storage base path folder - FileUtils.mkdir_p(File.dirname(new_path)) - - FileUtils.mv(old_path, new_path) - logger.info("Rolled project attachments back from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") - - true - end end end end diff --git a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb index d51988c6f42..639dd930618 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -70,10 +70,10 @@ describe Projects::HashedStorage::MigrateAttachmentsService do FileUtils.mkdir_p(base_path(hashed_storage)) end - it 'raises AttachmentMigrationError' do + it 'raises AttachmentCannotMoveError' do expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) - expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentMigrationError) + expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentCannotMoveError) end end end diff --git a/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb index 9e18428f412..6f4154d6011 100644 --- a/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_attachments_service_spec.rb @@ -72,10 +72,10 @@ describe Projects::HashedStorage::RollbackAttachmentsService do FileUtils.mkdir_p(base_path(legacy_storage)) end - it 'raises AttachmentRollbackError' do + it 'raises AttachmentCannotMoveError' do expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) - expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentRollbackError) + expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentCannotMoveError) end end end From 61d77a042da2ada48cdcd83f97923780aac407f4 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 1 Mar 2019 08:20:52 +0100 Subject: [PATCH 09/10] Migrate sidekiq queue to the new :hashed_storage namespace `project_migrate_hashed_storage` is now `hashed_storage:hashed_storage_project_migrate` --- ...611_migrate_project_migrate_sidekiq_queue.rb | 17 +++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 db/post_migrate/20190301081611_migrate_project_migrate_sidekiq_queue.rb diff --git a/db/post_migrate/20190301081611_migrate_project_migrate_sidekiq_queue.rb b/db/post_migrate/20190301081611_migrate_project_migrate_sidekiq_queue.rb new file mode 100644 index 00000000000..6af7902e0c4 --- /dev/null +++ b/db/post_migrate/20190301081611_migrate_project_migrate_sidekiq_queue.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class MigrateProjectMigrateSidekiqQueue < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + DOWNTIME = false + + def up + sidekiq_queue_migrate 'project_migrate_hashed_storage', to: 'hashed_storage:hashed_storage_project_migrate' + end + + def down + sidekiq_queue_migrate 'hashed_storage:hashed_storage_project_migrate', to: 'project_migrate_hashed_storage' + end +end diff --git a/db/schema.rb b/db/schema.rb index 24a98c374ca..2ddc8358433 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190228092516) do +ActiveRecord::Schema.define(version: 20190301081611) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" From 3524a618d61a401b589bf6025cb50e042d532a4f Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 1 Mar 2019 08:47:26 +0100 Subject: [PATCH 10/10] Improve migration/rollback logic for attachments --- .../hashed_storage/migrate_attachments_service.rb | 10 +++++++--- .../hashed_storage/rollback_attachments_service.rb | 10 +++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 2b8b7e459e0..9eaeb6eb4e7 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -22,10 +22,14 @@ module Projects @new_disk_path = project.disk_path result = move_folder!(origin, target) - project.save! - if result && block_given? - yield + if result + project.save! + + yield if block_given? + else + # Rollback changes + project.rollback! end result diff --git a/app/services/projects/hashed_storage/rollback_attachments_service.rb b/app/services/projects/hashed_storage/rollback_attachments_service.rb index c35b8787dee..6c370ac47e9 100644 --- a/app/services/projects/hashed_storage/rollback_attachments_service.rb +++ b/app/services/projects/hashed_storage/rollback_attachments_service.rb @@ -17,10 +17,14 @@ module Projects @new_disk_path = FileUploader.base_dir(project) result = move_folder!(origin, target) - project.save! - if result && block_given? - yield + if result + project.save! + + yield if block_given? + else + # Rollback changes + project.rollback! end result