adds dynamic threshold capacity checking to mirror worker

app/models/project.rb

@@ -53,7 +53,8 @@ class Project < ActiveRecord::Base
   after_create :create_project_feature, unless: :project_feature
   after_save :ensure_dir_exist, if: :namespace_id_changed?
   after_save :update_project_statistics, if: :namespace_id_changed?
-  after_save  :create_mirror_data, if:  ->(project) { project.mirror? && project.mirror_changed? }
+  after_save :create_mirror_data, if: ->(project) { project.mirror? && project.mirror_changed? }
+  after_save :destroy_mirror_data, if: ->(project) { !project.mirror? && project.mirror_changed? }
   # set last_activity_at to the same as created_at
   after_create :set_last_activity_at
@@ -251,7 +252,6 @@ class Project < ActiveRecord::Base
   add_authentication_token_field :runners_token
   before_save :ensure_runners_token
-  before_save :destroy_mirror_data, if: ->(project) { !project.mirror? && project.mirror_changed? }
   before_validation :mark_remote_mirrors_for_removal
   mount_uploader :avatar, AvatarUploader
@@ -362,7 +362,7 @@ class Project < ActiveRecord::Base
         project.mirror_last_update_at = timestamp
         project.mirror_last_successful_update_at = timestamp
-        project.mirror_data.apply_transition(timestamp)
+        project.mirror_data.apply_transition(timestamp: timestamp, reset_retry_count: true)
       end
       if current_application_settings.elasticsearch_indexing?
@@ -373,10 +373,9 @@ class Project < ActiveRecord::Base
     before_transition started: :failed do |project, _|
       if project.mirror?
         timestamp = Time.now
-        retry_count = project.mirror_data.retry_count + 1
         project.mirror_last_update_at = timestamp
-        project.mirror_data.apply_transition(timestamp, retry_count)
+        project.mirror_data.apply_transition(timestamp: timestamp)
       end
     end
   end

app/models/project_mirror_data.rb

@@ -8,19 +8,31 @@ class ProjectMirrorData < ActiveRecord::Base
   validates :project, presence: true
-  before_create :set_next_execution_ts!
-  def set_next_execution_ts!
-    self.next_execution_ts = Time.now
+  before_create :set_timestamps!
+  def set_timestamps!
+    timestamp = Time.now
+
+    self.next_execution_ts = timestamp
+    self.last_update_started_at = timestamp
   end
-  def apply_transition(timestamp, current_retry = 0)
-    base = (BACKOFF_PERIOD + rand(JITTER)) * (timestamp - last_update_started_at).seconds
-    retry_factor = current_retry == 0 ? 1 : current_retry
-    update(
-      retry_count: current_retry,
-      next_execution_ts: timestamp + [base * retry_factor, current_application_settings.mirror_max_delay.hours].min
-    )
+  def apply_transition(timestamp:, reset_retry_count: false)
+    base = (BACKOFF_PERIOD + rand(JITTER)) * (timestamp - self.last_update_started_at).seconds
+
+    reset_retry_count ? reset_retry_count! : increment_retry_count!
+    retry_factor = self.retry_count == 0 ? 1 : self.retry_count
+
+    self.next_execution_ts = timestamp + [base * retry_factor, current_application_settings.mirror_max_delay.hours].min
+
+    save
+  end
+
+  def increment_retry_count!
+    self.retry_count += 1
+  end
+  def reset_retry_count!
+    self.retry_count = 0
   end
 end

app/views/admin/application_settings/_form.html.haml

@@ -74,15 +74,15 @@
     .form-group
       = f.label :mirror_max_delay, class: 'control-label col-sm-2'
       .col-sm-10
-        = f.select :mirror_max_delay, value: f.object.mirror_max_delay, class: 'form-control', min: 0
+        = f.number_field :mirror_max_delay, value: f.object.mirror_max_delay, class: 'form-control', min: 0
     .form-group
       = f.label :mirror_max_capacity, class: 'control-label col-sm-2'
       .col-sm-10
-        = f.select :mirror_max_capacity, value: f.object.mirror_max_capacity, class: 'form-control', min: 0
+        = f.number_field :mirror_max_capacity, value: f.object.mirror_max_capacity, class: 'form-control', min: 0
     .form-group
       = f.label :mirror_capacity_threshold, class: 'control-label col-sm-2'
       .col-sm-10
-        = f.select :mirror_capacity_threshold, value: f.object.mirror_capacity_threshold, class: 'form-control', min: 0
+        = f.number_field :mirror_capacity_threshold, value: f.object.mirror_capacity_threshold, class: 'form-control', min: 0
     .form-group
       = f.label :session_expire_delay, 'Session duration (minutes)', class: 'control-label col-sm-2'
       .col-sm-10

app/workers/repository_update_mirror_worker.rb

@@ -29,5 +29,7 @@ class RepositoryUpdateMirrorWorker
     project.mark_import_as_failed(ex.message)
   rescue => ex
     raise UpdateMirrorError, "#{ex.class}: #{ex.message}" if project
+  ensure
+    UpdateAllMirrorsWorker.perform_async if Gitlab::Mirror.threshold_reached?
   end
 end

app/workers/update_all_mirrors_worker.rb

@@ -3,7 +3,7 @@ class UpdateAllMirrorsWorker
   include CronjobQueue
   LEASE_TIMEOUT = 5.minutes
-  LEASE_KEY = 'update_all_mirrors'
+  LEASE_KEY = 'update_all_mirrors'.freeze
   def perform
     # This worker requires updating the database state, which we can't
@@ -16,7 +16,7 @@ class UpdateAllMirrorsWorker
     fail_stuck_mirrors!
     return if Gitlab::Mirror.max_mirror_capacity_reached?
-    project.mirrors_to_sync.find_each(batch_size: 200, &:schedule_mirror_update)
+    Project.mirrors_to_sync.find_each(batch_size: 200, &:schedule_mirror_update)
     cancel_lease(lease_uuid)
   end

db/migrate/20170509153720_create_project_mirror_data.rb

 class CreateProjectMirrorData < ActiveRecord::Migration
-  def change
-    create_table :project_mirror_data do |t|
-      t.references :project
-      t.datetime :last_update_started_at
-      t.datetime :next_execution_ts
-      t.integer  :retry_count, default: 0
-
-      t.timestamps null: false
-    end
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    execute("CREATE TABLE project_mirror_data AS (SELECT id AS project_id, NOW() AS last_update_started_at, NOW() AS next_execution_ts, NOW() AS created_at, NOW() AS updated_at FROM projects WHERE mirror is TRUE);")
+
+    add_column :project_mirror_data, :id, :primary_key
+    add_column_with_default :project_mirror_data, :retry_count, :integer, default: 0
+    add_concurrent_foreign_key :project_mirror_data, :projects, column: :project_id, on_delete: :cascade
+    add_concurrent_index :project_mirror_data, [:project_id]
+  end
+
+  def down
+    drop_table :project_mirror_data if table_exists?(:project_mirror_data)
   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: 20170511101000) do
+ActiveRecord::Schema.define(version: 20170515093334) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1078,11 +1078,13 @@ ActiveRecord::Schema.define(version: 20170511101000) do
     t.integer "project_id"
     t.datetime "last_update_started_at"
     t.datetime "next_execution_ts"
-    t.integer "retry_count", default: 0
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime "created_at"
+    t.datetime "updated_at"
+    t.integer "retry_count", default: 0, null: false
   end
+  add_index "project_mirror_data", ["project_id"], name: "index_project_mirror_data_on_project_id", using: :btree
+
   create_table "project_statistics", force: :cascade do |t|
     t.integer "project_id", null: false
     t.integer "namespace_id", null: false
@@ -1653,6 +1655,7 @@ ActiveRecord::Schema.define(version: 20170511101000) do
   add_foreign_key "personal_access_tokens", "users"
   add_foreign_key "project_authorizations", "projects", on_delete: :cascade
   add_foreign_key "project_authorizations", "users", on_delete: :cascade
+  add_foreign_key "project_mirror_data", "projects", name: "fk_d1aad367d7", on_delete: :cascade
   add_foreign_key "project_statistics", "projects", on_delete: :cascade
   add_foreign_key "protected_branch_merge_access_levels", "namespaces", column: "group_id"
   add_foreign_key "protected_branch_merge_access_levels", "protected_branches"

lib/gitlab/mirror.rb

@@ -2,8 +2,8 @@ module Gitlab
   module Mirror
     include Gitlab::CurrentSettings
-    PULL_CAPACITY_KEY = 'MIRROR_PULL_CAPACITY'
-    SCHEDULER_CRON = '* * * * *'
+    PULL_CAPACITY_KEY = 'MIRROR_PULL_CAPACITY'.freeze
+    SCHEDULER_CRON = '* * * * *'.freeze
     class << self
       def update_all_mirrors_cron_job
@@ -38,6 +38,10 @@ module Gitlab
         available_capacity == 0
       end
+      def threshold_reached?
+        available_capacity >= current_application_settings.mirror_capacity_threshold
+      end
+
       def increment_capacity
         Gitlab::Redis.with { |redis| redis.incr(PULL_CAPACITY_KEY) }
       end

spec/controllers/projects/mirrors_controller_spec.rb

@@ -3,28 +3,28 @@ require 'spec_helper'
 describe Projects::MirrorsController do
   describe 'setting up a remote mirror' do
     context 'when the current project is a mirror' do
+      let(:project) { create(:project, :mirror, :import_finished) }
       before do
-        @project = create(:project, :mirror)
-        sign_in(@project.owner)
+        sign_in(project.owner)
       end
       it 'allows to create a remote mirror' do
         expect do
-          do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com' } })
+          do_put(project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => 'http://foo.com' } })
         end.to change { RemoteMirror.count }.to(1)
       end
       context 'when remote mirror has the same URL' do
         it 'does not allow to create the remote mirror' do
           expect do
-            do_put(@project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => @project.import_url } })
+            do_put(project, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => project.import_url } })
           end.not_to change { RemoteMirror.count }
         end
         context 'with disabled local mirror' do
           it 'allows to create a remote mirror' do
             expect do
-              do_put(@project, mirror: 0, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => @project.import_url } })
+              do_put(project, mirror: 0, remote_mirrors_attributes: { '0' => { 'enabled' => 1, 'url' => project.import_url } })
             end.to change { RemoteMirror.count }.to(1)
           end
         end

spec/factories/projects.rb

@@ -25,19 +25,16 @@ FactoryGirl.define do
     end
     trait :import_started do
-      import_url { generate(:url) }
       import_status :started
     end
     trait :import_finished do
-      import_started
       import_status :finished
     end
     trait :mirror do
-      import_started
-
       mirror true
+      import_url { generate(:url) }
       mirror_user_id { creator_id }
     end

spec/features/projects/mirror_spec.rb

@@ -13,9 +13,9 @@ feature 'Project mirror', feature: true do
     describe 'pressing "Update now"' do
       it 'returns with the project updating (job enqueued)' do
-        Sidekiq::Testing.fake! { click_link('Update Now') }
-        expect(page).to have_content('Updating')
+        expect(RepositoryUpdateMirrorWorker).to receive(:perform_async).with(project.id)
+        Sidekiq::Testing.fake! { click_link('Update Now') }
       end
     end
   end

spec/lib/gitlab/import_export/all_models.yml

@@ -259,6 +259,7 @@ project:
 - statistics
 - container_repositories
 - uploads
+- mirror_data
 award_emoji:
 - awardable
 - user

spec/lib/gitlab/mirror_spec.rb

@@ -8,16 +8,18 @@ describe Gitlab::Mirror do
     describe 'with jobs already running' do
       it 'creates a new cron job' do
-        expect_any_instance_of(Sidekiq::Cron::Job.find("update_all_mirrors_worker")).to receive(:destroy)
-        expect { described_class.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("udpate_all_mirrors_worker") }.from(nil).to(Sidekiq::Cron::Job)
-        expect(Sidekiq::Cron::Job.find("update_all_mirrors_worker").cron).to eq(cron)
+        described_class.configure_cron_job!
+        expect(subject).to receive(:destroy_cron_job!)
+        expect(Sidekiq::Cron::Job).to receive(:create)
+
+        expect { subject.configure_cron_job! }.to change { Sidekiq::Cron::Job.find("update_all_mirrors_worker") }
       end
     end
     describe 'without jobs already running' do
       before do
-        Sidekiq::Cron::Job.find("update_all_mirrors_worker").destroy
+        Sidekiq::Cron::Job.find("update_all_mirrors_worker")&.destroy
       end
       it 'creates update_all_mirrors_worker with cron of daily sync_time' do

spec/models/application_setting_spec.rb

@@ -24,26 +24,18 @@ describe ApplicationSetting, models: true do
     it { is_expected.to allow_value(10).for(:mirror_max_delay) }
     it { is_expected.not_to allow_value(0).for(:mirror_max_delay) }
     it { is_expected.not_to allow_value(1.0).for(:mirror_max_delay) }
-    it { is_expected.not_to allow_value(nil).for(:mirror_max_delay) }
     it { is_expected.not_to allow_value(-1).for(:mirror_max_delay) }
     it { is_expected.to allow_value(10).for(:mirror_max_capacity) }
     it { is_expected.not_to allow_value(0).for(:mirror_max_capacity) }
     it { is_expected.not_to allow_value(1.0).for(:mirror_max_capacity) }
-    it { is_expected.not_to allow_value(nil).for(:mirror_max_capacity) }
     it { is_expected.not_to allow_value(-1).for(:mirror_max_capacity) }
     it { is_expected.to allow_value(10).for(:mirror_capacity_threshold) }
     it { is_expected.not_to allow_value(0).for(:mirror_capacity_threshold) }
     it { is_expected.not_to allow_value(1.0).for(:mirror_capacity_threshold) }
-    it { is_expected.not_to allow_value(nil).for(:mirror_capacity_threshold) }
     it { is_expected.not_to allow_value(-1).for(:mirror_capacity_threshold) }
-
-    it 'is not allowed to be higher than mirror_max_capacity' do
-      allow(ApplicationSetting).to receive(:mirror_max_capacity).and_return(10)
-
-      is_expected.not_to allow_value(20).for(mirror_capacity_threshold)
-    end
+    it { is_expected.not_to allow_value(subject.mirror_max_capacity + 1).for(:mirror_capacity_threshold) }
     describe 'disabled_oauth_sign_in_sources validations' do
       before do

spec/workers/update_all_mirrors_worker_spec.rb

@@ -51,7 +51,7 @@ describe UpdateAllMirrorsWorker do
     end
     it 'transitions stuck mirrors to a failed state' do
-      project = create(:empty_project, :mirror, mirror_last_update_at: 12.hours.ago)
+      project = create(:empty_project, :mirror, :import_started, mirror_last_update_at: 12.hours.ago)
       fail_stuck_mirrors!
       project.reload
@@ -60,7 +60,7 @@ describe UpdateAllMirrorsWorker do
     end
     it 'updates the import_error message' do
-      project = create(:empty_project, :mirror, mirror_last_update_at: 12.hours.ago)
+      project = create(:empty_project, :mirror, :import_started, mirror_last_update_at: 12.hours.ago)
       fail_stuck_mirrors!
       project.reload