Revert create job service because of load balancing

Currently we still need to run EnsureStageService within a transaction,
because when it runs within in a transaction we are going to stick to
the primary database when using database load balancing. Extracting this
out of the transaction makes it possible to hit into problems with
replication lag in pipeline commit status API, which can cause a lot of
trouble.

7 files changed, 25 insertions, 81 deletions

diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 47af20975ec..c0263c0b4e2 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -50,9 +50,7 @@ class CommitStatus < ActiveRecord::Base
   ##
   # We still create some CommitStatuses outside of CreatePipelineService.
   #
-  # These are pages deployments and external statuses. We now handle these
-  # jobs using CreateJobService, but we still need to ensure that a job has
-  # a stage assigned. TODO, In future releases we will add model validation.
+  # These are pages deployments and external statuses.
   #
   before_create unless: :importing? do
     Ci::EnsureStageService.new(project, user).execute(self) do |stage|
diff --git a/app/services/ci/create_job_service.rb b/app/services/ci/create_job_service.rb
deleted file mode 100644
index 7cb77edd690..00000000000
--- a/app/services/ci/create_job_service.rb
+++ /dev/null
@@ -1,12 +0,0 @@
-module Ci
-  class CreateJobService < BaseService
-    def execute(subject = nil)
-      (subject || yield).tap do |subject|
-        Ci::EnsureStageService.new(project, current_user)
-          .execute(subject)
-
-        subject.save
-      end
-    end
-  end
-end
diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb
index 63d2b6c76e6..a773222bf17 100644
--- a/app/services/projects/update_pages_service.rb
+++ b/app/services/projects/update_pages_service.rb
@@ -58,16 +58,14 @@ module Projects
     end
 
     def create_status
-      Ci::CreateJobService.new(project, build.user).execute do
-        GenericCommitStatus.new(
-          project: project,
-          pipeline: build.pipeline,
-          user: build.user,
-          ref: build.ref,
-          stage: 'deploy',
-          name: 'pages:deploy'
-        )
-      end
+      GenericCommitStatus.new(
+        project: project,
+        pipeline: build.pipeline,
+        user: build.user,
+        ref: build.ref,
+        stage: 'deploy',
+        name: 'pages:deploy'
+      )
     end
 
     def extract_archive!(temp_path)
diff --git a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb
index 89ed422ea1c..be5a80261c0 100644
--- a/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb
+++ b/db/post_migrate/20180119121225_remove_redundant_pipeline_stages.rb
@@ -4,6 +4,21 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration
   DOWNTIME = false
 
   def up
+    remove_concurrent_index :ci_stages, [:pipeline_id, :name]
+
+    remove_redundant_pipeline_stages!
+
+    add_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true
+  end
+
+  def down
+    remove_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true
+    add_concurrent_index :ci_stages, [:pipeline_id, :name]
+  end
+
+  private
+
+  def remove_redundant_pipeline_stages!
     redundant_stages_ids = <<~SQL
       SELECT id FROM ci_stages WHERE (pipeline_id, name) IN (
         SELECT pipeline_id, name FROM ci_stages
@@ -27,8 +42,4 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration
       SQL
     end
   end
-
-  def down
-    # noop
-  end
 end
diff --git a/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb b/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb
deleted file mode 100644
index 4fa148b2446..00000000000
--- a/db/post_migrate/20180125111139_add_unique_pipeline_stage_name_index.rb
+++ /dev/null
@@ -1,17 +0,0 @@
-class AddUniquePipelineStageNameIndex < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    remove_concurrent_index :ci_stages, [:pipeline_id, :name]
-    add_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true
-  end
-
-  def down
-    remove_concurrent_index :ci_stages, [:pipeline_id, :name], unique: true
-    add_concurrent_index :ci_stages, [:pipeline_id, :name]
-  end
-end
diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb
index 3ec17c4d9f5..829eef18795 100644
--- a/lib/api/commit_statuses.rb
+++ b/lib/api/commit_statuses.rb
@@ -69,7 +69,6 @@ module API
         name = params[:name] || params[:context] || 'default'
 
         pipeline = @project.pipeline_for(ref, commit.sha)
-
         unless pipeline
           pipeline = @project.pipelines.create!(
             source: :external,
@@ -91,14 +90,7 @@ module API
         optional_attributes =
           attributes_for_keys(%w[target_url description coverage])
 
-        status.assign_attributes(optional_attributes) if optional_attributes.any?
-
-        if status.new_record?
-          Ci::CreateJobService.new(@project, current_user).execute(status)
-        else
-          status.save if status.changed?
-        end
-
+        status.update(optional_attributes) if optional_attributes.any?
         render_validation_error!(status) if status.invalid?
 
         begin
diff --git a/spec/services/ci/create_job_service_spec.rb b/spec/services/ci/create_job_service_spec.rb
deleted file mode 100644
index d9dd1a40325..00000000000
--- a/spec/services/ci/create_job_service_spec.rb
+++ /dev/null
@@ -1,26 +0,0 @@
-require 'spec_helper'
-
-describe Ci::CreateJobService, '#execute' do
-  set(:project) { create(:project, :repository) }
-  let(:user) { create(:admin) }
-  let(:status) { build(:ci_build) }
-  let(:service) { described_class.new(project, user) }
-
-  it 'persists job object instantiated in the block' do
-    expect(service.execute { status }).to be_persisted
-  end
-
-  it 'persists a job instance passed as an argument' do
-    expect(service.execute(status)).to be_persisted
-  end
-
-  it 'ensures that a job has a stage assigned' do
-    expect(service.execute(status).stage_id).to be_present
-  end
-
-  it 'does not raise error if status is invalid' do
-    status.name = nil
-
-    expect(service.execute(status)).not_to be_persisted
-  end
-end