From f5631ff262fcf84b79c35c34d5a5337440aa4621 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 7 Oct 2016 14:52:30 +0200 Subject: Fix ci pipeline processing with async jobs --- app/models/commit_status.rb | 31 ++++++++++++++++------------- app/services/ci/process_pipeline_service.rb | 2 ++ 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 9fa8d17e74e..38554e7a0ca 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' @@ -84,20 +85,6 @@ class CommitStatus < ActiveRecord::Base commit_status.update_attributes finished_at: Time.now 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) - 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) end @@ -105,10 +92,26 @@ class CommitStatus < ActiveRecord::Base after_transition any => :failed do |commit_status| MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.pipeline.project, nil).execute(commit_status) end + + after_transition do: :schedule_pipeline_update end delegate :sha, :short_sha, to: :pipeline + def schedule_pipeline_update + run_after_commit(:process_pipeline!) + end + + def process_pipeline! + pipeline.try do |pipeline| + if complete? + ProcessPipelineWorker.perform_async(pipeline.id) + else + UpdatePipelineWorker.perform_async(pipeline.id) + end + end + end + def before_sha pipeline.before_sha || Gitlab::Git::BLANK_SHA 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 -- cgit v1.2.1 From 04afdb613e2282fc6a2debb301a32dad08427f8a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 8 Oct 2016 20:24:43 +0200 Subject: Improve spec for merge when build succeeds feature --- .../merge_when_build_succeeds_service_spec.rb | 24 +++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) 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 -- cgit v1.2.1 From 74fd5cab155cb83ef47ba8f4e78ccfd714f73211 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 8 Oct 2016 20:25:16 +0200 Subject: Improve transitions and run hooks after transaction --- app/models/commit_status.rb | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 38554e7a0ca..81f041cf29e 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -85,33 +85,35 @@ class CommitStatus < ActiveRecord::Base commit_status.update_attributes finished_at: Time.now end + after_transition do |commit_status| + 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 + end + 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 + 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 - - after_transition do: :schedule_pipeline_update end delegate :sha, :short_sha, to: :pipeline - def schedule_pipeline_update - run_after_commit(:process_pipeline!) - end - - def process_pipeline! - pipeline.try do |pipeline| - if complete? - ProcessPipelineWorker.perform_async(pipeline.id) - else - UpdatePipelineWorker.perform_async(pipeline.id) - end - end - end - def before_sha pipeline.before_sha || Gitlab::Git::BLANK_SHA end -- cgit v1.2.1 From 3fb4d86c6dbb2298f6d5b0010ae0f6e26905b934 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 8 Oct 2016 20:34:45 +0200 Subject: Add temporary fix for race condition in MWBS --- app/models/commit_status.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 81f041cf29e..c8b168f5526 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -99,6 +99,9 @@ class CommitStatus < ActiveRecord::Base after_transition [:created, :pending, :running] => :success do |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 -- cgit v1.2.1 From 904de2d64b1b63a82cfba92e02b6c25ff94b725b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 8 Oct 2016 20:42:09 +0200 Subject: Check for transition loopback in commit status --- app/models/commit_status.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index c8b168f5526..5d6d534cd31 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -85,7 +85,9 @@ class CommitStatus < ActiveRecord::Base commit_status.update_attributes finished_at: Time.now end - after_transition do |commit_status| + after_transition do |commit_status, transition| + return if transition.loopback? + commit_status.run_after_commit do pipeline.try do |pipeline| if complete? -- cgit v1.2.1