Commit 8c01b8e7 authored by Patrick Bajao's avatar Patrick Bajao Committed by Nick Thomas

Check mergeability of MR asynchronously

Executing `MergeRequests::MergeabilityCheckService#execute` can be
time consuming and can likely cause a request to timeout.

Moving its execution asynchronously via `#async_execute` will help
in ensuring page and API requests that needs it can load faster.
parent 226b13b3
......@@ -12,7 +12,7 @@ export default {
<div class="mr-widget-body media">
<status-icon :show-disabled-button="true" status="loading" />
<div class="media-body space-children">
<span class="bold"> {{ s__('mrWidget|Checking ability to merge automatically') }} </span>
<span class="bold"> {{ s__('mrWidget|Checking ability to merge automatically') }} </span>
</div>
</div>
</template>
......@@ -7,7 +7,7 @@ export default function deviseState(data) {
return stateKey.missingBranch;
} else if (!data.commits_count) {
return stateKey.nothingToMerge;
} else if (this.mergeStatus === 'unchecked') {
} else if (this.mergeStatus === 'unchecked' || this.mergeStatus === 'checking') {
return stateKey.checking;
} else if (data.has_conflicts) {
return stateKey.conflicts;
......
......@@ -45,7 +45,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
def show
close_merge_request_if_no_source_project
@merge_request.check_mergeability
@merge_request.check_mergeability(async: true)
respond_to do |format|
format.html do
......
......@@ -160,20 +160,25 @@ class MergeRequest < ApplicationRecord
state_machine :merge_status, initial: :unchecked do
event :mark_as_unchecked do
transition [:can_be_merged, :unchecked] => :unchecked
transition [:can_be_merged, :checking, :unchecked] => :unchecked
transition [:cannot_be_merged, :cannot_be_merged_recheck] => :cannot_be_merged_recheck
end
event :mark_as_checking do
transition [:unchecked, :cannot_be_merged_recheck] => :checking
end
event :mark_as_mergeable do
transition [:unchecked, :cannot_be_merged_recheck] => :can_be_merged
transition [:unchecked, :cannot_be_merged_recheck, :checking] => :can_be_merged
end
event :mark_as_unmergeable do
transition [:unchecked, :cannot_be_merged_recheck] => :cannot_be_merged
transition [:unchecked, :cannot_be_merged_recheck, :checking] => :cannot_be_merged
end
state :unchecked
state :cannot_be_merged_recheck
state :checking
state :can_be_merged
state :cannot_be_merged
......@@ -191,7 +196,7 @@ class MergeRequest < ApplicationRecord
# rubocop: enable CodeReuse/ServiceClass
def check_state?(merge_status)
[:unchecked, :cannot_be_merged_recheck].include?(merge_status.to_sym)
[:unchecked, :cannot_be_merged_recheck, :checking].include?(merge_status.to_sym)
end
end
......@@ -812,10 +817,16 @@ class MergeRequest < ApplicationRecord
MergeRequests::ReloadDiffsService.new(self, current_user).execute
end
def check_mergeability
def check_mergeability(async: false)
return if Feature.enabled?(:merge_requests_conditional_mergeability_check, default_enabled: true) && !recheck_merge_status?
MergeRequests::MergeabilityCheckService.new(self).execute(retry_lease: false)
check_service = MergeRequests::MergeabilityCheckService.new(self)
if async && Feature.enabled?(:async_merge_request_check_mergeability, project)
check_service.async_execute
else
check_service.execute(retry_lease: false)
end
end
# rubocop: enable CodeReuse/ServiceClass
......
......@@ -12,6 +12,13 @@ module MergeRequests
@merge_request = merge_request
end
def async_execute
return service_error if service_error
return unless merge_request.mark_as_checking
MergeRequestMergeabilityCheckWorker.perform_async(merge_request.id)
end
# Updates the MR merge_status. Whenever it switches to a can_be_merged state,
# the merge-ref is refreshed.
#
......@@ -30,8 +37,7 @@ module MergeRequests
# and the merge-ref is synced. Success in case of being/becoming mergeable,
# error otherwise.
def execute(recheck: false, retry_lease: true)
return ServiceResponse.error(message: 'Invalid argument') unless merge_request
return ServiceResponse.error(message: 'Unsupported operation') if Gitlab::Database.read_only?
return service_error if service_error
return check_mergeability(recheck) unless merge_ref_auto_sync_lock_enabled?
in_write_lock(retry_lease: retry_lease) do |retried|
......@@ -155,5 +161,15 @@ module MergeRequests
def merge_ref_auto_sync_lock_enabled?
Feature.enabled?(:merge_ref_auto_sync_lock, project, default_enabled: true)
end
def service_error
strong_memoize(:service_error) do
if !merge_request
ServiceResponse.error(message: 'Invalid argument')
elsif Gitlab::Database.read_only?
ServiceResponse.error(message: 'Unsupported operation')
end
end
end
end
end
......@@ -191,3 +191,4 @@
- group_export
- self_monitoring_project_create
- self_monitoring_project_delete
- merge_request_mergeability_check
# frozen_string_literal: true
class MergeRequestMergeabilityCheckWorker
include ApplicationWorker
feature_category :source_code_management
def perform(merge_request_id)
merge_request = MergeRequest.find_by_id(merge_request_id)
unless merge_request
logger.error("Failed to find merge request with ID: #{merge_request_id}")
return
end
result =
::MergeRequests::MergeabilityCheckService
.new(merge_request)
.execute(recheck: false, retry_lease: false)
logger.error("Failed to check mergeability of merge request (#{merge_request_id}): #{result.message}") if result.error?
end
end
---
title: Check mergeability of MR asynchronously
merge_request: 21026
author:
type: performance
......@@ -102,6 +102,7 @@
- [self_monitoring_project_create, 2]
- [self_monitoring_project_delete, 2]
- [error_tracking_issue_link, 2]
- [merge_request_mergeability_check, 5]
# EE-specific queues
- [analytics, 1]
......
......@@ -61,6 +61,12 @@ Parameters:
| `in` | string | no | Modify the scope of the `search` attribute. `title`, `description`, or a string joining them with comma. Default is `title,description` |
| `wip` | string | no | Filter merge requests against their `wip` status. `yes` to return *only* WIP merge requests, `no` to return *non* WIP merge requests |
NOTE: **Note:**
[Starting in GitLab 12.8](https://gitlab.com/gitlab-org/gitlab/issues/29984),
the mergeability (`merge_status`) of each merge request will be checked
asynchronously when a request is made to this endpoint. Poll this API endpoint
to get updated status.
```json
[
{
......@@ -526,6 +532,12 @@ Parameters:
- `include_diverged_commits_count` (optional) - If `true` response includes the commits behind the target branch
- `include_rebase_in_progress` (optional) - If `true` response includes whether a rebase operation is in progress
NOTE: **Note:**
[Starting in GitLab 12.8](https://gitlab.com/gitlab-org/gitlab/issues/29984),
the mergeability (`merge_status`) of a merge request will be checked
asynchronously when a request is made to this endpoint. Poll this API endpoint
to get updated status.
```json
{
"id": 1,
......
......@@ -811,7 +811,7 @@ module API
# See https://gitlab.com/gitlab-org/gitlab-foss/issues/42344 for more
# information.
expose :merge_status do |merge_request|
merge_request.check_mergeability
merge_request.check_mergeability(async: true)
merge_request.merge_status
end
expose :diff_head_sha, as: :sha
......
......@@ -22448,7 +22448,7 @@ msgstr ""
msgid "mrWidget|Check out branch"
msgstr ""
msgid "mrWidget|Checking ability to merge automatically"
msgid "mrWidget|Checking ability to merge automatically"
msgstr ""
msgid "mrWidget|Cherry-pick"
......
......@@ -44,6 +44,21 @@ describe Projects::MergeRequestsController do
get :show, params: params.merge(extra_params)
end
context 'when merge request is unchecked' do
before do
merge_request.mark_as_unchecked!
end
it 'checks mergeability asynchronously' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
expect(service).not_to receive(:execute)
expect(service).to receive(:async_execute)
end
go
end
end
describe 'as html' do
context 'when diff files were cleaned' do
render_views
......
......@@ -19,7 +19,7 @@ describe 'Merge request > User sees merge widget', :js do
sign_in(user)
end
context 'new merge request' do
context 'new merge request', :sidekiq_might_not_need_inline do
before do
visit project_new_merge_request_path(
project,
......
......@@ -67,7 +67,7 @@ describe 'User squashes a merge request', :js do
end
end
context 'when squash is enabled on merge request creation' do
context 'when squash is enabled on merge request creation', :sidekiq_might_not_need_inline do
before do
visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: source_branch })
check 'merge_request[squash]'
......@@ -97,7 +97,7 @@ describe 'User squashes a merge request', :js do
end
end
context 'when squash is not enabled on merge request creation' do
context 'when squash is not enabled on merge request creation', :sidekiq_might_not_need_inline do
before do
visit project_new_merge_request_path(project, merge_request: { target_branch: 'master', source_branch: source_branch })
click_on 'Submit merge request'
......
......@@ -25,7 +25,7 @@ describe('MRWidgetChecking', () => {
it('renders information about merging', () => {
expect(vm.$el.querySelector('.media-body').textContent.trim()).toEqual(
'Checking ability to merge automatically',
'Checking ability to merge automatically',
);
});
});
......@@ -277,6 +277,7 @@ describe MergeRequest do
describe 'respond to' do
it { is_expected.to respond_to(:unchecked?) }
it { is_expected.to respond_to(:checking?) }
it { is_expected.to respond_to(:can_be_merged?) }
it { is_expected.to respond_to(:cannot_be_merged?) }
it { is_expected.to respond_to(:merge_params) }
......@@ -2084,43 +2085,75 @@ describe MergeRequest do
describe '#check_mergeability' do
let(:mergeability_service) { double }
subject { create(:merge_request, merge_status: 'unchecked') }
before do
allow(MergeRequests::MergeabilityCheckService).to receive(:new) do
mergeability_service
end
end
context 'if the merge status is unchecked' do
before do
subject.mark_as_unchecked!
end
shared_examples_for 'method that executes MergeabilityCheckService' do
it 'executes MergeabilityCheckService' do
expect(mergeability_service).to receive(:execute)
subject.check_mergeability
end
context 'when async is true' do
context 'and async_merge_request_check_mergeability feature flag is enabled' do
it 'executes MergeabilityCheckService asynchronously' do
expect(mergeability_service).to receive(:async_execute)
subject.check_mergeability(async: true)
end
end
context 'and async_merge_request_check_mergeability feature flag is disabled' do
before do
stub_feature_flags(async_merge_request_check_mergeability: false)
end
it 'executes MergeabilityCheckService' do
expect(mergeability_service).to receive(:execute)
subject.check_mergeability(async: true)
end
end
end
end
context 'if the merge status is unchecked' do
it_behaves_like 'method that executes MergeabilityCheckService'
end
context 'if the merge status is checking' do
before do
subject.mark_as_checking!
end
it_behaves_like 'method that executes MergeabilityCheckService'
end
context 'if the merge status is checked' do
context 'and feature flag is enabled' do
it 'executes MergeabilityCheckService' do
expect(mergeability_service).not_to receive(:execute)
before do
subject.mark_as_mergeable!
end
context 'and merge_requests_conditional_mergeability_check feature flag is enabled' do
it 'does not call MergeabilityCheckService' do
expect(MergeRequests::MergeabilityCheckService).not_to receive(:new)
subject.check_mergeability
end
end
context 'and feature flag is disabled' do
context 'and merge_requests_conditional_mergeability_check feature flag is disabled' do
before do
stub_feature_flags(merge_requests_conditional_mergeability_check: false)
end
it 'does not execute MergeabilityCheckService' do
expect(mergeability_service).to receive(:execute)
subject.check_mergeability
end
it_behaves_like 'method that executes MergeabilityCheckService'
end
end
end
......@@ -3145,7 +3178,7 @@ describe MergeRequest do
describe 'check_state?' do
it 'indicates whether MR is still checking for mergeability' do
state_machine = described_class.state_machines[:merge_status]
check_states = [:unchecked, :cannot_be_merged_recheck]
check_states = [:unchecked, :cannot_be_merged_recheck, :checking]
check_states.each do |merge_status|
expect(state_machine.check_state?(merge_status)).to be true
......
......@@ -65,6 +65,21 @@ describe API::MergeRequests do
end.not_to exceed_query_limit(control)
end
context 'when merge request is unchecked' do
before do
merge_request.mark_as_unchecked!
end
it 'checks mergeability asynchronously' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
expect(service).not_to receive(:execute)
expect(service).to receive(:async_execute)
end
get api(endpoint_path, user)
end
end
context 'with labels' do
include_context 'with labels'
......@@ -1003,6 +1018,21 @@ describe API::MergeRequests do
expect(json_response['user']['can_merge']).to be_falsy
end
context 'when merge request is unchecked' do
before do
merge_request.mark_as_unchecked!
end
it 'checks mergeability asynchronously' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService) do |service|
expect(service).not_to receive(:execute)
expect(service).to receive(:async_execute)
end
get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user)
end
end
end
describe 'GET /projects/:id/merge_requests/:merge_request_iid/participants' do
......
......@@ -53,9 +53,42 @@ describe MergeRequests::MergeabilityCheckService, :clean_gitlab_redis_shared_sta
end
end
let(:project) { create(:project, :repository) }
let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
describe '#async_execute' do
shared_examples_for 'no job is enqueued' do
it 'does not enqueue MergeRequestMergeabilityCheckWorker' do
expect(MergeRequestMergeabilityCheckWorker).not_to receive(:perform_async)
described_class.new(merge_request).async_execute
end
end
it 'enqueues MergeRequestMergeabilityCheckWorker' do
expect(MergeRequestMergeabilityCheckWorker).to receive(:perform_async)
described_class.new(merge_request).async_execute
end
context 'when read only DB' do
before do
allow(Gitlab::Database).to receive(:read_only?) { true }
end
it_behaves_like 'no job is enqueued'
end
context 'when merge_status is already checking' do
before do
merge_request.mark_as_checking
end
it_behaves_like 'no job is enqueued'
end
end
describe '#execute' do
let(:project) { create(:project, :repository) }
let(:merge_request) { create(:merge_request, merge_status: :unchecked, source_project: project, target_project: project) }
let(:repo) { project.repository }
subject { described_class.new(merge_request).execute }
......
# frozen_string_literal: true
require 'spec_helper'
describe MergeRequestMergeabilityCheckWorker do
subject { described_class.new }
describe '#perform' do
context 'when merge request does not exist' do
it 'does not execute MergeabilityCheckService' do
expect(MergeRequests::MergeabilityCheckService).not_to receive(:new)
subject.perform(1)
end
end
context 'when merge request exists' do
let(:merge_request) { create(:merge_request) }
it 'executes MergeabilityCheckService' do
expect_next_instance_of(MergeRequests::MergeabilityCheckService, merge_request) do |service|
expect(service).to receive(:execute).and_return(double(error?: false))
end
subject.perform(merge_request.id)
end
end
end
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