From 0a4d55a1c9f58f383f23b8f60bbe1acba1b8511c Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Mon, 13 Nov 2017 15:15:42 +0100 Subject: [PATCH 01/11] Refactor Hashed Storage migration to add additional migration steps --- app/models/storage/hashed_project.rb | 1 - .../migrate_repository_service.rb | 66 +++++++++++++++++++ .../hashed_storage_migration_service.rb | 63 ++---------------- .../migrate_repository_service_spec.rb} | 2 +- 4 files changed, 73 insertions(+), 59 deletions(-) create mode 100644 app/services/projects/hashed_storage/migrate_repository_service.rb rename spec/services/projects/{hashed_storage_migration_service_spec.rb => hashed_storage/migrate_repository_service_spec.rb} (97%) diff --git a/app/models/storage/hashed_project.rb b/app/models/storage/hashed_project.rb index f025f40994e..fae1b64961a 100644 --- a/app/models/storage/hashed_project.rb +++ b/app/models/storage/hashed_project.rb @@ -4,7 +4,6 @@ module Storage delegate :gitlab_shell, :repository_storage_path, to: :project ROOT_PATH_PREFIX = '@hashed'.freeze - STORAGE_VERSION = 1 def initialize(project) @project = project diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb new file mode 100644 index 00000000000..c03db5cc1b9 --- /dev/null +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -0,0 +1,66 @@ +module Projects + module HashedStorage + class MigrateRepositoryService < BaseService + include Gitlab::ShellAdapter + + attr_reader :old_disk_path, :new_disk_path, :old_wiki_disk_path, :old_storage_version, :logger + + def initialize(project, logger = nil) + @project = project + @logger = logger || Rails.logger + end + + def execute + @old_disk_path = project.disk_path + has_wiki = project.wiki.repository_exists? + + @old_storage_version = project.storage_version + project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository] + project.ensure_storage_path_exists + + @new_disk_path = project.disk_path + + result = move_repository(@old_disk_path, @new_disk_path) + + if has_wiki + @old_wiki_disk_path = "#{@old_disk_path}.wiki" + result &&= move_repository("#{@old_wiki_disk_path}", "#{@new_disk_path}.wiki") + end + + unless result + rollback_folder_move + return result + end + + project.repository_read_only = false + project.save! + + result + end + + private + + def move_repository(from_name, to_name) + from_exists = gitlab_shell.exists?(project.repository_storage_path, "#{from_name}.git") + to_exists = gitlab_shell.exists?(project.repository_storage_path, "#{to_name}.git") + + # If we don't find the repository on either original or target we should log that as it could be an issue if the + # 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 + elsif !from_exists + # Repository have been moved already. + return true + end + + gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) + end + + def rollback_folder_move + move_repository(@new_disk_path, @old_disk_path) + move_repository("#{@new_disk_path}.wiki", "#{@old_disk_path}.wiki") + end + end + end +end diff --git a/app/services/projects/hashed_storage_migration_service.rb b/app/services/projects/hashed_storage_migration_service.rb index f5945f3b87f..b61f71cf9a0 100644 --- a/app/services/projects/hashed_storage_migration_service.rb +++ b/app/services/projects/hashed_storage_migration_service.rb @@ -1,68 +1,17 @@ module Projects class HashedStorageMigrationService < BaseService - include Gitlab::ShellAdapter - - attr_reader :old_disk_path, :new_disk_path - + attr_reader :logger + def initialize(project, logger = nil) @project = project - @logger ||= Rails.logger + @logger = logger || Rails.logger end def execute - return if project.hashed_storage?(:repository) - - @old_disk_path = project.disk_path - has_wiki = project.wiki.repository_exists? - - project.storage_version = Storage::HashedProject::STORAGE_VERSION - project.ensure_storage_path_exists - - @new_disk_path = project.disk_path - - result = move_repository(@old_disk_path, @new_disk_path) - - if has_wiki - result &&= move_repository("#{@old_disk_path}.wiki", "#{@new_disk_path}.wiki") + # Migrate repository from Legacy to Hashed Storage + unless project.hashed_storage?(:repository) + return unless HashedStorage::MigrateRepositoryService.new(project, logger).execute end - - unless result - rollback_folder_move - return - end - - project.repository_read_only = false - project.save! - - block_given? ? yield : result - end - - private - - def move_repository(from_name, to_name) - from_exists = gitlab_shell.exists?(project.repository_storage_path, "#{from_name}.git") - to_exists = gitlab_shell.exists?(project.repository_storage_path, "#{to_name}.git") - - # If we don't find the repository on either original or target we should log that as it could be an issue if the - # 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 - elsif !from_exists - # Repository have been moved already. - return true - end - - gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) - end - - def rollback_folder_move - move_repository(@new_disk_path, @old_disk_path) - move_repository("#{@new_disk_path}.wiki", "#{@old_disk_path}.wiki") - end - - def logger - @logger end end end diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb similarity index 97% rename from spec/services/projects/hashed_storage_migration_service_spec.rb rename to spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index b71b47c59b6..ef91d21cf20 100644 --- a/spec/services/projects/hashed_storage_migration_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::HashedStorageMigrationService do +describe Projects::HashedStorage::MigrateRepositoryService do let(:gitlab_shell) { Gitlab::Shell.new } let(:project) { create(:project, :empty_repo, :wiki_repo) } let(:service) { described_class.new(project) } From 4af26c1c65e4cc1d18a37acbc2af6ae29e91b336 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 15 Nov 2017 14:42:38 +0100 Subject: [PATCH 02/11] WIP Attachments migration --- .../migrate_attachments_service.rb | 54 ++++++++++++++ .../hashed_storage_migration_service.rb | 7 +- .../migrate_attachments_service_spec.rb | 72 +++++++++++++++++++ .../hashed_storage_migration_service_spec.rb | 40 +++++++++++ 4 files changed, 172 insertions(+), 1 deletion(-) create mode 100644 app/services/projects/hashed_storage/migrate_attachments_service.rb create mode 100644 spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb create mode 100644 spec/services/projects/hashed_storage_migration_service_spec.rb diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb new file mode 100644 index 00000000000..58a47da2fcb --- /dev/null +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -0,0 +1,54 @@ +module Projects + module HashedStorage + class MigrateAttachmentsService < BaseService + attr_reader :logger + + BATCH_SIZE = 500 + + def initialize(project, logger = nil) + @project = project + @logger = logger || Rails.logger + end + + def execute + project_before_migration = project.dup + project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] + + project.uploads.find_each(batch_size: BATCH_SIZE) do |upload| + old_path = attachments_path(project_before_migration, upload) + new_path = attachments_path(project, upload) + move_attachment(old_path, new_path) + end + + project.save! + end + + private + + def attachments_path(project, upload) + File.join( + FileUploader.dynamic_path_segment(project), + upload.path + ) + end + + def move_attachment(old_path, new_path) + unless File.file?(old_path) + logger.error("Failed to migrate attachment from '#{old_path}' to '#{new_path}', source file doesn't exist (PROJECT_ID=#{project.id})") + return + end + + # Create attachments folder if doesn't exist yet + FileUtils.mkdir_p(File.dirname(new_path)) unless Dir.exist?(File.dirname(new_path)) + + if File.file?(new_path) + logger.info("Skipped attachment migration from '#{old_path}' to '#{new_path}', target file already exist (PROJECT_ID=#{project.id})") + return + end + + FileUtils.mv(old_path, new_path) + logger.info("Migrated project attachment from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") + end + end + end +end diff --git a/app/services/projects/hashed_storage_migration_service.rb b/app/services/projects/hashed_storage_migration_service.rb index b61f71cf9a0..662702c1db5 100644 --- a/app/services/projects/hashed_storage_migration_service.rb +++ b/app/services/projects/hashed_storage_migration_service.rb @@ -1,7 +1,7 @@ module Projects class HashedStorageMigrationService < BaseService attr_reader :logger - + def initialize(project, logger = nil) @project = project @logger = logger || Rails.logger @@ -12,6 +12,11 @@ module Projects unless project.hashed_storage?(:repository) return unless HashedStorage::MigrateRepositoryService.new(project, logger).execute end + + # Migrate attachments from Legacy to Hashed Storage + unless project.hashed_storage?(:attachments) + HashedStorage::MigrateAttachmentsService.new(project, logger).execute + 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 new file mode 100644 index 00000000000..81f05074261 --- /dev/null +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -0,0 +1,72 @@ +require 'spec_helper' + +describe Projects::HashedStorage::MigrateAttachmentsService do + subject(:service) { described_class.new(project) } + let(:project) { create(:project) } + let(:legacy_storage) { Storage::LegacyProject.new(project) } + let(:hashed_storage) { Storage::HashedProject.new(project) } + + let!(:upload) { Upload.find_by(path: file_uploader.relative_path) } + let(:file_uploader) { build(:file_uploader, project: project) } + let(:old_path) { attachments_path(legacy_storage, upload) } + let(:new_path) { attachments_path(hashed_storage, upload) } + + let(:other_file_uploader) { build(:file_uploader, project: project) } + let(:other_old_path) { attachments_path(legacy_storage, other_upload) } + let(:other_new_path) { attachments_path(hashed_storage, other_upload) } + + context '#execute' do + context 'when succeeds' do + it 'moves attachments to hashed storage layout' do + expect(File.file?(old_path)).to be_truthy + expect(File.file?(new_path)).to be_falsey + + service.execute + + expect(File.file?(old_path)).to be_falsey + expect(File.file?(new_path)).to be_truthy + end + end + + context 'when original file does not exist anymore' do + let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } + + before do + File.unlink(old_path) + end + + it 'skips moving the file and goes to next' do + expect(FileUtils).not_to receive(:mv).with(old_path, new_path) + expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original + + service.execute + + expect(File.file?(new_path)).to be_falsey + expect(File.file?(other_new_path)).to be_truthy + end + end + + context 'when target file already exists' do + let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } + + before do + FileUtils.mkdir_p(File.dirname(new_path)) + FileUtils.touch(new_path) + end + + it 'skips moving the file and goes to next' do + expect(FileUtils).not_to receive(:mv).with(old_path, new_path) + expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original + expect(File.file?(new_path)).to be_truthy + + service.execute + + expect(File.file?(old_path)).to be_truthy + end + end + end + + def attachments_path(storage, upload) + File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path, upload.path) + end +end diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb new file mode 100644 index 00000000000..28b6daf217e --- /dev/null +++ b/spec/services/projects/hashed_storage_migration_service_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Projects::HashedStorageMigrationService do + let(:project) { create(:project, :empty_repo, :wiki_repo) } + subject(:service) { described_class.new(project) } + + describe '#execute' do + context 'repository migration' do + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do + expect(Projects::HashedStorage::MigrateRepositoryService).to receive(:new).with(project, subject.logger).and_call_original + expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).to receive(:execute) + + service.execute + end + + it 'does not delegate migration if repository is already migrated' do + project.storage_version = ::Project::LATEST_STORAGE_VERSION + expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:execute) + + service.execute + end + end + + context 'attachments migration' do + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do + expect(Projects::HashedStorage::MigrateAttachmentsService).to receive(:new).with(project, subject.logger).and_call_original + expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).to receive(:execute) + + service.execute + end + + it 'does not delegate migration if attachments are already migrated' do + project.storage_version = ::Project::LATEST_STORAGE_VERSION + expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:execute) + + service.execute + end + end + end +end From d0a08ab888db33437c7df4eb37b5805757a6dce4 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sat, 18 Nov 2017 07:26:07 +0100 Subject: [PATCH 03/11] Improve storage migration rake task --- app/models/project.rb | 5 +- lib/tasks/gitlab/storage.rake | 87 ++++++++++++++++++++++++++++------- 2 files changed, 74 insertions(+), 18 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 894ded2a9f6..f20c0688bc2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -272,8 +272,9 @@ class Project < ActiveRecord::Base scope :pending_delete, -> { where(pending_delete: true) } scope :without_deleted, -> { where(pending_delete: false) } - scope :with_hashed_storage, -> { where('storage_version >= 1') } - scope :with_legacy_storage, -> { where(storage_version: [nil, 0]) } + scope :with_storage_feature, ->(feature) { where('storage_version >= :version', version: HASHED_STORAGE_FEATURES[feature]) } + scope :without_storage_feature, ->(feature) { where('storage_version < :version OR storage_version IS NULL', version: HASHED_STORAGE_FEATURES[feature]) } + scope :with_unmigrated_storage, -> { where('storage_version < :version OR storage_version IS NULL', version: LATEST_STORAGE_VERSION) } scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) } scope :sorted_by_stars, -> { reorder('projects.star_count DESC') } diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake index e05be4a3405..8ac73bc8ff2 100644 --- a/lib/tasks/gitlab/storage.rake +++ b/lib/tasks/gitlab/storage.rake @@ -2,10 +2,10 @@ namespace :gitlab do namespace :storage do desc 'GitLab | Storage | Migrate existing projects to Hashed Storage' task migrate_to_hashed: :environment do - legacy_projects_count = Project.with_legacy_storage.count + legacy_projects_count = Project.with_unmigrated_storage.count if legacy_projects_count == 0 - puts 'There are no projects using legacy storage. Nothing to do!' + puts 'There are no projects requiring storage migration. Nothing to do!' next end @@ -23,22 +23,42 @@ namespace :gitlab do desc 'Gitlab | Storage | Summary of existing projects using Legacy Storage' task legacy_projects: :environment do - projects_summary(Project.with_legacy_storage) + relation_summary('projects', Project.without_storage_feature(:repository)) end desc 'Gitlab | Storage | List existing projects using Legacy Storage' task list_legacy_projects: :environment do - projects_list(Project.with_legacy_storage) + projects_list('projects using Legacy Storage', Project.without_storage_feature(:repository)) end desc 'Gitlab | Storage | Summary of existing projects using Hashed Storage' task hashed_projects: :environment do - projects_summary(Project.with_hashed_storage) + relation_summary('projects using Hashed Storage', Project.with_storage_feature(:repository)) end desc 'Gitlab | Storage | List existing projects using Hashed Storage' task list_hashed_projects: :environment do - projects_list(Project.with_hashed_storage) + projects_list('projects using Hashed Storage', Project.with_storage_feature(:repository)) + end + + desc 'Gitlab | Storage | Summary of project attachments using Legacy Storage' + task legacy_attachments: :environment do + relation_summary('attachments using Legacy Storage', legacy_attachments_relation) + end + + desc 'Gitlab | Storage | List existing project attachments using Legacy Storage' + task list_legacy_attachments: :environment do + attachments_list('attachments using Legacy Storage', legacy_attachments_relation) + end + + desc 'Gitlab | Storage | Summary of project attachments using Hashed Storage' + task hashed_attachments: :environment do + relation_summary('attachments using Hashed Storage', hashed_attachments_relation) + end + + desc 'Gitlab | Storage | List existing project attachments using Hashed Storage' + task list_hashed_attachments: :environment do + attachments_list('attachments using Hashed Storage', hashed_attachments_relation) end def batch_size @@ -46,29 +66,43 @@ namespace :gitlab do end def project_id_batches(&block) - Project.with_legacy_storage.in_batches(of: batch_size, start: ENV['ID_FROM'], finish: ENV['ID_TO']) do |relation| # rubocop: disable Cop/InBatches + Project.with_unmigrated_storage.in_batches(of: batch_size, start: ENV['ID_FROM'], finish: ENV['ID_TO']) do |relation| # rubocop: disable Cop/InBatches ids = relation.pluck(:id) yield ids.min, ids.max end end - def projects_summary(relation) - projects_count = relation.count - puts "* Found #{projects_count} projects".color(:green) - - projects_count + def legacy_attachments_relation + Upload.joins(<<~SQL).where('projects.storage_version < :version OR projects.storage_version IS NULL', version: Project::HASHED_STORAGE_FEATURES[:attachments]) + JOIN projects + ON (uploads.model_type='Project' AND uploads.model_id=projects.id) + SQL end - def projects_list(relation) - projects_count = projects_summary(relation) + def hashed_attachments_relation + Upload.joins(<<~SQL).where('projects.storage_version >= :version', version: Project::HASHED_STORAGE_FEATURES[:attachments]) + JOIN projects + ON (uploads.model_type='Project' AND uploads.model_id=projects.id) + SQL + end + + def relation_summary(relation_name, relation) + relation_count = relation.count + puts "* Found #{relation_count} #{relation_name}".color(:green) + + relation_count + end + + def projects_list(relation_name, relation) + relation_count = relation_summary(relation_name, relation) projects = relation.with_route limit = ENV.fetch('LIMIT', 500).to_i - return unless projects_count > 0 + return unless relation_count > 0 - puts " ! Displaying first #{limit} projects..." if projects_count > limit + puts " ! Displaying first #{limit} #{relation_name}..." if relation_count > limit counter = 0 projects.find_in_batches(batch_size: batch_size) do |batch| @@ -81,5 +115,26 @@ namespace :gitlab do end end end + + def attachments_list(relation_name, relation) + relation_count = relation_summary(relation_name, relation) + + limit = ENV.fetch('LIMIT', 500).to_i + + return unless relation_count > 0 + + puts " ! Displaying first #{limit} #{relation_name}..." if relation_count > limit + + counter = 0 + relation.find_in_batches(batch_size: batch_size) do |batch| + batch.each do |upload| + counter += 1 + + puts " - #{upload.path} (id: #{upload.id})".color(:red) + + return if counter >= limit # rubocop:disable Lint/NonLocalExitFromIterator + end + end + end end end From f0f6a237d7e95fcc5d52e85aef151f0327bf2fdc Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Mon, 20 Nov 2017 23:35:17 +0100 Subject: [PATCH 04/11] attachments migration should move only the base folder --- .../migrate_attachments_service.rb | 37 ++++++---------- .../migrate_attachments_service_spec.rb | 42 ++++++++----------- 2 files changed, 30 insertions(+), 49 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 58a47da2fcb..68b9a72661c 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -3,51 +3,38 @@ module Projects class MigrateAttachmentsService < BaseService attr_reader :logger - BATCH_SIZE = 500 - def initialize(project, logger = nil) @project = project @logger = logger || Rails.logger end def execute - project_before_migration = project.dup + old_path = FileUploader.dynamic_path_segment(project) project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] + new_path = FileUploader.dynamic_path_segment(project) - project.uploads.find_each(batch_size: BATCH_SIZE) do |upload| - old_path = attachments_path(project_before_migration, upload) - new_path = attachments_path(project, upload) - move_attachment(old_path, new_path) - end - + move_folder!(old_path, new_path) project.save! end private - def attachments_path(project, upload) - File.join( - FileUploader.dynamic_path_segment(project), - upload.path - ) - end - - def move_attachment(old_path, new_path) - unless File.file?(old_path) - logger.error("Failed to migrate attachment from '#{old_path}' to '#{new_path}', source file doesn't exist (PROJECT_ID=#{project.id})") + def move_folder!(old_path, new_path) + unless File.exist?(old_path) + logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist (PROJECT_ID=#{project.id})") return end - # Create attachments folder if doesn't exist yet - FileUtils.mkdir_p(File.dirname(new_path)) unless Dir.exist?(File.dirname(new_path)) - - if File.file?(new_path) - logger.info("Skipped attachment migration from '#{old_path}' to '#{new_path}', target file already exist (PROJECT_ID=#{project.id})") + if File.exist?(new_path) + logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") return end + # Create hashed storage base path folder + FileUtils.mkdir_p(File.expand_path('..', new_path)) + FileUtils.mv(old_path, new_path) - logger.info("Migrated project attachment from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") + logger.info("Migrated project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") 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 81f05074261..ce43a7e4d54 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -8,65 +8,59 @@ describe Projects::HashedStorage::MigrateAttachmentsService do let!(:upload) { Upload.find_by(path: file_uploader.relative_path) } let(:file_uploader) { build(:file_uploader, project: project) } - let(:old_path) { attachments_path(legacy_storage, upload) } - let(:new_path) { attachments_path(hashed_storage, upload) } - - let(:other_file_uploader) { build(:file_uploader, project: project) } - let(:other_old_path) { attachments_path(legacy_storage, other_upload) } - let(:other_new_path) { attachments_path(hashed_storage, other_upload) } + let(:old_path) { File.join(base_path(legacy_storage), upload.path) } + let(:new_path) { File.join(base_path(hashed_storage), upload.path) } context '#execute' do context 'when succeeds' do it 'moves attachments to hashed storage layout' do expect(File.file?(old_path)).to be_truthy expect(File.file?(new_path)).to be_falsey + expect(File.exist?(base_path(legacy_storage))).to be_truthy + expect(File.exist?(base_path(hashed_storage))).to be_falsey + expect(FileUtils).to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)).and_call_original service.execute + expect(File.exist?(base_path(hashed_storage))).to be_truthy + expect(File.exist?(base_path(legacy_storage))).to be_falsey expect(File.file?(old_path)).to be_falsey expect(File.file?(new_path)).to be_truthy end end - context 'when original file does not exist anymore' do - let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } - + context 'when original folder does not exist anymore' do before do - File.unlink(old_path) + FileUtils.rm_rf(base_path(legacy_storage)) end - it 'skips moving the file and goes to next' do - expect(FileUtils).not_to receive(:mv).with(old_path, new_path) - expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original + it 'skips moving folders and go to next' do + expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) service.execute + expect(File.exist?(base_path(hashed_storage))).to be_falsey expect(File.file?(new_path)).to be_falsey - expect(File.file?(other_new_path)).to be_truthy end end - context 'when target file already exists' do - let!(:other_upload) { Upload.find_by(path: other_file_uploader.relative_path) } - + context 'when target folder already exists' do before do - FileUtils.mkdir_p(File.dirname(new_path)) - FileUtils.touch(new_path) + FileUtils.mkdir_p(base_path(hashed_storage)) end it 'skips moving the file and goes to next' do - expect(FileUtils).not_to receive(:mv).with(old_path, new_path) - expect(FileUtils).to receive(:mv).with(other_old_path, other_new_path).and_call_original - expect(File.file?(new_path)).to be_truthy + expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) service.execute + expect(File.exist?(base_path(legacy_storage))).to be_truthy expect(File.file?(old_path)).to be_truthy end end end - def attachments_path(storage, upload) - File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path, upload.path) + def base_path(storage) + File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path) end end From 9fd31eb469de5099f7a7b44840d7930bc3c42bbe Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 21 Nov 2017 00:23:43 +0100 Subject: [PATCH 05/11] Raises error when migration cannot happen so job is cancelled --- .../projects/hashed_storage/migrate_attachments_service.rb | 4 +++- .../hashed_storage/migrate_attachments_service_spec.rb | 7 ++----- 2 files changed, 5 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 68b9a72661c..26026899ebe 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -1,5 +1,7 @@ module Projects module HashedStorage + AttachmentMigrationError = Class.new(StandardError) + class MigrateAttachmentsService < BaseService attr_reader :logger @@ -27,7 +29,7 @@ module Projects if File.exist?(new_path) logger.error("Cannot migrate attachments from '#{old_path}' to '#{new_path}', target path already exist (PROJECT_ID=#{project.id})") - return + raise AttachmentMigrationError, "Target path '#{new_path}' already exist" end # Create hashed storage base path folder 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 ce43a7e4d54..de2abfc1985 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -49,13 +49,10 @@ describe Projects::HashedStorage::MigrateAttachmentsService do FileUtils.mkdir_p(base_path(hashed_storage)) end - it 'skips moving the file and goes to next' do + it 'raises AttachmentMigrationError' do expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage)) - service.execute - - expect(File.exist?(base_path(legacy_storage))).to be_truthy - expect(File.file?(old_path)).to be_truthy + expect { service.execute }.to raise_error(Projects::HashedStorage::AttachmentMigrationError) end end end From 4b87c1afaa652d72fa6aeeb4fe52fa3883e2f4c8 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Tue, 21 Nov 2017 16:38:32 +0100 Subject: [PATCH 06/11] when rollingback repository migration, toggle readonly mode back --- .../hashed_storage/migrate_attachments_service.rb | 4 ++-- .../projects/hashed_storage/migrate_repository_service.rb | 2 +- .../hashed_storage/migrate_repository_service_spec.rb | 8 +++++--- 3 files changed, 8 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 26026899ebe..b58b6f57ed7 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -22,8 +22,8 @@ module Projects private def move_folder!(old_path, new_path) - unless File.exist?(old_path) - logger.info("Skipped attachments migration from '#{old_path}' to '#{new_path}', source path doesn't exist (PROJECT_ID=#{project.id})") + 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})") return end diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index c03db5cc1b9..da43877a185 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -29,7 +29,7 @@ module Projects unless result rollback_folder_move - return result + project.storage_version = nil end project.repository_read_only = false 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 ef91d21cf20..3a3e47fd9c0 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -48,18 +48,20 @@ describe Projects::HashedStorage::MigrateRepositoryService do expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.git")).to be_falsey expect(gitlab_shell.exists?(project.repository_storage_path, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey + expect(project.repository_read_only?).to be_falsey end context 'when rollback fails' do - before do - from_name = legacy_storage.disk_path - to_name = hashed_storage.disk_path + let(:from_name) { legacy_storage.disk_path } + let(:to_name) { hashed_storage.disk_path } + before do hashed_storage.ensure_storage_path_exists gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name) end it 'does not try to move nil repository over hashed' do + expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage_path, from_name, to_name) expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") service.execute From 65bd6868d014e23c21e4d5ecff468124b2c72f4c Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 22 Nov 2017 06:35:53 +0100 Subject: [PATCH 07/11] Codestyle changes and Added Exclusive Lease to hashed storage migration --- .../migrate_attachments_service.rb | 2 +- .../project_migrate_hashed_storage_worker.rb | 26 ++++++++++- ...ject_migrate_hashed_storage_worker_spec.rb | 46 +++++++++++++------ 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index b58b6f57ed7..93f44110605 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -33,7 +33,7 @@ module Projects end # Create hashed storage base path folder - FileUtils.mkdir_p(File.expand_path('..', new_path)) + 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})") diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb index ca276d7801c..e3ecd6bc950 100644 --- a/app/workers/project_migrate_hashed_storage_worker.rb +++ b/app/workers/project_migrate_hashed_storage_worker.rb @@ -2,10 +2,34 @@ class ProjectMigrateHashedStorageWorker include Sidekiq::Worker include DedicatedSidekiqQueue + LEASE_TIMEOUT = 30.seconds.to_i + def perform(project_id) project = Project.find_by(id: project_id) return if project.nil? || project.pending_delete? - ::Projects::HashedStorageMigrationService.new(project, logger).execute + uuid = try_obtain_lease_for(project_id) + if uuid + ::Projects::HashedStorageMigrationService.new(project, logger).execute + else + false + end + rescue => ex + cancel_lease_for(project_id, uuid) + raise ex + end + + private + + def try_obtain_lease_for(project_id) + Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT).try_obtain + end + + def lease_key(project_id) + "project_migrate_hashed_storage_worker:#{project_id}" + end + + def cancel_lease_for(project_id, uuid) + Gitlab::ExclusiveLease.cancel(lease_key(project_id), uuid) end end diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb index f5226dee0ad..8cacdfa7173 100644 --- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb +++ b/spec/workers/project_migrate_hashed_storage_worker_spec.rb @@ -5,25 +5,43 @@ describe ProjectMigrateHashedStorageWorker do let(:project) { create(:project, :empty_repo) } let(:pending_delete_project) { create(:project, :empty_repo, pending_delete: true) } - it 'skips when project no longer exists' do - nonexistent_id = 999999999999 + context 'when have exclusive lease' do + before do + allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true) + end - expect(::Projects::HashedStorageMigrationService).not_to receive(:new) - subject.perform(nonexistent_id) + it 'skips when project no longer exists' do + nonexistent_id = 999999999999 + + expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + subject.perform(nonexistent_id) + end + + it 'skips when project is pending delete' do + expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + + subject.perform(pending_delete_project.id) + end + + it 'delegates removal to service class' do + service = double('service') + expect(::Projects::HashedStorageMigrationService).to receive(:new).with(project, subject.logger).and_return(service) + expect(service).to receive(:execute) + + subject.perform(project.id) + end end - it 'skips when project is pending delete' do - expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + context 'when dont have exclusive lease' do + before do + allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false) + end - subject.perform(pending_delete_project.id) - end + it 'skips when dont have lease' do + expect(::Projects::HashedStorageMigrationService).not_to receive(:new) - it 'delegates removal to service class' do - service = double('service') - expect(::Projects::HashedStorageMigrationService).to receive(:new).with(project, subject.logger).and_return(service) - expect(service).to receive(:execute) - - subject.perform(project.id) + subject.perform(project.id) + end end end end From 10c2ba7dbbae15cde019135fe89b20dbd793dadf Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sat, 18 Nov 2017 08:12:37 +0100 Subject: [PATCH 08/11] Changelog and Documentation for storage migration of project attachments --- ...hed-storage-attachments-migration-path.yml | 5 + doc/administration/raketasks/storage.md | 99 ++++++++++++++++--- 2 files changed, 90 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/hashed-storage-attachments-migration-path.yml diff --git a/changelogs/unreleased/hashed-storage-attachments-migration-path.yml b/changelogs/unreleased/hashed-storage-attachments-migration-path.yml new file mode 100644 index 00000000000..32535437046 --- /dev/null +++ b/changelogs/unreleased/hashed-storage-attachments-migration-path.yml @@ -0,0 +1,5 @@ +--- +title: Hashed Storage migration script now supports migrating project attachments +merge_request: 15352 +author: +type: added diff --git a/doc/administration/raketasks/storage.md b/doc/administration/raketasks/storage.md index bac8fa4bd9d..6ec5baeb6e3 100644 --- a/doc/administration/raketasks/storage.md +++ b/doc/administration/raketasks/storage.md @@ -1,10 +1,43 @@ # Repository Storage Rake Tasks This is a collection of rake tasks you can use to help you list and migrate -existing projects from Legacy storage to the new Hashed storage type. +existing projects and attachments associated with it from Legacy storage to +the new Hashed storage type. You can read more about the storage types [here][storage-types]. +## Migrate existing projects to Hashed storage + +Before migrating your existing projects, you should +[enable hashed storage][storage-migration] for the new projects as well. + +This task will schedule all your existing projects and attachments associated with it to be migrated to the +**Hashed** storage type: + +**Omnibus Installation** + +```bash +gitlab-rake gitlab:storage:migrate_to_hashed +``` + +**Source Installation** + +```bash +rake gitlab:storage:migrate_to_hashed + +``` + +You can monitor the progress in the _Admin > Monitoring > Background jobs_ screen. +There is a specific Queue you can watch to see how long it will take to finish: **project_migrate_hashed_storage** + +After it reaches zero, you can confirm every project has been migrated by running the commands bellow. +If you find it necessary, you can run this migration script again to schedule missing projects. + +Any error or warning will be logged in the sidekiq's log file. + +You only need the `gitlab:storage:migrate_to_hashed` rake task to migrate your repositories, but we have additional +commands below that helps you inspect projects and attachments in both legacy and hashed storage. + ## List projects on Legacy storage To have a simple summary of projects using **Legacy** storage: @@ -73,35 +106,73 @@ rake gitlab:storage:list_hashed_projects ``` -## Migrate existing projects to Hashed storage +## List attachments on Legacy storage -Before migrating your existing projects, you should -[enable hashed storage][storage-migration] for the new projects as well. - -This task will schedule all your existing projects to be migrated to the -**Hashed** storage type: +To have a simple summary of project attachments using **Legacy** storage: **Omnibus Installation** ```bash -gitlab-rake gitlab:storage:migrate_to_hashed +gitlab-rake gitlab:storage:legacy_attachments ``` **Source Installation** ```bash -rake gitlab:storage:migrate_to_hashed +rake gitlab:storage:legacy_attachments ``` -You can monitor the progress in the _Admin > Monitoring > Background jobs_ screen. -There is a specific Queue you can watch to see how long it will take to finish: **project_migrate_hashed_storage** +------ -After it reaches zero, you can confirm every project has been migrated by running the commands above. -If you find it necessary, you can run this migration script again to schedule missing projects. +To list project attachments using **Legacy** storage: -Any error or warning will be logged in the sidekiq log file. +**Omnibus Installation** +```bash +gitlab-rake gitlab:storage:list_legacy_attachments +``` + +**Source Installation** + +```bash +rake gitlab:storage:list_legacy_attachments + +``` + +## List attachments on Hashed storage + +To have a simple summary of project attachments using **Hashed** storage: + +**Omnibus Installation** + +```bash +gitlab-rake gitlab:storage:hashed_attachments +``` + +**Source Installation** + +```bash +rake gitlab:storage:hashed_attachments + +``` + +------ + +To list project attachments using **Hashed** storage: + +**Omnibus Installation** + +```bash +gitlab-rake gitlab:storage:list_hashed_attachments +``` + +**Source Installation** + +```bash +rake gitlab:storage:list_hashed_attachments + +``` [storage-types]: ../repository_storage_types.md [storage-migration]: ../repository_storage_types.md#how-to-migrate-to-hashed-storage From 05876a49c67fd94777801c09129e7dd77873e668 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 22 Nov 2017 16:29:03 +0100 Subject: [PATCH 09/11] fix exclusive lease specs fro hashed storage migration --- app/workers/project_migrate_hashed_storage_worker.rb | 12 ++++++------ .../project_migrate_hashed_storage_worker_spec.rb | 12 +++++++++--- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb index e3ecd6bc950..127aa6b9d7d 100644 --- a/app/workers/project_migrate_hashed_storage_worker.rb +++ b/app/workers/project_migrate_hashed_storage_worker.rb @@ -8,23 +8,23 @@ class ProjectMigrateHashedStorageWorker project = Project.find_by(id: project_id) return if project.nil? || project.pending_delete? - uuid = try_obtain_lease_for(project_id) + uuid = lease_for(project_id).try_obtain if uuid ::Projects::HashedStorageMigrationService.new(project, logger).execute else false end rescue => ex - cancel_lease_for(project_id, uuid) + cancel_lease_for(project_id, uuid) if uuid raise ex end - private - - def try_obtain_lease_for(project_id) - Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT).try_obtain + def lease_for(project_id) + Gitlab::ExclusiveLease.new(lease_key(project_id), timeout: LEASE_TIMEOUT) end + private + def lease_key(project_id) "project_migrate_hashed_storage_worker:#{project_id}" end diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb index 8cacdfa7173..2e3951e7afc 100644 --- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb +++ b/spec/workers/project_migrate_hashed_storage_worker_spec.rb @@ -1,13 +1,16 @@ require 'spec_helper' -describe ProjectMigrateHashedStorageWorker do +describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do describe '#perform' do let(:project) { create(:project, :empty_repo) } let(:pending_delete_project) { create(:project, :empty_repo, pending_delete: true) } context 'when have exclusive lease' do before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true) + lease = subject.lease_for(project.id) + + allow(Gitlab::ExclusiveLease).to receive(:new).and_return(lease) + allow(lease).to receive(:try_obtain).and_return(true) end it 'skips when project no longer exists' do @@ -34,7 +37,10 @@ describe ProjectMigrateHashedStorageWorker do context 'when dont have exclusive lease' do before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false) + lease = subject.lease_for(project.id) + + allow(Gitlab::ExclusiveLease).to receive(:new).and_return(lease) + allow(lease).to receive(:try_obtain).and_return(false) end it 'skips when dont have lease' do From dc62441ffd47d63f342c6accbd5049f6e8f99303 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 24 Nov 2017 03:31:08 +0100 Subject: [PATCH 10/11] Backport EE changes to make test possible when prepending modules --- .../migrate_attachments_service.rb | 4 ++++ .../hashed_storage/migrate_repository_service.rb | 4 ++++ .../hashed_storage_migration_service_spec.rb | 16 ++++++++++------ 3 files changed, 18 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 93f44110605..8cac6221a96 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -17,6 +17,10 @@ module Projects move_folder!(old_path, new_path) project.save! + + if block_given? + yield + end end private diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index da43877a185..7212e7524ab 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -35,6 +35,10 @@ module Projects project.repository_read_only = false project.save! + if result && block_given? + yield + end + result end diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb index 28b6daf217e..466f0b5d7c2 100644 --- a/spec/services/projects/hashed_storage_migration_service_spec.rb +++ b/spec/services/projects/hashed_storage_migration_service_spec.rb @@ -6,32 +6,36 @@ describe Projects::HashedStorageMigrationService do describe '#execute' do context 'repository migration' do + let(:repository_service) { Projects::HashedStorage::MigrateRepositoryService.new(project, subject.logger) } + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do - expect(Projects::HashedStorage::MigrateRepositoryService).to receive(:new).with(project, subject.logger).and_call_original - expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).to receive(:execute) + expect(Projects::HashedStorage::MigrateRepositoryService).to receive(:new).with(project, subject.logger).and_return(repository_service) + expect(repository_service).to receive(:execute) service.execute end it 'does not delegate migration if repository is already migrated' do project.storage_version = ::Project::LATEST_STORAGE_VERSION - expect_any_instance_of(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:execute) + expect(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:new) service.execute end end context 'attachments migration' do + let(:attachments_service) { Projects::HashedStorage::MigrateAttachmentsService.new(project, subject.logger) } + it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do - expect(Projects::HashedStorage::MigrateAttachmentsService).to receive(:new).with(project, subject.logger).and_call_original - expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).to receive(:execute) + expect(Projects::HashedStorage::MigrateAttachmentsService).to receive(:new).with(project, subject.logger).and_return(attachments_service) + expect(attachments_service).to receive(:execute) service.execute end it 'does not delegate migration if attachments are already migrated' do project.storage_version = ::Project::LATEST_STORAGE_VERSION - expect_any_instance_of(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:execute) + expect(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:new) service.execute end From 58f32622ce9c2d08001da7b91065942cdc5a0f4a Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 24 Nov 2017 09:59:21 +0100 Subject: [PATCH 11/11] Changes to Attachments Migration for EE and Geo compatibility --- .../migrate_attachments_service.rb | 19 +++++++++++++------ app/uploaders/file_uploader.rb | 11 +++++++++-- .../migrate_attachments_service_spec.rb | 2 +- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb index 8cac6221a96..f8aaec8a9c0 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -3,7 +3,7 @@ module Projects AttachmentMigrationError = Class.new(StandardError) class MigrateAttachmentsService < BaseService - attr_reader :logger + attr_reader :logger, :old_path, :new_path def initialize(project, logger = nil) @project = project @@ -11,16 +11,21 @@ module Projects end def execute - old_path = FileUploader.dynamic_path_segment(project) - project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] - new_path = FileUploader.dynamic_path_segment(project) + @old_path = project.full_path + @new_path = project.disk_path - move_folder!(old_path, new_path) + origin = FileUploader.dynamic_path_segment(project) + project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:attachments] + target = FileUploader.dynamic_path_segment(project) + + result = move_folder!(origin, target) project.save! - if block_given? + if result && block_given? yield end + + result end private @@ -41,6 +46,8 @@ module Projects 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 diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index f4a5cf75018..71658df5b41 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -31,12 +31,19 @@ class FileUploader < GitlabUploader # Returns a String without a trailing slash def self.dynamic_path_segment(project) if project.hashed_storage?(:attachments) - File.join(CarrierWave.root, base_dir, project.disk_path) + dynamic_path_builder(project.disk_path) else - File.join(CarrierWave.root, base_dir, project.full_path) + dynamic_path_builder(project.full_path) end end + # Auxiliary method to build dynamic path segment when not using a project model + # + # Prefer to use the `.dynamic_path_segment` as it includes Hashed Storage specific logic + def self.dynamic_path_builder(path) + File.join(CarrierWave.root, base_dir, path) + end + attr_accessor :model attr_reader :secret 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 de2abfc1985..50e59954f73 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -58,6 +58,6 @@ describe Projects::HashedStorage::MigrateAttachmentsService do end def base_path(storage) - File.join(CarrierWave.root, FileUploader.base_dir, storage.disk_path) + FileUploader.dynamic_path_builder(storage.disk_path) end end