From c17f99d7afa5c9e25b6a9ebb7bfeb7b3bb9d20ef Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 10 Mar 2020 10:28:26 -0500 Subject: [PATCH 1/2] Add feature_flags attribute to GitlabProjects --- ruby/lib/gitlab/git/gitlab_projects.rb | 12 +++++++++--- ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb | 5 ++++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb index aa1a27cd24..d2abadbefe 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/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb index 8ff497f872..55ac55b130 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 -- GitLab From faf8946742a1b8660c3a6cf117f0479bfe031187 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 10 Mar 2020 10:33:27 -0500 Subject: [PATCH 2/2] Retry push mirror with accepted refs When the `gitaly-feature-ruby-push-mirror-retry` flag is enabled, a failed push mirror will attempt a second push with only refs that were accepted. --- .../unreleased/rs-mirror-with-retry-retry.yml | 5 ++ ruby/lib/gitlab/git/repository_mirroring.rb | 22 ++++--- ruby/spec/lib/gitlab/git/push_results_spec.rb | 2 +- .../gitlab/git/repository_mirroring_spec.rb | 66 ++++++++++++++++++- 4 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 changelogs/unreleased/rs-mirror-with-retry-retry.yml 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 0000000000..20f5669b45 --- /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/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb index 4b226fef96..35fb0ea811 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/push_results_spec.rb b/ruby/spec/lib/gitlab/git/push_results_spec.rb index 4a2cd2e02d..62205fff94 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 927cb863e0..37578bb135 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 -- GitLab