stop using sidekiq cron for push mirrors

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
+
+  BACKOFF_DELAY = 5
+  MAX_RETRIES = 5
 
   attr_encrypted :credentials,
                  key: Gitlab::Application.secrets.db_key_base,
@@ -9,12 +13,11 @@ class RemoteMirror < ActiveRecord::Base
                  insecure_mode: true,
                  algorithm: 'aes-256-cbc'
 
+  ignore_column :sync_time
+
   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? }
 
@@ -49,14 +52,14 @@ class RemoteMirror < ActiveRecord::Base
 
     after_transition any => :started, do: :schedule_update_job
 
-    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
@@ -77,7 +80,7 @@ class RemoteMirror < ActiveRecord::Base
     return unless project
     return if !enabled || update_in_progress?
 
-    update_failed? ? update_retry : update_start
+    update_start
   end
 
   def mark_for_delete_if_blank_url
@@ -135,9 +138,7 @@ class RemoteMirror < ActiveRecord::Base
   end
 
   def add_update_job
-    if project && project.repository_exists?
-      RepositoryUpdateRemoteMirrorWorker.perform_async(self.id)
-    end
+    RepositoryUpdateRemoteMirrorWorker.perform_async(self.id, Time.now) if project&.repository_exists?
   end
 
   def refresh_remote

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_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 every push with the content
+        of the current repository.
         = 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 five minutes after every push.
         .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

@@ -4,14 +4,22 @@ class RepositoryUpdateRemoteMirrorWorker
   include Sidekiq::Worker
   include Gitlab::ShellAdapter
 
-  sidekiq_options queue: :project_mirror, retry: false
+  sidekiq_options queue: :project_mirror, retry: RemoteMirror::MAX_RETRIES
 
-  def perform(remote_mirror_id)
+  sidekiq_retry_in do |count|
+    RemoteMirror::BACKOFF_DELAY**count
+  end
+
+  def perform(remote_mirror_id, current_time)
     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)
+      remote_mirror  = RemoteMirror.find(remote_mirror_id)
+      last_update_at = remote_mirror.last_update_at
+      project        = remote_mirror.project
+      current_user   = project.creator
+
+      return if last_update_at && last_update_at > current_time
+
+      result = Projects::UpdateRemoteMirrorService.new(project, current_user).execute(remote_mirror)
 
       if result[:status] == :error
         remote_mirror.mark_as_failed(result[:message])

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!
 

doc/workflow/repository_mirroring.md

@@ -75,6 +75,9 @@ 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.
 
+Push mirroring unlike pull mirroring, does not have any synchronization options available,
+therefore triggering the update whenever a push happens to the respective GitLab repository.
+
 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/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_as_failed_stuck_remote_mirrors('some message')
+        end.to change { project.remote_mirrors.stuck.count }.from(1).to(0)
+      end
+    end
+
     context 'mirror' do
       subject { build(:project, mirror: true) }
 

spec/services/git_push_service_spec.rb

@@ -9,9 +9,32 @@ describe GitPushService, services: true do
   before do
     project.team << [user, :master]
     @blankrev = Gitlab::Git::BLANK_SHA
-    @oldrev = sample_commit.parent_id
-    @newrev = sample_commit.id
-    @ref = 'refs/heads/master'
+    @oldrev   = sample_commit.parent_id
+    @newrev   = sample_commit.id
+    @ref      = 'refs/heads/master'
+  end
+
+  describe 'with remote mirrors' do
+    let(:project)  { create(:project, :remote_mirror) }
+    let(:oldrev)   { @oldrev }
+    let(:newrev)   { @newrev }
+    let(:ref)      { @ref }
+
+    subject do
+      described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref )
+    end
+
+    it 'fails stuck remote mirrors' do
+      expect(subject).to receive(:update_remote_mirrors)
+
+      subject.execute
+    end
+
+    it 'updates remote mirrors' do
+      expect(subject).to receive(:update_remote_mirrors)
+
+      subject.execute
+    end
   end
 
   describe 'Push branches' do
@@ -19,7 +42,7 @@ describe GitPushService, services: true do
     let(:newrev) { @newrev }
 
     subject do
-      execute_service(project, user, oldrev, newrev, @ref )
+      execute_service(project, user, oldrev, newrev, @ref)
     end
 
     context 'new branch' do

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 }
+
+  before do
+    remote_mirror.update_attributes(update_status: 'started')
+  end
+
+  describe '#perform' do
+    it 'sets sync 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 sync as failed when update remote mirror service executes with errors' do
+      expect_any_instance_of(Projects::UpdateRemoteMirrorService).to receive(:execute).with(remote_mirror).and_return(status: :error, message: 'fail!')
+
+      expect { subject.perform(remote_mirror.id, Time.now) }.to change { remote_mirror.reload.update_status }.to('failed')
+    end
+
+    it 'does nothing if last_update_at is higher than the time the job was scheduled in' do
+      expect_any_instance_of(RemoteMirror).to receive(:last_update_at).and_return(Time.now + 10.minutes)
+      expect_any_instance_of(Projects::UpdateRemoteMirrorService).not_to receive(:execute).with(remote_mirror)
+
+      expect(subject.perform(remote_mirror.id, Time.now)).to be_nil
+    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