Merge branch '4163-move-uploads-to-object-storage-vsizov' into 'master'

[Geo] Skip attachments that is stored in the object storage

Closes #4626

See merge request gitlab-org/gitlab-ee!4346

app/models/upload.rb

@@ -9,6 +9,8 @@ class Upload < ActiveRecord::Base
   validates :model, presence: true
   validates :uploader, presence: true
 
+  scope :with_files_stored_locally, -> { where(store: [nil, ObjectStorage::Store::LOCAL]) }
+
   before_save  :calculate_checksum!, if: :foreground_checksummable?
   after_commit :schedule_checksum,   if: :checksummable?
 

changelogs/unreleased-ee/4163-move-uploads-to-object-storage-vsizov.yml

+---
+title: "[Geo] Skip attachments that is stored in the object storage"
+merge_request:
+author:
+type: fixed

ee/app/finders/geo/attachment_registry_finder.rb

 module Geo
   class AttachmentRegistryFinder < FileRegistryFinder
     def attachments
-      if selective_sync?
-        Upload.where(group_uploads.or(project_uploads).or(other_uploads))
-      else
-        Upload.all
-      end
+      relation =
+        if selective_sync?
+          Upload.where(group_uploads.or(project_uploads).or(other_uploads))
+        else
+          Upload.all
+        end
+
+      relation.with_files_stored_locally
     end
 
     def count_attachments
@@ -114,6 +117,7 @@ module Geo
       fdw_table = Geo::Fdw::Upload.table_name
 
       Geo::Fdw::Upload.joins("INNER JOIN file_registry ON file_registry.file_id = #{fdw_table}.id")
+        .with_files_stored_locally
         .merge(Geo::FileRegistry.attachments)
     end
 
@@ -124,6 +128,7 @@ module Geo
       Geo::Fdw::Upload.joins("LEFT OUTER JOIN file_registry
                                            ON file_registry.file_id = #{fdw_table}.id
                                           AND file_registry.file_type IN (#{upload_types})")
+        .with_files_stored_locally
         .where(file_registry: { id: nil })
         .where.not(id: except_registry_ids)
     end

ee/app/finders/geo/job_artifact_registry_finder.rb

@@ -78,7 +78,7 @@ module Geo
       Geo::Fdw::Ci::JobArtifact.joins("LEFT OUTER JOIN file_registry
                                               ON file_registry.file_id = #{fdw_table}.id
                                              AND file_registry.file_type = 'job_artifact'")
-        .merge(Geo::Fdw::Ci::JobArtifact.with_files_stored_locally)
+        .with_files_stored_locally
         .where(file_registry: { id: nil })
         .where.not(id: except_registry_ids)
     end

ee/app/finders/geo/lfs_object_registry_finder.rb

@@ -79,7 +79,7 @@ module Geo
       Geo::Fdw::LfsObject.joins("LEFT OUTER JOIN file_registry
                                               ON file_registry.file_id = #{fdw_table}.id
                                              AND file_registry.file_type = 'lfs'")
-        .merge(Geo::Fdw::LfsObject.with_files_stored_locally)
+        .with_files_stored_locally
         .where(file_registry: { id: nil })
         .where.not(id: except_registry_ids)
     end

ee/app/models/geo/fdw/upload.rb

@@ -2,6 +2,8 @@ module Geo
   module Fdw
     class Upload < ::Geo::BaseFdw
       self.table_name = Gitlab::Geo.fdw_table('uploads')
+
+      scope :with_files_stored_locally, -> { where(store: [nil, ObjectStorage::Store::LOCAL]) }
     end
   end
 end

spec/ee/spec/finders/geo/attachment_registry_finder_spec.rb

@@ -18,6 +18,8 @@ describe Geo::AttachmentRegistryFinder, :geo do
   let(:upload_5) { create(:upload, model: synced_project) }
   let(:upload_6) { create(:upload, :personal_snippet_upload) }
   let(:upload_7) { create(:upload, model: synced_subgroup) }
+  let(:upload_8) { create(:upload, :object_storage, model: unsynced_project) }
+  let(:upload_9) { create(:upload, :object_storage, model: unsynced_group) }
   let(:lfs_object) { create(:lfs_object) }
 
   subject { described_class.new(current_node: secondary) }
@@ -117,6 +119,14 @@ describe Geo::AttachmentRegistryFinder, :geo do
 
         expect(uploads.map(&:id)).to match_array([upload_3.id, upload_4.id])
       end
+
+      it 'excludes remote uploads without an entry on the tracking database' do
+        create(:geo_file_registry, :avatar, file_id: upload_1.id, success: true)
+
+        uploads = subject.find_unsynced_attachments(batch_size: 10)
+
+        expect(uploads).not_to include(upload_8, upload_9)
+      end
     end
   end
 
@@ -250,7 +260,7 @@ describe Geo::AttachmentRegistryFinder, :geo do
         subject.find_unsynced_attachments(batch_size: 10)
       end
 
-      it 'returns LFS objects without an entry on the tracking database' do
+      it 'returns uploads without an entry on the tracking database' do
         create(:geo_file_registry, :avatar, file_id: upload_1.id, success: true)
 
         uploads = subject.find_unsynced_attachments(batch_size: 10)
@@ -265,6 +275,14 @@ describe Geo::AttachmentRegistryFinder, :geo do
 
         expect(uploads).to match_array([upload_3, upload_4])
       end
+
+      it 'excludes remote uploads without an entry on the tracking database' do
+        create(:geo_file_registry, :avatar, file_id: upload_1.id, success: true)
+
+        uploads = subject.find_unsynced_attachments(batch_size: 10)
+
+        expect(uploads).not_to include(upload_8, upload_9)
+      end
     end
   end
 end

spec/factories/uploads.rb

@@ -25,6 +25,10 @@ FactoryBot.define do
       uploader "FileUploader"
     end
 
+    trait :object_storage do
+      store ObjectStorage::Store::REMOTE
+    end
+
     trait :namespace_upload do
       model { build(:group) }
       path { File.join(secret, 'myfile.jpg') }