Commit 5f57c7a5 authored by Grzegorz Bizon's avatar Grzegorz Bizon

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.
parent 48b0bc33
...@@ -50,9 +50,7 @@ class CommitStatus < ActiveRecord::Base ...@@ -50,9 +50,7 @@ class CommitStatus < ActiveRecord::Base
## ##
# We still create some CommitStatuses outside of CreatePipelineService. # We still create some CommitStatuses outside of CreatePipelineService.
# #
# These are pages deployments and external statuses. We now handle these # These are pages deployments and external statuses.
# 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.
# #
before_create unless: :importing? do before_create unless: :importing? do
Ci::EnsureStageService.new(project, user).execute(self) do |stage| Ci::EnsureStageService.new(project, user).execute(self) do |stage|
......
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
...@@ -58,7 +58,6 @@ module Projects ...@@ -58,7 +58,6 @@ module Projects
end end
def create_status def create_status
Ci::CreateJobService.new(project, build.user).execute do
GenericCommitStatus.new( GenericCommitStatus.new(
project: project, project: project,
pipeline: build.pipeline, pipeline: build.pipeline,
...@@ -68,7 +67,6 @@ module Projects ...@@ -68,7 +67,6 @@ module Projects
name: 'pages:deploy' name: 'pages:deploy'
) )
end end
end
def extract_archive!(temp_path) def extract_archive!(temp_path)
if artifacts.ends_with?('.tar.gz') || artifacts.ends_with?('.tgz') if artifacts.ends_with?('.tar.gz') || artifacts.ends_with?('.tgz')
......
...@@ -4,6 +4,21 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration ...@@ -4,6 +4,21 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration
DOWNTIME = false DOWNTIME = false
def up 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 redundant_stages_ids = <<~SQL
SELECT id FROM ci_stages WHERE (pipeline_id, name) IN ( SELECT id FROM ci_stages WHERE (pipeline_id, name) IN (
SELECT pipeline_id, name FROM ci_stages SELECT pipeline_id, name FROM ci_stages
...@@ -27,8 +42,4 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration ...@@ -27,8 +42,4 @@ class RemoveRedundantPipelineStages < ActiveRecord::Migration
SQL SQL
end end
end end
def down
# noop
end
end end
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
...@@ -69,7 +69,6 @@ module API ...@@ -69,7 +69,6 @@ module API
name = params[:name] || params[:context] || 'default' name = params[:name] || params[:context] || 'default'
pipeline = @project.pipeline_for(ref, commit.sha) pipeline = @project.pipeline_for(ref, commit.sha)
unless pipeline unless pipeline
pipeline = @project.pipelines.create!( pipeline = @project.pipelines.create!(
source: :external, source: :external,
...@@ -91,14 +90,7 @@ module API ...@@ -91,14 +90,7 @@ module API
optional_attributes = optional_attributes =
attributes_for_keys(%w[target_url description coverage]) attributes_for_keys(%w[target_url description coverage])
status.assign_attributes(optional_attributes) if optional_attributes.any? status.update(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
render_validation_error!(status) if status.invalid? render_validation_error!(status) if status.invalid?
begin begin
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment