Merge branch 'fix/create-pipeline-with-builds-in-transaction' into 'master'

Create pipeline along with builds in the transation

## What does this MR do?

This MR makes it possible to create pipeline along with all associated builds in the transaction, to avoid having empty pipelines when asynchronous job gets terminated.

This will simplify implementation of `PipelineUnlockWorker` in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6988 and improve reliability of the CI as a whole.

## What are the relevant issue numbers?

Related to #24361

See merge request !7742

5 files changed, 108 insertions, 95 deletions

diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index cde856b0186..e3bc9847200 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -45,9 +45,15 @@ module Ci
         return error('No builds for this pipeline.')
       end
 
-      pipeline.save
-      pipeline.process!
-      pipeline
+      Ci::Pipeline.transaction do
+        pipeline.save
+
+        Ci::CreatePipelineBuildsService
+          .new(project, current_user)
+          .execute(pipeline)
+      end
+
+      pipeline.tap(&:process!)
     end
 
     private
diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb
index 8face432d97..2e028c44d8b 100644
--- a/app/services/ci/process_pipeline_service.rb
+++ b/app/services/ci/process_pipeline_service.rb
@@ -5,10 +5,7 @@ module Ci
     def execute(pipeline)
       @pipeline = pipeline
 
-      # This method will ensure that our pipeline does have all builds for all stages created
-      if created_builds.empty?
-        create_builds!
-      end
+      ensure_created_builds! # TODO, remove me in 9.0
 
       new_builds =
         stage_indexes_of_created_builds.map do |index|
@@ -22,10 +19,6 @@ module Ci
 
     private
 
-    def create_builds!
-      Ci::CreatePipelineBuildsService.new(project, current_user).execute(pipeline)
-    end
-
     def process_stage(index)
       current_status = status_for_prior_stages(index)
 
@@ -76,5 +69,18 @@ module Ci
     def created_builds
       pipeline.builds.created
     end
+
+    # This method is DEPRECATED and should be removed in 9.0.
+    #
+    # We need it to maintain backwards compatibility with  previous versions
+    # when builds were not created within one transaction with the pipeline.
+    #
+    def ensure_created_builds!
+      return if created_builds.any?
+
+      Ci::CreatePipelineBuildsService
+        .new(project, current_user)
+        .execute(pipeline)
+    end
   end
 end
diff --git a/changelogs/unreleased/fix-create-pipeline-with-builds-in-transaction.yml b/changelogs/unreleased/fix-create-pipeline-with-builds-in-transaction.yml
new file mode 100644
index 00000000000..e37841e80c3
--- /dev/null
+++ b/changelogs/unreleased/fix-create-pipeline-with-builds-in-transaction.yml
@@ -0,0 +1,4 @@
+---
+title: Create builds in transaction to avoid empty pipelines
+merge_request: 7742
+author: 
diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb
index ac2a1ba5dff..1735791f644 100644
--- a/spec/factories/ci/pipelines.rb
+++ b/spec/factories/ci/pipelines.rb
@@ -7,26 +7,30 @@ FactoryGirl.define do
     project factory: :empty_project
 
     factory :ci_pipeline_without_jobs do
-      after(:build) do |commit|
-        allow(commit).to receive(:ci_yaml_file) { YAML.dump({}) }
+      after(:build) do |pipeline|
+        allow(pipeline).to receive(:ci_yaml_file) { YAML.dump({}) }
       end
     end
 
     factory :ci_pipeline_with_one_job do
-      after(:build) do |commit|
-        allow(commit).to receive(:ci_yaml_file) { YAML.dump({ rspec: { script: "ls" } }) }
-      end
-    end
-
-    factory :ci_pipeline_with_two_job do
-      after(:build) do |commit|
-        allow(commit).to receive(:ci_yaml_file) { YAML.dump({ rspec: { script: "ls" }, spinach: { script: "ls" } }) }
+      after(:build) do |pipeline|
+        allow(pipeline).to receive(:ci_yaml_file) do
+          YAML.dump({ rspec: { script: "ls" } })
+        end
       end
     end
 
     factory :ci_pipeline do
-      after(:build) do |commit|
-        allow(commit).to receive(:ci_yaml_file) { File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci.yml')) }
+      transient { config nil }
+
+      after(:build) do |pipeline, evaluator|
+        allow(pipeline).to receive(:ci_yaml_file) do
+          if evaluator.config
+            YAML.dump(evaluator.config)
+          else
+            File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci.yml'))
+          end
+        end
       end
     end
   end
diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb
index ff113efd916..ebb11166964 100644
--- a/spec/services/ci/process_pipeline_service_spec.rb
+++ b/spec/services/ci/process_pipeline_service_spec.rb
@@ -1,31 +1,10 @@
 require 'spec_helper'
 
 describe Ci::ProcessPipelineService, services: true do
-  let(:pipeline) { create(:ci_pipeline, ref: 'master') }
+  let(:pipeline) { create(:ci_empty_pipeline, ref: 'master') }
   let(:user) { create(:user) }
-  let(:config) { nil }
-
-  before do
-    allow(pipeline).to receive(:ci_yaml_file).and_return(config)
-  end
 
   describe '#execute' do
-    def all_builds
-      pipeline.builds
-    end
-
-    def builds
-      all_builds.where.not(status: [:created, :skipped])
-    end
-
-    def process_pipeline
-      described_class.new(pipeline.project, user).execute(pipeline)
-    end
-
-    def succeed_pending
-      builds.pending.update_all(status: 'success')
-    end
-
     context 'start queuing next builds' do
       before do
         create(:ci_build, :created, pipeline: pipeline, name: 'linux', stage_idx: 0)
@@ -223,10 +202,6 @@ describe Ci::ProcessPipelineService, services: true do
           pipeline.builds.running_or_pending.each(&:success)
           expect(manual_actions).to be_many # production and clear cache
         end
-
-        def manual_actions
-          pipeline.manual_actions
-        end
       end
     end
 
@@ -282,15 +257,6 @@ describe Ci::ProcessPipelineService, services: true do
           expect(builds.map(&:status)).to eq(%w[success skipped pending])
         end
       end
-
-      def create_build(name, stage_idx, when_value = nil)
-        create(:ci_build,
-               :created,
-               pipeline: pipeline,
-               name: name,
-               stage_idx: stage_idx,
-               when: when_value)
-      end
     end
 
     context 'when failed build in the middle stage is retried' do
@@ -327,65 +293,92 @@ describe Ci::ProcessPipelineService, services: true do
       end
     end
 
-    context 'creates a builds from .gitlab-ci.yml' do
-      let(:config) do
-        YAML.dump({
-          rspec: {
-            stage: 'test',
-            script: 'rspec'
-          },
-          rubocop: {
-            stage: 'test',
-            script: 'rubocop'
-          },
-          deploy: {
-            stage: 'deploy',
-            script: 'deploy'
-          }
-        })
+    context 'when there are builds that are not created yet' do
+      let(:pipeline) do
+        create(:ci_pipeline, config: config)
       end
 
-      # Using stubbed .gitlab-ci.yml created in commit factory
-      #
+      let(:config) do
+        { rspec: { stage: 'test', script: 'rspec' },
+          deploy: { stage: 'deploy', script: 'rsync' } }
+      end
 
       before do
-        stub_ci_pipeline_yaml_file(config)
         create(:ci_build, :created, pipeline: pipeline, name: 'linux', stage: 'build', stage_idx: 0)
         create(:ci_build, :created, pipeline: pipeline, name: 'mac', stage: 'build', stage_idx: 0)
       end
 
-      it 'when processing a pipeline' do
-        # Currently we have two builds with state created
+      it 'processes the pipeline' do
+        # Currently we have five builds with state created
+        #
         expect(builds.count).to eq(0)
         expect(all_builds.count).to eq(2)
 
-        # Create builds will mark the created as pending
-        expect(process_pipeline).to be_truthy
+        # Process builds service will enqueue builds from the first stage.
+        #
+        process_pipeline
+
         expect(builds.count).to eq(2)
         expect(all_builds.count).to eq(2)
 
-        # When we builds succeed we will create a rest of pipeline from .gitlab-ci.yml
-        # We will have 2 succeeded, 2 pending (from stage test), total 5 (one more build from deploy)
+        # When builds succeed we will enqueue remaining builds.
+        #
+        # We will have 2 succeeded, 1 pending (from stage test), total 4 (two
+        # additional build from `.gitlab-ci.yml`).
+        #
         succeed_pending
-        expect(process_pipeline).to be_truthy
+        process_pipeline
+
         expect(builds.success.count).to eq(2)
-        expect(builds.pending.count).to eq(2)
-        expect(all_builds.count).to eq(5)
+        expect(builds.pending.count).to eq(1)
+        expect(all_builds.count).to eq(4)
 
-        # When we succeed the 2 pending from stage test,
-        # We will queue a deploy stage, no new builds will be created
+        # When pending build succeeds in stage test, we enqueue deploy stage.
+        #
         succeed_pending
-        expect(process_pipeline).to be_truthy
+        process_pipeline
+
         expect(builds.pending.count).to eq(1)
-        expect(builds.success.count).to eq(4)
-        expect(all_builds.count).to eq(5)
+        expect(builds.success.count).to eq(3)
+        expect(all_builds.count).to eq(4)
 
-        # When we succeed last pending build, we will have a total of 5 succeeded builds, no new builds will be created
+        # When the last one succeeds we have 4 successful builds.
+        #
         succeed_pending
-        expect(process_pipeline).to be_falsey
-        expect(builds.success.count).to eq(5)
-        expect(all_builds.count).to eq(5)
+        process_pipeline
+
+        expect(builds.success.count).to eq(4)
+        expect(all_builds.count).to eq(4)
       end
     end
   end
+
+  def all_builds
+    pipeline.builds
+  end
+
+  def builds
+    all_builds.where.not(status: [:created, :skipped])
+  end
+
+  def process_pipeline
+    described_class.new(pipeline.project, user).execute(pipeline)
+  end
+
+  def succeed_pending
+    builds.pending.update_all(status: 'success')
+  end
+
+  def manual_actions
+    pipeline.manual_actions
+  end
+
+  def create_build(name, stage_idx, when_value = nil)
+    create(:ci_build,
+           :created,
+           pipeline: pipeline,
+           name: name,
+           stage_idx: stage_idx,
+           when: when_value)
+  end
 end