Commit fcd41d81 authored by pbair's avatar pbair

Allow small variance when running migration jobs

When running batched background migrations, allow a small variance in
the `interval_elapsed?` check, to prevent issues where the cron skips a
job execution due to missing the next execution window by only several
seconds.

Without the variance, a job scheduled to run every 2 minutes will often
run closer to every 3 minutes in practice.
parent f2b06d08
...@@ -10,6 +10,7 @@ module Database ...@@ -10,6 +10,7 @@ module Database
LEASE_TIMEOUT_MULTIPLIER = 3 LEASE_TIMEOUT_MULTIPLIER = 3
MINIMUM_LEASE_TIMEOUT = 10.minutes.freeze MINIMUM_LEASE_TIMEOUT = 10.minutes.freeze
INTERVAL_VARIANCE = 5.seconds.freeze
def perform def perform
return unless Feature.enabled?(:execute_batched_migrations_on_schedule, type: :ops) && active_migration return unless Feature.enabled?(:execute_batched_migrations_on_schedule, type: :ops) && active_migration
...@@ -22,7 +23,7 @@ module Database ...@@ -22,7 +23,7 @@ module Database
# models don't inherit from ApplicationRecord # models don't inherit from ApplicationRecord
active_migration.reload # rubocop:disable Cop/ActiveRecordAssociationReload active_migration.reload # rubocop:disable Cop/ActiveRecordAssociationReload
run_active_migration if active_migration.active? && active_migration.interval_elapsed? run_active_migration if active_migration.active? && active_migration.interval_elapsed?(variance: INTERVAL_VARIANCE)
end end
end end
......
...@@ -27,8 +27,11 @@ module Gitlab ...@@ -27,8 +27,11 @@ module Gitlab
active.queue_order.first active.queue_order.first
end end
def interval_elapsed? def interval_elapsed?(variance: 0)
last_job.nil? || last_job.created_at <= Time.current - interval return true unless last_job
interval_with_variance = interval - variance
last_job.created_at <= Time.current - interval_with_variance
end end
def create_batched_job!(min, max) def create_batched_job!(min, max)
......
...@@ -87,6 +87,34 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m ...@@ -87,6 +87,34 @@ RSpec.describe Gitlab::Database::BackgroundMigration::BatchedMigration, type: :m
end end
end end
end end
context 'when an interval variance is given' do
let(:variance) { 2.seconds }
context 'when the last job is less than an interval with variance old' do
it 'returns false' do
freeze_time do
create(:batched_background_migration_job,
batched_migration: batched_migration,
created_at: Time.current - 1.minute - 57.seconds)
expect(batched_migration.interval_elapsed?(variance: variance)).to eq(false)
end
end
end
context 'when the last job is more than an interval with variance old' do
it 'returns true' do
freeze_time do
create(:batched_background_migration_job,
batched_migration: batched_migration,
created_at: Time.current - 1.minute - 58.seconds)
expect(batched_migration.interval_elapsed?(variance: variance)).to eq(true)
end
end
end
end
end end
end end
......
...@@ -40,12 +40,13 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi ...@@ -40,12 +40,13 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi
let(:lease_timeout) { 15.minutes } let(:lease_timeout) { 15.minutes }
let(:lease_key) { 'batched_background_migration_worker' } let(:lease_key) { 'batched_background_migration_worker' }
let(:migration) { build(:batched_background_migration, :active, interval: job_interval) } let(:migration) { build(:batched_background_migration, :active, interval: job_interval) }
let(:interval_variance) { described_class::INTERVAL_VARIANCE }
before do before do
allow(Gitlab::Database::BackgroundMigration::BatchedMigration).to receive(:active_migration) allow(Gitlab::Database::BackgroundMigration::BatchedMigration).to receive(:active_migration)
.and_return(migration) .and_return(migration)
allow(migration).to receive(:interval_elapsed?).and_return(true) allow(migration).to receive(:interval_elapsed?).with(variance: interval_variance).and_return(true)
allow(migration).to receive(:reload) allow(migration).to receive(:reload)
end end
...@@ -66,7 +67,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi ...@@ -66,7 +67,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi
it 'does not run the migration' do it 'does not run the migration' do
expect_to_obtain_exclusive_lease(lease_key, timeout: lease_timeout) expect_to_obtain_exclusive_lease(lease_key, timeout: lease_timeout)
expect(migration).to receive(:interval_elapsed?).and_return(false) expect(migration).to receive(:interval_elapsed?).with(variance: interval_variance).and_return(false)
expect(worker).not_to receive(:run_active_migration) expect(worker).not_to receive(:run_active_migration)
......
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