Add maximum retry count migration and related logic

app/controllers/projects/mirrors_controller.rb

@@ -62,7 +62,7 @@ class Projects::MirrorsController < Projects::ApplicationController
       @project.update_remote_mirrors
       flash[:notice] = "The remote repository is being updated..."
     else
-      @project.force_import_job!
+      force_import_job
       flash[:notice] = "The repository is being updated..."
     end
 
@@ -71,6 +71,14 @@ class Projects::MirrorsController < Projects::ApplicationController
 
   private
 
+  def force_import_job
+    if @project.hard_failed?
+      @project.resume
+    else
+      @project.force_import_job!
+    end
+  end
+
   def remote_mirror
     @remote_mirror = @project.remote_mirrors.first_or_initialize
   end

app/helpers/mirror_helper.rb

 module MirrorHelper
+  def mirror_update_failed?
+    return true if @project.hard_failed?
+
+    @project.mirror_last_update_failed?
+  end
+
+  def render_mirror_failed_message(status:, icon:)
+    message = get_mirror_failed_message(status)
+
+    return message unless icon
+
+    message_with_icon = "#{icon('warning triangle')} #{message}"
+    return message_with_icon unless can?(current_user, :admin_project, @project)
+
+    link_to(project_mirror_path(@project)) { "#{icon('warning triangle')} #{message}" }
+  end
+
   def branch_diverged_tooltip_message
     message = s_('Branches|The branch could not be updated automatically because it has diverged from its upstream counterpart.')
 
@@ -13,4 +30,15 @@ module MirrorHelper
   def options_for_mirror_user
     options_from_collection_for_select(default_mirror_users, :id, :name, @project.mirror_user_id || current_user.id)
   end
+
+  private
+
+  def get_mirror_failed_message(status)
+    if status == :failed
+      "The repository failed to update #{time_ago_with_tooltip(@project.last_update_at)}."
+    else
+      "The repository failed to update #{time_ago_with_tooltip(last_update_at)}.<br>"\
+        "Repository mirroring has been paused due to too many failed attempts, and can be resumed by a project admin."
+    end
+  end
 end

app/helpers/projects_helper.rb

@@ -512,12 +512,6 @@ module ProjectsHelper
     end
   end
 
-  def can_force_update_mirror?(project)
-    return true unless project.mirror_last_update_at
-
-    Time.now - project.mirror_last_update_at >= 5.minutes
-  end
-
   def membership_locked?
     if @project.group && @project.group.membership_lock
       true

app/models/project_mirror_data.rb

 class ProjectMirrorData < ActiveRecord::Base
-  include Gitlab::CurrentSettings
-
   BACKOFF_PERIOD = 24.seconds
   JITTER = 6.seconds
 
@@ -36,6 +34,10 @@ class ProjectMirrorData < ActiveRecord::Base
     self.update_attributes(next_execution_timestamp: Time.now)
   end
 
+  def retry_limit_exceeded?
+    self.retry_count > Gitlab::Mirror::MAX_RETRY
+  end
+
   private
 
   def base_delay(timestamp)

app/views/projects/mirrors/pull/_mirror_update_fail.html.haml

-- if @project.mirror_last_update_failed?
+- if mirror_update_failed?
   .panel.panel-danger
     .panel-heading
-      The repository failed to update #{time_ago_with_tooltip(@project.mirror_last_update_at)}.
-      - if @project.mirror_ever_updated_successfully?
-        Last successful update #{time_ago_with_tooltip(@project.mirror_last_successful_update_at)}.
+      = render partial: 'shared/projects/mirror_status', show_icon: false
     .panel-body
       %pre
         :preserve

app/views/shared/_mirror_status.html.haml

-- case @project.mirror_last_update_status
-- when :success
-  Updated #{time_ago_with_tooltip(@project.mirror_last_successful_update_at)}.
-- when :failed
-  - if can?(current_user, :admin_project, @project)
-    = link_to project_mirror_path(@project) do
-      = icon("exclamation-triangle")
-      Failed to update #{time_ago_with_tooltip(@project.mirror_last_update_at)}.
-  - else
-    = icon("exclamation-triangle")
-    Failed to update #{time_ago_with_tooltip(@project.mirror_last_update_at)}.
-
-  - if @project.mirror_ever_updated_successfully?
-    Last successful update #{time_ago_with_tooltip(@project.mirror_last_successful_update_at)}.
+= render partial: 'shared/projects/mirror_status'

app/views/shared/projects/_mirror_status.html.haml

+- last_successful_update_at = @project.mirror_last_successful_update_at
+- mirror_last_update_status = @project.mirror_last_update_status
+- show_icon = local_assigns.fetch(:show_icon, true)
+
+- if mirror_last_update_status == :success
+  Updated #{time_ago_with_tooltip(last_successful_update_at)}.
+- else
+  = render_mirror_failed_message(status: mirror_last_update_status, icon: show_icon)
+
+  - if @project.mirror_ever_updated_successfully?
+    %br
+    Last successful update #{time_ago_with_tooltip(last_successful_update_at)}.

changelogs/unreleased-ee/2831-hard-fail-mirror-when-retry-count-number-exceeds-the-limit.yml

+---
+title: Mirrors can now hard fail, keeping them from being retried until a project
+  admin takes action.
+merge_request: 3117
+author:
+type: added

doc/workflow/repository_mirroring.md

@@ -84,6 +84,22 @@ this branch to prevent any changes from being lost.
 
 ![Diverged branch](repository_mirroring/repository_mirroring_diverged_branch.png)
 
+### Hard failure
+
+>[Introduced][ee-3117] in GitLab Enterprise Edition 10.2.
+
+Once a mirror gets retried 14 times in a row, it will get marked as hard failed,
+this will become visible in either the project main dashboard or in the 
+pull mirror settings page.
+
+![Hard failed mirror main notice](repository_mirroring/repository_mirroring_hard_failed_main.png)
+
+![Hard failed mirror settings notice](repository_mirroring/repository_mirroring_hard_failed_settings.png)
+
+When a project is hard failed, it will no longer get picked up for mirroring.
+A user can resume the project mirroring again by either [forcing an update](#forcing-an-update)
+or by changing the import URL field in the repository settings.
+
 ### SSH authentication
 
 > [Introduced][ee-2551] in GitLab Enterprise Edition Starter 9.5
@@ -248,6 +264,7 @@ to resolve this issue.
 
 [ee-51]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/51
 [ee-2551]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2551
+[ee-3117]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/3117
 [perms]: ../user/permissions.md
 [hooks]: https://docs.gitlab.com/ee/administration/custom_hooks.html
 [deploy-key]: ../ssh/README.md#deploy-keys

ee/app/helpers/ee/projects_helper.rb

 module EE
   module ProjectsHelper
+    def can_force_update_mirror?(project)
+      return true if project.hard_failed?
+      return true unless project.mirror_last_update_at
+
+      Time.now - project.mirror_last_update_at >= 5.minutes
+    end
+
     def can_change_push_rule?(push_rule, rule)
       return true if push_rule.global?
 

ee/app/models/ee/project.rb

@@ -40,7 +40,12 @@ module EE
       scope :with_shared_runners_limit_enabled, -> { with_shared_runners.non_public_only }
 
       scope :mirror, -> { where(mirror: true) }
-      scope :mirrors_to_sync, ->(freeze_at) { mirror.joins(:mirror_data).without_import_status(:scheduled, :started).where("next_execution_timestamp <= ?", freeze_at) }
+
+      scope :mirrors_to_sync, ->(freeze_at) do
+        mirror.joins(:mirror_data).without_import_status(:hard_failed, :scheduled, :started)
+          .where("next_execution_timestamp <= ?", freeze_at)
+      end
+
       scope :with_remote_mirrors, -> { joins(:remote_mirrors).where(remote_mirrors: { enabled: true }).distinct }
       scope :with_wiki_enabled, -> { with_feature_enabled(:wiki) }
 
@@ -100,6 +105,7 @@ module EE
 
     def scheduled_mirror?
       return false unless mirror_with_content?
+      return false if hard_failed?
       return true if import_scheduled?
 
       self.mirror_data.next_execution_timestamp <= Time.now
@@ -112,11 +118,10 @@ module EE
     def mirror_last_update_status
       return unless mirror_updated?
 
-      if self.mirror_last_update_at == self.mirror_last_successful_update_at
-        :success
-      else
-        :failed
-      end
+      return :success if self.last_mirror_update_succeeded?
+      return :failed unless self.mirror_data.retry_limit_exceeded?
+
+      :hard_failed
     end
 
     def mirror_last_update_success?
@@ -131,6 +136,10 @@ module EE
       mirror_updated? && self.mirror_last_successful_update_at
     end
 
+    def last_mirror_update_succeeded?
+      self.mirror_last_update_at == self.mirror_last_successful_update_at
+    end
+
     def has_remote_mirror?
       feature_available?(:repository_mirrors) &&
         remote_mirror_available? &&
@@ -204,7 +213,10 @@ module EE
     end
 
     def force_import_job!
+      import_resume if hard_failed?
+
       self.mirror_data.set_next_execution_to_now!
+
       UpdateAllMirrorsWorker.perform_async
     end
 

ee/app/models/ee/project/import_status_state_machine.rb

@@ -5,6 +5,16 @@ module EE
 
       included do
         state_machine :import_status, initial: :none do
+          event :import_hard_fail do
+            transition failed: :hard_failed
+          end
+
+          event :import_resume do
+            transition hard_failed: :failed
+          end
+
+          state :hard_failed
+
           before_transition [:none, :finished, :failed] => :scheduled do |project, _|
             project.mirror_data&.last_update_scheduled_at = Time.now
           end
@@ -32,6 +42,8 @@ module EE
               mirror_data = project.mirror_data
               mirror_data.increment_retry_count!
               mirror_data.set_next_execution_timestamp!
+
+              project.run_after_commit { import_hard_fail } if mirror_data.retry_limit_exceeded?
             end
           end
 
@@ -54,6 +66,13 @@ module EE
             end
           end
 
+          before_transition hard_failed: :failed do |project, _|
+            if project.mirror?
+              project.mirror_data.reset_retry_count!
+              project.force_import_job!
+            end
+          end
+
           after_transition [:finished, :failed] => [:scheduled, :started] do |project, _|
             ::Gitlab::Mirror.increment_capacity(project.id) if project.mirror?
           end

lib/gitlab/mirror.rb

@@ -6,7 +6,14 @@ module Gitlab
     SCHEDULER_CRON = '* * * * *'.freeze
     PULL_CAPACITY_KEY = 'MIRROR_PULL_CAPACITY'.freeze
     JITTER = 1.minute
+
+    # TODO: Make MIN_DELAY configurable and MAX_RETRY adjust according to it
+    # ISSUE: https://gitlab.com/gitlab-org/gitlab-ee/issues/3885
+    # MAX RETRY value was calculated based on the triangular number with a 15 minutes factor
+    # https://en.wikipedia.org/wiki/Triangular_number in order to make sure the mirror
+    # gets retried for a full day before it becomes hard failed
     MIN_DELAY = 15.minutes
+    MAX_RETRY = 14
 
     class << self
       def configure_cron_job!

spec/controllers/projects/mirrors_controller_spec.rb

@@ -155,6 +155,20 @@ describe Projects::MirrorsController do
 
       put :update_now, { namespace_id: project.namespace.to_param, project_id: project.to_param }
     end
+
+    context 'when mirror is hard_failed' do
+      it 'forces update and changes to failed' do
+        allow(UpdateAllMirrorsWorker).to receive(:perform_async).and_return(nil)
+
+        project = create(:project, :mirror, :import_hard_failed)
+        sign_in(project.owner)
+
+        expect_any_instance_of(Project).to receive(:import_resume).and_call_original
+        expect do
+          put :update_now, { namespace_id: project.namespace.to_param, project_id: project.to_param }
+        end.to change { project.reload.import_status }.from('hard_failed').to('failed')
+      end
+    end
   end
 
   describe 'forcing an update on a push mirror' do

spec/ee/spec/models/ee/project_spec.rb

@@ -19,6 +19,34 @@ describe Project do
     it { is_expected.to have_many(:audit_events) }
   end
 
+  describe 'after update' do
+    context 'when mirror is hard failed and import url changed' do
+      it 'resumes the mirroring' do
+        allow(UpdateAllMirrorsWorker).to receive(:perform_async).and_return(nil)
+
+        project = create(:project, :repository, :mirror, :import_hard_failed)
+
+        expect do
+          project.update_attributes(import_url: 'http://foo.com')
+        end.to change(project, :import_status).from('hard_failed').to('failed')
+      end
+    end
+  end
+
+  describe 'resuming from a hard failed mirror' do
+    it 'resets retry count and schedules a mirroring worker' do
+      timestamp = Time.now
+      project = create(:project, :mirror, :import_hard_failed)
+
+      expect(UpdateAllMirrorsWorker).to receive(:perform_async)
+
+      Timecop.freeze(timestamp) do
+        expect { project.import_resume }.to change(project.mirror_data, :retry_count).to(0)
+        expect(project.mirror_data.next_execution_timestamp).to eq(timestamp)
+      end
+    end
+  end
+
   describe '#push_rule' do
     let(:project) { create(:project, push_rule: create(:push_rule)) }
 
@@ -200,6 +228,22 @@ describe Project do
     end
   end
 
+  describe '#force_import_job!' do
+    it 'sets next execution timestamp to now and schedules UpdateAllMirrorsWorker' do
+      timestamp = Time.now
+      project = create(:project, :mirror)
+      project.mirror_data.update_attributes(next_execution_timestamp: timestamp - 3.minutes)
+
+      Timecop.freeze(timestamp) do
+        expect { project.force_import_job! }.to change(project.mirror_data, :next_execution_timestamp).to(timestamp)
+      end
+
+      expect(UpdateAllMirrorsWorker).to receive(:perform_async)
+
+      project.run_callbacks(:commit)
+    end
+  end
+
   describe '#fetch_mirror' do
     where(:import_url, :auth_method, :expected) do
       'http://foo:bar@example.com' | 'password'       | 'http://foo:bar@example.com'
@@ -239,6 +283,103 @@ describe Project do
     end
   end
 
+  describe '#scheduled_mirror?' do
+    context 'when mirror is expected to run soon' do
+      it 'returns true' do
+        timestamp = Time.now
+        project = create(:project, :mirror, :import_finished, :repository)
+        project.mirror_last_update_at = timestamp - 3.minutes
+        project.mirror_data.next_execution_timestamp = timestamp - 2.minutes
+
+        expect(project.scheduled_mirror?).to be true
+      end
+    end
+
+    context 'when mirror was scheduled' do
+      it 'returns true' do
+        project = create(:project, :mirror, :import_scheduled, :repository)
+
+        expect(project.scheduled_mirror?).to be true
+      end
+    end
+
+    context 'when mirror is hard_failed' do
+      it 'returns false' do
+        project = create(:project, :mirror, :import_hard_failed)
+
+        expect(project.scheduled_mirror?).to be false
+      end
+    end
+  end
+
+  describe  '#updating_mirror?' do
+    context 'when repository is empty' do
+      it 'returns false' do
+        project = create(:project, :mirror, :import_started)
+
+        expect(project.updating_mirror?).to be false
+      end
+    end
+
+    context 'when project is not a mirror' do
+      it 'returns false' do
+        project = create(:project, :import_started)
+
+        expect(project.updating_mirror?).to be false
+      end
+    end
+
+    context 'when mirror is in progress' do
+      it 'returns true' do
+        project = create(:project, :mirror, :import_started, :repository)
+
+        expect(project.updating_mirror?).to be true
+      end
+    end
+  end
+
+  describe '#mirror_last_update_status' do
+    let(:project) { create(:project, :mirror) }
+
+    context 'when mirror has not updated' do
+      it 'returns nil' do
+        expect(project.mirror_last_update_status).to be_nil
+      end
+    end
+
+    context 'when mirror has updated' do
+      before do
+        project.mirror_last_update_at = Time.now
+      end
+
+      context 'when last update time equals the time of the last successful update' do
+        it 'returns success' do
+          timestamp = Time.now
+          project.mirror_last_update_at = timestamp
+          project.mirror_last_successful_update_at = timestamp
+
+          expect(project.mirror_last_update_status).to eq(:success)
+        end
+      end
+
+      context 'when retry count has not reached the limit' do
+        it 'returns failed' do
+          project.mirror_data.retry_count = Gitlab::Mirror::MAX_RETRY
+
+          expect(project.mirror_last_update_status).to eq(:failed)
+        end
+      end
+
+      context 'when retry count has reached the limit' do
+        it 'returns hard_failed' do
+          project.mirror_data.retry_count = Gitlab::Mirror::MAX_RETRY + 1
+
+          expect(project.mirror_last_update_status).to eq(:hard_failed)
+        end
+      end
+    end
+  end
+
   describe '#has_remote_mirror?' do
     let(:project) { create(:project, :remote_mirror, :import_started) }
     subject { project.has_remote_mirror? }

spec/factories/projects.rb

@@ -85,6 +85,14 @@ FactoryGirl.define do
       import_status :failed
     end
 
+    trait :import_hard_failed do
+      import_status :hard_failed
+
+      after(:create) do |project|
+        project.mirror_data&.update_attributes(retry_count: Gitlab::Mirror::MAX_RETRY + 1)
+      end
+    end
+
     trait :mirror do
       mirror true
       import_url { generate(:url) }

spec/models/project_mirror_data_spec.rb

@@ -7,10 +7,6 @@ describe ProjectMirrorData, type: :model do
     it { is_expected.to belong_to(:project) }
   end
 
-  describe 'modules' do
-    it { is_expected.to include_module(Gitlab::CurrentSettings) }
-  end
-
   describe 'validations' do
     it { is_expected.to validate_presence_of(:project) }
   end
@@ -27,6 +23,17 @@ describe ProjectMirrorData, type: :model do
     end
   end
 
+  describe 'when update' do
+    context 'when retry limit reached' do
+      it 'marks mirror as hard failed' do
+        project = create(:project, :mirror, :import_started)
+        project.mirror_data.update_attributes(retry_count: Gitlab::Mirror::MAX_RETRY)
+
+        expect { project.import_fail }.to change(project, :import_status).from('started').to('hard_failed')
+      end
+    end
+  end
+
   describe '#reset_retry_count!' do
     let(:mirror_data) { create(:project, :mirror, :import_finished).mirror_data }
 

spec/models/project_spec.rb

@@ -1938,68 +1938,6 @@ describe Project do
     end
   end
 
-  describe '#scheduled_mirror?' do
-    context 'when mirror is expected to run soon' do
-      it 'returns true' do
-        timestamp = Time.now
-        project = create(:project, :mirror, :import_finished, :repository)
-
-        project.mirror_last_update_at = timestamp - 3.minutes
-        project.mirror_data.next_execution_timestamp = timestamp - 2.minutes
-
-        expect(project.scheduled_mirror?).to be true
-      end
-    end
-
-    context 'when mirror was scheduled' do
-      it 'returns true' do
-        project = create(:project, :mirror, :import_scheduled, :repository)
-
-        expect(project.scheduled_mirror?).to be true
-      end
-    end
-  end
-
-  describe  '#updating_mirror?' do
-    context 'when repository is empty' do
-      it 'returns false' do
-        project = create(:project, :mirror, :import_started)
-
-        expect(project.updating_mirror?).to be false
-      end
-    end
-
-    context 'when project is not a mirror' do
-      it 'returns false' do
-        project = create(:project, :import_started)
-
-        expect(project.updating_mirror?).to be false
-      end
-    end
-
-    context 'when mirror is in progress' do
-      it 'returns true' do
-        project = create(:project, :mirror, :import_started, :repository)
-
-        expect(project.updating_mirror?).to be true
-      end
-    end
-  end
-
-  describe '#force_import_job!' do
-    it 'sets next execution timestamp to now and schedules UpdateAllMirrorsWorker' do
-      timestamp = Time.now
-      project = create(:project, :mirror)
-
-      project.mirror_data.update_attributes(next_execution_timestamp: timestamp - 3.minutes)
-
-      expect(UpdateAllMirrorsWorker).to receive(:perform_async)
-      Timecop.freeze(timestamp) do
-        expect { project.force_import_job! }.to change { project.mirror_data.reload.next_execution_timestamp }.to be_within(1.second).of(timestamp)
-      end
-    end
-  end
-
   describe '#add_import_job' do
     let(:import_jid) { '123' }