Merge branch 'stop-using-sidekiq-cron-for-push-mirrors' into 'master'

implements new logic for push mirrors to update on push

See merge request !1616

app/controllers/projects/mirrors_controller.rb

@@ -49,6 +49,6 @@ class Projects::MirrorsController < Projects::ApplicationController
   def mirror_params
     params.require(:project).permit(:mirror, :import_url, :mirror_user_id,
                                     :mirror_trigger_builds, :sync_time,
-                                    remote_mirrors_attributes: [:url, :id, :enabled, :sync_time])
+                                    remote_mirrors_attributes: [:url, :id, :enabled])
   end
 end

app/models/application_setting.rb

@@ -182,7 +182,7 @@ class ApplicationSetting < ActiveRecord::Base
   before_save :ensure_runners_registration_token
   before_save :ensure_health_check_access_token
 
-  after_update :update_mirror_cron_jobs, if: :minimum_mirror_sync_time_changed?
+  after_update :update_mirror_cron_job, if: :minimum_mirror_sync_time_changed?
 
   after_commit do
     Rails.cache.write(CACHE_KEY, self)
@@ -291,13 +291,11 @@ class ApplicationSetting < ActiveRecord::Base
     end
   end
 
-  def update_mirror_cron_jobs
+  def update_mirror_cron_job
     Project.mirror.where('sync_time < ?', minimum_mirror_sync_time)
       .update_all(sync_time: minimum_mirror_sync_time)
-    RemoteMirror.where('sync_time < ?', minimum_mirror_sync_time)
-      .update_all(sync_time: minimum_mirror_sync_time)
 
-    Gitlab::Mirror.configure_cron_jobs!
+    Gitlab::Mirror.configure_cron_job!
   end
 
   def elasticsearch_url

app/models/project.rb

@@ -639,6 +639,14 @@ class Project < ActiveRecord::Base
     remote_mirrors.each(&:sync)
   end
 
+  def mark_stuck_remote_mirrors_as_failed!
+    remote_mirrors.stuck.update_all(
+      update_status: :failed,
+      last_error: 'The remote mirror took to long to complete.',
+      last_update_at: Time.now
+    )
+  end
+
   def fetch_mirror
     return unless mirror?
 

app/models/remote_mirror.rb

 class RemoteMirror < ActiveRecord::Base
   include AfterCommitQueue
+  include IgnorableColumn
+
+  ignore_column :sync_time
+
+  BACKOFF_DELAY = 5.minutes
 
   attr_encrypted :credentials,
                  key: Gitlab::Application.secrets.db_key_base,
@@ -12,9 +17,6 @@ class RemoteMirror < ActiveRecord::Base
   belongs_to :project, inverse_of: :remote_mirrors
 
   validates :url, presence: true, url: { protocols: %w(ssh git http https), allow_blank: true }
-  validates :sync_time,
-    presence: true,
-    inclusion: { in: Gitlab::Mirror::SYNC_TIME_OPTIONS.values }
 
   validate  :url_availability, if: -> (mirror) { mirror.url_changed? || mirror.enabled? }
 
@@ -28,7 +30,7 @@ class RemoteMirror < ActiveRecord::Base
 
   state_machine :update_status, initial: :none do
     event :update_start do
-      transition [:none, :finished] => :started
+      transition [:none, :finished, :failed] => :started
     end
 
     event :update_finish do
@@ -39,24 +41,22 @@ class RemoteMirror < ActiveRecord::Base
       transition started: :failed
     end
 
-    event :update_retry do
-      transition failed: :started
-    end
-
     state :started
     state :finished
     state :failed
 
-    after_transition any => :started, do: :schedule_update_job
+    after_transition any => :started do |remote_mirror, _|
+      remote_mirror.update(last_update_started_at: Time.now)
+    end
 
-    after_transition started: :finished do |remote_mirror, transaction|
+    after_transition started: :finished do |remote_mirror, _|
       timestamp = Time.now
       remote_mirror.update_attributes!(
         last_update_at: timestamp, last_successful_update_at: timestamp, last_error: nil
       )
     end
 
-    after_transition started: :failed do |remote_mirror, transaction|
+    after_transition started: :failed do |remote_mirror, _|
       remote_mirror.update(last_update_at: Time.now)
     end
   end
@@ -74,10 +74,13 @@ class RemoteMirror < ActiveRecord::Base
   end
 
   def sync
-    return unless project
-    return if !enabled || update_in_progress?
+    return unless project && enabled
 
-    update_failed? ? update_retry : update_start
+    RepositoryUpdateRemoteMirrorWorker.perform_in(BACKOFF_DELAY, self.id, Time.now) if project&.repository_exists?
+  end
+
+  def updated_since?(timestamp)
+    last_update_started_at && last_update_started_at > timestamp && !update_failed?
   end
 
   def mark_for_delete_if_blank_url
@@ -130,16 +133,6 @@ class RemoteMirror < ActiveRecord::Base
     )
   end
 
-  def schedule_update_job
-    run_after_commit(:add_update_job)
-  end
-
-  def add_update_job
-    if project && project.repository_exists?
-      RepositoryUpdateRemoteMirrorWorker.perform_async(self.id)
-    end
-  end
-
   def refresh_remote
     return unless project
 

app/services/git_push_service.rb

@@ -59,6 +59,7 @@ class GitPushService < BaseService
     execute_related_hooks
     perform_housekeeping
 
+    update_remote_mirrors
     update_caches
   end
 
@@ -96,6 +97,13 @@ class GitPushService < BaseService
 
   protected
 
+  def update_remote_mirrors
+    return if @project.remote_mirrors.empty?
+
+    @project.mark_stuck_remote_mirrors_as_failed!
+    @project.update_remote_mirrors
+  end
+
   def execute_related_hooks
     # Update merge requests that may be affected by this push. A new branch
     # could cause the last commit of a merge request to change.

app/views/projects/mirrors/_show.html.haml

@@ -52,8 +52,8 @@
       %h4.prepend-top-0
         Push to a remote repository
       %p.light
-        Set up the remote repository that you want to update with the content
-        of the current repository every hour.
+        Set up the remote repository that you want to update with the content of the current repository
+        every time someone pushes to it.
         = link_to 'Read more', help_page_path('workflow/repository_mirroring', anchor: 'pushing-to-a-remote-repository'), target: '_blank'
     .col-lg-9
       = render "shared/remote_mirror_update_button", remote_mirror: @remote_mirror
@@ -73,13 +73,10 @@
           .prepend-left-20
             = rm_form.label :enabled, "Remote mirror repository", class: "label-light append-bottom-0"
             %p.light.append-bottom-0
-              Automatically update the remote mirror's branches, tags, and commits from this repository every hour.
+              Automatically update the remote mirror's branches, tags, and commits from this repository every time someone pushes to it.
         .form-group.has-feedback
           = rm_form.label :url, "Git repository URL", class: "label-light"
           = rm_form.text_field :url, class: "form-control", placeholder: 'https://username:password@gitlab.company.com/group/project.git'
           = render "projects/mirrors/instructions"
-        .form-group
-          = rm_form.label :sync_time, "Synchronization time", class: "label-light append-bottom-0"
-          = rm_form.select :sync_time, options_for_select(mirror_sync_time_options, @remote_mirror.sync_time), {}, class: 'form-control remote-mirror-sync-time'
         = f.submit 'Save changes', class: 'btn btn-create', name: 'update_remote_mirror'
 %hr

app/workers/repository_update_remote_mirror_worker.rb

 class RepositoryUpdateRemoteMirrorWorker
-  UpdateRemoteMirrorError = Class.new(StandardError)
+  UpdateAlreadyInProgressError = Class.new(StandardError)
+  UpdateError = Class.new(StandardError)
 
   include Sidekiq::Worker
   include Gitlab::ShellAdapter
 
-  sidekiq_options queue: :project_mirror, retry: false
-
-  def perform(remote_mirror_id)
-    begin
-      remote_mirror = RemoteMirror.find(remote_mirror_id)
-      project       = remote_mirror.project
-      current_user  = project.creator
-      result        = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(remote_mirror)
-
-      if result[:status] == :error
-        remote_mirror.mark_as_failed(result[:message])
-      else
-        remote_mirror.update_finish
-      end
-    rescue => ex
-      remote_mirror.mark_as_failed("We're sorry, a temporary error occurred, please try again.")
-
-      raise UpdateRemoteMirrorError, "#{ex.class}: #{Gitlab::UrlSanitizer.sanitize(ex.message)}"
-    end
+  sidekiq_options queue: :project_mirror, retry: 3, dead: false
+
+  sidekiq_retry_in { |count| 30 * count }
+
+  sidekiq_retries_exhausted do |msg, _|
+    Sidekiq.logger.warn "Failed #{msg['class']} with #{msg['args']}: #{msg['error_message']}"
+  end
+
+  def perform(remote_mirror_id, scheduled_time)
+    remote_mirror = RemoteMirror.find(remote_mirror_id)
+    return if remote_mirror.updated_since?(scheduled_time)
+
+    raise UpdateAlreadyInProgressError if remote_mirror.update_in_progress?
+    remote_mirror.update_start
+
+    project = remote_mirror.project
+    current_user = project.creator
+    result = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(remote_mirror)
+
+    raise UpdateError, result[:message] if result[:status] == :error
+    remote_mirror.update_finish
+  rescue UpdateAlreadyInProgressError
+    raise
+  rescue UpdateError => ex
+    remote_mirror.mark_as_failed(Gitlab::UrlSanitizer.sanitize(ex.message))
+    raise
+  rescue => ex
+    raise UpdateError, "#{ex.class}: #{ex.message}"
   end
 end

app/workers/update_all_remote_mirrors_worker.rb

-class UpdateAllRemoteMirrorsWorker
-  include Sidekiq::Worker
-  include CronjobQueue
-
-  def perform
-    fail_stuck_mirrors!
-
-    remote_mirrors_to_sync.find_each(batch_size: 50).each(&:sync)
-  end
-
-  def fail_stuck_mirrors!
-    RemoteMirror.stuck.find_each(batch_size: 50) do |remote_mirror|
-      remote_mirror.mark_as_failed('The mirror update took too long to complete.')
-    end
-  end
-
-  private
-
-  def remote_mirrors_to_sync
-    RemoteMirror.where("last_successful_update_at + #{Gitlab::Database.minute_interval('sync_time')} <= ? OR sync_time IN (?)", DateTime.now, Gitlab::Mirror.sync_times)
-  end
-end

changelogs/unreleased-ee/stop-using-sidekiq-cron-for-push-mirrors.yml

+---
+title: Stop using sidekiq cron for push mirrors
+merge_request: 1616
+author:

config/initializers/sidekiq.rb

@@ -40,7 +40,7 @@ Sidekiq.configure_server do |config|
   end
   Sidekiq::Cron::Job.load_from_hash! cron_jobs
 
-  Gitlab::Mirror.configure_cron_jobs!
+  Gitlab::Mirror.configure_cron_job!
 
   Gitlab::Geo.configure_cron_jobs!
 

db/migrate/20170427180205_add_last_update_started_at_column_to_remote_mirrors.rb

+class AddLastUpdateStartedAtColumnToRemoteMirrors < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :remote_mirrors, :last_update_started_at, :datetime
+  end
+end

db/post_migrate/20170427111108_remove_sync_time_column_from_remote_mirrors.rb

+class RemoveSyncTimeColumnFromRemoteMirrors < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    remove_concurrent_index :remote_mirrors, [:sync_time] if index_exists? :remote_mirrors, [:sync_time]
+    remove_column :remote_mirrors, :sync_time, :integer
+  end
+
+  def down
+    add_column :remote_mirrors, :sync_time, :integer
+    add_concurrent_index :remote_mirrors, [:sync_time]
+  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: 20170426175636) do
+ActiveRecord::Schema.define(version: 20170427180205) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1241,12 +1241,11 @@ ActiveRecord::Schema.define(version: 20170426175636) do
     t.string "encrypted_credentials_salt"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
-    t.integer "sync_time", default: 60, null: false
+    t.datetime "last_update_started_at"
   end
 
   add_index "remote_mirrors", ["last_successful_update_at"], name: "index_remote_mirrors_on_last_successful_update_at", using: :btree
   add_index "remote_mirrors", ["project_id"], name: "index_remote_mirrors_on_project_id", using: :btree
-  add_index "remote_mirrors", ["sync_time"], name: "index_remote_mirrors_on_sync_time", using: :btree
 
   create_table "routes", force: :cascade do |t|
     t.integer "source_id", null: false
@@ -1624,4 +1623,4 @@ ActiveRecord::Schema.define(version: 20170426175636) do
   add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
-end
\ No newline at end of file
+end

doc/workflow/repository_mirroring.md

@@ -71,9 +71,10 @@ repository to push to. Hit **Save changes** for the changes to take effect.
 
 Similarly to the pull mirroring, since the upstream repository functions as a
 mirror to the repository in GitLab, you are advised not to push commits directly
-to the mirrored repository. Instead, any commits should be pushed to GitLab,
-and will end up in the mirrored repository automatically within the configured time,
-or when a [forced update](#forcing-an-update) is initiated.
+to the mirrored repository. Instead, all changes will end up in the mirrored repository
+whenever commits are pushed to GitLab, or when a [forced update](#forcing-an-update) is initiated.
+
+Pushes into GitLab are automatically pushed to the remote mirror 5 minutes after they come in.
 
 In case of a diverged branch, you will see an error indicated at the
 **Mirror repository** settings.

lib/gitlab/mirror.rb

@@ -41,22 +41,12 @@ module Gitlab
         sync_times
       end
 
-      def configure_cron_jobs!
+      def configure_cron_job!
         minimum_mirror_sync_time = current_application_settings.minimum_mirror_sync_time rescue FIFTEEN
         sync_time = SYNC_TIME_TO_CRON[minimum_mirror_sync_time]
-        update_all_mirrors_worker_job = Sidekiq::Cron::Job.find("update_all_mirrors_worker")
-        update_all_remote_mirrors_worker_job = Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker")
 
-        if update_all_mirrors_worker_job && update_all_remote_mirrors_worker_job
-          update_all_mirrors_worker_job.destroy
-          update_all_remote_mirrors_worker_job.destroy
-        end
+        Sidekiq::Cron::Job.find("update_all_mirrors_worker")&.destroy
 
-        Sidekiq::Cron::Job.create(
-          name: 'update_all_remote_mirrors_worker',
-          cron: sync_time,
-          class: 'UpdateAllRemoteMirrorsWorker'
-        )
         Sidekiq::Cron::Job.create(
           name: 'update_all_mirrors_worker',
           cron: sync_time,

spec/controllers/projects/mirrors_controller_spec.rb

@@ -41,24 +41,6 @@ describe Projects::MirrorsController do
         end.to change { RemoteMirror.count }.to(1)
       end
 
-      context 'sync_time update' do
-        it 'allows sync_time update with valid time' do
-          sync_times.each do |sync_time|
-            expect do
-              do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com', 'sync_time' => sync_time } })
-            end.to change { RemoteMirror.where(sync_time: sync_time).count }.by(1)
-          end
-        end
-
-        it 'fails to update sync_time with invalid time' do
-          expect(@project.remote_mirrors.count).to eq(0)
-
-          expect do
-            do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com', 'sync_time' => 1000 } })
-          end.not_to change { @project.remote_mirrors.count }
-        end
-      end
-
       context 'when remote mirror has the same URL' do
         it 'does not allow to create the remote mirror' do
           expect do

spec/factories/projects.rb

@@ -65,13 +65,12 @@ FactoryGirl.define do
 
     trait :remote_mirror do
       transient do
-        sync_time Gitlab::Mirror::HOURLY
         url "http://foo.com"
         enabled true
       end
 
       after(:create) do |project, evaluator|
-        project.remote_mirrors.create!(url: evaluator.url, enabled: evaluator.enabled, sync_time: evaluator.sync_time)
+        project.remote_mirrors.create!(url: evaluator.url, enabled: evaluator.enabled)
       end
     end
 

spec/features/projects/mirror_spec.rb

@@ -30,7 +30,6 @@ feature 'Project mirror', feature: true do
 
           it 'shows the correct selector options' do
             expect(page).to have_selector('.project-mirror-sync-time > option', count: index + 1)
-            expect(page).to have_selector('.remote-mirror-sync-time > option', count: index + 1)
           end
         end
       end

spec/lib/gitlab/mirror_spec.rb

@@ -71,7 +71,7 @@ describe Gitlab::Mirror do
     after { Timecop.return }
   end
 
-  describe '#configure_cron_jobs!' do
+  describe '#configure_cron_job!' do
     let(:daily_cron)   { Gitlab::Mirror::SYNC_TIME_TO_CRON[Gitlab::Mirror::DAILY] }
     let(:twelve_cron)  { Gitlab::Mirror::SYNC_TIME_TO_CRON[Gitlab::Mirror::TWELVE] }
     let(:six_cron)     { Gitlab::Mirror::SYNC_TIME_TO_CRON[Gitlab::Mirror::SIX] }
@@ -82,7 +82,7 @@ describe Gitlab::Mirror do
     describe 'with jobs already running' do
       def setup_mirrors_cron_job(current, updated_time)
         allow_any_instance_of(ApplicationSetting).to receive(:minimum_mirror_sync_time).and_return(current)
-        Gitlab::Mirror.configure_cron_jobs!
+        Gitlab::Mirror.configure_cron_job!
         allow_any_instance_of(ApplicationSetting).to receive(:minimum_mirror_sync_time).and_return(updated_time)
       end
 
@@ -90,11 +90,7 @@ describe Gitlab::Mirror do
         before { setup_mirrors_cron_job(Gitlab::Mirror::HOURLY, Gitlab::Mirror::DAILY) }
 
         it 'changes cron of update_all_mirrors_worker to daily' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(hourly_cron).to(daily_cron)
-        end
-
-        it 'changes cron of update_all_remote_mirrors_worker to daily' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron }.from(hourly_cron).to(daily_cron)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(hourly_cron).to(daily_cron)
         end
       end
 
@@ -102,11 +98,7 @@ describe Gitlab::Mirror do
         before { setup_mirrors_cron_job(Gitlab::Mirror::DAILY, Gitlab::Mirror::TWELVE) }
 
         it 'changes cron of update_all_mirrors_worker to every twelve hours' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(twelve_cron)
-        end
-
-        it 'changes cron of update_all_remote_mirrors_worker to every twelve hours' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron }.from(daily_cron).to(twelve_cron)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(twelve_cron)
         end
       end
 
@@ -114,11 +106,7 @@ describe Gitlab::Mirror do
         before { setup_mirrors_cron_job(Gitlab::Mirror::DAILY, Gitlab::Mirror::SIX) }
 
         it 'changes cron of update_all_mirrors_worker to every six hours' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(six_cron)
-        end
-
-        it 'changes cron of update_all_remote_mirrors_worker to every six hours' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron }.from(daily_cron).to(six_cron)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(six_cron)
         end
       end
 
@@ -126,11 +114,7 @@ describe Gitlab::Mirror do
         before { setup_mirrors_cron_job(Gitlab::Mirror::DAILY, Gitlab::Mirror::THREE) }
 
         it 'changes cron of update_all_mirrors_worker to every three hours' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(three_cron)
-        end
-
-        it 'changes cron of update_all_remote_mirrors_worker to every three hours' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron }.from(daily_cron).to(three_cron)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(three_cron)
         end
       end
 
@@ -138,11 +122,7 @@ describe Gitlab::Mirror do
         before { setup_mirrors_cron_job(Gitlab::Mirror::DAILY, Gitlab::Mirror::HOURLY) }
 
         it 'changes cron of update_all_mirrors_worker to hourly' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(hourly_cron)
-        end
-
-        it 'changes cron of update_all_remote_mirrors_worker to hourly' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron }.from(daily_cron).to(hourly_cron)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(hourly_cron)
         end
       end
 
@@ -150,11 +130,7 @@ describe Gitlab::Mirror do
         before { setup_mirrors_cron_job(Gitlab::Mirror::DAILY, Gitlab::Mirror::FIFTEEN) }
 
         it 'changes cron of update_all_mirrors_worker to fifteen' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(fifteen_cron)
-        end
-
-        it 'changes cron of update_all_remote_mirrors_worker to fifteen' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron }.from(daily_cron).to(fifteen_cron)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron }.from(daily_cron).to(fifteen_cron)
         end
       end
     end
@@ -162,89 +138,58 @@ describe Gitlab::Mirror do
     describe 'without jobs already running' do
       before do
         Sidekiq::Cron::Job.find("update_all_mirrors_worker").destroy
-        Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").destroy
       end
 
       describe 'with daily minimum_mirror_sync_time' do
         before { allow_any_instance_of(ApplicationSetting).to receive(:minimum_mirror_sync_time).and_return(Gitlab::Mirror::DAILY) }
 
         it 'creates update_all_mirrors_worker with cron of daily sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
           expect(Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron).to eq(daily_cron)
         end
-
-        it 'creates update_all_remote_mirrors_worker with cron of daily sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
-          expect(Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron).to eq(daily_cron)
-        end
       end
 
       describe 'with twelve hours minimum_mirror_sync_time' do
         before { allow_any_instance_of(ApplicationSetting).to receive(:minimum_mirror_sync_time).and_return(Gitlab::Mirror::TWELVE) }
 
         it 'creates update_all_mirrors_worker with cron of every twelve hours sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
           expect(Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron).to eq(twelve_cron)
         end
-
-        it 'creates update_all_remote_mirrors_worker with cron of every twelve hours sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
-          expect(Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron).to eq(twelve_cron)
-        end
       end
 
       describe 'with six hours minimum_mirror_sync_time' do
         before { allow_any_instance_of(ApplicationSetting).to receive(:minimum_mirror_sync_time).and_return(Gitlab::Mirror::SIX) }
 
         it 'creates update_all_mirrors_worker with cron of every six hours sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
           expect(Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron).to eq(six_cron)
         end
-
-        it 'creates update_all_remote_mirrors_worker with cron of every six hours sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
-          expect(Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron).to eq(six_cron)
-        end
       end
 
       describe 'with three hours minimum_mirror_sync_time' do
         before { allow_any_instance_of(ApplicationSetting).to receive(:minimum_mirror_sync_time).and_return(Gitlab::Mirror::THREE) }
 
         it 'creates update_all_mirrors_worker with cron of every three hours sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
           expect(Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron).to eq(three_cron)
         end
-
-        it 'creates update_all_remote_mirrors_worker with cron of every three hours sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
-          expect(Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron).to eq(three_cron)
-        end
       end
 
       describe 'with hourly minimum_mirror_sync_time' do
         before { allow_any_instance_of(ApplicationSetting).to receive(:minimum_mirror_sync_time).and_return(Gitlab::Mirror::HOURLY) }
 
         it 'creates update_all_mirrors_worker with cron of hourly sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
           expect(Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron).to eq(hourly_cron)
         end
-
-        it 'creates update_all_remote_mirrors_worker with cron of hourly sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
-          expect(Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron).to eq(hourly_cron)
-        end
       end
 
       describe 'with fifteen minimum_mirror_sync_time' do
         it 'creates update_all_mirrors_worker with cron of fifteen sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
+          expect { Gitlab::Mirror.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
           expect(Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron).to eq(fifteen_cron)
         end
-
-        it 'creates update_all_remote_mirrors_worker with cron of fifteen sync_time' do
-          expect { Gitlab::Mirror.configure_cron_jobs! }.to change { Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
-          expect(Sidekiq::Cron::Job.find("update_all_remote_mirrors_worker").cron).to eq(fifteen_cron)
-        end
       end
     end
   end

spec/models/application_setting_spec.rb

@@ -89,7 +89,6 @@ describe ApplicationSetting, models: true do
         Sidekiq::Logging.logger = nil
         sync_times.each do |sync_time|
           create(:project, :mirror, sync_time: sync_time)
-          create(:project, :remote_mirror, sync_time: sync_time)
         end
       end
 
@@ -98,8 +97,8 @@ describe ApplicationSetting, models: true do
           subject { setting.update_attributes(minimum_mirror_sync_time: sync_time) }
 
           it "updates minimum mirror sync time to #{sync_time}" do
-            expect_any_instance_of(ApplicationSetting).to receive(:update_mirror_cron_jobs).and_call_original
-            expect(Gitlab::Mirror).to receive(:configure_cron_jobs!)
+            expect_any_instance_of(ApplicationSetting).to receive(:update_mirror_cron_job).and_call_original
+            expect(Gitlab::Mirror).to receive(:configure_cron_job!)
 
             subject
           end
@@ -107,10 +106,6 @@ describe ApplicationSetting, models: true do
           it 'updates every mirror to the current minimum_mirror_sync_time' do
             expect { subject }.to change { Project.mirror.where('sync_time < ?', sync_time).count }.from(index + 1).to(0)
           end
-
-          it 'updates every remote mirror to the current minimum_mirror_sync_time' do
-            expect { subject }.to change { RemoteMirror.where('sync_time < ?', sync_time).count }.from(index + 1).to(0)
-          end
         end
       end
 
@@ -119,8 +114,8 @@ describe ApplicationSetting, models: true do
         let(:sync_time) { Gitlab::Mirror::FIFTEEN }
 
         it 'does not update minimum_mirror_sync_time' do
-          expect_any_instance_of(ApplicationSetting).not_to receive(:update_mirror_cron_jobs)
-          expect(Gitlab::Mirror).not_to receive(:configure_cron_jobs!)
+          expect_any_instance_of(ApplicationSetting).not_to receive(:update_mirror_cron_job)
+          expect(Gitlab::Mirror).not_to receive(:configure_cron_job!)
           expect(setting.minimum_mirror_sync_time).to eq(Gitlab::Mirror::FIFTEEN)
 
           setting.update_attributes(minimum_mirror_sync_time: sync_time)
@@ -129,10 +124,6 @@ describe ApplicationSetting, models: true do
         it 'updates every mirror to the current minimum_mirror_sync_time' do
           expect { setting.update_attributes(minimum_mirror_sync_time: sync_time) }.not_to change { Project.mirror.where('sync_time < ?', sync_time).count }
         end
-
-        it 'updates every remote mirror to the current minimum_mirror_sync_time' do
-          expect { setting.update_attributes(minimum_mirror_sync_time: sync_time) }.not_to change { RemoteMirror.where('sync_time < ?', sync_time).count }
-        end
       end
     end
 

spec/models/project_spec.rb

@@ -195,6 +195,21 @@ describe Project, models: true do
       end
     end
 
+    context '#mark_stuck_remote_mirrors_as_failed!' do
+      it 'fails stuck remote mirrors' do
+        project = create(:project, :remote_mirror)
+
+        project.remote_mirrors.first.update_attributes(
+          update_status: :started,
+          last_update_at: 2.days.ago
+        )
+
+        expect do
+          project.mark_stuck_remote_mirrors_as_failed!
+        end.to change { project.remote_mirrors.stuck.count }.from(1).to(0)
+      end
+    end
+
     context 'mirror' do
       subject { build(:project, mirror: true) }
 

spec/models/remote_mirror_spec.rb

@@ -85,6 +85,66 @@ describe RemoteMirror do
     end
   end
 
+  context '#sync' do
+    let(:remote_mirror) { create(:project, :remote_mirror).remote_mirrors.first }
+
+    before do
+      Timecop.freeze(Time.now)
+    end
+
+    context 'with remote mirroring enabled' do
+      it 'schedules a RepositoryUpdateRemoteMirrorWorker to run within a certain backoff delay' do
+        expect(RepositoryUpdateRemoteMirrorWorker).to receive(:perform_in).with(RemoteMirror::BACKOFF_DELAY, remote_mirror.id, Time.now)
+
+        remote_mirror.sync
+      end
+    end
+
+    context 'with remote mirroring disabled' do
+      it 'returns nil' do
+        remote_mirror.update_attributes(enabled: false)
+
+        expect(remote_mirror.sync).to be_nil
+      end
+    end
+
+    context 'without project' do
+      it 'returns nil' do
+        allow_any_instance_of(RemoteMirror).to receive(:project).and_return(nil)
+
+        expect(remote_mirror.sync).to be_nil
+      end
+    end
+  end
+
+  context '#updated_since?' do
+    let(:remote_mirror) { create(:project, :remote_mirror).remote_mirrors.first }
+    let(:timestamp) { Time.now - 5.minutes }
+
+    before do
+      Timecop.freeze(Time.now)
+      remote_mirror.update_attributes(last_update_started_at: Time.now)
+    end
+
+    context 'when remote mirror does not have status failed' do
+      it 'returns true when last update started after the timestamp' do
+        expect(remote_mirror.updated_since?(timestamp)).to be true
+      end
+
+      it 'returns false when last update started before the timestamp' do
+        expect(remote_mirror.updated_since?(Time.now + 5.minutes)).to be  false
+      end
+    end
+
+    context 'when remote mirror has status failed' do
+      it 'returns false when last update started after the timestamp' do
+        remote_mirror.update_attributes(update_status: 'failed')
+
+        expect(remote_mirror.updated_since?(timestamp)).to be false
+      end
+    end
+  end
+
   context 'no project' do
     it 'includes mirror with a project in pending_delete' do
       mirror = create_mirror(url: 'http://cantbeblank',

spec/services/git_push_service_spec.rb

@@ -3,27 +3,45 @@ require 'spec_helper'
 describe GitPushService, services: true do
   include RepoHelpers
 
-  let(:user)    { create(:user) }
-  let(:project) { create(:project, :repository) }
+  let(:user)     { create(:user) }
+  let(:project)  { create(:project, :repository) }
+  let(:blankrev) { Gitlab::Git::BLANK_SHA }
+  let(:oldrev)   { sample_commit.parent_id }
+  let(:newrev)   { sample_commit.id }
+  let(:ref)      { 'refs/heads/master' }
 
   before do
     project.team << [user, :master]
-    @blankrev = Gitlab::Git::BLANK_SHA
-    @oldrev = sample_commit.parent_id
-    @newrev = sample_commit.id
-    @ref = 'refs/heads/master'
   end
 
-  describe 'Push branches' do
-    let(:oldrev) { @oldrev }
-    let(:newrev) { @newrev }
+  describe 'with remote mirrors' do
+    let(:project)  { create(:project, :remote_mirror) }
 
     subject do
-      execute_service(project, user, oldrev, newrev, @ref )
+      described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
+    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
+
+  describe 'Push branches' do
+    subject do
+      execute_service(project, user, oldrev, newrev, ref)
     end
 
     context 'new branch' do
-      let(:oldrev) { @blankrev }
+      let(:oldrev) { blankrev }
 
       it { is_expected.to be_truthy }
 
@@ -51,7 +69,7 @@ describe GitPushService, services: true do
     end
 
     context 'rm branch' do
-      let(:newrev) { @blankrev }
+      let(:newrev) { blankrev }
 
       it { is_expected.to be_truthy }
 
@@ -70,24 +88,20 @@ describe GitPushService, services: true do
   end
 
   describe "Git Push Data" do
-    before do
-      service = execute_service(project, user, @oldrev, @newrev, @ref )
-      @push_data = service.push_data
-      @commit = project.commit(@newrev)
-    end
+    let(:commit) { project.commit(newrev) }
 
-    subject { @push_data }
+    subject { push_data_from_service(project, user, oldrev, newrev, ref) }
 
     it { is_expected.to include(object_kind: 'push') }
-    it { is_expected.to include(before: @oldrev) }
-    it { is_expected.to include(after: @newrev) }
-    it { is_expected.to include(ref: @ref) }
+    it { is_expected.to include(before: oldrev) }
+    it { is_expected.to include(after: newrev) }
+    it { is_expected.to include(ref: ref) }
     it { is_expected.to include(user_id: user.id) }
     it { is_expected.to include(user_name: user.name) }
     it { is_expected.to include(project_id: project.id) }
 
     context "with repository data" do
-      subject { @push_data[:repository] }
+      subject { push_data_from_service(project, user, oldrev, newrev, ref)[:repository] }
 
       it { is_expected.to include(name: project.name) }
       it { is_expected.to include(url: project.url_to_repo) }
@@ -96,7 +110,7 @@ describe GitPushService, services: true do
     end
 
     context "with commits" do
-      subject { @push_data[:commits] }
+      subject { push_data_from_service(project, user, oldrev, newrev, ref)[:commits] }
 
       it { is_expected.to be_an(Array) }
       it 'has 1 element' do
@@ -104,11 +118,11 @@ describe GitPushService, services: true do
       end
 
       context "the commit" do
-        subject { @push_data[:commits].first }
+        subject { push_data_from_service(project, user, oldrev, newrev, ref)[:commits].first }
 
-        it { is_expected.to include(id: @commit.id) }
-        it { is_expected.to include(message: @commit.safe_message) }
-        it { is_expected.to include(timestamp: @commit.date.xmlschema) }
+        it { is_expected.to include(id: commit.id) }
+        it { is_expected.to include(message: commit.safe_message) }
+        it { is_expected.to include(timestamp: commit.date.xmlschema) }
         it do
           is_expected.to include(
             url: [
@@ -116,16 +130,16 @@ describe GitPushService, services: true do
               project.namespace.to_param,
               project.to_param,
               'commit',
-              @commit.id
+              commit.id
             ].join('/')
           )
         end
 
         context "with a author" do
-          subject { @push_data[:commits].first[:author] }
+          subject { push_data_from_service(project, user, oldrev, newrev, ref)[:commits].first[:author] }
 
-          it { is_expected.to include(name: @commit.author_name) }
-          it { is_expected.to include(email: @commit.author_email) }
+          it { is_expected.to include(name: commit.author_name) }
+          it { is_expected.to include(email: commit.author_email) }
         end
       end
     end
@@ -143,40 +157,37 @@ describe GitPushService, services: true do
     it "does not trigger indexer when push to non-default branch" do
       expect_any_instance_of(Gitlab::Elastic::Indexer).not_to receive(:run)
 
-      execute_service(project, user, @oldrev, @newrev, 'refs/heads/other')
+      execute_service(project, user, oldrev, newrev, 'refs/heads/other')
     end
 
     it "triggers indexer when push to default branch" do
       expect_any_instance_of(Gitlab::Elastic::Indexer).to receive(:run)
 
-      execute_service(project, user, @oldrev, @newrev, 'refs/heads/master')
+      execute_service(project, user, oldrev, newrev, ref)
     end
   end
 
   describe "Push Event" do
-    before do
-      service = execute_service(project, user, @oldrev, @newrev, @ref )
-      @event = Event.find_by_action(Event::PUSHED)
-      @push_data = service.push_data
-    end
+    let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) }
+    let(:event) { Event.find_by_action(Event::PUSHED) }
 
-    it { expect(@event).not_to be_nil }
-    it { expect(@event.project).to eq(project) }
-    it { expect(@event.action).to eq(Event::PUSHED) }
-    it { expect(@event.data).to eq(@push_data) }
+    it { expect(event).not_to be_nil }
+    it { expect(event.project).to eq(project) }
+    it { expect(event.action).to eq(Event::PUSHED) }
+    it { expect(event.data).to eq(push_data) }
 
     context "Updates merge requests" do
       it "when pushing a new branch for the first time" do
         expect(UpdateMergeRequestsWorker).to receive(:perform_async).
-                                                with(project.id, user.id, @blankrev, 'newrev', 'refs/heads/master')
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+            with(project.id, user.id, blankrev, 'newrev', ref)
+        execute_service(project, user, blankrev, 'newrev', ref)
       end
     end
     
     context "Sends System Push data" do
       it "when pushing on a branch" do
-        expect(SystemHookPushWorker).to receive(:perform_async).with(@push_data, :push_hooks)
-        execute_service(project, user, @oldrev, @newrev, @ref )
+        expect(SystemHookPushWorker).to receive(:perform_async).with(push_data, :push_hooks)
+        execute_service(project, user, oldrev, newrev, ref)
       end
     end
   end
@@ -186,13 +197,13 @@ describe GitPushService, services: true do
       it "calls the copy attributes method for the first push to the default branch" do
         expect(project.repository).to receive(:copy_gitattributes).with('master')
 
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master')
+        execute_service(project, user, blankrev, 'newrev', ref)
       end
 
       it "calls the copy attributes method for changes to the default branch" do
-        expect(project.repository).to receive(:copy_gitattributes).with('refs/heads/master')
+        expect(project.repository).to receive(:copy_gitattributes).with(ref)
 
-        execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master')
+        execute_service(project, user, 'oldrev', 'newrev', ref)
       end
     end
 
@@ -205,7 +216,7 @@ describe GitPushService, services: true do
       it "does not call copy attributes method" do
         expect(project.repository).not_to receive(:copy_gitattributes)
 
-        execute_service(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, oldrev, newrev, ref)
       end
     end
   end
@@ -215,7 +226,7 @@ describe GitPushService, services: true do
       it "when pushing a branch for the first time" do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        execute_service(project, user, blankrev, 'newrev', ref)
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
         expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
@@ -226,7 +237,7 @@ describe GitPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        execute_service(project, user, blankrev, 'newrev', ref)
         expect(project.protected_branches).to be_empty
       end
 
@@ -236,7 +247,7 @@ describe GitPushService, services: true do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
 
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        execute_service(project, user, blankrev, 'newrev', ref)
 
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
@@ -253,7 +264,7 @@ describe GitPushService, services: true do
         expect(project.default_branch).to eq("master")
         expect_any_instance_of(ProtectedBranches::CreateService).not_to receive(:execute)
 
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        execute_service(project, user, blankrev, 'newrev', ref)
 
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::NO_ACCESS])
@@ -265,7 +276,7 @@ describe GitPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        execute_service(project, user, blankrev, 'newrev', ref)
         expect(project.protected_branches).not_to be_empty
         expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
         expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
@@ -273,7 +284,7 @@ describe GitPushService, services: true do
 
       it "when pushing new commits to existing branch" do
         expect(project).to receive(:execute_hooks)
-        execute_service(project, user, 'oldrev', 'newrev', 'refs/heads/master' )
+        execute_service(project, user, 'oldrev', 'newrev', ref)
       end
     end
   end
@@ -303,7 +314,7 @@ describe GitPushService, services: true do
     it "creates a note if a pushed commit mentions an issue" do
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author)
 
-      execute_service(project, user, @oldrev, @newrev, @ref )
+      execute_service(project, user, oldrev, newrev, ref)
     end
 
     it "only creates a cross-reference note if one doesn't already exist" do
@@ -311,7 +322,7 @@ describe GitPushService, services: true do
 
       expect(SystemNoteService).not_to receive(:cross_reference).with(issue, commit, commit_author)
 
-      execute_service(project, user, @oldrev, @newrev, @ref )
+      execute_service(project, user, oldrev, newrev, ref)
     end
 
     it "defaults to the pushing user if the commit's author is not known" do
@@ -321,16 +332,16 @@ describe GitPushService, services: true do
       )
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, user)
 
-      execute_service(project, user, @oldrev, @newrev, @ref )
+      execute_service(project, user, oldrev, newrev, ref)
     end
 
     it "finds references in the first push to a non-default branch" do
-      allow(project.repository).to receive(:commits_between).with(@blankrev, @newrev).and_return([])
-      allow(project.repository).to receive(:commits_between).with("master", @newrev).and_return([commit])
+      allow(project.repository).to receive(:commits_between).with(blankrev, newrev).and_return([])
+      allow(project.repository).to receive(:commits_between).with("master", newrev).and_return([commit])
 
       expect(SystemNoteService).to receive(:cross_reference).with(issue, commit, commit_author)
 
-      execute_service(project, user, @blankrev, @newrev, 'refs/heads/other' )
+      execute_service(project, user, blankrev, newrev, 'refs/heads/other')
     end
   end
 
@@ -360,14 +371,14 @@ describe GitPushService, services: true do
 
     context "while saving the 'first_mentioned_in_commit_at' metric for an issue" do
       it 'sets the metric for referenced issues' do
-        execute_service(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, oldrev, newrev, ref)
 
         expect(issue.reload.metrics.first_mentioned_in_commit_at).to be_like_time(commit_time)
       end
 
       it 'does not set the metric for non-referenced issues' do
         non_referenced_issue = create(:issue, project: project)
-        execute_service(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, oldrev, newrev, ref)
 
         expect(non_referenced_issue.reload.metrics.first_mentioned_in_commit_at).to be_nil
       end
@@ -399,18 +410,18 @@ describe GitPushService, services: true do
 
     context "to default branches" do
       it "closes issues" do
-        execute_service(project, commit_author, @oldrev, @newrev, @ref )
+        execute_service(project, commit_author, oldrev, newrev, ref)
         expect(Issue.find(issue.id)).to be_closed
       end
 
       it "adds a note indicating that the issue is now closed" do
         expect(SystemNoteService).to receive(:change_status).with(issue, project, commit_author, "closed", closing_commit)
-        execute_service(project, commit_author, @oldrev, @newrev, @ref )
+        execute_service(project, commit_author, oldrev, newrev, ref)
       end
 
       it "doesn't create additional cross-reference notes" do
         expect(SystemNoteService).not_to receive(:cross_reference)
-        execute_service(project, commit_author, @oldrev, @newrev, @ref )
+        execute_service(project, commit_author, oldrev, newrev, ref)
       end
 
       it "doesn't close issues when external issue tracker is in use" do
@@ -421,7 +432,7 @@ describe GitPushService, services: true do
 
         # The push still shouldn't create cross-reference notes.
         expect do
-          execute_service(project, commit_author, @oldrev, @newrev,  'refs/heads/hurf' )
+          execute_service(project, commit_author, oldrev, newrev,  'refs/heads/hurf')
         end.not_to change { Note.where(project_id: project.id, system: true).count }
       end
     end
@@ -434,11 +445,11 @@ describe GitPushService, services: true do
 
       it "creates cross-reference notes" do
         expect(SystemNoteService).to receive(:cross_reference).with(issue, closing_commit, commit_author)
-        execute_service(project, user, @oldrev, @newrev, @ref )
+        execute_service(project, user, oldrev, newrev, ref)
       end
 
       it "doesn't close issues" do
-        execute_service(project, user, @oldrev, @newrev, @ref )
+        execute_service(project, user, oldrev, newrev, ref)
         expect(Issue.find(issue.id)).to be_opened
       end
     end
@@ -472,7 +483,7 @@ describe GitPushService, services: true do
         let(:message) { "this is some work.\n\nrelated to JIRA-1" }
 
         it "initiates one api call to jira server to mention the issue" do
-          execute_service(project, user, @oldrev, @newrev, @ref)
+          execute_service(project, user, oldrev, newrev, ref)
 
           expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with(
             body: /mentioned this issue in/
@@ -482,7 +493,11 @@ describe GitPushService, services: true do
 
       context "closing an issue" do
         let(:message)         { "this is some work.\n\ncloses JIRA-1" }
-        let(:comment_body)    { { body: "Issue solved with [#{closing_commit.id}|http://#{Gitlab.config.gitlab.host}/#{project.path_with_namespace}/commit/#{closing_commit.id}]." }.to_json }
+        let(:comment_body) do
+          {
+            body: "Issue solved with [#{closing_commit.id}|http://#{Gitlab.config.gitlab.host}/#{project.path_with_namespace}/commit/#{closing_commit.id}]."
+          }.to_json
+        end
 
         before do
           open_issue   = JIRA::Resource::Issue.new(jira_tracker.client, attrs: { "id" => "JIRA-1" })
@@ -496,13 +511,13 @@ describe GitPushService, services: true do
 
         context "using right markdown" do
           it "initiates one api call to jira server to close the issue" do
-            execute_service(project, commit_author, @oldrev, @newrev, @ref )
+            execute_service(project, commit_author, oldrev, newrev, ref)
 
             expect(WebMock).to have_requested(:post, jira_api_transition_url('JIRA-1')).once
           end
 
           it "initiates one api call to jira server to comment on the issue" do
-            execute_service(project, commit_author, @oldrev, @newrev, @ref )
+            execute_service(project, commit_author, oldrev, newrev, ref)
 
             expect(WebMock).to have_requested(:post, jira_api_comment_url('JIRA-1')).with(
               body: comment_body
@@ -514,13 +529,13 @@ describe GitPushService, services: true do
           let(:message) { "this is some work.\n\ncloses #1" }
 
           it "does not initiates one api call to jira server to close the issue" do
-            execute_service(project, commit_author, @oldrev, @newrev, @ref )
+            execute_service(project, commit_author, oldrev, newrev, ref)
 
             expect(WebMock).not_to have_requested(:post, jira_api_transition_url('JIRA-1'))
           end
 
           it "does not initiates one api call to jira server to comment on the issue" do
-            execute_service(project, commit_author, @oldrev, @newrev, @ref )
+            execute_service(project, commit_author, oldrev, newrev, ref)
 
             expect(WebMock).not_to have_requested(:post, jira_api_comment_url('JIRA-1')).with(
               body: comment_body
@@ -533,7 +548,7 @@ describe GitPushService, services: true do
 
   describe "empty project" do
     let(:project) { create(:project_empty_repo) }
-    let(:new_ref) { 'refs/heads/feature'}
+    let(:new_ref) { 'refs/heads/feature' }
 
     before do
       allow(project).to receive(:default_branch).and_return('feature')
@@ -541,7 +556,7 @@ describe GitPushService, services: true do
     end
 
     it 'push to first branch updates HEAD' do
-      execute_service(project, user, @blankrev, @newrev, new_ref )
+      execute_service(project, user, blankrev, newrev, new_ref)
     end
   end
 
@@ -562,7 +577,7 @@ describe GitPushService, services: true do
     it 'does not perform housekeeping when not needed' do
       expect(housekeeping).not_to receive(:execute)
 
-      execute_service(project, user, @oldrev, @newrev, @ref)
+      execute_service(project, user, oldrev, newrev, ref)
     end
 
     context 'when housekeeping is needed' do
@@ -573,20 +588,20 @@ describe GitPushService, services: true do
       it 'performs housekeeping' do
         expect(housekeeping).to receive(:execute)
 
-        execute_service(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, oldrev, newrev, ref)
       end
 
       it 'does not raise an exception' do
         allow(housekeeping).to receive(:try_obtain_lease).and_return(false)
 
-        execute_service(project, user, @oldrev, @newrev, @ref)
+        execute_service(project, user, oldrev, newrev, ref)
       end
     end
 
     it 'increments the push counter' do
       expect(housekeeping).to receive(:increment!)
 
-      execute_service(project, user, @oldrev, @newrev, @ref)
+      execute_service(project, user, oldrev, newrev, ref)
     end
   end
 
@@ -594,9 +609,9 @@ describe GitPushService, services: true do
     let(:service) do
       described_class.new(project,
                           user,
-                          oldrev: sample_commit.parent_id,
-                          newrev: sample_commit.id,
-                          ref: 'refs/heads/master')
+                          oldrev: oldrev,
+                          newrev: newrev,
+                          ref: ref)
     end
 
     context 'on the default branch' do
@@ -639,9 +654,9 @@ describe GitPushService, services: true do
     let(:service) do
       described_class.new(project,
                           user,
-                          oldrev: sample_commit.parent_id,
-                          newrev: sample_commit.id,
-                          ref: 'refs/heads/master')
+                          oldrev: oldrev,
+                          newrev: newrev,
+                          ref: ref)
     end
 
     it 'only schedules a limited number of commits' do
@@ -655,8 +670,12 @@ describe GitPushService, services: true do
   end
 
   def execute_service(project, user, oldrev, newrev, ref)
-    service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref )
+    service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
     service.execute
     service
   end
+
+  def push_data_from_service(project, user, oldrev, newrev, ref)
+    execute_service(project, user, oldrev, newrev, ref).push_data
+  end
 end

spec/workers/repository_update_remote_mirror_worker_spec.rb

+require 'rails_helper'
+
+describe RepositoryUpdateRemoteMirrorWorker do
+  subject { described_class.new }
+
+  let(:remote_mirror) { create(:project, :remote_mirror).remote_mirrors.first }
+  let(:scheduled_time) { Time.now - 5.minutes }
+
+  before do
+    Timecop.freeze(Time.now)
+  end
+
+  describe '#perform' do
+    context 'with status none' do
+      before do
+        remote_mirror.update_attributes(update_status: 'none')
+      end
+
+      it 'sets status as finished when update remote mirror service executes successfully' do
+        expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :success)
+
+        expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.update_status }.to('finished')
+      end
+
+      it 'sets status as failed when update remote mirror service executes with errors' do
+        error_message = 'fail!'
+
+        expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :error, message: error_message)
+        expect do
+          subject.perform(remote_mirror.id, Time.now)
+        end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateError, error_message)
+
+        expect(remote_mirror.reload.update_status).to eq('failed')
+      end
+
+      it 'does nothing if last_update_started_at is higher than the time the job was scheduled in' do
+        remote_mirror.update_attributes(last_update_started_at: Time.now)
+
+        expect_any_instance_of(RemoteMirror).to receive(:updated_since?).with(scheduled_time).and_return(true)
+        expect_any_instance_of(Projects::UpdateRemoteMirrorService).not_to receive(:execute).with(remote_mirror)
+
+        expect(subject.perform(remote_mirror.id, scheduled_time)).to be_nil
+      end
+    end
+
+    context 'with another worker already running' do
+      before do
+        remote_mirror.update_attributes(update_status: 'started')
+      end
+
+      it 'raises RemoteMirrorUpdateAlreadyInProgressError' do
+        expect do
+          subject.perform(remote_mirror.id, Time.now)
+        end.to raise_error(RepositoryUpdateRemoteMirrorWorker::UpdateAlreadyInProgressError)
+      end
+    end
+
+    context 'with status failed' do
+      before do
+        remote_mirror.update_attributes(update_status: 'failed')
+      end
+
+      it 'sets status as finished if last_update_started_at is higher than the time the job was scheduled in' do
+        remote_mirror.update_attributes(last_update_started_at: Time.now)
+
+        expect_any_instance_of(RemoteMirror).to receive(:updated_since?).with(scheduled_time).and_return(false)
+        expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :success)
+
+        expect { subject.perform(remote_mirror.id, scheduled_time) }.to change { remote_mirror.reload.update_status }.to('finished')
+      end
+    end
+  end
+end

spec/workers/update_all_remote_mirrors_worker_spec.rb

-require 'rails_helper'
-
-describe UpdateAllRemoteMirrorsWorker do
-  subject(:worker) { described_class.new }
-
-  describe "#perform" do
-    let!(:fifteen_mirror) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::FIFTEEN) }
-    let!(:hourly_mirror) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::HOURLY) }
-    let!(:three_mirror) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::THREE) }
-    let!(:six_mirror) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::SIX) }
-    let!(:twelve_mirror) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::TWELVE) }
-    let!(:daily_mirror) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::DAILY) }
-    let!(:outdated_mirror) { create(:project, :remote_mirror) }
-
-    it 'fails stuck mirrors' do
-      expect(worker).to receive(:fail_stuck_mirrors!)
-
-      worker.perform
-    end
-
-    describe 'sync time' do
-      def expect_worker_to_enqueue_mirrors(mirrors)
-        mirrors.each do |mirror|
-          expect(RepositoryUpdateRemoteMirrorWorker).to receive(:perform_async).with(mirror.id)
-        end
-
-        worker.perform
-      end
-
-      before do
-        time = DateTime.now.change(time_params)
-
-        Timecop.freeze(time)
-        outdated_mirror.remote_mirrors.first.update_attributes(last_successful_update_at: time - (Gitlab::Mirror::DAILY + 5).minutes)
-      end
-
-      describe 'fifteen' do
-        let!(:time_params) { { hour: 1, min: 15 } }
-        let(:mirrors) { [fifteen_mirror, outdated_mirror] }
-
-        it { expect_worker_to_enqueue_mirrors(mirrors) }
-      end
-
-      describe "hourly" do
-        let!(:time_params) { { hour: 1 } }
-        let(:mirrors) { [fifteen_mirror, hourly_mirror, outdated_mirror] }
-
-        it { expect_worker_to_enqueue_mirrors(mirrors) }
-      end
-
-      describe "three" do
-        let!(:time_params) { { hour: 3 } }
-        let(:mirrors) { [fifteen_mirror, hourly_mirror, three_mirror, outdated_mirror] }
-
-        it { expect_worker_to_enqueue_mirrors(mirrors) }
-      end
-
-      describe "six" do
-        let!(:time_params) { { hour: 6 } }
-        let(:mirrors) { [fifteen_mirror, hourly_mirror, three_mirror, six_mirror, outdated_mirror] }
-
-        it { expect_worker_to_enqueue_mirrors(mirrors) }
-      end
-
-      describe "twelve" do
-        let!(:time_params) { { hour: 12 } }
-        let(:mirrors) { [fifteen_mirror, hourly_mirror, three_mirror, six_mirror, twelve_mirror, outdated_mirror] }
-
-        it { expect_worker_to_enqueue_mirrors(mirrors) }
-      end
-
-      describe "daily" do
-        let!(:time_params) { { hour: 0 } }
-        let(:mirrors) { [fifteen_mirror, hourly_mirror, three_mirror, six_mirror, twelve_mirror, daily_mirror, outdated_mirror] }
-
-        it { expect_worker_to_enqueue_mirrors(mirrors) }
-      end
-
-      after { Timecop.return }
-    end
-  end
-end