Merge branch 'mk/backfill-local-only-by-default' into 'master'

Geo: Fix package file backfill with sync object storage disabled

See merge request gitlab-org/gitlab!36771

db/migrate/20200715202659_add_index_on_package_files_file_store.rb

+# frozen_string_literal: true
+
+class AddIndexOnPackageFilesFileStore < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :packages_package_files, :file_store
+  end
+
+  def down
+    remove_concurrent_index :packages_package_files, :file_store
+  end
+end

db/structure.sql

@@ -19853,6 +19853,8 @@ CREATE INDEX index_packages_maven_metadata_on_package_id_and_path ON public.pack
 
 CREATE INDEX index_packages_nuget_dl_metadata_on_dependency_link_id ON public.packages_nuget_dependency_link_metadata USING btree (dependency_link_id);
 
+CREATE INDEX index_packages_package_files_on_file_store ON public.packages_package_files USING btree (file_store);
+
 CREATE INDEX index_packages_package_files_on_package_id_and_file_name ON public.packages_package_files USING btree (package_id, file_name);
 
 CREATE INDEX index_packages_packages_on_name_trigram ON public.packages_packages USING gin (name public.gin_trgm_ops);
@@ -23864,6 +23866,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200713071042
 20200713141854
 20200713152443
+20200715202659
 20200716044023
 20200716120419
 \.

ee/app/models/ee/packages/package_file_geo.rb

@@ -11,7 +11,22 @@ module EE
       end
 
       class_methods do
+        # @return [ActiveRecord::Relation<Packages::PackageFile>] scope of everything that should be synced
         def replicables_for_geo_node
+          selective_sync_scope.merge(object_storage_scope)
+        end
+
+        private
+
+        # @return [ActiveRecord::Relation<Packages::PackageFile>] scope observing object storage settings
+        def object_storage_scope
+          return self.all if ::Gitlab::Geo.current_node.sync_object_storage?
+
+          self.with_files_stored_locally
+        end
+
+        # @return [ActiveRecord::Relation<Packages::PackageFile>] scope observing selective sync settings
+        def selective_sync_scope
           return self.all unless ::Gitlab::Geo.current_node.selective_sync?
 
           query = ::Packages::Package.where(project_id: ::Gitlab::Geo.current_node.projects).select(:id)

ee/changelogs/unreleased/mk-backfill-local-only-by-default.yml

+---
+title: 'Geo: Package file backfill/removal accounts for object storage settings'
+merge_request: 36771
+author:
+type: fixed

ee/spec/models/packages/package_file_spec.rb

@@ -2,6 +2,8 @@
 require 'spec_helper'
 
 RSpec.describe Packages::PackageFile, type: :model do
+  include ::EE::GeoHelpers
+
   describe '#calculate_checksum!' do
     let(:package_file) { create(:conan_package_file, :conan_recipe_file) }
 
@@ -32,4 +34,139 @@ RSpec.describe Packages::PackageFile, type: :model do
       expect(Geo::BlobVerificationPrimaryWorker).to have_received(:perform_async).with('package_file', package_file.id)
     end
   end
+
+  describe '.replicables_for_geo_node' do
+    subject { described_class.replicables_for_geo_node }
+
+    it 'returns a package files scope' do
+      secondary = create(:geo_node)
+      package_file = create(:package_file)
+      stub_current_geo_node(secondary)
+
+      expect(subject).to be_an(ActiveRecord::Relation)
+      expect(subject).to include(package_file)
+    end
+
+    context 'object storage' do
+      before do
+        stub_current_geo_node(secondary)
+        stub_package_file_object_storage
+      end
+
+      let_it_be(:local_stored) { create(:package_file) }
+
+      # Cannot let_it_be because it depends on stub_package_file_object_storage
+      let!(:object_stored) { create(:package_file, :object_storage) }
+
+      context 'with sync object storage enabled' do
+        let_it_be(:secondary) { create(:geo_node, sync_object_storage: true) }
+
+        it 'includes local stored and object stored records' do
+          expect(subject).to include(local_stored)
+          expect(subject).to include(object_stored)
+        end
+      end
+
+      context 'with sync object storage disabled' do
+        let_it_be(:secondary) { create(:geo_node, sync_object_storage: false) }
+
+        it 'includes local stored and excludes object stored records' do
+          expect(subject).to include(local_stored)
+          expect(subject).not_to include(object_stored)
+        end
+      end
+    end
+
+    context 'selective sync' do
+      # Create a package file owned by a project on shard foo
+      let_it_be(:project_on_shard_foo) { create_project_on_shard('foo') }
+      let_it_be(:package_on_shard_foo) { create(:conan_package, without_package_files: true, project: project_on_shard_foo) }
+      let_it_be(:package_file_on_shard_foo) { create(:conan_package_file, package: package_on_shard_foo) }
+
+      # Create a package file owned by a project on shard bar
+      let_it_be(:project_on_shard_bar) { create_project_on_shard('bar') }
+      let_it_be(:package_on_shard_bar) { create(:conan_package, without_package_files: true, project: project_on_shard_bar) }
+      let_it_be(:package_file_on_shard_bar) { create(:conan_package_file, package: package_on_shard_bar) }
+
+      # Create a package file owned by a particular namespace, and create
+      # another package file owned via a nested group.
+      let_it_be(:root_group) { create(:group) }
+      let_it_be(:subgroup) { create(:group, parent: root_group) }
+      let_it_be(:project_in_root_group) { create(:project, group: root_group) }
+      let_it_be(:project_in_subgroup) { create(:project, group: subgroup) }
+      let_it_be(:package_in_root_group) { create(:conan_package, without_package_files: true, project: project_in_root_group) }
+      let_it_be(:package_in_subgroup) { create(:conan_package, without_package_files: true, project: project_in_subgroup) }
+      let_it_be(:package_file_in_root_group) { create(:conan_package_file, package: package_in_root_group) }
+      let_it_be(:package_file_in_subgroup) { create(:conan_package_file, package: package_in_subgroup) }
+
+      before do
+        stub_current_geo_node(secondary)
+      end
+
+      context 'without selective sync' do
+        let_it_be(:secondary) { create(:geo_node) }
+
+        it 'includes records owned by projects in all shards' do
+          expect(subject).to include(package_file_on_shard_foo)
+          expect(subject).to include(package_file_on_shard_bar)
+        end
+
+        it 'includes records owned by projects in all namespaces' do
+          expect(subject).to include(package_file_in_root_group)
+          expect(subject).to include(package_file_in_subgroup)
+        end
+      end
+
+      context 'with selective sync by shard' do
+        let_it_be(:secondary) { create(:geo_node, selective_sync_type: 'shards', selective_sync_shards: ['foo']) }
+
+        it 'includes records owned by projects on a selected shard' do
+          expect(subject).to include(package_file_on_shard_foo)
+        end
+
+        it 'excludes records owned by projects not on a selected shard' do
+          expect(subject).not_to include(package_file_on_shard_bar)
+        end
+      end
+
+      context 'with selective sync by namespace' do
+        context 'with sync object storage enabled' do
+          let_it_be(:secondary) { create(:geo_node, selective_sync_type: 'namespaces', namespaces: [root_group]) }
+
+          it 'includes records owned by projects on a selected namespace' do
+            expect(subject).to include(package_file_in_root_group)
+            expect(subject).to include(package_file_in_subgroup)
+          end
+
+          it 'excludes records owned by projects not on a selected namespace' do
+            expect(subject).not_to include(package_file_on_shard_foo)
+            expect(subject).not_to include(package_file_on_shard_bar)
+          end
+        end
+
+        # The most complex permutation
+        context 'with sync object storage disabled' do
+          let_it_be(:secondary) { create(:geo_node, selective_sync_type: 'namespaces', namespaces: [root_group], sync_object_storage: false) }
+
+          it 'includes locally stored records owned by projects on a selected namespace' do
+            expect(subject).to include(package_file_in_root_group)
+            expect(subject).to include(package_file_in_subgroup)
+          end
+
+          it 'excludes locally stored records owned by projects not on a selected namespace' do
+            expect(subject).not_to include(package_file_on_shard_foo)
+            expect(subject).not_to include(package_file_on_shard_bar)
+          end
+
+          it 'excludes object stored records owned by projects on a selected namespace' do
+            package_file_in_root_group.update_column(:file_store, ::Packages::PackageFileUploader::Store::REMOTE)
+            package_file_in_subgroup.update_column(:file_store, ::Packages::PackageFileUploader::Store::REMOTE)
+
+            expect(subject).not_to include(package_file_in_root_group)
+            expect(subject).not_to include(package_file_in_subgroup)
+          end
+        end
+      end
+    end
+  end
 end

ee/spec/support/helpers/ee/geo_helpers.rb

@@ -33,6 +33,15 @@ module EE
       ::Gitlab::ShardHealthCache.update(Array(shards))
     end
 
+    def create_project_on_shard(shard_name)
+      project = create(:project)
+
+      # skipping validation which requires the shard name to exist in Gitlab.config.repositories.storages.keys
+      project.update_column(:repository_storage, shard_name)
+
+      project
+    end
+
     def registry_factory_name(registry_class)
       registry_class.underscore.tr('/', '_').to_sym
     end