Call RenameRepository instead of RenameNamespace

call RenameRepository RPC instead of RenameNamespace in gitlab_shell. This is a step towards deprecating all RenameNamespace calls.

Call RenameRepository instead of RenameNamespace
call RenameRepository RPC instead of RenameNamespace in gitlab_shell. This is a step towards deprecating all RenameNamespace calls.
fe158ad1 · John Cai · 95d16dc0 · fe158ad1 · fe158ad1 · fe158ad1
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
-1.62.0
+1.63.0
--- a/Gemfile
+++ b/Gemfile
@@ -428,7 +428,7 @@ group :ed25519 do
 end

 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 1.58.0'
+gem 'gitaly', '~> 1.63.0'

 gem 'grpc', '~> 1.19.0'


--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -332,7 +332,7 @@ GEM
      po_to_json (>= 1.0.0)
      rails (>= 3.2.0)
    git (1.5.0)
-    gitaly (1.58.0)
+    gitaly (1.63.0)
      grpc (~> 1.0)
    github-markup (1.7.0)
    gitlab-labkit (0.5.2)
@@ -1126,7 +1126,7 @@ DEPENDENCIES
  gettext (~> 3.2.2)
  gettext_i18n_rails (~> 1.8.0)
  gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 1.58.0)
+  gitaly (~> 1.63.0)
  github-markup (~> 1.7.0)
  gitlab-labkit (~> 0.5)
  gitlab-markup (~> 1.7.0)

--- a/app/models/concerns/storage/legacy_namespace.rb
+++ b/app/models/concerns/storage/legacy_namespace.rb
@@ -54,25 +54,19 @@ def prepare_for_destroy
    private

    def move_repositories
-      # Move the namespace directory in all storages used by member projects
-      repository_storages.each do |repository_storage|
-        # Ensure old directory exists before moving it
-        gitlab_shell.add_namespace(repository_storage, full_path_before_last_save)
+      all_projects.each do |project|
+        next unless project.legacy_storage?

-        # Ensure new directory exists before moving it (if there's a parent)
-        gitlab_shell.add_namespace(repository_storage, parent.full_path) if parent
-
-        unless gitlab_shell.mv_namespace(repository_storage, full_path_before_last_save, full_path)
-
-          Rails.logger.error "Exception moving path #{repository_storage} from #{full_path_before_last_save} to #{full_path}" # rubocop:disable Gitlab/RailsLogger
-
-          # if we cannot move namespace directory we should rollback
-          # db changes in order to prevent out of sync between db and fs
+        unless gitlab_shell.mv_repository(project.repository_storage, full_repo_path_before_last_save(project), project.disk_path )
          raise Gitlab::UpdatePathError.new('namespace directory cannot be moved')
        end
      end
    end

+    def full_repo_path_before_last_save(project)
+      project.disk_path.gsub(full_path, full_path_before_last_save)
+    end
+
    def old_repository_storages
      @old_repository_storage_paths ||= repository_storages
    end

--- a/app/models/storage/legacy_project.rb
+++ b/app/models/storage/legacy_project.rb
@@ -38,7 +38,7 @@ def rename_repo(old_full_path: nil, new_full_path: nil)
        # However we cannot allow rollback since we moved repository
        # So we basically we mute exceptions in next actions
        begin
-          gitlab_shell.mv_repository(repository_storage, "#{old_full_path}.wiki", "#{new_full_path}.wiki")
+          gitlab_shell.mv_wiki(repository_storage, old_full_path, new_full_path) if project.wiki.exists?
          return true
        rescue => e
          Rails.logger.error "Exception renaming #{old_full_path} -> #{new_full_path}: #{e}" # rubocop:disable Gitlab/RailsLogger

--- a/app/services/projects/hashed_storage/base_repository_service.rb
+++ b/app/services/projects/hashed_storage/base_repository_service.rb
@@ -29,20 +29,12 @@ def has_wiki?
      # 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}) ..."

-          # We return true so we still reflect the change in the database.
-          # Next time the repository is (re)created it will be under the new storage layout
-          return true
-        elsif !from_exists
-          # Repository have been moved already.
-          return true
-        end
+        # Repository have been moved already.
+        return true unless from_exists

        gitlab_shell.mv_repository(project.repository_storage, from_name, to_name)
      end

--- a/app/services/projects/hashed_storage/migrate_repository_service.rb
+++ b/app/services/projects/hashed_storage/migrate_repository_service.rb
@@ -8,7 +8,6 @@ def execute

        @old_storage_version = project.storage_version
        project.storage_version = ::Project::HASHED_STORAGE_FEATURES[:repository]
-        project.ensure_storage_path_exists

        @new_disk_path = project.disk_path


--- a/changelogs/unreleased/jc-legacy-storage-to-hashed-storage.yml
+++ b/changelogs/unreleased/jc-legacy-storage-to-hashed-storage.yml
+---
+title: Call RenameRepository instead of RenameNamespace
+merge_request: 32684
+author:
+type: deprecated
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -131,6 +131,12 @@ def branch_count
        end
      end

+      def rename(new_relative_path)
+        wrapped_gitaly_errors do
+          gitaly_repository_client.rename(new_relative_path)
+        end
+      end
+
      def expire_has_local_branches_cache
        clear_memoization(:has_local_branches)
      end

--- a/lib/gitlab/gitaly_client/repository_service.rb
+++ b/lib/gitlab/gitaly_client/repository_service.rb
@@ -346,6 +346,12 @@ def disconnect_alternates
        GitalyClient.call(@storage, :object_pool_service, :disconnect_git_alternates, request)
      end

+      def rename(relative_path)
+        request = Gitaly::RenameRepositoryRequest.new(repository: @gitaly_repo, relative_path: relative_path)
+
+        GitalyClient.call(@storage, :repository_service, :rename_repository, request, timeout: GitalyClient.fast_timeout)
+      end
+
      private

      def search_results_from_response(gitaly_response)

--- a/lib/gitlab/shell.rb
+++ b/lib/gitlab/shell.rb
@@ -117,16 +117,26 @@ def import_repository(storage, name, url, gl_project_path)
    # mv_namespace. Given the underlying implementation is a move action,
    # indescriminate of what the folders might be.
    #
-    # storage - project's storage path
+    # storage - project's storage name
    # path - project disk path
    # new_path - new project disk path
    #
    # Ex.
-    #   mv_repository("/path/to/storage", "gitlab/gitlab-ci", "randx/gitlab-ci-new")
+    #   mv_repository("default", "gitlab/gitlab-ci", "randx/gitlab-ci-new")
    def mv_repository(storage, path, new_path)
      return false if path.empty? || new_path.empty?

-      !!mv_directory(storage, "#{path}.git", "#{new_path}.git")
+      Gitlab::Git::Repository.new(storage, "#{path}.git", nil, nil).rename("#{new_path}.git")
+    rescue => e
+      Gitlab::Sentry.track_acceptable_exception(e, extra: { path: path, new_path: new_path, storage: storage })
+
+      false
+    end
+
+    def mv_wiki(storage, path, new_path)
+      return false if path.empty? || new_path.empty?
+
+      Gitlab::Git::Repository.new(storage, "#{path}.wiki.git", nil, nil).rename("#{new_path}.wiki.git")
    end

    # Fork repository to new path

--- a/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb
+++ b/spec/lib/gitlab/database/rename_reserved_paths_migration/v1/rename_projects_spec.rb
@@ -21,6 +21,8 @@
    it 'searches using nested paths' do
      namespace = create(:namespace, path: 'hello')
      project = create(:project, :legacy_storage, path: 'THE-path', namespace: namespace)
+      project.create_repository
+      project.create_wiki

      result_ids = described_class.new(['Hello/the-path'], migration)
                     .projects_for_paths.map(&:id)
@@ -30,6 +32,9 @@

    it 'includes the correct projects' do
      project = create(:project, :legacy_storage, path: 'THE-path')
+      project.create_repository
+      project.create_wiki
+
      _other_project = create(:project, :legacy_storage)

      result_ids = subject.projects_for_paths.map(&:id)
@@ -41,6 +46,13 @@
  describe '#rename_projects' do
    let!(:projects) { create_list(:project, 2, :legacy_storage, path: 'the-path') }

+    before do
+      projects.each do |p|
+        p.create_repository
+        p.create_wiki
+      end
+    end
+
    it 'renames each project' do
      expect(subject).to receive(:rename_project).twice

@@ -56,6 +68,10 @@
  end

  describe '#rename_project' do
+    before do
+      project.create_repository
+      project.create_wiki
+    end
    it 'renames path & route for the project' do
      expect(subject).to receive(:rename_path_for_routable)
                           .with(project)
@@ -81,6 +97,11 @@
  end

  describe '#move_project_folders' do
+    before do
+      project.create_repository
+      project.create_wiki
+    end
+
    it 'moves the wiki & the repo' do
      expect(subject).to receive(:move_repository)
                           .with(project, 'known-parent/the-path.wiki', 'known-parent/the-path0.wiki')
@@ -125,6 +146,11 @@
    let(:known_parent) { create(:namespace, path: 'known-parent') }
    let(:project) { create(:project, :repository, :legacy_storage, path: 'the-path', namespace: known_parent) }

+    before do
+      project.create_repository
+      project.create_wiki
+    end
+
    it 'moves the repository for a project' do
      expected_path = File.join(TestEnv.repos_path, 'known-parent', 'new-repo.git')

@@ -135,6 +161,11 @@
  end

  describe '#revert_renames', :redis do
+    before do
+      project.create_repository
+      project.create_wiki
+    end
+
    it 'renames the routes back to the previous values' do
      subject.rename_project(project)

@@ -152,7 +183,6 @@
    end

    it 'moves the repositories back to their original place' do
-      project.create_repository
      subject.rename_project(project)

      expected_path = File.join(TestEnv.repos_path, 'known-parent', 'the-path.git')

--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -362,9 +362,9 @@ def expect_project_directories_at(namespace_path)
      it 'updates project full path in .git/config' do
        parent.update(path: 'mygroup_new')

-        expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}"
-        expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}"
-        expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}"
+        expect(project_rugged(project_in_parent_group.reload).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}"
+        expect(project_rugged(hashed_project_in_subgroup.reload).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}"
+        expect(project_rugged(legacy_project_in_subgroup.reload).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}"
      end

      it 'updates the project storage location' do