diff options
26 files changed, 330 insertions, 169 deletions
diff --git a/app/services/projects/after_rename_service.rb b/app/services/projects/after_rename_service.rb index c3cd9d1ea4a..fafdecb3222 100644 --- a/app/services/projects/after_rename_service.rb +++ b/app/services/projects/after_rename_service.rb @@ -62,7 +62,7 @@ module Projects def rename_or_migrate_repository! success = if migrate_to_hashed_storage? - ::Projects::HashedStorageMigrationService + ::Projects::HashedStorage::MigrationService .new(project, full_path_before) .execute else diff --git a/app/services/projects/hashed_storage/base_repository_service.rb b/app/services/projects/hashed_storage/base_repository_service.rb new file mode 100644 index 00000000000..761c81d776f --- /dev/null +++ b/app/services/projects/hashed_storage/base_repository_service.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +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) + + class BaseRepositoryService < BaseService + include Gitlab::ShellAdapter + + attr_reader :old_disk_path, :new_disk_path, :old_wiki_disk_path, :old_storage_version, :logger, :move_wiki + + def initialize(project, old_disk_path, logger: nil) + @project = project + @logger = logger || Gitlab::AppLogger + @old_disk_path = old_disk_path + @old_wiki_disk_path = "#{old_disk_path}.wiki" + @move_wiki = has_wiki? + end + + protected + + # rubocop: disable CodeReuse/ActiveRecord + def has_wiki? + gitlab_shell.exists?(project.repository_storage, "#{old_wiki_disk_path}.git") + end + # rubocop: enable CodeReuse/ActiveRecord + + # rubocop: disable CodeReuse/ActiveRecord + def move_repository(from_name, to_name) + from_exists = gitlab_shell.exists?(project.repository_storage, "#{from_name}.git") + to_exists = gitlab_shell.exists?(project.repository_storage, "#{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, from_name, to_name) + end + # rubocop: enable CodeReuse/ActiveRecord + + def rollback_folder_move + move_repository(new_disk_path, old_disk_path) + move_repository("#{new_disk_path}.wiki", old_wiki_disk_path) + 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 a1f0302aeb7..03e0685d2cd 100644 --- a/app/services/projects/hashed_storage/migrate_attachments_service.rb +++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb @@ -12,6 +12,7 @@ module Projects @logger = logger || Rails.logger @old_disk_path = old_disk_path @new_disk_path = project.disk_path + @skipped = false end def execute @@ -32,24 +33,29 @@ module Projects result end + def skipped? + @skipped + end + private - def move_folder!(old_disk_path, new_disk_path) - unless File.directory?(old_disk_path) - logger.info("Skipped attachments migration from '#{old_disk_path}' to '#{new_disk_path}', source path doesn't exist or is not a directory (PROJECT_ID=#{project.id})") - return + 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_disk_path) - logger.error("Cannot migrate attachments from '#{old_disk_path}' to '#{new_disk_path}', target path already exist (PROJECT_ID=#{project.id})") - raise AttachmentMigrationError, "Target path '#{new_disk_path}' already exist" + 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" end # Create hashed storage base path folder - FileUtils.mkdir_p(File.dirname(new_disk_path)) + FileUtils.mkdir_p(File.dirname(new_path)) - FileUtils.mv(old_disk_path, new_disk_path) - logger.info("Migrated project attachments from '#{old_disk_path}' to '#{new_disk_path}' (PROJECT_ID=#{project.id})") + FileUtils.mv(old_path, new_path) + logger.info("Migrated project attachments from '#{old_path}' to '#{new_path}' (PROJECT_ID=#{project.id})") true end diff --git a/app/services/projects/hashed_storage/migrate_repository_service.rb b/app/services/projects/hashed_storage/migrate_repository_service.rb index 2d851866a18..9c672283c7e 100644 --- a/app/services/projects/hashed_storage/migrate_repository_service.rb +++ b/app/services/projects/hashed_storage/migrate_repository_service.rb @@ -2,21 +2,7 @@ module Projects module HashedStorage - RepositoryMigrationError = Class.new(StandardError) - - class MigrateRepositoryService < BaseService - include Gitlab::ShellAdapter - - attr_reader :old_disk_path, :new_disk_path, :old_wiki_disk_path, :old_storage_version, :logger, :move_wiki - - def initialize(project, old_disk_path, logger: nil) - @project = project - @logger = logger || Rails.logger - @old_disk_path = old_disk_path - @old_wiki_disk_path = "#{old_disk_path}.wiki" - @move_wiki = has_wiki? - end - + class MigrateRepositoryService < BaseRepositoryService def execute try_to_set_repository_read_only! @@ -61,36 +47,6 @@ module Projects raise RepositoryMigrationError, migration_error end end - - # rubocop: disable CodeReuse/ActiveRecord - def has_wiki? - gitlab_shell.exists?(project.repository_storage, "#{old_wiki_disk_path}.git") - end - # rubocop: enable CodeReuse/ActiveRecord - - # rubocop: disable CodeReuse/ActiveRecord - def move_repository(from_name, to_name) - from_exists = gitlab_shell.exists?(project.repository_storage, "#{from_name}.git") - to_exists = gitlab_shell.exists?(project.repository_storage, "#{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, from_name, to_name) - end - # rubocop: enable CodeReuse/ActiveRecord - - def rollback_folder_move - move_repository(new_disk_path, old_disk_path) - move_repository("#{new_disk_path}.wiki", old_wiki_disk_path) - end end end end diff --git a/app/services/projects/hashed_storage/migration_service.rb b/app/services/projects/hashed_storage/migration_service.rb new file mode 100644 index 00000000000..f132dca61c9 --- /dev/null +++ b/app/services/projects/hashed_storage/migration_service.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module Projects + module HashedStorage + class MigrationService < 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 || Gitlab::AppLogger + end + + def execute + # Migrate repository from Legacy to Hashed Storage + unless project.hashed_storage?(:repository) + return false unless migrate_repository + end + + # Migrate attachments from Legacy to Hashed Storage + unless project.hashed_storage?(:attachments) + return false unless migrate_attachments + end + + true + end + + private + + def migrate_repository + HashedStorage::MigrateRepositoryService.new(project, old_disk_path, logger: logger).execute + end + + def migrate_attachments + HashedStorage::MigrateAttachmentsService.new(project, old_disk_path, logger: logger).execute + end + end + end +end diff --git a/app/services/projects/hashed_storage_migration_service.rb b/app/services/projects/hashed_storage_migration_service.rb deleted file mode 100644 index a0e734005f8..00000000000 --- a/app/services/projects/hashed_storage_migration_service.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -module Projects - class HashedStorageMigrationService < 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 - # Migrate repository from Legacy to Hashed Storage - unless project.hashed_storage?(:repository) - return unless HashedStorage::MigrateRepositoryService.new(project, old_disk_path, logger: logger).execute - end - - # Migrate attachments from Legacy to Hashed Storage - unless project.hashed_storage?(:attachments) - HashedStorage::MigrateAttachmentsService.new(project, old_disk_path, logger: logger).execute - end - - true - end - end -end diff --git a/app/views/admin/application_settings/_repository_storage.html.haml b/app/views/admin/application_settings/_repository_storage.html.haml index c6c29ed1f21..7a2bbfcdc4d 100644 --- a/app/views/admin/application_settings/_repository_storage.html.haml +++ b/app/views/admin/application_settings/_repository_storage.html.haml @@ -11,7 +11,6 @@ .form-text.text-muted Enable immutable, hash-based paths and repository names to store repositories on disk. This prevents repositories from having to be moved or renamed when the Project URL changes and may improve disk I/O performance. - %em (EXPERIMENTAL) .form-group = f.label :repository_storages, 'Storage paths for new projects', class: 'label-bold' = f.select :repository_storages, repository_storages_options_for_select(@application_setting.repository_storages), diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 4a306fc7a26..85c123c2704 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -45,6 +45,8 @@ - github_importer:github_import_stage_import_pull_requests - github_importer:github_import_stage_import_repository +- hashed_storage:hashed_storage_migrator + - mail_scheduler:mail_scheduler_issue_due - mail_scheduler:mail_scheduler_notification_service @@ -131,7 +133,6 @@ - repository_fork - repository_import - repository_remove_remote -- storage_migrator - system_hook_push - update_merge_requests - upload_checksum diff --git a/app/workers/hashed_storage/migrator_worker.rb b/app/workers/hashed_storage/migrator_worker.rb new file mode 100644 index 00000000000..49e347d4060 --- /dev/null +++ b/app/workers/hashed_storage/migrator_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module HashedStorage + class MigratorWorker + 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_migrate(start: start, finish: finish) + end + end +end diff --git a/app/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb index 4c6339f7701..1c8f313e6e9 100644 --- a/app/workers/project_migrate_hashed_storage_worker.rb +++ b/app/workers/project_migrate_hashed_storage_worker.rb @@ -4,21 +4,25 @@ 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) - project = Project.find_by(id: project_id) - return if project.nil? || project.pending_delete? - uuid = lease_for(project_id).try_obtain + if uuid - ::Projects::HashedStorageMigrationService.new(project, old_disk_path || project.full_path, logger: logger).execute + 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 - false + return false end - rescue => ex + + ensure cancel_lease_for(project_id, uuid) if uuid - raise ex end # rubocop: enable CodeReuse/ActiveRecord @@ -29,7 +33,8 @@ class ProjectMigrateHashedStorageWorker private def lease_key(project_id) - "project_migrate_hashed_storage_worker:#{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) diff --git a/app/workers/storage_migrator_worker.rb b/app/workers/storage_migrator_worker.rb deleted file mode 100644 index fa76fbac55c..00000000000 --- a/app/workers/storage_migrator_worker.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -class StorageMigratorWorker - include ApplicationWorker - - def perform(start, finish) - migrator = Gitlab::HashedStorage::Migrator.new - migrator.bulk_migrate(start, finish) - end -end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 5f87d04c697..1e094c03171 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -68,7 +68,7 @@ - [background_migration, 1] - [gcp_cluster, 1] - [project_migrate_hashed_storage, 1] - - [storage_migrator, 1] + - [hashed_storage, 1] - [pages_domain_verification, 1] - [object_storage_upload, 1] - [object_storage, 1] diff --git a/db/post_migrate/20190124200344_migrate_storage_migrator_sidekiq_queue.rb b/db/post_migrate/20190124200344_migrate_storage_migrator_sidekiq_queue.rb new file mode 100644 index 00000000000..193bd571831 --- /dev/null +++ b/db/post_migrate/20190124200344_migrate_storage_migrator_sidekiq_queue.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MigrateStorageMigratorSidekiqQueue < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + sidekiq_queue_migrate 'storage_migrator', to: 'hashed_storage:hashed_storage_migrator' + end + + def down + sidekiq_queue_migrate 'hashed_storage:hashed_storage_migrator', to: 'storage_migrator' + end +end diff --git a/db/schema.rb b/db/schema.rb index cd502d06bf4..859f007dbe1 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: 20190115054216) do +ActiveRecord::Schema.define(version: 20190124200344) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/lib/gitlab/hashed_storage/migrator.rb b/lib/gitlab/hashed_storage/migrator.rb index 1f29cf10cad..bf463077dcc 100644 --- a/lib/gitlab/hashed_storage/migrator.rb +++ b/lib/gitlab/hashed_storage/migrator.rb @@ -11,21 +11,21 @@ module Gitlab # Schedule a range of projects to be bulk migrated with #bulk_migrate asynchronously # - # @param [Object] start first project id for the range - # @param [Object] finish last project id for the range - def bulk_schedule(start, finish) - StorageMigratorWorker.perform_async(start, finish) + # @param [Integer] start first project id for the range + # @param [Integer] finish last project id for the range + def bulk_schedule(start:, finish:) + ::HashedStorage::MigratorWorker.perform_async(start, finish) end # Start migration of projects from specified range # - # Flagging a project to be migrated is a synchronous action, + # Flagging a project to be migrated is a synchronous action # but the migration runs through async jobs # - # @param [Object] start first project id for the range - # @param [Object] finish last project id for the range + # @param [Integer] start first project id for the range + # @param [Integer] finish last project id for the range # rubocop: disable CodeReuse/ActiveRecord - def bulk_migrate(start, finish) + def bulk_migrate(start:, finish:) projects = build_relation(start, finish) projects.with_route.find_each(batch_size: BATCH_SIZE) do |project| @@ -34,9 +34,9 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord - # Flag a project to be migrated + # Flag a project to be migrated to Hashed Storage # - # @param [Object] project that will be migrated + # @param [Project] project that will be migrated def migrate(project) Rails.logger.info "Starting storage migration of #{project.full_path} (ID=#{project.id})..." @@ -45,6 +45,10 @@ module Gitlab Rails.logger.error("#{err.message} migrating storage of #{project.full_path} (ID=#{project.id}), trace - #{err.backtrace}") end + def rollback(project) + # TODO: implement rollback strategy + end + private # rubocop: disable CodeReuse/ActiveRecord diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake index 09dc3aa9882..f9ce3e1d338 100644 --- a/lib/tasks/gitlab/storage.rake +++ b/lib/tasks/gitlab/storage.rake @@ -37,7 +37,7 @@ 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, finish) + storage_migrator.bulk_schedule(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 01d43ed00a2..3942f168ceb 100644 --- a/spec/lib/gitlab/hashed_storage/migrator_spec.rb +++ b/spec/lib/gitlab/hashed_storage/migrator_spec.rb @@ -4,7 +4,7 @@ describe Gitlab::HashedStorage::Migrator do describe '#bulk_schedule' do it 'schedules job to StorageMigratorWorker' do Sidekiq::Testing.fake! do - expect { subject.bulk_schedule(1, 5) }.to change(StorageMigratorWorker.jobs, :size).by(1) + expect { subject.bulk_schedule(start: 1, finish: 5) }.to change(HashedStorage::MigratorWorker.jobs, :size).by(1) end end end @@ -15,13 +15,13 @@ describe Gitlab::HashedStorage::Migrator do it 'enqueue jobs to ProjectMigrateHashedStorageWorker' do Sidekiq::Testing.fake! do - expect { subject.bulk_migrate(ids.min, ids.max) }.to change(ProjectMigrateHashedStorageWorker.jobs, :size).by(2) + expect { subject.bulk_migrate(start: ids.min, finish: ids.max) }.to change(ProjectMigrateHashedStorageWorker.jobs, :size).by(2) end end it 'rescues and log exceptions' do allow_any_instance_of(Project).to receive(:migrate_to_hashed_storage!).and_raise(StandardError) - expect { subject.bulk_migrate(ids.min, ids.max) }.not_to raise_error + expect { subject.bulk_migrate(start: ids.min, finish: ids.max) }.not_to raise_error end it 'delegates each project in specified range to #migrate' do @@ -29,12 +29,12 @@ describe Gitlab::HashedStorage::Migrator do expect(subject).to receive(:migrate).with(project) end - subject.bulk_migrate(ids.min, ids.max) + subject.bulk_migrate(start: ids.min, finish: ids.max) end it 'has migrated projects set as writable' do perform_enqueued_jobs do - subject.bulk_migrate(ids.min, ids.max) + subject.bulk_migrate(start: ids.min, finish: ids.max) end projects.each do |project| @@ -46,7 +46,7 @@ describe Gitlab::HashedStorage::Migrator do describe '#migrate' do let(:project) { create(:project, :legacy_storage, :empty_repo) } - it 'enqueues job to ProjectMigrateHashedStorageWorker' do + it 'enqueues project migration job' do Sidekiq::Testing.fake! do expect { subject.migrate(project) }.to change(ProjectMigrateHashedStorageWorker.jobs, :size).by(1) end @@ -58,7 +58,7 @@ describe Gitlab::HashedStorage::Migrator do expect { subject.migrate(project) }.not_to raise_error end - it 'migrate project' do + it 'migrates project storage' do perform_enqueued_jobs do subject.migrate(project) end @@ -73,5 +73,19 @@ describe Gitlab::HashedStorage::Migrator do expect(project.reload.repository_read_only?).to be_falsey end + + context 'when project is already on hashed storage' do + let(:project) { create(:project, :empty_repo) } + + it 'doesnt enqueue any migration job' do + Sidekiq::Testing.fake! do + expect { subject.migrate(project) }.not_to change(ProjectMigrateHashedStorageWorker.jobs, :size) + end + end + + it 'returns false' do + expect(subject.migrate(project)).to be_falsey + end + end end end diff --git a/spec/migrations/migrate_storage_migrator_sidekiq_queue_spec.rb b/spec/migrations/migrate_storage_migrator_sidekiq_queue_spec.rb new file mode 100644 index 00000000000..f8cf76cb339 --- /dev/null +++ b/spec/migrations/migrate_storage_migrator_sidekiq_queue_spec.rb @@ -0,0 +1,47 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20190124200344_migrate_storage_migrator_sidekiq_queue.rb') + +describe MigrateStorageMigratorSidekiqQueue, :sidekiq, :redis do + include Gitlab::Database::MigrationHelpers + + context 'when there are jobs in the queues' do + it 'correctly migrates queue when migrating up' do + Sidekiq::Testing.disable! do + stubbed_worker(queue: :storage_migrator).perform_async(1, 5) + + described_class.new.up + + expect(sidekiq_queue_length('storage_migrator')).to eq 0 + expect(sidekiq_queue_length('hashed_storage:hashed_storage_migrator')).to eq 1 + end + end + + it 'correctly migrates queue when migrating down' do + Sidekiq::Testing.disable! do + stubbed_worker(queue: :'hashed_storage:hashed_storage_migrator').perform_async(1, 5) + + described_class.new.down + + expect(sidekiq_queue_length('storage_migrator')).to eq 1 + expect(sidekiq_queue_length('hashed_storage:hashed_storage_migrator')).to eq 0 + end + end + end + + context 'when there are no jobs in the queues' do + it 'does not raise error when migrating up' do + expect { described_class.new.up }.not_to raise_error + end + + it 'does not raise error when migrating down' do + expect { described_class.new.down }.not_to raise_error + end + end + + def stubbed_worker(queue:) + Class.new do + include Sidekiq::Worker + sidekiq_options queue: queue + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 437f0066450..4b061b5e24f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3224,7 +3224,7 @@ describe Project do end context 'legacy storage' do - let(:project) { create(:project, :repository, :legacy_storage) } + set(:project) { create(:project, :repository, :legacy_storage) } let(:gitlab_shell) { Gitlab::Shell.new } let(:project_storage) { project.send(:storage) } @@ -3279,13 +3279,14 @@ describe Project do end describe '#migrate_to_hashed_storage!' do + let(:project) { create(:project, :empty_repo, :legacy_storage) } + it 'returns true' do expect(project.migrate_to_hashed_storage!).to be_truthy end - it 'does not validate project visibility' do - expect(project).not_to receive(:visibility_level_allowed_as_fork) - expect(project).not_to receive(:visibility_level_allowed_by_group) + it 'does not run validation' do + expect(project).not_to receive(:valid?) project.migrate_to_hashed_storage! end @@ -3315,7 +3316,7 @@ describe Project do end context 'hashed storage' do - let(:project) { create(:project, :repository, skip_disk_validation: true) } + set(:project) { create(:project, :repository, skip_disk_validation: true) } let(:gitlab_shell) { Gitlab::Shell.new } let(:hash) { Digest::SHA2.hexdigest(project.id.to_s) } let(:hashed_prefix) { File.join('@hashed', hash[0..1], hash[2..3]) } @@ -3372,6 +3373,8 @@ describe Project do end describe '#migrate_to_hashed_storage!' do + let(:project) { create(:project, :repository, skip_disk_validation: true) } + it 'returns nil' do expect(project.migrate_to_hashed_storage!).to be_nil end @@ -3381,10 +3384,12 @@ describe Project do end context 'when partially migrated' do - it 'returns true' do + it 'enqueues a job' do project = create(:project, storage_version: 1, skip_disk_validation: true) - expect(project.migrate_to_hashed_storage!).to be_truthy + Sidekiq::Testing.fake! do + expect { project.migrate_to_hashed_storage! }.to change(ProjectMigrateHashedStorageWorker.jobs, :size).by(1) + end end end end diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index bc5366a3339..b8055a285f2 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -101,10 +101,10 @@ describe Projects::AfterRenameService do end context 'with hashed storage upgrade when renaming enabled' do - it 'calls HashedStorageMigrationService with correct options' do + it 'calls HashedStorage::MigrationService with correct options' do stub_application_setting(hashed_storage_enabled: true) - expect_next_instance_of(::Projects::HashedStorageMigrationService) do |service| + expect_next_instance_of(::Projects::HashedStorage::MigrationService) do |service| expect(service).to receive(:execute).and_return(true) 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 28d8a95fe07..61dbb57ec08 100644 --- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Projects::HashedStorage::MigrateAttachmentsService do subject(:service) { described_class.new(project, project.full_path, logger: nil) } - let(:project) { create(:project, :legacy_storage) } + let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) } let(:legacy_storage) { Storage::LegacyProject.new(project) } let(:hashed_storage) { Storage::HashedProject.new(project) } @@ -28,6 +28,16 @@ describe Projects::HashedStorage::MigrateAttachmentsService do 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 @@ -43,6 +53,16 @@ describe Projects::HashedStorage::MigrateAttachmentsService do expect(File.exist?(base_path(hashed_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 @@ -58,6 +78,18 @@ describe Projects::HashedStorage::MigrateAttachmentsService do end end + context '#old_disk_path' do + it 'returns old disk_path for project' do + expect(service.old_disk_path).to eq(project.full_path) + end + end + + context '#new_disk_path' do + it 'returns new disk_path for project' do + expect(service.new_disk_path).to eq(project.disk_path) + end + end + def base_path(storage) File.join(FileUploader.root, storage.disk_path) 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 b720f37ffdb..0772dc4b85b 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -8,9 +8,12 @@ describe Projects::HashedStorage::MigrateRepositoryService do let(:legacy_storage) { Storage::LegacyProject.new(project) } let(:hashed_storage) { Storage::HashedProject.new(project) } - subject(:service) { described_class.new(project, project.full_path) } + subject(:service) { described_class.new(project, project.disk_path) } describe '#execute' do + let(:old_disk_path) { legacy_storage.disk_path } + let(:new_disk_path) { hashed_storage.disk_path } + before do allow(service).to receive(:gitlab_shell) { gitlab_shell } end @@ -33,8 +36,8 @@ describe Projects::HashedStorage::MigrateRepositoryService do it 'renames project and wiki repositories' do service.execute - expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.git")).to be_truthy - expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.wiki.git")).to be_truthy + 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 hashed and not read-only' do @@ -45,8 +48,8 @@ describe Projects::HashedStorage::MigrateRepositoryService do end it 'move operation is called for both repositories' do - expect_move_repository(project.disk_path, hashed_storage.disk_path) - expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") + expect_move_repository(old_disk_path, new_disk_path) + expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki") service.execute end @@ -62,32 +65,27 @@ describe Projects::HashedStorage::MigrateRepositoryService do context 'when one move fails' do it 'rollsback repositories to original name' do - from_name = project.disk_path - to_name = hashed_storage.disk_path allow(service).to receive(:move_repository).and_call_original - allow(service).to receive(:move_repository).with(from_name, to_name).once { false } # will disable first move only + 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, "#{hashed_storage.disk_path}.git")).to be_falsey - expect(gitlab_shell.exists?(project.repository_storage, "#{hashed_storage.disk_path}.wiki.git")).to be_falsey + 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 - 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, from_name, to_name) + gitlab_shell.mv_repository(project.repository_storage, old_disk_path, new_disk_path) end - it 'does not try to move nil repository over hashed' do - expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, from_name, to_name) - expect_move_repository("#{project.disk_path}.wiki", "#{hashed_storage.disk_path}.wiki") + 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 diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage/migration_service_spec.rb index 5368c3828dd..b4647586363 100644 --- a/spec/services/projects/hashed_storage_migration_service_spec.rb +++ b/spec/services/projects/hashed_storage/migration_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Projects::HashedStorageMigrationService do +describe Projects::HashedStorage::MigrationService do let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) } let(:logger) { double } diff --git a/spec/tasks/gitlab/storage_rake_spec.rb b/spec/tasks/gitlab/storage_rake_spec.rb index be902d7c679..6b50670c3c0 100644 --- a/spec/tasks/gitlab/storage_rake_spec.rb +++ b/spec/tasks/gitlab/storage_rake_spec.rb @@ -58,7 +58,7 @@ describe 'rake gitlab:storage:*' do context '0 legacy projects' do it 'does nothing' do - expect(StorageMigratorWorker).not_to receive(:perform_async) + expect(::HashedStorage::MigratorWorker).not_to receive(:perform_async) run_rake_task(task) end @@ -72,9 +72,9 @@ describe 'rake gitlab:storage:*' do stub_env('BATCH' => 1) end - it 'enqueues one StorageMigratorWorker per project' do + it 'enqueues one HashedStorage::MigratorWorker per project' do projects.each do |project| - expect(StorageMigratorWorker).to receive(:perform_async).with(project.id, project.id) + expect(::HashedStorage::MigratorWorker).to receive(:perform_async).with(project.id, project.id) end run_rake_task(task) @@ -86,10 +86,10 @@ describe 'rake gitlab:storage:*' do stub_env('BATCH' => 2) end - it 'enqueues one StorageMigratorWorker per 2 projects' do + it 'enqueues one HashedStorage::MigratorWorker per 2 projects' do projects.map(&:id).sort.each_slice(2) do |first, last| last ||= first - expect(StorageMigratorWorker).to receive(:perform_async).with(first, last) + expect(::HashedStorage::MigratorWorker).to receive(:perform_async).with(first, last) end run_rake_task(task) diff --git a/spec/workers/storage_migrator_worker_spec.rb b/spec/workers/hashed_storage/migrator_worker_spec.rb index 808084c8f7c..a85f820a3eb 100644 --- a/spec/workers/storage_migrator_worker_spec.rb +++ b/spec/workers/hashed_storage/migrator_worker_spec.rb @@ -1,13 +1,13 @@ require 'spec_helper' -describe StorageMigratorWorker do +describe HashedStorage::MigratorWorker do subject(:worker) { described_class.new } let(:projects) { create_list(:project, 2, :legacy_storage, :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_migrate).with(5, 10) + expect_any_instance_of(Gitlab::HashedStorage::Migrator).to receive(:bulk_migrate).with(start: 5, finish: 10) worker.perform(5, 10) end diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb index 3703320418b..333eb6a0569 100644 --- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb +++ b/spec/workers/project_migrate_hashed_storage_worker_spec.rb @@ -4,12 +4,13 @@ describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do include ExclusiveLeaseHelpers describe '#perform' do - let(:project) { create(:project, :empty_repo) } + let(:project) { create(:project, :empty_repo, :legacy_storage) } let(:lease_key) { "project_migrate_hashed_storage_worker:#{project.id}" } - let(:lease_timeout) { ProjectMigrateHashedStorageWorker::LEASE_TIMEOUT } + let(:lease_timeout) { described_class::LEASE_TIMEOUT } + let(:migration_service) { ::Projects::HashedStorage::MigrationService } it 'skips when project no longer exists' do - expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + expect(migration_service).not_to receive(:new) subject.perform(-1) end @@ -17,29 +18,29 @@ describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do it 'skips when project is pending delete' do pending_delete_project = create(:project, :empty_repo, pending_delete: true) - expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + expect(migration_service).not_to receive(:new) subject.perform(pending_delete_project.id) end - it 'delegates removal to service class when have exclusive lease' do + it 'delegates migration to service class when we have exclusive lease' do stub_exclusive_lease(lease_key, 'uuid', timeout: lease_timeout) - migration_service = spy + service_spy = spy - allow(::Projects::HashedStorageMigrationService) + allow(migration_service) .to receive(:new).with(project, project.full_path, logger: subject.logger) - .and_return(migration_service) + .and_return(service_spy) subject.perform(project.id) - expect(migration_service).to have_received(:execute) + expect(service_spy).to have_received(:execute) end - it 'skips when dont have lease when dont have exclusive lease' do + it 'skips when it cant acquire the exclusive lease' do stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) - expect(::Projects::HashedStorageMigrationService).not_to receive(:new) + expect(migration_service).not_to receive(:new) subject.perform(project.id) end |