diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2016-10-10 13:39:51 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2016-10-10 13:39:51 +0000 |
commit | 359b4f9a0303e3b55004c7d4d6cacc1ef8e6e25f (patch) | |
tree | 65768e15d53f001467c23c25831ee2aea67cc335 | |
parent | f5576b16baa7e6717a380566049235ee6ce40b05 (diff) | |
parent | 904de2d64b1b63a82cfba92e02b6c25ff94b725b (diff) | |
download | gitlab-ce-359b4f9a0303e3b55004c7d4d6cacc1ef8e6e25f.tar.gz |
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
-rw-r--r-- | app/models/commit_status.rb | 32 | ||||
-rw-r--r-- | app/services/ci/process_pipeline_service.rb | 2 | ||||
-rw-r--r-- | spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb | 24 |
3 files changed, 42 insertions, 16 deletions
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 9fa8d17e74e..5d6d534cd31 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -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? - - if commit_status.complete? - ProcessPipelineWorker.perform_async(pipeline.id) + return if transition.loopback? + + 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 diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb index 36c93dddadb..d3dd30b2588 100644 --- a/app/services/ci/process_pipeline_service.rb +++ b/app/services/ci/process_pipeline_service.rb @@ -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 diff --git a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb index 520e906b21f..9a29e400654 100644 --- a/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_build_succeeds_service_spec.rb @@ -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 |