Merge branch 'fix/gb/pipeline-index-latest' into 'master'

Fix pipeline index json Gitaly N+1 with `latest?`

Closes #19697

See merge request gitlab-org/gitlab!34160

app/models/ci/pipeline.rb

@@ -553,10 +553,28 @@ module Ci
       end
     end
 
+    def lazy_ref_commit
+      return unless ::Gitlab::Ci::Features.pipeline_latest?
+
+      BatchLoader.for(ref).batch do |refs, loader|
+        next unless project.repository_exists?
+
+        project.repository.list_commits_by_ref_name(refs).then do |commits|
+          loader.call(ref, commits[ref])
+        end
+      end
+    end
+
     def latest?
       return false unless git_ref && commit.present?
 
-      project.commit(git_ref) == commit
+      unless ::Gitlab::Ci::Features.pipeline_latest?
+        return project.commit(git_ref) == commit
+      end
+
+      return false if lazy_ref_commit.nil?
+
+      lazy_ref_commit.id == commit.id
     end
 
     def retried

changelogs/unreleased/fix-gb-pipeline-index-latest.yml

+---
+title: Improve pipeline index controller performance by resolving Gitaly N+1 calls
+merge_request: 34160
+author:
+type: performance

lib/gitlab/ci/features.rb

@@ -38,6 +38,10 @@ module Gitlab
         ::Feature.enabled?(:ci_atomic_processing, project, default_enabled: true)
       end
 
+      def self.pipeline_latest?
+        ::Feature.enabled?(:ci_pipeline_latest, default_enabled: true)
+      end
+
       def self.release_generation_enabled?
         ::Feature.enabled?(:ci_release_generation)
       end

lib/gitlab/ci/pipeline/preloader.rb

@@ -17,6 +17,7 @@ module Gitlab
           pipelines.each do |pipeline|
             self.new(pipeline).tap do |preloader|
               preloader.preload_commit_authors
+              preloader.preload_ref_commits
               preloader.preload_pipeline_warnings
               preloader.preload_stages_warnings
             end
@@ -27,12 +28,19 @@ module Gitlab
           @pipeline = pipeline
         end
 
+        # This also preloads the author of every commit. We're using "lazy_author"
+        # here since "author" immediately loads the data on the first call.
         def preload_commit_authors
-          # This also preloads the author of every commit. We're using "lazy_author"
-          # here since "author" immediately loads the data on the first call.
           @pipeline.commit.try(:lazy_author)
         end
 
+        # This preloads latest commits for given refs and therefore makes it
+        # much less expensive to check if a pipeline is a latest one for
+        # given branch.
+        def preload_ref_commits
+          @pipeline.lazy_ref_commit
+        end
+
         def preload_pipeline_warnings
           # This preloads the number of warnings for every pipeline, ensuring
           # that Ci::Pipeline#has_warnings? doesn't execute any additional
@@ -40,10 +48,10 @@ module Gitlab
           @pipeline.number_of_warnings
         end
 
+        # This preloads the number of warnings for every stage, ensuring
+        # that Ci::Stage#has_warnings? doesn't execute any additional
+        # queries.
         def preload_stages_warnings
-          # This preloads the number of warnings for every stage, ensuring
-          # that Ci::Stage#has_warnings? doesn't execute any additional
-          # queries.
           @pipeline.stages.each { |stage| stage.number_of_warnings }
         end
       end

lib/gitlab/git/repository.rb

@@ -1008,6 +1008,12 @@ module Gitlab
         end
       end
 
+      def list_commits_by_ref_name(refs)
+        wrapped_gitaly_errors do
+          gitaly_commit_client.list_commits_by_ref_name(refs)
+        end
+      end
+
       def last_commit_for_path(sha, path, literal_pathspec: false)
         wrapped_gitaly_errors do
           gitaly_commit_client.last_commit_for_path(sha, path, literal_pathspec: literal_pathspec)

lib/gitlab/gitaly_client/commit_service.rb

@@ -391,6 +391,21 @@ module Gitlab
         messages
       end
 
+      def list_commits_by_ref_name(refs)
+        request = Gitaly::ListCommitsByRefNameRequest
+          .new(repository: @gitaly_repo, ref_names: refs)
+
+        response = GitalyClient.call(@repository.storage, :commit_service, :list_commits_by_ref_name, request, timeout: GitalyClient.medium_timeout)
+
+        commit_refs = response.flat_map do |message|
+          message.commit_refs.map do |commit_ref|
+            [commit_ref.ref_name, Gitlab::Git::Commit.new(@repository, commit_ref.commit)]
+          end
+        end
+
+        Hash[commit_refs]
+      end
+
       private
 
       def call_commit_diff(request_params, options = {})

spec/controllers/projects/pipelines_controller_spec.rb

@@ -77,9 +77,9 @@ RSpec.describe Projects::PipelinesController do
 
         expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original
 
-        # ListCommitsByOid, RepositoryExists, HasLocalBranches
+        # ListCommitsByOid, RepositoryExists, HasLocalBranches, ListCommitsByRefNames
         expect { get_pipelines_index_json }
-          .to change { Gitlab::GitalyClient.get_request_count }.by(3)
+          .to change { Gitlab::GitalyClient.get_request_count }.by(4)
       end
     end
 
@@ -111,13 +111,15 @@ RSpec.describe Projects::PipelinesController do
       context 'scope is branches or tags' do
         before do
           create(:ci_pipeline, :failed, project: project, ref: 'v1.0.0', tag: true)
+          create(:ci_pipeline, :failed, project: project, ref: 'master', tag: false)
+          create(:ci_pipeline, :failed, project: project, ref: 'feature', tag: false)
         end
 
         context 'when scope is branches' do
           it 'returns matched pipelines' do
             get_pipelines_index_json(scope: 'branches')
 
-            check_pipeline_response(returned: 1, all: 7, running: 2, pending: 1, finished: 4)
+            check_pipeline_response(returned: 2, all: 9, running: 2, pending: 1, finished: 6)
           end
         end
 
@@ -125,7 +127,7 @@ RSpec.describe Projects::PipelinesController do
           it 'returns matched pipelines' do
             get_pipelines_index_json(scope: 'tags')
 
-            check_pipeline_response(returned: 1, all: 7, running: 2, pending: 1, finished: 4)
+            check_pipeline_response(returned: 1, all: 9, running: 2, pending: 1, finished: 6)
           end
         end
       end
@@ -234,7 +236,8 @@ RSpec.describe Projects::PipelinesController do
       user = create(:user)
       pipeline = create(:ci_empty_pipeline, status: status,
                                             project: project,
-                                            sha: sha,
+                                            sha: sha.id,
+                                            ref: sha.id.first(8),
                                             user: user,
                                             merge_request: merge_request)
 

spec/lib/gitlab/ci/pipeline/preloader_spec.rb

@@ -28,8 +28,9 @@ describe Gitlab::Ci::Pipeline::Preloader do
       end
     end
 
-    it 'preloads commit authors and number of warnings' do
+    it 'preloads commit authors, number of warnings and ref commits' do
       expect(commit).to receive(:lazy_author)
+      expect(pipeline).to receive(:lazy_ref_commit)
       expect(pipeline).to receive(:number_of_warnings)
       expect(stage).to receive(:number_of_warnings)
 
@@ -38,6 +39,7 @@ describe Gitlab::Ci::Pipeline::Preloader do
 
     it 'returns original collection' do
       allow(commit).to receive(:lazy_author)
+      allow(pipeline).to receive(:lazy_ref_commit)
       allow(pipeline).to receive(:number_of_warnings)
       allow(stage).to receive(:number_of_warnings)
 

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -381,4 +381,15 @@ describe Gitlab::GitalyClient::CommitService do
       commits.map { |commit| Gitlab::Git::Commit.new(repository, commit) }
     end
   end
+
+  describe '#list_commits_by_ref_name' do
+    it 'lists latest commits grouped by a ref name' do
+      response = client.list_commits_by_ref_name(%w[master feature v1.0.0 nonexistent])
+
+      expect(response.fetch('master').id).to eq 'b83d6e391c22777fca1ed3012fce84f633d7fed0'
+      expect(response.fetch('feature').id).to eq '0b4bc9a49b562e85de7cc9e834518ea6828729b9'
+      expect(response.fetch('v1.0.0').id).to eq '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9'
+      expect(response).not_to have_key 'nonexistent'
+    end
+  end
 end

spec/models/ci/pipeline_spec.rb

@@ -1488,6 +1488,12 @@ describe Ci::Pipeline, :mailer do
              sha: project.commit.sha)
     end
 
+    describe '#lazy_ref_commit' do
+      it 'returns the latest commit for a ref lazily' do
+        expect(pipeline.lazy_ref_commit.id).to eq project.commit(pipeline.ref).id
+      end
+    end
+
     describe '#latest?' do
       context 'with latest sha' do
         it 'returns true' do
@@ -1496,17 +1502,26 @@ describe Ci::Pipeline, :mailer do
       end
 
       context 'with a branch name as the ref' do
-        it 'looks up commit with the full ref name' do
-          expect(pipeline.project).to receive(:commit).with('refs/heads/master').and_call_original
+        it 'looks up a commit for a branch' do
+          expect(pipeline.ref).to eq 'master'
+          expect(pipeline).to be_latest
+        end
+      end
+
+      context 'with a tag name as a ref' do
+        it 'looks up a commit for a tag' do
+          expect(project.repository.branch_names).not_to include 'v1.0.0'
+
+          pipeline.update(sha: project.commit('v1.0.0').sha, ref: 'v1.0.0', tag: true)
 
+          expect(pipeline).to be_tag
           expect(pipeline).to be_latest
         end
       end
 
       context 'with not latest sha' do
         before do
-          pipeline.update(
-            sha: project.commit("#{project.default_branch}~1").sha)
+          pipeline.update(sha: project.commit("#{project.default_branch}~1").sha)
         end
 
         it 'returns false' do