diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index c4d5e7a12474f15b95dcb4bb82418f5be716c271..0420bb13b81a6f7a92c6dce49e14cd7b4d4304bc 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -978,7 +978,7 @@ def self_and_upstreams end # With multi-project and parent-child pipelines - def self_with_upstreams_and_downstreams + def all_pipelines_in_hierarchy object_hierarchy.all_objects end @@ -992,6 +992,7 @@ def self_and_descendants object_hierarchy(project_condition: :same).base_and_descendants end + # Follow the parent-child relationships and return the top-level parent def root_ancestor return self unless child? @@ -1000,6 +1001,12 @@ def root_ancestor .first end + # Follow the upstream pipeline relationships, regardless of multi-project or + # parent-child, and return the top-level ancestor. + def upstream_root + object_hierarchy.base_and_ancestors(hierarchy_order: :desc).first + end + def bridge_triggered? source_bridge.present? end diff --git a/app/services/ci/create_downstream_pipeline_service.rb b/app/services/ci/create_downstream_pipeline_service.rb index 0a0c614bb8786422e32ef0f115df71f8c64853c5..b38b3b933533529a4b88518e95c3d454730d87b0 100644 --- a/app/services/ci/create_downstream_pipeline_service.rb +++ b/app/services/ci/create_downstream_pipeline_service.rb @@ -6,6 +6,7 @@ module Ci # specifications. class CreateDownstreamPipelineService < ::BaseService include Gitlab::Utils::StrongMemoize + include Ci::DownstreamPipelineHelpers DuplicateDownstreamPipelineError = Class.new(StandardError) @@ -37,6 +38,8 @@ def execute(bridge) .execute(pipeline_params.fetch(:source), **pipeline_params[:execute_params]) .payload + log_downstream_pipeline_creation(downstream_pipeline) + downstream_pipeline.tap do |pipeline| update_bridge_status!(@bridge, pipeline) end diff --git a/app/services/ci/expire_pipeline_cache_service.rb b/app/services/ci/expire_pipeline_cache_service.rb index 8622b1a5863f1870d9a20b20ad8465a38af807c1..bf2355c447afe3ac48eff887ef2b24bb269c267b 100644 --- a/app/services/ci/expire_pipeline_cache_service.rb +++ b/app/services/ci/expire_pipeline_cache_service.rb @@ -86,7 +86,7 @@ def update_etag_cache(pipeline, store) etag_paths << path end - pipeline.self_with_upstreams_and_downstreams.includes(project: [:route, { namespace: :route }]).each do |relative_pipeline| # rubocop: disable CodeReuse/ActiveRecord + pipeline.all_pipelines_in_hierarchy.includes(project: [:route, { namespace: :route }]).each do |relative_pipeline| # rubocop: disable CodeReuse/ActiveRecord etag_paths << project_pipeline_path(relative_pipeline.project, relative_pipeline) etag_paths << graphql_pipeline_path(relative_pipeline) etag_paths << graphql_pipeline_sha_path(relative_pipeline.sha) diff --git a/app/services/ci/pipeline_trigger_service.rb b/app/services/ci/pipeline_trigger_service.rb index 7746382b845b8de3ea796e9f53d7fac2b530ac1e..06eb1aee8e67d62fa62d78f74a2000240737733f 100644 --- a/app/services/ci/pipeline_trigger_service.rb +++ b/app/services/ci/pipeline_trigger_service.rb @@ -4,6 +4,7 @@ module Ci class PipelineTriggerService < BaseService include Gitlab::Utils::StrongMemoize include Services::ReturnServiceResponses + include Ci::DownstreamPipelineHelpers def execute if trigger_from_token @@ -69,6 +70,7 @@ def create_pipeline_from_job(job) pipeline.source_pipeline = source end + log_downstream_pipeline_creation(response.payload) pipeline_service_response(response.payload) end diff --git a/app/services/concerns/ci/downstream_pipeline_helpers.rb b/app/services/concerns/ci/downstream_pipeline_helpers.rb new file mode 100644 index 0000000000000000000000000000000000000000..b738a33208596c6e71af741fa1cf5189ab7a7a45 --- /dev/null +++ b/app/services/concerns/ci/downstream_pipeline_helpers.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Ci + module DownstreamPipelineHelpers + def log_downstream_pipeline_creation(downstream_pipeline) + return unless Feature.enabled?(:ci_log_downstream_pipeline_creation, project, default_enabled: :yaml) + return unless downstream_pipeline&.persisted? + + hierarchy_size = downstream_pipeline.all_pipelines_in_hierarchy.count + root_pipeline = downstream_pipeline.upstream_root + + ::Gitlab::AppLogger.info( + message: "downstream pipeline created", + class: self.class.name, + root_pipeline_id: root_pipeline.id, + downstream_pipeline_id: downstream_pipeline.id, + downstream_pipeline_relationship: downstream_pipeline.parent_pipeline? ? :parent_child : :multi_project, + hierarchy_size: hierarchy_size, + root_pipeline_plan: root_pipeline.project.actual_plan_name, + root_pipeline_namespace_path: root_pipeline.project.namespace.full_path, + root_pipeline_project_path: root_pipeline.project.full_path + ) + end + end +end diff --git a/config/feature_flags/development/ci_log_downstream_pipeline_creation.yml b/config/feature_flags/development/ci_log_downstream_pipeline_creation.yml new file mode 100644 index 0000000000000000000000000000000000000000..340055afd8cd52ca38c52c5fa59fb45cb31ae659 --- /dev/null +++ b/config/feature_flags/development/ci_log_downstream_pipeline_creation.yml @@ -0,0 +1,8 @@ +--- +name: ci_log_downstream_pipeline_creation +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84760 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/358425 +milestone: '15.0' +type: development +group: group::pipeline execution +default_enabled: false diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index f6ad5efc09f4071dffe46fbf6e76efc34ec9500b..c90d10bc9a2dbced33de11f3e8aa77c6cea58f0c 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -3443,6 +3443,46 @@ def create_pipeline(status, ref, sha) end end + describe '#upstream_root' do + subject { pipeline.upstream_root } + + let_it_be(:pipeline) { create(:ci_pipeline) } + + context 'when pipeline is child of child pipeline' do + let!(:root_ancestor) { create(:ci_pipeline) } + let!(:parent_pipeline) { create(:ci_pipeline, child_of: root_ancestor) } + let!(:pipeline) { create(:ci_pipeline, child_of: parent_pipeline) } + + it 'returns the root ancestor' do + expect(subject).to eq(root_ancestor) + end + end + + context 'when pipeline is root ancestor' do + let!(:child_pipeline) { create(:ci_pipeline, child_of: pipeline) } + + it 'returns itself' do + expect(subject).to eq(pipeline) + end + end + + context 'when pipeline is standalone' do + it 'returns itself' do + expect(subject).to eq(pipeline) + end + end + + context 'when pipeline is multi-project downstream pipeline' do + let!(:upstream_pipeline) do + create(:ci_pipeline, project: create(:project), upstream_of: pipeline) + end + + it 'returns the upstream pipeline' do + expect(subject).to eq(upstream_pipeline) + end + end + end + describe '#stuck?' do let(:pipeline) { create(:ci_empty_pipeline, :created) } diff --git a/spec/services/ci/create_downstream_pipeline_service_spec.rb b/spec/services/ci/create_downstream_pipeline_service_spec.rb index 6142704b00ee8bb54b431134ee65d6d120e1fdab..11fb564b843264a2f17eb5ae73562f3f911e0386 100644 --- a/spec/services/ci/create_downstream_pipeline_service_spec.rb +++ b/spec/services/ci/create_downstream_pipeline_service_spec.rb @@ -35,18 +35,20 @@ upstream_project.add_developer(user) end + subject { service.execute(bridge) } + context 'when downstream project has not been found' do let(:trigger) do { trigger: { project: 'unknown/project' } } end it 'does not create a pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes pipeline bridge job status to failed' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason) @@ -56,12 +58,12 @@ context 'when user can not access downstream project' do it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason) @@ -75,12 +77,12 @@ end it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'insufficient_bridge_permissions' @@ -96,12 +98,12 @@ end it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a new pipeline in a downstream project' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.user).to eq bridge.user expect(pipeline.project).to eq downstream_project @@ -111,8 +113,14 @@ expect(pipeline.source_bridge).to be_a ::Ci::Bridge end + it_behaves_like 'logs downstream pipeline creation' do + let(:expected_root_pipeline) { upstream_pipeline } + let(:expected_hierarchy_size) { 2 } + let(:expected_downstream_relationship) { :multi_project } + end + it 'updates bridge status when downstream pipeline gets processed' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_created expect(bridge.reload).to be_success @@ -136,7 +144,7 @@ bridge_id: bridge.id, project_id: bridge.project.id) .and_call_original expect(Ci::CreatePipelineService).not_to receive(:new) - expect(service.execute(bridge)).to eq({ message: "Already has a downstream pipeline", status: :error }) + expect(subject).to eq({ message: "Already has a downstream pipeline", status: :error }) end end @@ -146,7 +154,7 @@ end it 'is using default branch name' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.ref).to eq 'master' end @@ -158,12 +166,12 @@ end it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a new pipeline in a downstream project' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.user).to eq bridge.user expect(pipeline.project).to eq downstream_project @@ -174,7 +182,7 @@ end it 'updates the bridge status when downstream pipeline gets processed' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_failed expect(bridge.reload).to be_failed @@ -188,12 +196,12 @@ context 'detects a circular dependency' do it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'invalid_bridge_trigger' @@ -209,12 +217,12 @@ shared_examples 'creates a child pipeline' do it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a child pipeline in the same project' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.reload expect(pipeline.builds.map(&:name)).to match_array(%w[rspec echo]) @@ -227,14 +235,14 @@ end it 'updates bridge status when downstream pipeline gets processed' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_created expect(bridge.reload).to be_success end it 'propagates parent pipeline settings to the child pipeline' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.reload expect(pipeline.ref).to eq(upstream_pipeline.ref) @@ -264,8 +272,14 @@ it_behaves_like 'creates a child pipeline' + it_behaves_like 'logs downstream pipeline creation' do + let(:expected_root_pipeline) { upstream_pipeline } + let(:expected_hierarchy_size) { 2 } + let(:expected_downstream_relationship) { :parent_child } + end + it 'updates the bridge job to success' do - expect { service.execute(bridge) }.to change { bridge.status }.to 'success' + expect { subject }.to change { bridge.status }.to 'success' end context 'when bridge uses "depend" strategy' do @@ -276,7 +290,7 @@ end it 'does not update the bridge job status' do - expect { service.execute(bridge) }.not_to change { bridge.status } + expect { subject }.not_to change { bridge.status } end end @@ -306,7 +320,7 @@ it_behaves_like 'creates a child pipeline' it 'propagates the merge request to the child pipeline' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.merge_request).to eq(merge_request) expect(pipeline).to be_merge_request @@ -322,11 +336,17 @@ end it 'creates the pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) expect(bridge.reload).to be_success end + + it_behaves_like 'logs downstream pipeline creation' do + let(:expected_root_pipeline) { upstream_pipeline.parent_pipeline } + let(:expected_hierarchy_size) { 3 } + let(:expected_downstream_relationship) { :parent_child } + end end context 'when upstream pipeline has a parent pipeline, which has a parent pipeline' do @@ -345,7 +365,7 @@ end it 'does not create a second descendant pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } expect(bridge.reload).to be_failed @@ -370,7 +390,7 @@ end it 'create the pipeline' do - expect { service.execute(bridge) }.to change { Ci::Pipeline.count }.by(1) + expect { subject }.to change { Ci::Pipeline.count }.by(1) end end @@ -382,11 +402,11 @@ end it 'creates a new pipeline allowing variables to be passed downstream' do - expect { service.execute(bridge) }.to change { Ci::Pipeline.count }.by(1) + expect { subject }.to change { Ci::Pipeline.count }.by(1) end it 'passes variables downstream from the bridge' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.map(&:key).tap do |variables| expect(variables).to include 'BRIDGE' @@ -444,12 +464,12 @@ describe 'cyclical dependency detection' do shared_examples 'detects cyclical pipelines' do it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'pipeline_loop_detected' @@ -458,12 +478,12 @@ shared_examples 'passes cyclical pipeline precondition' do it 'creates a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count } end it 'expect bridge build not to be failed' do - service.execute(bridge) + subject expect(bridge.reload).not_to be_failed end @@ -537,19 +557,19 @@ end it 'creates only one new pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change { Ci::Pipeline.count }.by(1) end it 'creates a new pipeline in the downstream project' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.user).to eq bridge.user expect(pipeline.project).to eq downstream_project end it 'drops the bridge' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.reload).to be_failed expect(bridge.reload).to be_failed @@ -573,7 +593,7 @@ bridge_id: bridge.id, downstream_pipeline_id: kind_of(Numeric)) - service.execute(bridge) + subject end end @@ -583,7 +603,7 @@ end it 'passes bridge variables to downstream pipeline' do - pipeline = service.execute(bridge) + pipeline = subject expect(pipeline.variables.first) .to have_attributes(key: 'BRIDGE', value: 'var') @@ -596,7 +616,7 @@ end it 'does not pass pipeline variables directly downstream' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.map(&:key).tap do |variables| expect(variables).not_to include 'PIPELINE_VARIABLE' @@ -609,7 +629,7 @@ end it 'makes it possible to pass pipeline variable downstream' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.find_by(key: 'BRIDGE').tap do |variable| expect(variable.value).to eq 'my-value-var' @@ -622,12 +642,12 @@ end it 'does not create a new pipeline' do - expect { service.execute(bridge) } + expect { subject } .not_to change { Ci::Pipeline.count } end it 'ignores variables passed downstream from the bridge' do - pipeline = service.execute(bridge) + pipeline = subject pipeline.variables.map(&:key).tap do |variables| expect(variables).not_to include 'BRIDGE' @@ -635,7 +655,7 @@ end it 'sets errors', :aggregate_failures do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') @@ -679,7 +699,7 @@ context 'that include the bridge job' do it 'creates the downstream pipeline' do - expect { service.execute(bridge) } + expect { subject } .to change(downstream_project.ci_pipelines, :count).by(1) end end @@ -692,7 +712,7 @@ end it 'changes status of the bridge build' do - service.execute(bridge) + subject expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq 'insufficient_bridge_permissions' @@ -710,7 +730,7 @@ end it 'does not create a pipeline and drops the bridge' do - expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + expect { subject }.not_to change(downstream_project.ci_pipelines, :count) expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') @@ -733,7 +753,7 @@ end it 'does not create a pipeline and drops the bridge' do - expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + expect { subject }.not_to change(downstream_project.ci_pipelines, :count) expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') @@ -755,7 +775,7 @@ end it 'creates the pipeline but drops the bridge' do - expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1) + expect { subject }.to change(downstream_project.ci_pipelines, :count).by(1) expect(bridge.reload).to be_failed expect(bridge.failure_reason).to eq('downstream_pipeline_creation_failed') @@ -787,7 +807,7 @@ end it 'creates the pipeline' do - expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1) + expect { subject }.to change(downstream_project.ci_pipelines, :count).by(1) expect(bridge.reload).to be_success end @@ -795,7 +815,7 @@ context 'when not passing the required variable' do it 'does not create the pipeline' do - expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count) + expect { subject }.not_to change(downstream_project.ci_pipelines, :count) end end end diff --git a/spec/services/ci/pipeline_trigger_service_spec.rb b/spec/services/ci/pipeline_trigger_service_spec.rb index 29d12b0dd0ed035a881c1ac6fdde65e737a1ff79..a794dedc658b5732b619b97f17730c7dff5db492 100644 --- a/spec/services/ci/pipeline_trigger_service_spec.rb +++ b/spec/services/ci/pipeline_trigger_service_spec.rb @@ -187,6 +187,14 @@ expect(result[:status]).to eq(:success) end + it_behaves_like 'logs downstream pipeline creation' do + subject { result[:pipeline] } + + let(:expected_root_pipeline) { pipeline } + let(:expected_hierarchy_size) { 2 } + let(:expected_downstream_relationship) { :multi_project } + end + context 'when commit message has [ci skip]' do before do allow_next_instance_of(Ci::Pipeline) do |instance| diff --git a/spec/support/shared_examples/ci/log_downstream_pipeline_shared_examples.rb b/spec/support/shared_examples/ci/log_downstream_pipeline_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..1ccd41c31feb74ea81f6b5cb09cd6eafc558be5e --- /dev/null +++ b/spec/support/shared_examples/ci/log_downstream_pipeline_shared_examples.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'logs downstream pipeline creation' do + def record_downstream_pipeline_logs + logs = [] + allow(::Gitlab::AppLogger).to receive(:info) do |args| + logs << args + end + + yield + + logs.find { |log| log[:message] == "downstream pipeline created" } + end + + it 'logs details' do + pipeline = nil + + log_entry = record_downstream_pipeline_logs do + pipeline = subject + end + + expect(log_entry).to be_present + expect(log_entry).to eq( + message: "downstream pipeline created", + class: described_class.name, + root_pipeline_id: expected_root_pipeline.id, + downstream_pipeline_id: pipeline.id, + downstream_pipeline_relationship: expected_downstream_relationship, + hierarchy_size: expected_hierarchy_size, + root_pipeline_plan: expected_root_pipeline.project.actual_plan_name, + root_pipeline_namespace_path: expected_root_pipeline.project.namespace.full_path, + root_pipeline_project_path: expected_root_pipeline.project.full_path) + end + + context 'when feature flag ci_log_downstream_pipeline_creation is disabled' do + before do + stub_feature_flags(ci_log_downstream_pipeline_creation: false) + end + + it 'does not log the creation' do + log_entry = record_downstream_pipeline_logs { subject } + + expect(log_entry).to be_nil + end + end +end