Does not sync LFS object on Object Storage when is not enabled

Add some checks to skip file transfers on a Geo secondary
node when file is stored on Object Storage but syncing
object storage is disabled or the object storage is
not configure for the data type on the secondary node.

ee/app/services/geo/file_download_service.rb

@@ -22,6 +22,7 @@ module Geo
         mark_as_synced = download_result.success || download_result.primary_missing_file
 
         log_file_download(mark_as_synced, download_result, start_time)
+
         update_registry(download_result.bytes_downloaded,
                         mark_as_synced: mark_as_synced,
                         missing_on_primary: download_result.primary_missing_file)
@@ -49,8 +50,9 @@ module Geo
         bytes_downloaded: download_result.bytes_downloaded,
         failed_before_transfer: download_result.failed_before_transfer,
         primary_missing_file: download_result.primary_missing_file,
+        reason: download_result.reason,
         download_time_s: (Time.current - start_time).to_f.round(3)
-      }
+      }.compact
 
       log_info("File download", metadata)
     end

ee/lib/gitlab/geo/replication/base_downloader.rb

@@ -12,7 +12,7 @@ module Gitlab
         end
 
         class Result
-          attr_reader :success, :bytes_downloaded, :primary_missing_file, :failed_before_transfer
+          attr_reader :success, :bytes_downloaded, :primary_missing_file, :failed_before_transfer, :reason
 
           def self.from_transfer_result(transfer_result)
             Result.new(success: transfer_result.success,
@@ -20,18 +20,19 @@ module Gitlab
                        bytes_downloaded: transfer_result.bytes_downloaded)
           end
 
-          def initialize(success:, bytes_downloaded:, primary_missing_file: false, failed_before_transfer: false)
+          def initialize(success:, bytes_downloaded:, reason: nil, primary_missing_file: false, failed_before_transfer: false)
             @success = success
             @bytes_downloaded = bytes_downloaded
             @primary_missing_file = primary_missing_file
             @failed_before_transfer = failed_before_transfer
+            @reason = reason
           end
         end
 
         private
 
-        def fail_before_transfer
-          Result.new(success: false, bytes_downloaded: 0, failed_before_transfer: true)
+        def fail_before_transfer(reason: nil)
+          Result.new(success: false, bytes_downloaded: 0, reason: reason, failed_before_transfer: true)
         end
 
         def missing_on_primary

ee/lib/gitlab/geo/replication/lfs_downloader.rb

@@ -9,13 +9,13 @@ module Gitlab
       #   * Returning a detailed Result
       #
       class LfsDownloader < BaseDownloader
-        def execute
-          lfs_object = find_resource
-          return fail_before_transfer unless lfs_object.present?
+        include ::Gitlab::Utils::StrongMemoize
 
-          transfer = ::Gitlab::Geo::Replication::LfsTransfer.new(lfs_object)
+        def execute
+          check_result = check_preconditions
+          return check_result if check_result
 
-          result = if lfs_object.local_store?
+          result = if local_store?
                      transfer.download_from_primary
                    else
                      transfer.stream_from_primary_to_object_storage
@@ -26,8 +26,42 @@ module Gitlab
 
         private
 
-        def find_resource
-          LfsObject.find_by_id(object_db_id)
+        def local_store?
+          resource.local_store?
+        end
+
+        def resource
+          strong_memoize(:resource) { LfsObject.find_by_id(object_db_id) }
+        end
+
+        def transfer
+          strong_memoize(:transfer) { ::Gitlab::Geo::Replication::LfsTransfer.new(resource) }
+        end
+
+        def check_preconditions
+          unless resource.present?
+            return fail_before_transfer(reason: "Skipping transfer as the #{object_type.to_s.capitalize} (ID = #{object_db_id}) could not be found")
+          end
+
+          unless local_store?
+            unless sync_object_storage_enabled?
+              return fail_before_transfer(reason: "Skipping transfer as this secondary node is not allowed to replicate content on Object Storage")
+            end
+
+            unless object_store_enabled?
+              return fail_before_transfer(reason: "Skipping transfer as this secondary node is not configured to store #{object_type.to_s.capitalize} on Object Storage")
+            end
+          end
+
+          nil
+        end
+
+        def object_store_enabled?
+          LfsObjectUploader.object_store_enabled?
+        end
+
+        def sync_object_storage_enabled?
+          Gitlab::Geo.current_node.sync_object_storage
         end
       end
     end

ee/spec/lib/gitlab/geo/replication/lfs_downloader_spec.rb

@@ -3,31 +3,98 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Geo::Replication::LfsDownloader, :geo do
-  let(:lfs_object) { create(:lfs_object) }
+  include ::EE::GeoHelpers
 
   describe '#execute' do
+    let_it_be(:secondary, reload: true) { create(:geo_node) }
+
+    before do
+      stub_current_geo_node(secondary)
+    end
+
     context 'with LFS object' do
-      it 'returns a FileDownloader::Result object' do
-        downloader = described_class.new(:lfs, lfs_object.id)
+      context 'on local storage' do
+        let(:lfs_object) { create(:lfs_object) }
+
+        subject(:downloader) { described_class.new(:lfs, lfs_object.id) }
+
+        it 'downloads the LFS file from the primary' do
           result = Gitlab::Geo::Replication::BaseTransfer::Result.new(success: true, bytes_downloaded: 1)
 
-        allow_next_instance_of(Gitlab::Geo::Replication::LfsTransfer) do |instance|
-          allow(instance).to receive(:download_from_primary).and_return(result)
+          expect_next_instance_of(Gitlab::Geo::Replication::LfsTransfer) do |instance|
+            expect(instance).to receive(:download_from_primary).and_return(result)
           end
 
-        expect(downloader.execute).to be_a(Gitlab::Geo::Replication::FileDownloader::Result)
+          expect(downloader.execute).to have_attributes(success: true, bytes_downloaded: 1)
         end
       end
 
-    context 'with unknown job artifact' do
-      let(:downloader) { described_class.new(:lfs, 10000) }
+      context 'on object storage' do
+        before do
+          stub_lfs_object_storage
+        end
+
+        let!(:lfs_object) { create(:lfs_object, :object_storage) }
 
-      it 'returns a FileDownloader::Result object' do
-        expect(downloader.execute).to be_a(Gitlab::Geo::Replication::FileDownloader::Result)
+        subject(:downloader) { described_class.new(:lfs, lfs_object.id) }
+
+        it 'streams the LFS file from the primary to object storage' do
+          result = Gitlab::Geo::Replication::BaseTransfer::Result.new(success: true, bytes_downloaded: 1)
+
+          expect_next_instance_of(Gitlab::Geo::Replication::LfsTransfer) do |instance|
+            expect(instance).to receive(:stream_from_primary_to_object_storage).and_return(result)
+          end
+
+          expect(downloader.execute).to have_attributes(success: true, bytes_downloaded: 1)
+        end
+
+        context 'with object storage sync disabled' do
+          before do
+            secondary.update_column(:sync_object_storage, false)
           end
 
           it 'returns a result indicating a failure before a transfer was attempted' do
-        expect(downloader.execute.failed_before_transfer).to be_truthy
+            result = downloader.execute
+
+            expect(result).to have_attributes(
+              success: false,
+              failed_before_transfer: true,
+              reason: 'Skipping transfer as this secondary node is not allowed to replicate content on Object Storage'
+            )
+          end
+        end
+
+        context 'with object storage disabled' do
+          before do
+            stub_lfs_object_storage(enabled: false)
+          end
+
+          it 'returns a result indicating a failure before a transfer was attempted' do
+            result = downloader.execute
+
+            expect(result).to have_attributes(
+              success: false,
+              failed_before_transfer: true,
+              reason: 'Skipping transfer as this secondary node is not configured to store Lfs on Object Storage'
+            )
+          end
+        end
+      end
+    end
+
+    context 'with unknown object ID' do
+      let(:unknown_id) { LfsObject.maximum(:id).to_i + 1 }
+
+      subject(:downloader) { described_class.new(:lfs, unknown_id) }
+
+      it 'returns a result indicating a failure before a transfer was attempted' do
+        result = downloader.execute
+
+        expect(result).to have_attributes(
+          success: false,
+          failed_before_transfer: true,
+          reason: "Skipping transfer as the Lfs (ID = #{unknown_id}) could not be found"
+        )
       end
     end
   end

ee/spec/services/geo/file_download_service_spec.rb

@@ -120,10 +120,14 @@ RSpec.describe Geo::FileDownloadService do
     context 'for a new file' do
       context 'when the downloader fails before attempting a transfer' do
         it 'logs that the download failed before attempting a transfer' do
-          result = double(:result, success: false, bytes_downloaded: 0, primary_missing_file: false, failed_before_transfer: true)
+          result = double(:result, success: false, bytes_downloaded: 0, primary_missing_file: false, failed_before_transfer: true, reason: 'Something went wrong')
           downloader = double(:downloader, execute: result)
-          expect(download_service).to receive(:downloader).and_return(downloader)
-          expect(Gitlab::Geo::Logger).to receive(:info).with(hash_including(:message, :download_time_s, download_success: false, bytes_downloaded: 0, failed_before_transfer: true)).and_call_original
+          allow(download_service).to receive(:downloader).and_return(downloader)
+
+          expect(Gitlab::Geo::Logger)
+            .to receive(:info)
+            .with(hash_including(:message, :download_time_s, download_success: false, reason: 'Something went wrong', bytes_downloaded: 0, failed_before_transfer: true))
+            .and_call_original
 
           execute!
         end