Merge branch...

Merge branch '214749-stale-cache-causes-projects-moved-to-indexed-group-not-to-be-indexed-in-elasticsearch' into 'master'

Invalidate ES cache when projects or groups are transferred

See merge request gitlab-org/gitlab!50712

app/services/projects/transfer_service.rb

@@ -37,7 +37,7 @@ module Projects
 
     private
 
-    attr_reader :old_path, :new_path, :new_namespace
+    attr_reader :old_path, :new_path, :new_namespace, :old_namespace
 
     # rubocop: disable CodeReuse/ActiveRecord
     def transfer(project)
@@ -96,7 +96,7 @@ module Projects
         execute_system_hooks
       end
 
-      move_pages(project)
+      post_update_hooks(project)
     rescue Exception # rubocop:disable Lint/RescueException
       rollback_side_effects
       raise
@@ -104,6 +104,11 @@ module Projects
       refresh_permissions
     end
 
+    # Overridden in EE
+    def post_update_hooks(project)
+      move_pages(project)
+    end
+
     def transfer_missing_group_resources(group)
       Labels::TransferService.new(current_user, group, project).execute
 

ee/app/models/concerns/elastic/projects_search.rb

@@ -22,6 +22,10 @@ module Elastic
       def maintain_elasticsearch_destroy
         ElasticDeleteProjectWorker.perform_async(self.id, self.es_id)
       end
+
+      def invalidate_elasticsearch_indexes_cache!
+        ::Gitlab::CurrentSettings.invalidate_elasticsearch_indexes_cache_for_project!(self.id)
+      end
     end
   end
 end

ee/app/models/ee/application_setting.rb

@@ -202,6 +202,10 @@ module EE
       ::Gitlab::Elastic::ElasticsearchEnabledCache.delete(:namespace)
     end
 
+    def invalidate_elasticsearch_indexes_cache_for_project!(project_id)
+      ::Gitlab::Elastic::ElasticsearchEnabledCache.delete_record(:project, project_id)
+    end
+
     def elasticsearch_limited_projects(ignore_namespaces = false)
       return ::Project.where(id: ElasticsearchIndexedProject.select(:project_id)) if ignore_namespaces
 

ee/app/services/ee/groups/transfer_service.rb

@@ -15,10 +15,9 @@ module EE
 
       override :post_update_hooks
       def post_update_hooks(updated_project_ids)
-        ::Project.id_in(updated_project_ids).find_each do |project|
-          project.maintain_elasticsearch_update(updated_attributes: [:visibility_level]) if project.maintaining_elasticsearch?
-        end
         super
+
+        update_elasticsearch_hooks(updated_project_ids)
       end
 
       def lost_groups
@@ -30,6 +29,24 @@ module EE
           ancestors
         end
       end
+
+      def update_elasticsearch_hooks(updated_project_ids)
+        project_ids = updated_project_ids
+
+        # Handle when group is moved to a new group. There is no way to know
+        # whether the group was using Elasticsearch before the transfer, so the ES cache is invalidated
+        # for each associated project. Otherwise, it is assumed all projects are indexed
+        # and only those with visibility changes have their ES cache entry invalidated
+        if ::Gitlab::CurrentSettings.elasticsearch_limit_indexing?
+          project_ids = group.all_projects.select(:id)
+        end
+
+        ::Project.id_in(project_ids).find_each do |project|
+          project.invalidate_elasticsearch_indexes_cache!
+
+          project.maintain_elasticsearch_update(updated_attributes: [:visibility_level]) if project.maintaining_elasticsearch?
+        end
+      end
     end
   end
 end

ee/app/services/ee/projects/transfer_service.rb

@@ -26,6 +26,25 @@ module EE
 
         ::Epics::TransferService.new(current_user, group, project).execute
       end
+
+      override :post_update_hooks
+      def post_update_hooks(project)
+        super
+
+        update_elasticsearch_hooks
+      end
+
+      def update_elasticsearch_hooks
+        return unless ::Gitlab::CurrentSettings.elasticsearch_limit_indexing?
+
+        # handle when project is moved to a new namespace with different elasticsearch settings
+        # than the old namespace
+        if old_namespace.use_elasticsearch? != new_namespace.use_elasticsearch?
+          project.invalidate_elasticsearch_indexes_cache!
+
+          project.maintain_elasticsearch_update(updated_attributes: [:visibility_level]) if project.maintaining_elasticsearch?
+        end
+      end
     end
   end
 end

ee/changelogs/unreleased/214749-stale-cache-causes-projects-moved-to-indexed-group-not-to-be-in.yml

+---
+title: Invalidate Elasticsearch cache when moving projects or groups
+merge_request: 50712
+author:
+type: fixed

ee/lib/gitlab/elastic/elasticsearch_enabled_cache.rb

@@ -58,6 +58,15 @@ module Gitlab
           Gitlab::Redis::Cache.with { |redis| redis.unlink(redis_key(type)) }
         end
 
+        # Deletes the specific record for this type. Only one key in the cache will
+        # be removed.
+        #
+        # @param type [Symbol] the type of resource, `:project` or `:namespace`
+        # @param record_id [Integer] the id of the record
+        def delete_record(type, record_id)
+          Gitlab::Redis::Cache.with { |redis| redis.hdel(redis_key(type), record_id) }
+        end
+
         private
 
         def redis_key(type)

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

@@ -58,4 +58,34 @@ RSpec.describe Gitlab::Elastic::ElasticsearchEnabledCache, :clean_gitlab_redis_c
       expect(described_class.fetch(:namespace, 1) { true }).to eq(false)
     end
   end
+
+  describe '.delete_record' do
+    it 'clears the cached value' do
+      expect(described_class.fetch(:project, 1) { true }).to eq(true)
+
+      described_class.delete_record(:project, 1)
+
+      expect(described_class.fetch(:project, 1) { false }).to eq(false)
+    end
+
+    it 'does not clear the cache for another record of the same type' do
+      expect(described_class.fetch(:project, 1) { true }).to eq(true)
+      expect(described_class.fetch(:project, 2) { false }).to eq(false)
+
+      described_class.delete_record(:project, 1)
+
+      expect(described_class.fetch(:project, 1) { false }).to eq(false)
+      expect(described_class.fetch(:project, 2) { true }).to eq(false)
+    end
+
+    it 'does not clear the cache for another record of a different type' do
+      expect(described_class.fetch(:project, 1) { true }).to eq(true)
+      expect(described_class.fetch(:namespace, 1) { false }).to eq(false)
+
+      described_class.delete_record(:project, 1)
+
+      expect(described_class.fetch(:project, 1) { false }).to eq(false)
+      expect(described_class.fetch(:namespace, 1) { true }).to eq(false)
+    end
+  end
 end

ee/spec/models/application_setting_spec.rb

@@ -442,6 +442,15 @@ RSpec.describe ApplicationSetting do
     end
   end
 
+  describe '#invalidate_elasticsearch_indexes_cache_for_project!' do
+    it 'deletes the ElasticsearchEnabledCache for a single project' do
+      project_id = 1
+      expect(::Gitlab::Elastic::ElasticsearchEnabledCache).to receive(:delete_record).with(:project, project_id)
+
+      setting.invalidate_elasticsearch_indexes_cache_for_project!(project_id)
+    end
+  end
+
   describe '#search_using_elasticsearch?' do
     # Constructs a truth table to run the specs against
     where(indexing: [true, false], searching: [true, false], limiting: [true, false], advanced_global_search_for_limited_indexing: [true, false])

ee/spec/services/groups/transfer_service_spec.rb

@@ -14,29 +14,70 @@ RSpec.describe Groups::TransferService, '#execute' do
     new_group&.add_owner(user)
   end
 
-  context 'when visibility changes' do
-    let(:new_group) { create(:group, :private) }
-
+  describe 'elasticsearch indexing', :elastic, :aggregate_failures do
     before do
       stub_ee_application_setting(elasticsearch_indexing: true)
     end
 
-    it 'reindexes projects and associated issues', :elastic do
-      project1 = create(:project, :repository, :public, namespace: group)
-      project2 = create(:project, :repository, :public, namespace: group)
-      project3 = create(:project, :repository, :private, namespace: group)
+    context 'when elasticsearch_limit_indexing is on' do
+      before do
+        stub_ee_application_setting(elasticsearch_limit_indexing: true)
+      end
+
+      let(:project) { create(:project, :repository, :public, namespace: group) }
+
+      context 'when moving from a non-indexed namespace to an indexed namespace' do
+        before do
+          create(:elasticsearch_indexed_namespace, namespace: new_group)
+        end
+
+        it 'invalidates the cache and indexes the project and associated issues' do
+          expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+          expect(::Gitlab::CurrentSettings).to receive(:invalidate_elasticsearch_indexes_cache_for_project!).with(project.id).and_call_original
+
+          transfer_service.execute(new_group)
+        end
+      end
+
+      context 'when both namespaces are indexed' do
+        before do
+          create(:elasticsearch_indexed_namespace, namespace: group)
+          create(:elasticsearch_indexed_namespace, namespace: new_group)
+        end
+
+        it 'invalidates the cache and indexes the project and associated issues' do
+          expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+          expect(::Gitlab::CurrentSettings).to receive(:invalidate_elasticsearch_indexes_cache_for_project!).with(project.id).and_call_original
+
+          transfer_service.execute(new_group)
+        end
+      end
+    end
+
+    context 'when elasticsearch_limit_indexing is off' do
+      context 'when visibility changes' do
+        let(:new_group) { create(:group, :private) }
+
+        it 'reindexes projects and associated issues' do
+          project1 = create(:project, :repository, :public, namespace: group)
+          project2 = create(:project, :repository, :public, namespace: group)
+          project3 = create(:project, :repository, :private, namespace: group)
 
-      expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project1)
-      expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, ['issues'])
-      expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project2)
-      expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, ['issues'])
-      expect(Elastic::ProcessBookkeepingService).not_to receive(:track!).with(project3)
-      expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, ['issues'])
+          expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project1)
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project1.id, ['issues'])
+          expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project2)
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project2.id, ['issues'])
+          expect(Elastic::ProcessBookkeepingService).not_to receive(:track!).with(project3)
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async).with('Project', project3.id, ['issues'])
 
-      transfer_service.execute(new_group)
+          transfer_service.execute(new_group)
 
-      expect(transfer_service.error).not_to be
-      expect(group.parent).to eq(new_group)
+          expect(transfer_service.error).not_to be
+          expect(group.parent).to eq(new_group)
+        end
+      end
     end
   end
 

ee/spec/services/projects/transfer_service_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe Projects::TransferService do
   include EE::GeoHelpers
 
   let_it_be(:user) { create(:user) }
-  let_it_be(:group) { create(:group) }
-  let(:project) { create(:project, :repository, :legacy_storage, namespace: user.namespace) }
+  let_it_be(:group) { create(:group, :public) }
+  let(:project) { create(:project, :repository, :public, :legacy_storage, namespace: user.namespace) }
 
   subject { described_class.new(project, user) }
 
@@ -63,4 +63,66 @@ RSpec.describe Projects::TransferService do
       subject.execute(group)
     end
   end
+
+  describe 'elasticsearch indexing', :elastic, :aggregate_failures do
+    before do
+      stub_ee_application_setting(elasticsearch_indexing: true)
+    end
+
+    context 'when visibility level changes' do
+      let_it_be(:group) { create(:group, :private) }
+
+      it 'reindexes the project and associated issues' do
+        expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
+        expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+
+        subject.execute(group)
+      end
+    end
+
+    context 'when elasticsearch_limit_indexing is on' do
+      before do
+        stub_ee_application_setting(elasticsearch_limit_indexing: true)
+      end
+
+      context 'when transferring between a non-indexed namespace and an indexed namespace' do
+        before do
+          create(:elasticsearch_indexed_namespace, namespace: group)
+        end
+
+        it 'invalidates the cache and indexes the project and associated issues' do
+          expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
+          expect(ElasticAssociationIndexerWorker).to receive(:perform_async).with('Project', project.id, ['issues'])
+          expect(::Gitlab::CurrentSettings).to receive(:invalidate_elasticsearch_indexes_cache_for_project!).with(project.id).and_call_original
+
+          subject.execute(group)
+        end
+      end
+
+      context 'when both namespaces are indexed' do
+        before do
+          create(:elasticsearch_indexed_namespace, namespace: group)
+          create(:elasticsearch_indexed_namespace, namespace: project.namespace)
+        end
+
+        it 'does not invalidate the cache and reindexes the project only' do
+          expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
+          expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
+          expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!)
+
+          subject.execute(group)
+        end
+      end
+    end
+
+    context 'when elasticsearch_limit_indexing is off' do
+      it 'does not invalidate the cache and reindexes the project only' do
+        expect(Elastic::ProcessBookkeepingService).to receive(:track!).with(project)
+        expect(ElasticAssociationIndexerWorker).not_to receive(:perform_async)
+        expect(::Gitlab::CurrentSettings).not_to receive(:invalidate_elasticsearch_indexes_cache_for_project!).with(project.id).and_call_original
+
+        subject.execute(group)
+      end
+    end
+  end
 end