Merge branch '356401-skip-imports-on-max-retries' into 'master'

Skip registry import after too many retries

See merge request gitlab-org/gitlab!83340

app/models/container_repository.rb

@@ -14,6 +14,7 @@ class ContainerRepository < ApplicationRecord
   ACTIVE_MIGRATION_STATES = %w[pre_importing importing].freeze
   MIGRATION_STATES = (IDLE_MIGRATION_STATES + ACTIVE_MIGRATION_STATES).freeze
   ABORTABLE_MIGRATION_STATES = (ACTIVE_MIGRATION_STATES + %w[pre_import_done default]).freeze
+  SKIPPABLE_MIGRATION_STATES = (ABORTABLE_MIGRATION_STATES + %w[import_aborted]).freeze
 
   IRRECONCILABLE_MIGRATIONS_STATUSES = %w[import_in_progress pre_import_in_progress pre_import_canceled import_canceled].freeze
 
@@ -137,7 +138,7 @@ class ContainerRepository < ApplicationRecord
     end
 
     event :skip_import do
-      transition ABORTABLE_MIGRATION_STATES.map(&:to_sym) => :import_skipped
+      transition SKIPPABLE_MIGRATION_STATES.map(&:to_sym) => :import_skipped
     end
 
     event :retry_pre_import do
@@ -184,6 +185,12 @@ class ContainerRepository < ApplicationRecord
       container_repository.migration_retries_count += 1
     end
 
+    after_transition any => :import_aborted do |container_repository|
+      if container_repository.retried_too_many_times?
+        container_repository.skip_import(reason: :too_many_retries)
+      end
+    end
+
     before_transition import_aborted: any do |container_repository|
       container_repository.migration_aborted_at = nil
       container_repository.migration_aborted_in_state = nil
@@ -325,6 +332,10 @@ class ContainerRepository < ApplicationRecord
     end
   end
 
+  def retried_too_many_times?
+    migration_retries_count >= ContainerRegistry::Migration.max_retries
+  end
+
   def last_import_step_done_at
     [migration_pre_import_done_at, migration_import_done_at, migration_aborted_at].compact.max
   end

spec/models/container_repository_spec.rb

@@ -302,6 +302,19 @@ RSpec.describe ContainerRepository, :aggregate_failures do
         expect(repository.migration_aborted_in_state).to eq('importing')
         expect(repository).to be_import_aborted
       end
+
+      context 'above the max retry limit' do
+        before do
+          stub_application_setting(container_registry_import_max_retries: 1)
+        end
+
+        it 'skips the migration' do
+          expect { subject }.to change { repository.migration_skipped_at }
+
+          expect(repository.reload).to be_import_skipped
+          expect(repository.migration_skipped_reason).to eq('too_many_retries')
+        end
+      end
     end
 
     describe '#skip_import' do
@@ -309,7 +322,7 @@ RSpec.describe ContainerRepository, :aggregate_failures do
 
       subject { repository.skip_import(reason: :too_many_retries) }
 
-      it_behaves_like 'transitioning from allowed states', ContainerRepository::ABORTABLE_MIGRATION_STATES
+      it_behaves_like 'transitioning from allowed states', ContainerRepository::SKIPPABLE_MIGRATION_STATES
 
       it 'sets migration_skipped_at and migration_skipped_reason' do
         expect { subject }.to change { repository.reload.migration_skipped_at }
@@ -1136,6 +1149,30 @@ RSpec.describe ContainerRepository, :aggregate_failures do
     end
   end
 
+  describe '#retried_too_many_times?' do
+    subject { repository.retried_too_many_times? }
+
+    before do
+      stub_application_setting(container_registry_import_max_retries: 3)
+    end
+
+    context 'migration_retries_count is equal or greater than max_retries' do
+      before do
+        repository.update_column(:migration_retries_count, 3)
+      end
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'migration_retries_count is lower than max_retries' do
+      before do
+        repository.update_column(:migration_retries_count, 2)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
   context 'with repositories' do
     let_it_be_with_reload(:repository) { create(:container_repository, :cleanup_unscheduled) }
     let_it_be(:other_repository) { create(:container_repository, :cleanup_unscheduled) }