Global lease for database reindexing

The idea here is to only ever have one reindexing process running at a
time.

We generously choose the timeout for this (1 day), but in the regular
case we expect things to succeed much quicker. The timeout needs to be
above 3 times the statement timeout for reindexing (3 steps that can
take up to 6 hours currently). Overall, it doesn't hurt to set a high
timeout for this - it may just slow reindexing process down (chances are
low for this).

lib/gitlab/database/reindexing.rb

@@ -4,11 +4,7 @@ module Gitlab
   module Database
     module Reindexing
       def self.perform(index_selector)
-        Array.wrap(index_selector).each do |index|
-          ReindexAction.keep_track_of(index) do
-            ConcurrentReindex.new(index).perform
-          end
-        end
+        Coordinator.new(index_selector).perform
       end
 
       def self.candidate_indexes

lib/gitlab/database/reindexing/coordinator.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Database
+    module Reindexing
+      class Coordinator
+        include ExclusiveLeaseGuard
+
+        # Maximum lease time for the global Redis lease
+        # This should be higher than the maximum time for any
+        # long running step in the reindexing process (compare with
+        # statement timeouts).
+        TIMEOUT_PER_ACTION = 1.day
+
+        attr_reader :indexes
+
+        def initialize(indexes)
+          @indexes = indexes
+        end
+
+        def perform
+          indexes.each do |index|
+            # This obtains a global lease such that there's
+            # only one live reindexing process at a time.
+            try_obtain_lease do
+              ReindexAction.keep_track_of(index) do
+                ConcurrentReindex.new(index).perform
+              end
+            end
+          end
+        end
+
+        private
+
+        def lease_timeout
+          TIMEOUT_PER_ACTION
+        end
+      end
+    end
+  end
+end

lib/tasks/gitlab/db.rake

@@ -180,7 +180,7 @@ namespace :gitlab do
       end
 
       indexes = if args[:index_name]
-                  Gitlab::Database::PostgresIndex.by_identifier(args[:index_name])
+                  [Gitlab::Database::PostgresIndex.by_identifier(args[:index_name])]
                 else
                   Gitlab::Database::Reindexing.candidate_indexes.random_few(2)
                 end

spec/lib/gitlab/database/reindexing/coordinator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Database::Reindexing::Coordinator do
+  include ExclusiveLeaseHelpers
+
+  describe '.perform' do
+    subject { described_class.new(indexes).perform }
+
+    let(:indexes) { [instance_double(Gitlab::Database::PostgresIndex), instance_double(Gitlab::Database::PostgresIndex)] }
+    let(:reindexers) { [instance_double(Gitlab::Database::Reindexing::ConcurrentReindex), instance_double(Gitlab::Database::Reindexing::ConcurrentReindex)] }
+
+    let!(:lease) { stub_exclusive_lease(lease_key, uuid, timeout: lease_timeout) }
+    let(:lease_key) { 'gitlab/database/reindexing/coordinator' }
+    let(:lease_timeout) { 1.day }
+    let(:uuid) { 'uuid' }
+
+    before do
+      allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:keep_track_of).and_yield
+
+      indexes.zip(reindexers).each do |index, reindexer|
+        allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer)
+        allow(reindexer).to receive(:perform)
+      end
+    end
+
+    it 'performs concurrent reindexing for each index' do
+      indexes.zip(reindexers).each do |index, reindexer|
+        expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).ordered.and_return(reindexer)
+        expect(reindexer).to receive(:perform)
+      end
+
+      subject
+    end
+
+    it 'keeps track of actions and creates ReindexAction records' do
+      indexes.each do |index|
+        expect(Gitlab::Database::Reindexing::ReindexAction).to receive(:keep_track_of).with(index).and_yield
+      end
+
+      subject
+    end
+
+    context 'locking' do
+      it 'acquires a lock while reindexing' do
+        indexes.each do |index|
+          expect(lease).to receive(:try_obtain).ordered.and_return(uuid)
+          action = instance_double(Gitlab::Database::Reindexing::ConcurrentReindex)
+          expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).ordered.with(index).and_return(action)
+          expect(action).to receive(:perform).ordered
+          expect(Gitlab::ExclusiveLease).to receive(:cancel).ordered.with(lease_key, uuid)
+        end
+
+        subject
+      end
+
+      it 'does does not perform reindexing actions if lease is not granted' do
+        indexes.each do |index|
+          expect(lease).to receive(:try_obtain).ordered.and_return(false)
+          expect(Gitlab::Database::Reindexing::ConcurrentReindex).not_to receive(:new)
+        end
+
+        subject
+      end
+    end
+  end
+end

spec/lib/gitlab/database/reindexing_spec.rb

@@ -3,53 +3,19 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::Reindexing do
-  describe '.perform' do
-    before do
-      allow(Gitlab::Database::Reindexing::ReindexAction).to receive(:keep_track_of).and_yield
-    end
-
-    shared_examples_for 'reindexing' do
-      before do
-        indexes.zip(reindexers).each do |index, reindexer|
-          allow(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).and_return(reindexer)
-          allow(reindexer).to receive(:perform)
-        end
-      end
-
-      it 'performs concurrent reindexing for each index' do
-        indexes.zip(reindexers).each do |index, reindexer|
-          expect(Gitlab::Database::Reindexing::ConcurrentReindex).to receive(:new).with(index).ordered.and_return(reindexer)
-          expect(reindexer).to receive(:perform)
-        end
-
-        subject
-      end
-
-      it 'keeps track of actions and creates ReindexAction records' do
-        indexes.each do |index|
-          expect(Gitlab::Database::Reindexing::ReindexAction).to receive(:keep_track_of).with(index).and_yield
-        end
+  include ExclusiveLeaseHelpers
 
-        subject
-      end
-    end
-
-    context 'with multiple indexes' do
-      subject { described_class.perform(indexes) }
-
-      let(:indexes) { [instance_double('Gitlab::Database::PostgresIndex'), instance_double('Gitlab::Database::PostgresIndex')] }
-      let(:reindexers) { [instance_double('Gitlab::Database::Reindexing::ConcurrentReindex'), instance_double('Gitlab::Database::Reindexing::ConcurrentReindex')] }
-
-      it_behaves_like 'reindexing'
-    end
+  describe '.perform' do
+    subject { described_class.perform(indexes) }
 
-    context 'single index' do
-      subject { described_class.perform(indexes.first) }
+    let(:coordinator) { instance_double(Gitlab::Database::Reindexing::Coordinator) }
+    let(:indexes) { double }
 
-      let(:indexes) { [instance_double('Gitlab::Database::PostgresIndex')] }
-      let(:reindexers) { [instance_double('Gitlab::Database::Reindexing::ConcurrentReindex')] }
+    it 'delegates to Coordinator' do
+      expect(Gitlab::Database::Reindexing::Coordinator).to receive(:new).with(indexes).and_return(coordinator)
+      expect(coordinator).to receive(:perform)
 
-      it_behaves_like 'reindexing'
+      subject
     end
   end
 

spec/tasks/gitlab/db_rake_spec.rb

@@ -225,7 +225,7 @@ RSpec.describe 'gitlab:db namespace rake task' do
 
       it 'calls the index rebuilder with the proper arguments' do
         expect(Gitlab::Database::PostgresIndex).to receive(:by_identifier).with('public.foo_idx').and_return(index)
-        expect(Gitlab::Database::Reindexing).to receive(:perform).with(index)
+        expect(Gitlab::Database::Reindexing).to receive(:perform).with([index])
 
         run_rake_task('gitlab:db:reindex', '[public.foo_idx]')
       end