Merge branch '297192-fix-cached-es-helper' into 'master'

Remove cached/memoized ES client/helper

See merge request gitlab-org/gitlab!51310

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')