Merge branch 'extract_storage_move_creation' into 'master'

Extract storage move logic out of Project

See merge request gitlab-org/gitlab!32707

app/models/project.rb

@@ -2073,21 +2073,6 @@ class Project < ApplicationRecord
     end
   end
 
-  def change_repository_storage(new_repository_storage_key)
-    return if repository_read_only?
-    return if repository_storage == new_repository_storage_key
-
-    raise ArgumentError unless ::Gitlab.config.repositories.storages.key?(new_repository_storage_key)
-
-    storage_move = repository_storage_moves.create!(
-      source_storage_name: repository_storage,
-      destination_storage_name: new_repository_storage_key
-    )
-    storage_move.schedule!
-
-    self.repository_read_only = true
-  end
-
   def pushes_since_gc
     Gitlab::Redis::SharedState.with { |redis| redis.get(pushes_since_gc_redis_shared_state_key).to_i }
   end

app/models/project_repository_storage_move.rb

@@ -18,6 +18,7 @@ class ProjectRepositoryStorageMove < ApplicationRecord
     on: :create,
     presence: true,
     inclusion: { in: ->(_) { Gitlab.config.repositories.storages.keys } }
+  validate :project_repository_writable, on: :create
 
   state_machine initial: :initial do
     event :schedule do
@@ -36,7 +37,9 @@ class ProjectRepositoryStorageMove < ApplicationRecord
       transition [:initial, :scheduled, :started] => :failed
     end
 
-    after_transition initial: :scheduled do |storage_move, _|
+    after_transition initial: :scheduled do |storage_move|
+      storage_move.project.update_column(:repository_read_only, true)
+
       storage_move.run_after_commit do
         ProjectUpdateRepositoryStorageWorker.perform_async(
           storage_move.project_id,
@@ -46,6 +49,17 @@ class ProjectRepositoryStorageMove < ApplicationRecord
       end
     end
 
+    after_transition started: :finished do |storage_move|
+      storage_move.project.update_columns(
+        repository_read_only: false,
+        repository_storage: storage_move.destination_storage_name
+      )
+    end
+
+    after_transition started: :failed do |storage_move|
+      storage_move.project.update_column(:repository_read_only, false)
+    end
+
     state :initial, value: 1
     state :scheduled, value: 2
     state :started, value: 3
@@ -55,4 +69,10 @@ class ProjectRepositoryStorageMove < ApplicationRecord
 
   scope :order_created_at_desc, -> { order(created_at: :desc) }
   scope :with_projects, -> { includes(project: :route) }
+
+  private
+
+  def project_repository_writable
+    errors.add(:project, _('is read only')) if project&.repository_read_only?
+  end
 end

app/services/projects/update_repository_storage_service.rb

@@ -24,7 +24,7 @@ module Projects
         mark_old_paths_for_archive
 
         repository_storage_move.finish!
-        project.update!(repository_storage: destination_storage_name, repository_read_only: false)
+
         project.leave_pool_repository
         project.track_project_repository
       end
@@ -34,10 +34,7 @@ module Projects
       ServiceResponse.success
 
     rescue StandardError => e
-      project.transaction do
       repository_storage_move.do_fail!
-        project.update!(repository_read_only: false)
-      end
 
       Gitlab::ErrorTracking.track_exception(e, project_path: project.full_path)
 

app/services/projects/update_service.rb

@@ -13,8 +13,12 @@ module Projects
 
       ensure_wiki_exists if enabling_wiki?
 
-      if changing_storage_size?
-        project.change_repository_storage(params.delete(:repository_storage))
+      if changing_repository_storage?
+        storage_move = project.repository_storage_moves.build(
+          source_storage_name: project.repository_storage,
+          destination_storage_name: params.delete(:repository_storage)
+        )
+        storage_move.schedule
       end
 
       yield if block_given?
@@ -145,10 +149,11 @@ module Projects
       project.previous_changes.include?(:pages_https_only)
     end
 
-    def changing_storage_size?
+    def changing_repository_storage?
       new_repository_storage = params[:repository_storage]
 
       new_repository_storage && project.repository.exists? &&
+        project.repository_storage != new_repository_storage &&
         can?(current_user, :change_repository_storage, project)
     end
   end

locale/gitlab.pot

@@ -26101,6 +26101,9 @@ msgstr ""
 msgid "is not in the group enforcing Group Managed Account"
 msgstr ""
 
+msgid "is read only"
+msgstr ""
+
 msgid "is too long (%{current_value}). The maximum size is %{max_size}."
 msgstr ""
 

spec/factories/project_repository_storage_moves.rb

@@ -10,5 +10,9 @@ FactoryBot.define do
     trait :scheduled do
       state { ProjectRepositoryStorageMove.state_machines[:state].states[:scheduled].value }
     end
+
+    trait :started do
+      state { ProjectRepositoryStorageMove.state_machines[:state].states[:started].value }
+    end
   end
 end

spec/models/project_repository_storage_move_spec.rb

@@ -30,25 +30,36 @@ RSpec.describe ProjectRepositoryStorageMove, type: :model do
         expect(subject.errors[:destination_storage_name].first).to match(/is not included in the list/)
       end
     end
-  end
 
-  describe 'state transitions' do
-    using RSpec::Parameterized::TableSyntax
+    context 'project repository read-only' do
+      subject { build(:project_repository_storage_move, project: project) }
 
-    context 'when in the default state' do
-      subject(:storage_move) { create(:project_repository_storage_move, project: project, destination_storage_name: 'test_second_storage') }
+      let(:project) { build(:project, repository_read_only: true) }
+
+      it "does not allow the project to be read-only on create" do
+        expect(subject).not_to be_valid
+        expect(subject.errors[:project].first).to match(/is read only/)
+      end
+    end
+  end
 
+  describe 'state transitions' do
     let(:project) { create(:project) }
 
     before do
       stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' })
     end
 
+    context 'when in the default state' do
+      subject(:storage_move) { create(:project_repository_storage_move, project: project, destination_storage_name: 'test_second_storage') }
+
       context 'and transits to scheduled' do
         it 'triggers ProjectUpdateRepositoryStorageWorker' do
           expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage', storage_move.id)
 
           storage_move.schedule!
+
+          expect(project).to be_repository_read_only
         end
       end
 
@@ -59,5 +70,26 @@ RSpec.describe ProjectRepositoryStorageMove, type: :model do
         end
       end
     end
+
+    context 'when started' do
+      subject(:storage_move) { create(:project_repository_storage_move, :started, project: project, destination_storage_name: 'test_second_storage') }
+
+      context 'and transits to finished' do
+        it 'sets the repository storage and marks the project as writable' do
+          storage_move.finish!
+
+          expect(project.repository_storage).to eq('test_second_storage')
+          expect(project).not_to be_repository_read_only
+        end
+      end
+
+      context 'and transits to failed' do
+        it 'marks the project as writable' do
+          storage_move.do_fail!
+
+          expect(project).not_to be_repository_read_only
+        end
+      end
+    end
   end
 end

spec/models/project_spec.rb

@@ -2837,48 +2837,6 @@ describe Project do
     end
   end
 
-  describe '#change_repository_storage' do
-    let(:project) { create(:project, :repository) }
-    let(:read_only_project) { create(:project, :repository, repository_read_only: true) }
-
-    before do
-      stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' })
-    end
-
-    it 'schedules the transfer of the repository to the new storage and locks the project' do
-      expect(ProjectUpdateRepositoryStorageWorker).to receive(:perform_async).with(project.id, 'test_second_storage', anything)
-
-      project.change_repository_storage('test_second_storage')
-      project.save!
-
-      expect(project).to be_repository_read_only
-      expect(project.repository_storage_moves.last).to have_attributes(
-        source_storage_name: "default",
-        destination_storage_name: "test_second_storage"
-      )
-    end
-
-    it "doesn't schedule the transfer if the repository is already read-only" do
-      expect(ProjectUpdateRepositoryStorageWorker).not_to receive(:perform_async)
-
-      read_only_project.change_repository_storage('test_second_storage')
-      read_only_project.save!
-    end
-
-    it "doesn't lock or schedule the transfer if the storage hasn't changed" do
-      expect(ProjectUpdateRepositoryStorageWorker).not_to receive(:perform_async)
-
-      project.change_repository_storage(project.repository_storage)
-      project.save!
-
-      expect(project).not_to be_repository_read_only
-    end
-
-    it 'throws an error if an invalid repository storage is provided' do
-      expect { project.change_repository_storage('unknown') }.to raise_error(ArgumentError)
-    end
-  end
-
   describe '#pushes_since_gc' do
     let(:project) { create(:project) }
 

spec/requests/api/projects_spec.rb

@@ -2466,11 +2466,11 @@ describe API::Projects do
 
         let(:admin) { create(:admin) }
 
-        it 'returns 500 when repository storage is unknown' do
+        it 'returns 400 when repository storage is unknown' do
           put(api("/projects/#{new_project.id}", admin), params: { repository_storage: unknown_storage })
 
-          expect(response).to have_gitlab_http_status(:internal_server_error)
-          expect(json_response['message']).to match('ArgumentError')
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['message']['repository_storage_moves']).to eq(['is invalid'])
         end
 
         it 'returns 200 when repository storage has changed' do

spec/services/projects/update_repository_storage_service_spec.rb

@@ -16,7 +16,7 @@ describe Projects::UpdateRepositoryStorageService do
     end
 
     context 'without wiki and design repository' do
-      let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: false) }
+      let(:project) { create(:project, :repository, wiki_enabled: false) }
       let(:destination) { 'test_second_storage' }
       let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) }
       let!(:checksum) { project.repository.checksum }
@@ -131,7 +131,7 @@ describe Projects::UpdateRepositoryStorageService do
 
     context 'with wiki repository' do
       include_examples 'moves repository to another storage', 'wiki' do
-        let(:project) { create(:project, :repository, repository_read_only: true, wiki_enabled: true) }
+        let(:project) { create(:project, :repository, wiki_enabled: true) }
         let(:repository) { project.wiki.repository }
         let(:destination) { 'test_second_storage' }
         let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) }
@@ -144,7 +144,7 @@ describe Projects::UpdateRepositoryStorageService do
 
     context 'with design repository' do
       include_examples 'moves repository to another storage', 'design' do
-        let(:project) { create(:project, :repository, repository_read_only: true) }
+        let(:project) { create(:project, :repository) }
         let(:repository) { project.design_repository }
         let(:destination) { 'test_second_storage' }
         let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) }

spec/services/projects/update_service_spec.rb

@@ -552,6 +552,63 @@ describe Projects::UpdateService do
         end
       end
     end
+
+    describe 'when changing repository_storage' do
+      let(:repository_read_only) { false }
+      let(:project) { create(:project, :repository, repository_read_only: repository_read_only) }
+      let(:opts) { { repository_storage: 'test_second_storage' } }
+
+      before do
+        stub_storage_settings('test_second_storage' => { 'path' => 'tmp/tests/extra_storage' })
+      end
+
+      shared_examples 'the transfer was not scheduled' do
+        it 'does not schedule the transfer' do
+          expect do
+            update_project(project, user, opts)
+          end.not_to change(project.repository_storage_moves, :count)
+        end
+      end
+
+      context 'authenticated as admin' do
+        let(:user) { create(:admin) }
+
+        it 'schedules the transfer of the repository to the new storage and locks the project' do
+          update_project(project, admin, opts)
+
+          expect(project).to be_repository_read_only
+          expect(project.repository_storage_moves.last).to have_attributes(
+            state: ::ProjectRepositoryStorageMove.state_machines[:state].states[:scheduled].value,
+            source_storage_name: 'default',
+            destination_storage_name: 'test_second_storage'
+          )
+        end
+
+        context 'the repository is read-only' do
+          let(:repository_read_only) { true }
+
+          it_behaves_like 'the transfer was not scheduled'
+        end
+
+        context 'the storage has not changed' do
+          let(:opts) { { repository_storage: 'default' } }
+
+          it_behaves_like 'the transfer was not scheduled'
+        end
+
+        context 'the storage does not exist' do
+          let(:opts) { { repository_storage: 'nonexistent' } }
+
+          it_behaves_like 'the transfer was not scheduled'
+        end
+      end
+
+      context 'authenticated as user' do
+        let(:user) { create(:user) }
+
+        it_behaves_like 'the transfer was not scheduled'
+      end
+    end
   end
 
   describe '#run_auto_devops_pipeline?' do
@@ -611,25 +668,6 @@ describe Projects::UpdateService do
     end
   end
 
-  describe 'repository_storage' do
-    let(:admin) { create(:admin) }
-    let(:user) { create(:user) }
-    let(:project) { create(:project, :repository) }
-    let(:opts) { { repository_storage: 'test_second_storage' } }
-
-    it 'calls the change repository storage method if the storage changed' do
-      expect(project).to receive(:change_repository_storage).with('test_second_storage')
-
-      update_project(project, admin, opts).inspect
-    end
-
-    it "doesn't call the change repository storage for non-admin users" do
-      expect(project).not_to receive(:change_repository_storage)
-
-      update_project(project, user, opts).inspect
-    end
-  end
-
   def update_project(project, user, opts)
     described_class.new(project, user, opts).execute
   end