Merge branch 'sh-remote-mirror-fail-on-lfs' into 'master'

Fail remote mirror if LFS sync fails

See merge request gitlab-org/gitlab!77339

app/services/projects/update_remote_mirror_service.rb

@@ -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)

config/feature_flags/development/remote_mirror_fail_on_lfs.yml

+---
+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

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -131,16 +131,65 @@ 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
+
+      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
+
+            execute!
+
+            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
+
+        context 'when remote_mirror_fail_on_lfs feature flag is disabled' do
+          before do
+            stub_feature_flags(remote_mirror_fail_on_lfs: false)
+          end
+
+          it 'does not fail update' do
+            expect(Gitlab::AppJsonLogger).to receive(:info).with(
+              hash_including(message: "Error synching remote mirror")).and_call_original
+
+            execute!
+
+            expect(remote_mirror.update_status).to eq('finished')
+            expect(remote_mirror.last_error).to be_nil
+          end
+        end
+      end
+
+      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
-        remote_mirror.update!(url: 'ssh://example.com')
+          ssh_mirror.update!(url: 'ssh://example.com')
 
           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
@@ -148,15 +197,16 @@ RSpec.describe Projects::UpdateRemoteMirrorService do
 
           expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
 
-        execute!
+          service.execute(ssh_mirror, retries)
         end
 
         it 'does nothing if non-password auth is specified' do
-        remote_mirror.update!(auth_method: 'ssh_public_key')
+          ssh_mirror.update!(auth_method: 'ssh_public_key')
 
           expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
 
-        execute!
+          service.execute(ssh_mirror, retries)
+        end
       end
     end
   end