diff --git a/changelogs/unreleased/rs-mirror-with-retry.yml b/changelogs/unreleased/rs-mirror-with-retry.yml new file mode 100644 index 0000000000000000000000000000000000000000..b15af9a81389438a43564f6873777bd62a3f4523 --- /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/changelogs/unreleased/rs-push-with-porcelain.yml b/changelogs/unreleased/rs-push-with-porcelain.yml new file mode 100644 index 0000000000000000000000000000000000000000..9e45166ba648e7e2a29a9b58c3c3e5e9e8798222 --- /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 diff --git a/ruby/lib/gitlab/git/gitlab_projects.rb b/ruby/lib/gitlab/git/gitlab_projects.rb index a23cdfa91b541cc83dbb3d1f7c4cfedff2477746..aa1a27cd241c67bb951e99d8b1d5c0feef09255d 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/lib/gitlab/git/push_results.rb b/ruby/lib/gitlab/git/push_results.rb new file mode 100644 index 0000000000000000000000000000000000000000..15ac644f3016bc26082d05b46d8482e753041c84 --- /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/lib/gitlab/git/repository_mirroring.rb b/ruby/lib/gitlab/git/repository_mirroring.rb index 26534051df62eb7ab9c1ad3331a933cc754ba3bd..a1d75251ecc2a89d39c3942f730e8c8fac1690a0 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 @@ -30,9 +32,44 @@ 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_error + 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 + rejected_branches = results.rejected_branches + + @gitlab_projects.logger.info( + "Failed to push to remote #{remote_name}. " \ + "Accepted: #{accepted_branches.join(', ')} / " \ + "Rejected: #{rejected_branches.join(', ')}" + ) + + 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 def delete_remote_branches(remote_name, branch_names, env: {}) diff --git a/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb b/ruby/spec/lib/gitlab/git/gitlab_projects_spec.rb index a1ef8b51bff96e3e3192f49d01f83301d756522f..8ff497f872c2379fd2d8d3b983754c7f590a52fe 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 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 0000000000000000000000000000000000000000..4381721808a6531ab2e31b81af7df7a1280c473c --- /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 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 0000000000000000000000000000000000000000..f958570bf3c84613bf2cb08c33b4430bad72cc2e --- /dev/null +++ b/ruby/spec/lib/gitlab/git/repository_mirroring_spec.rb @@ -0,0 +1,128 @@ +# 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 } + let(:features) do + GitalyServer::FeatureFlags.new( + 'gitaly-feature-ruby-push-mirror-retry' => 'true' + ) + end + + 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 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 + + 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 + + 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