Merge branch '118753-debounce-pull-mirror' into 'master'

Debounce pull mirror invocation

See merge request gitlab-org/gitlab!30157

ee/app/controllers/ee/projects/mirrors_controller.rb

@@ -41,7 +41,7 @@ module EE
           project.update_remote_mirrors
           flash[:notice] = _('The remote repository is being updated...')
         else
-          project.import_state.force_import_job!
+          StartPullMirroringService.new(project, current_user, pause_on_hard_failure: false).execute
           flash[:notice] = _('The repository is being updated...')
         end
 

ee/app/services/start_pull_mirroring_service.rb

 # frozen_string_literal: true
 
 class StartPullMirroringService < BaseService
+  INTERVAL = 5.minutes
+
   def execute
-    return error('Mirroring for the project is on pause', 403) if project.import_state.hard_failed?
+    import_state = project.import_state
+
+    if import_state.hard_failed?
+      return error('Mirroring for the project is on pause', 403) if params[:pause_on_hard_failure]
+
+      import_state.reset_retry_count
+    end
+
+    if update_now?(import_state)
+      import_state.force_import_job!
+    else
+      import_state.update(next_execution_timestamp: INTERVAL.since(import_state.last_update_at))
+    end
 
-    project.import_state.force_import_job!
     success
   end
+
+  private
+
+  def update_now?(import_state)
+    import_state.last_update_at.nil? || import_state.last_update_at < INTERVAL.ago
+  end
 end

ee/changelogs/unreleased/118753-debounce-pull-mirror.yml

+---
+title: Debounce pull mirror invocation
+merge_request: 30157
+author:
+type: performance

ee/lib/api/project_mirror.rb

@@ -59,7 +59,7 @@ module API
       end
 
       def start_pull_mirroring
-        result = StartPullMirroringService.new(project, mirror_user).execute
+        result = StartPullMirroringService.new(project, mirror_user, pause_on_hard_failure: true).execute
 
         render_api_error!(result[:message], result[:http_status]) if result[:status] == :error
       end

ee/spec/controllers/projects/mirrors_controller_spec.rb

@@ -122,9 +122,11 @@ describe Projects::MirrorsController do
       project = create(:project, :mirror)
       sign_in(project.owner)
 
-      expect_any_instance_of(EE::ProjectImportState).to receive(:force_import_job!)
-
-      put :update_now, params: { namespace_id: project.namespace.to_param, project_id: project.to_param }
+      Sidekiq::Testing.fake! do
+        expect { put :update_now, params: { namespace_id: project.namespace.to_param, project_id: project.to_param } }
+          .to change { UpdateAllMirrorsWorker.jobs.size }
+          .by(1)
+      end
     end
   end
 

ee/spec/features/projects/mirror_spec.rb

@@ -19,18 +19,20 @@ describe 'Project mirror', :js do
       let(:timestamp) { Time.now }
 
       before do
-        import_state.update(next_execution_timestamp: timestamp + 10.minutes)
+        import_state.update(last_update_at: 6.minutes.ago, next_execution_timestamp: timestamp + 10.minutes)
       end
 
       context 'when able to force update' do
         it 'forces import' do
-          expect_any_instance_of(EE::ProjectImportState).to receive(:force_import_job!)
-
           Timecop.freeze(timestamp) do
             visit project_mirror_path(project)
           end
 
-          Sidekiq::Testing.fake! { find('.js-force-update-mirror').click }
+          Sidekiq::Testing.fake! do
+            expect { find('.js-force-update-mirror').click }
+              .to change { UpdateAllMirrorsWorker.jobs.size }
+              .by(1)
+          end
         end
       end
 

ee/spec/requests/api/project_mirror_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 describe API::ProjectMirror do
   describe 'POST /projects/:id/mirror/pull' do
     let(:visibility) { Gitlab::VisibilityLevel::PUBLIC }
-    let(:project_mirrored) { create(:project, :repository, :mirror, :import_finished, visibility: visibility) }
+    let(:project_mirrored) { create(:project, :repository, :mirror, visibility: visibility) }
 
     def do_post(user: nil, params: {}, headers: { 'X-Hub-Signature' => 'signature' })
       api_path = api("/projects/#{project_mirrored.id}/mirror/pull", user)
@@ -267,27 +267,25 @@ describe API::ProjectMirror do
               it 'triggers the pull mirroring operation' do
                 project_member(:maintainer, user)
 
-                expect(StartPullMirroringService)
-                  .to receive(:new)
-                  .with(project_mirrored, user)
-                  .and_call_original
+                Sidekiq::Testing.fake! do
+                  expect { do_post(user: user, headers: {}) }
+                    .to change { UpdateAllMirrorsWorker.jobs.size }
+                    .by(1)
 
-                do_post(user: user, headers: {})
-
-                expect(response).to have_gitlab_http_status(:ok)
+                  expect(response).to have_gitlab_http_status(:ok)
+                end
               end
             end
 
             context 'is authenticated as owner' do
               it 'triggers the pull mirroring operation' do
-                expect(StartPullMirroringService)
-                  .to receive(:new)
-                  .with(project_mirrored, project_mirrored.creator)
-                  .and_call_original
-
-                do_post(user: project_mirrored.creator, headers: {})
+                Sidekiq::Testing.fake! do
+                  expect { do_post(user: project_mirrored.creator, headers: {}) }
+                    .to change { UpdateAllMirrorsWorker.jobs.size }
+                    .by(1)
 
-                expect(response).to have_gitlab_http_status(:ok)
+                  expect(response).to have_gitlab_http_status(:ok)
+                end
               end
             end
           end

ee/spec/services/start_pull_mirroring_service_spec.rb

@@ -7,28 +7,102 @@ describe StartPullMirroringService do
   let(:import_state) { create(:import_state, project: project) }
   let(:user) { create(:user) }
 
-  subject { described_class.new(project, user) }
+  subject { described_class.new(project, user, pause_on_hard_failure: pause_on_hard_failure) }
 
-  context "when retried more than #{Gitlab::Mirror::MAX_RETRY} times" do
-    before do
-      import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
+  shared_examples_for 'retry count did not reset' do
+    it 'does not reset import state retry_count' do
+      expect { execute }.not_to change { import_state.retry_count }
     end
+  end
+
+  shared_examples_for 'pull mirroring has started' do
+    shared_examples_for 'force mirror update' do
+      it 'enqueues UpdateAllMirrorsWorker' do
+        Sidekiq::Testing.fake! do
+          expect { execute }
+            .to change { UpdateAllMirrorsWorker.jobs.size }
+            .by(1)
+          expect(execute[:status]).to eq(:success)
+        end
+      end
+    end
+
+    it_behaves_like 'force mirror update'
+
+    context 'when project mirror has been updated in the last 5 minutes' do
+      it 'schedules next execution' do
+        Timecop.freeze(Time.current) do
+          import_state.update(last_update_at: 3.minutes.ago)
+
+          expect { execute }
+            .to change { import_state.next_execution_timestamp }
+            .to(2.minutes.from_now)
+            .and not_change { UpdateAllMirrorsWorker.jobs.size }
+        end
+      end
+    end
+
+    context 'when project mirror has been updated more than 5 minutes ago' do
+      before do
+        import_state.update(last_update_at: 6.minutes.ago)
+      end
 
-    it 'does not start pull mirroring' do
-      expect(UpdateAllMirrorsWorker).not_to receive(:perform_async)
-      expect(subject.execute[:status]).to eq(:error)
+      it_behaves_like 'force mirror update'
     end
   end
 
-  context 'when does not reach the max retry limit yet' do
-    before do
-      import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY - 1)
+  context 'when pause_on_hard_failure is false' do
+    let(:pause_on_hard_failure) { false }
+
+    context "when retried more than #{Gitlab::Mirror::MAX_RETRY} times" do
+      before do
+        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
+      end
+
+      it_behaves_like 'pull mirroring has started'
+
+      it 'resets the import state retry_count' do
+        expect { execute }.to change { import_state.retry_count }.to(0)
+      end
     end
 
-    it 'starts pull mirroring' do
-      expect(UpdateAllMirrorsWorker).to receive(:perform_async).once
-      expect(import_state.reload.retry_count).not_to eq(0)
-      expect(subject.execute[:status]).to eq(:success)
+    context 'when does not reach the max retry limit yet' do
+      before do
+        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY - 1)
+      end
+
+      it_behaves_like 'pull mirroring has started'
+      it_behaves_like 'retry count did not reset'
     end
   end
+
+  context 'when pause_on_hard_failure is true' do
+    let(:pause_on_hard_failure) { true }
+
+    context "when retried more than #{Gitlab::Mirror::MAX_RETRY} times" do
+      before do
+        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
+      end
+
+      it_behaves_like 'retry count did not reset'
+
+      it 'does not start pull mirroring' do
+        expect { execute }.to not_change { UpdateAllMirrorsWorker.jobs.size }
+        expect(execute[:status]).to eq(:error)
+      end
+    end
+
+    context 'when does not reach the max retry limit yet' do
+      before do
+        import_state.update(retry_count: Gitlab::Mirror::MAX_RETRY - 1)
+      end
+
+      it_behaves_like 'pull mirroring has started'
+      it_behaves_like 'retry count did not reset'
+    end
+  end
+
+  def execute
+    subject.execute
+  end
 end