Merge branch '29651-child-of-child-pipeline' into 'master'

Implement allowing child pipeline to have child pipeline

See merge request gitlab-org/gitlab!41102

app/models/ci/pipeline.rb

@@ -821,11 +821,15 @@ module Ci
       all_merge_requests.order(id: :desc)
     end
 
-    # If pipeline is a child of another pipeline, include the parent
-    # and the siblings, otherwise return only itself and children.
     def same_family_pipeline_ids
-      parent = parent_pipeline || self
-      [parent.id] + parent.child_pipelines.pluck(:id)
+      if ::Gitlab::Ci::Features.child_of_child_pipeline_enabled?(project)
+        ::Gitlab::Ci::PipelineObjectHierarchy.new(base_and_ancestors).base_and_descendants.select(:id)
+      else
+        # If pipeline is a child of another pipeline, include the parent
+        # and the siblings, otherwise return only itself and children.
+        parent = parent_pipeline || self
+        [parent.id] + parent.child_pipelines.pluck(:id)
+      end
     end
 
     def bridge_triggered?
@@ -1058,6 +1062,14 @@ module Ci
       self.ci_ref = Ci::Ref.ensure_for(self)
     end
 
+    def base_and_ancestors
+      # Without using `unscoped`, caller scope is also included into the query.
+      # Using `unscoped` here will be redundant after Rails 6.1
+      ::Gitlab::Ci::PipelineObjectHierarchy
+        .new(self.class.unscoped.where(id: id))
+        .base_and_ancestors
+    end
+
     private
 
     def add_message(severity, content)

app/models/concerns/enums/commit_status.rb

@@ -23,9 +23,10 @@ module Enums
         insufficient_bridge_permissions: 1_001,
         downstream_bridge_project_not_found: 1_002,
         invalid_bridge_trigger: 1_003,
-        bridge_pipeline_is_child_pipeline: 1_006,
+        bridge_pipeline_is_child_pipeline: 1_006, # not used anymore, but cannot be deleted because of old data
         downstream_pipeline_creation_failed: 1_007,
-        secrets_provider_not_found: 1_008
+        secrets_provider_not_found: 1_008,
+        reached_max_descendant_pipelines_depth: 1_009
       }
     end
   end

app/presenters/commit_status_presenter.rb

@@ -20,7 +20,8 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated
     insufficient_bridge_permissions: 'This job could not be executed because of insufficient permissions to create a downstream pipeline',
     bridge_pipeline_is_child_pipeline: 'This job belongs to a child pipeline and cannot create further child pipelines',
     downstream_pipeline_creation_failed: 'The downstream pipeline could not be created',
-    secrets_provider_not_found: 'The secrets provider can not be found'
+    secrets_provider_not_found: 'The secrets provider can not be found',
+    reached_max_descendant_pipelines_depth: 'Maximum child pipeline depth has been reached'
   }.freeze
 
   private_constant :CALLOUT_FAILURE_MESSAGES

app/services/ci/create_downstream_pipeline_service.rb

@@ -9,6 +9,8 @@ module Ci
 
     DuplicateDownstreamPipelineError = Class.new(StandardError)
 
+    MAX_DESCENDANTS_DEPTH = 2
+
     def execute(bridge)
       @bridge = bridge
 
@@ -75,9 +77,16 @@ module Ci
 
       # TODO: Remove this condition if favour of model validation
       # https://gitlab.com/gitlab-org/gitlab/issues/38338
-      if @bridge.triggers_child_pipeline? && @bridge.pipeline.parent_pipeline.present?
-        @bridge.drop!(:bridge_pipeline_is_child_pipeline)
-        return false
+      if ::Gitlab::Ci::Features.child_of_child_pipeline_enabled?(project)
+        if has_max_descendants_depth?
+          @bridge.drop!(:reached_max_descendant_pipelines_depth)
+          return false
+        end
+      else
+        if @bridge.triggers_child_pipeline? && @bridge.pipeline.parent_pipeline.present?
+          @bridge.drop!(:bridge_pipeline_is_child_pipeline)
+          return false
+        end
       end
 
       unless can_create_downstream_pipeline?(target_ref)
@@ -108,5 +117,12 @@ module Ci
         @bridge.downstream_project
       end
     end
+
+    def has_max_descendants_depth?
+      return false unless @bridge.triggers_child_pipeline?
+
+      ancestors_of_new_child = @bridge.pipeline.base_and_ancestors
+      ancestors_of_new_child.count > MAX_DESCENDANTS_DEPTH
+    end
   end
 end

config/feature_flags/development/ci_child_of_child_pipeline.yml

+---
+name: ci_child_of_child_pipeline
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/41102
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/243747
+group: 'group::continuous integration'
+type: development
+default_enabled: false

doc/ci/parent_child_pipelines.md

@@ -152,6 +152,10 @@ This is [resolved in GitLab 12.10](https://gitlab.com/gitlab-org/gitlab/-/issues
 
 ## Limitations
 
-A parent pipeline can trigger many child pipelines, but a child pipeline cannot trigger
-further child pipelines. See the [related issue](https://gitlab.com/gitlab-org/gitlab/-/issues/29651)
-for discussion on possible future improvements.
+In GitLab 13.3 and older, a parent pipeline can trigger many child pipelines, but
+those child pipeline cannot trigger further child pipelines.
+
+In GitLab 13.4 and newer, the [maximum depth of child pipelines was increased to 2](https://gitlab.com/gitlab-org/gitlab/-/issues/29651).
+A parent pipeline can trigger many child pipelines. These child pipelines can trigger
+their own child pipelines. This second layer of child pipelines cannot trigger further
+child pipelines.

lib/gitlab/ci/features.rb

@@ -74,6 +74,10 @@ module Gitlab
       def self.coverage_report_view?(project)
         ::Feature.enabled?(:coverage_report_view, project)
       end
+
+      def self.child_of_child_pipeline_enabled?(project)
+        ::Feature.enabled?(:ci_child_of_child_pipeline, project, default_enabled: false)
+      end
     end
   end
 end

lib/gitlab/ci/pipeline_object_hierarchy.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    class PipelineObjectHierarchy < ::Gitlab::ObjectHierarchy
+      private
+
+      def middle_table
+        ::Ci::Sources::Pipeline.arel_table
+      end
+
+      def from_tables(cte)
+        [objects_table, cte.table, middle_table]
+      end
+
+      def parent_id_column(_cte)
+        middle_table[:source_pipeline_id]
+      end
+
+      def ancestor_conditions(cte)
+        middle_table[:source_pipeline_id].eq(objects_table[:id]).and(
+          middle_table[:pipeline_id].eq(cte.table[:id])
+        )
+      end
+
+      def descendant_conditions(cte)
+        middle_table[:pipeline_id].eq(objects_table[:id]).and(
+          middle_table[:source_pipeline_id].eq(cte.table[:id])
+        )
+      end
+    end
+  end
+end

lib/gitlab/ci/status/build/failed.rb

@@ -25,7 +25,8 @@ module Gitlab
             insufficient_bridge_permissions: 'no permissions to trigger downstream pipeline',
             bridge_pipeline_is_child_pipeline: 'creation of child pipeline not allowed from another child pipeline',
             downstream_pipeline_creation_failed: 'downstream pipeline can not be created',
-            secrets_provider_not_found: 'secrets provider can not be found'
+            secrets_provider_not_found: 'secrets provider can not be found',
+            reached_max_descendant_pipelines_depth: 'reached maximum depth of child pipelines'
           }.freeze
 
           private_constant :REASONS

lib/gitlab/object_hierarchy.rb

@@ -133,8 +133,8 @@ module Gitlab
 
       # Recursively get all the ancestors of the base set.
       parent_query = model
-        .from([objects_table, cte.table])
-        .where(objects_table[:id].eq(cte.table[:parent_id]))
+        .from(from_tables(cte))
+        .where(ancestor_conditions(cte))
         .except(:order)
 
       if hierarchy_order
@@ -148,7 +148,7 @@ module Gitlab
         ).where(cte.table[:tree_cycle].eq(false))
       end
 
-      parent_query = parent_query.where(cte.table[:parent_id].not_eq(stop_id)) if stop_id
+      parent_query = parent_query.where(parent_id_column(cte).not_eq(stop_id)) if stop_id
 
       cte << parent_query
       cte
@@ -166,8 +166,8 @@ module Gitlab
 
       # Recursively get all the descendants of the base set.
       descendants_query = model
-        .from([objects_table, cte.table])
-        .where(objects_table[:parent_id].eq(cte.table[:id]))
+        .from(from_tables(cte))
+        .where(descendant_conditions(cte))
         .except(:order)
 
       if with_depth
@@ -190,6 +190,22 @@ module Gitlab
       model.arel_table
     end
 
+    def parent_id_column(cte)
+      cte.table[:parent_id]
+    end
+
+    def from_tables(cte)
+      [objects_table, cte.table]
+    end
+
+    def ancestor_conditions(cte)
+      objects_table[:id].eq(cte.table[:parent_id])
+    end
+
+    def descendant_conditions(cte)
+      objects_table[:parent_id].eq(cte.table[:id])
+    end
+
     def read_only(relation)
       # relations using a CTE are not safe to use with update_all as it will
       # throw away the CTE, hence we mark them as read-only.

spec/lib/gitlab/ci/pipeline_object_hierarchy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
+  let_it_be(:project) { create_default(:project, :repository) }
+  let_it_be(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) }
+  let_it_be(:ancestor) { create(:ci_pipeline, project: pipeline.project) }
+  let_it_be(:parent) { create(:ci_pipeline, project: pipeline.project) }
+  let_it_be(:child) { create(:ci_pipeline, project: pipeline.project) }
+  let_it_be(:cousin_parent) { create(:ci_pipeline, project: pipeline.project) }
+  let_it_be(:cousin) { create(:ci_pipeline, project: pipeline.project) }
+
+  before_all do
+    create_source_relation(ancestor, parent)
+    create_source_relation(ancestor, cousin_parent)
+    create_source_relation(parent, child)
+    create_source_relation(cousin_parent, cousin)
+  end
+
+  describe '#base_and_ancestors' do
+    it 'includes the base and its ancestors' do
+      relation = described_class.new(::Ci::Pipeline.where(id: parent.id)).base_and_ancestors
+
+      expect(relation).to contain_exactly(ancestor, parent)
+    end
+
+    it 'can find ancestors upto a certain level' do
+      relation = described_class.new(::Ci::Pipeline.where(id: child.id)).base_and_ancestors(upto: ancestor.id)
+
+      expect(relation).to contain_exactly(parent, child)
+    end
+
+    describe 'hierarchy_order option' do
+      let(:relation) do
+        described_class.new(::Ci::Pipeline.where(id: child.id)).base_and_ancestors(hierarchy_order: hierarchy_order)
+      end
+
+      context ':asc' do
+        let(:hierarchy_order) { :asc }
+
+        it 'orders by child to ancestor' do
+          expect(relation).to eq([child, parent, ancestor])
+        end
+      end
+
+      context ':desc' do
+        let(:hierarchy_order) { :desc }
+
+        it 'orders by ancestor to child' do
+          expect(relation).to eq([ancestor, parent, child])
+        end
+      end
+    end
+  end
+
+  describe '#base_and_descendants' do
+    it 'includes the base and its descendants' do
+      relation = described_class.new(::Ci::Pipeline.where(id: parent.id)).base_and_descendants
+
+      expect(relation).to contain_exactly(parent, child)
+    end
+
+    context 'when with_depth is true' do
+      let(:relation) do
+        described_class.new(::Ci::Pipeline.where(id: ancestor.id)).base_and_descendants(with_depth: true)
+      end
+
+      it 'includes depth in the results' do
+        object_depths = {
+          ancestor.id => 1,
+          parent.id => 2,
+          cousin_parent.id => 2,
+          child.id => 3,
+          cousin.id => 3
+        }
+
+        relation.each do |object|
+          expect(object.depth).to eq(object_depths[object.id])
+        end
+      end
+    end
+  end
+
+  describe '#all_objects' do
+    it 'includes its ancestors and descendants' do
+      relation = described_class.new(::Ci::Pipeline.where(id: parent.id)).all_objects
+
+      expect(relation).to contain_exactly(ancestor, parent, child)
+    end
+
+    it 'returns all family tree' do
+      relation = described_class.new(
+        ::Ci::Pipeline.where(id: child.id),
+        described_class.new(::Ci::Pipeline.where(id: child.id)).base_and_ancestors
+      ).all_objects
+
+      expect(relation).to contain_exactly(ancestor, parent, cousin_parent, child, cousin)
+    end
+  end
+
+  private
+
+  def create_source_relation(parent, child)
+    create(:ci_sources_pipeline,
+           source_job: create(:ci_build, pipeline: parent),
+           source_project: parent.project,
+           pipeline: child,
+           project: child.project)
+  end
+end

spec/models/ci/pipeline_spec.rb

@@ -2616,11 +2616,11 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   end
 
   describe '#same_family_pipeline_ids' do
-    subject(:same_family_pipeline_ids) { pipeline.same_family_pipeline_ids }
+    subject { pipeline.same_family_pipeline_ids.map(&:id) }
 
     context 'when pipeline is not child nor parent' do
       it 'returns just the pipeline id' do
-        expect(same_family_pipeline_ids).to contain_exactly(pipeline.id)
+        expect(subject).to contain_exactly(pipeline.id)
       end
     end
 
@@ -2643,7 +2643,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       end
 
       it 'returns parent sibling and self ids' do
-        expect(same_family_pipeline_ids).to contain_exactly(parent.id, pipeline.id, sibling.id)
+        expect(subject).to contain_exactly(parent.id, pipeline.id, sibling.id)
       end
     end
 
@@ -2659,7 +2659,46 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       end
 
       it 'returns self and child ids' do
-        expect(same_family_pipeline_ids).to contain_exactly(pipeline.id, child.id)
+        expect(subject).to contain_exactly(pipeline.id, child.id)
+      end
+    end
+
+    context 'when pipeline is a child of a child pipeline' do
+      let(:ancestor) { create(:ci_pipeline, project: pipeline.project) }
+      let(:parent) { create(:ci_pipeline, project: pipeline.project) }
+      let(:cousin_parent) { create(:ci_pipeline, project: pipeline.project) }
+      let(:cousin) { create(:ci_pipeline, project: pipeline.project) }
+
+      before do
+        create(:ci_sources_pipeline,
+               source_job: create(:ci_build, pipeline: ancestor),
+               source_project: ancestor.project,
+               pipeline: parent,
+               project: parent.project)
+
+        create(:ci_sources_pipeline,
+               source_job: create(:ci_build, pipeline: ancestor),
+               source_project: ancestor.project,
+               pipeline: cousin_parent,
+               project: cousin_parent.project)
+
+        create(:ci_sources_pipeline,
+               source_job: create(:ci_build, pipeline: parent),
+               source_project: parent.project,
+               pipeline: pipeline,
+               project: pipeline.project)
+
+        create(:ci_sources_pipeline,
+               source_job: create(:ci_build, pipeline: cousin_parent),
+               source_project: cousin_parent.project,
+               pipeline: cousin,
+               project: cousin.project)
+      end
+
+      it 'returns all family ids' do
+        expect(subject).to contain_exactly(
+          ancestor.id, parent.id, cousin_parent.id, cousin.id, pipeline.id
+        )
       end
     end
   end

spec/services/ci/create_downstream_pipeline_service_spec.rb

@@ -311,7 +311,7 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
           end
         end
 
-        context 'when upstream pipeline is a child pipeline' do
+        context 'when upstream pipeline is a first descendant pipeline' do
           let!(:pipeline_source) do
             create(:ci_sources_pipeline,
               source_pipeline: create(:ci_pipeline, project: upstream_pipeline.project),
@@ -323,12 +323,53 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
             upstream_pipeline.update!(source: :parent_pipeline)
           end
 
-          it 'does not create a further child pipeline' do
+          it 'creates the pipeline' do
+            expect { service.execute(bridge) }
+              .to change { Ci::Pipeline.count }.by(1)
+
+            expect(bridge.reload).to be_success
+          end
+
+          context 'when FF ci_child_of_child_pipeline is disabled' do
+            before do
+              stub_feature_flags(ci_child_of_child_pipeline: false)
+            end
+
+            it 'does not create a further child pipeline' do
+              expect { service.execute(bridge) }
+                .not_to change { Ci::Pipeline.count }
+
+              expect(bridge.reload).to be_failed
+              expect(bridge.failure_reason).to eq 'bridge_pipeline_is_child_pipeline'
+            end
+          end
+        end
+
+        context 'when upstream pipeline is a second descendant pipeline' do
+          let!(:pipeline_source) do
+            parent_of_upstream_pipeline = create(:ci_pipeline, project: upstream_pipeline.project)
+
+            create(:ci_sources_pipeline,
+              source_pipeline: create(:ci_pipeline, project: upstream_pipeline.project),
+              pipeline: parent_of_upstream_pipeline
+            )
+
+            create(:ci_sources_pipeline,
+              source_pipeline: parent_of_upstream_pipeline,
+              pipeline: upstream_pipeline
+            )
+          end
+
+          before do
+            upstream_pipeline.update!(source: :parent_pipeline)
+          end
+
+          it 'does not create a second descendant pipeline' do
             expect { service.execute(bridge) }
               .not_to change { Ci::Pipeline.count }
 
             expect(bridge.reload).to be_failed
-            expect(bridge.failure_reason).to eq 'bridge_pipeline_is_child_pipeline'
+            expect(bridge.failure_reason).to eq 'reached_max_descendant_pipelines_depth'
           end
         end
       end