Only read rebase status from the model

Prior to 12.1, rebase status was looked up directly from Gitaly. In
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/14417 , a DB
column was added to track the status instead. However, we couldn't stop
looking at the gitaly status immediately, since some rebases may been
running across the upgrade.

Now that we're in 12.3, it is safe to remove the direct-to-gitaly
lookup. This also happens to fix a 500 error that is seen when viewing
an MR for a fork where the source project has been removed.

We still look at the Gitaly status in the service, just in case Gitaly
and Sidekiq get out of sync - I assume this is possible, and it's a
relatively cheap check.

Since we atomically check and set `merge_requests.rebase_jid`, we
should never enqueue two `RebaseWorker` jobs in parallel.
This commit is contained in:
Nick Thomas 2019-08-15 18:54:08 +00:00 committed by Mayra Cabrera
parent cf5d64e250
commit d31b733fee
4 changed files with 4 additions and 53 deletions

View file

@ -220,18 +220,7 @@ class MergeRequest < ApplicationRecord
end end
def rebase_in_progress? def rebase_in_progress?
(rebase_jid.present? && Gitlab::SidekiqStatus.running?(rebase_jid)) || rebase_jid.present? && Gitlab::SidekiqStatus.running?(rebase_jid)
gitaly_rebase_in_progress?
end
# TODO: remove the Gitaly lookup after v12.1, when rebase_jid will be reliable
def gitaly_rebase_in_progress?
strong_memoize(:gitaly_rebase_in_progress) do
# The source project can be deleted
next false unless source_project
source_project.repository.rebase_in_progress?(id)
end
end end
# Use this method whenever you need to make sure the head_pipeline is synced with the # Use this method whenever you need to make sure the head_pipeline is synced with the

View file

@ -15,7 +15,8 @@ module MergeRequests
end end
def rebase def rebase
if merge_request.gitaly_rebase_in_progress? # Ensure Gitaly isn't already running a rebase
if source_project.repository.rebase_in_progress?(merge_request.id)
log_error('Rebase task canceled: Another rebase is already in progress', save_message_on_model: true) log_error('Rebase task canceled: Another rebase is already in progress', save_message_on_model: true)
return false return false
end end

View file

@ -3015,9 +3015,6 @@ describe MergeRequest do
subject { merge_request.rebase_in_progress? } subject { merge_request.rebase_in_progress? }
it do it do
# Stub out the legacy gitaly implementation
allow(merge_request).to receive(:gitaly_rebase_in_progress?) { false }
allow(Gitlab::SidekiqStatus).to receive(:running?).with(rebase_jid) { jid_valid } allow(Gitlab::SidekiqStatus).to receive(:running?).with(rebase_jid) { jid_valid }
merge_request.rebase_jid = rebase_jid merge_request.rebase_jid = rebase_jid
@ -3027,42 +3024,6 @@ describe MergeRequest do
end end
end end
describe '#gitaly_rebase_in_progress?' do
let(:repo_path) do
Gitlab::GitalyClient::StorageSettings.allow_disk_access do
subject.source_project.repository.path
end
end
let(:rebase_path) { File.join(repo_path, "gitlab-worktree", "rebase-#{subject.id}") }
before do
system(*%W(#{Gitlab.config.git.bin_path} -C #{repo_path} worktree add --detach #{rebase_path} master))
end
it 'returns true when there is a current rebase directory' do
expect(subject.rebase_in_progress?).to be_truthy
end
it 'returns false when there is no rebase directory' do
FileUtils.rm_rf(rebase_path)
expect(subject.rebase_in_progress?).to be_falsey
end
it 'returns false when the rebase directory has expired' do
time = 20.minutes.ago.to_time
File.utime(time, time, rebase_path)
expect(subject.rebase_in_progress?).to be_falsey
end
it 'returns false when the source project has been removed' do
allow(subject).to receive(:source_project).and_return(nil)
expect(subject.rebase_in_progress?).to be_falsey
end
end
describe '#allow_collaboration' do describe '#allow_collaboration' do
let(:merge_request) do let(:merge_request) do
build(:merge_request, source_branch: 'fixes', allow_collaboration: true) build(:merge_request, source_branch: 'fixes', allow_collaboration: true)

View file

@ -25,7 +25,7 @@ describe MergeRequests::RebaseService do
describe '#execute' do describe '#execute' do
context 'when another rebase is already in progress' do context 'when another rebase is already in progress' do
before do before do
allow(merge_request).to receive(:gitaly_rebase_in_progress?).and_return(true) allow(repository).to receive(:rebase_in_progress?).with(merge_request.id).and_return(true)
end end
it 'saves the error message' do it 'saves the error message' do