diff --git a/changelogs/unreleased/rs-mirror-with-retry-retry.yml b/changelogs/unreleased/rs-mirror-with-retry-retry.yml new file mode 100644 index 0000000000000000000000000000000000000000..20f5669b45dce622dc70fa5abc0b4949b0d49ac9 --- /dev/null +++ b/changelogs/unreleased/rs-mirror-with-retry-retry.yml @@ -0,0 +1,5 @@ +--- +title: Retry push mirror with accepted refs +merge_request: 1899 +author: +type: added diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb index aa1a27cd241c67bb951e99d8b1d5c0feef09255d..d2abadbefec74be9a0b46a1c184318e22246dbc3 100644 --- a/ruby/lib/gitlab/git/gitlab_projects.rb +++ b/ruby/lib/gitlab/git/gitlab_projects.rb @@ -19,6 +19,9 @@ module Gitlab attr_reader :logger + # GitalyServer::FeatureFlags instance for the current call + attr_reader :feature_flags + def self.from_gitaly(gitaly_repository, call) storage_path = GitalyServer.storage_path(call) @@ -26,16 +29,19 @@ module Gitlab storage_path, gitaly_repository.relative_path, global_hooks_path: Gitlab::Git::Hook.directory, - logger: Rails.logger + logger: Rails.logger, + feature_flags: GitalyServer.feature_flags(call) ) end - def initialize(shard_path, repository_relative_path, global_hooks_path:, logger:) + def initialize(shard_path, repository_relative_path, global_hooks_path:, logger:, feature_flags: nil) @shard_path = shard_path @repository_relative_path = repository_relative_path - @logger = logger @global_hooks_path = global_hooks_path + @logger = logger + @feature_flags = feature_flags || GitalyServer::FeatureFlags.new({}) + @output = StringIO.new end diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb index 4b226fef964c1c8974f262f28b91484c17cd9453..35fb0ea81162d353f20d80e2d700c29576445e59 100644 --- a/ruby/lib/gitlab/git/repository_mirroring.rb +++ b/ruby/lib/gitlab/git/repository_mirroring.rb @@ -37,16 +37,20 @@ module Gitlab return true if success results = PushResults.new(@gitlab_projects.output) + accepted_refs = results.accepted_refs + rejected_refs = results.rejected_refs + + if @gitlab_projects.feature_flags.enabled?(:push_mirror_retry) && accepted_refs.any? + @gitlab_projects.push_branches(remote_name, GITLAB_PROJECTS_TIMEOUT, forced, accepted_refs, env: env) + else + @gitlab_projects.logger.info( + "Failed to push to remote #{remote_name}. " \ + "Accepted: #{accepted_refs.join(', ')} / " \ + "Rejected: #{rejected_refs.join(', ')}" + ) + end - accepted_refs = results.accepted_refs.join(', ') - rejected_refs = results.rejected_refs.join(', ') - - @gitlab_projects.logger.info( - "Failed to push to remote #{remote_name}. " \ - "Accepted: #{accepted_refs} / " \ - "Rejected: #{rejected_refs}" - ) - + # NOTE: Even with retry, the initial push still failed, so error out gitlab_projects_error end diff --git a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb index 8ff497f872c2379fd2d8d3b983754c7f590a52fe..55ac55b130a90cf8d665c9e0a57260a2738633b6 100644 --- a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb +++ b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb @@ -9,6 +9,7 @@ describe Gitlab::Git::GitlabProjects do let(:hooks_path) { File.join(tmp_repo_path, 'hooks') } let(:tmp_repo_path) { TEST_REPO_PATH } let(:tmp_repos_path) { DEFAULT_STORAGE_DIR } + let(:feature_flags) { double.as_null_object } if $VERBOSE let(:logger) { Logger.new(STDOUT) } @@ -20,7 +21,8 @@ describe Gitlab::Git::GitlabProjects do described_class.new( *args, global_hooks_path: hooks_path, - logger: logger + logger: logger, + feature_flags: feature_flags ) end @@ -52,6 +54,7 @@ describe Gitlab::Git::GitlabProjects do it { expect(gl_projects.repository_relative_path).to eq(repo_name) } it { expect(gl_projects.repository_absolute_path).to eq(File.join(tmp_repos_path, repo_name)) } it { expect(gl_projects.logger).to eq(logger) } + it { expect(gl_projects.feature_flags).to eq(feature_flags) } end describe '#push_branches' do diff --git a/ruby/spec/lib/gitlab/git/push_results_spec.rb b/ruby/spec/lib/gitlab/git/push_results_spec.rb index 4a2cd2e02d30b73845aa0c63705cc3f18f25c711..62205fff94ff66232db857da98cefc7cf0056588 100644 --- a/ruby/spec/lib/gitlab/git/push_results_spec.rb +++ b/ruby/spec/lib/gitlab/git/push_results_spec.rb @@ -73,7 +73,7 @@ describe Gitlab::Git::PushResults do describe Gitlab::Git::PushResults::Result do describe '#ref_name' do it 'deletes only one prefix' do - # It's valid (albeit insane) for a branch to be named `refs/tags/foo` + # It's valid (albeit insane) for a branch to be named `refs/tags/foo` ref_name = 'refs/heads/refs/tags/branch' result = described_class.new('!', ref_name, ref_name, '') diff --git a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb index 927cb863e09f93e47f2a2c77d09f891bb3e3a62b..37578bb135689957f32839f180b38793cff0047f 100644 --- a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb +++ b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb @@ -33,7 +33,13 @@ describe Gitlab::Git::RepositoryMirroring do end end - context 'with a failed push' do + context 'with a failed push and disabled retries' do + before do + allow(projects_stub) + .to receive(:feature_flags) + .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 @@ -53,7 +59,7 @@ describe Gitlab::Git::RepositoryMirroring do # Push fails expect(projects_stub).to receive(:push_branches).and_return(false) - # The CommandError gets re-raised, matching existing behavior + # The CommandError gets raised, matching existing behavior expect { repository.push_remote_branches('remote_a', %w[master develop]) } .to raise_error(Gitlab::Git::CommandError, output) @@ -61,5 +67,61 @@ describe Gitlab::Git::RepositoryMirroring do expect(logger).to have_received(:info).with(%r{Accepted: develop / Rejected: master}) end end + + context 'with a failed push and enabled retries' do + let(:feature_flags) do + GitalyServer::FeatureFlags.new('gitaly-feature-ruby-push-mirror-retry' => 'true') + end + + before do + expect(projects_stub).to receive(:feature_flags).and_return(feature_flags) + end + + it 'retries with accepted refs' do + output = "Oh no, push mirroring failed!" + + # Once for parsing, once for the exception + allow(projects_stub).to receive(:output).and_return(output) + + push_results = double( + accepted_refs: %w[develop], + rejected_refs: %w[master] + ) + expect(Gitlab::Git::PushResults).to receive(:new) + .with(output) + .and_return(push_results) + + # First push fails + expect(projects_stub) + .to receive(:push_branches) + .with('remote_a', anything, true, %w[master develop], env: {}) + .and_return(false) + + # Second push still fails, but we tried! + expect(projects_stub) + .to receive(:push_branches) + .with('remote_a', anything, true, %w[develop], env: {}) + .and_return(false) + + # The CommandError gets raised, matching existing behavior + expect { repository.push_remote_branches('remote_a', %w[master develop]) } + .to raise_error(Gitlab::Git::CommandError, output) + end + + it 'does nothing with no accepted refs' do + push_results = double( + accepted_refs: [], + rejected_refs: %w[master develop] + ) + expect(Gitlab::Git::PushResults).to receive(:new).and_return(push_results) + + # Nothing was accepted, so we only push once + expect(projects_stub).to receive(:push_branches).once.and_return(false) + + # The CommandError gets raised, matching existing behavior + expect { repository.push_remote_branches('remote_a', %w[master develop]) } + .to raise_error(Gitlab::Git::CommandError) + end + end end end