From 9803a6eece6ed448a498f4026edaa8fe72a6895f Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Thu, 29 Oct 2020 12:39:53 +0000 Subject: [PATCH] List pipeline reports from self and child pipelines Make the parent pipeline the entry point for listing all the reports generated within its hierarchy as long as `strategy:depend` was used to link pipelines. By surfacing only reports from dependent child pipelines we ensure that a parent pipeline completes always after the child pipelines generating reports. This eliminates race conditions with detached child pipelines. --- app/models/ci/pipeline.rb | 82 +++++++---- ...reports-to-include-child-pipeline-jobs.yml | 5 + ...include_child_pipeline_jobs_in_reports.yml | 7 + lib/gitlab/ci/pipeline_object_hierarchy.rb | 9 ++ spec/models/ci/pipeline_spec.rb | 139 +++++++++++++++--- 5 files changed, 196 insertions(+), 46 deletions(-) create mode 100644 changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml create mode 100644 config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 93f6c3c59e1b66..ad3c656830d742 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -637,30 +637,6 @@ def batch_lookup_report_artifact_for_file_type(file_type) .last end - # This batch loads the latest reports for each CI job artifact - # type (e.g. sast, dast, etc.) in a single SQL query to eliminate - # the need to do N different `job_artifacts.where(file_type: - # X).last` calls. - # - # Return a hash of file type => array of 1 job artifact - def latest_report_artifacts - ::Gitlab::SafeRequestStore.fetch("pipeline:#{self.id}:latest_report_artifacts") do - # Note we use read_attribute(:project_id) to read the project - # ID instead of self.project_id. The latter appears to load - # the Project model. This extra filter doesn't appear to - # affect query plan but included to ensure we don't leak the - # wrong informaiton. - ::Ci::JobArtifact.where( - id: job_artifacts.with_reports - .select('max(ci_job_artifacts.id) as id') - .where(project_id: self.read_attribute(:project_id)) - .group(:file_type) - ) - .preload(:job) - .group_by(&:file_type) - end - end - def has_kubernetes_active? project.deployment_platform&.active? end @@ -922,9 +898,13 @@ def builds_in_self_and_descendants # Without using `unscoped`, caller scope is also included into the query. # Using `unscoped` here will be redundant after Rails 6.1 def self_and_descendants - ::Gitlab::Ci::PipelineObjectHierarchy - .new(self.class.unscoped.where(id: id), options: { same_project: true }) - .base_and_descendants + if parent? + ::Gitlab::Ci::PipelineObjectHierarchy + .new(self.class.unscoped.where(id: id), options: { same_project: true }) + .base_and_descendants + else + self.class.where(id: self) + end end def root_ancestor @@ -950,7 +930,9 @@ def child? end def parent? - child_pipelines.exists? + strong_memoize(:is_parent) do + child_pipelines.exists? + end end def created_successfully? @@ -975,7 +957,11 @@ def latest_builds_with_artifacts end def latest_report_builds(reports_scope = ::Ci::JobArtifact.with_reports) - builds.latest.with_reports(reports_scope) + if include_child_pipeline_jobs_in_reports? + builds_in_self_and_descendants.with_reports(reports_scope).preload(:pipeline) + else + builds.latest.with_reports(reports_scope) + end end def latest_test_report_builds @@ -1213,6 +1199,44 @@ def reset_ancestor_bridges! private + # This batch loads the latest reports for each CI job artifact + # type (e.g. sast, dast, etc.) in a single SQL query to eliminate + # the need to do N different `job_artifacts.where(file_type: + # X).last` calls. + # + # Return a hash of file type => array of 1 job artifact + def latest_report_artifacts + ::Gitlab::SafeRequestStore.fetch("pipeline:#{self.id}:latest_report_artifacts") do + # Note we use read_attribute(:project_id) to read the project + # ID instead of self.project_id. The latter appears to load + # the Project model. This extra filter doesn't appear to + # affect query plan but included to ensure we don't leak the + # wrong informaiton. + ::Ci::JobArtifact.where( + id: job_artifacts_in_self_and_descendants.with_reports + .select('max(ci_job_artifacts.id) as id') + .where(project_id: self.read_attribute(:project_id)) + .group(:file_type) + ) + .preload(:job) + .group_by(&:file_type) + end + end + + def job_artifacts_in_self_and_descendants + if include_child_pipeline_jobs_in_reports? + ::Ci::JobArtifact.where(job: builds_in_self_and_descendants) + else + job_artifacts + end + end + + def include_child_pipeline_jobs_in_reports? + strong_memoize(:include_child_pipeline_jobs_in_reports) do + ::Feature.enabled?(:include_child_pipeline_jobs_in_reports, project, default_enabled: :yaml) + end + end + def add_message(severity, content) return unless Gitlab::Ci::Features.store_pipeline_messages?(project) diff --git a/changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml b/changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml new file mode 100644 index 00000000000000..56531f6fb190b7 --- /dev/null +++ b/changelogs/unreleased/ci-reports-to-include-child-pipeline-jobs.yml @@ -0,0 +1,5 @@ +--- +title: Make parent pipelines list reports from child pipelines in MR widgets +merge_request: 46576 +author: +type: added diff --git a/config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml b/config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml new file mode 100644 index 00000000000000..e00cd22227f388 --- /dev/null +++ b/config/feature_flags/development/include_child_pipeline_jobs_in_reports.yml @@ -0,0 +1,7 @@ +--- +name: include_child_pipeline_jobs_in_reports +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/4657 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/273200 +type: development +group: group::continuous integration +default_enabled: false diff --git a/lib/gitlab/ci/pipeline_object_hierarchy.rb b/lib/gitlab/ci/pipeline_object_hierarchy.rb index de3262b10e06ff..5a2174690c92e4 100644 --- a/lib/gitlab/ci/pipeline_object_hierarchy.rb +++ b/lib/gitlab/ci/pipeline_object_hierarchy.rb @@ -1,5 +1,14 @@ # frozen_string_literal: true +# This class allows to traverse a pipeline hierarchy where +# multiple pipelines are connected with each others through +# bridge jobs (e.g. multi-project or parent-child pipelines). +# +# Available options: +# - `same_project: true` to traverse the hierarchy in the +# context of parent-child pipelines. Use `false` for all +# upstream/downstream pipelines regardless of the project +# module Gitlab module Ci class PipelineObjectHierarchy < ::Gitlab::ObjectHierarchy diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index af6d7ab4250e83..b9335000e7f76e 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -3276,6 +3276,43 @@ def create_build(name, stage_idx) end end + describe '#self_and_descendants' do + subject(:pipelines) { pipeline.self_and_descendants } + + let!(:pipeline) { create(:ci_pipeline, project: project) } + + context 'when pipeline is standalone' do + it 'contains only itself' do + expect(pipelines).to contain_exactly(pipeline) + end + end + + context 'when pipeline is parent of another pipeline' do + let!(:child_pipeline) { create(:ci_pipeline, child_of: pipeline) } + + it 'returns parent and child pipelines' do + expect(pipelines).to contain_exactly(pipeline, child_pipeline) + end + + context 'when child pipeline is parent of another child pipeline' do + let!(:child_of_child_pipeline) { create(:ci_pipeline, child_of: child_pipeline) } + + it 'includes the child of child pipeline' do + expect(pipelines).to contain_exactly(pipeline, child_pipeline, child_of_child_pipeline) + end + end + end + + context 'when pipeline has a parent pipeline' do + let!(:parent_pipeline) { create(:ci_pipeline, project: project) } + let!(:pipeline) { create(:ci_pipeline, child_of: parent_pipeline) } + + it 'does not include the parent' do + expect(pipelines).to contain_exactly(pipeline) + end + end + end + describe '#builds_in_self_and_descendants' do subject(:builds) { pipeline.builds_in_self_and_descendants } @@ -3366,36 +3403,104 @@ def create_build(name, stage_idx) end describe '#batch_lookup_report_artifact_for_file_type' do - context 'with code quality report artifact' do - let(:pipeline) { create(:ci_pipeline, :with_codequality_report, project: project) } + before do + stub_feature_flags(include_child_pipeline_jobs_in_reports: feature_flag_status) + end + + subject(:artifact) { pipeline.batch_lookup_report_artifact_for_file_type(:codequality) } + + let!(:pipeline) { create(:ci_pipeline, :with_codequality_report, project: project) } + + context 'when feature flag `include_child_pipeline_jobs_in_reports` is enabled' do + let(:feature_flag_status) { true } + + context 'with report artifact' do + it 'returns the report artifact' do + expect(artifact).to eq(pipeline.job_artifacts.sample) + end + end + + context 'with report artifact in child pipeline' do + let!(:child_pipeline) { create(:ci_pipeline, :with_codequality_report, child_of: pipeline) } - it "returns the code quality artifact" do - expect(pipeline.batch_lookup_report_artifact_for_file_type(:codequality)).to eq(pipeline.job_artifacts.sample) + it 'returns the child report artifact' do + expect(artifact).to eq(child_pipeline.job_artifacts.sample) + end + end + end + + context 'when feature flag `include_child_pipeline_jobs_in_reports` is disabled' do + let(:feature_flag_status) { false } + + context 'with report artifact' do + it 'returns the report artifact' do + expect(artifact).to eq(pipeline.job_artifacts.sample) + end + end + + context 'with report artifact in dependent child pipeline' do + let!(:child_pipeline) { create(:ci_pipeline, :with_codequality_report, child_of: pipeline) } + + it 'always returns parent report artifact' do + expect(artifact).to eq(pipeline.job_artifacts.sample) + end end end end describe '#latest_report_builds' do - it 'returns build with test artifacts' do - test_build = create(:ci_build, :test_reports, pipeline: pipeline, project: project) - coverage_build = create(:ci_build, :coverage_reports, pipeline: pipeline, project: project) - create(:ci_build, :artifacts, pipeline: pipeline, project: project) + let!(:test_build) { create(:ci_build, :test_reports, pipeline: pipeline) } + let!(:coverage_build) { create(:ci_build, :coverage_reports, pipeline: pipeline) } + let!(:other_build) { create(:ci_build, :artifacts, pipeline: pipeline) } + + shared_examples 'returns report builds from current pipeline' do + it 'returns builds with test artifacts' do + expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build) + end + + it 'filters builds by scope' do + expect(pipeline.latest_report_builds(Ci::JobArtifact.test_reports)).to contain_exactly(test_build) + end - expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build) + it 'only returns not retried builds' do + create(:ci_build, :test_reports, :retried, pipeline: pipeline) + + expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build) + end end - it 'filters builds by scope' do - test_build = create(:ci_build, :test_reports, pipeline: pipeline, project: project) - create(:ci_build, :coverage_reports, pipeline: pipeline, project: project) + context 'when feature flag `include_child_pipeline_jobs_in_reports` is enabled' do + before do + stub_feature_flags(include_child_pipeline_jobs_in_reports: true) + end + + it_behaves_like 'returns report builds from current pipeline' - expect(pipeline.latest_report_builds(Ci::JobArtifact.test_reports)).to contain_exactly(test_build) + context 'when pipeline has reports in child pipelines' do + let(:child_pipeline) { create(:ci_pipeline, child_of: pipeline) } + let!(:child_build) { create(:ci_build, :test_reports, pipeline: child_pipeline) } + + it 'returns buils with reports from parent and child pipelines' do + expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build, child_build) + end + end end - it 'only returns not retried builds' do - test_build = create(:ci_build, :test_reports, pipeline: pipeline, project: project) - create(:ci_build, :test_reports, :retried, pipeline: pipeline, project: project) + context 'when feature flag `include_child_pipeline_jobs_in_reports` is disabled' do + before do + stub_feature_flags(include_child_pipeline_jobs_in_reports: false) + end - expect(pipeline.latest_report_builds).to contain_exactly(test_build) + it_behaves_like 'returns report builds from current pipeline' + + context 'when pipeline has reports in child pipelines' do + let(:child_pipeline) { create(:ci_pipeline, child_of: pipeline) } + let!(:child_build) { create(:ci_build, :test_reports, pipeline: child_pipeline) } + + it 'returns only builds from parent pipeline' do + expect(pipeline.latest_report_builds).to contain_exactly(test_build, coverage_build) + end + end end end -- GitLab