Merge branch 'fix/async-pipeline-processing-stale-data' into 'master'
Fix ci pipeline processing with async jobs
## What does this MR do?
This MR fixes CI pipeline processing with asynchronous jobs called from `around_transition` provided by state machine.
For details see https://github.com/pluginaweek/state_machine/issues/191 and commit f68acba
.
## Why was this MR needed?
Recently merged https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6650 introduced problem with asynchronous job being deployed when `around_transition` still held transaction, which caused race condition that prevented pipeline status to change in a proper way.
## What are the relevant issue numbers?
Closes #23111
See merge request !6736
This commit is contained in:
commit
359b4f9a03
3 changed files with 41 additions and 15 deletions
|
@ -1,6 +1,7 @@
|
|||
class CommitStatus < ActiveRecord::Base
|
||||
include HasStatus
|
||||
include Importable
|
||||
include AfterCommitQueue
|
||||
|
||||
self.table_name = 'ci_builds'
|
||||
|
||||
|
@ -85,25 +86,34 @@ class CommitStatus < ActiveRecord::Base
|
|||
end
|
||||
|
||||
after_transition do |commit_status, transition|
|
||||
commit_status.pipeline.try do |pipeline|
|
||||
break if transition.loopback?
|
||||
return if transition.loopback?
|
||||
|
||||
if commit_status.complete?
|
||||
ProcessPipelineWorker.perform_async(pipeline.id)
|
||||
commit_status.run_after_commit do
|
||||
pipeline.try do |pipeline|
|
||||
if complete?
|
||||
ProcessPipelineWorker.perform_async(pipeline.id)
|
||||
else
|
||||
UpdatePipelineWorker.perform_async(pipeline.id)
|
||||
end
|
||||
end
|
||||
|
||||
UpdatePipelineWorker.perform_async(pipeline.id)
|
||||
end
|
||||
|
||||
true
|
||||
end
|
||||
|
||||
after_transition [:created, :pending, :running] => :success do |commit_status|
|
||||
MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status)
|
||||
commit_status.run_after_commit do
|
||||
# TODO, temporary fix for race condition
|
||||
UpdatePipelineWorker.new.perform(pipeline.id)
|
||||
|
||||
MergeRequests::MergeWhenBuildSucceedsService
|
||||
.new(pipeline.project, nil).trigger(self)
|
||||
end
|
||||
end
|
||||
|
||||
after_transition any => :failed do |commit_status|
|
||||
MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.pipeline.project, nil).execute(commit_status)
|
||||
commit_status.run_after_commit do
|
||||
MergeRequests::AddTodoWhenBuildFailsService
|
||||
.new(pipeline.project, nil).execute(self)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -16,6 +16,8 @@ module Ci
|
|||
process_stage(index)
|
||||
end
|
||||
|
||||
@pipeline.update_status
|
||||
|
||||
# Return a flag if a when builds got enqueued
|
||||
new_builds.flatten.any?
|
||||
end
|
||||
|
|
|
@ -110,9 +110,21 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
|
|||
|
||||
context 'properly handles multiple stages' do
|
||||
let(:ref) { mr_merge_if_green_enabled.source_branch }
|
||||
let!(:build) { create(:ci_build, :created, pipeline: pipeline, ref: ref, name: 'build', stage: 'build') }
|
||||
let!(:test) { create(:ci_build, :created, pipeline: pipeline, ref: ref, name: 'test', stage: 'test') }
|
||||
let(:pipeline) { create(:ci_empty_pipeline, ref: mr_merge_if_green_enabled.source_branch, project: project) }
|
||||
let(:sha) { project.commit(ref).id }
|
||||
|
||||
let(:pipeline) do
|
||||
create(:ci_empty_pipeline, ref: ref, sha: sha, project: project)
|
||||
end
|
||||
|
||||
let!(:build) do
|
||||
create(:ci_build, :created, pipeline: pipeline, ref: ref,
|
||||
name: 'build', stage: 'build')
|
||||
end
|
||||
|
||||
let!(:test) do
|
||||
create(:ci_build, :created, pipeline: pipeline, ref: ref,
|
||||
name: 'test', stage: 'test')
|
||||
end
|
||||
|
||||
before do
|
||||
# This behavior of MergeRequest: we instantiate a new object
|
||||
|
@ -121,14 +133,16 @@ describe MergeRequests::MergeWhenBuildSucceedsService do
|
|||
end
|
||||
end
|
||||
|
||||
it "doesn't merge if some stages failed" do
|
||||
it "doesn't merge if any of stages failed" do
|
||||
expect(MergeWorker).not_to receive(:perform_async)
|
||||
|
||||
build.success
|
||||
test.drop
|
||||
end
|
||||
|
||||
it 'merge when all stages succeeded' do
|
||||
it 'merges when all stages succeeded' do
|
||||
expect(MergeWorker).to receive(:perform_async)
|
||||
|
||||
build.success
|
||||
test.success
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue