From 07a196be3dca5125454262bb96967d5895081c56 Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Thu, 20 Dec 2018 17:44:27 +0100
Subject: Remove "Experimental" text from Hashed Storage settings page

---
 app/views/admin/application_settings/_repository_storage.html.haml | 1 -
 1 file changed, 1 deletion(-)

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),
-- 
cgit v1.2.1


From c2c34eba62f26bed8cad8b07934e14b4519eef7c Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Sat, 22 Dec 2018 14:54:33 +0100
Subject: Prepare rake task for storage rollback

We are keeping compatibility with existing scheduled jobs.
---
 app/workers/storage_migrator_worker.rb          | 10 +++++++--
 lib/gitlab/hashed_storage/migrator.rb           | 29 +++++++++++++++++--------
 lib/tasks/gitlab/storage.rake                   |  2 +-
 spec/lib/gitlab/hashed_storage/migrator_spec.rb | 10 ++++-----
 spec/tasks/gitlab/storage_rake_spec.rb          |  4 ++--
 spec/workers/storage_migrator_worker_spec.rb    |  2 +-
 6 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/app/workers/storage_migrator_worker.rb b/app/workers/storage_migrator_worker.rb
index fa76fbac55c..714c4eed5b2 100644
--- a/app/workers/storage_migrator_worker.rb
+++ b/app/workers/storage_migrator_worker.rb
@@ -3,8 +3,14 @@
 class StorageMigratorWorker
   include ApplicationWorker
 
-  def perform(start, finish)
+  # @param [Integer] start initial ID of the batch
+  # @param [Integer] finish last ID of the batch
+  # @param [String] operation the operation to be performed: ['migrate', 'rollback']
+  def perform(start, finish, operation = :migrate)
+    # when scheduling a job symbols are converted to string, we need to convert back
+    operation = operation.to_sym if operation
+
     migrator = Gitlab::HashedStorage::Migrator.new
-    migrator.bulk_migrate(start, finish)
+    migrator.bulk_migrate(start: start, finish: finish, operation: operation)
   end
 end
diff --git a/lib/gitlab/hashed_storage/migrator.rb b/lib/gitlab/hashed_storage/migrator.rb
index 1f29cf10cad..4794b1978db 100644
--- a/lib/gitlab/hashed_storage/migrator.rb
+++ b/lib/gitlab/hashed_storage/migrator.rb
@@ -11,10 +11,11 @@ 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
+      # @param [Symbol] operation [:migrate, :rollback]
+      def bulk_schedule(start:, finish:, operation: :migrate)
+        StorageMigratorWorker.perform_async(start, finish, operation)
       end
 
       # Start migration of projects from specified range
@@ -22,21 +23,27 @@ module Gitlab
       # 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
+      # @param [Symbol] operation [:migrate, :rollback]
       # rubocop: disable CodeReuse/ActiveRecord
-      def bulk_migrate(start, finish)
+      def bulk_migrate(start:, finish:, operation: :migrate)
         projects = build_relation(start, finish)
 
         projects.with_route.find_each(batch_size: BATCH_SIZE) do |project|
-          migrate(project)
+          case operation
+          when :migrate
+            migrate(project)
+          when :rollback
+            rollback(project)
+          end
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       # Flag a project to be migrated
       #
-      # @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 +52,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..8212be90b97 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, operation: :migrate)
 
         print '.'
       end
diff --git a/spec/lib/gitlab/hashed_storage/migrator_spec.rb b/spec/lib/gitlab/hashed_storage/migrator_spec.rb
index 01d43ed00a2..6d68cb0744b 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(StorageMigratorWorker.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|
diff --git a/spec/tasks/gitlab/storage_rake_spec.rb b/spec/tasks/gitlab/storage_rake_spec.rb
index be902d7c679..3b8d5ad7015 100644
--- a/spec/tasks/gitlab/storage_rake_spec.rb
+++ b/spec/tasks/gitlab/storage_rake_spec.rb
@@ -74,7 +74,7 @@ describe 'rake gitlab:storage:*' do
 
         it 'enqueues one StorageMigratorWorker per project' do
           projects.each do |project|
-            expect(StorageMigratorWorker).to receive(:perform_async).with(project.id, project.id)
+            expect(StorageMigratorWorker).to receive(:perform_async).with(project.id, project.id, :migrate)
           end
 
           run_rake_task(task)
@@ -89,7 +89,7 @@ describe 'rake gitlab:storage:*' do
         it 'enqueues one StorageMigratorWorker 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(StorageMigratorWorker).to receive(:perform_async).with(first, last, :migrate)
           end
 
           run_rake_task(task)
diff --git a/spec/workers/storage_migrator_worker_spec.rb b/spec/workers/storage_migrator_worker_spec.rb
index 808084c8f7c..63f3c1d1c59 100644
--- a/spec/workers/storage_migrator_worker_spec.rb
+++ b/spec/workers/storage_migrator_worker_spec.rb
@@ -7,7 +7,7 @@ describe StorageMigratorWorker do
 
   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, operation: :migrate)
 
       worker.perform(5, 10)
     end
-- 
cgit v1.2.1


From cf52488ccba8913027334c16892ee1fce1241169 Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Tue, 15 Jan 2019 04:30:55 +0100
Subject: Move MigrationService to HashedStorage module

This is part of the refactor to include a RollbackService into
HashedStorage module
---
 app/services/projects/after_rename_service.rb      |  2 +-
 .../projects/hashed_storage/migration_service.rb   | 29 ++++++++++++
 .../projects/hashed_storage_migration_service.rb   | 27 -----------
 .../project_migrate_hashed_storage_worker.rb       |  2 +-
 .../services/projects/after_rename_service_spec.rb |  4 +-
 .../hashed_storage/migration_service_spec.rb       | 52 ++++++++++++++++++++++
 .../hashed_storage_migration_service_spec.rb       | 52 ----------------------
 .../project_migrate_hashed_storage_worker_spec.rb  |  8 ++--
 8 files changed, 89 insertions(+), 87 deletions(-)
 create mode 100644 app/services/projects/hashed_storage/migration_service.rb
 delete mode 100644 app/services/projects/hashed_storage_migration_service.rb
 create mode 100644 spec/services/projects/hashed_storage/migration_service_spec.rb
 delete mode 100644 spec/services/projects/hashed_storage_migration_service_spec.rb

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/migration_service.rb b/app/services/projects/hashed_storage/migration_service.rb
new file mode 100644
index 00000000000..140c35bdd52
--- /dev/null
+++ b/app/services/projects/hashed_storage/migration_service.rb
@@ -0,0 +1,29 @@
+# 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 || 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
+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/workers/project_migrate_hashed_storage_worker.rb b/app/workers/project_migrate_hashed_storage_worker.rb
index 4c6339f7701..c3cdf102382 100644
--- a/app/workers/project_migrate_hashed_storage_worker.rb
+++ b/app/workers/project_migrate_hashed_storage_worker.rb
@@ -12,7 +12,7 @@ class ProjectMigrateHashedStorageWorker
 
     uuid = lease_for(project_id).try_obtain
     if uuid
-      ::Projects::HashedStorageMigrationService.new(project, old_disk_path || project.full_path, logger: logger).execute
+      ::Projects::HashedStorage::MigrationService.new(project, old_disk_path || project.full_path, logger: logger).execute
     else
       false
     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/migration_service_spec.rb b/spec/services/projects/hashed_storage/migration_service_spec.rb
new file mode 100644
index 00000000000..b4647586363
--- /dev/null
+++ b/spec/services/projects/hashed_storage/migration_service_spec.rb
@@ -0,0 +1,52 @@
+require 'spec_helper'
+
+describe Projects::HashedStorage::MigrationService do
+  let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) }
+  let(:logger) { double }
+
+  subject(:service) { described_class.new(project, project.full_path, logger: logger) }
+
+  describe '#execute' do
+    context 'repository migration' do
+      let(:repository_service) { Projects::HashedStorage::MigrateRepositoryService.new(project, project.full_path, logger: logger) }
+
+      it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
+        expect(Projects::HashedStorage::MigrateRepositoryService)
+          .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 migration if repository is already migrated' do
+        project.storage_version = ::Project::LATEST_STORAGE_VERSION
+        expect(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:new)
+
+        service.execute
+      end
+    end
+
+    context 'attachments migration' do
+      let(:attachments_service) { Projects::HashedStorage::MigrateAttachmentsService.new(project, project.full_path, logger: logger) }
+
+      it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
+        expect(Projects::HashedStorage::MigrateAttachmentsService)
+          .to receive(:new)
+                .with(project, project.full_path, logger: 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(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:new)
+
+        service.execute
+      end
+    end
+  end
+end
diff --git a/spec/services/projects/hashed_storage_migration_service_spec.rb b/spec/services/projects/hashed_storage_migration_service_spec.rb
deleted file mode 100644
index 5368c3828dd..00000000000
--- a/spec/services/projects/hashed_storage_migration_service_spec.rb
+++ /dev/null
@@ -1,52 +0,0 @@
-require 'spec_helper'
-
-describe Projects::HashedStorageMigrationService do
-  let(:project) { create(:project, :empty_repo, :wiki_repo, :legacy_storage) }
-  let(:logger) { double }
-
-  subject(:service) { described_class.new(project, project.full_path, logger: logger) }
-
-  describe '#execute' do
-    context 'repository migration' do
-      let(:repository_service) { Projects::HashedStorage::MigrateRepositoryService.new(project, project.full_path, logger: logger) }
-
-      it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
-        expect(Projects::HashedStorage::MigrateRepositoryService)
-          .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 migration if repository is already migrated' do
-        project.storage_version = ::Project::LATEST_STORAGE_VERSION
-        expect(Projects::HashedStorage::MigrateRepositoryService).not_to receive(:new)
-
-        service.execute
-      end
-    end
-
-    context 'attachments migration' do
-      let(:attachments_service) { Projects::HashedStorage::MigrateAttachmentsService.new(project, project.full_path, logger: logger) }
-
-      it 'delegates migration to Projects::HashedStorage::MigrateRepositoryService' do
-        expect(Projects::HashedStorage::MigrateAttachmentsService)
-          .to receive(:new)
-                .with(project, project.full_path, logger: 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(Projects::HashedStorage::MigrateAttachmentsService).not_to receive(:new)
-
-        service.execute
-      end
-    end
-  end
-end
diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb
index 3703320418b..18470923316 100644
--- a/spec/workers/project_migrate_hashed_storage_worker_spec.rb
+++ b/spec/workers/project_migrate_hashed_storage_worker_spec.rb
@@ -9,7 +9,7 @@ describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do
     let(:lease_timeout) { ProjectMigrateHashedStorageWorker::LEASE_TIMEOUT }
 
     it 'skips when project no longer exists' do
-      expect(::Projects::HashedStorageMigrationService).not_to receive(:new)
+      expect(::Projects::HashedStorage::MigrationService).not_to receive(:new)
 
       subject.perform(-1)
     end
@@ -17,7 +17,7 @@ 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(::Projects::HashedStorage::MigrationService).not_to receive(:new)
 
       subject.perform(pending_delete_project.id)
     end
@@ -27,7 +27,7 @@ describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do
 
       migration_service = spy
 
-      allow(::Projects::HashedStorageMigrationService)
+      allow(::Projects::HashedStorage::MigrationService)
         .to receive(:new).with(project, project.full_path, logger: subject.logger)
         .and_return(migration_service)
 
@@ -39,7 +39,7 @@ describe ProjectMigrateHashedStorageWorker, :clean_gitlab_redis_shared_state do
     it 'skips when dont have lease when dont have exclusive lease' do
       stub_exclusive_lease_taken(lease_key, timeout: lease_timeout)
 
-      expect(::Projects::HashedStorageMigrationService).not_to receive(:new)
+      expect(::Projects::HashedStorage::MigrationService).not_to receive(:new)
 
       subject.perform(project.id)
     end
-- 
cgit v1.2.1


From b88f27c8d1cb44201490bf51db143f4267735775 Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Tue, 15 Jan 2019 04:37:27 +0100
Subject: Extract BaseRepositoryService from MigrateRepository

This will serve as new base class for both Migrate and Rollback
---
 .../hashed_storage/base_repository_service.rb      | 57 ++++++++++++++++++++++
 .../hashed_storage/migrate_repository_service.rb   | 46 +----------------
 2 files changed, 58 insertions(+), 45 deletions(-)
 create mode 100644 app/services/projects/hashed_storage/base_repository_service.rb

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..cc5366f1f61
--- /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 || Rails.logger
+        @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_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
-- 
cgit v1.2.1


From 7bc16889df458865ffbbb7bef8087c04a5768a1d Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Thu, 17 Jan 2019 02:53:50 +0100
Subject: Refactor Storage Migration

Specs were reviewed and improved to better cover the current behavior.
There was some standardization done as well to facilitate the
implementation of the rollback functionality.

StorageMigratorWorker was extracted to HashedStorage namespace were
RollbackerWorker will live one as well.
---
 .../hashed_storage/migrate_attachments_service.rb  | 20 +++++++-------
 .../projects/hashed_storage/migration_service.rb   | 14 ++++++++--
 app/workers/all_queues.yml                         |  3 +-
 app/workers/hashed_storage/migrator_worker.rb      | 16 +++++++++++
 .../project_migrate_hashed_storage_worker.rb       | 21 ++++++++------
 app/workers/storage_migrator_worker.rb             | 16 -----------
 config/sidekiq_queues.yml                          |  2 +-
 lib/gitlab/hashed_storage/migrator.rb              | 19 ++++---------
 lib/tasks/gitlab/storage.rake                      |  2 +-
 spec/lib/gitlab/hashed_storage/migrator_spec.rb    | 20 ++++++++++++--
 spec/models/project_spec.rb                        | 19 ++++++++-----
 .../migrate_attachments_service_spec.rb            | 24 +++++++++++++++-
 .../migrate_repository_service_spec.rb             | 32 ++++++++++------------
 spec/tasks/gitlab/storage_rake_spec.rb             | 10 +++----
 .../workers/hashed_storage/migrator_worker_spec.rb | 25 +++++++++++++++++
 .../project_migrate_hashed_storage_worker_spec.rb  | 23 ++++++++--------
 spec/workers/storage_migrator_worker_spec.rb       | 25 -----------------
 17 files changed, 170 insertions(+), 121 deletions(-)
 create mode 100644 app/workers/hashed_storage/migrator_worker.rb
 delete mode 100644 app/workers/storage_migrator_worker.rb
 create mode 100644 spec/workers/hashed_storage/migrator_worker_spec.rb
 delete mode 100644 spec/workers/storage_migrator_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 a1f0302aeb7..7476980a117 100644
--- a/app/services/projects/hashed_storage/migrate_attachments_service.rb
+++ b/app/services/projects/hashed_storage/migrate_attachments_service.rb
@@ -34,22 +34,22 @@ module Projects
 
       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})")
+          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/migration_service.rb b/app/services/projects/hashed_storage/migration_service.rb
index 140c35bdd52..7320f41ebf0 100644
--- a/app/services/projects/hashed_storage/migration_service.rb
+++ b/app/services/projects/hashed_storage/migration_service.rb
@@ -14,16 +14,26 @@ module Projects
       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
+          return false unless migrate_repository
         end
 
         # Migrate attachments from Legacy to Hashed Storage
         unless project.hashed_storage?(:attachments)
-          HashedStorage::MigrateAttachmentsService.new(project, old_disk_path, logger: logger).execute
+          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/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 c3cdf102382..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::HashedStorage::MigrationService.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 714c4eed5b2..00000000000
--- a/app/workers/storage_migrator_worker.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# frozen_string_literal: true
-
-class StorageMigratorWorker
-  include ApplicationWorker
-
-  # @param [Integer] start initial ID of the batch
-  # @param [Integer] finish last ID of the batch
-  # @param [String] operation the operation to be performed: ['migrate', 'rollback']
-  def perform(start, finish, operation = :migrate)
-    # when scheduling a job symbols are converted to string, we need to convert back
-    operation = operation.to_sym if operation
-
-    migrator = Gitlab::HashedStorage::Migrator.new
-    migrator.bulk_migrate(start: start, finish: finish, operation: operation)
-  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/lib/gitlab/hashed_storage/migrator.rb b/lib/gitlab/hashed_storage/migrator.rb
index 4794b1978db..bf463077dcc 100644
--- a/lib/gitlab/hashed_storage/migrator.rb
+++ b/lib/gitlab/hashed_storage/migrator.rb
@@ -13,35 +13,28 @@ module Gitlab
       #
       # @param [Integer] start first project id for the range
       # @param [Integer] finish last project id for the range
-      # @param [Symbol] operation [:migrate, :rollback]
-      def bulk_schedule(start:, finish:, operation: :migrate)
-        StorageMigratorWorker.perform_async(start, finish, operation)
+      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 [Integer] start first project id for the range
       # @param [Integer] finish last project id for the range
-      # @param [Symbol] operation [:migrate, :rollback]
       # rubocop: disable CodeReuse/ActiveRecord
-      def bulk_migrate(start:, finish:, operation: :migrate)
+      def bulk_migrate(start:, finish:)
         projects = build_relation(start, finish)
 
         projects.with_route.find_each(batch_size: BATCH_SIZE) do |project|
-          case operation
-          when :migrate
-            migrate(project)
-          when :rollback
-            rollback(project)
-          end
+          migrate(project)
         end
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      # Flag a project to be migrated
+      # Flag a project to be migrated to Hashed Storage
       #
       # @param [Project] project that will be migrated
       def migrate(project)
diff --git a/lib/tasks/gitlab/storage.rake b/lib/tasks/gitlab/storage.rake
index 8212be90b97..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: start, finish: finish, operation: :migrate)
+        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 6d68cb0744b..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(start: 1, finish: 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
@@ -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/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/hashed_storage/migrate_attachments_service_spec.rb b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
index 28d8a95fe07..bef12df583e 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,10 @@ 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
     end
 
     context 'when original folder does not exist anymore' do
@@ -43,6 +47,12 @@ 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(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage))
+
+        expect(service.execute).to be_truthy
+      end
     end
 
     context 'when target folder already exists' do
@@ -58,6 +68,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/tasks/gitlab/storage_rake_spec.rb b/spec/tasks/gitlab/storage_rake_spec.rb
index 3b8d5ad7015..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, :migrate)
+            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, :migrate)
+            expect(::HashedStorage::MigratorWorker).to receive(:perform_async).with(first, last)
           end
 
           run_rake_task(task)
diff --git a/spec/workers/hashed_storage/migrator_worker_spec.rb b/spec/workers/hashed_storage/migrator_worker_spec.rb
new file mode 100644
index 00000000000..a85f820a3eb
--- /dev/null
+++ b/spec/workers/hashed_storage/migrator_worker_spec.rb
@@ -0,0 +1,25 @@
+require 'spec_helper'
+
+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(start: 5, finish: 10)
+
+      worker.perform(5, 10)
+    end
+
+    it 'migrates projects in the specified range' do
+      perform_enqueued_jobs do
+        worker.perform(ids.min, ids.max)
+      end
+
+      projects.each do |project|
+        expect(project.reload.hashed_storage?(:attachments)).to be_truthy
+      end
+    end
+  end
+end
diff --git a/spec/workers/project_migrate_hashed_storage_worker_spec.rb b/spec/workers/project_migrate_hashed_storage_worker_spec.rb
index 18470923316..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::HashedStorage::MigrationService).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::HashedStorage::MigrationService).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::HashedStorage::MigrationService)
+      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::HashedStorage::MigrationService).not_to receive(:new)
+      expect(migration_service).not_to receive(:new)
 
       subject.perform(project.id)
     end
diff --git a/spec/workers/storage_migrator_worker_spec.rb b/spec/workers/storage_migrator_worker_spec.rb
deleted file mode 100644
index 63f3c1d1c59..00000000000
--- a/spec/workers/storage_migrator_worker_spec.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-require 'spec_helper'
-
-describe StorageMigratorWorker 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(start: 5, finish: 10, operation: :migrate)
-
-      worker.perform(5, 10)
-    end
-
-    it 'migrates projects in the specified range' do
-      perform_enqueued_jobs do
-        worker.perform(ids.min, ids.max)
-      end
-
-      projects.each do |project|
-        expect(project.reload.hashed_storage?(:attachments)).to be_truthy
-      end
-    end
-  end
-end
-- 
cgit v1.2.1


From 02ac66de9063178ef03c3580cae886e4137948be Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Thu, 24 Jan 2019 19:07:10 +0100
Subject: Track when MigrateAttachmentsService is skipped

We need this new state for the Geo event logic in EE
---
 .../projects/hashed_storage/migrate_attachments_service.rb |  6 ++++++
 .../hashed_storage/migrate_attachments_service_spec.rb     | 14 ++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/app/services/projects/hashed_storage/migrate_attachments_service.rb b/app/services/projects/hashed_storage/migrate_attachments_service.rb
index 7476980a117..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,11 +33,16 @@ module Projects
         result
       end
 
+      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
 
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 bef12df583e..61dbb57ec08 100644
--- a/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
+++ b/spec/services/projects/hashed_storage/migrate_attachments_service_spec.rb
@@ -32,6 +32,12 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
       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
@@ -49,10 +55,14 @@ describe Projects::HashedStorage::MigrateAttachmentsService do
       end
 
       it 'returns true' do
-        expect(FileUtils).not_to receive(:mv).with(base_path(legacy_storage), base_path(hashed_storage))
-
         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
-- 
cgit v1.2.1


From 33ec8f7e7ca99777cebd3b5e6ab7ace8c3a5d6b5 Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Thu, 24 Jan 2019 20:31:58 +0100
Subject: Sidekiq queue migration for HashedStorage::MigratorWorker

Migrate jobs from `storage_migrator` to
`hashed_storage:hashed_storage_migrator`.
---
 ...00344_migrate_storage_migrator_sidekiq_queue.rb | 18 +++++++++
 db/schema.rb                                       |  2 +-
 .../migrate_storage_migrator_sidekiq_queue_spec.rb | 47 ++++++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 db/post_migrate/20190124200344_migrate_storage_migrator_sidekiq_queue.rb
 create mode 100644 spec/migrations/migrate_storage_migrator_sidekiq_queue_spec.rb

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/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
-- 
cgit v1.2.1


From 41712ebe0b22913c6fb844e62c77dc666b0b06a2 Mon Sep 17 00:00:00 2001
From: Gabriel Mazetto <brodock@gmail.com>
Date: Fri, 25 Jan 2019 19:05:26 +0100
Subject: Use Gitlab::AppLogger instead of Rails.logger

---
 app/services/projects/hashed_storage/base_repository_service.rb | 2 +-
 app/services/projects/hashed_storage/migration_service.rb       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/services/projects/hashed_storage/base_repository_service.rb b/app/services/projects/hashed_storage/base_repository_service.rb
index cc5366f1f61..761c81d776f 100644
--- a/app/services/projects/hashed_storage/base_repository_service.rb
+++ b/app/services/projects/hashed_storage/base_repository_service.rb
@@ -15,7 +15,7 @@ module Projects
 
       def initialize(project, old_disk_path, logger: nil)
         @project = project
-        @logger = logger || Rails.logger
+        @logger = logger || Gitlab::AppLogger
         @old_disk_path = old_disk_path
         @old_wiki_disk_path = "#{old_disk_path}.wiki"
         @move_wiki = has_wiki?
diff --git a/app/services/projects/hashed_storage/migration_service.rb b/app/services/projects/hashed_storage/migration_service.rb
index 7320f41ebf0..f132dca61c9 100644
--- a/app/services/projects/hashed_storage/migration_service.rb
+++ b/app/services/projects/hashed_storage/migration_service.rb
@@ -8,7 +8,7 @@ module Projects
       def initialize(project, old_disk_path, logger: nil)
         @project = project
         @old_disk_path = old_disk_path
-        @logger = logger || Rails.logger
+        @logger = logger || Gitlab::AppLogger
       end
 
       def execute
-- 
cgit v1.2.1