diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue index cf26003d03813f18d6c250576eff70614d9c3589..a5e3115397acdd2b3904bc9314540deb370e6d7c 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_checking.vue @@ -12,7 +12,7 @@ export default {
- {{ s__('mrWidget|Checking ability to merge automatically') }} + {{ s__('mrWidget|Checking ability to merge automatically…') }}
diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js index 3ab229567f6f33fcec8632911f5802347e5d4c4f..a298331c1fccdd38488a4ac663fbbb2be384158c 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js @@ -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; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 170256704880ca63d9b59f463d23873635b3924c..22de8dd4109297b281b051b227126be5b1879110 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -45,7 +45,7 @@ def index 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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7162ba08a7632bfcbacea87d342f02310a87fe35..4ed73d979ce3e8df904ad640671e443e509a91bd 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -160,20 +160,25 @@ class << self 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 << self # 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 @@ def reload_diff(current_user = nil) 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 diff --git a/app/services/merge_requests/mergeability_check_service.rb b/app/services/merge_requests/mergeability_check_service.rb index 962e2327b3ed10b22a346e36dd3cfbc0d9853e5c..5b79e4d01f2fc618c1b2b2537bd2c3e47f250854 100644 --- a/app/services/merge_requests/mergeability_check_service.rb +++ b/app/services/merge_requests/mergeability_check_service.rb @@ -12,6 +12,13 @@ def initialize(merge_request) @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 @@ def initialize(merge_request) # 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 @@ def merge_ref_auto_sync_enabled? 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 diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 502e7976ef2db79b9c6bccd8a2b17ce5d7508cee..f26a2201550f760a32e2541061c2427def15f852 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -191,3 +191,4 @@ - group_export - self_monitoring_project_create - self_monitoring_project_delete +- merge_request_mergeability_check diff --git a/app/workers/merge_request_mergeability_check_worker.rb b/app/workers/merge_request_mergeability_check_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..ed35284b66c4360f08e6f3985c8bcf478cf5f21c --- /dev/null +++ b/app/workers/merge_request_mergeability_check_worker.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml b/changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml new file mode 100644 index 0000000000000000000000000000000000000000..c1192390dcef28a610bcd697856a705041c86866 --- /dev/null +++ b/changelogs/unreleased/29984-asynchronous-mr-mergeability-check.yml @@ -0,0 +1,5 @@ +--- +title: Check mergeability of MR asynchronously +merge_request: 21026 +author: +type: performance diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 06f7f01b83d4577371be922d6662164c68cdef92..e341c91899b0ce910f29dac055a4ba1e0c2a89ab 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -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] diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index d85310de1598d9252423f3942bea5ebb168e44ad..3a00e862094d0011e85ab3142459f827379ecb28 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -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, diff --git a/lib/api/entities.rb b/lib/api/entities.rb index dfd0e676586dd1aeaa8a268e47c96ef59e349cd2..97be90030c8ad0ba21f993f04c44831a7e52cb17 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -811,7 +811,7 @@ class MergeRequestBasic < IssuableEntity # 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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 942c81aafd5eddbb274db16ed7632b5f355a5466..eb5baf3ffdd4cfd073b5c4c533744cebd2ddafdf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -22436,7 +22436,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" diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index d5b1bfe0ac4263210f38ee7f19f4ee6a1f755edd..df2727d13cfb346aeeff3341973019ee0a76fbb3 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -44,6 +44,21 @@ def go(extra_params = {}) 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 diff --git a/spec/features/merge_request/user_sees_merge_widget_spec.rb b/spec/features/merge_request/user_sees_merge_widget_spec.rb index 098f41f120d33eae099344636d30460b97cf47e8..17754400b9176c54758c52ea13c600bbcb81685b 100644 --- a/spec/features/merge_request/user_sees_merge_widget_spec.rb +++ b/spec/features/merge_request/user_sees_merge_widget_spec.rb @@ -19,7 +19,7 @@ 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, diff --git a/spec/features/merge_requests/user_squashes_merge_request_spec.rb b/spec/features/merge_requests/user_squashes_merge_request_spec.rb index a9b96c5bbf571b9a1a9ab74f059e00dd7d0bdcba..4115907d074da49254813f4dbb2947bbd5a7d162 100644 --- a/spec/features/merge_requests/user_squashes_merge_request_spec.rb +++ b/spec/features/merge_requests/user_squashes_merge_request_spec.rb @@ -67,7 +67,7 @@ def accept_mr 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 @@ def accept_mr 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' diff --git a/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js b/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js index 4f6451473e8cd8baf2a6fd47919c72b733339b72..efccd507fe2d8347fec3e18e459d06ef54d25c11 100644 --- a/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js +++ b/spec/javascripts/vue_mr_widget/components/states/mr_widget_checking_spec.js @@ -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…', ); }); }); diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index c6894c04385fa6bfd6eae89f6b5efe20dc0ea1ca..9f2567192484e94073c4ec6c6f3b62bfb8d35eab 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -277,6 +277,7 @@ 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 @@ def set_compare(merge_request) 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 @@ def create_build(pipeline, coverage, name) 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 diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index ae0596bea98c13468faefc99d08371c7a3b1115a..c3d92a90d116d65da50a801ad6b8ce6f5b295649 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -65,6 +65,21 @@ 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 @@ 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 diff --git a/spec/services/merge_requests/mergeability_check_service_spec.rb b/spec/services/merge_requests/mergeability_check_service_spec.rb index a864da0a6fba076d91ec1e452840a9ac6b127b95..8f17e8083e39681acbdd0499762749eb25844391 100644 --- a/spec/services/merge_requests/mergeability_check_service_spec.rb +++ b/spec/services/merge_requests/mergeability_check_service_spec.rb @@ -53,9 +53,42 @@ 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 } diff --git a/spec/workers/merge_request_mergeability_check_worker_spec.rb b/spec/workers/merge_request_mergeability_check_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2331664215f2e58e52d6fb68f357b15fb52fd691 --- /dev/null +++ b/spec/workers/merge_request_mergeability_check_worker_spec.rb @@ -0,0 +1,29 @@ +# 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