Merge branch 'mirror-scheduler-fix' into 'master'

Fixes scheduling issue related to all mirrors

See merge request !1183

app/workers/update_all_mirrors_worker.rb

@@ -27,7 +27,7 @@ class UpdateAllMirrorsWorker
   private
 
   def mirrors_to_sync
-    Project.mirror.where(sync_time: Gitlab::Mirror.sync_times)
+    Project.mirror.where("mirror_last_successful_update_at + #{Gitlab::Database.minute_interval('sync_time')} <= ? OR sync_time IN (?)", DateTime.now, Gitlab::Mirror.sync_times)
   end
 
   def try_obtain_lease

app/workers/update_all_remote_mirrors_worker.rb

@@ -17,6 +17,6 @@ class UpdateAllRemoteMirrorsWorker
   private
 
   def remote_mirrors_to_sync
-    RemoteMirror.where(sync_time: Gitlab::Mirror.sync_times)
+    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/mirror-scheduler-fix.yml

+---
+title: Try to update mirrors again after 15 minutes if the previous update failed
+merge_request: 1183
+author:

db/migrate/20170208144550_add_index_to_mirrors_last_update_at_fields.rb

+class AddIndexToMirrorsLastUpdateAtFields < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    add_concurrent_index :projects, :mirror_last_successful_update_at
+    add_concurrent_index :remote_mirrors, :last_successful_update_at
+  end
+end

db/schema.rb

@@ -1127,6 +1127,7 @@ ActiveRecord::Schema.define(version: 20170215200045) do
   add_index "projects", ["description"], name: "index_projects_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"}
   add_index "projects", ["last_activity_at"], name: "index_projects_on_last_activity_at", using: :btree
   add_index "projects", ["last_repository_check_failed"], name: "index_projects_on_last_repository_check_failed", using: :btree
+  add_index "projects", ["mirror_last_successful_update_at"], name: "index_projects_on_mirror_last_successful_update_at", using: :btree
   add_index "projects", ["name"], name: "index_projects_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
   add_index "projects", ["namespace_id"], name: "index_projects_on_namespace_id", using: :btree
   add_index "projects", ["path"], name: "index_projects_on_path", using: :btree
@@ -1216,6 +1217,7 @@ ActiveRecord::Schema.define(version: 20170215200045) do
     t.integer "sync_time", default: 60, null: false
   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
 

lib/gitlab/database.rb

@@ -24,7 +24,7 @@ module Gitlab
     def self.nulls_last_order(field, direction = 'ASC')
       order = "#{field} #{direction}"
 
-      if Gitlab::Database.postgresql?
+      if postgresql?
         order << ' NULLS LAST'
       else
         # `field IS NULL` will be `0` for non-NULL columns and `1` for NULL
@@ -38,7 +38,7 @@ module Gitlab
     def self.nulls_first_order(field, direction = 'ASC')
       order = "#{field} #{direction}"
 
-      if Gitlab::Database.postgresql?
+      if postgresql?
         order << ' NULLS FIRST'
       else
         # `field IS NULL` will be `0` for non-NULL columns and `1` for NULL
@@ -50,7 +50,11 @@ module Gitlab
     end
 
     def self.random
-      Gitlab::Database.postgresql? ? "RANDOM()" : "RAND()"
+      postgresql? ? "RANDOM()" : "RAND()"
+    end
+
+    def self.minute_interval(value)
+      postgresql? ? "#{value} * '1 minute'::interval" : "INTERVAL #{value} MINUTE"
     end
 
     def true_value

spec/workers/update_all_mirrors_worker_spec.rb

 require 'rails_helper'
 
 describe UpdateAllMirrorsWorker do
-  before do
-    allow_any_instance_of(Gitlab::ExclusiveLease)
-      .to receive(:try_obtain).and_return(true)
-  end
+  before { allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(true) }
 
   describe '#perform' do
-    project_count_with_time = { DateTime.now.beginning_of_hour + 15.minutes => 1,
-                                DateTime.now.beginning_of_hour => 2,
-                                DateTime.now.beginning_of_day => 3
-                              }
-
+    let(:worker) { described_class.new }
     let!(:mirror1) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::FIFTEEN) }
     let!(:mirror2) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::HOURLY) }
     let!(:mirror3) { create(:empty_project, :mirror, sync_time: Gitlab::Mirror::DAILY) }
-    let(:mirrors) { Project.mirror.where(sync_time: Gitlab::Mirror.sync_times) }
+    let!(:mirror4) { create(:empty_project, :mirror) }
 
     it 'fails stuck mirrors' do
-      worker = described_class.new
-
       expect(worker).to receive(:fail_stuck_mirrors!)
 
       worker.perform
     end
 
-    project_count_with_time.each do |time, project_count|
-      describe "at #{time}" do
-        before do
-          allow(DateTime).to receive(:now).and_return(time)
-        end
-
-        it 'enqueues a job on mirrored Projects' do
-          worker = described_class.new
-
-          expect(mirrors.count).to eq(project_count)
+    describe 'sync_time' do
+      def expect_worker_to_update_mirrors(mirrors)
         mirrors.each do |mirror|
           expect(worker).to receive(:rand).with((mirror.sync_time / 2).minutes).and_return(mirror.sync_time / 2)
           expect(RepositoryUpdateMirrorDispatchWorker).to receive(:perform_in).with(mirror.sync_time / 2, mirror.id)
         end
+      end
+
+      def setup(time)
+        Timecop.freeze(time)
+        mirror4.update_attributes(mirror_last_successful_update_at: time - (Gitlab::Mirror::DAILY + 5).minutes)
+      end
+
+      describe 'fifteen' do
+        let(:mirrors) { [mirror1, mirror4] }
+
+        before { setup(DateTime.now.beginning_of_hour + 15.minutes) }
+
+        it 'enqueues a job on mirrored projects' do
+          expect_worker_to_update_mirrors(mirrors)
 
           worker.perform
         end
       end
+
+      describe 'hourly' do
+        let(:mirrors) { [mirror1, mirror2, mirror4] }
+
+        before { setup(DateTime.now.beginning_of_hour) }
+
+        it 'enqueues a job on mirrored projects' do
+          expect_worker_to_update_mirrors(mirrors)
+
+          worker.perform
+        end
+      end
+
+      describe 'daily' do
+        let(:mirrors) { [mirror1, mirror2, mirror3, mirror4] }
+
+        before { setup(DateTime.now.beginning_of_day) }
+
+        it 'enqueues a job on mirrored projects' do
+          expect_worker_to_update_mirrors(mirrors)
+
+          worker.perform
+        end
+      end
+
+      after { Timecop.return }
     end
 
     it 'does not execute if cannot get the lease' do
-      allow_any_instance_of(Gitlab::ExclusiveLease)
-        .to receive(:try_obtain).and_return(false)
+      allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false)
 
-      worker = described_class.new
       create(:empty_project, :mirror)
 
       expect(worker).not_to receive(:fail_stuck_mirrors!)

spec/workers/update_all_remote_mirrors_worker.rb

 require 'rails_helper'
 
 describe UpdateAllRemoteMirrorsWorker do
-  describe "#perform" do
-    project_count_with_time = { DateTime.now.beginning_of_hour + 15.minutes => 1,
-                                DateTime.now.beginning_of_hour => 2,
-                                DateTime.now.beginning_of_day => 3
-                              }
+  let(:worker) { described_class.new }
 
+  describe "#perform" do
     let!(:mirror1) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::FIFTEEN) }
     let!(:mirror2) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::HOURLY) }
     let!(:mirror3) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::DAILY) }
-    let(:mirrors) { RemoteMirror.where(sync_time: Gitlab::Mirror.sync_times) }
+    let!(:mirror4) { create(:project, :remote_mirror, sync_time: Gitlab::Mirror::DAILY) }
 
     it 'fails stuck mirrors' do
-      worker = described_class.new
-
       expect(worker).to receive(:fail_stuck_mirrors!)
 
       worker.perform
     end
 
-    project_count_with_time.each do |time, project_count|
-      describe "at #{time}" do
-        before do
-          allow(DateTime).to receive(:now).and_return(time)
+    describe 'sync time' do
+      def expect_worker_to_update_mirrors(mirrors)
+        mirrors.each do |mirror|
+          expect(RepositoryUpdateRemoteMirrorWorker).to receive(:perform_async).with(mirror.id)
+        end
       end
 
-        it 'enqueues a job on mirrored Projects' do
-          worker = described_class.new
+      def setup(time)
+        Timecop.freeze(time)
+        mirror4.remote_mirrors.first.update_attributes(last_successful_update_at: time - (Gitlab::Mirror::DAILY + 5).minutes)
+      end
 
-          expect(mirrors.count).to eq(project_count)
-          mirrors.each do |mirror|
-            expect(RepositoryUpdateRemoteMirrorWorker).to receive(:perform_async).with(mirror.id)
+      describe 'fifteen' do
+        let(:mirrors) { [mirror1, mirror4] }
+
+        before { setup(DateTime.now.beginning_of_hour + 15.minutes) }
+
+        it 'enqueues a job on remote mirrored projects' do
+          expect_worker_to_update_mirrors(mirrors)
+
+          worker.perform
         end
+      end
+
+      describe "hourly" do
+        let(:mirrors) { [mirror1, mirror2, mirror4] }
+
+        before { setup(DateTime.now.beginning_of_hour) }
+
+        it 'enqueues a job on remote mirrored projects' do
+          expect_worker_to_update_mirrors(mirrors)
+
+          worker.perform
+        end
+      end
+
+      describe "daily" do
+        let(:mirrors) { [mirror1, mirror2, mirror3, mirror4] }
+
+        before { setup(DateTime.now.beginning_of_day) }
+
+        it 'enqueues a job on remote mirrored projects' do
+          expect_worker_to_update_mirrors(mirrors)
 
           worker.perform
         end
       end
+
+      after { Timecop.return }
     end
   end
 end