Commit 2a07c88b authored by pbair's avatar pbair

Ensure min lease timeout for batched migrations

For batched migrations with short intervals between jobs, ensure a
minimum `ExclusiveLease` timeout value is always set, so the lease has
less chance of expiring before the job is finished.

Also increase the lease timeout for migrations with longer intervals.
parent 2a034467
...@@ -8,6 +8,9 @@ module Database ...@@ -8,6 +8,9 @@ module Database
feature_category :database feature_category :database
idempotent! idempotent!
LEASE_TIMEOUT_MULTIPLIER = 3
MINIMUM_LEASE_TIMEOUT = 10.minutes.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
...@@ -33,14 +36,19 @@ module Database ...@@ -33,14 +36,19 @@ module Database
Gitlab::Database::BackgroundMigration::BatchedMigrationRunner.new.run_migration_job(active_migration) Gitlab::Database::BackgroundMigration::BatchedMigrationRunner.new.run_migration_job(active_migration)
end end
def with_exclusive_lease(timeout) def with_exclusive_lease(interval)
lease = Gitlab::ExclusiveLease.new(lease_key, timeout: timeout * 2) timeout = max(interval * LEASE_TIMEOUT_MULTIPLIER, MINIMUM_LEASE_TIMEOUT)
lease = Gitlab::ExclusiveLease.new(lease_key, timeout: timeout)
yield if lease.try_obtain yield if lease.try_obtain
ensure ensure
lease&.cancel lease&.cancel
end end
def max(left, right)
left >= right ? left : right
end
def lease_key def lease_key
self.class.name.demodulize.underscore self.class.name.demodulize.underscore
end end
......
...@@ -36,8 +36,10 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi ...@@ -36,8 +36,10 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi
end end
context 'when active migrations exist' do context 'when active migrations exist' do
let(:job_interval) { 5.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: 2.minutes) } let(:migration) { build(:batched_background_migration, :active, interval: job_interval) }
before do before do
allow(Gitlab::Database::BackgroundMigration::BatchedMigration).to receive(:active_migration) allow(Gitlab::Database::BackgroundMigration::BatchedMigration).to receive(:active_migration)
...@@ -49,7 +51,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi ...@@ -49,7 +51,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi
context 'when the reloaded migration is no longer active' do context 'when the reloaded migration is no longer active' do
it 'does not run the migration' do it 'does not run the migration' do
expect_to_obtain_exclusive_lease(lease_key, timeout: 4.minutes) expect_to_obtain_exclusive_lease(lease_key, timeout: lease_timeout)
expect(migration).to receive(:reload) expect(migration).to receive(:reload)
expect(migration).to receive(:active?).and_return(false) expect(migration).to receive(:active?).and_return(false)
...@@ -62,7 +64,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi ...@@ -62,7 +64,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi
context 'when the interval has not elapsed' do context 'when the interval has not elapsed' do
it 'does not run the migration' do it 'does not run the migration' do
expect_to_obtain_exclusive_lease(lease_key, timeout: 4.minutes) 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?).and_return(false)
...@@ -74,7 +76,24 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi ...@@ -74,7 +76,24 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi
context 'when the reloaded migration is still active and the interval has elapsed' do context 'when the reloaded migration is still active and the interval has elapsed' do
it 'runs the migration' do it 'runs the migration' do
expect_to_obtain_exclusive_lease(lease_key, timeout: 4.minutes) expect_to_obtain_exclusive_lease(lease_key, timeout: lease_timeout)
expect_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |instance|
expect(instance).to receive(:run_migration_job).with(migration)
end
expect(worker).to receive(:run_active_migration).and_call_original
worker.perform
end
end
context 'when the calculated timeout is less than the minimum allowed' do
let(:minimum_timeout) { described_class::MINIMUM_LEASE_TIMEOUT }
let(:job_interval) { 2.minutes }
it 'sets the lease timeout to the minimum value' do
expect_to_obtain_exclusive_lease(lease_key, timeout: minimum_timeout)
expect_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |instance| expect_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |instance|
expect(instance).to receive(:run_migration_job).with(migration) expect(instance).to receive(:run_migration_job).with(migration)
...@@ -87,7 +106,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi ...@@ -87,7 +106,7 @@ RSpec.describe Database::BatchedBackgroundMigrationWorker, '#perform', :clean_gi
end end
it 'always cleans up the exclusive lease' do it 'always cleans up the exclusive lease' do
lease = stub_exclusive_lease_taken(lease_key, timeout: 4.minutes) lease = stub_exclusive_lease_taken(lease_key, timeout: lease_timeout)
expect(lease).to receive(:try_obtain).and_return(true) expect(lease).to receive(:try_obtain).and_return(true)
......
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