Merge branch 'issue_41021' into 'master'
Prevent worker that updates merge requests head pipeline from failing jobs Closes #41021 See merge request gitlab-org/gitlab-ce!15870
This commit is contained in:
commit
013e681625
|
@ -85,6 +85,14 @@ class MergeRequest < ActiveRecord::Base
|
|||
transition locked: :opened
|
||||
end
|
||||
|
||||
before_transition any => :opened do |merge_request|
|
||||
merge_request.merge_jid = nil
|
||||
|
||||
merge_request.run_after_commit do
|
||||
UpdateHeadPipelineForMergeRequestWorker.perform_async(merge_request.id)
|
||||
end
|
||||
end
|
||||
|
||||
state :opened
|
||||
state :closed
|
||||
state :merged
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -23,7 +23,12 @@ 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)
|
||||
|
||||
# Do not reopen merge requests using direct queries.
|
||||
# We rely on state machine callbacks to update head_pipeline_id
|
||||
merge_requests_to_reopen.each(&:unlock_mr)
|
||||
|
||||
Rails.logger.info("Updated state of locked merge jobs. JIDs: #{completed_jids.join(', ')}")
|
||||
end
|
||||
|
|
|
@ -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)
|
||||
Rails.logger.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
|
||||
|
|
|
@ -1857,4 +1857,20 @@ 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', merge_jid: 123) }
|
||||
|
||||
it 'updates merge request head pipeline and sets merge_jid to nil' do
|
||||
pipeline = create(:ci_empty_pipeline, project: subject.project, ref: subject.source_branch, sha: subject.source_branch_sha)
|
||||
|
||||
subject.unlock_mr
|
||||
|
||||
subject.reload
|
||||
expect(subject.head_pipeline).to eq(pipeline)
|
||||
expect(subject.merge_jid).to be_nil
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue