Merge branch 'loosen-cyclical-pipelines-precondition' into 'master'

Loosen rule to detect cyclical pipelines

See merge request gitlab-org/gitlab!78171

app/services/ci/create_downstream_pipeline_service.rb

@@ -125,7 +125,9 @@ module Ci
           config_checksum(pipeline) unless pipeline.child?
         end
 
-        pipeline_checksums.uniq.length != pipeline_checksums.length
+        # To avoid false positives we allow 1 cycle in the ancestry and
+        # fail when 2 cycles are detected: A -> B -> A -> B -> A
+        pipeline_checksums.tally.any? { |_checksum, occurrences| occurrences > 2 }
       end
     end
 
@@ -137,7 +139,7 @@ module Ci
     end
 
     def config_checksum(pipeline)
-      [pipeline.project_id, pipeline.ref].hash
+      [pipeline.project_id, pipeline.ref, pipeline.source].hash
     end
   end
 end

spec/services/ci/create_downstream_pipeline_service_spec.rb

@@ -441,44 +441,99 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
       end
     end
 
-    context 'when relationship between pipelines is cyclical' do
-      before do
-        pipeline_a = create(:ci_pipeline, project: upstream_project)
-        pipeline_b = create(:ci_pipeline, project: downstream_project)
-        pipeline_c = create(:ci_pipeline, project: upstream_project)
+    describe 'cyclical dependency detection' do
+      shared_examples 'detects cyclical pipelines' do
+        it 'does not create a new pipeline' do
+          expect { service.execute(bridge) }
+            .not_to change { Ci::Pipeline.count }
+        end
+
+        it 'changes status of the bridge build' do
+          service.execute(bridge)
 
-        create_source_pipeline(pipeline_a, pipeline_b)
-        create_source_pipeline(pipeline_b, pipeline_c)
-        create_source_pipeline(pipeline_c, upstream_pipeline)
+          expect(bridge.reload).to be_failed
+          expect(bridge.failure_reason).to eq 'pipeline_loop_detected'
+        end
       end
 
-      it 'does not create a new pipeline' do
-        expect { service.execute(bridge) }
-          .not_to change { Ci::Pipeline.count }
+      shared_examples 'passes cyclical pipeline precondition' do
+        it 'creates a new pipeline' do
+          expect { service.execute(bridge) }
+            .to change { Ci::Pipeline.count }
+        end
+
+        it 'expect bridge build not to be failed' do
+          service.execute(bridge)
+
+          expect(bridge.reload).not_to be_failed
+        end
       end
 
-      it 'changes status of the bridge build' do
-        service.execute(bridge)
+      context 'when pipeline ancestry contains 2 cycles of dependencies' do
+        before do
+          # A(push on master) -> B(pipeline on master) -> A(push on master) ->
+          #   B(pipeline on master) -> A(push on master)
+          pipeline_1 = create(:ci_pipeline, project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, project: downstream_project, source: :pipeline)
+          pipeline_3 = create(:ci_pipeline, project: upstream_project, source: :push)
+          pipeline_4 = create(:ci_pipeline, project: downstream_project, source: :pipeline)
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, pipeline_3)
+          create_source_pipeline(pipeline_3, pipeline_4)
+          create_source_pipeline(pipeline_4, upstream_pipeline)
+        end
 
-        expect(bridge.reload).to be_failed
-        expect(bridge.failure_reason).to eq 'pipeline_loop_detected'
+        it_behaves_like 'detects cyclical pipelines'
+
+        context 'when ci_drop_cyclical_triggered_pipelines is not enabled' do
+          before do
+            stub_feature_flags(ci_drop_cyclical_triggered_pipelines: false)
+          end
+
+          it_behaves_like 'passes cyclical pipeline precondition'
+        end
       end
 
-      context 'when ci_drop_cyclical_triggered_pipelines is not enabled' do
+      context 'when source in the ancestry differ' do
         before do
-          stub_feature_flags(ci_drop_cyclical_triggered_pipelines: false)
+          # A(push on master) -> B(pipeline on master) -> A(pipeline on master)
+          pipeline_1 = create(:ci_pipeline, project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, project: downstream_project, source: :pipeline)
+          upstream_pipeline.update!(source: :pipeline)
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, upstream_pipeline)
         end
 
-        it 'creates a new pipeline' do
-          expect { service.execute(bridge) }
-            .to change { Ci::Pipeline.count }
+        it_behaves_like 'passes cyclical pipeline precondition'
+      end
+
+      context 'when ref in the ancestry differ' do
+        before do
+          # A(push on master) -> B(pipeline on master) -> A(push on feature-1)
+          pipeline_1 = create(:ci_pipeline, ref: 'master', project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, ref: 'master', project: downstream_project, source: :pipeline)
+          upstream_pipeline.update!(ref: 'feature-1')
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, upstream_pipeline)
         end
 
-        it 'expect bridge build not to be failed' do
-          service.execute(bridge)
+        it_behaves_like 'passes cyclical pipeline precondition'
+      end
 
-          expect(bridge.reload).not_to be_failed
+      context 'when only 1 cycle is detected' do
+        before do
+          # A(push on master) -> B(pipeline on master) -> A(push on master)
+          pipeline_1 = create(:ci_pipeline, ref: 'master', project: upstream_project, source: :push)
+          pipeline_2 = create(:ci_pipeline, ref: 'master', project: downstream_project, source: :pipeline)
+
+          create_source_pipeline(pipeline_1, pipeline_2)
+          create_source_pipeline(pipeline_2, upstream_pipeline)
         end
+
+        it_behaves_like 'passes cyclical pipeline precondition'
       end
     end