Merge branch 'mk/apply-selective-sync-to-design-repo-update' into 'master'

Geo: Apply selective sync to design repo updates

Closes #233505

See merge request gitlab-org/gitlab!39916

ee/changelogs/unreleased/mk-apply-selective-sync-to-design-repo-update.yml

+---
+title: 'Geo: Apply selective sync to design repo updates'
+merge_request: 39916
+author:
+type: fixed

ee/lib/gitlab/geo/log_cursor/events/design_repository_updated_event.rb

@@ -9,8 +9,10 @@ module Gitlab
 
           def process
             job_id =
-              unless skippable?
+              if replicable_project?(event.project_id)
                 registry.repository_updated!
+                registry.save
+
                 schedule_job(event)
               end
 
@@ -20,7 +22,7 @@ module Gitlab
           private
 
           def registry
-            @registry ||= ::Geo::DesignRegistry.safe_find_or_create_by(project_id: event.project_id)
+            @registry ||= ::Geo::DesignRegistry.find_or_initialize_by(project_id: event.project_id) # rubocop: disable CodeReuse/ActiveRecord
           end
 
           def schedule_job(event)
@@ -34,7 +36,7 @@ module Gitlab
               'Design repository update',
               project_id: event.project_id,
               scheduled_at: Time.now,
-              skippable: skippable?,
+              replicable_project: replicable_project?(event.project_id),
               job_id: job_id)
           end
         end

ee/spec/factories/geo_nodes.rb

@@ -22,6 +22,11 @@ FactoryBot.define do
     trait :local_storage_only do
       sync_object_storage { false }
     end
+
+    trait :selective_sync_excludes_all_projects do
+      selective_sync_type { 'shards' }
+      selective_sync_shards { ['non-existent'] }
+    end
   end
 
   factory :geo_node_with_selective_sync_for, parent: :geo_node do

ee/spec/lib/gitlab/geo/log_cursor/events/container_repository_updated_event_spec.rb

@@ -6,18 +6,19 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::ContainerRepositoryUpdatedEvent,
   include ::EE::GeoHelpers
 
   let_it_be(:secondary) { create(:geo_node) }
-  let_it_be(:selective_sync_secondary) { create(:geo_node, selective_sync_type: 'shards', selective_sync_shards: ['non-existent']) }
+  let_it_be(:secondary_excludes_all_projects) { create(:geo_node, :selective_sync_excludes_all_projects) }
   let(:logger) { Gitlab::Geo::LogCursor::Logger.new(described_class, Logger::INFO) }
   let(:event_log) { create(:geo_event_log, :container_repository_updated_event) }
   let!(:event_log_state) { create(:geo_event_log_state, event_id: event_log.id - 1) }
   let(:container_repository_updated_event) { event_log.container_repository_updated_event }
   let(:container_repository) { container_repository_updated_event.container_repository }
+  let(:sync_worker_class) { ::Geo::ContainerRepositorySyncWorker }
+  let(:registry_class) { ::Geo::ContainerRepositoryRegistry }
+  let(:registry_factory) { :geo_container_repository_registry }
+  let(:registry_factory_args) { [:synced, container_repository: container_repository] }
+  let(:sync_worker_expected_arg) { container_repository.id }
 
-  subject { described_class.new(container_repository_updated_event, Time.now, logger) }
-
-  around do |example|
-    Sidekiq::Testing.fake! { example.run }
-  end
+  subject(:event) { described_class.new(container_repository_updated_event, Time.now, logger) }
 
   before do
     stub_current_geo_node(secondary)
@@ -29,57 +30,46 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::ContainerRepositoryUpdatedEvent,
         stub_config(geo: { registry_replication: { enabled: true } })
       end
 
-      context 'when a registry does not yet exist' do
       context "when the container repository's project is not excluded by selective sync" do
-          # TODO: Fix the bug and un-x the test https://gitlab.com/gitlab-org/gitlab/-/issues/233514
-          xit 'creates a registry' do
-            expect { subject.process }.to change(Geo::ContainerRepositoryRegistry, :count).by(1)
-          end
-
-          it_behaves_like 'event schedules a sync worker', ::Geo::ContainerRepositorySyncWorker do
-            let(:expected_id) { container_repository.id }
-          end
-
-          it_behaves_like 'logs event source info'
-        end
+        # TODO: Fix https://gitlab.com/gitlab-org/gitlab/-/issues/233514 and
+        # use this test, and remove the other tests in this context.
+        # it_behaves_like 'event should trigger a sync'
 
-        context "when the container repository's project is excluded by selective sync" do
-          before do
-            stub_current_geo_node(selective_sync_secondary)
-          end
-
-          it_behaves_like 'event does not create a registry', ::Geo::ContainerRepositoryRegistry
-          it_behaves_like 'event does not schedule a sync worker', ::Geo::ContainerRepositorySyncWorker
+        context 'when a registry does not yet exist' do
+          it_behaves_like 'event schedules a sync worker'
           it_behaves_like 'logs event source info'
         end
-      end
 
         context 'when a registry exists' do
-        let!(:registry) { create(:geo_container_repository_registry, :synced) }
-
-        context "when the container repository's project is not excluded by selective sync" do
-          # TODO: Fix the bug and un-x the test https://gitlab.com/gitlab-org/gitlab/-/issues/233514
-          xit 'transitions the registry to pending' do
-            expect { subject.process }.to change(registry, :pending?).to(true)
-          end
-
-          it_behaves_like 'event schedules a sync worker', ::Geo::ContainerRepositorySyncWorker do
-            let(:expected_id) { container_repository.id }
-          end
+          let!(:registry) { create(registry_factory, *registry_factory_args) }
 
+          it_behaves_like 'event schedules a sync worker'
           it_behaves_like 'logs event source info'
         end
+      end
 
       context "when the container repository's project is excluded by selective sync" do
         before do
-            stub_current_geo_node(selective_sync_secondary)
+          stub_current_geo_node(secondary_excludes_all_projects)
         end
 
-          it 'does not transition the registry to pending state' do
-            expect { subject.process }.not_to change(registry, :pending?)
+        context 'when a registry does not exist' do
+          it_behaves_like 'event does not create a registry'
+          it_behaves_like 'event does not schedule a sync worker'
+          it_behaves_like 'logs event source info'
         end
 
-          it_behaves_like 'event does not schedule a sync worker', ::Geo::ContainerRepositorySyncWorker
+        context 'when a registry exists' do
+          let!(:registry) { create(registry_factory, *registry_factory_args) }
+
+          # This describes an optimization to avoid double-checking a heavy
+          # (330ms is heavy for the log cursor) selective sync query too often:
+          # If the registry exists, then we assume it *should* exist. This will
+          # usually be accurate. The responsibility falls to proper handling of
+          # delete events as well as the `RegistryConsistencyWorker` to remove
+          # registries.
+          it_behaves_like 'event transitions a registry to pending'
+          it_behaves_like 'event schedules a sync worker'
           it_behaves_like 'logs event source info'
         end
       end
@@ -90,9 +80,17 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::ContainerRepositoryUpdatedEvent,
         stub_config(geo: { registry_replication: { enabled: false } })
       end
 
-      context "even when the container repository's project is not excluded by selective sync" do
-        it_behaves_like 'event does not create a registry', ::Geo::ContainerRepositoryRegistry
-        it_behaves_like 'event does not schedule a sync worker', ::Geo::ContainerRepositorySyncWorker
+      context 'when a registry does not exist' do
+        it_behaves_like 'event does not create a registry'
+        it_behaves_like 'event does not schedule a sync worker'
+        it_behaves_like 'logs event source info'
+      end
+
+      context 'when a registry exists' do
+        let!(:registry) { create(registry_factory, *registry_factory_args) }
+
+        it_behaves_like 'event does not transition a registry to pending'
+        it_behaves_like 'event does not schedule a sync worker'
         it_behaves_like 'logs event source info'
       end
     end

ee/spec/lib/gitlab/geo/log_cursor/events/design_repository_updated_event_spec.rb

@@ -6,47 +6,58 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::DesignRepositoryUpdatedEvent, :cl
   include ::EE::GeoHelpers
 
   let_it_be(:secondary) { create(:geo_node) }
-
+  let_it_be(:secondary_excludes_all_projects) { create(:geo_node, :selective_sync_excludes_all_projects) }
   let(:logger) { Gitlab::Geo::LogCursor::Logger.new(described_class, Logger::INFO) }
   let(:project) { create(:project) }
-  let(:event) { create(:geo_design_repository_updated_event, project: project) }
-  let(:event_log) { create(:geo_event_log, repository_updated_event: event) }
+  let(:design_repository_updated_event) { create(:geo_design_repository_updated_event, project: project) }
+  let(:event_log) { create(:geo_event_log, repository_updated_event: design_repository_updated_event) }
   let!(:event_log_state) { create(:geo_event_log_state, event_id: event_log.id - 1) }
+  let(:sync_worker_class) { ::Geo::DesignRepositorySyncWorker }
+  let(:registry_class) { ::Geo::DesignRegistry }
+  let(:registry_factory) { :geo_design_registry }
+  let(:registry_factory_args) { [:synced, project: project] }
+  let(:sync_worker_expected_arg) { project.id }
 
-  subject { described_class.new(event, Time.now, logger) }
-
-  around do |example|
-    Sidekiq::Testing.fake! { example.run }
-  end
+  subject(:event) { described_class.new(design_repository_updated_event, Time.now, logger) }
 
   before do
     stub_current_geo_node(secondary)
   end
 
-  shared_examples 'DesignRepositoryUpdatedEvent' do
-    it 'creates a new registry when a design registry does not exist' do
-      expect { subject.process }.to change(Geo::DesignRegistry, :count).by(1)
-    end
-
-    it 'marks registry as pending when a design registry exists' do
-      registry = create(:geo_design_registry, :synced, project: project)
-
-      expect { subject.process }.to change { registry.reload.state }.from('synced').to('pending')
-    end
-  end
-
   describe '#process' do
     context 'when the associated shard is healthy' do
       before do
         allow(Gitlab::ShardHealthCache).to receive(:healthy_shard?).with('default').and_return(true)
       end
 
-      it_behaves_like 'DesignRepositoryUpdatedEvent'
+      context "when the container repository's project is not excluded by selective sync" do
+        it_behaves_like 'event should trigger a sync'
+      end
+
+      context "when the container repository's project is excluded by selective sync" do
+        before do
+          stub_current_geo_node(secondary_excludes_all_projects)
+        end
 
-      it 'schedules a Geo::DesignRepositorySyncWorker' do
-        expect(Geo::DesignRepositorySyncWorker).to receive(:perform_async).with(project.id).once
+        context 'when a registry does not yet exist' do
+          it_behaves_like 'event does not create a registry'
+          it_behaves_like 'event does not schedule a sync worker'
+          it_behaves_like 'logs event source info'
+        end
 
-        subject.process
+        # This describes an optimization to avoid double-checking a heavy (330ms
+        # is heavy for the log cursor) selective sync query too often:
+        # If the registry exists, then we assume it *should* exist. This will
+        # usually be accurate. The responsibility falls to proper handling of
+        # delete events as well as the `RegistryConsistencyWorker` to remove
+        # registries.
+        context 'when a registry exists' do
+          let!(:registry) { create(registry_factory, *registry_factory_args) }
+
+          it_behaves_like 'event transitions a registry to pending'
+          it_behaves_like 'event schedules a sync worker'
+          it_behaves_like 'logs event source info'
+        end
       end
     end
 
@@ -55,15 +66,19 @@ RSpec.describe Gitlab::Geo::LogCursor::Events::DesignRepositoryUpdatedEvent, :cl
         allow(Gitlab::ShardHealthCache).to receive(:healthy_shard?).with('default').and_return(false)
       end
 
-      it_behaves_like 'DesignRepositoryUpdatedEvent'
+      context 'when a registry does not yet exist' do
+        it_behaves_like 'event creates a registry'
+        it_behaves_like 'event does not schedule a sync worker'
+        it_behaves_like 'logs event source info'
+      end
 
-      it 'does not schedule a Geo::DesignRepositorySyncWorker job' do
-        expect(Geo::DesignRepositorySyncWorker).not_to receive(:perform_async).with(project.id)
+      context 'when a registry exists' do
+        let!(:registry) { create(registry_factory, *registry_factory_args) }
 
-        subject.process
+        it_behaves_like 'event transitions a registry to pending'
+        it_behaves_like 'event does not schedule a sync worker'
+        it_behaves_like 'logs event source info'
       end
     end
-
-    it_behaves_like 'logs event source info'
   end
 end

ee/spec/support/shared_examples/lib/gitlab/geo/geo_log_cursor_event_shared_examples.rb

 # frozen_string_literal: true
 
-RSpec.shared_examples 'event does not create a registry' do |registry_class|
+# Let variables required:
+#
+# - event
+# - registry_class
+#
+RSpec.shared_examples 'event creates a registry' do
+  it 'creates a registry with pending state' do
+    expect { event.process }.to change(registry_class.with_state(:pending), :count).by(1)
+  end
+end
+
+# Let variables required:
+#
+# - event
+# - registry_class
+#
+RSpec.shared_examples 'event does not create a registry' do
   it 'does not create a registry' do
-    expect { subject.process }.not_to change(registry_class, :count)
+    expect { event.process }.not_to change(registry_class, :count)
   end
 end
 
 # Let variables required:
 #
-# - expected_id
+# - event
+# - registry
 #
-RSpec.shared_examples 'event schedules a sync worker' do |sync_worker|
+RSpec.shared_examples 'event transitions a registry to pending' do
+  it 'transitions the registry to pending' do
+    event.process
+
+    expect(registry.reload.pending?).to be_truthy
+  end
+end
+
+# Let variables required:
+#
+# - event
+# - registry
+#
+RSpec.shared_examples 'event does not transition a registry to pending' do
+  it 'does not transition a registry to pending' do
+    event.process
+
+    expect(registry.reload.pending?).to be_falsey
+  end
+end
+
+# Let variables required:
+#
+# - event
+# - sync_worker_expected_arg
+# - sync_worker_class
+#
+RSpec.shared_examples 'event schedules a sync worker' do
   it 'schedules a sync worker' do
-    expect(sync_worker).to receive(:perform_async).with(expected_id)
+    expect(sync_worker_class).to receive(:perform_async).with(sync_worker_expected_arg)
 
-    subject.process
+    event.process
   end
 end
 
-RSpec.shared_examples 'event does not schedule a sync worker' do |sync_worker|
+# Let variables required:
+#
+# - event
+# - sync_worker_class
+#
+RSpec.shared_examples 'event does not schedule a sync worker' do
   it 'does not schedule a sync worker' do
-    expect(sync_worker).not_to receive(:perform_async)
+    expect(sync_worker_class).not_to receive(:perform_async)
+
+    event.process
+  end
+end
+
+# Let variables required:
+#
+# - event
+# - registry_factory
+# - registry_factory_args
+# - sync_worker_class
+# - sync_worker_expected_arg
+#
+RSpec.shared_examples 'event should trigger a sync' do
+  context 'when a registry does not yet exist' do
+    it_behaves_like 'event creates a registry'
+    it_behaves_like 'event schedules a sync worker'
+    it_behaves_like 'logs event source info'
+  end
+
+  context 'when a registry exists' do
+    let!(:registry) { create(registry_factory, *registry_factory_args) }
 
-    subject.process
+    it_behaves_like 'event transitions a registry to pending'
+    it_behaves_like 'event schedules a sync worker'
+    it_behaves_like 'logs event source info'
   end
 end