Merge branch...

Merge branch '227681-remove-feature-flag-to-make-registry-table-ssot-for-container-registry' into 'master'

Geo - Remove feature flag to make registry table SSOT for Container Registry

Closes #227681

See merge request gitlab-org/gitlab!38356

ee/app/finders/geo/container_repository_registry_finder.rb

@@ -7,11 +7,11 @@ module Geo
     end
 
     def count_synced
-      registries.merge(Geo::ContainerRepositoryRegistry.synced).count
+      Geo::ContainerRepositoryRegistry.synced.count
     end
 
     def count_failed
-      registries.merge(Geo::ContainerRepositoryRegistry.failed).count
+      Geo::ContainerRepositoryRegistry.failed.count
     end
 
     def count_registry
@@ -63,43 +63,5 @@ module Geo
         .limit(batch_size)
     end
     # rubocop:enable CodeReuse/ActiveRecord
-
-    # Find limited amount of non replicated container repositories.
-    #
-    # You can pass a list with `except_ids:` so you can exclude items you
-    # already scheduled but haven't finished and aren't persisted to the database yet
-    #
-    # @param [Integer] batch_size used to limit the results returned
-    # @param [Array<Integer>] except_ids ids that will be ignored from the query
-    # rubocop:disable CodeReuse/ActiveRecord
-    def find_unsynced(batch_size:, except_ids: [])
-      current_node_fdw
-        .container_repositories
-        .missing_container_repository_registry
-        .id_not_in(except_ids)
-        .limit(batch_size)
-    end
-    # rubocop:enable CodeReuse/ActiveRecord
-
-    # rubocop:disable CodeReuse/ActiveRecord
-    def find_retryable_failed_ids(batch_size:, except_ids: [])
-      Geo::ContainerRepositoryRegistry
-        .failed
-        .retry_due
-        .model_id_not_in(except_ids)
-        .limit(batch_size)
-        .pluck_container_repository_key
-    end
-    # rubocop:enable CodeReuse/ActiveRecord
-
-    private
-
-    def registries
-      if Geo::ContainerRepositoryRegistry.registry_consistency_worker_enabled?
-        Geo::ContainerRepositoryRegistry.all
-      else
-        current_node_fdw.container_repositories.inner_join_container_repository_registry
-      end
-    end
   end
 end

ee/app/models/geo/container_repository_registry.rb

@@ -55,10 +55,6 @@ class Geo::ContainerRepositoryRegistry < Geo::BaseRegistry
     where(nil).pluck(:container_repository_id)
   end
 
-  def self.registry_consistency_worker_enabled?
-    Feature.enabled?(:geo_container_registry_ssot_sync, default_enabled: true)
-  end
-
   def self.replication_enabled?
     Gitlab.config.geo.registry_replication.enabled
   end

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

-# frozen_string_literal: true
-
-module Geo
-  module Fdw
-    class ContainerRepository < ::Geo::BaseFdw
-      self.table_name = Gitlab::Geo::Fdw.foreign_table_name('container_repositories')
-      self.primary_key = :id
-
-      belongs_to :project, class_name: 'Geo::Fdw::Project', inverse_of: :container_repositories
-
-      scope :project_id_in, ->(ids) { joins(:project).merge(Geo::Fdw::Project.id_in(ids)) }
-
-      class << self
-        def inner_join_container_repository_registry
-          join_statement =
-            arel_table
-              .join(container_repository_registry_table, Arel::Nodes::InnerJoin)
-              .on(arel_table[:id].eq(container_repository_registry_table[:container_repository_id]))
-
-          joins(join_statement.join_sources)
-        end
-
-        def missing_container_repository_registry
-          left_outer_join_container_repository_registry
-            .where(container_repository_registry_table[:id].eq(nil))
-        end
-
-        private
-
-        def container_repository_registry_table
-          Geo::ContainerRepositoryRegistry.arel_table
-        end
-
-        def left_outer_join_container_repository_registry
-          join_statement =
-            arel_table
-              .join(container_repository_registry_table, Arel::Nodes::OuterJoin)
-              .on(arel_table[:id].eq(container_repository_registry_table[:container_repository_id]))
-
-          joins(join_statement.join_sources)
-        end
-      end
-    end
-  end
-end

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

@@ -26,12 +26,6 @@ module Geo
         end
       end
 
-      def container_repositories
-        return Geo::Fdw::ContainerRepository.all unless selective_sync?
-
-        Geo::Fdw::ContainerRepository.project_id_in(projects)
-      end
-
       private
 
       def projects_for_selected_namespaces

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

@@ -8,8 +8,6 @@ module Geo
       self.primary_key = :id
       self.table_name = Gitlab::Geo::Fdw.foreign_table_name('projects')
 
-      has_many :container_repositories, class_name: 'Geo::Fdw::ContainerRepository'
-
       belongs_to :namespace, class_name: 'Geo::Fdw::Namespace'
 
       alias_method :parent, :namespace

ee/app/workers/geo/container_repository_sync_dispatch_worker.rb

@@ -47,26 +47,15 @@ module Geo
     end
 
     def find_container_repository_ids_not_synced(batch_size:)
-      if Geo::ContainerRepositoryRegistry.registry_consistency_worker_enabled?
       registry_finder
         .find_never_synced_registries(batch_size: batch_size, except_ids: scheduled_repository_ids)
         .pluck_model_foreign_key
-      else
-        registry_finder
-          .find_unsynced(batch_size: batch_size, except_ids: scheduled_repository_ids)
-          .pluck_primary_key
-      end
     end
 
     def find_retryable_container_registry_ids(batch_size:)
-      if Geo::ContainerRepositoryRegistry.registry_consistency_worker_enabled?
       registry_finder
         .find_retryable_dirty_registries(batch_size: batch_size, except_ids: scheduled_repository_ids)
         .pluck_model_foreign_key
-      else
-        registry_finder
-            .find_retryable_failed_ids(batch_size: batch_size, except_ids: scheduled_repository_ids)
-      end
     end
 
     def registry_finder

ee/spec/finders/geo/container_repository_registry_finder_spec.rb

@@ -4,110 +4,6 @@ require 'spec_helper'
 RSpec.describe Geo::ContainerRepositoryRegistryFinder, :geo do
   include ::EE::GeoHelpers
 
-  before do
-    stub_registry_replication_config(enabled: true)
-  end
-
-  context 'when geo_container_registry_ssot_sync is disabled', :geo_fdw do
-    let!(:secondary) { create(:geo_node) }
-    let!(:container_repository) { create(:container_repository) }
-    let!(:failed_registry) { create(:container_repository_registry, :sync_failed) }
-    let!(:synced_registry) { create(:container_repository_registry, :synced) }
-    let(:synced_group) { create(:group) }
-    let(:unsynced_group) { create(:group) }
-    let(:synced_project) { create(:project, group: synced_group) }
-    let(:unsynced_project) { create(:project, :broken_storage, group: unsynced_group) }
-
-    subject { described_class.new(current_node_id: secondary.id) }
-
-    before do
-      stub_current_geo_node(secondary)
-      stub_feature_flags(geo_container_registry_ssot_sync: false)
-    end
-
-    describe '#count_syncable' do
-      it 'returns number of container repositories' do
-        expect(subject.count_syncable).to eq(3)
-      end
-    end
-
-    describe '#count_synced' do
-      it 'returns only synced registry' do
-        expect(subject.count_synced).to eq(1)
-      end
-    end
-
-    describe '#count_failed' do
-      it 'returns only failed registry' do
-        expect(subject.count_failed).to eq(1)
-      end
-    end
-
-    describe '#count_registry' do
-      it 'returns number of all registries' do
-        expect(subject.count_registry).to eq(2)
-      end
-    end
-
-    describe '#find_unsynced' do
-      it 'returns repositories without an entry in the tracking database' do
-        repositories = subject.find_unsynced(batch_size: 10)
-
-        expect(repositories).to match_ids(container_repository)
-      end
-
-      it 'returns repositories without an entry in the tracking database, excluding exception list' do
-        except_repository = create(:container_repository)
-        repositories = subject.find_unsynced(batch_size: 10, except_ids: [except_repository.id])
-
-        expect(repositories).to match_ids(container_repository)
-      end
-
-      context 'with selective sync by namespace' do
-        let(:secondary) { create(:geo_node, selective_sync_type: 'namespaces', namespaces: [synced_group]) }
-
-        it 'returns repositories without an entry in the tracking database, excluding exception list' do
-          except_repository = create(:container_repository, project: synced_project)
-          repository = create(:container_repository, project: synced_project, name: 'second')
-
-          repositories = subject.find_unsynced(batch_size: 10, except_ids: [except_repository.id])
-
-          expect(repositories).to match_ids(repository)
-        end
-      end
-
-      context 'with selective sync by shard' do
-        let(:secondary) { create(:geo_node, selective_sync_type: 'shards', selective_sync_shards: ['broken']) }
-
-        it 'returns repositories without an entry in the tracking database' do
-          unsynced_repository = create(:container_repository, project: unsynced_project)
-
-          repositories = subject.find_unsynced(batch_size: 10)
-
-          expect(repositories).to match_ids(unsynced_repository)
-        end
-      end
-    end
-
-    describe '#find_retryable_failed_ids' do
-      it 'returns only registry that have to be retried' do
-        result = subject.find_retryable_failed_ids(batch_size: 10)
-
-        expect(result).to eq([failed_registry.container_repository_id])
-      end
-
-      it 'returns only registry that have to be retried, excluding exception list' do
-        except_repository = create(:container_repository)
-        create(:container_repository_registry, :sync_failed, container_repository: except_repository)
-
-        result = subject.find_retryable_failed_ids(batch_size: 10, except_ids: [except_repository.id])
-
-        expect(result).to eq([failed_registry.container_repository_id])
-      end
-    end
-  end
-
-  context 'when geo_container_registry_ssot_sync is enabled' do
   let_it_be(:secondary) { create(:geo_node) }
   let_it_be(:synced_group) { create(:group) }
   let_it_be(:nested_group) { create(:group, parent: synced_group) }
@@ -125,7 +21,7 @@ RSpec.describe Geo::ContainerRepositoryRegistryFinder, :geo do
 
   before do
     stub_current_geo_node(secondary)
-      stub_feature_flags(geo_container_registry_ssot_sync: true)
+    stub_registry_replication_config(enabled: true)
   end
 
   describe '#count_syncable' do
@@ -296,12 +192,12 @@ RSpec.describe Geo::ContainerRepositoryRegistryFinder, :geo do
   end
 
   describe '#find_never_synced_registries' do
-      let!(:registry_container_registry_1) { create(:container_repository_registry, :synced, container_repository_id: container_repository_1.id) }
-      let!(:registry_container_registry_2) { create(:container_repository_registry, :sync_failed, container_repository_id: container_repository_2.id) }
-      let!(:registry_container_registry_3) { create(:container_repository_registry, container_repository_id: container_repository_3.id, last_synced_at: nil) }
-      let!(:registry_container_registry_4) { create(:container_repository_registry, container_repository_id: container_repository_4.id, last_synced_at: 3.days.ago, retry_at: 2.days.ago) }
-      let!(:registry_container_registry_5) { create(:container_repository_registry, container_repository_id: container_repository_5.id, last_synced_at: 6.days.ago) }
-      let!(:registry_container_registry_6) { create(:container_repository_registry, container_repository_id: container_repository_6.id, last_synced_at: nil) }
+    let_it_be(:registry_container_registry_1) { create(:container_repository_registry, :synced, container_repository_id: container_repository_1.id) }
+    let_it_be(:registry_container_registry_2) { create(:container_repository_registry, :sync_failed, container_repository_id: container_repository_2.id) }
+    let_it_be(:registry_container_registry_3) { create(:container_repository_registry, container_repository_id: container_repository_3.id, last_synced_at: nil) }
+    let_it_be(:registry_container_registry_4) { create(:container_repository_registry, container_repository_id: container_repository_4.id, last_synced_at: 3.days.ago, retry_at: 2.days.ago) }
+    let_it_be(:registry_container_registry_5) { create(:container_repository_registry, container_repository_id: container_repository_5.id, last_synced_at: 6.days.ago) }
+    let_it_be(:registry_container_registry_6) { create(:container_repository_registry, container_repository_id: container_repository_6.id, last_synced_at: nil) }
 
     it 'returns registries for projects that have never been synced' do
       registries = subject.find_never_synced_registries(batch_size: 10)
@@ -317,12 +213,12 @@ RSpec.describe Geo::ContainerRepositoryRegistryFinder, :geo do
   end
 
   describe '#find_retryable_dirty_registries' do
-      let!(:registry_container_registry_1) { create(:container_repository_registry, :synced, container_repository_id: container_repository_1.id) }
-      let!(:registry_container_registry_2) { create(:container_repository_registry, :sync_started, container_repository_id: container_repository_2.id) }
-      let!(:registry_container_registry_3) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_3.id, last_synced_at: nil) }
-      let!(:registry_container_registry_4) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_4.id, last_synced_at: 3.days.ago, retry_at: 2.days.ago) }
-      let!(:registry_container_registry_5) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_5.id, last_synced_at: 6.days.ago) }
-      let!(:registry_container_registry_6) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_6.id, last_synced_at: nil) }
+    let_it_be(:registry_container_registry_1) { create(:container_repository_registry, :synced, container_repository_id: container_repository_1.id) }
+    let_it_be(:registry_container_registry_2) { create(:container_repository_registry, :sync_started, container_repository_id: container_repository_2.id) }
+    let_it_be(:registry_container_registry_3) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_3.id, last_synced_at: nil) }
+    let_it_be(:registry_container_registry_4) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_4.id, last_synced_at: 3.days.ago, retry_at: 2.days.ago) }
+    let_it_be(:registry_container_registry_5) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_5.id, last_synced_at: 6.days.ago) }
+    let_it_be(:registry_container_registry_6) { create(:container_repository_registry, state: :failed, container_repository_id: container_repository_6.id, last_synced_at: nil) }
 
     it 'returns registries for projects that have been recently updated' do
       registries = subject.find_retryable_dirty_registries(batch_size: 10)
@@ -336,5 +232,4 @@ RSpec.describe Geo::ContainerRepositoryRegistryFinder, :geo do
       expect(registries).to match_ids(registry_container_registry_3)
     end
   end
-  end
 end

ee/spec/models/geo/fdw/container_repository_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Geo::Fdw::ContainerRepository, :geo, type: :model do
-  context 'relationships' do
-    it { is_expected.to belong_to(:project).class_name('Geo::Fdw::Project') }
-  end
-end

ee/spec/models/geo/fdw/project_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Geo::Fdw::Project, :geo_fdw, type: :model do
-  context 'relationships' do
-    it { is_expected.to have_many(:container_repositories).class_name('Geo::Fdw::ContainerRepository') }
-  end
-end

ee/spec/workers/geo/container_repository_sync_dispatch_worker_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe Geo::ContainerRepositorySyncDispatchWorker, :geo, :use_sql_query_
   include ::EE::GeoHelpers
   include ExclusiveLeaseHelpers
 
-  let(:primary) { create(:geo_node, :primary) }
-  let(:secondary) { create(:geo_node) }
+  let_it_be(:primary) { create(:geo_node, :primary) }
+  let_it_be(:secondary) { create(:geo_node) }
 
   before do
     stub_current_geo_node(secondary)
@@ -49,94 +49,6 @@ RSpec.describe Geo::ContainerRepositorySyncDispatchWorker, :geo, :use_sql_query_
     expect(Geo::ContainerRepositorySyncWorker).not_to receive(:perform_async)
   end
 
-  context 'when geo_container_registry_ssot_sync is disabled', :geo_fdw do
-    before do
-      stub_feature_flags(geo_container_registry_ssot_sync: false)
-    end
-
-    it 'performs Geo::ContainerRepositorySyncWorker' do
-      container_repository = create(:container_repository)
-
-      expect(Geo::ContainerRepositorySyncWorker).to receive(:perform_async).with(container_repository.id)
-
-      subject.perform
-    end
-
-    it 'performs Geo::ContainerRepositorySyncWorker for failed syncs' do
-      registry = create(:container_repository_registry, :sync_failed)
-
-      expect(Geo::ContainerRepositorySyncWorker).to receive(:perform_async)
-        .with(registry.container_repository_id).once.and_return(spy)
-
-      subject.perform
-    end
-
-    it 'does not perform Geo::ContainerRepositorySyncWorker for synced repositories' do
-      create(:container_repository_registry, :synced)
-
-      expect(Geo::ContainerRepositorySyncWorker).not_to receive(:perform_async)
-
-      subject.perform
-    end
-
-    context 'with a failed sync' do
-      it 'does not stall backfill' do
-        failed_registry = create(:container_repository_registry, :sync_failed)
-        unsynced_container_repository = create(:container_repository)
-
-        stub_const('Geo::Scheduler::SchedulerWorker::DB_RETRIEVE_BATCH_SIZE', 1)
-
-        expect(Geo::ContainerRepositorySyncWorker).not_to receive(:perform_async).with(failed_registry.container_repository_id)
-        expect(Geo::ContainerRepositorySyncWorker).to receive(:perform_async).with(unsynced_container_repository.id)
-
-        subject.perform
-      end
-
-      it 'does not retry failed files when retry_at is tomorrow' do
-        failed_registry = create(:container_repository_registry, :sync_failed, retry_at: Date.tomorrow)
-
-        expect(Geo::ContainerRepositorySyncWorker)
-          .not_to receive(:perform_async).with( failed_registry.container_repository_id)
-
-        subject.perform
-      end
-
-      it 'retries failed files when retry_at is in the past' do
-        failed_registry = create(:container_repository_registry, :sync_failed, retry_at: Date.yesterday)
-
-        expect(Geo::ContainerRepositorySyncWorker)
-          .to receive(:perform_async).with(failed_registry.container_repository_id)
-
-        subject.perform
-      end
-    end
-
-    context 'when node has namespace restrictions', :request_store do
-      let(:synced_group) { create(:group) }
-      let(:project_in_synced_group) { create(:project, group: synced_group) }
-      let(:unsynced_project) { create(:project) }
-
-      before do
-        secondary.update!(selective_sync_type: 'namespaces', namespaces: [synced_group])
-      end
-
-      it 'does not perform Geo::ContainerRepositorySyncWorker for repositories that does not belong to selected namespaces' do
-        container_repository = create(:container_repository, project: project_in_synced_group)
-        create(:container_repository, project: unsynced_project)
-
-        expect(Geo::ContainerRepositorySyncWorker).to receive(:perform_async)
-          .with(container_repository.id).once.and_return(spy)
-
-        subject.perform
-      end
-    end
-  end
-
-  context 'when geo_container_registry_ssot_sync is enabled' do
-    before do
-      stub_feature_flags(geo_container_registry_ssot_sync: true)
-    end
-
   it 'performs Geo::ContainerRepositorySyncWorker' do
     registry = create(:container_repository_registry)
 
@@ -193,5 +105,4 @@ RSpec.describe Geo::ContainerRepositorySyncDispatchWorker, :geo, :use_sql_query_
       subject.perform
     end
   end
-  end
 end

ee/spec/workers/geo/secondary/registry_consistency_worker_spec.rb

@@ -108,30 +108,6 @@ RSpec.describe Geo::Secondary::RegistryConsistencyWorker, :geo do
       expect(Geo::ContainerRepositoryRegistry.where(container_repository_id: container_repository.id).count).to eq(1)
     end
 
-    context 'when geo_container_registry_ssot_sync is disabled' do
-      before do
-        stub_feature_flags(geo_container_registry_ssot_sync: false)
-      end
-
-      it 'returns false' do
-        expect(subject.perform).to be_falsey
-      end
-
-      it 'does not execute RegistryConsistencyService for container repositories' do
-        allow(Geo::RegistryConsistencyService).to receive(:new).with(Geo::JobArtifactRegistry, batch_size: batch_size).and_call_original
-        allow(Geo::RegistryConsistencyService).to receive(:new).with(Geo::LfsObjectRegistry, batch_size: batch_size).and_call_original
-        allow(Geo::RegistryConsistencyService).to receive(:new).with(Geo::PackageFileRegistry, batch_size: batch_size).and_call_original
-        allow(Geo::RegistryConsistencyService).to receive(:new).with(Geo::UploadRegistry, batch_size: batch_size).and_call_original
-        allow(Geo::RegistryConsistencyService).to receive(:new).with(Geo::ProjectRegistry, batch_size: batch_size).and_call_original
-        allow(Geo::RegistryConsistencyService).to receive(:new).with(Geo::DesignRegistry, batch_size: batch_size).and_call_original
-        allow(Geo::RegistryConsistencyService).to receive(:new).with(Geo::VulnerabilityExportRegistry, batch_size: batch_size).and_call_original
-
-        expect(Geo::RegistryConsistencyService).not_to receive(:new).with(Geo::ContainerRepositoryRegistry, batch_size: batch_size)
-
-        subject.perform
-      end
-    end
-
     context 'when the current Geo node is disabled or primary' do
       before do
         stub_primary_node