Merge branch...

Merge branch '494418-zoekt-project-search-should-select-correct-zoekt-node-instead-of-first-node-for-namespace' into 'master' 

Zoekt project search should select correct zoekt node for replica search

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167455



Merged-by: Dmitry Gruzd <dgruzd@gitlab.com>
Approved-by: Dmitry Gruzd <dgruzd@gitlab.com>
Reviewed-by: Dmitry Gruzd <dgruzd@gitlab.com>
Reviewed-by: Terri Chu <tchu@gitlab.com>
Co-authored-by: rkumar555 <rkumar@gitlab.com>

ee/app/models/ee/project.rb

@@ -80,7 +80,7 @@ def preload_protected_branches
       has_one :security_statistics, class_name: 'Security::ProjectStatistics'
       has_one :dependency_proxy_packages_setting, class_name: '::DependencyProxy::Packages::Setting', inverse_of: :project
-      has_one :zoekt_repository, class_name: '::Search::Zoekt::Repository', inverse_of: :project
+      has_many :zoekt_repositories, class_name: '::Search::Zoekt::Repository', inverse_of: :project
       has_one :secrets_manager, class_name: '::SecretsManagement::ProjectSecretsManager'
       has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent

ee/app/models/search/zoekt/node.rb

@@ -19,6 +19,8 @@ class Node < ApplicationRecord
         through: :indices, source: :zoekt_enabled_namespace, class_name: '::Search::Zoekt::EnabledNamespace'
       has_many :tasks,
         foreign_key: :zoekt_node_id, inverse_of: :node, class_name: '::Search::Zoekt::Task'
+      has_many :zoekt_repositories,
+        through: :indices, source: :zoekt_repositories, class_name: '::Search::Zoekt::Repository'
       validates :index_base_url, presence: true
       validates :search_base_url, presence: true
@@ -33,6 +35,12 @@ class Node < ApplicationRecord
       scope :by_name, ->(*names) { where("metadata->>'name' IN (?)", names) }
       scope :lost, -> { where(last_seen_at: ..LOST_DURATION_THRESHOLD.ago) }
       scope :online, -> { where(last_seen_at: ONLINE_DURATION_THRESHOLD.ago..) }
+      scope :searchable, -> { online }
+      scope :searchable_for_project, ->(project) do
+        searchable.joins(:zoekt_repositories)
+                  .merge(Repository.searchable)
+                  .where(zoekt_repositories: { project: project })
+      end
       def self.find_or_initialize_by_task_request(params)
         params = params.with_indifferent_access

ee/app/models/search/zoekt/repository.rb

@@ -5,11 +5,13 @@ module Zoekt
     class Repository < ApplicationRecord
       include EachBatch
+      SEARCHABLE_STATES = %i[ready].freeze
+
       self.table_name = 'zoekt_repositories'
       belongs_to :zoekt_index, inverse_of: :zoekt_repositories, class_name: '::Search::Zoekt::Index'
-      belongs_to :project, inverse_of: :zoekt_repository, class_name: 'Project'
+      belongs_to :project, inverse_of: :zoekt_repositories, class_name: 'Project'
       has_many :tasks,
         foreign_key: :zoekt_repository_id, inverse_of: :zoekt_repository, class_name: '::Search::Zoekt::Task'
@@ -47,6 +49,8 @@ class Repository < ApplicationRecord
       scope :for_zoekt_indices, ->(indices) { where(zoekt_index: indices) }
+      scope :searchable, -> { where(state: SEARCHABLE_STATES) }
+
       def self.create_tasks(project_id:, zoekt_index:, task_type:, perform_at:)
         project = Project.find_by_id(project_id)
         find_or_initialize_by(project_identifier: project_id, project: project, zoekt_index: zoekt_index).tap do |item|

ee/app/services/ee/search/project_service.rb

@@ -76,6 +76,11 @@ def zoekt_searchable_scope
       def zoekt_projects
         @zoekt_projects ||= ::Project.id_in(project)
       end
+
+      override :zoekt_nodes
+      def zoekt_nodes
+        @zoekt_nodes ||= ::Search::Zoekt::Node.searchable_for_project(zoekt_searchable_scope)
+      end
     end
   end
 end

ee/spec/models/ee/project_spec.rb

@@ -71,7 +71,7 @@
     it { is_expected.to have_many(:approval_policy_rules).through(:approval_policy_rule_project_links) }
     it { is_expected.to have_one(:github_integration) }
-    it { is_expected.to have_one(:zoekt_repository) }
+    it { is_expected.to have_many(:zoekt_repositories) }
     it { is_expected.to have_one(:google_cloud_platform_artifact_registry_integration) }
     it { is_expected.to have_one(:google_cloud_platform_workload_identity_federation_integration) }
     it { is_expected.to have_one(:git_guardian_integration) }

ee/spec/models/search/zoekt/node_spec.rb

@@ -3,16 +3,17 @@
 require 'spec_helper'
 RSpec.describe ::Search::Zoekt::Node, feature_category: :global_search do
-  let_it_be(:indexed_namespace1) { create(:namespace) }
-  let_it_be(:indexed_namespace2) { create(:namespace) }
-  let_it_be(:unindexed_namespace) { create(:namespace) }
   let_it_be_with_reload(:node) do
     create(:zoekt_node, index_base_url: 'http://example.com:1234/', search_base_url: 'http://example.com:4567/')
   end
+  let_it_be(:indexed_namespace1) { create(:namespace) }
+  let_it_be(:indexed_namespace2) { create(:namespace) }
+  let_it_be(:unindexed_namespace) { create(:namespace) }
+  let_it_be(:enabled_namespace1) { create(:zoekt_enabled_namespace, namespace: indexed_namespace1) }
+  let_it_be(:zoekt_index) { create(:zoekt_index, :ready, node: node, zoekt_enabled_namespace: enabled_namespace1) }
+
   before do
-    enabled_namespace1 = create(:zoekt_enabled_namespace, namespace: indexed_namespace1)
-    create(:zoekt_index, :ready, node: node, zoekt_enabled_namespace: enabled_namespace1)
     enabled_namespace2 = create(:zoekt_enabled_namespace, namespace: indexed_namespace2)
     create(:zoekt_index, :ready, node: node, zoekt_enabled_namespace: enabled_namespace2)
   end
@@ -21,6 +22,7 @@
     it { is_expected.to have_many(:indices).inverse_of(:node) }
     it { is_expected.to have_many(:tasks).inverse_of(:node) }
     it { is_expected.to have_many(:enabled_namespaces).through(:indices) }
+    it { is_expected.to have_many(:zoekt_repositories).through(:indices) }
   end
   describe 'scopes' do
@@ -42,6 +44,16 @@
       end
     end
+    describe '.searchable', :freeze_time do
+      let_it_be(:searchable_node) { create(:zoekt_node) }
+      let_it_be(:non_searchable_node) { create(:zoekt_node, :offline) }
+
+      it 'returns nodes considered to be searchable' do
+        expect(described_class.searchable).to include searchable_node
+        expect(described_class.searchable).not_to include non_searchable_node
+      end
+    end
+
     describe '.by_name' do
       let_it_be(:node1) { create(:zoekt_node, metadata: { name: 'node1' }) }
       let_it_be(:node2) { create(:zoekt_node, metadata: { name: 'node2' }) }
@@ -53,6 +65,49 @@
         expect(described_class.by_name('non_existent')).to be_empty
       end
     end
+
+    describe '.searchable_for_project' do
+      let_it_be(:project) { create(:project, namespace: indexed_namespace1) }
+      let_it_be(:zoekt_index) { create(:zoekt_index) }
+
+      context 'when zoekt_repository for the given project does not exists' do
+        it 'is empty' do
+          expect(described_class.searchable_for_project(project)).to be_empty
+        end
+      end
+
+      context 'when zoekt_repository for the given project exists' do
+        let_it_be_with_reload(:zoekt_repository) do
+          create(:zoekt_repository, project: project, zoekt_index: zoekt_index)
+        end
+
+        context 'when there is no ready repository' do
+          it 'is empty' do
+            expect(described_class.searchable_for_project(project)).to be_empty
+          end
+        end
+
+        context 'when there is a ready repository' do
+          before do
+            zoekt_repository.ready!
+          end
+
+          it 'returns the nodes' do
+            expect(described_class.searchable_for_project(project)).not_to be_empty
+          end
+
+          context 'when there is no online nodes' do
+            before do
+              Search::Zoekt::Node.update_all(last_seen_at: Search::Zoekt::Node::ONLINE_DURATION_THRESHOLD.ago - 1.hour)
+            end
+
+            it 'is empty' do
+              expect(described_class.searchable_for_project(project)).to be_empty
+            end
+          end
+        end
+      end
+    end
   end
   describe 'validations' do

ee/spec/models/search/zoekt/repository_spec.rb

@@ -7,7 +7,7 @@
   describe 'relations' do
     it { is_expected.to belong_to(:zoekt_index).inverse_of(:zoekt_repositories) }
-    it { is_expected.to belong_to(:project).inverse_of(:zoekt_repository) }
+    it { is_expected.to belong_to(:project).inverse_of(:zoekt_repositories) }
   end
   describe 'before_validation' do
@@ -60,6 +60,22 @@
           zoekt_repository2, zoekt_repository3
       end
     end
+
+    describe '.searchable' do
+      let_it_be(:ready) { create(:zoekt_repository, state: :ready) }
+      let_it_be(:pending) { create(:zoekt_repository, state: :pending) }
+      let_it_be(:initializing) { create(:zoekt_repository, state: :initializing) }
+      let_it_be(:orphaned) { create(:zoekt_repository, state: :orphaned) }
+      let_it_be(:pending_deletion) { create(:zoekt_repository, state: :pending_deletion) }
+      let_it_be(:failed) { create(:zoekt_repository, state: :failed) }
+
+      subject(:records) { described_class.searchable }
+
+      it 'returns all repositories with state includes in SEARCHABLE_STATES' do
+        expect(records).to include ready
+        expect(records).not_to include pending, initializing, orphaned, pending_deletion, failed
+      end
+    end
   end
   describe '.create_tasks', :freeze_time do

ee/spec/requests/api/graphql/search/blob_search_spec.rb

@@ -6,7 +6,7 @@
   include GraphqlHelpers
   let_it_be(:current_user) { create(:user) }
   let_it_be(:group) { create(:group) }
-  let_it_be(:project) { create(:project, :public, :repository, name: 'awesome project', group: group) }
+  let_it_be_with_reload(:project) { create(:project, :public, :repository, name: 'awesome project', group: group) }
   let(:fields) { all_graphql_fields_for(Types::Search::Blob::BlobSearchType, max_depth: 4) }
   let(:query) { graphql_query_for(:blobSearch, { search: 'test', group_id: "gid://gitlab/Group/#{group.id}" }, fields) }

ee/spec/services/search/project_service_spec.rb

@@ -610,4 +610,13 @@
       end
     end
   end
+
+  describe '#zoekt_nodes' do
+    subject(:service) { described_class.new(user, project, scope: 'blobs') }
+
+    it 'calls on Node.searchable_for_project' do
+      expect(Search::Zoekt::Node).to receive(:searchable_for_project).with(project).and_return(:result)
+      expect(service.zoekt_nodes).to eq(:result)
+    end
+  end
 end

ee/spec/support/search/zoekt.rb

@@ -11,6 +11,7 @@ def ensure_zoekt_node!
           search_base_url: search_base_url
         ) do |node|
           node.uuid = SecureRandom.uuid
+          node.last_seen_at = Time.zone.now
         end
       end
       module_function :ensure_zoekt_node!
@@ -39,15 +40,53 @@ def zoekt_ensure_namespace_indexed!(namespace)
         index.update!(state: :ready)
       end
+      # Since in the test setup it is complicated to achieve indexing via pulling tasks,
+      # we are sending the HTTP post request to the indexer for indexing.
+      # At the end if indexed files exists(success callback), we are moving task to done and zoekt_repository to ready
       def zoekt_ensure_project_indexed!(project)
         zoekt_ensure_namespace_indexed!(project.namespace)
-        project.repository.update_zoekt_index!
-        # Add delay to allow Zoekt wbeserver to finish the indexing
+        ::Search::Zoekt::IndexingTaskService.new(project.id, :index_repo).execute
+        repository_storage = project.repository_storage
+        connection_info = Gitlab::GitalyClient.connection_data(repository_storage)
+        repository_path = "#{project.repository.disk_path}.git"
+        address = connection_info['address']
+        if address.match?(%r{\Aunix:[^/.]})
+          path = address.split('unix:').last
+          address = "unix:#{Rails.root.join(path)}"
+        end
+        payload = {
+          GitalyConnectionInfo: {
+            Address: address,
+            Token: connection_info['token'],
+            Storage: repository_storage,
+            Path: repository_path
+          },
+          Callback: { name: 'index' },
+          RepoId: project.id,
+          FileSizeLimit: Gitlab::CurrentSettings.elasticsearch_indexed_file_size_limit_kb.kilobytes,
+          Timeout: '10s',
+          Force: true
+        }
+        defaults = {
+          headers: { 'Content-Type' => 'application/json' },
+          body: payload.to_json,
+          allow_local_requests: true,
+          timeout: 10.seconds.to_i
+        }
+        node = zoekt_node
+        url = ::Gitlab::Search::Zoekt::Client.new.send(:join_url, node.index_base_url, '/indexer/index')
+        ::Gitlab::HTTP.post(url, defaults)
+        # Add delay to allow Zoekt webserver to finish the indexing
         10.times do
           results = Gitlab::Search::Zoekt::Client.new.search('.*', num: 1, project_ids: [project.id],
-            node_id: zoekt_node.id, search_mode: :regex)
-          break if results.file_count > 0
+            node_id: node.id, search_mode: :regex)
+
+          if results.file_count > 0
+            Search::Zoekt::Task.index_repo.where(project_identifier: project.id).update_all(state: :done)
+            project.zoekt_repositories.update_all(state: :ready)
+            break
+          end
           sleep 0.01
         end

spec/lib/gitlab/import_export/all_models.yml

@@ -883,7 +883,7 @@ project:
 - dora_performance_scores
 - xray_reports
 - member_approvals
-- zoekt_repository
+- zoekt_repositories
 - security_policy_management_project_linked_configurations
 - security_policy_project_linked_projects
 - security_policy_project_linked_namespaces