Commit 0640b3d1 authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Store MergeWorker JID on merge request, and clean up stuck merges

parent b12107a0
......@@ -3,4 +3,5 @@ lib/gitlab/sanitizers/svg/whitelist.rb
lib/gitlab/diff/position_tracer.rb
app/policies/project_policy.rb
app/models/concerns/relative_positioning.rb
app/workers/stuck_merge_jobs_worker.rb
lib/gitlab/redis/*.rb
import statusIcon from '../mr_widget_status_icon';
export default {
name: 'MRWidgetLocked',
name: 'MRWidgetMerging',
props: {
mr: { type: Object, required: true },
},
......@@ -13,7 +13,7 @@ export default {
<status-icon status="loading" />
<div class="media-body">
<h4>
This merge request is in the process of being merged, during which time it is locked and cannot be closed
This merge request is in the process of being merged
</h4>
<section class="mr-info-list">
<p>
......
......@@ -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';
......
......@@ -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,
......
......@@ -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;
}
......
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',
......
......@@ -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
......
......@@ -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?
......
......@@ -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
......
......@@ -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
......
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
---
title: Unlock stuck merge request and set the proper state
merge_request: 13207
author:
......@@ -395,6 +395,10 @@ Settings.cron_jobs['remove_old_web_hook_logs_worker'] ||= Settingslogic.new({})
Settings.cron_jobs['remove_old_web_hook_logs_worker']['cron'] ||= '40 0 * * *'
Settings.cron_jobs['remove_old_web_hook_logs_worker']['job_class'] = 'RemoveOldWebHookLogsWorker'
Settings.cron_jobs['stuck_merge_jobs_worker'] ||= Settingslogic.new({})
Settings.cron_jobs['stuck_merge_jobs_worker']['cron'] ||= '0 */2 * * *'
Settings.cron_jobs['stuck_merge_jobs_worker']['job_class'] = 'StuckMergeJobsWorker'
#
# GitLab Shell
#
......
class AddMergeJidToMergeRequests < ActiveRecord::Migration
DOWNTIME = false
def change
add_column :merge_requests, :merge_jid, :string
end
end
class RemoveLockedAtColumnFromMergeRequests < ActiveRecord::Migration
DOWNTIME = false
def up
remove_column :merge_requests, :locked_at
end
def down
add_column :merge_requests, :locked_at, :datetime_with_timezone
end
end
......@@ -840,7 +840,6 @@ ActiveRecord::Schema.define(version: 20170803130232) do
t.integer "target_project_id", null: false
t.integer "iid"
t.text "description"
t.datetime "locked_at"
t.integer "updated_by_id"
t.text "merge_error"
t.text "merge_params"
......@@ -858,6 +857,7 @@ ActiveRecord::Schema.define(version: 20170803130232) do
t.integer "last_edited_by_id"
t.integer "head_pipeline_id"
t.boolean "ref_fetched"
t.string "merge_jid"
end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
......
......@@ -438,7 +438,6 @@ X-Gitlab-Event: Note Hook
"iid": 1,
"description": "Et voluptas corrupti assumenda temporibus. Architecto cum animi eveniet amet asperiores. Vitae numquam voluptate est natus sit et ad id.",
"position": 0,
"locked_at": null,
"source":{
"name":"Gitlab Test",
"description":"Aut reprehenderit ut est.",
......
......@@ -219,4 +219,17 @@ describe 'Merge request', :js do
expect(page).to have_field('remove-source-branch-input', disabled: true)
end
end
context 'ongoing merge process' do
it 'shows Merging state' do
allow_any_instance_of(MergeRequest).to receive(:merge_ongoing?).and_return(true)
visit project_merge_request_path(project, merge_request)
wait_for_requests
expect(page).not_to have_button('Merge')
expect(page).to have_content('This merge request is in the process of being merged')
end
end
end
......@@ -19,7 +19,6 @@
"human_time_estimate": { "type": ["integer", "null"] },
"human_total_time_spent": { "type": ["integer", "null"] },
"in_progress_merge_commit_sha": { "type": ["string", "null"] },
"locked_at": { "type": ["string", "null"] },
"merge_error": { "type": ["string", "null"] },
"merge_commit_sha": { "type": ["string", "null"] },
"merge_params": { "type": ["object", "null"] },
......@@ -94,7 +93,8 @@
"commit_change_content_path": { "type": "string" },
"remove_wip_path": { "type": "string" },
"commits_count": { "type": "integer" },
"remove_source_branch": { "type": ["boolean", "null"] }
"remove_source_branch": { "type": ["boolean", "null"] },
"merge_ongoing": { "type": "boolean" }
},
"additionalProperties": false
}
import Vue from 'vue';
import lockedComponent from '~/vue_merge_request_widget/components/states/mr_widget_locked';
import mergingComponent from '~/vue_merge_request_widget/components/states/mr_widget_merging';
describe('MRWidgetLocked', () => {
describe('MRWidgetMerging', () => {
describe('props', () => {
it('should have props', () => {
const { mr } = lockedComponent.props;
const { mr } = mergingComponent.props;
expect(mr.type instanceof Object).toBeTruthy();
expect(mr.required).toBeTruthy();
......@@ -13,7 +13,7 @@ describe('MRWidgetLocked', () => {
describe('template', () => {
it('should have correct elements', () => {
const Component = Vue.extend(lockedComponent);
const Component = Vue.extend(mergingComponent);
const mr = {
targetBranchPath: '/branch-path',
targetBranch: 'branch',
......@@ -24,7 +24,7 @@ describe('MRWidgetLocked', () => {
}).$el;
expect(el.classList.contains('mr-widget-body')).toBeTruthy();
expect(el.innerText).toContain('it is locked');
expect(el.innerText).toContain('This merge request is in the process of being merged');
expect(el.innerText).toContain('changes will be merged into');
expect(el.querySelector('.label-branch a').getAttribute('href')).toEqual(mr.targetBranchPath);
expect(el.querySelector('.label-branch a').textContent).toContain(mr.targetBranch);
......
......@@ -20,7 +20,6 @@ export default {
"human_time_estimate": null,
"human_total_time_spent": null,
"in_progress_merge_commit_sha": null,
"locked_at": null,
"merge_commit_sha": "53027d060246c8f47e4a9310fb332aa52f221775",
"merge_error": null,
"merge_params": {
......
......@@ -342,7 +342,7 @@ describe('mrWidgetOptions', () => {
expect(comps['mr-widget-related-links']).toBeDefined();
expect(comps['mr-widget-merged']).toBeDefined();
expect(comps['mr-widget-closed']).toBeDefined();
expect(comps['mr-widget-locked']).toBeDefined();
expect(comps['mr-widget-merging']).toBeDefined();
expect(comps['mr-widget-failed-to-merge']).toBeDefined();
expect(comps['mr-widget-wip']).toBeDefined();
expect(comps['mr-widget-archived']).toBeDefined();
......
......@@ -2534,7 +2534,6 @@
"iid": 9,
"description": null,
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -2983,7 +2982,6 @@
"iid": 8,
"description": null,
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -3267,7 +3265,6 @@
"iid": 7,
"description": "Et commodi deserunt aspernatur vero rerum. Ut non dolorum alias in odit est libero. Voluptatibus eos in et vitae repudiandae facilis ex mollitia.",
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -3551,7 +3548,6 @@
"iid": 6,
"description": "Dicta magnam non voluptates nam dignissimos nostrum deserunt. Dolorum et suscipit iure quae doloremque. Necessitatibus saepe aut labore sed.",
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -4241,7 +4237,6 @@
"iid": 5,
"description": "Est eaque quasi qui qui. Similique voluptatem impedit iusto ratione reprehenderit. Itaque est illum ut nulla aut.",
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -4789,7 +4784,6 @@
"iid": 4,
"description": "Nam magnam odit velit rerum. Sapiente dolore sunt saepe debitis. Culpa maiores ut ad dolores dolorem et.",
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -5288,7 +5282,6 @@
"iid": 3,
"description": "Libero nesciunt mollitia quis odit eos vero quasi. Iure voluptatem ut sint pariatur voluptates ut aut. Laborum possimus unde illum ipsum eum.",
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -5548,7 +5541,6 @@
"iid": 2,
"description": "Ut dolor quia aliquid dolore et nisi. Est minus suscipit enim quaerat sapiente consequatur rerum. Eveniet provident consequatur dolor accusantium reiciendis.",
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......@@ -6238,7 +6230,6 @@
"iid": 1,
"description": "Eveniet nihil ratione veniam similique qui aut sapiente tempora. Sed praesentium iusto dignissimos possimus id repudiandae quo nihil. Qui doloremque autem et iure fugit.",
"position": 0,
"locked_at": null,
"updated_by_id": null,
"merge_error": null,
"merge_params": {
......
......@@ -145,7 +145,6 @@ MergeRequest:
- iid
- description
- position
- locked_at
- updated_by_id
- merge_error
- merge_params
......
......@@ -1369,6 +1369,32 @@ describe MergeRequest do
end
end
describe '#merge_ongoing?' do
it 'returns true when merge process is ongoing for merge_jid' do
merge_request = create(:merge_request, merge_jid: 'foo')
allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(1)
expect(merge_request.merge_ongoing?).to be(true)
end
it 'returns false when no merge process running for merge_jid' do
merge_request = build(:merge_request, merge_jid: 'foo')
allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(0)
expect(merge_request.merge_ongoing?).to be(false)
end
it 'returns false when merge_jid is nil' do
merge_request = build(:merge_request, merge_jid: nil)
expect(Gitlab::SidekiqStatus).not_to receive(:num_running)
expect(merge_request.merge_ongoing?).to be(false)
end
end
describe "#closed_without_fork?" do
let(:project) { create(:project) }
let(:fork_project) { create(:project, forked_from_project: project) }
......
......@@ -47,7 +47,7 @@ describe MergeRequestEntity do
:cancel_merge_when_pipeline_succeeds_path,
:create_issue_to_resolve_discussions_path,
:source_branch_path, :target_branch_commits_path,
:target_branch_tree_path, :commits_count)
:target_branch_tree_path, :commits_count, :merge_ongoing)
end
it 'has email_patches_path' do
......
......@@ -27,4 +27,15 @@ describe MergeWorker do
expect(source_project.repository.branch_names).not_to include('markdown')
end
end
it 'persists merge_jid' do
merge_request = create(:merge_request, merge_jid: nil)
user = create(:user)
worker = described_class.new
allow(worker).to receive(:jid) { '999' }
expect { worker.perform(merge_request.id, user.id, {}) }
.to change { merge_request.reload.merge_jid }.from(nil).to('999')
end
end
require 'spec_helper'
describe StuckMergeJobsWorker do
describe 'perform' do
let(:worker) { described_class.new }
context 'merge job identified as completed' do
it 'updates merge request to merged when locked but has merge_commit_sha' do
allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(%w(123 456))
mr_with_sha = create(:merge_request, :locked, merge_jid: '123', state: :locked, merge_commit_sha: 'foo-bar-baz')
mr_without_sha = create(:merge_request, :locked, merge_jid: '123', state: :locked, merge_commit_sha: nil)
worker.perform
expect(mr_with_sha.reload).to be_merged
expect(mr_without_sha.reload).to be_opened
end
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)
worker.perform
expect(merge_request.reload).to be_opened
end
it 'logs updated stuck merge job ids' do
allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return(%w(123 456))
create(:merge_request, :locked, merge_jid: '123')
create(:merge_request, :locked, merge_jid: '456')
expect(Rails).to receive_message_chain(:logger, :info).with('Updated state of locked merge jobs. JIDs: 123, 456')
worker.perform
end
end
context 'merge job not identified as completed' do
it 'does not change merge request state when job is not completed yet' do
allow(Gitlab::SidekiqStatus).to receive(:completed_jids).and_return([])
merge_request = create(:merge_request, :locked, merge_jid: '123')
expect { worker.perform }.not_to change { merge_request.reload.state }.from('locked')
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