Merge branch '218990-fj-add-snippet-update-repository-service' into 'master'

Create Snippets::UpdateRepositoryStorageService

See merge request gitlab-org/gitlab!49226

app/services/concerns/update_repository_storage_methods.rb

+# frozen_string_literal: true
+
+module UpdateRepositoryStorageMethods
+  Error = Class.new(StandardError)
+  SameFilesystemError = Class.new(Error)
+
+  attr_reader :repository_storage_move
+  delegate :container, :source_storage_name, :destination_storage_name, to: :repository_storage_move
+
+  def initialize(repository_storage_move)
+    @repository_storage_move = repository_storage_move
+  end
+
+  def execute
+    repository_storage_move.with_lock do
+      return ServiceResponse.success unless repository_storage_move.scheduled? # rubocop:disable Cop/AvoidReturnFromBlocks
+
+      repository_storage_move.start!
+    end
+
+    raise SameFilesystemError if same_filesystem?(source_storage_name, destination_storage_name)
+
+    mirror_repositories
+
+    repository_storage_move.transaction do
+      repository_storage_move.finish_replication!
+
+      track_repository(destination_storage_name)
+    end
+
+    remove_old_paths
+    enqueue_housekeeping
+
+    repository_storage_move.finish_cleanup!
+
+    ServiceResponse.success
+  rescue StandardError => e
+    repository_storage_move.do_fail!
+
+    Gitlab::ErrorTracking.track_exception(e, container_klass: container.class.to_s, container_path: container.full_path)
+
+    ServiceResponse.error(
+      message: s_("UpdateRepositoryStorage|Error moving repository storage for %{container_full_path} - %{message}") % { container_full_path: container.full_path, message: e.message }
+    )
+  end
+
+  private
+
+  def track_repository(destination_shard)
+    raise NotImplementedError
+  end
+
+  def mirror_repositories
+    raise NotImplementedError
+  end
+
+  def mirror_repository(type:)
+    unless wait_for_pushes(type)
+      raise Error, s_('UpdateRepositoryStorage|Timeout waiting for %{type} repository pushes') % { type: type.name }
+    end
+
+    repository = type.repository_for(container)
+    full_path = repository.full_path
+    raw_repository = repository.raw
+    checksum = repository.checksum
+
+    # Initialize a git repository on the target path
+    new_repository = Gitlab::Git::Repository.new(
+      destination_storage_name,
+      raw_repository.relative_path,
+      raw_repository.gl_repository,
+      full_path
+    )
+
+    new_repository.replicate(raw_repository)
+    new_checksum = new_repository.checksum
+
+    if checksum != new_checksum
+      raise Error, s_('UpdateRepositoryStorage|Failed to verify %{type} repository checksum from %{old} to %{new}') % { type: type.name, old: checksum, new: new_checksum }
+    end
+  end
+
+  def same_filesystem?(old_storage, new_storage)
+    Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage)
+  end
+
+  def remove_old_paths
+    if container.repository_exists?
+      Gitlab::Git::Repository.new(
+        source_storage_name,
+        "#{container.disk_path}.git",
+        nil,
+        nil
+      ).remove
+    end
+  end
+
+  def enqueue_housekeeping
+    # no-op
+  end
+
+  def wait_for_pushes(type)
+    reference_counter = container.reference_counter(type: type)
+
+    # Try for 30 seconds, polling every 10
+    3.times do
+      return true if reference_counter.value == 0
+
+      sleep 10
+    end
+
+    false
+  end
+end

app/services/projects/update_repository_storage_service.rb

@@ -2,59 +2,19 @@
 
 module Projects
   class UpdateRepositoryStorageService
-    Error = Class.new(StandardError)
-    SameFilesystemError = Class.new(Error)
+    include UpdateRepositoryStorageMethods
 
-    attr_reader :repository_storage_move
-    delegate :project, :source_storage_name, :destination_storage_name, to: :repository_storage_move
+    delegate :project, to: :repository_storage_move
 
-    def initialize(repository_storage_move)
-      @repository_storage_move = repository_storage_move
-    end
-
-    def execute
-      repository_storage_move.with_lock do
-        return ServiceResponse.success unless repository_storage_move.scheduled? # rubocop:disable Cop/AvoidReturnFromBlocks
-
-        repository_storage_move.start!
-      end
-
-      raise SameFilesystemError if same_filesystem?(source_storage_name, destination_storage_name)
-
-      mirror_repositories
-
-      repository_storage_move.transaction do
-        repository_storage_move.finish_replication!
+    private
 
+    def track_repository(_destination_storage_name)
       project.leave_pool_repository
       project.track_project_repository
     end
 
-      remove_old_paths
-      enqueue_housekeeping
-
-      repository_storage_move.finish_cleanup!
-
-      ServiceResponse.success
-
-    rescue StandardError => e
-      repository_storage_move.do_fail!
-
-      Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path)
-
-      ServiceResponse.error(
-        message: s_("UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}") % { project_full_path: project.full_path, message: e.message }
-      )
-    end
-
-    private
-
-    def same_filesystem?(old_storage, new_storage)
-      Gitlab::GitalyClient.filesystem_id(old_storage) == Gitlab::GitalyClient.filesystem_id(new_storage)
-    end
-
     def mirror_repositories
-      mirror_repository if project.repository_exists?
+      mirror_repository(type: Gitlab::GlRepository::PROJECT) if project.repository_exists?
 
       if project.wiki.repository_exists?
         mirror_repository(type: Gitlab::GlRepository::WIKI)
@@ -65,41 +25,21 @@ module Projects
       end
     end
 
-    def mirror_repository(type: Gitlab::GlRepository::PROJECT)
-      unless wait_for_pushes(type)
-        raise Error, s_('UpdateRepositoryStorage|Timeout waiting for %{type} repository pushes') % { type: type.name }
-      end
-
-      repository = type.repository_for(project)
-      full_path = repository.full_path
-      raw_repository = repository.raw
-      checksum = repository.checksum
-
-      # Initialize a git repository on the target path
-      new_repository = Gitlab::Git::Repository.new(
-        destination_storage_name,
-        raw_repository.relative_path,
-        raw_repository.gl_repository,
-        full_path
-      )
-
-      new_repository.replicate(raw_repository)
-      new_checksum = new_repository.checksum
+    # The underlying FetchInternalRemote call uses a `git fetch` to move data
+    # to the new repository, which leaves it in a less-well-packed state,
+    # lacking bitmaps and commit graphs. Housekeeping will boost performance
+    # significantly.
+    def enqueue_housekeeping
+      return unless Gitlab::CurrentSettings.housekeeping_enabled?
+      return unless Feature.enabled?(:repack_after_shard_migration, project)
 
-      if checksum != new_checksum
-        raise Error, s_('UpdateRepositoryStorage|Failed to verify %{type} repository checksum from %{old} to %{new}') % { type: type.name, old: checksum, new: new_checksum }
-      end
+      Projects::HousekeepingService.new(project, :gc).execute
+    rescue Projects::HousekeepingService::LeaseTaken
+      # No action required
     end
 
     def remove_old_paths
-      if project.repository_exists?
-        Gitlab::Git::Repository.new(
-          source_storage_name,
-          "#{project.disk_path}.git",
-          nil,
-          nil
-        ).remove
-      end
+      super
 
       if project.wiki.repository_exists?
         Gitlab::Git::Repository.new(
@@ -119,31 +59,5 @@ module Projects
         ).remove
       end
     end
-
-    # The underlying FetchInternalRemote call uses a `git fetch` to move data
-    # to the new repository, which leaves it in a less-well-packed state,
-    # lacking bitmaps and commit graphs. Housekeeping will boost performance
-    # significantly.
-    def enqueue_housekeeping
-      return unless Gitlab::CurrentSettings.housekeeping_enabled?
-      return unless Feature.enabled?(:repack_after_shard_migration, project)
-
-      Projects::HousekeepingService.new(project, :gc).execute
-    rescue Projects::HousekeepingService::LeaseTaken
-      # No action required
-    end
-
-    def wait_for_pushes(type)
-      reference_counter = project.reference_counter(type: type)
-
-      # Try for 30 seconds, polling every 10
-      3.times do
-        return true if reference_counter.value == 0
-
-        sleep 10
-      end
-
-      false
-    end
   end
 end

app/services/snippets/update_repository_storage_service.rb

+# frozen_string_literal: true
+
+module Snippets
+  class UpdateRepositoryStorageService
+    include UpdateRepositoryStorageMethods
+
+    delegate :snippet, to: :repository_storage_move
+
+    private
+
+    def track_repository(destination_storage_name)
+      snippet.track_snippet_repository(destination_storage_name)
+    end
+
+    def mirror_repositories
+      return unless snippet.repository_exists?
+
+      mirror_repository(type: Gitlab::GlRepository::SNIPPET)
+    end
+  end
+end

locale/gitlab.pot

@@ -29920,7 +29920,7 @@ msgstr ""
 msgid "UpdateProject|Project could not be updated!"
 msgstr ""
 
-msgid "UpdateRepositoryStorage|Error moving repository storage for %{project_full_path} - %{message}"
+msgid "UpdateRepositoryStorage|Error moving repository storage for %{container_full_path} - %{message}"
 msgstr ""
 
 msgid "UpdateRepositoryStorage|Failed to verify %{type} repository checksum from %{old} to %{new}"

spec/services/snippets/update_repository_storage_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Snippets::UpdateRepositoryStorageService do
+  include Gitlab::ShellAdapter
+
+  subject { described_class.new(repository_storage_move) }
+
+  describe "#execute" do
+    let_it_be_with_reload(:snippet) { create(:snippet, :repository) }
+    let_it_be(:destination) { 'test_second_storage' }
+    let_it_be(:checksum) { snippet.repository.checksum }
+
+    let(:repository_storage_move_state) { :scheduled }
+    let(:repository_storage_move) { create(:snippet_repository_storage_move, repository_storage_move_state, container: snippet, destination_storage_name: destination) }
+    let(:snippet_repository_double) { double(:repository) }
+    let(:original_snippet_repository_double) { double(:repository) }
+
+    before do
+      allow(Gitlab.config.repositories.storages).to receive(:keys).and_return(%w[default test_second_storage])
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with('default').and_call_original
+      allow(Gitlab::GitalyClient).to receive(:filesystem_id).with(destination).and_return(SecureRandom.uuid)
+      allow(Gitlab::Git::Repository).to receive(:new).and_call_original
+      allow(Gitlab::Git::Repository).to receive(:new)
+        .with(destination, snippet.repository.raw.relative_path, snippet.repository.gl_repository, snippet.repository.full_path)
+        .and_return(snippet_repository_double)
+      allow(Gitlab::Git::Repository).to receive(:new)
+        .with('default', snippet.repository.raw.relative_path, nil, nil)
+        .and_return(original_snippet_repository_double)
+    end
+
+    context 'when the move succeeds' do
+      it 'moves the repository to the new storage and unmarks the repository as read only' do
+        old_path = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+          snippet.repository.path_to_repo
+        end
+
+        expect(snippet_repository_double).to receive(:replicate)
+          .with(snippet.repository.raw)
+        expect(snippet_repository_double).to receive(:checksum)
+          .and_return(checksum)
+        expect(original_snippet_repository_double).to receive(:remove)
+
+        result = subject.execute
+        snippet.reload
+
+        expect(result).to be_success
+        expect(snippet).not_to be_repository_read_only
+        expect(snippet.repository_storage).to eq(destination)
+        expect(gitlab_shell.repository_exists?('default', old_path)).to be(false)
+        expect(snippet.snippet_repository.shard_name).to eq(destination)
+      end
+    end
+
+    context 'when the filesystems are the same' do
+      let(:destination) { snippet.repository_storage }
+
+      it 'bails out and does nothing' do
+        result = subject.execute
+
+        expect(result).to be_error
+        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
+        expect(snippet_repository_double).to receive(:replicate)
+          .with(snippet.repository.raw)
+          .and_raise(Gitlab::Git::CommandError)
+
+        result = subject.execute
+
+        expect(result).to be_error
+        expect(snippet).not_to be_repository_read_only
+        expect(snippet.repository_storage).to eq('default')
+        expect(repository_storage_move).to be_failed
+      end
+    end
+
+    context 'when the cleanup fails' do
+      it 'sets the correct state' do
+        expect(snippet_repository_double).to receive(:replicate)
+          .with(snippet.repository.raw)
+        expect(snippet_repository_double).to receive(:checksum)
+          .and_return(checksum)
+        expect(original_snippet_repository_double).to receive(:remove)
+          .and_raise(Gitlab::Git::CommandError)
+
+        result = subject.execute
+
+        expect(result).to be_error
+        expect(repository_storage_move).to be_cleanup_failed
+      end
+    end
+
+    context 'when the checksum does not match' do
+      it 'unmarks the repository as read-only without updating the repository storage' do
+        expect(snippet_repository_double).to receive(:replicate)
+          .with(snippet.repository.raw)
+        expect(snippet_repository_double).to receive(:checksum)
+          .and_return('not matching checksum')
+
+        result = subject.execute
+
+        expect(result).to be_error
+        expect(snippet).not_to be_repository_read_only
+        expect(snippet.repository_storage).to eq('default')
+      end
+    end
+
+    context 'when the repository move is finished' do
+      let(:repository_storage_move_state) { :finished }
+
+      it 'is idempotent' do
+        expect do
+          result = subject.execute
+
+          expect(result).to be_success
+        end.not_to change(repository_storage_move, :state)
+      end
+    end
+
+    context 'when the repository move is failed' do
+      let(:repository_storage_move_state) { :failed }
+
+      it 'is idempotent' do
+        expect do
+          result = subject.execute
+
+          expect(result).to be_success
+        end.not_to change(repository_storage_move, :state)
+      end
+    end
+  end
+end