Merge branch '351250-parallelize-ci-remote-includes' into 'master'

Add CI remote include parallelization

See merge request gitlab-org/gitlab!136715



Merged-by: Furkan Ayhan <furkanayhn@gmail.com>
Reviewed-by: Kamil Trzciński <ayufan@ayufan.eu>
Reviewed-by: Furkan Ayhan <furkanayhn@gmail.com>

config/feature_flags/development/ci_parallel_remote_includes.yml

+---
+name: ci_parallel_remote_includes
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/136715
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/413770
+milestone: '16.7'
+type: development
+group: group::pipeline authoring
+default_enabled: false

lib/gitlab/ci/config/external/file/remote.rb

@@ -14,9 +14,20 @@ def initialize(params, context)
               super
             end
 
+            def preload_content
+              fetch_async_content
+            end
+
             def content
-              strong_memoize(:content) { fetch_remote_content }
+              fetch_with_error_handling do
+                if fetch_async_content
+                  fetch_async_content.value
+                else
+                  fetch_sync_content
+                end
+              end
             end
+            strong_memoize_attr :content
 
             def metadata
               super.merge(
@@ -42,11 +53,23 @@ def validate_location!
 
             private
 
-            def fetch_remote_content
+            def fetch_async_content
+              return if ::Feature.disabled?(:ci_parallel_remote_includes, context.project)
+
+              # It starts fetching the remote content in a separate thread and returns a promise immediately.
+              Gitlab::HTTP.get(location, async: true).execute
+            end
+            strong_memoize_attr :fetch_async_content
+
+            def fetch_sync_content
+              context.logger.instrument(:config_file_fetch_remote_content) do
+                Gitlab::HTTP.get(location)
+              end
+            end
+
+            def fetch_with_error_handling
               begin
-                response = context.logger.instrument(:config_file_fetch_remote_content) do
-                  Gitlab::HTTP.get(location)
-                end
+                response = yield
               rescue SocketError
                 errors.push("Remote file `#{masked_location}` could not be fetched because of a socket error!")
               rescue Timeout::Error

lib/gitlab/ci/config/external/mapper/verifier.rb

@@ -25,7 +25,7 @@ def process_without_instrumentation(files)
                 file.preload_context if file.valid?
               end
 
-              # We do not combine the loops because we need to load the context of all files via `BatchLoader`.
+              # We do not combine the loops because we need to preload the context of all files via `BatchLoader`.
               files.each do |file| # rubocop:disable Style/CombinableLoops
                 verify_execution_time!
 
@@ -33,7 +33,8 @@ def process_without_instrumentation(files)
                 file.preload_content if file.valid?
               end
 
-              # We do not combine the loops because we need to load the content of all files via `BatchLoader`.
+              # We do not combine the loops because we need to preload the content of all files via `BatchLoader`
+              # or `Concurrent::Promise`.
               files.each do |file| # rubocop:disable Style/CombinableLoops
                 verify_execution_time!
 

spec/lib/gitlab/ci/config/external/file/remote_spec.rb

@@ -75,7 +75,9 @@
 
     context 'with a timeout' do
       before do
-        allow(Gitlab::HTTP).to receive(:get).and_raise(Timeout::Error)
+        allow_next_instance_of(HTTParty::Request) do |instance|
+          allow(instance).to receive(:perform).and_raise(Timeout::Error)
+        end
       end
 
       it { is_expected.to be_falsy }
@@ -94,24 +96,33 @@
     end
   end
 
-  describe "#content" do
+  # When the FF ci_parallel_remote_includes is removed,
+  # convert this `shared_context` to `describe` and remove `rubocop:disable`.
+  shared_context "#content" do # rubocop:disable RSpec/ContextWording -- This is temporary until the FF is removed.
+    subject(:content) do
+      remote_file.preload_content
+      remote_file.content
+    end
+
     context 'with a valid remote file' do
       before do
         stub_full_request(location).to_return(body: remote_file_content)
       end
 
       it 'returns the content of the file' do
-        expect(remote_file.content).to eql(remote_file_content)
+        expect(content).to eql(remote_file_content)
       end
     end
 
     context 'with a timeout' do
       before do
-        allow(Gitlab::HTTP).to receive(:get).and_raise(Timeout::Error)
+        allow_next_instance_of(HTTParty::Request) do |instance|
+          allow(instance).to receive(:perform).and_raise(Timeout::Error)
+        end
       end
 
       it 'is falsy' do
-        expect(remote_file.content).to be_falsy
+        expect(content).to be_falsy
       end
     end
 
@@ -123,7 +134,7 @@
       end
 
       it 'is nil' do
-        expect(remote_file.content).to be_nil
+        expect(content).to be_nil
       end
     end
 
@@ -131,11 +142,21 @@
       let(:location) { 'http://localhost:8080' }
 
       it 'is nil' do
-        expect(remote_file.content).to be_nil
+        expect(content).to be_nil
       end
     end
   end
 
+  it_behaves_like "#content"
+
+  context 'when the FF ci_parallel_remote_includes is disabled' do
+    before do
+      stub_feature_flags(ci_parallel_remote_includes: false)
+    end
+
+    it_behaves_like "#content"
+  end
+
   describe "#error_message" do
     subject(:error_message) do
       Gitlab::Ci::Config::External::Mapper::Verifier.new(context).process([remote_file])
@@ -234,13 +255,18 @@
   end
 
   describe '#to_hash' do
+    subject(:to_hash) do
+      remote_file.preload_content
+      remote_file.to_hash
+    end
+
     before do
       stub_full_request(location).to_return(body: remote_file_content)
     end
 
     context 'with a valid remote file' do
       it 'returns the content as a hash' do
-        expect(remote_file.to_hash).to eql(
+        expect(to_hash).to eql(
           before_script: ["apt-get update -qq && apt-get install -y -qq sqlite3 libsqlite3-dev nodejs",
                           "ruby -v",
                           "which ruby",
@@ -260,7 +286,7 @@
       end
 
       it 'returns the content as a hash' do
-        expect(remote_file.to_hash).to eql(
+        expect(to_hash).to eql(
           include: [
             { local: 'another-file.yml',
               rules: [{ exists: ['Dockerfile'] }] }
@@ -293,7 +319,7 @@
 
       it 'returns the content as a hash' do
         expect(remote_file).to be_valid
-        expect(remote_file.to_hash).to eql(
+        expect(to_hash).to eql(
           include: [
             { local: 'some-file.yml',
               rules: [{ exists: ['Dockerfile'] }] }

spec/lib/gitlab/ci/config/external/mapper_spec.rb

@@ -85,7 +85,13 @@
             an_instance_of(Gitlab::Ci::Config::External::File::Remote))
         end
 
-        it_behaves_like 'logging config file fetch', 'config_file_fetch_remote_content_duration_s', 1
+        context 'when the FF ci_parallel_remote_includes is disabled' do
+          before do
+            stub_feature_flags(ci_parallel_remote_includes: false)
+          end
+
+          it_behaves_like 'logging config file fetch', 'config_file_fetch_remote_content_duration_s', 1
+        end
       end
 
       context 'when the key is a remote file hash' do