Removes hard_failed state out of project state machine. Now when the mirror...

Removes hard_failed state out of project state machine. Now when the mirror reaches the retry count limit it will stay as failed but will not get scheduled anymore, and a new message will appear

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
-      force_import_job
+      @project.force_import_job!
       flash[:notice] = "The repository is being updated..."
     end
 
@@ -71,14 +71,6 @@ 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?
+  def render_mirror_failed_message(raw_message:)
+    mirror_last_update_at = @project.mirror_last_update_at
+    message = "The repository failed to update #{time_ago_with_tooltip(mirror_last_update_at)}.".html_safe
 
-    @project.mirror_last_update_failed?
-  end
-
-  def render_mirror_failed_message(status:, icon:)
-    message = get_mirror_failed_message(status)
-
-    return message unless icon
+    return message if raw_message
 
-    message_with_icon = "#{icon('warning triangle')} #{message}"
-    return message_with_icon unless can?(current_user, :admin_project, @project)
+    message.insert(0, "#{icon('warning triangle')} ")
 
-    link_to(project_mirror_path(@project)) { "#{icon('warning triangle')} #{message}" }
+    if can?(current_user, :admin_project, @project)
+      link_to message, project_mirror_path(@project)
+    else
+      message
+    end
   end
 
   def branch_diverged_tooltip_message
@@ -30,15 +28,4 @@ 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/models/project_mirror_data.rb

@@ -7,21 +7,19 @@ class ProjectMirrorData < ActiveRecord::Base
   validates :project, presence: true
   validates :next_execution_timestamp, presence: true
 
-  before_validation on: :create do
-    self.next_execution_timestamp = Time.now
-  end
+  before_validation :set_next_execution_to_now, on: :create
 
-  def reset_retry_count!
+  def reset_retry_count
     self.retry_count = 0
   end
 
-  def increment_retry_count!
+  def increment_retry_count
     self.retry_count += 1
   end
 
   # We schedule the next sync time based on the duration of the
   # last mirroring period and add it a fixed backoff period with a random jitter
-  def set_next_execution_timestamp!
+  def set_next_execution_timestamp
     timestamp = Time.now
     retry_factor = [1, self.retry_count].max
     delay = [base_delay(timestamp), Gitlab::Mirror.min_delay].max
@@ -30,8 +28,8 @@ class ProjectMirrorData < ActiveRecord::Base
     self.next_execution_timestamp = timestamp + delay
   end
 
-  def set_next_execution_to_now!
-    self.update_attributes(next_execution_timestamp: Time.now)
+  def set_next_execution_to_now
+    self.next_execution_timestamp = Time.now
   end
 
   def retry_limit_exceeded?

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

-- if mirror_update_failed?
+- if @project.mirror_last_update_failed?
   .panel.panel-danger
     .panel-heading
-      = render partial: 'shared/projects/mirror_status', show_icon: false
+      = render partial: 'shared/mirror_status', locals: { raw_message: true }
     .panel-body
       %pre
         :preserve

app/views/shared/_mirror_status.html.haml

-= render partial: 'shared/projects/mirror_status'
+- last_successful_update_at = @project.mirror_last_successful_update_at
+- raw_message = local_assigns.fetch(:raw_message, false)
+
+- case @project.mirror_last_update_status
+- when :success
+  Updated #{time_ago_with_tooltip(last_successful_update_at)}.
+- when :failed
+  = render_mirror_failed_message(raw_message: raw_message)
+
+  - if @project.mirror_hard_failed?
+    %br
+    Repository mirroring has been paused due to too many failed attempts, and can be resumed by a project admin.
+
+  - if @project.mirror_ever_updated_successfully?
+    %br
+    Last successful update #{time_ago_with_tooltip(last_successful_update_at)}.

app/views/shared/_mirror_update_button.html.haml

@@ -17,6 +17,6 @@
       = link_to update_now_project_mirror_path(@project), method: :post, class: 'btn' do
         = icon("refresh")
         Update Now
-    - if @project.mirror_last_update_success?
+    - if @project.mirror_last_update_succeeded?
       %p.inline.prepend-left-10
         Successfully updated #{time_ago_with_tooltip(@project.mirror_last_successful_update_at)}.

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)}.

db/migrate/20171107090120_add_composite_index_to_project_mirror_data_next_execution_timestamp_and_retry_count.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddCompositeIndexToProjectMirrorDataNextExecutionTimestampAndRetryCount < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_mirror_data_on_next_execution_and_retry_count'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:project_mirror_data, [:next_execution_timestamp, :retry_count], name: INDEX_NAME)
+  end
+
+  def down
+    if index_exists?(:project_mirror_data, [:next_execution_timestamp, :retry_count], name: INDEX_NAME)
+      remove_concurrent_index(:project_mirror_data, [:next_execution_timestamp, :retry_count], name: INDEX_NAME)
+    end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20171026082505) do
+ActiveRecord::Schema.define(version: 20171107090120) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1570,6 +1570,7 @@ ActiveRecord::Schema.define(version: 20171026082505) do
     t.datetime_with_timezone "updated_at"
   end
 
+  add_index "project_mirror_data", ["next_execution_timestamp", "retry_count"], name: "index_mirror_data_on_next_execution_and_retry_count", using: :btree
   add_index "project_mirror_data", ["project_id"], name: "index_project_mirror_data_on_project_id", unique: true, using: :btree
 
   create_table "project_statistics", force: :cascade do |t|

doc/workflow/repository_mirroring.md

@@ -98,7 +98,7 @@ pull mirror settings page.
 
 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.
+or by changing the import URL in repository settings.
 
 ### SSH authentication
 

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 if project.mirror_hard_failed?
       return true unless project.mirror_last_update_at
 
       Time.now - project.mirror_last_update_at >= 5.minutes

ee/app/models/ee/project.rb

@@ -42,8 +42,9 @@ module EE
       scope :mirror, -> { where(mirror: true) }
 
       scope :mirrors_to_sync, ->(freeze_at) do
-        mirror.joins(:mirror_data).without_import_status(:hard_failed, :scheduled, :started)
+        mirror.joins(:mirror_data).without_import_status(:scheduled, :started)
           .where("next_execution_timestamp <= ?", freeze_at)
+          .where("project_mirror_data.retry_count <= ?", ::Gitlab::Mirror::MAX_RETRY)
       end
 
       scope :with_remote_mirrors, -> { joins(:remote_mirrors).where(remote_mirrors: { enabled: true }).distinct }
@@ -105,7 +106,7 @@ module EE
 
     def scheduled_mirror?
       return false unless mirror_with_content?
-      return false if hard_failed?
+      return false if mirror_hard_failed?
       return true if import_scheduled?
 
       self.mirror_data.next_execution_timestamp <= Time.now
@@ -118,13 +119,14 @@ module EE
     def mirror_last_update_status
       return unless mirror_updated?
 
-      return :success if self.last_mirror_update_succeeded?
-      return :failed unless self.mirror_data.retry_limit_exceeded?
-
-      :hard_failed
+      if self.mirror_last_update_at == self.mirror_last_successful_update_at
+        :success
+      else
+        :failed
+      end
     end
 
-    def mirror_last_update_success?
+    def mirror_last_update_succeeded?
       mirror_last_update_status == :success
     end
 
@@ -136,8 +138,8 @@ 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
+    def mirror_hard_failed?
+      self.mirror_data.retry_limit_exceeded?
     end
 
     def has_remote_mirror?
@@ -213,9 +215,12 @@ module EE
     end
 
     def force_import_job!
-      import_resume if hard_failed?
+      mirror_data = self.mirror_data
+
+      mirror_data.set_next_execution_to_now
+      mirror_data.reset_retry_count if mirror_data.retry_limit_exceeded?
 
-      self.mirror_data.set_next_execution_to_now!
+      mirror_data.save!
 
       UpdateAllMirrorsWorker.perform_async
     end

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

@@ -5,16 +5,6 @@ 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
@@ -25,9 +15,8 @@ module EE
 
           before_transition scheduled: :failed do |project, _|
             if project.mirror?
-              timestamp = Time.now
-              project.mirror_last_update_at = timestamp
-              project.mirror_data.next_execution_timestamp = timestamp
+              project.mirror_last_update_at = Time.now
+              project.mirror_data.set_next_execution_to_now
             end
           end
 
@@ -40,10 +29,8 @@ module EE
               project.mirror_last_update_at = Time.now
 
               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?
+              mirror_data.increment_retry_count
+              mirror_data.set_next_execution_timestamp
             end
           end
 
@@ -54,8 +41,8 @@ module EE
               project.mirror_last_successful_update_at = timestamp
 
               mirror_data = project.mirror_data
-              mirror_data.reset_retry_count!
-              mirror_data.set_next_execution_timestamp!
+              mirror_data.reset_retry_count
+              mirror_data.set_next_execution_timestamp
             end
 
             if ::Gitlab::CurrentSettings.current_application_settings.elasticsearch_indexing?
@@ -66,13 +53,6 @@ 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

spec/controllers/projects/mirrors_controller_spec.rb

@@ -148,26 +148,12 @@ describe Projects::MirrorsController do
 
   describe 'forcing an update on a pull mirror' do
     it 'forces update' do
-      expect_any_instance_of(EE::Project).to receive(:force_import_job!)
-
       project = create(:project, :mirror)
       sign_in(project.owner)
 
-      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(EE::Project).to receive(:force_import_job!)
 
-        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
 

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

@@ -19,30 +19,58 @@ 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)
+  describe '.mirrors_to_sync' do
+    let(:timestamp) { Time.now }
 
-        project = create(:project, :repository, :mirror, :import_hard_failed)
+    context 'when mirror is scheduled' do
+      it 'returns empty' do
+        create(:project, :mirror, :import_scheduled)
 
-        expect do
-          project.update_attributes(import_url: 'http://foo.com')
-        end.to change(project, :import_status).from('hard_failed').to('failed')
+        expect(described_class.mirrors_to_sync(timestamp)).to be_empty
       end
     end
+
+    context 'when mirror is started' do
+      it 'returns empty' do
+        create(:project, :mirror, :import_scheduled)
+
+        expect(described_class.mirrors_to_sync(timestamp)).to be_empty
+      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)
+    context 'when mirror is finished' do
+      let!(:project) { create(:project, :mirror, :import_finished) }
 
-      expect(UpdateAllMirrorsWorker).to receive(:perform_async)
+      it 'returns project if next_execution_timestamp is not in the future' do
+        expect(described_class.mirrors_to_sync(timestamp)).to match_array(project)
+      end
 
-      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)
+      it 'returns empty if next_execution_timestamp is in the future' do
+        project.mirror_data.update_attributes(next_execution_timestamp: timestamp + 2.minutes)
+
+        expect(described_class.mirrors_to_sync(timestamp)).to be_empty
+      end
+    end
+
+    context 'when project is failed' do
+      let!(:project) { create(:project, :mirror, :import_failed) }
+
+      it 'returns project if next_execution_timestamp is not in the future' do
+        expect(described_class.mirrors_to_sync(timestamp)).to match_array(project)
+      end
+
+      it 'returns empty if next_execution_timestamp is in the future' do
+        project.mirror_data.update_attributes(next_execution_timestamp: timestamp + 2.minutes)
+
+        expect(described_class.mirrors_to_sync(timestamp)).to be_empty
+      end
+
+      context 'with retry limit exceeded' do
+        let!(:project) { create(:project, :mirror, :import_hard_failed) }
+
+        it 'returns empty' do
+          expect(described_class.mirrors_to_sync(timestamp)).to be_empty
+        end
       end
     end
   end
@@ -232,15 +260,26 @@ describe Project 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, :next_execution_timestamp).to(timestamp)
       end
+    end
+
+    context 'when mirror is hard failed' 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)
 
-      project.run_callbacks(:commit)
+        Timecop.freeze(timestamp) do
+          expect { project.force_import_job! }.to change(project.mirror_data, :retry_count).to(0)
+          expect(project.mirror_data.next_execution_timestamp).to eq(timestamp)
+        end
+      end
     end
   end
 
@@ -348,35 +387,27 @@ describe Project do
     end
 
     context 'when mirror has updated' do
+      let(:timestamp) { Time.now }
+
       before do
-        project.mirror_last_update_at = Time.now
+        project.mirror_last_update_at = timestamp
       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
+      context 'when last update time does not equal the time of the last successful update' do
         it 'returns failed' do
-          project.mirror_data.retry_count = Gitlab::Mirror::MAX_RETRY
+          project.mirror_last_successful_update_at = Time.now - 1.minute
 
           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
 

spec/factories/projects.rb

@@ -78,15 +78,21 @@ FactoryGirl.define do
     end
 
     trait :import_finished do
+      timestamp = Time.now
+
       import_status :finished
+      mirror_last_update_at timestamp
+      mirror_last_successful_update_at timestamp
     end
 
     trait :import_failed do
       import_status :failed
+      mirror_last_update_at Time.now
     end
 
     trait :import_hard_failed do
-      import_status :hard_failed
+      import_status :failed
+      mirror_last_update_at Time.now - 1.minute
 
       after(:create) do |project|
         project.mirror_data&.update_attributes(retry_count: Gitlab::Mirror::MAX_RETRY + 1)

spec/models/project_mirror_data_spec.rb

@@ -23,36 +23,25 @@ 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
+  describe '#reset_retry_count' do
     let(:mirror_data) { create(:project, :mirror, :import_finished).mirror_data }
 
     it 'resets retry_count to 0' do
       mirror_data.retry_count = 3
 
-      expect { mirror_data.reset_retry_count! }.to change { mirror_data.retry_count }.from(3).to(0)
+      expect { mirror_data.reset_retry_count }.to change { mirror_data.retry_count }.from(3).to(0)
     end
   end
 
-  describe '#increment_retry_count!' do
+  describe '#increment_retry_count' do
     let(:mirror_data) { create(:project, :mirror, :import_finished).mirror_data }
 
     it 'increments retry_count' do
-      expect { mirror_data.increment_retry_count! }.to change { mirror_data.retry_count }.from(0).to(1)
+      expect { mirror_data.increment_retry_count }.to change { mirror_data.retry_count }.from(0).to(1)
     end
   end
 
-  describe '#set_next_execution_timestamp!' do
+  describe '#set_next_execution_timestamp' do
     let(:mirror_data) { create(:project, :mirror, :import_finished).mirror_data }
     let!(:timestamp) { Time.now }
     let!(:jitter) { 2.seconds }
@@ -75,7 +64,7 @@ describe ProjectMirrorData, type: :model do
       context 'when incrementing retry count' do
         it 'applies transition successfully' do
           mirror_data.retry_count = 2
-          mirror_data.increment_retry_count!
+          mirror_data.increment_retry_count
 
           expect_next_execution_timestamp(mirror_data, timestamp + 78.minutes)
         end
@@ -103,7 +92,7 @@ describe ProjectMirrorData, type: :model do
         context 'when incrementing retry count' do
           it 'applies transition successfully' do
             mirror_data.retry_count = 3
-            mirror_data.increment_retry_count!
+            mirror_data.increment_retry_count
 
             expect_next_execution_timestamp(mirror_data, timestamp + 62.minutes)
           end
@@ -126,7 +115,7 @@ describe ProjectMirrorData, type: :model do
         context 'when incrementing retry count' do
           it 'applies transition successfully' do
             mirror_data.retry_count = 2
-            mirror_data.increment_retry_count!
+            mirror_data.increment_retry_count
 
             expect_next_execution_timestamp(mirror_data, max_timestamp + mirror_jitter)
           end
@@ -137,7 +126,7 @@ describe ProjectMirrorData, type: :model do
     def expect_next_execution_timestamp(mirror_data, new_timestamp)
       Timecop.freeze(timestamp) do
         expect do
-          mirror_data.set_next_execution_timestamp!
+          mirror_data.set_next_execution_timestamp
         end.to change { mirror_data.next_execution_timestamp }.to eq(new_timestamp)
       end
     end

spec/views/shared/_mirror_status.html.haml_spec.rb

+require 'spec_helper'
+
+describe 'shared/_mirror_status.html.haml' do
+  include ApplicationHelper
+
+  context 'when mirror has not updated yet' do
+    it 'does not render anything' do
+      @project = create(:project, :mirror)
+
+      sign_in(@project.owner)
+
+      render 'shared/mirror_status'
+
+      expect(rendered).to be_empty
+    end
+  end
+
+  context 'when mirror successful' do
+    it 'renders success message' do
+      @project = create(:project, :mirror, :import_finished)
+
+      sign_in(@project.owner)
+
+      render 'shared/mirror_status'
+
+      expect(rendered).to have_content("Updated")
+    end
+  end
+
+  context 'when mirror failed' do
+    before do
+      @project = create(:project, :mirror, :import_failed)
+
+      sign_in(@project.owner)
+    end
+
+    it 'renders failure message' do
+      render 'shared/mirror_status', raw_message: true
+
+      expect(rendered).to have_content("The repository failed to update")
+    end
+
+    context 'with a previous successful update' do
+      it 'renders failure message' do
+        @project.mirror_last_successful_update_at = Time.now - 1.minute
+
+        render 'shared/mirror_status', raw_message: true
+
+        expect(rendered).to have_content("Last successful update")
+      end
+    end
+
+    context 'with a hard failed mirror' do
+      it 'renders hard failed message' do
+        @project.mirror_data.retry_count = Gitlab::Mirror::MAX_RETRY + 1
+
+        render 'shared/mirror_status', raw_message: true
+
+        expect(rendered).to have_content("Repository mirroring has been paused due to too many failed attempts, and can be resumed by a project admin.")
+      end
+    end
+  end
+end