Merge branch 'add-lower-bound-to-pull-mirror-scheduling' into 'master'

Adds lower bound to pull mirror scheduling feature

See merge request !2366

app/models/ee/application_setting.rb

@@ -16,7 +16,7 @@ module EE
 
       validates :mirror_max_delay,
                 presence: true,
-                numericality: { allow_nil: true, only_integer: true, greater_than: 0 }
+                numericality: { allow_nil: true, only_integer: true, greater_than: :mirror_max_delay_in_minutes }
 
       validates :mirror_max_capacity,
                 presence: true,
@@ -50,6 +50,10 @@ module EE
 
     private
 
+    def mirror_max_delay_in_minutes
+      ::Gitlab::Mirror.min_delay_upper_bound / 60
+    end
+
     def mirror_capacity_threshold_less_than
       return unless mirror_max_capacity && mirror_capacity_threshold
 

app/models/project_mirror_data.rb

@@ -27,6 +27,7 @@ class ProjectMirrorData < ActiveRecord::Base
     timestamp = Time.now
     retry_factor = [1, self.retry_count].max
     delay = [base_delay(timestamp) * retry_factor, Gitlab::Mirror.max_delay].min
+    delay = [delay, Gitlab::Mirror.min_delay].max
 
     self.next_execution_timestamp = timestamp + delay
   end

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

@@ -3,7 +3,7 @@
     %legend Repository mirror settings
     .form-group
       = f.label :mirror_max_delay, class: 'control-label col-sm-2' do
-        Maximum delay (Hours)
+        Maximum delay (Minutes)
       .col-sm-10
         = f.number_field :mirror_max_delay, class: 'form-control', min: 0
         %span.help-block#mirror_max_delay_help_block

changelogs/unreleased-ee/add-lower-bound-to-pull-mirror-scheduling.yml

+---
+title: Adds lower bound to pull mirror scheduling feature
+merge_request: 2366
+author:

config/initializers/1_settings.rb

@@ -265,7 +265,7 @@ Settings.gitlab['default_projects_features'] ||= {}
 Settings.gitlab['webhook_timeout'] ||= 10
 Settings.gitlab['max_attachment_size'] ||= 10
 Settings.gitlab['session_expire_delay'] ||= 10080
-Settings.gitlab['mirror_max_delay'] ||= 5
+Settings.gitlab['mirror_max_delay'] ||= 300
 Settings.gitlab['mirror_max_capacity'] ||= 30
 Settings.gitlab['mirror_capacity_threshold'] ||= 15
 Settings.gitlab.default_projects_features['issues']             = true if Settings.gitlab.default_projects_features['issues'].nil?

db/migrate/20170719182937_convert_max_mirror_delay_to_minutes_in_application_settings.rb

+class ConvertMaxMirrorDelayToMinutesInApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    change_column_default :application_settings, :mirror_max_delay, 300
+    execute 'UPDATE application_settings SET mirror_max_delay = COALESCE(mirror_max_delay, 5) * 60'
+  end
+
+  def down
+    change_column_default :application_settings, :mirror_max_delay, 5
+    execute 'UPDATE application_settings SET mirror_max_delay = COALESCE(mirror_max_delay, 300) / 60'
+  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: 20170717150329) do
+ActiveRecord::Schema.define(version: 20170719182937) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -137,7 +137,7 @@ ActiveRecord::Schema.define(version: 20170717150329) do
     t.string "clientside_sentry_dsn"
     t.boolean "prometheus_metrics_enabled", default: false, null: false
     t.boolean "check_namespace_plan", default: false, null: false
-    t.integer "mirror_max_delay", default: 5, null: false
+    t.integer "mirror_max_delay", default: 300, null: false
     t.integer "mirror_max_capacity", default: 100, null: false
     t.integer "mirror_capacity_threshold", default: 50, null: false
     t.boolean "authorized_keys_enabled", default: true, null: false

lib/gitlab/mirror.rb

@@ -5,7 +5,8 @@ module Gitlab
     # Runs scheduler every minute
     SCHEDULER_CRON = '* * * * *'.freeze
     PULL_CAPACITY_KEY = 'MIRROR_PULL_CAPACITY'.freeze
-    UPPER_JITTER = 1.minute
+    JITTER = 1.minute
+    MIN_DELAY = 15.minutes
 
     class << self
       def configure_cron_job!
@@ -49,7 +50,15 @@ module Gitlab
       end
 
       def max_delay
-        current_application_settings.mirror_max_delay.hours + rand(UPPER_JITTER)
+        current_application_settings.mirror_max_delay.minutes + rand(JITTER)
+      end
+
+      def min_delay_upper_bound
+        MIN_DELAY + JITTER
+      end
+
+      def min_delay
+        MIN_DELAY + rand(JITTER)
       end
 
       def max_capacity

spec/lib/gitlab/mirror_spec.rb

@@ -157,4 +157,10 @@ describe Gitlab::Mirror do
       expect(described_class.max_delay).to be_within(1.minute).of(5.hours)
     end
   end
+
+  describe '#min_delay' do
+    it 'returns min delay with some jitter' do
+      expect(described_class.min_delay).to be_within(1.minute).of(15.minutes)
+    end
+  end
 end

spec/migrations/update_authorized_keys_file_spec.rb

@@ -97,7 +97,7 @@ describe UpdateAuthorizedKeysFile, :migration do
             ActiveRecord::Base.connection.change_column_null :application_settings, :authorized_keys_enabled, true
             ActiveRecord::Base.connection.change_column :application_settings, :authorized_keys_enabled, :boolean, default: nil
 
-            ApplicationSetting.first.update(authorized_keys_enabled: nil)
+            ApplicationSetting.first.update(authorized_keys_enabled: nil, mirror_max_delay: 300)
           end
 
           it { is_expected.to be_truthy }
@@ -105,7 +105,7 @@ describe UpdateAuthorizedKeysFile, :migration do
 
         context 'when authorized_keys_enabled is explicitly false' do
           before do
-            ApplicationSetting.first.update!(authorized_keys_enabled: false)
+            ApplicationSetting.first.update!(authorized_keys_enabled: false, mirror_max_delay: 300)
           end
 
           it { is_expected.to be_falsey }

spec/models/application_setting_spec.rb

@@ -21,25 +21,6 @@ describe ApplicationSetting, models: true do
     it { is_expected.to allow_value(https).for(:after_sign_out_path) }
     it { is_expected.not_to allow_value(ftp).for(:after_sign_out_path) }
 
-    it { is_expected.to allow_value(10).for(:mirror_max_delay) }
-    it { is_expected.not_to allow_value(nil).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(-1).for(:mirror_max_delay) }
-
-    it { is_expected.to allow_value(10).for(:mirror_max_capacity) }
-    it { is_expected.not_to allow_value(nil).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(-1).for(:mirror_max_capacity) }
-
-    it { is_expected.to allow_value(10).for(:mirror_capacity_threshold) }
-    it { is_expected.not_to allow_value(nil).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(-1).for(:mirror_capacity_threshold) }
-    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
         allow(Devise).to receive(:omniauth_providers).and_return([:github])

spec/models/ee/application_setting_spec.rb

@@ -3,6 +3,28 @@ require 'spec_helper'
 describe ApplicationSetting do
   let(:setting) { described_class.create_from_defaults }
 
+  describe 'validations' do
+    it { is_expected.to allow_value(100).for(:mirror_max_delay) }
+    it { is_expected.not_to allow_value(nil).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(-1).for(:mirror_max_delay) }
+    it { is_expected.not_to allow_value((Gitlab::Mirror::MIN_DELAY - 1.minute) / 60).for(:mirror_max_delay) }
+
+    it { is_expected.to allow_value(10).for(:mirror_max_capacity) }
+    it { is_expected.not_to allow_value(nil).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(-1).for(:mirror_max_capacity) }
+
+    it { is_expected.to allow_value(10).for(:mirror_capacity_threshold) }
+    it { is_expected.not_to allow_value(nil).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(-1).for(:mirror_capacity_threshold) }
+    it { is_expected.not_to allow_value(subject.mirror_max_capacity + 1).for(:mirror_capacity_threshold) }
+  end
+
   describe '#should_check_namespace_plan?' do
     before do
       stub_application_setting(check_namespace_plan: check_namespace_plan_column)

spec/models/project_mirror_data_spec.rb

@@ -80,31 +80,60 @@ describe ProjectMirrorData, type: :model do
       end
     end
 
-    context 'when base delay is higher than mirror_max_delay' do
-      let!(:upper_jitter) { 30.seconds }
-      let(:max_timestamp) { timestamp + current_application_settings.mirror_max_delay.hours }
+    context 'when boundaries are surpassed' do
+      let!(:mirror_jitter) { 30.seconds }
 
-      before do
-        allow_any_instance_of(Gitlab::Mirror).to receive(:rand).and_return(upper_jitter)
-        mirror_data.last_update_started_at = timestamp - 1.hour
-      end
+      context 'when base delay is lower than mirror min_delay' do
+        before do
+          allow_any_instance_of(Gitlab::Mirror).to receive(:rand).and_return(mirror_jitter)
+          mirror_data.last_update_started_at = timestamp - 1.second
+        end
 
-      context 'when reseting retry count' do
-        it 'applies transition successfully' do
-          expect do
-            mirror_data.set_next_execution_timestamp!
-          end.to change { mirror_data.next_execution_timestamp }.to be_within(interval).of(max_timestamp + upper_jitter)
+        context 'when resetting retry count' do
+          it 'applies transition successfully' do
+            expect do
+              mirror_data.set_next_execution_timestamp!
+            end.to change { mirror_data.next_execution_timestamp }.to be_within(interval).of(timestamp + 15.minutes)
+          end
+        end
+
+        context 'when incrementing retry count' do
+          it 'applies transition successfully' do
+            mirror_data.retry_count = 2
+            mirror_data.increment_retry_count!
+
+            expect do
+              mirror_data.set_next_execution_timestamp!
+            end.to change { mirror_data.next_execution_timestamp }.to be_within(interval).of(timestamp + 15.minutes)
+          end
         end
       end
 
-      context 'when incrementing retry count' do
-        it 'applies transition successfully' do
-          mirror_data.retry_count = 2
-          mirror_data.increment_retry_count!
+      context 'when base delay is higher than mirror_max_delay' do
+        let(:max_timestamp) { timestamp + current_application_settings.mirror_max_delay.minutes }
 
-          expect do
-            mirror_data.set_next_execution_timestamp!
-          end.to change { mirror_data.next_execution_timestamp }.to be_within(interval).of(max_timestamp + upper_jitter)
+        before do
+          allow_any_instance_of(Gitlab::Mirror).to receive(:rand).and_return(mirror_jitter)
+          mirror_data.last_update_started_at = timestamp - 1.hour
+        end
+
+        context 'when resetting retry count' do
+          it 'applies transition successfully' do
+            expect do
+              mirror_data.set_next_execution_timestamp!
+            end.to change { mirror_data.next_execution_timestamp }.to be_within(interval).of(max_timestamp + mirror_jitter)
+          end
+        end
+
+        context 'when incrementing retry count' do
+          it 'applies transition successfully' do
+            mirror_data.retry_count = 2
+            mirror_data.increment_retry_count!
+
+            expect do
+              mirror_data.set_next_execution_timestamp!
+            end.to change { mirror_data.next_execution_timestamp }.to be_within(interval).of(max_timestamp + mirror_jitter)
+          end
         end
       end
     end