Commit c48ba55b authored by Tiago Botelho's avatar Tiago Botelho

Adds lower bound to pull mirror scheduling feature

parent f6df3808
......@@ -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
......
......@@ -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
......
......@@ -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
......
---
title: Adds lower bound to pull mirror scheduling feature
merge_request: 2366
author:
......@@ -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?
......
......@@ -5,11 +5,11 @@ class ConvertMaxMirrorDelayToMinutesInApplicationSettings < ActiveRecord::Migrat
def up
change_column_default :application_settings, :mirror_max_delay, 300
execute 'UPDATE application_settings SET 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 = 5'
execute 'UPDATE application_settings SET mirror_max_delay = COALESCE(mirror_max_delay, 300) / 60'
end
end
......@@ -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
......
......@@ -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
......@@ -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 }
......
......@@ -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])
......
......@@ -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)
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment