Commit f483edbd authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'run-pipeline-for-merge-train-at-train-ref' into 'master'

Fix race condition of `refs/merge` competing overwrite

Closes #12562

See merge request gitlab-org/gitlab-ee!14495
parents c3f467d2 9e8e3ed8
......@@ -1129,6 +1129,19 @@ class MergeRequest < ApplicationRecord
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/merge"
end
def train_ref_path
"refs/#{Repository::REF_MERGE_REQUEST}/#{iid}/train"
end
def cleanup_refs(only: :all)
target_refs = []
target_refs << ref_path if %i[all head].include?(only)
target_refs << merge_ref_path if %i[all merge].include?(only)
target_refs << train_ref_path if %i[all train].include?(only)
project.repository.delete_refs(*target_refs)
end
def self.merge_request_ref?(ref)
ref.start_with?("refs/#{Repository::REF_MERGE_REQUEST}/")
end
......
......@@ -845,6 +845,10 @@ class Repository
raw.merge_to_ref(user, source_sha, branch, target_ref, message, first_parent_ref)
end
def delete_refs(*ref_names)
raw.delete_refs(*ref_names)
end
def ff_merge(user, source, target_branch, merge_request: nil)
their_commit_id = commit(source)&.id
raise 'Invalid merge source' if their_commit_id.nil?
......
......@@ -20,7 +20,7 @@ module EE
has_many :approver_groups, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_many :approval_rules, class_name: 'ApprovalMergeRequestRule', inverse_of: :merge_request
has_many :draft_notes
has_one :merge_train
has_one :merge_train, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :blocks_as_blocker,
class_name: 'MergeRequestBlock',
......
# frozen_string_literal: true
class MergeTrain < ApplicationRecord
include AfterCommitQueue
belongs_to :target_project, class_name: "Project"
belongs_to :merge_request
belongs_to :user
belongs_to :pipeline, class_name: 'Ci::Pipeline'
after_destroy do |merge_train|
run_after_commit { merge_train.merge_request.cleanup_refs(only: :train) }
end
class << self
def all_in_train(merge_request)
joined_merge_requests(merge_request).order('merge_trains.id ASC')
......
......@@ -24,7 +24,7 @@ module AutoMerge
next_merge_request = merge_request.merge_train&.next
super do
if merge_request.merge_train&.delete
if merge_request.merge_train&.destroy
SystemNoteService.cancel_merge_train(merge_request, project, current_user)
AutoMergeProcessWorker.perform_async(next_merge_request.id) if next_merge_request
end
......
......@@ -22,12 +22,13 @@ module MergeTrains
end
def create_merge_ref(merge_request)
::MergeRequests::MergeToRefService.new(merge_request.project, merge_request.merge_user).execute(merge_request)
::MergeRequests::MergeToRefService.new(merge_request.project, merge_request.merge_user, target_ref: merge_request.train_ref_path)
.execute(merge_request)
end
def create_pipeline(merge_request, merge_status)
pipeline = ::Ci::CreatePipelineService.new(merge_request.source_project, merge_request.merge_user,
ref: merge_request.merge_ref_path,
ref: merge_request.train_ref_path,
checkout_sha: merge_status[:commit_id],
target_sha: merge_status[:target_id],
source_sha: merge_status[:source_id])
......
......@@ -68,7 +68,7 @@ module MergeTrains
raise ProcessError, 'failed to merge' unless merge_request.merged?
merge_train.delete
merge_train.destroy
end
def stale_pipeline?
......
---
title: Fix race condition of `refs/merge` competing overwrite
merge_request: 14495
author:
type: fixed
......@@ -193,6 +193,23 @@ describe API::MergeRequests do
end
end
describe "DELETE /projects/:id/merge_requests/:merge_request_iid" do
context "when the merge request is on the merge train" do
let!(:merge_request) { create(:merge_request, :on_train, source_project: project, target_project: project) }
before do
::MergeRequests::MergeToRefService.new(merge_request.project, merge_request.merge_user, target_ref: merge_request.train_ref_path)
.execute(merge_request)
end
it 'removes train ref' do
expect do
delete api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user)
end.to change { project.repository.ref_exists?(merge_request.train_ref_path) }.from(true).to(false)
end
end
end
context 'when authenticated' do
def expect_response_contain_exactly(*items)
expect(response).to have_gitlab_http_status(200)
......
......@@ -125,6 +125,24 @@ describe AutoMerge::MergeTrainService do
subject
end
context 'when train ref exists' do
before do
merge_request.project.repository.create_ref(merge_request.target_branch, merge_request.train_ref_path)
end
it 'deletes train ref' do
expect { subject }
.to change { merge_request.project.repository.ref_exists?(merge_request.train_ref_path) }
.from(true).to(false)
end
end
context 'when train ref does not exist' do
it 'does not raise an error' do
expect { subject }.not_to raise_error
end
end
context 'when the other merge request is following the merge request' do
let!(:merge_request_2) do
create(:merge_request, :on_train,
......
......@@ -78,8 +78,14 @@ describe MergeTrains::CreatePipelineService do
stub_ci_pipeline_yaml_file(YAML.dump(ci_yaml))
end
it 'calls Ci::CreatePipelineService' do
expect_next_instance_of(Ci::CreatePipelineService, project, maintainer, any_args) do |pipeline_service|
it 'creates train ref' do
expect { subject }
.to change { merge_request.project.repository.ref_exists?(merge_request.train_ref_path) }
.from(false).to(true)
end
it 'calls Ci::CreatePipelineService for creating pipeline on train ref' do
expect_next_instance_of(Ci::CreatePipelineService, project, maintainer, hash_including(ref: merge_request.train_ref_path)) do |pipeline_service|
expect(pipeline_service).to receive(:execute)
.with(:merge_request_event, hash_including(merge_request: merge_request)).and_call_original
end
......
......@@ -114,11 +114,12 @@ describe MergeTrains::RefreshMergeRequestService do
context 'when the merge request is the first queue' do
it 'merges the merge request' do
expect(merge_request).to receive(:cleanup_refs).with(only: :train)
expect_next_instance_of(MergeRequests::MergeService, project, maintainer, anything) do |service|
expect(service).to receive(:execute).with(merge_request)
end
subject
expect { subject }.to change { MergeTrain.count }.by(-1)
end
context 'when it failed to merge the merge request' do
......
......@@ -3218,4 +3218,34 @@ describe MergeRequest do
it { is_expected.to be_truthy }
end
end
describe '#cleanup_refs' do
subject { merge_request.cleanup_refs(only: only) }
let(:merge_request) { build(:merge_request) }
context 'when removing all refs' do
let(:only) { :all }
it 'deletes all refs from the target project' do
expect(merge_request.target_project.repository)
.to receive(:delete_refs)
.with(merge_request.ref_path, merge_request.merge_ref_path, merge_request.train_ref_path)
subject
end
end
context 'when removing only train ref' do
let(:only) { :train }
it 'deletes train ref from the target project' do
expect(merge_request.target_project.repository)
.to receive(:delete_refs)
.with(merge_request.train_ref_path)
subject
end
end
end
end
......@@ -191,6 +191,15 @@ describe MergeRequests::MergeToRefService do
it { expect(todo).not_to be_done }
end
context 'when target ref is passed as a parameter' do
let(:params) { { commit_message: 'merge train', target_ref: target_ref } }
it_behaves_like 'successfully merges to ref with merge method' do
let(:first_parent_ref) { 'refs/heads/master' }
let(:target_ref) { 'refs/merge-requests/1/train' }
end
end
describe 'cascading merge refs' do
set(:project) { create(:project, :repository) }
let(:params) { { commit_message: 'Cascading merge', first_parent_ref: first_parent_ref, target_ref: target_ref } }
......
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