Merge branch 'improve-pipeline-processing' into 'master'

Improve pipeline processing

## What does this MR do?
This works on top of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5295 trying to solve some edge cases introduced by that Merge Request. The fix switches to a state machine which is already a part of `Ci::Pipeline` and uses events with conditional transitions to switch between pipeline states.

This is approach is much more bullet proof and much easier to understand than a previous one where we were calling a `reload_status!` which manually updated `status`. Previous approach become confusing and prone to number of errors.

## Why was this MR needed?
This improves changes introduced by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5295

## What are the relevant issue numbers?
None, yet.

## Screenshots (if relevant)
Not needed.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added (not needed since changelog for https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5295 is already introduced)
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added (not needed)
- Tests
  - [x] Added for this feature/bug (most of tests do cover the triggering of Pipeline)
  - [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5782

5 files changed, 93 insertions, 46 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 88a340379b8..3d6c6ea3209 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -42,24 +42,25 @@ module Ci
       end
 
       def retry(build, user = nil)
-        new_build = Ci::Build.new(status: 'pending')
-        new_build.ref = build.ref
-        new_build.tag = build.tag
-        new_build.options = build.options
-        new_build.commands = build.commands
-        new_build.tag_list = build.tag_list
-        new_build.project = build.project
-        new_build.pipeline = build.pipeline
-        new_build.name = build.name
-        new_build.allow_failure = build.allow_failure
-        new_build.stage = build.stage
-        new_build.stage_idx = build.stage_idx
-        new_build.trigger_request = build.trigger_request
-        new_build.yaml_variables = build.yaml_variables
-        new_build.when = build.when
-        new_build.user = user
-        new_build.environment = build.environment
-        new_build.save
+        new_build = Ci::Build.create(
+          ref: build.ref,
+          tag: build.tag,
+          options: build.options,
+          commands: build.commands,
+          tag_list: build.tag_list,
+          project: build.project,
+          pipeline: build.pipeline,
+          name: build.name,
+          allow_failure: build.allow_failure,
+          stage: build.stage,
+          stage_idx: build.stage_idx,
+          trigger_request: build.trigger_request,
+          yaml_variables: build.yaml_variables,
+          when: build.when,
+          user: user,
+          environment: build.environment,
+          status_event: 'enqueue'
+        )
         MergeRequests::AddTodoWhenBuildFailsService.new(build.project, nil).close(new_build)
         new_build
       end
@@ -101,7 +102,7 @@ module Ci
 
     def play(current_user = nil)
       # Try to queue a current build
-      if self.queue
+      if self.enqueue
         self.update(user: current_user)
         self
       else
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 718fe3290c1..8cfba92ae9b 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -19,6 +19,45 @@ module Ci
 
     after_save :keep_around_commits
 
+    state_machine :status, initial: :created do
+      event :enqueue do
+        transition created: :pending
+        transition [:success, :failed, :canceled, :skipped] => :running
+      end
+
+      event :run do
+        transition any => :running
+      end
+
+      event :skip do
+        transition any => :skipped
+      end
+
+      event :drop do
+        transition any => :failed
+      end
+
+      event :succeed do
+        transition any => :success
+      end
+
+      event :cancel do
+        transition any => :canceled
+      end
+
+      before_transition [:created, :pending] => :running do |pipeline|
+        pipeline.started_at = Time.now
+      end
+
+      before_transition any => [:success, :failed, :canceled] do |pipeline|
+        pipeline.finished_at = Time.now
+      end
+
+      before_transition do |pipeline|
+        pipeline.update_duration
+      end
+    end
+
     # ref can't be HEAD or SHA, can only be branch/tag name
     scope :latest_successful_for, ->(ref = default_branch) do
       where(ref: ref).success.order(id: :desc).limit(1)
@@ -89,16 +128,12 @@ module Ci
 
     def cancel_running
       builds.running_or_pending.each(&:cancel)
-
-      reload_status!
     end
 
     def retry_failed(user)
       builds.latest.failed.select(&:retryable?).each do |build|
         Ci::Build.retry(build, user)
       end
-
-      reload_status!
     end
 
     def latest?
@@ -185,7 +220,17 @@ module Ci
 
     def process!
       Ci::ProcessPipelineService.new(project, user).execute(self)
-      reload_status!
+    end
+
+    def build_updated
+      case latest_builds_status
+      when 'pending' then enqueue
+      when 'running' then run
+      when 'success' then succeed
+      when 'failed' then drop
+      when 'canceled' then cancel
+      when 'skipped' then skip
+      end
     end
 
     def predefined_variables
@@ -194,22 +239,18 @@ module Ci
       ]
     end
 
-    def reload_status!
-      statuses.reload
-      self.status =
-        if yaml_errors.blank?
-          statuses.latest.status || 'skipped'
-        else
-          'failed'
-        end
-      self.started_at = statuses.started_at
-      self.finished_at = statuses.finished_at
+    def update_duration
       self.duration = statuses.latest.duration
-      save
     end
 
     private
 
+    def latest_builds_status
+      return 'failed' unless yaml_errors.blank?
+
+      statuses.latest.status || 'skipped'
+    end
+
     def keep_around_commits
       return unless project
 
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 20713314a25..703ca90edb6 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -26,7 +26,7 @@ class CommitStatus < ActiveRecord::Base
   scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) }
 
   state_machine :status do
-    event :queue do
+    event :enqueue do
       transition [:created, :skipped] => :pending
     end
 
@@ -62,6 +62,17 @@ class CommitStatus < ActiveRecord::Base
       commit_status.update_attributes finished_at: Time.now
     end
 
+    # We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
+    around_transition any => [:success, :failed, :canceled] do |commit_status, block|
+      block.call
+
+      commit_status.pipeline.try(:process!)
+    end
+
+    after_transition do |commit_status, transition|
+      commit_status.pipeline.try(:build_updated) unless transition.loopback?
+    end
+
     after_transition [:created, :pending, :running] => :success do |commit_status|
       MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status)
     end
@@ -69,13 +80,6 @@ class CommitStatus < ActiveRecord::Base
     after_transition any => :failed do |commit_status|
       MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.pipeline.project, nil).execute(commit_status)
     end
-
-    # We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
-    around_transition any => [:success, :failed, :canceled] do |commit_status, block|
-      block.call
-
-      commit_status.pipeline.process! if commit_status.pipeline
-    end
   end
 
   delegate :sha, :short_sha, to: :pipeline
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb
index 7398fd8e10a..cde856b0186 100644
--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -37,7 +37,8 @@ module Ci
       end
 
       if !ignore_skip_ci && skip_ci?
-        return error('Creation of pipeline is skipped', save: save_on_errors)
+        pipeline.skip if save_on_errors
+        return pipeline
       end
 
       unless pipeline.config_builds_attributes.present?
@@ -93,7 +94,7 @@ module Ci
 
     def error(message, save: false)
       pipeline.errors.add(:base, message)
-      pipeline.reload_status! if save
+      pipeline.drop if save
       pipeline
     end
   end
diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb
index 86c4823d18a..6f7610d42ba 100644
--- a/app/services/ci/process_pipeline_service.rb
+++ b/app/services/ci/process_pipeline_service.rb
@@ -37,7 +37,7 @@ module Ci
       return false unless Statuseable::COMPLETED_STATUSES.include?(current_status)
 
       if valid_statuses_for_when(build.when).include?(current_status)
-        build.queue
+        build.enqueue
         true
       else
         build.skip