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

Test sampled jobs from background migrations

See merge request gitlab-org/gitlab!78213

lib/gitlab/background_migration/job_coordinator.rb

@@ -50,34 +50,41 @@ module Gitlab
         Gitlab::Database::SharedModel.using_connection(connection, &block)
       end
 
-      def steal(steal_class, retry_dead_jobs: false)
-        with_shared_connection do
+      def pending_jobs(include_dead_jobs: false)
+        Enumerator.new do |y|
           queues = [
             Sidekiq::ScheduledSet.new,
             Sidekiq::Queue.new(self.queue)
           ]
 
-          if retry_dead_jobs
+          if include_dead_jobs
             queues << Sidekiq::RetrySet.new
             queues << Sidekiq::DeadSet.new
           end
 
           queues.each do |queue|
             queue.each do |job|
-              migration_class, migration_args = job.args
+              y << job if job.klass == worker_class.name
+            end
+          end
+        end
+      end
+
+      def steal(steal_class, retry_dead_jobs: false)
+        with_shared_connection do
+          pending_jobs(include_dead_jobs: retry_dead_jobs).each do |job|
+            migration_class, migration_args = job.args
 
-              next unless job.klass == worker_class.name
-              next unless migration_class == steal_class
-              next if block_given? && !(yield job)
+            next unless migration_class == steal_class
+            next if block_given? && !(yield job)
 
-              begin
-                perform(migration_class, migration_args) if job.delete
-              rescue Exception # rubocop:disable Lint/RescueException
-                worker_class # enqueue this migration again
-                  .perform_async(migration_class, migration_args)
+            begin
+              perform(migration_class, migration_args) if job.delete
+            rescue Exception # rubocop:disable Lint/RescueException
+              worker_class # enqueue this migration again
+                .perform_async(migration_class, migration_args)
 
-                raise
-              end
+              raise
             end
           end
         end

lib/gitlab/database/migrations/test_background_runner.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module Migrations
+      class TestBackgroundRunner
+        # TODO - build a rake task to call this method, and support it in the gitlab-com-database-testing project.
+        # Until then, we will inject a migration with a very high timestamp during database testing
+        # that calls this class to run jobs
+        # See https://gitlab.com/gitlab-org/database-team/gitlab-com-database-testing/-/issues/41 for details
+
+        def initialize
+          @job_coordinator = Gitlab::BackgroundMigration.coordinator_for_database(Gitlab::Database::MAIN_DATABASE_NAME)
+        end
+
+        def traditional_background_migrations
+          @job_coordinator.pending_jobs
+        end
+
+        def run_jobs(for_duration:)
+          jobs_to_run = traditional_background_migrations.group_by { |j| class_name_for_job(j) }
+          return if jobs_to_run.empty?
+
+          # without .to_f, we do integer division
+          # For example, 3.minutes / 2 == 1.minute whereas 3.minutes / 2.to_f == (1.minute + 30.seconds)
+          duration_per_migration_type = for_duration / jobs_to_run.count.to_f
+          jobs_to_run.each do |_migration_name, jobs|
+            run_until = duration_per_migration_type.from_now
+            jobs.shuffle.each do |j|
+              break if run_until <= Time.current
+
+              run_job(j)
+            end
+          end
+        end
+
+        private
+
+        def run_job(job)
+          Gitlab::BackgroundMigration.perform(job.args[0], job.args[1])
+        end
+
+        def class_name_for_job(job)
+          job.args[0]
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/job_coordinator_spec.rb

@@ -38,13 +38,67 @@ RSpec.describe Gitlab::BackgroundMigration::JobCoordinator do
     end
   end
 
+  describe '#pending_jobs' do
+    context 'when there are enqueued jobs' do
+      let(:queue) do
+        [
+          instance_double(Sidekiq::JobRecord, args: [1, 'queue'], klass: worker_class.name),
+          instance_double(Sidekiq::JobRecord, args: [2, 'queue'], klass: worker_class.name)
+        ]
+      end
+
+      let(:queue_incorrect_job_class) do
+        [
+          instance_double(Sidekiq::JobRecord, args: [1, 'queue'], klass: 'SomeOtherClass')
+        ]
+      end
+
+      let(:scheduled_set) do
+        [instance_double(Sidekiq::JobRecord, args: [3, 'scheduled'], klass: worker_class.name)]
+      end
+
+      let(:retry_set) do
+        [instance_double(Sidekiq::JobRecord, args: [4, 'retry'], klass: worker_class.name)]
+      end
+
+      let(:dead_set) do
+        [instance_double(Sidekiq::JobRecord, args: [5, 'dead'], klass: worker_class.name)]
+      end
+
+      before do
+        allow(Sidekiq::Queue).to receive(:new)
+                                   .with(coordinator.queue)
+                                   .and_return(queue + queue_incorrect_job_class)
+        allow(Sidekiq::ScheduledSet).to receive(:new).and_return(scheduled_set)
+        allow(Sidekiq::RetrySet).to receive(:new).and_return(retry_set)
+        allow(Sidekiq::DeadSet).to receive(:new).and_return(dead_set)
+      end
+
+      it 'does not include jobs for other workers' do
+        expect(coordinator.pending_jobs).not_to include(queue_incorrect_job_class.first)
+      end
+
+      context 'when not including dead jobs' do
+        it 'includes current and future jobs' do
+          expect(coordinator.pending_jobs(include_dead_jobs: false).to_a).to match_array(queue + scheduled_set)
+        end
+      end
+
+      context 'when including dead jobs' do
+        it 'includes current and future jobs, and also dead and retry jobs' do
+          expect(coordinator.pending_jobs(include_dead_jobs: true).to_a).to match_array(queue + scheduled_set + retry_set + dead_set)
+        end
+      end
+    end
+  end
+
   describe '#steal' do
     context 'when there are enqueued jobs present' do
       let(:queue) do
         [
-          double(args: ['Foo', [10, 20]], klass: worker_class.name),
-          double(args: ['Bar', [20, 30]], klass: worker_class.name),
-          double(args: ['Foo', [20, 30]], klass: 'MergeWorker')
+          instance_double(Sidekiq::JobRecord, args: ['Foo', [10, 20]], klass: worker_class.name),
+          instance_double(Sidekiq::JobRecord, args: ['Bar', [20, 30]], klass: worker_class.name),
+          instance_double(Sidekiq::JobRecord, args: ['Foo', [20, 30]], klass: 'MergeWorker')
         ]
       end
 

spec/lib/gitlab/database/migrations/test_background_runner_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::Migrations::TestBackgroundRunner, :redis do
+  include Gitlab::Database::Migrations::BackgroundMigrationHelpers
+
+  # In order to test the interaction between queueing sidekiq jobs and seeing those jobs in queues,
+  # we need to disable sidekiq's testing mode and actually send our jobs to redis
+  around do |ex|
+    Sidekiq::Testing.disable! { ex.run }
+  end
+
+  context 'without jobs to run' do
+    it 'returns immediately' do
+      runner = described_class.new
+      expect(runner).not_to receive(:run_job)
+      described_class.new.run_jobs(for_duration: 1.second)
+    end
+  end
+
+  context 'with jobs to run' do
+    let(:migration_name) { 'TestBackgroundMigration' }
+
+    before do
+      (1..5).each do |i|
+        migrate_in(i.minutes, migration_name, [i])
+      end
+    end
+
+    context 'finding pending background jobs' do
+      it 'finds all the migrations' do
+        expect(described_class.new.traditional_background_migrations.to_a.size).to eq(5)
+      end
+    end
+
+    context 'running migrations', :freeze_time do
+      def define_background_migration(name)
+        klass = Class.new do
+          # Can't simply def perform here as we won't have access to the block,
+          # similarly can't define_method(:perform, &block) here as it would change the block receiver
+          define_method(:perform) { |*args| yield(*args) }
+        end
+        stub_const("Gitlab::BackgroundMigration::#{name}", klass)
+        klass
+      end
+
+      def expect_migration_call_counts(migrations_to_calls)
+        migrations_to_calls.each do |migration, calls|
+          expect_next_instances_of(migration, calls) do |m|
+            expect(m).to receive(:perform).and_call_original
+          end
+        end
+      end
+
+      it 'runs the migration class correctly' do
+        calls = []
+        define_background_migration(migration_name) do |i|
+          calls << i
+        end
+        described_class.new.run_jobs(for_duration: 1.second) # Any time would work here as we do not advance time
+        expect(calls).to contain_exactly(1, 2, 3, 4, 5)
+      end
+
+      it 'runs the migration for a uniform amount of time' do
+        migration = define_background_migration(migration_name) do |i|
+          travel(1.minute)
+        end
+
+        expect_migration_call_counts(migration => 3)
+
+        described_class.new.run_jobs(for_duration: 3.minutes)
+      end
+
+      context 'with multiple migrations to run' do
+        let(:other_migration_name) { 'OtherBackgroundMigration' }
+
+        before do
+          (1..5).each do |i|
+            migrate_in(i.minutes, other_migration_name, [i])
+          end
+        end
+
+        it 'splits the time between migrations when all migrations use all their time' do
+          migration = define_background_migration(migration_name) do |i|
+            travel(1.minute)
+          end
+
+          other_migration = define_background_migration(other_migration_name) do |i|
+            travel(2.minutes)
+          end
+
+          expect_migration_call_counts(
+            migration => 2, # 1 minute jobs for 90 seconds, can finish the first and start the second
+            other_migration => 1 # 2 minute jobs for 90 seconds, past deadline after a single job
+          )
+
+          described_class.new.run_jobs(for_duration: 3.minutes)
+        end
+
+        it 'does not give leftover time to extra migrations' do
+          # This is currently implemented this way for simplicity, but it could make sense to change this behavior.
+
+          migration = define_background_migration(migration_name) do
+            travel(1.second)
+          end
+          other_migration = define_background_migration(other_migration_name) do
+            travel(1.minute)
+          end
+          expect_migration_call_counts(
+            migration => 5,
+            other_migration => 2
+          )
+
+          described_class.new.run_jobs(for_duration: 3.minutes)
+        end
+      end
+    end
+  end
+end