Merge branch 'move_same_fs_check' into 'master'

Move SameFilesystemError into service

See merge request gitlab-org/gitlab!29218

app/services/projects/update_repository_storage_service.rb

@@ -5,12 +5,15 @@ module Projects
     include Gitlab::ShellAdapter
 
     Error = Class.new(StandardError)
+    SameFilesystemError = Class.new(Error)
 
     def initialize(project)
       @project = project
     end
 
     def execute(new_repository_storage_key)
+      raise SameFilesystemError if same_filesystem?(project.repository.storage, new_repository_storage_key)
+
       mirror_repositories(new_repository_storage_key)
 
       mark_old_paths_for_archive
@@ -33,6 +36,10 @@ module Projects
 
     private
 
+    def same_filesystem?(old_storage, new_storage)
+      Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage)
+    end
+
     def mirror_repositories(new_repository_storage_key)
       mirror_repository(new_repository_storage_key)
 

app/workers/project_update_repository_storage_worker.rb

@@ -3,21 +3,11 @@
 class ProjectUpdateRepositoryStorageWorker # rubocop:disable Scalability/IdempotentWorker
   include ApplicationWorker
 
-  SameFilesystemError = Class.new(StandardError)
-
   feature_category :gitaly
 
   def perform(project_id, new_repository_storage_key)
     project = Project.find(project_id)
 
-    raise SameFilesystemError if same_filesystem?(project.repository.storage, new_repository_storage_key)
-
     ::Projects::UpdateRepositoryStorageService.new(project).execute(new_repository_storage_key)
   end
-
-  private
-
-  def same_filesystem?(old_storage, new_storage)
-    Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage)
-  end
 end

spec/services/projects/fork_service_spec.rb

@@ -311,6 +311,8 @@ describe Projects::ForkService do
       fork_before_move = fork_project(project)
 
       # Stub everything required to move a project to a Gitaly shard that does not exist
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
       stub_storage_settings('test_second_storage' => { 'path' => TestEnv::SECOND_STORAGE_PATH })
       allow_any_instance_of(Gitlab::Git::Repository).to receive(:create_repository)
         .and_return(true)

spec/services/projects/update_repository_storage_service_spec.rb

@@ -20,6 +20,8 @@ describe Projects::UpdateRepositoryStorageService do
       let(:project_repository_double) { double(:repository) }
 
       before do
+        allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+        allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
         allow(Gitlab::Git::Repository).to receive(:new).and_call_original
         allow(Gitlab::Git::Repository).to receive(:new)
           .with('test_second_storage', project.repository.raw.relative_path, project.repository.gl_repository, project.repository.full_path)
@@ -49,17 +51,20 @@ describe Projects::UpdateRepositoryStorageService do
         end
       end
 
-      context 'when the project is already on the target storage' do
+      context 'when the filesystems are the same' do
         it 'bails out and does nothing' do
           result = subject.execute(project.repository_storage)
 
           expect(result[:status]).to eq(:error)
-          expect(result[:message]).to match(/repository and source have the same storage/)
+          expect(result[:message]).to match(/SameFilesystemError/)
         end
       end
 
       context 'when the move fails' do
         it 'unmarks the repository as read-only without updating the repository storage' do
+          allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+          allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
+
           expect(project_repository_double).to receive(:create_repository)
             .and_return(true)
           expect(project_repository_double).to receive(:replicate)
@@ -77,6 +82,9 @@ describe Projects::UpdateRepositoryStorageService do
 
       context 'when the checksum does not match' do
         it 'unmarks the repository as read-only without updating the repository storage' do
+          allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+          allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
+
           expect(project_repository_double).to receive(:create_repository)
             .and_return(true)
           expect(project_repository_double).to receive(:replicate)
@@ -97,6 +105,9 @@ describe Projects::UpdateRepositoryStorageService do
         let!(:pool) { create(:pool_repository, :ready, source_project: project) }
 
         it 'leaves the pool' do
+          allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+          allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
+
           expect(project_repository_double).to receive(:create_repository)
             .and_return(true)
           expect(project_repository_double).to receive(:replicate)

spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb

@@ -22,6 +22,9 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context 'when the move succeeds', :clean_gitlab_redis_shared_state do
     before do
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
+
       allow(project_repository_double).to receive(:create_repository)
         .and_return(true)
       allow(project_repository_double).to receive(:replicate)
@@ -83,17 +86,19 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
     end
   end
 
-  context 'when the project is already on the target storage' do
+  context 'when the filesystems are the same' do
     it 'bails out and does nothing' do
       result = subject.execute(project.repository_storage)
 
       expect(result[:status]).to eq(:error)
-      expect(result[:message]).to match(/repository and source have the same storage/)
+      expect(result[:message]).to match(/SameFilesystemError/)
     end
   end
 
   context "when the move of the #{repository_type} repository fails" do
     it 'unmarks the repository as read-only without updating the repository storage' do
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
       allow(project_repository_double).to receive(:create_repository)
         .and_return(true)
       allow(project_repository_double).to receive(:replicate)
@@ -119,6 +124,8 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
   context "when the checksum of the #{repository_type} repository does not match" do
     it 'unmarks the repository as read-only without updating the repository storage' do
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('test_second_storage').and_return(SecureRandom.uuid)
       allow(project_repository_double).to receive(:create_repository)
         .and_return(true)
       allow(project_repository_double).to receive(:replicate)

spec/workers/project_update_repository_storage_worker_spec.rb

@@ -9,33 +9,12 @@ describe ProjectUpdateRepositoryStorageWorker do
   subject { described_class.new }
 
   describe "#perform" do
-    context 'when source and target repositories are on different filesystems' do
-      before do
-        allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
-        allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('new_storage').and_return(SecureRandom.uuid)
+    it "calls the update repository storage service" do
+      expect_next_instance_of(Projects::UpdateRepositoryStorageService) do |instance|
+        expect(instance).to receive(:execute).with('new_storage')
       end
 
-      it "calls the update repository storage service" do
-        expect_next_instance_of(Projects::UpdateRepositoryStorageService) do |instance|
-          expect(instance).to receive(:execute).with('new_storage')
-        end
-
-        subject.perform(project.id, 'new_storage')
-      end
-    end
-
-    context 'when source and target repositories are on the same filesystems' do
-      let(:filesystem_id) { SecureRandom.uuid }
-
-      before do
-        allow(Gitlab::GitalyClient).to receive(:filesystem_id).and_return(filesystem_id)
-      end
-
-      it 'raises an error' do
-        expect_any_instance_of(::Projects::UpdateRepositoryStorageService).not_to receive(:new)
-
-        expect { subject.perform(project.id, 'new_storage') }.to raise_error(ProjectUpdateRepositoryStorageWorker::SameFilesystemError)
-      end
+      subject.perform(project.id, 'new_storage')
     end
   end
 end