From 7ccb283da490a6f2cacaf06bbe6b3f7ae155d870 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 11 Dec 2017 19:00:11 -0200 Subject: Prevent worker that updates merge requests head pipeline from failing jobs --- app/models/merge_request.rb | 6 ++++++ app/services/ci/create_pipeline_service.rb | 2 +- app/workers/stuck_merge_jobs_worker.rb | 8 +++++++- .../update_head_pipeline_for_merge_request_worker.rb | 13 ++++++++++++- changelogs/unreleased/issue_41021.yml | 5 +++++ spec/models/merge_request_spec.rb | 14 ++++++++++++++ spec/services/ci/create_pipeline_service_spec.rb | 16 ++++++++++++++-- spec/workers/stuck_merge_jobs_worker_spec.rb | 6 ++++-- ...update_head_pipeline_for_merge_request_worker_spec.rb | 2 +- 9 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/issue_41021.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 422f138c4ea..c40e2a09012 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -83,6 +83,12 @@ class MergeRequest < ActiveRecord::Base transition locked: :opened end + before_transition any => :opened do |merge_request| + Sidekiq::Worker.skipping_transaction_check do + UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id) + end + end + state :opened state :closed state :merged diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 85db2760e23..c8b112132b3 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -81,7 +81,7 @@ module Ci end def related_merge_requests - MergeRequest.where(source_project: pipeline.project, source_branch: pipeline.ref) + MergeRequest.opened.where(source_project: pipeline.project, source_branch: pipeline.ref) end end end diff --git a/app/workers/stuck_merge_jobs_worker.rb b/app/workers/stuck_merge_jobs_worker.rb index 36d2a2e6466..e31fa0fea47 100644 --- a/app/workers/stuck_merge_jobs_worker.rb +++ b/app/workers/stuck_merge_jobs_worker.rb @@ -23,7 +23,13 @@ class StuckMergeJobsWorker merge_requests = MergeRequest.where(id: completed_ids) merge_requests.where.not(merge_commit_sha: nil).update_all(state: :merged) - merge_requests.where(merge_commit_sha: nil).update_all(state: :opened, merge_jid: nil) + + merge_requests_to_reopen = merge_requests.where(merge_commit_sha: nil) + merge_requests_to_reopen.update_all(merge_jid: nil) + + # Do not reopen merge requests using direct queries. + # We rely on state machine callbacks to update head_pipeline_id + merge_requests_to_reopen.map(&:unlock_mr) Rails.logger.info("Updated state of locked merge jobs. JIDs: #{completed_jids.join(', ')}") end diff --git a/app/workers/update_head_pipeline_for_merge_request_worker.rb b/app/workers/update_head_pipeline_for_merge_request_worker.rb index 0a2e9b63578..9614daf1976 100644 --- a/app/workers/update_head_pipeline_for_merge_request_worker.rb +++ b/app/workers/update_head_pipeline_for_merge_request_worker.rb @@ -8,8 +8,19 @@ class UpdateHeadPipelineForMergeRequestWorker pipeline = Ci::Pipeline.where(project: merge_request.source_project, ref: merge_request.source_branch).last return unless pipeline && pipeline.latest? - raise ArgumentError, 'merge request sha does not equal pipeline sha' if merge_request.diff_head_sha != pipeline.sha + + if merge_request.diff_head_sha != pipeline.sha + log_error_message_for(merge_request) + + return + end merge_request.update_attribute(:head_pipeline_id, pipeline.id) end + + def log_error_message_for(merge_request) + Gitlab::EnvironmentLogger.error( + "Outdated head pipeline for active merge request: id=#{merge_request.id}, source_branch=#{merge_request.source_branch}, diff_head_sha=#{merge_request.diff_head_sha}" + ) + end end diff --git a/changelogs/unreleased/issue_41021.yml b/changelogs/unreleased/issue_41021.yml new file mode 100644 index 00000000000..d94f4b2fecd --- /dev/null +++ b/changelogs/unreleased/issue_41021.yml @@ -0,0 +1,5 @@ +--- +title: Prevent worker that updates merge requests head pipeline from failing jobs +merge_request: +author: +type: fixed diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 30a5a3bbff7..13a6d125ebc 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1855,4 +1855,18 @@ describe MergeRequest do it_behaves_like 'throttled touch' do subject { create(:merge_request, updated_at: 1.hour.ago) } end + + context 'state machine transitions' do + describe '#unlock_mr' do + subject { create(:merge_request, state: 'locked') } + + it 'updates merge request head pipeline' do + pipeline = create(:ci_empty_pipeline, project: subject.project, ref: subject.source_branch, sha: subject.source_branch_sha) + + subject.unlock_mr + + expect(subject.reload.head_pipeline).to eq(pipeline) + end + end + end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 41ce81e0651..267258b33a8 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -64,6 +64,18 @@ describe Ci::CreatePipelineService do create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project) end + context 'when related merge request is already merged' do + let!(:merged_merge_request) do + create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project, state: 'merged') + end + + it 'does not schedule update head pipeline job' do + expect(UpdateHeadPipelineForMergeRequestWorker).not_to receive(:perform_async).with(merged_merge_request.id) + + execute_service + end + end + context 'when the head pipeline sha equals merge request sha' do it 'updates head pipeline of each merge request' do merge_request_1 @@ -77,13 +89,13 @@ describe Ci::CreatePipelineService do end context 'when the head pipeline sha does not equal merge request sha' do - it 'raises the ArgumentError error from worker and does not update the head piepeline of MRs' do + it 'does not update the head piepeline of MRs' do merge_request_1 merge_request_2 allow_any_instance_of(Ci::Pipeline).to receive(:latest?).and_return(true) - expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.to raise_error(ArgumentError) + expect { execute_service(after: 'ae73cb07c9eeaf35924a10f713b364d32b2dd34f') }.not_to raise_error last_pipeline = Ci::Pipeline.last diff --git a/spec/workers/stuck_merge_jobs_worker_spec.rb b/spec/workers/stuck_merge_jobs_worker_spec.rb index f8b55e873df..c2c2a5f9121 100644 --- a/spec/workers/stuck_merge_jobs_worker_spec.rb +++ b/spec/workers/stuck_merge_jobs_worker_spec.rb @@ -14,7 +14,6 @@ describe StuckMergeJobsWorker do mr_with_sha.reload mr_without_sha.reload - expect(mr_with_sha).to be_merged expect(mr_without_sha).to be_opened expect(mr_with_sha.merge_jid).to be_present @@ -24,10 +23,13 @@ describe StuckMergeJobsWorker do it 'updates merge request to opened when locked but has not been merged' do allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(%w(123)) merge_request = create(:merge_request, :locked, merge_jid: '123', state: :locked) + pipeline = create(:ci_empty_pipeline, project: merge_request.project, ref: merge_request.source_branch, sha: merge_request.source_branch_sha) worker.perform - expect(merge_request.reload).to be_opened + merge_request.reload + expect(merge_request).to be_opened + expect(merge_request.head_pipeline).to eq(pipeline) end it 'logs updated stuck merge job ids' do diff --git a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb index 522e1566271..9adde5fc21a 100644 --- a/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb +++ b/spec/workers/update_head_pipeline_for_merge_request_worker_spec.rb @@ -22,7 +22,7 @@ describe UpdateHeadPipelineForMergeRequestWorker do end it 'does not update head_pipeline_id' do - expect { subject.perform(merge_request.id) }.to raise_error(ArgumentError) + expect { subject.perform(merge_request.id) }.not_to raise_error expect(merge_request.reload.head_pipeline_id).to eq(nil) end -- cgit v1.2.1