From 96df1115e69261eb20b83b3728dbaa40c8bf0ef9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 12 Feb 2020 12:20:59 -0600 Subject: [PATCH 1/5] Add Gitlab::Git::PushResults class Provides a simple interface to the output of `git push --porcelain`. --- ruby/lib/gitlab/git/push_results.rb | 77 +++++++++++++++++++ ruby/spec/lib/gitlab/git/push_results_spec.rb | 70 +++++++++++++++++ 2 files changed, 147 insertions(+) create mode 100644 ruby/lib/gitlab/git/push_results.rb create mode 100644 ruby/spec/lib/gitlab/git/push_results_spec.rb diff --git a/ruby/lib/gitlab/git/push_results.rb b/ruby/lib/gitlab/git/push_results.rb new file mode 100644 index 00000000000..15ac644f301 --- /dev/null +++ b/ruby/lib/gitlab/git/push_results.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +module Gitlab + module Git + # Parses the output of a `git push --porcelain` command + class PushResults + attr_reader :all + + def initialize(raw_output) + # If --porcelain is used, then each line of the output is of the form: + # \t : \t () + # + # See https://git-scm.com/docs/git-push#_output + @all = raw_output.each_line.map do |line| + fields = line.split("\t") + + # Sanity check for porcelain output + next unless fields.size >= 3 + + # The fast-forward flag is a space, but there's also a space before + # the tab delimiter, so we end up with a String of two spaces. Just + # take the first character. + flag = fields.shift.slice(0) + + next unless Result.valid_flag?(flag) + + from, to = fields.shift.split(':').map(&:strip) + summary = fields.shift.strip + + Result.new(flag, from, to, summary) + end.compact + end + + # Returns an Array of branch names that were not rejected nor up-to-date + def accepted_branches + all.select(&:accepted?).collect(&:branch_name) + end + + # Returns an Array of branch names that were rejected + def rejected_branches + all.select(&:rejected?).collect(&:branch_name) + end + + Result = Struct.new(:flag_char, :from, :to, :summary) do + # A single character indicating the status of the ref + FLAGS = { + ' ' => :fast_forward, # (space) for a successfully pushed fast-forward; + '+' => :forced, # + for a successful forced update; + '-' => :deleted, # - for a successfully deleted ref; + '*' => :new, # * for a successfully pushed new ref; + '!' => :rejected, # ! for a ref that was rejected or failed to push; and + '=' => :up_to_date # = for a ref that was up to date and did not need pushing. + }.freeze + + def self.valid_flag?(flag) + FLAGS.keys.include?(flag) + end + + def flag + FLAGS[flag_char] + end + + def rejected? + flag == :rejected + end + + def accepted? + !rejected? && flag != :up_to_date + end + + def branch_name + to.delete_prefix('refs/heads/') + end + end + end + end +end diff --git a/ruby/spec/lib/gitlab/git/push_results_spec.rb b/ruby/spec/lib/gitlab/git/push_results_spec.rb new file mode 100644 index 00000000000..4381721808a --- /dev/null +++ b/ruby/spec/lib/gitlab/git/push_results_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Git::PushResults do + it 'parses porcelain output' do + output = <<~OUTPUT + To gitlab.com:gitlab-org/security/gitlab-foss.git + = \t refs/heads/12-5-stable:refs/heads/12-5-stable \t [up to date] + = \t refs/heads/12-6-stable:refs/heads/12-6-stable \t [up to date] + * \t refs/heads/rs-some-new-branch:refs/heads/rs-some-new-branch \t [new branch] + \t refs/heads/rs-fast-forward:refs/heads/rs-fast-forward \t [fast-forward] + - \t refs/heads/rs-deleted:refs/heads/rs-deleted \t [deleted] + + \t refs/heads/rs-forced:refs/heads/rs-forced \t [forced] + ! \t refs/heads/12-7-stable:refs/heads/12-7-stable \t [rejected] (fetch first) + Done + error: failed to push some refs to 'git@gitlab.com:gitlab-org/security/gitlab-foss.git' + hint: Updates were rejected because the remote contains work that you do + hint: not have locally. This is usually caused by another repository pushing + hint: to the same ref. You may want to first integrate the remote changes + hint: (e.g., 'git pull ...') before pushing again. + hint: See the 'Note about fast-forwards' in 'git push --help' for details. + OUTPUT + + results = described_class.new(output) + + expect(results.all.size).to eq(7) + expect(results.accepted_branches).to contain_exactly( + 'rs-some-new-branch', + 'rs-fast-forward', + 'rs-forced', + 'rs-deleted' + ) + expect(results.rejected_branches).to contain_exactly('12-7-stable') + end + + it 'ignores non-porcelain output' do + output = <<~OUTPUT + remote: GitLab: You are not allowed to force push code to a protected branch on this project. + To + ! [remote rejected] 12-5-stable -> 12-5-stable (pre-receive hook declined) + ! [remote rejected] 12-6-stable -> 12-6-stable (pre-receive hook declined) + ! [remote rejected] 12-7-stable -> 12-7-stable (pre-receive hook declined) + ! [remote rejected] master -> master (pre-receive hook declined) + error: failed to push some refs to '[FILTERED]@gitlab.com/gitlab-org/security/gitlab-foss.git' + OUTPUT + + expect(described_class.new(output).all).to eq([]) + end + + it 'handles output without any recognizable flags' do + output = <<~OUTPUT + To gitlab.com:gitlab-org/security/gitlab-foss.git + Done + hint: Updates were rejected because the remote contains work that you do + hint: not have locally. This is usually caused by another repository pushing + hint: to the same ref. You may want to first integrate the remote changes + hint: (e.g., 'git pull ...') before pushing again. + hint: See the 'Note about fast-forwards' in 'git push --help' for details. + OUTPUT + + expect(described_class.new(output).all).to eq([]) + end + + it 'handles invalid output' do + output = 'You get nothing!' + + expect(described_class.new(output).all).to eq([]) + end +end -- GitLab From 98012422a4bfd722f5f1515cbdb6f7a7046b0e38 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 12 Feb 2020 12:31:45 -0600 Subject: [PATCH 2/5] Push with `--porcelain` flag --- ruby/lib/gitlab/git/gitlab_projects.rb | 1 + ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb index a23cdfa91b5..aa1a27cd241 100644 --- a/ruby/lib/gitlab/git/gitlab_projects.rb +++ b/ruby/lib/gitlab/git/gitlab_projects.rb @@ -73,6 +73,7 @@ module Gitlab logger.info "Pushing #{branches.count} branches from #{repository_absolute_path} to remote #{remote_name}" cmd = %W(#{Gitlab.config.git.bin_path} push) + cmd << '--porcelain' cmd << '--force' if force cmd += %W(-- #{remote_name}).concat(branches) diff --git a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb index a1ef8b51bff..8ff497f872c 100644 --- a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb +++ b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb @@ -60,10 +60,10 @@ describe Gitlab::Git::GitlabProjects do let(:force) { false } let(:branch_names) { 20.times.map { |i| "branch#{i}" } } let(:cmd1) do - %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) + branch_names[0, 10] + %W(#{Gitlab.config.git.bin_path} push --porcelain -- #{remote_name}) + branch_names[0, 10] end let(:cmd2) do - %W(#{Gitlab.config.git.bin_path} push -- #{remote_name}) + branch_names[10, 10] + %W(#{Gitlab.config.git.bin_path} push --porcelain -- #{remote_name}) + branch_names[10, 10] end subject { gl_projects.push_branches(remote_name, 600, force, branch_names, env: env) } @@ -84,7 +84,7 @@ describe Gitlab::Git::GitlabProjects do context 'with --force' do let(:branch_names) { ['master'] } - let(:cmd) { %W(#{Gitlab.config.git.bin_path} push --force -- #{remote_name} #{branch_names[0]}) } + let(:cmd) { %W(#{Gitlab.config.git.bin_path} push --porcelain --force -- #{remote_name} #{branch_names[0]}) } let(:force) { true } it 'executes the command' do -- GitLab From 2929984abc35b381104425c810edf9d0b32bc019 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 18 Feb 2020 10:38:37 -0600 Subject: [PATCH 3/5] Log details for a failed push_remote_branches call We now take the output of the CommandError, which is the `git push` output, and run it through PushResults to see which branches were rejected and which were accepted. A follow-up change will attempt to retry the push with only the accepted branches. --- ruby/lib/gitlab/git/repository_mirroring.rb | 19 +++++- .../gitlab/git/repository_mirroring_spec.rb | 64 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb diff --git a/ruby/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb index 26534051df6..31d36f27add 100644 --- a/ruby/lib/gitlab/git/repository_mirroring.rb +++ b/ruby/lib/gitlab/git/repository_mirroring.rb @@ -1,3 +1,5 @@ +require_relative 'push_results' + module Gitlab module Git module RepositoryMirroring @@ -32,7 +34,22 @@ module Gitlab 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_error + 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(', ') + + @gitlab_projects.logger.info( + "Failed to push to remote #{remote_name}. " \ + "Accepted: #{accepted_branches} / " \ + "Rejected: #{rejected_branches}" + ) + + raise ex + end end def delete_remote_branches(remote_name, branch_names, env: {}) diff --git a/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb new file mode 100644 index 00000000000..af303975fe9 --- /dev/null +++ b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Git::RepositoryMirroring do + class FakeRepository + include Gitlab::Git::RepositoryMirroring + + def initialize(projects_stub) + @gitlab_projects = projects_stub + end + + def gitlab_projects_error + raise Gitlab::Git::CommandError, @gitlab_projects.output + end + end + + describe '#push_remote_branches' do + let(:projects_stub) { double.as_null_object } + + subject(:repository) { FakeRepository.new(projects_stub) } + + context 'with a successful first push' do + it 'returns true' do + expect(projects_stub).to receive(:push_branches) + .with('remote_a', anything, true, %w[master], env: {}) + .once + .and_return(true) + + expect(projects_stub).not_to receive(:output) + + expect(repository.push_remote_branches('remote_a', %w[master])).to eq(true) + end + end + + context 'with a failed push' do + it 'logs a list of branch results and raises CommandError' do + output = "Oh no, push mirroring failed!" + logger = spy + + allow(projects_stub).to receive(:output).once.and_return(output) + allow(projects_stub).to receive(:logger).and_return(logger) + + push_results = double( + accepted_branches: %w[develop], + rejected_branches: %w[master] + ) + expect(Gitlab::Git::PushResults).to receive(:new) + .with(output) + .and_return(push_results) + + # Cause an exception via gitlab_projects_error + expect(projects_stub).to receive(:push_branches).and_return(false) + + # The CommandError gets re-raised, matching existing behavior + expect { repository.push_remote_branches('remote_a', %w[master develop]) } + .to raise_error(Gitlab::Git::CommandError, output) + + # Ensure we logged a message with the PushResults info + expect(logger).to have_received(:info).with(%r{Accepted: develop / Rejected: master}) + end + end + end +end -- GitLab From 7eeb25634dc411e1f44002c33a76d0e365138e71 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Tue, 18 Feb 2020 10:48:09 -0600 Subject: [PATCH 4/5] Add changelog for `--porcelain` flag --- changelogs/unreleased/rs-push-with-porcelain.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/rs-push-with-porcelain.yml diff --git a/changelogs/unreleased/rs-push-with-porcelain.yml b/changelogs/unreleased/rs-push-with-porcelain.yml new file mode 100644 index 00000000000..9e45166ba64 --- /dev/null +++ b/changelogs/unreleased/rs-push-with-porcelain.yml @@ -0,0 +1,5 @@ +--- +title: Push with the --porcelain flag and parse output of failed pushes +merge_request: 1845 +author: +type: added -- GitLab From e80f16e0cd73b1663d43fa79e9f1ad8a2e0fa9eb Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 12 Feb 2020 12:24:32 -0600 Subject: [PATCH 5/5] 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. --- .../unreleased/rs-mirror-with-retry.yml | 5 ++ ruby/lib/gitlab/git/repository_mirroring.rb | 32 +++++++-- .../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 00000000000..b15af9a8138 --- /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 31d36f27add..a1d75251ecc 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 af303975fe9..f958570bf3c 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 -- GitLab