Commit 504f77b4 authored by Felipe's avatar Felipe

Prevent worker that updates merge requests head pipeline from failing jobs

parent 0cdc840b
Pipeline #15077033 passed with stages
in 56 minutes and 48 seconds
......@@ -83,6 +83,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
......@@ -1855,4 +1855,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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment