Merge branch 'bvl-only-one-pushmirror-per-push' into 'master'

Only schedule updates of remote mirrors once per push

See merge request gitlab-org/gitlab!17902

app/services/git/base_hooks_service.rb

@@ -15,8 +15,6 @@ module Git
       # Not a hook, but it needs access to the list of changed commits
       enqueue_invalidate_cache
 
-      update_remote_mirrors
-
       success
     end
 
@@ -121,13 +119,6 @@ module Git
       {}
     end
 
-    def update_remote_mirrors
-      return unless project.has_remote_mirror?
-
-      project.mark_stuck_remote_mirrors_as_failed!
-      project.update_remote_mirrors
-    end
-
     def log_pipeline_errors(exception)
       data = {
         class: self.class.name,

app/services/git/branch_push_service.rb

@@ -57,13 +57,6 @@ module Git
       Ci::StopEnvironmentsService.new(project, current_user).execute(branch_name)
     end
 
-    def update_remote_mirrors
-      return unless project.has_remote_mirror?
-
-      project.mark_stuck_remote_mirrors_as_failed!
-      project.update_remote_mirrors
-    end
-
     def execute_related_hooks
       BranchHooksService.new(project, current_user, params).execute
     end

app/workers/post_receive.rb

@@ -70,6 +70,7 @@ class PostReceive
       refs << ref
     end
 
+    update_remote_mirrors(post_received)
     after_project_changes_hooks(post_received, user, refs.to_a, changes)
   end
 
@@ -93,6 +94,16 @@ class PostReceive
     )
   end
 
+  def update_remote_mirrors(post_received)
+    return unless post_received.includes_branches? || post_received.includes_tags?
+
+    project = post_received.project
+    return unless project.has_remote_mirror?
+
+    project.mark_stuck_remote_mirrors_as_failed!
+    project.update_remote_mirrors
+  end
+
   def after_project_changes_hooks(post_received, user, refs, changes)
     hook_data = Gitlab::DataBuilder::Repository.update(post_received.project, user, changes, refs)
     SystemHooksService.new.execute_hooks(hook_data, :repository_update_hooks)

changelogs/unreleased/bvl-only-one-pushmirror-per-push.yml

+---
+title: Only schedule updating push-mirrors once per push
+merge_request: 17902
+author:
+type: performance

spec/services/git/base_hooks_service_spec.rb

@@ -85,78 +85,4 @@ describe Git::BaseHooksService do
       end
     end
   end
-
-  describe 'with remote mirrors' do
-    class TestService < described_class
-      def commits
-        []
-      end
-    end
-
-    let(:project) { create(:project, :repository, :remote_mirror) }
-
-    subject { TestService.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref) }
-
-    before do
-      expect(subject).to receive(:execute_project_hooks)
-    end
-
-    context 'when remote mirror feature is enabled' do
-      it 'fails stuck remote mirrors' do
-        allow(project).to receive(:update_remote_mirrors).and_return(project.remote_mirrors)
-        expect(project).to receive(:mark_stuck_remote_mirrors_as_failed!)
-
-        subject.execute
-      end
-
-      it 'updates remote mirrors' do
-        expect(project).to receive(:update_remote_mirrors)
-
-        subject.execute
-      end
-    end
-
-    context 'when remote mirror feature is disabled' do
-      before do
-        stub_application_setting(mirror_available: false)
-      end
-
-      context 'with remote mirrors global setting overridden' do
-        before do
-          project.remote_mirror_available_overridden = true
-        end
-
-        it 'fails stuck remote mirrors' do
-          allow(project).to receive(:update_remote_mirrors).and_return(project.remote_mirrors)
-          expect(project).to receive(:mark_stuck_remote_mirrors_as_failed!)
-
-          subject.execute
-        end
-
-        it 'updates remote mirrors' do
-          expect(project).to receive(:update_remote_mirrors)
-
-          subject.execute
-        end
-      end
-
-      context 'without remote mirrors global setting overridden' do
-        before do
-          project.remote_mirror_available_overridden = false
-        end
-
-        it 'does not fails stuck remote mirrors' do
-          expect(project).not_to receive(:mark_stuck_remote_mirrors_as_failed!)
-
-          subject.execute
-        end
-
-        it 'does not updates remote mirrors' do
-          expect(project).not_to receive(:update_remote_mirrors)
-
-          subject.execute
-        end
-      end
-    end
-  end
 end

spec/services/git/branch_hooks_service_spec.rb

@@ -19,12 +19,6 @@ describe Git::BranchHooksService do
     described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
   end
 
-  it 'update remote mirrors' do
-    expect(service).to receive(:update_remote_mirrors).and_call_original
-
-    service.execute
-  end
-
   describe "Git Push Data" do
     subject(:push_data) { service.send(:push_data) }
 

spec/services/git/tag_hooks_service_spec.rb

@@ -18,12 +18,6 @@ describe Git::TagHooksService, :service do
     described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
   end
 
-  it 'update remote mirrors' do
-    expect(service).to receive(:update_remote_mirrors).and_call_original
-
-    service.execute
-  end
-
   describe 'System hooks' do
     it 'Executes system hooks' do
       push_data = service.send(:push_data)

spec/workers/post_receive_spec.rb

@@ -61,6 +61,16 @@ describe PostReceive do
       end
     end
 
+    shared_examples 'not updating remote mirrors' do
+      it 'does not schedule an update' do
+        expect(project).not_to receive(:has_remote_mirror?)
+        expect(project).not_to receive(:mark_stuck_remote_mirrors_as_failed!)
+        expect(project).not_to receive(:update_remote_mirrors)
+
+        perform
+      end
+    end
+
     context 'empty changes' do
       it "does not call any PushService but runs after project hooks" do
         expect(Git::BranchPushService).not_to receive(:new)
@@ -69,6 +79,8 @@ describe PostReceive do
 
         perform(changes: "")
       end
+
+      it_behaves_like 'not updating remote mirrors'
     end
 
     context 'unidentified user' do
@@ -88,6 +100,16 @@ describe PostReceive do
         allow(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::PROJECT])
       end
 
+      shared_examples 'updating remote mirrors' do
+        it 'schedules an update if the project had mirrors' do
+          expect(project).to receive(:has_remote_mirror?).and_return(true)
+          expect(project).to receive(:mark_stuck_remote_mirrors_as_failed!)
+          expect(project).to receive(:update_remote_mirrors)
+
+          perform
+        end
+      end
+
       context "branches" do
         let(:changes) do
           <<~EOF
@@ -126,6 +148,8 @@ describe PostReceive do
           perform
         end
 
+        it_behaves_like 'updating remote mirrors'
+
         context 'with a default branch' do
           let(:changes) do
             <<~EOF
@@ -191,6 +215,8 @@ describe PostReceive do
 
           perform
         end
+
+        it_behaves_like 'updating remote mirrors'
       end
 
       context "merge-requests" do
@@ -202,6 +228,8 @@ describe PostReceive do
 
           perform
         end
+
+        it_behaves_like 'not updating remote mirrors'
       end
 
       context "gitlab-ci.yml" do