Remove cached/memoized ES helper

Any Elasticsearch admin setting changes will not be updated for the
Elasticsearch migrations until sidekiq is restarted. For example
changing the URL or other settings will keep using the old URL in the
migration until sidekiq is restarted. This change fixes this problem by
removing class level memoization of Gitlab::Elastic::Helper

ee/changelogs/unreleased/297192-fix-cached-es-helper.yml

+---
+title: Remove cached/memoized ES helper
+merge_request: 51310
+author:
+type: fixed

ee/lib/gitlab/elastic/helper.rb

@@ -38,7 +38,7 @@ module Gitlab
         end
 
         def default
-          @default ||= self.new
+          self.new
         end
       end
 

ee/spec/controllers/admin/elasticsearch_controller_spec.rb

@@ -4,14 +4,16 @@ require 'spec_helper'
 
 RSpec.describe Admin::ElasticsearchController do
   let(:admin) { create(:admin) }
+  let(:helper) { Gitlab::Elastic::Helper.new }
 
   describe 'POST #enqueue_index' do
     before do
       sign_in(admin)
+      allow(Gitlab::Elastic::Helper).to receive(:default).and_return(helper)
     end
 
     it 'starts indexing' do
-      expect(Gitlab::Elastic::Helper.default).to(receive(:index_exists?)).and_return(true)
+      expect(helper).to(receive(:index_exists?)).and_return(true)
       expect_next_instance_of(::Elastic::IndexProjectsService) do |service|
         expect(service).to receive(:execute)
       end

ee/spec/lib/ee/gitlab/elastic/helper_spec.rb

@@ -39,6 +39,12 @@ RSpec.describe Gitlab::Elastic::Helper do
     end
   end
 
+  describe '.default' do
+    it 'does not cache the value' do
+      expect(described_class.default.object_id).not_to eq(described_class.default.object_id)
+    end
+  end
+
   describe '#default_mappings' do
     it 'has only one type' do
       expect(helper.default_mappings.keys).to match_array %i(doc)

ee/spec/models/elastic/reindexing_task_spec.rb

@@ -3,6 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe Elastic::ReindexingTask, type: :model do
+  let(:helper) { Gitlab::Elastic::Helper.new }
+
+  before do
+    allow(Gitlab::Elastic::Helper).to receive(:default).and_return(helper)
+  end
+
   describe 'relations' do
     it { is_expected.to have_many(:subtasks) }
   end
@@ -32,11 +38,11 @@ RSpec.describe Elastic::ReindexingTask, type: :model do
 
     it 'deletes the correct indices' do
       other_tasks.each do |task|
-        expect(Gitlab::Elastic::Helper.default).not_to receive(:delete_index).with(index_name: task.subtasks.first.index_name_from)
+        expect(helper).not_to receive(:delete_index).with(index_name: task.subtasks.first.index_name_from)
       end
 
       tasks_for_deletion.each do |task|
-        expect(Gitlab::Elastic::Helper.default).to receive(:delete_index).with(index_name: task.subtasks.first.index_name_from).and_return(true)
+        expect(helper).to receive(:delete_index).with(index_name: task.subtasks.first.index_name_from).and_return(true)
       end
 
       described_class.drop_old_indices!

ee/spec/services/application_settings/update_service_spec.rb

@@ -35,13 +35,19 @@ RSpec.describe ApplicationSettings::UpdateService do
     end
 
     context 'elasticsearch_indexing update' do
+      let(:helper) { Gitlab::Elastic::Helper.new }
+
+      before do
+        allow(Gitlab::Elastic::Helper).to receive(:default).and_return(helper)
+      end
+
       context 'index creation' do
         let(:opts) { { elasticsearch_indexing: true } }
 
         context 'when index exists' do
           it 'skips creating a new index' do
-            expect(Gitlab::Elastic::Helper.default).to(receive(:index_exists?)).and_return(true)
-            expect(Gitlab::Elastic::Helper.default).not_to(receive(:create_empty_index))
+            expect(helper).to(receive(:index_exists?)).and_return(true)
+            expect(helper).not_to(receive(:create_empty_index))
 
             service.execute
           end
@@ -49,8 +55,8 @@ RSpec.describe ApplicationSettings::UpdateService do
 
         context 'when index does not exist' do
           it 'creates a new index' do
-            expect(Gitlab::Elastic::Helper.default).to(receive(:index_exists?)).and_return(false)
-            expect(Gitlab::Elastic::Helper.default).to(receive(:create_empty_index))
+            expect(helper).to(receive(:index_exists?)).and_return(false)
+            expect(helper).to(receive(:create_empty_index))
 
             service.execute
           end

ee/spec/services/elastic/cluster_reindexing_service_spec.rb

@@ -5,11 +5,17 @@ require 'spec_helper'
 RSpec.describe Elastic::ClusterReindexingService, :elastic do
   subject { described_class.new }
 
+  let(:helper) { Gitlab::Elastic::Helper.new }
+
+  before do
+    allow(Gitlab::Elastic::Helper).to receive(:default).and_return(helper)
+  end
+
   context 'state: initial' do
     let(:task) { create(:elastic_reindexing_task, state: :initial) }
 
     it 'aborts if the main index does not use aliases' do
-      allow(Gitlab::Elastic::Helper.default).to receive(:alias_exists?).and_return(false)
+      allow(helper).to receive(:alias_exists?).and_return(false)
 
       expect { subject.execute }.to change { task.reload.state }.from('initial').to('failure')
       expect(task.reload.error_message).to match(/use aliases/)
@@ -23,8 +29,8 @@ RSpec.describe Elastic::ClusterReindexingService, :elastic do
     end
 
     it 'errors when there is not enough space' do
-      allow(Gitlab::Elastic::Helper.default).to receive(:index_size_bytes).and_return(100.megabytes)
-      allow(Gitlab::Elastic::Helper.default).to receive(:cluster_free_size_bytes).and_return(30.megabytes)
+      allow(helper).to receive(:index_size_bytes).and_return(100.megabytes)
+      allow(helper).to receive(:cluster_free_size_bytes).and_return(30.megabytes)
 
       expect { subject.execute }.to change { task.reload.state }.from('initial').to('failure')
       expect(task.reload.error_message).to match(/storage available/)
@@ -43,10 +49,10 @@ RSpec.describe Elastic::ClusterReindexingService, :elastic do
     it 'triggers reindexing' do
       task = create(:elastic_reindexing_task, state: :indexing_paused)
 
-      allow(Gitlab::Elastic::Helper.default).to receive(:create_empty_index).and_return('new_index_name' => 'new_index')
-      allow(Gitlab::Elastic::Helper.default).to receive(:create_standalone_indices).and_return('new_issues_name' => 'new_issues')
-      allow(Gitlab::Elastic::Helper.default).to receive(:reindex).with(from: anything, to: 'new_index_name').and_return('task_id_1')
-      allow(Gitlab::Elastic::Helper.default).to receive(:reindex).with(from: anything, to: 'new_issues_name').and_return('task_id_2')
+      allow(helper).to receive(:create_empty_index).and_return('new_index_name' => 'new_index')
+      allow(helper).to receive(:create_standalone_indices).and_return('new_issues_name' => 'new_issues')
+      allow(helper).to receive(:reindex).with(from: anything, to: 'new_index_name').and_return('task_id_1')
+      allow(helper).to receive(:reindex).with(from: anything, to: 'new_issues_name').and_return('task_id_2')
 
       expect { subject.execute }.to change { task.reload.state }.from('indexing_paused').to('reindexing')
 
@@ -72,13 +78,13 @@ RSpec.describe Elastic::ClusterReindexingService, :elastic do
     end
 
     before do
-      allow(Gitlab::Elastic::Helper.default).to receive(:task_status).and_return({ 'completed' => true })
-      allow(Gitlab::Elastic::Helper.default).to receive(:refresh_index).and_return(true)
+      allow(helper).to receive(:task_status).and_return({ 'completed' => true })
+      allow(helper).to receive(:refresh_index).and_return(true)
     end
 
     context 'errors are raised' do
       before do
-        allow(Gitlab::Elastic::Helper.default).to receive(:documents_count).with(index_name: subtask.index_name_to).and_return(subtask.reload.documents_count * 2)
+        allow(helper).to receive(:documents_count).with(index_name: subtask.index_name_to).and_return(subtask.reload.documents_count * 2)
       end
 
       it 'errors if documents count is different' do
@@ -87,14 +93,14 @@ RSpec.describe Elastic::ClusterReindexingService, :elastic do
       end
 
       it 'errors if reindexing is failed' do
-        allow(Gitlab::Elastic::Helper.default).to receive(:task_status).and_return({ 'completed' => true, 'error' => { 'type' => 'search_phase_execution_exception' } })
+        allow(helper).to receive(:task_status).and_return({ 'completed' => true, 'error' => { 'type' => 'search_phase_execution_exception' } })
 
         expect { subject.execute }.to change { task.reload.state }.from('reindexing').to('failure')
         expect(task.reload.error_message).to match(/has failed with/)
       end
 
       it 'errors if task is not found' do
-        allow(Gitlab::Elastic::Helper.default).to receive(:task_status).and_raise(Elasticsearch::Transport::Transport::Errors::NotFound)
+        allow(helper).to receive(:task_status).and_raise(Elasticsearch::Transport::Transport::Errors::NotFound)
 
         expect { subject.execute }.to change { task.reload.state }.from('reindexing').to('failure')
         expect(task.reload.error_message).to match(/couldn't load task status/i)
@@ -103,12 +109,12 @@ RSpec.describe Elastic::ClusterReindexingService, :elastic do
 
     context 'task finishes correctly' do
       before do
-        allow(Gitlab::Elastic::Helper.default).to receive(:documents_count).with(index_name: subtask.index_name_to).and_return(subtask.reload.documents_count)
+        allow(helper).to receive(:documents_count).with(index_name: subtask.index_name_to).and_return(subtask.reload.documents_count)
       end
 
       it 'launches all state steps' do
-        expect(Gitlab::Elastic::Helper.default).to receive(:update_settings).with(index_name: subtask.index_name_to, settings: expected_default_settings)
-        expect(Gitlab::Elastic::Helper.default).to receive(:switch_alias).with(to: subtask.index_name_to, from: subtask.index_name_from, alias_name: subtask.alias_name)
+        expect(helper).to receive(:update_settings).with(index_name: subtask.index_name_to, settings: expected_default_settings)
+        expect(helper).to receive(:switch_alias).with(to: subtask.index_name_to, from: subtask.index_name_from, alias_name: subtask.alias_name)
         expect(Gitlab::CurrentSettings).to receive(:update!).with(elasticsearch_pause_indexing: false)
 
         expect { subject.execute }.to change { task.reload.state }.from('reindexing').to('success')