Commit 49cb4b3d authored by Luke Duncalfe's avatar Luke Duncalfe Committed by Douwe Maan

Add support for two-step Gitaly Rebase RPC

The new two-step Gitaly `Rebase` RPC yields the rebase commit SHA to the
client before proceeding with the rebase.

This avoids an issue where the rebase commit SHA was returned when the
RPC had fully completed, and in some cases this would be after the Rails
`post_receive` worker services had already run. In these situations,
the merge request did not yet have its rebase_commit_sha attribute set
introducing the possibility for bugs (such as previous approvals being
reset).

https://gitlab.com/gitlab-org/gitlab-ee/issues/5966
parent 4e67122e
...@@ -417,7 +417,7 @@ group :ed25519 do ...@@ -417,7 +417,7 @@ group :ed25519 do
end end
# Gitaly GRPC client # Gitaly GRPC client
gem 'gitaly-proto', '~> 1.26.0', require: 'gitaly' gem 'gitaly-proto', '~> 1.27.0', require: 'gitaly'
gem 'grpc', '~> 1.19.0' gem 'grpc', '~> 1.19.0'
......
...@@ -283,7 +283,7 @@ GEM ...@@ -283,7 +283,7 @@ GEM
gettext_i18n_rails (>= 0.7.1) gettext_i18n_rails (>= 0.7.1)
po_to_json (>= 1.0.0) po_to_json (>= 1.0.0)
rails (>= 3.2.0) rails (>= 3.2.0)
gitaly-proto (1.26.0) gitaly-proto (1.27.0)
grpc (~> 1.0) grpc (~> 1.0)
github-markup (1.7.0) github-markup (1.7.0)
gitlab-default_value_for (3.1.1) gitlab-default_value_for (3.1.1)
...@@ -1056,7 +1056,7 @@ DEPENDENCIES ...@@ -1056,7 +1056,7 @@ DEPENDENCIES
gettext (~> 3.2.2) gettext (~> 3.2.2)
gettext_i18n_rails (~> 1.8.0) gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3) gettext_i18n_rails_js (~> 1.3)
gitaly-proto (~> 1.26.0) gitaly-proto (~> 1.27.0)
github-markup (~> 1.7.0) github-markup (~> 1.7.0)
gitlab-default_value_for (~> 3.1.1) gitlab-default_value_for (~> 3.1.1)
gitlab-labkit (~> 0.2.0) gitlab-labkit (~> 0.2.0)
......
...@@ -1037,11 +1037,41 @@ class Repository ...@@ -1037,11 +1037,41 @@ class Repository
raw_repository.fetch_ref(source_repository.raw_repository, source_ref: source_ref, target_ref: target_ref) raw_repository.fetch_ref(source_repository.raw_repository, source_ref: source_ref, target_ref: target_ref)
end end
# DEPRECATED: https://gitlab.com/gitlab-org/gitaly/issues/1628
def rebase_deprecated(user, merge_request)
rebase_sha = raw.rebase_deprecated(
user,
merge_request.id,
branch: merge_request.source_branch,
branch_sha: merge_request.source_branch_sha,
remote_repository: merge_request.target_project.repository.raw,
remote_branch: merge_request.target_branch
)
# To support the full deprecated behaviour, set the
# `rebase_commit_sha` for the merge_request here and return the value
merge_request.update(rebase_commit_sha: rebase_sha)
rebase_sha
end
def rebase(user, merge_request) def rebase(user, merge_request)
raw.rebase(user, merge_request.id, branch: merge_request.source_branch, if Feature.disabled?(:two_step_rebase, default_enabled: true)
branch_sha: merge_request.source_branch_sha, return rebase_deprecated(user, merge_request)
remote_repository: merge_request.target_project.repository.raw, end
remote_branch: merge_request.target_branch)
MergeRequest.transaction do
raw.rebase(
user,
merge_request.id,
branch: merge_request.source_branch,
branch_sha: merge_request.source_branch_sha,
remote_repository: merge_request.target_project.repository.raw,
remote_branch: merge_request.target_branch
) do |commit_id|
merge_request.update!(rebase_commit_sha: commit_id)
end
end
end end
def squash(user, merge_request, message) def squash(user, merge_request, message)
......
...@@ -20,17 +20,7 @@ module MergeRequests ...@@ -20,17 +20,7 @@ module MergeRequests
return false return false
end end
log_prefix = "#{self.class.name} info (#{merge_request.to_reference(full: true)}):" repository.rebase(current_user, merge_request)
Gitlab::GitLogger.info("#{log_prefix} rebase started")
rebase_sha = repository.rebase(current_user, merge_request)
Gitlab::GitLogger.info("#{log_prefix} rebased to #{rebase_sha}")
merge_request.update(rebase_commit_sha: rebase_sha)
Gitlab::GitLogger.info("#{log_prefix} rebase SHA saved: #{rebase_sha}")
true true
rescue => e rescue => e
......
---
title: Fix approvals sometimes being reset after a merge request is rebased
merge_request: 27446
author:
type: fixed
...@@ -834,7 +834,8 @@ module Gitlab ...@@ -834,7 +834,8 @@ module Gitlab
gitaly_repository_client.create_from_snapshot(url, auth) gitaly_repository_client.create_from_snapshot(url, auth)
end end
def rebase(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_branch:) # DEPRECATED: https://gitlab.com/gitlab-org/gitaly/issues/1628
def rebase_deprecated(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_branch:)
wrapped_gitaly_errors do wrapped_gitaly_errors do
gitaly_operation_client.user_rebase(user, rebase_id, gitaly_operation_client.user_rebase(user, rebase_id,
branch: branch, branch: branch,
...@@ -844,6 +845,20 @@ module Gitlab ...@@ -844,6 +845,20 @@ module Gitlab
end end
end end
def rebase(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_branch:, &block)
wrapped_gitaly_errors do
gitaly_operation_client.rebase(
user,
rebase_id,
branch: branch,
branch_sha: branch_sha,
remote_repository: remote_repository,
remote_branch: remote_branch,
&block
)
end
end
def rebase_in_progress?(rebase_id) def rebase_in_progress?(rebase_id)
wrapped_gitaly_errors do wrapped_gitaly_errors do
gitaly_repository_client.rebase_in_progress?(rebase_id) gitaly_repository_client.rebase_in_progress?(rebase_id)
......
...@@ -197,6 +197,7 @@ module Gitlab ...@@ -197,6 +197,7 @@ module Gitlab
start_repository: start_repository) start_repository: start_repository)
end end
# DEPRECATED: https://gitlab.com/gitlab-org/gitaly/issues/1628
def user_rebase(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_branch:) def user_rebase(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_branch:)
request = Gitaly::UserRebaseRequest.new( request = Gitaly::UserRebaseRequest.new(
repository: @gitaly_repo, repository: @gitaly_repo,
...@@ -225,6 +226,49 @@ module Gitlab ...@@ -225,6 +226,49 @@ module Gitlab
end end
end end
def rebase(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_branch:)
request_enum = QueueEnumerator.new
rebase_sha = nil
response_enum = GitalyClient.call(
@repository.storage,
:operation_service,
:user_rebase_confirmable,
request_enum.each,
remote_storage: remote_repository.storage
)
# First request
request_enum.push(
Gitaly::UserRebaseConfirmableRequest.new(
header: Gitaly::UserRebaseConfirmableRequest::Header.new(
repository: @gitaly_repo,
user: Gitlab::Git::User.from_gitlab(user).to_gitaly,
rebase_id: rebase_id.to_s,
branch: encode_binary(branch),
branch_sha: branch_sha,
remote_repository: remote_repository.gitaly_repository,
remote_branch: encode_binary(remote_branch)
)
)
)
perform_next_gitaly_rebase_request(response_enum) do |response|
rebase_sha = response.rebase_sha
end
yield rebase_sha
# Second request confirms with gitaly to finalize the rebase
request_enum.push(Gitaly::UserRebaseConfirmableRequest.new(apply: true))
perform_next_gitaly_rebase_request(response_enum)
rebase_sha
ensure
request_enum.close
end
def user_squash(user, squash_id, branch, start_sha, end_sha, author, message) def user_squash(user, squash_id, branch, start_sha, end_sha, author, message)
request = Gitaly::UserSquashRequest.new( request = Gitaly::UserSquashRequest.new(
repository: @gitaly_repo, repository: @gitaly_repo,
...@@ -346,6 +390,20 @@ module Gitlab ...@@ -346,6 +390,20 @@ module Gitlab
private private
def perform_next_gitaly_rebase_request(response_enum)
response = response_enum.next
if response.pre_receive_error.present?
raise Gitlab::Git::PreReceiveError, response.pre_receive_error
elsif response.git_error.present?
raise Gitlab::Git::Repository::GitError, response.git_error
end
yield response if block_given?
response
end
def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:) def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
request_class = "Gitaly::User#{rpc.to_s.camelcase}Request".constantize request_class = "Gitaly::User#{rpc.to_s.camelcase}Request".constantize
......
...@@ -1451,6 +1451,91 @@ describe Repository do ...@@ -1451,6 +1451,91 @@ describe Repository do
end end
end end
describe '#rebase' do
let(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project) }
shared_examples_for 'a method that can rebase successfully' do
it 'returns the rebase commit sha' do
rebase_commit_sha = repository.rebase(user, merge_request)
head_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha
expect(rebase_commit_sha).to eq(head_sha)
end
it 'sets the `rebase_commit_sha` for the given merge request' do
rebase_commit_sha = repository.rebase(user, merge_request)
expect(rebase_commit_sha).not_to be_nil
expect(merge_request.rebase_commit_sha).to eq(rebase_commit_sha)
end
end
context 'when two_step_rebase feature is enabled' do
before do
stub_feature_flags(two_step_rebase: true)
end
it_behaves_like 'a method that can rebase successfully'
it 'executes the new Gitaly RPC' do
expect_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:rebase)
expect_any_instance_of(Gitlab::GitalyClient::OperationService).not_to receive(:user_rebase)
repository.rebase(user, merge_request)
end
describe 'rolling back the `rebase_commit_sha`' do
let(:new_sha) { Digest::SHA1.hexdigest('foo') }
it 'does not rollback when there are no errors' do
second_response = double(pre_receive_error: nil, git_error: nil)
mock_gitaly(second_response)
repository.rebase(user, merge_request)
expect(merge_request.reload.rebase_commit_sha).to eq(new_sha)
end
it 'does rollback when an error is encountered in the second step' do
second_response = double(pre_receive_error: 'my_error', git_error: nil)
mock_gitaly(second_response)
expect do
repository.rebase(user, merge_request)
end.to raise_error(Gitlab::Git::PreReceiveError)
expect(merge_request.reload.rebase_commit_sha).to be_nil
end
def mock_gitaly(second_response)
responses = [
double(rebase_sha: new_sha).as_null_object,
second_response
]
expect_any_instance_of(
Gitaly::OperationService::Stub
).to receive(:user_rebase_confirmable).and_return(responses.each)
end
end
end
context 'when two_step_rebase feature is disabled' do
before do
stub_feature_flags(two_step_rebase: false)
end
it_behaves_like 'a method that can rebase successfully'
it 'executes the deprecated Gitaly RPC' do
expect_any_instance_of(Gitlab::GitalyClient::OperationService).to receive(:user_rebase)
expect_any_instance_of(Gitlab::GitalyClient::OperationService).not_to receive(:rebase)
repository.rebase(user, merge_request)
end
end
end
describe '#revert' do describe '#revert' do
let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') } let(:new_image_commit) { repository.commit('33f3729a45c02fc67d00adb1b8bca394b0e761d9') }
let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } let(:update_image_commit) { repository.commit('2f63565e7aac07bcdadb654e253078b727143ec4') }
......
...@@ -73,7 +73,7 @@ describe MergeRequests::RebaseService do ...@@ -73,7 +73,7 @@ describe MergeRequests::RebaseService do
end end
context 'valid params' do context 'valid params' do
describe 'successful rebase' do shared_examples_for 'a service that can execute a successful rebase' do
before do before do
service.execute(merge_request) service.execute(merge_request)
end end
...@@ -99,6 +99,22 @@ describe MergeRequests::RebaseService do ...@@ -99,6 +99,22 @@ describe MergeRequests::RebaseService do
end end
end end
context 'when the two_step_rebase feature is enabled' do
before do
stub_feature_flags(two_step_rebase: true)
end
it_behaves_like 'a service that can execute a successful rebase'
end
context 'when the two_step_rebase feature is disabled' do
before do
stub_feature_flags(two_step_rebase: false)
end
it_behaves_like 'a service that can execute a successful rebase'
end
context 'fork' do context 'fork' do
describe 'successful fork rebase' do describe 'successful fork rebase' do
let(:forked_project) do let(:forked_project) do
......
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