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.

7 files changed, 86 insertions, 46 deletions

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/storage_migrator_worker_spec.rb b/spec/workers/hashed_storage/migrator_worker_spec.rb
index 63f3c1d1c59..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(start: 5, finish: 10, operation: :migrate)
+      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 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