Merge branch 'delay-background-migrations' into 'master'

Run background migrations with a minimum interval

Closes #41624

See merge request gitlab-org/gitlab-ce!16230

8 files changed, 110 insertions, 11 deletions

diff --git a/app/workers/background_migration_worker.rb b/app/workers/background_migration_worker.rb
index aeb3bc019b9..376703f6319 100644
--- a/app/workers/background_migration_worker.rb
+++ b/app/workers/background_migration_worker.rb
@@ -1,10 +1,53 @@
 class BackgroundMigrationWorker
   include ApplicationWorker
 
+  # The minimum amount of time between processing two jobs of the same migration
+  # class.
+  #
+  # This interval is set to 5 minutes so autovacuuming and other maintenance
+  # related tasks have plenty of time to clean up after a migration has been
+  # performed.
+  MIN_INTERVAL = 5.minutes.to_i
+
   # Performs the background migration.
   #
   # See Gitlab::BackgroundMigration.perform for more information.
+  #
+  # class_name - The class name of the background migration to run.
+  # arguments - The arguments to pass to the migration class.
   def perform(class_name, arguments = [])
-    Gitlab::BackgroundMigration.perform(class_name, arguments)
+    should_perform, ttl = perform_and_ttl(class_name)
+
+    if should_perform
+      Gitlab::BackgroundMigration.perform(class_name, arguments)
+    else
+      # If the lease could not be obtained this means either another process is
+      # running a migration of this class or we ran one recently. In this case
+      # we'll reschedule the job in such a way that it is picked up again around
+      # the time the lease expires.
+      self.class.perform_in(ttl || MIN_INTERVAL, class_name, arguments)
+    end
+  end
+
+  def perform_and_ttl(class_name)
+    if always_perform?
+      # In test environments `perform_in` will run right away. This can then
+      # lead to stack level errors in the above `#perform`. To work around this
+      # we'll just perform the migration right away in the test environment.
+      [true, nil]
+    else
+      lease = lease_for(class_name)
+
+      [lease.try_obtain, lease.ttl]
+    end
+  end
+
+  def lease_for(class_name)
+    Gitlab::ExclusiveLease
+      .new("#{self.class.name}:#{class_name}", timeout: MIN_INTERVAL)
+  end
+
+  def always_perform?
+    Rails.env.test?
   end
 end
diff --git a/changelogs/unreleased/delay-background-migrations.yml b/changelogs/unreleased/delay-background-migrations.yml
new file mode 100644
index 00000000000..aa12591e7d2
--- /dev/null
+++ b/changelogs/unreleased/delay-background-migrations.yml
@@ -0,0 +1,5 @@
+---
+title: Run background migrations with a minimum interval
+merge_request:
+author:
+type: changed
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 33171f83692..7b35c24d153 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -842,6 +842,12 @@ into similar problems in the future (e.g. when new tables are created).
       def queue_background_migration_jobs_by_range_at_intervals(model_class, job_class_name, delay_interval, batch_size: BACKGROUND_MIGRATION_BATCH_SIZE)
         raise "#{model_class} does not have an ID to use for batch ranges" unless model_class.column_names.include?('id')
 
+        # To not overload the worker too much we enforce a minimum interval both
+        # when scheduling and performing jobs.
+        if delay_interval < BackgroundMigrationWorker::MIN_INTERVAL
+          delay_interval = BackgroundMigrationWorker::MIN_INTERVAL
+        end
+
         model_class.each_batch(of: batch_size) do |relation, index|
           start_id, end_id = relation.pluck('MIN(id), MAX(id)').first
 
diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb
index 3f7b42456af..dbb8f317afe 100644
--- a/lib/gitlab/exclusive_lease.rb
+++ b/lib/gitlab/exclusive_lease.rb
@@ -71,5 +71,16 @@ module Gitlab
         redis.exists(@redis_shared_state_key)
       end
     end
+
+    # Returns the TTL of the Redis key.
+    #
+    # This method will return `nil` if no TTL could be obtained.
+    def ttl
+      Gitlab::Redis::SharedState.with do |redis|
+        ttl = redis.ttl(@redis_shared_state_key)
+
+        ttl if ttl.positive?
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb
index 7727a1d81b1..43761c2fe0c 100644
--- a/spec/lib/gitlab/database/migration_helpers_spec.rb
+++ b/spec/lib/gitlab/database/migration_helpers_spec.rb
@@ -1006,12 +1006,12 @@ describe Gitlab::Database::MigrationHelpers do
       context 'with batch_size option' do
         it 'queues jobs correctly' do
           Sidekiq::Testing.fake! do
-            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.seconds, batch_size: 2)
+            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes, batch_size: 2)
 
             expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id2]])
-            expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.seconds.from_now.to_f)
+            expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.minutes.from_now.to_f)
             expect(BackgroundMigrationWorker.jobs[1]['args']).to eq(['FooJob', [id3, id3]])
-            expect(BackgroundMigrationWorker.jobs[1]['at']).to eq(20.seconds.from_now.to_f)
+            expect(BackgroundMigrationWorker.jobs[1]['at']).to eq(20.minutes.from_now.to_f)
           end
         end
       end
@@ -1019,10 +1019,10 @@ describe Gitlab::Database::MigrationHelpers do
       context 'without batch_size option' do
         it 'queues jobs correctly' do
           Sidekiq::Testing.fake! do
-            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.seconds)
+            model.queue_background_migration_jobs_by_range_at_intervals(User, 'FooJob', 10.minutes)
 
             expect(BackgroundMigrationWorker.jobs[0]['args']).to eq(['FooJob', [id1, id3]])
-            expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.seconds.from_now.to_f)
+            expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.minutes.from_now.to_f)
           end
         end
       end
diff --git a/spec/lib/gitlab/exclusive_lease_spec.rb b/spec/lib/gitlab/exclusive_lease_spec.rb
index 7322a326b01..6193e177668 100644
--- a/spec/lib/gitlab/exclusive_lease_spec.rb
+++ b/spec/lib/gitlab/exclusive_lease_spec.rb
@@ -73,4 +73,19 @@ describe Gitlab::ExclusiveLease, :clean_gitlab_redis_shared_state do
       described_class.new(key, timeout: 3600).try_obtain
     end
   end
+
+  describe '#ttl' do
+    it 'returns the TTL of the Redis key' do
+      lease = described_class.new('kittens', timeout: 100)
+      lease.try_obtain
+
+      expect(lease.ttl <= 100).to eq(true)
+    end
+
+    it 'returns nil when the lease does not exist' do
+      lease = described_class.new('kittens', timeout: 10)
+
+      expect(lease.ttl).to be_nil
+    end
+  end
 end
diff --git a/spec/migrations/normalize_ldap_extern_uids_spec.rb b/spec/migrations/normalize_ldap_extern_uids_spec.rb
index 262d7742aaf..56a78f52802 100644
--- a/spec/migrations/normalize_ldap_extern_uids_spec.rb
+++ b/spec/migrations/normalize_ldap_extern_uids_spec.rb
@@ -27,11 +27,11 @@ describe NormalizeLdapExternUids, :migration, :sidekiq do
         migrate!
 
         expect(BackgroundMigrationWorker.jobs[0]['args']).to eq([described_class::MIGRATION, [1, 2]])
-        expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(10.seconds.from_now.to_f)
+        expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(5.minutes.from_now.to_f)
         expect(BackgroundMigrationWorker.jobs[1]['args']).to eq([described_class::MIGRATION, [3, 4]])
-        expect(BackgroundMigrationWorker.jobs[1]['at']).to eq(20.seconds.from_now.to_f)
+        expect(BackgroundMigrationWorker.jobs[1]['at']).to eq(10.minutes.from_now.to_f)
         expect(BackgroundMigrationWorker.jobs[2]['args']).to eq([described_class::MIGRATION, [5, 5]])
-        expect(BackgroundMigrationWorker.jobs[2]['at']).to eq(30.seconds.from_now.to_f)
+        expect(BackgroundMigrationWorker.jobs[2]['at']).to eq(15.minutes.from_now.to_f)
         expect(BackgroundMigrationWorker.jobs.size).to eq 3
       end
     end
diff --git a/spec/workers/background_migration_worker_spec.rb b/spec/workers/background_migration_worker_spec.rb
index 1c54cf55fa0..d67e7698635 100644
--- a/spec/workers/background_migration_worker_spec.rb
+++ b/spec/workers/background_migration_worker_spec.rb
@@ -1,13 +1,32 @@
 require 'spec_helper'
 
-describe BackgroundMigrationWorker, :sidekiq do
+describe BackgroundMigrationWorker, :sidekiq, :clean_gitlab_redis_shared_state do
+  let(:worker) { described_class.new }
+
   describe '.perform' do
     it 'performs a background migration' do
       expect(Gitlab::BackgroundMigration)
         .to receive(:perform)
         .with('Foo', [10, 20])
 
-      described_class.new.perform('Foo', [10, 20])
+      worker.perform('Foo', [10, 20])
+    end
+
+    it 'reschedules a migration if it was performed recently' do
+      expect(worker)
+        .to receive(:always_perform?)
+        .and_return(false)
+
+      worker.lease_for('Foo').try_obtain
+
+      expect(Gitlab::BackgroundMigration)
+        .not_to receive(:perform)
+
+      expect(described_class)
+        .to receive(:perform_in)
+        .with(a_kind_of(Numeric), 'Foo', [10, 20])
+
+      worker.perform('Foo', [10, 20])
     end
   end
 end