Merge branch 'jv-ci-refspec-sha' into 'master'

CI: use commit SHA in persistent refspec

See merge request gitlab-org/gitlab!51208

app/presenters/ci/build_runner_presenter.rb

@@ -93,7 +93,22 @@ module Ci
     end
 
     def refspec_for_persistent_ref
-      "+#{persistent_ref_path}:#{persistent_ref_path}"
+      #
+      # End-to-end test coverage for CI fetching seems to not be strong, so we
+      # are using a feature flag here to close the confidence gap. My (JV)
+      # confidence about the change is very high but if something is wrong
+      # with it after all, this would cause all CI jobs on gitlab.com to fail.
+      #
+      # The roll-out will be tracked in
+      # https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746.
+      #
+      if Feature.enabled?(:scalability_ci_fetch_sha, type: :ops)
+        # Use persistent_ref.sha because it causes 'git fetch' to do less work.
+        # See https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746.
+        "+#{pipeline.persistent_ref.sha}:#{pipeline.persistent_ref.path}"
+      else
+        "+#{pipeline.persistent_ref.path}:#{pipeline.persistent_ref.path}"
+      end
     end
 
     def persistent_ref_exist?
@@ -107,10 +122,6 @@ module Ci
       pipeline.persistent_ref.exist?
     end
 
-    def persistent_ref_path
-      pipeline.persistent_ref.path
-    end
-
     def git_depth_variable
       strong_memoize(:git_depth_variable) do
         variables&.find { |variable| variable[:key] == 'GIT_DEPTH' }

changelogs/unreleased/jv-ci-refspec-sha.yml

+---
+title: 'CI: use commit SHA in persistent refspec'
+merge_request: 51208
+author:
+type: performance

config/feature_flags/ops/scalability_ci_fetch_sha.yml

+---
+name: scalability_ci_fetch_sha
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51208
+rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/746
+milestone: '13.8'
+type: ops
+group: team::Scalability
+default_enabled: false

doc/ci/pipelines/index.md

@@ -89,9 +89,9 @@ This table lists the refspecs injected for each pipeline type:
 
 | Pipeline type                                                      | Refspecs                                                                                       |
 |---------------                                                     |----------------------------------------                                                        |
-| Pipeline for Branches                                              | `+refs/pipelines/<id>:refs/pipelines/<id>` and `+refs/heads/<name>:refs/remotes/origin/<name>` |
-| pipeline for Tags                                                  | `+refs/pipelines/<id>:refs/pipelines/<id>` and `+refs/tags/<name>:refs/tags/<name>`            |
-| [Pipeline for Merge Requests](../merge_request_pipelines/index.md) | `+refs/pipelines/<id>:refs/pipelines/<id>`                                                     |
+| Pipeline for Branches                                              | `+<sha>:refs/pipelines/<id>` and `+refs/heads/<name>:refs/remotes/origin/<name>` |
+| pipeline for Tags                                                  | `+<sha>:refs/pipelines/<id>` and `+refs/tags/<name>:refs/tags/<name>`            |
+| [Pipeline for Merge Requests](../merge_request_pipelines/index.md) | `+<sha>:refs/pipelines/<id>`                                                     |
 
 The refs `refs/heads/<name>` and `refs/tags/<name>` exist in your
 project repository. GitLab generates the special ref `refs/pipelines/<id>` during a

spec/presenters/ci/build_runner_presenter_spec.rb

@@ -189,7 +189,21 @@ RSpec.describe Ci::BuildRunnerPresenter do
 
     it 'returns the correct refspecs' do
       is_expected.to contain_exactly("+refs/heads/#{build.ref}:refs/remotes/origin/#{build.ref}",
-                                     "+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}")
+                                     "+#{pipeline.sha}:refs/pipelines/#{pipeline.id}")
+    end
+
+    it 'uses a SHA in the persistent refspec' do
+      expect(subject[0]).to match(/^\+[0-9a-f]{40}:refs\/pipelines\/[0-9]+$/)
+    end
+
+    context 'when the scalability_ci_fetch_sha feature flag is disabled' do
+      before do
+        stub_feature_flags(scalability_ci_fetch_sha: false)
+      end
+
+      it 'fetches the ref by name' do
+        expect(subject[0]).to eq("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}")
+      end
     end
 
     context 'when ref is tag' do
@@ -197,7 +211,7 @@ RSpec.describe Ci::BuildRunnerPresenter do
 
       it 'returns the correct refspecs' do
         is_expected.to contain_exactly("+refs/tags/#{build.ref}:refs/tags/#{build.ref}",
-                                       "+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}")
+                                       "+#{pipeline.sha}:refs/pipelines/#{pipeline.id}")
       end
 
       context 'when GIT_DEPTH is zero' do
@@ -208,7 +222,7 @@ RSpec.describe Ci::BuildRunnerPresenter do
         it 'returns the correct refspecs' do
           is_expected.to contain_exactly('+refs/tags/*:refs/tags/*',
                                          '+refs/heads/*:refs/remotes/origin/*',
-                                         "+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}")
+                                         "+#{pipeline.sha}:refs/pipelines/#{pipeline.id}")
         end
       end
     end
@@ -224,7 +238,7 @@ RSpec.describe Ci::BuildRunnerPresenter do
 
       it 'returns the correct refspecs' do
         is_expected
-          .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}")
+          .to contain_exactly("+#{pipeline.sha}:refs/pipelines/#{pipeline.id}")
       end
 
       context 'when GIT_DEPTH is zero' do
@@ -234,7 +248,7 @@ RSpec.describe Ci::BuildRunnerPresenter do
 
         it 'returns the correct refspecs' do
           is_expected
-            .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+            .to contain_exactly("+#{pipeline.sha}:refs/pipelines/#{pipeline.id}",
                                 '+refs/heads/*:refs/remotes/origin/*',
                                 '+refs/tags/*:refs/tags/*')
         end
@@ -244,7 +258,7 @@ RSpec.describe Ci::BuildRunnerPresenter do
         let(:merge_request) { create(:merge_request, :with_legacy_detached_merge_request_pipeline) }
 
         it 'returns the correct refspecs' do
-          is_expected.to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+          is_expected.to contain_exactly("+#{pipeline.sha}:refs/pipelines/#{pipeline.id}",
                                           "+refs/heads/#{build.ref}:refs/remotes/origin/#{build.ref}")
         end
       end
@@ -262,7 +276,7 @@ RSpec.describe Ci::BuildRunnerPresenter do
 
       it 'exposes the persistent pipeline ref' do
         is_expected
-          .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+          .to contain_exactly("+#{pipeline.sha}:refs/pipelines/#{pipeline.id}",
                               "+refs/heads/#{build.ref}:refs/remotes/origin/#{build.ref}")
       end
     end

spec/requests/api/ci/runner/jobs_request_post_spec.rb

@@ -156,7 +156,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
               'sha' => job.sha,
               'before_sha' => job.before_sha,
               'ref_type' => 'branch',
-              'refspecs' => ["+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+              'refspecs' => ["+#{pipeline.sha}:refs/pipelines/#{pipeline.id}",
                              "+refs/heads/#{job.ref}:refs/remotes/origin/#{job.ref}"],
               'depth' => project.ci_default_git_depth }
           end
@@ -284,7 +284,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
                 expect(response).to have_gitlab_http_status(:created)
                 expect(json_response['git_info']['refspecs'])
-                  .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+                  .to contain_exactly("+#{pipeline.sha}:refs/pipelines/#{pipeline.id}",
                                       '+refs/tags/*:refs/tags/*',
                                       '+refs/heads/*:refs/remotes/origin/*')
               end
@@ -346,7 +346,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
                 expect(response).to have_gitlab_http_status(:created)
                 expect(json_response['git_info']['refspecs'])
-                  .to contain_exactly("+refs/pipelines/#{pipeline.id}:refs/pipelines/#{pipeline.id}",
+                  .to contain_exactly("+#{pipeline.sha}:refs/pipelines/#{pipeline.id}",
                                       '+refs/tags/*:refs/tags/*',
                                       '+refs/heads/*:refs/remotes/origin/*')
               end