Commit 10c8a8ba authored by Stan Hu's avatar Stan Hu

Fail remote mirror if LFS sync fails

Previously for a remote mirror update, `UpdateRemoteMirrorService` would
ignore the state of `Lfs::PushService#execute`, which prevented a
project maintainer from knowing that an actual problem existed with the
sync.

We now fail the mirror, while allowing the Git push to continue. This is
guarded behind the `remote_mirror_fail_on_lfs` feature flag, and
additional logging is added to track when this happens.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/340482

Changelog: changed
parent 3267b1fc
......@@ -41,18 +41,49 @@ module Projects
remote_mirror.update_start!
# LFS objects must be sent first, or the push has dangling pointers
send_lfs_objects!(remote_mirror)
lfs_status = send_lfs_objects!(remote_mirror)
response = remote_mirror.update_repository
failed, failure_message = failure_status(lfs_status, response, remote_mirror)
# When the issue https://gitlab.com/gitlab-org/gitlab/-/issues/349262 is closed,
# we can move this block within failure_status.
if failed
remote_mirror.mark_as_failed!(failure_message)
else
remote_mirror.update_finish!
end
end
def failure_status(lfs_status, response, remote_mirror)
message = ''
failed = false
lfs_sync_failed = false
if lfs_status&.dig(:status) == :error
lfs_sync_failed = true
message += "Error synchronizing LFS files:"
message += "\n\n#{lfs_status[:message]}\n\n"
failed = Feature.enabled?(:remote_mirror_fail_on_lfs, project, default_enabled: :yaml)
end
if response.divergent_refs.any?
message = "Some refs have diverged and have not been updated on the remote:"
message += "Some refs have diverged and have not been updated on the remote:"
message += "\n\n#{response.divergent_refs.join("\n")}"
failed = true
end
remote_mirror.mark_as_failed!(message)
else
remote_mirror.update_finish!
if message.present?
Gitlab::AppJsonLogger.info(message: "Error synching remote mirror",
project_id: project.id,
project_path: project.full_path,
remote_mirror_id: remote_mirror.id,
lfs_sync_failed: lfs_sync_failed,
divergent_ref_list: response.divergent_refs)
end
[failed, message]
end
def send_lfs_objects!(remote_mirror)
......
---
name: remote_mirror_fail_on_lfs
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77339
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/349262
milestone: '14.7'
type: development
group: group::source code
default_enabled: false
......@@ -131,32 +131,82 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
expect_next_instance_of(Lfs::PushService) do |service|
expect(service).to receive(:execute)
end
expect(Gitlab::AppJsonLogger).not_to receive(:info)
execute!
expect(remote_mirror.update_status).to eq('finished')
expect(remote_mirror.last_error).to be_nil
end
it 'does nothing to an SSH repository' do
remote_mirror.update!(url: 'ssh://example.com')
context 'when LFS objects fail to push' do
before do
expect_next_instance_of(Lfs::PushService) do |service|
expect(service).to receive(:execute).and_return({ status: :error, message: 'unauthorized' })
end
end
context 'when remote_mirror_fail_on_lfs feature flag enabled' do
it 'fails update' do
expect(Gitlab::AppJsonLogger).to receive(:info).with(
hash_including(message: "Error synching remote mirror")).and_call_original
expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
execute!
execute!
end
expect(remote_mirror.update_status).to eq('failed')
expect(remote_mirror.last_error).to eq("Error synchronizing LFS files:\n\nunauthorized\n\n")
end
end
it 'does nothing if LFS is disabled' do
expect(project).to receive(:lfs_enabled?) { false }
context 'when remote_mirror_fail_on_lfs feature flag is disabled' do
before do
stub_feature_flags(remote_mirror_fail_on_lfs: false)
end
expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
it 'does not fail update' do
expect(Gitlab::AppJsonLogger).to receive(:info).with(
hash_including(message: "Error synching remote mirror")).and_call_original
execute!
execute!
expect(remote_mirror.update_status).to eq('finished')
expect(remote_mirror.last_error).to be_nil
end
end
end
it 'does nothing if non-password auth is specified' do
remote_mirror.update!(auth_method: 'ssh_public_key')
context 'with SSH repository' do
let(:ssh_mirror) { create(:remote_mirror, project: project, enabled: true) }
before do
allow(ssh_mirror)
.to receive(:update_repository)
.and_return(double(divergent_refs: []))
end
it 'does nothing to an SSH repository' do
ssh_mirror.update!(url: 'ssh://example.com')
expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
execute!
service.execute(ssh_mirror, retries)
end
it 'does nothing if LFS is disabled' do
expect(project).to receive(:lfs_enabled?) { false }
expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
service.execute(ssh_mirror, retries)
end
it 'does nothing if non-password auth is specified' do
ssh_mirror.update!(auth_method: 'ssh_public_key')
expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
service.execute(ssh_mirror, retries)
end
end
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment