Don't move project backed by hashed storage when handling renamed events

app/services/geo/move_repository_service.rb

@@ -2,23 +2,24 @@ module Geo
   class MoveRepositoryService
     include Gitlab::ShellAdapter
 
-    attr_reader :id, :name, :old_path_with_namespace, :new_path_with_namespace
+    attr_reader :id, :old_path_with_namespace, :new_path_with_namespace
 
-    def initialize(id, name, old_path_with_namespace, new_path_with_namespace)
+    def initialize(id, old_path_with_namespace, new_path_with_namespace)
       @id = id
-      @name = name
       @old_path_with_namespace = old_path_with_namespace
       @new_path_with_namespace = new_path_with_namespace
     end
 
     def async_execute
-      GeoRepositoryMoveWorker.perform_async(id, name, old_path_with_namespace, new_path_with_namespace)
+      GeoRepositoryMoveWorker.perform_async(id, old_path_with_namespace, new_path_with_namespace)
     end
 
     def execute
       project = Project.find(id)
       project.expire_caches_before_rename(old_path_with_namespace)
 
+      return true if project.hashed_storage?(:repository)
+
       # Make sure target directory exists (used when transfering repositories)
       project.ensure_storage_path_exists
 

app/workers/geo_repository_move_worker.rb

@@ -2,7 +2,7 @@ class GeoRepositoryMoveWorker
   include Sidekiq::Worker
   include GeoQueue
 
-  def perform(id, name, old_path_with_namespace, new_path_with_namespace)
-    Geo::MoveRepositoryService.new(id, name, old_path_with_namespace, new_path_with_namespace).execute
+  def perform(id, old_path_with_namespace, new_path_with_namespace)
+    Geo::MoveRepositoryService.new(id, old_path_with_namespace, new_path_with_namespace).execute
   end
 end

ee/app/models/ee/project.rb

@@ -445,7 +445,6 @@ module EE
     end
     alias_method :merge_requests_ff_only_enabled?, :merge_requests_ff_only_enabled
 
-    # TODO: check storage type and NOOP when not using Legacy
     def rename_repo
       raise NotImplementedError unless defined?(super)
 

lib/gitlab/geo/log_cursor/daemon.rb

@@ -183,7 +183,7 @@ module Gitlab
           new_path = event.new_path_with_namespace
 
           job_id = ::Geo::MoveRepositoryService
-                     .new(event.project_id, '', old_path, new_path)
+                     .new(event.project_id, old_path, new_path)
                      .async_execute
 
           log_event_info(

spec/ee/spec/models/ee/namespace_spec.rb

@@ -74,7 +74,8 @@ describe Namespace do
       it 'logs the Geo::RepositoryRenamedEvent for each project inside namespace' do
         parent = create(:namespace)
         child = create(:group, name: 'child', path: 'child', parent: parent)
-        project_1 = create(:project_empty_repo, namespace: parent)
+        project_legacy_storage = create(:project_empty_repo, namespace: parent)
+        create(:project, :hashed, namespace: child)
         create(:project_empty_repo, namespace: child)
         full_path_was = "#{parent.full_path}_old"
         new_path = parent.full_path
@@ -85,10 +86,10 @@ describe Namespace do
         allow(parent).to receive(:full_path).and_return(new_path)
 
         allow(gitlab_shell).to receive(:mv_namespace)
-          .with(project_1.repository_storage_path, full_path_was, new_path)
+          .with(project_legacy_storage.repository_storage_path, full_path_was, new_path)
           .and_return(true)
 
-        expect { parent.move_dir }.to change(Geo::RepositoryRenamedEvent, :count).by(2)
+        expect { parent.move_dir }.to change(Geo::RepositoryRenamedEvent, :count).by(3)
       end
     end
   end

spec/ee/spec/models/ee/project_spec.rb

@@ -936,14 +936,19 @@ describe Project do
       let(:project) { create(:project, :repository) }
       let(:gitlab_shell) { Gitlab::Shell.new }
 
-      before do
-        allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
-        allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
-      end
+      it 'logs the Geo::RepositoryRenamedEvent for project backed by legacy storage' do
+        project_hashed_storage = create(:project, :hashed)
 
-      it 'logs the Geo::RepositoryRenamedEvent' do
-        stub_container_registry_config(enabled: false)
+        allow(project_hashed_storage).to receive(:gitlab_shell).and_return(gitlab_shell)
+        allow(project_hashed_storage).to receive(:previous_changes).and_return('path' => ['foo'])
+        allow(gitlab_shell).to receive(:mv_repository).twice.and_return(true)
 
+        expect { project_hashed_storage.rename_repo }.to change(Geo::RepositoryRenamedEvent, :count)
+      end
+
+      it 'logs the Geo::RepositoryRenamedEvent for project backed by legacy storage' do
+        allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
+        allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
         allow(gitlab_shell).to receive(:mv_repository).twice.and_return(true)
 
         expect(Geo::RepositoryRenamedEventStore).to receive(:new)

spec/lib/gitlab/geo/log_cursor/daemon_spec.rb

@@ -211,7 +211,7 @@ describe Gitlab::Geo::LogCursor::Daemon, :postgresql do
         new_path_with_namespace = repository_rename_event.new_path_with_namespace
 
         expect(::GeoRepositoryMoveWorker).to receive(:perform_async)
-          .with(project_id, '', old_path_with_namespace, new_path_with_namespace)
+          .with(project_id, old_path_with_namespace, new_path_with_namespace)
 
         daemon.run_once!
       end

spec/services/geo/move_repository_service_spec.rb

@@ -2,33 +2,47 @@ require 'spec_helper'
 
 describe Geo::MoveRepositoryService do
   let(:project) { create(:project, :repository) }
-  let(:new_path) { project.full_path + '+renamed' }
-  let(:full_new_path) { File.join(project.repository_storage_path, new_path) }
-  subject { described_class.new(project.id, project.name, project.full_path, new_path) }
+  let(:new_path) { "#{project.full_path}+renamed" }
 
   describe '#execute' do
-    it 'renames the path' do
+    it 'moves project backed by legacy storage' do
       old_path = project.repository.path_to_repo
-      expect(File.directory?(old_path)).to be_truthy
+      full_new_path = File.join(project.repository_storage_path, new_path)
 
-      expect(subject.execute).to eq(true)
+      service = described_class.new(project.id, project.full_path, new_path)
 
+      expect(File.directory?(old_path)).to be_truthy
+      expect(service.execute).to eq(true)
       expect(File.directory?(old_path)).to be_falsey
       expect(File.directory?("#{full_new_path}.git")).to be_truthy
     end
+
+    it 'does not move project backed by hashed storage' do
+      project_hashed_storage = create(:project, :hashed)
+      gitlab_shell = Gitlab::Shell.new
+
+      service = described_class.new(project_hashed_storage.id, project_hashed_storage.full_path, new_path)
+
+      allow(service).to receive(:gitlab_shell).and_return(gitlab_shell)
+
+      expect(service.execute).to eq(true)
+      expect(gitlab_shell).not_to receive(:mv_repository)
+    end
   end
 
   describe '#async_execute' do
+    subject(:service) { described_class.new(project.id, project.full_path, new_path) }
+
     it 'starts the worker' do
       expect(GeoRepositoryMoveWorker).to receive(:perform_async)
 
-      subject.async_execute
+      service.async_execute
     end
 
     it 'returns job id' do
       allow(GeoRepositoryMoveWorker).to receive(:perform_async).and_return('foo')
 
-      expect(subject.async_execute).to eq('foo')
+      expect(service.async_execute).to eq('foo')
     end
   end
 end

spec/services/geo/repository_renamed_event_store_spec.rb

@@ -6,13 +6,13 @@ describe Geo::RepositoryRenamedEventStore do
   let(:old_path) { 'foo' }
   let(:old_path_with_namespace) { "#{project.namespace.full_path}/foo" }
 
-  subject { described_class.new(project, old_path: old_path, old_path_with_namespace: old_path_with_namespace) }
+  subject(:event_store) { described_class.new(project, old_path: old_path, old_path_with_namespace: old_path_with_namespace) }
 
   describe '#create' do
     it 'does not create an event when not running on a primary node' do
       allow(Gitlab::Geo).to receive(:primary?) { false }
 
-      expect { subject.create }.not_to change(Geo::RepositoryRenamedEvent, :count)
+      expect { event_store.create }.not_to change(Geo::RepositoryRenamedEvent, :count)
     end
 
     context 'when running on a primary node' do
@@ -27,11 +27,11 @@ describe Geo::RepositoryRenamedEventStore do
       end
 
       it 'creates a renamed event' do
-        expect { subject.create }.to change(Geo::RepositoryRenamedEvent, :count).by(1)
+        expect { event_store.create }.to change(Geo::RepositoryRenamedEvent, :count).by(1)
       end
 
       it 'tracks old and new paths for project repositories' do
-        subject.create
+        event_store.create
 
         event = Geo::RepositoryRenamedEvent.last