From 0640b3d1d89b7a4eda343eb23b0518a835ac9106 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 31 Jul 2017 19:01:36 -0300 Subject: Store MergeWorker JID on merge request, and clean up stuck merges --- .../components/states/mr_widget_locked.js | 29 ------------------ .../components/states/mr_widget_merging.js | 29 ++++++++++++++++++ .../vue_merge_request_widget/dependencies.js | 2 +- .../vue_merge_request_widget/mr_widget_options.js | 4 +-- .../stores/mr_widget_store.js | 9 ++++-- .../vue_merge_request_widget/stores/state_maps.js | 4 +-- .../projects/merge_requests_controller.rb | 5 ---- app/models/merge_request.rb | 22 ++++---------- app/serializers/merge_request_entity.rb | 2 +- app/workers/merge_worker.rb | 2 ++ app/workers/stuck_merge_jobs_worker.rb | 34 ++++++++++++++++++++++ 11 files changed, 83 insertions(+), 59 deletions(-) delete mode 100644 app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js create mode 100644 app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js create mode 100644 app/workers/stuck_merge_jobs_worker.rb (limited to 'app') diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js deleted file mode 100644 index a12f418e1af..00000000000 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_locked.js +++ /dev/null @@ -1,29 +0,0 @@ -import statusIcon from '../mr_widget_status_icon'; - -export default { - name: 'MRWidgetLocked', - props: { - mr: { type: Object, required: true }, - }, - components: { - statusIcon, - }, - template: ` -
- -
-

- This merge request is in the process of being merged, during which time it is locked and cannot be closed -

-
-

- The changes will be merged into - - {{mr.targetBranch}} - -

-
-
-
- `, -}; diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js new file mode 100644 index 00000000000..f6d1a4feeb2 --- /dev/null +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merging.js @@ -0,0 +1,29 @@ +import statusIcon from '../mr_widget_status_icon'; + +export default { + name: 'MRWidgetMerging', + props: { + mr: { type: Object, required: true }, + }, + components: { + statusIcon, + }, + template: ` +
+ +
+

+ This merge request is in the process of being merged +

+
+

+ The changes will be merged into + + {{mr.targetBranch}} + +

+
+
+
+ `, +}; diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js index 546a3f625c7..49340c232c8 100644 --- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js +++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js @@ -19,7 +19,7 @@ export { default as WidgetRelatedLinks } from './components/mr_widget_related_li export { default as MergedState } from './components/states/mr_widget_merged'; export { default as FailedToMerge } from './components/states/mr_widget_failed_to_merge'; export { default as ClosedState } from './components/states/mr_widget_closed'; -export { default as LockedState } from './components/states/mr_widget_locked'; +export { default as MergingState } from './components/states/mr_widget_merging'; export { default as WipState } from './components/states/mr_widget_wip'; export { default as ArchivedState } from './components/states/mr_widget_archived'; export { default as ConflictsState } from './components/states/mr_widget_conflicts'; diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 577d77f09a6..0042c48816f 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -8,7 +8,7 @@ import { WidgetRelatedLinks, MergedState, ClosedState, - LockedState, + MergingState, WipState, ArchivedState, ConflictsState, @@ -212,7 +212,7 @@ export default { 'mr-widget-related-links': WidgetRelatedLinks, 'mr-widget-merged': MergedState, 'mr-widget-closed': ClosedState, - 'mr-widget-locked': LockedState, + 'mr-widget-merging': MergingState, 'mr-widget-failed-to-merge': FailedToMerge, 'mr-widget-wip': WipState, 'mr-widget-archived': ArchivedState, diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index fddafb0ddfa..fbea764b739 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -73,6 +73,7 @@ export default class MergeRequestStore { this.canCancelAutomaticMerge = !!data.cancel_merge_when_pipeline_succeeds_path; this.hasSHAChanged = this.sha !== data.diff_head_sha; this.canBeMerged = data.can_be_merged || false; + this.mergeOngoing = data.merge_ongoing; // Cherry-pick and Revert actions related this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false; @@ -94,6 +95,11 @@ export default class MergeRequestStore { } setState(data) { + if (this.mergeOngoing) { + this.state = 'merging'; + return; + } + if (this.isOpen) { this.state = getStateKey.call(this, data); } else { @@ -104,9 +110,6 @@ export default class MergeRequestStore { case 'closed': this.state = 'closed'; break; - case 'locked': - this.state = 'locked'; - break; default: this.state = null; } diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js index 605dd3a1ff4..9074a064a6d 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js @@ -1,7 +1,7 @@ const stateToComponentMap = { merged: 'mr-widget-merged', closed: 'mr-widget-closed', - locked: 'mr-widget-locked', + merging: 'mr-widget-merging', conflicts: 'mr-widget-conflicts', missingBranch: 'mr-widget-missing-branch', workInProgress: 'mr-widget-wip', @@ -20,7 +20,7 @@ const stateToComponentMap = { }; const statesToShowHelpWidget = [ - 'locked', + 'merging', 'conflicts', 'workInProgress', 'readyToMerge', diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d361e661d0e..4de814d0ca8 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -67,11 +67,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo @noteable = @merge_request @commits_count = @merge_request.commits_count - if @merge_request.locked_long_ago? - @merge_request.unlock_mr - @merge_request.close - end - labels set_pipeline_variables diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 8ca850b6d96..e0c779b4fbc 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -61,16 +61,6 @@ class MergeRequest < ActiveRecord::Base transition locked: :opened end - after_transition any => :locked do |merge_request, transition| - merge_request.locked_at = Time.now - merge_request.save - end - - after_transition locked: (any - :locked) do |merge_request, transition| - merge_request.locked_at = nil - merge_request.save - end - state :opened state :closed state :merged @@ -392,6 +382,12 @@ class MergeRequest < ActiveRecord::Base 'Source project is not a fork of the target project' end + def merge_ongoing? + return false unless merge_jid + + Gitlab::SidekiqStatus.num_running([merge_jid]) > 0 + end + def closed_without_fork? closed? && source_project_missing? end @@ -725,12 +721,6 @@ class MergeRequest < ActiveRecord::Base end end - def locked_long_ago? - return false unless locked? - - locked_at.nil? || locked_at < (Time.now - 1.day) - end - def has_ci? has_ci_integration = source_project.try(:ci_service) uses_gitlab_ci = all_pipelines.any? diff --git a/app/serializers/merge_request_entity.rb b/app/serializers/merge_request_entity.rb index 7f17f2bf604..07650ce6f20 100644 --- a/app/serializers/merge_request_entity.rb +++ b/app/serializers/merge_request_entity.rb @@ -2,7 +2,6 @@ class MergeRequestEntity < IssuableEntity include RequestAwareEntity expose :in_progress_merge_commit_sha - expose :locked_at expose :merge_commit_sha expose :merge_error expose :merge_params @@ -32,6 +31,7 @@ class MergeRequestEntity < IssuableEntity expose :head_pipeline, with: PipelineDetailsEntity, as: :pipeline # Booleans + expose :merge_ongoing?, as: :merge_ongoing expose :work_in_progress?, as: :work_in_progress expose :source_branch_exists?, as: :source_branch_exists expose :mergeable_discussions_state?, as: :mergeable_discussions_state diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb index 48e2da338f6..c3b58df92c1 100644 --- a/app/workers/merge_worker.rb +++ b/app/workers/merge_worker.rb @@ -7,6 +7,8 @@ class MergeWorker current_user = User.find(current_user_id) merge_request = MergeRequest.find(merge_request_id) + merge_request.update_column(:merge_jid, jid) + MergeRequests::MergeService.new(merge_request.target_project, current_user, params) .execute(merge_request) end diff --git a/app/workers/stuck_merge_jobs_worker.rb b/app/workers/stuck_merge_jobs_worker.rb new file mode 100644 index 00000000000..7843179d77c --- /dev/null +++ b/app/workers/stuck_merge_jobs_worker.rb @@ -0,0 +1,34 @@ +class StuckMergeJobsWorker + include Sidekiq::Worker + include CronjobQueue + + def perform + stuck_merge_requests.find_in_batches(batch_size: 100) do |group| + jids = group.map(&:merge_jid) + + # Find the jobs that aren't currently running or that exceeded the threshold. + completed_jids = Gitlab::SidekiqStatus.completed_jids(jids) + + if completed_jids.any? + completed_ids = group.select { |merge_request| completed_jids.include?(merge_request.merge_jid) }.map(&:id) + + apply_current_state!(completed_jids, completed_ids) + end + end + end + + private + + def apply_current_state!(completed_jids, completed_ids) + 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) + + Rails.logger.info("Updated state of locked merge jobs. JIDs: #{completed_jids.join(', ')}") + end + + def stuck_merge_requests + MergeRequest.select('id, merge_jid').with_state(:locked).where.not(merge_jid: nil).reorder(nil) + end +end -- cgit v1.2.3 From 16cffa97f64f55a16a50cf861e133021f31c828f Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 7 Aug 2017 16:34:57 -0300 Subject: Move locked_at removal to post-deployment migration --- app/models/merge_request.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'app') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e0c779b4fbc..e83b11f7668 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -8,6 +8,7 @@ class MergeRequest < ActiveRecord::Base include CreatedAtFilterable ignore_column :position + ignore_column :locked_at belongs_to :target_project, class_name: "Project" belongs_to :source_project, class_name: "Project" -- cgit v1.2.3