diff --git a/config/feature_flags/development/ci_parallel_remote_includes.yml b/config/feature_flags/development/ci_parallel_remote_includes.yml new file mode 100644 index 0000000000000000000000000000000000000000..c1ac315c22427827653fc53af64c55b047523373 --- /dev/null +++ b/config/feature_flags/development/ci_parallel_remote_includes.yml @@ -0,0 +1,8 @@ +--- +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 diff --git a/lib/gitlab/ci/config/external/file/remote.rb b/lib/gitlab/ci/config/external/file/remote.rb index bc8cebb8c3efe1e82b454383edaf374736907acb..fc90b497f85aa72accfd5f3ee6b5a6c23d0b3884 100644 --- a/lib/gitlab/ci/config/external/file/remote.rb +++ b/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 diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb index 0e296aa0b5b2899bcf98b6182c5c8f3e3f4ae115..3bb0df88803d644abae07a4590a67756ab0d244a 100644 --- a/lib/gitlab/ci/config/external/mapper/verifier.rb +++ b/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! diff --git a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb b/spec/lib/gitlab/ci/config/external/file/remote_spec.rb index f8d3d1019f5e5df57b828ab6dc78107e3dcfa49b..7293e6401129eed961c25c59ad79ca612c711920 100644 --- a/spec/lib/gitlab/ci/config/external/file/remote_spec.rb +++ b/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'] }] } diff --git a/spec/lib/gitlab/ci/config/external/mapper_spec.rb b/spec/lib/gitlab/ci/config/external/mapper_spec.rb index 5f28b45496f82a7178c434ee44d791939645ca29..d67b0ff88959d7cf6d34d95cb64ea3642266f4e1 100644 --- a/spec/lib/gitlab/ci/config/external/mapper_spec.rb +++ b/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