From e80f16e0cd73b1663d43fa79e9f1ad8a2e0fa9eb Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 12 Feb 2020 12:24:32 -0600 Subject: Add push_remote_branches retry If the first push fails, we parse the output and try again with only the branches that would have been successful, which should resolve the "one failure blocks all mirroring" behavior we currently see. This functionality is behind a `gitaly_ruby_push_mirror_retry` feature flag. --- changelogs/unreleased/rs-mirror-with-retry.yml | 5 ++ ruby/lib/gitlab/git/repository_mirroring.rb | 32 +++++++++-- .../lib/gitlab/git/repository_mirroring_spec.rb | 66 +++++++++++++++++++++- 3 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/rs-mirror-with-retry.yml diff --git a/changelogs/unreleased/rs-mirror-with-retry.yml b/changelogs/unreleased/rs-mirror-with-retry.yml new file mode 100644 index 000000000..b15af9a81 --- /dev/null +++ b/changelogs/unreleased/rs-mirror-with-retry.yml @@ -0,0 +1,5 @@ +--- +title: If a push mirror fails, try again with the refs that would have succeeded +merge_request: 1823 +author: +type: added diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb index 31d36f27a..a1d75251e 100644 --- a/ruby/lib/gitlab/git/repository_mirroring.rb +++ b/ruby/lib/gitlab/git/repository_mirroring.rb @@ -32,23 +32,43 @@ module Gitlab end def push_remote_branches(remote_name, branch_names, forced: true, env: {}) - success = @gitlab_projects.push_branches(remote_name, GITLAB_PROJECTS_TIMEOUT, forced, branch_names, env: env) + success = @gitlab_projects.push_branches( + remote_name, + GITLAB_PROJECTS_TIMEOUT, + forced, + branch_names, + env: env + ) begin success || gitlab_projects_error rescue CommandError => ex results = PushResults.new(ex.message) - accepted_branches = results.accepted_branches.join(', ') - rejected_branches = results.rejected_branches.join(', ') + accepted_branches = results.accepted_branches + rejected_branches = results.rejected_branches @gitlab_projects.logger.info( "Failed to push to remote #{remote_name}. " \ - "Accepted: #{accepted_branches} / " \ - "Rejected: #{rejected_branches}" + "Accepted: #{accepted_branches.join(', ')} / " \ + "Rejected: #{rejected_branches.join(', ')}" ) - raise ex + raise ex unless accepted_branches.any? && + @gitlab_projects.features.enabled?(:push_mirror_retry) + + @gitlab_projects.logger.info("Retrying failed push to #{remote_name} with limited branches: #{accepted_branches}") + + # Try one more push without branches that failed + success = @gitlab_projects.push_branches( + remote_name, + GITLAB_PROJECTS_TIMEOUT, + forced, + accepted_branches, + env: env + ) + + success || gitlab_projects_error end end diff --git a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb index af303975f..f958570bf 100644 --- a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb +++ b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb @@ -17,6 +17,11 @@ describe Gitlab::Git::RepositoryMirroring do describe '#push_remote_branches' do let(:projects_stub) { double.as_null_object } + let(:features) do + GitalyServer::FeatureFlags.new( + 'gitaly-feature-ruby-push-mirror-retry' => 'true' + ) + end subject(:repository) { FakeRepository.new(projects_stub) } @@ -33,7 +38,12 @@ describe Gitlab::Git::RepositoryMirroring do end end - context 'with a failed push' do + context 'with a failed push and no retry' do + before do + allow(projects_stub).to receive(:features) + .and_return(double(enabled?: false)) + end + it 'logs a list of branch results and raises CommandError' do output = "Oh no, push mirroring failed!" logger = spy @@ -60,5 +70,59 @@ describe Gitlab::Git::RepositoryMirroring do expect(logger).to have_received(:info).with(%r{Accepted: develop / Rejected: master}) end end + + context 'with a failed first push and failed retry push' do + before do + allow(projects_stub).to receive(:features).and_return(features) + end + + it 'raises a `CommandError`' do + # First push with two branches fails + expect(projects_stub).to receive(:push_branches) + .with('remote_a', anything, true, %w[master develop], env: {}) + .once + .and_return(false) + + # Only one branch was accepted + push_results = double(accepted_branches: %w[develop]).as_null_object + stub_const('Gitlab::Git::PushResults', push_results) + + # Retry push also fails, but we tried! + expect(projects_stub).to receive(:push_branches) + .with('remote_a', anything, true, %w[develop], env: {}) + .once + .and_return(false) + + allow(projects_stub).to receive(:output).and_return('output') + + expect { repository.push_remote_branches('remote_a', %w[master develop]) } + .to raise_error(Gitlab::Git::CommandError, 'output') + end + end + + context 'with a failed first push and a successful retry push' do + it 'returns true' do + # First push with two branches fails + expect(projects_stub).to receive(:push_branches) + .with('remote_a', anything, true, %w[master develop], env: {}) + .once + .and_return(false) + + # Only one branch was accepted + push_results = double(accepted_branches: %w[develop]).as_null_object + stub_const('Gitlab::Git::PushResults', push_results) + + # Retry push succeeds + expect(projects_stub).to receive(:push_branches) + .with('remote_a', anything, true, %w[develop], env: {}) + .once + .and_return(true) + + expect(projects_stub).to receive(:output).once.and_return('first_output') + + expect(repository.push_remote_branches('remote_a', %w[master develop])) + .to eq(true) + end + end end end -- cgit v1.2.3