Merge branch '357498-update-enqueue-waiting-time-checks' into 'master'

Update EnqueuerWorker to handle nil time checks

See merge request gitlab-org/gitlab!84315

app/models/container_repository.rb

@@ -58,8 +58,8 @@ class ContainerRepository < ApplicationRecord
   scope :import_in_process, -> { where(migration_state: %w[pre_importing pre_import_done importing]) }
 
   scope :recently_done_migration_step, -> do
-    where(migration_state: %w[import_done pre_import_done import_aborted])
-      .order(Arel.sql('GREATEST(migration_pre_import_done_at, migration_import_done_at, migration_aborted_at) DESC'))
+    where(migration_state: %w[import_done pre_import_done import_aborted import_skipped])
+      .order(Arel.sql('GREATEST(migration_pre_import_done_at, migration_import_done_at, migration_aborted_at, migration_skipped_at) DESC'))
   end
 
   scope :ready_for_import, -> do
@@ -160,7 +160,7 @@ class ContainerRepository < ApplicationRecord
       end
     end
 
-    before_transition %i[pre_importing import_aborted] => :pre_import_done do |container_repository|
+    before_transition any => :pre_import_done do |container_repository|
       container_repository.migration_pre_import_done_at = Time.zone.now
     end
 
@@ -359,7 +359,7 @@ class ContainerRepository < ApplicationRecord
   end
 
   def last_import_step_done_at
-    [migration_pre_import_done_at, migration_import_done_at, migration_aborted_at].compact.max
+    [migration_pre_import_done_at, migration_import_done_at, migration_aborted_at, migration_skipped_at].compact.max
   end
 
   def external_import_status

app/workers/container_registry/migration/enqueuer_worker.rb

@@ -82,7 +82,7 @@ module ContainerRegistry
       def waiting_time_passed?
         delay = migration.enqueue_waiting_time
         return true if delay == 0
-        return true unless last_step_completed_repository
+        return true unless last_step_completed_repository&.last_import_step_done_at
 
         last_step_completed_repository.last_import_step_done_at < Time.zone.now - delay
       end

db/migrate/20220408135815_update_index_on_greated_done_at_on_container_repositories.rb

+# frozen_string_literal: true
+
+class UpdateIndexOnGreatedDoneAtOnContainerRepositories < Gitlab::Database::Migration[1.0]
+  OLD_INDEX_NAME = 'index_container_repositories_on_greatest_done_at'
+  NEW_INDEX_NAME = 'index_container_repositories_on_greatest_completed_at'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :container_repositories,
+      'GREATEST(migration_pre_import_done_at, migration_import_done_at, migration_aborted_at, migration_skipped_at)',
+      where: "migration_state IN ('import_done', 'pre_import_done', 'import_aborted', 'import_skipped')",
+      name: NEW_INDEX_NAME
+    remove_concurrent_index_by_name :container_repositories, OLD_INDEX_NAME
+  end
+
+  def down
+    add_concurrent_index :container_repositories,
+      'GREATEST(migration_pre_import_done_at, migration_import_done_at, migration_aborted_at)',
+      where: "migration_state IN ('import_done', 'pre_import_done', 'import_aborted')",
+      name: OLD_INDEX_NAME
+    remove_concurrent_index_by_name :container_repositories, NEW_INDEX_NAME
+  end
+end

db/schema_migrations/20220408135815

+01d8ab924e8c76b54d316ba94089eabea28999e4ce747e6c51803e1ea97b37df
\ No newline at end of file

db/structure.sql

@@ -27308,7 +27308,7 @@ CREATE INDEX index_composer_cache_files_where_namespace_id_is_null ON packages_c
 
 CREATE INDEX index_container_expiration_policies_on_next_run_at_and_enabled ON container_expiration_policies USING btree (next_run_at, enabled);
 
-CREATE INDEX index_container_repositories_on_greatest_done_at ON container_repositories USING btree (GREATEST(migration_pre_import_done_at, migration_import_done_at, migration_aborted_at)) WHERE (migration_state = ANY (ARRAY['import_done'::text, 'pre_import_done'::text, 'import_aborted'::text]));
+CREATE INDEX index_container_repositories_on_greatest_completed_at ON container_repositories USING btree (GREATEST(migration_pre_import_done_at, migration_import_done_at, migration_aborted_at, migration_skipped_at)) WHERE (migration_state = ANY (ARRAY['import_done'::text, 'pre_import_done'::text, 'import_aborted'::text, 'import_skipped'::text]));
 
 CREATE INDEX index_container_repositories_on_migration_state_import_done_at ON container_repositories USING btree (migration_state, migration_import_done_at);
 

spec/models/container_repository_spec.rb

@@ -1271,11 +1271,12 @@ RSpec.describe ContainerRepository, :aggregate_failures do
       let_it_be(:import_done_repository) { create(:container_repository, :import_done, migration_pre_import_done_at: 3.days.ago, migration_import_done_at: 2.days.ago) }
       let_it_be(:import_aborted_repository) { create(:container_repository, :import_aborted, migration_pre_import_done_at: 5.days.ago, migration_aborted_at: 1.day.ago) }
       let_it_be(:pre_import_done_repository) { create(:container_repository, :pre_import_done, migration_pre_import_done_at: 1.hour.ago) }
+      let_it_be(:import_skipped_repository) { create(:container_repository, :import_skipped, migration_skipped_at: 90.minutes.ago) }
 
       subject { described_class.recently_done_migration_step }
 
       it 'returns completed imports by done_at date' do
-        expect(subject.to_a).to eq([pre_import_done_repository, import_aborted_repository, import_done_repository])
+        expect(subject.to_a).to eq([pre_import_done_repository, import_skipped_repository, import_aborted_repository, import_done_repository])
       end
     end
 
@@ -1296,13 +1297,15 @@ RSpec.describe ContainerRepository, :aggregate_failures do
     describe '#last_import_step_done_at' do
       let_it_be(:aborted_at) { Time.zone.now - 1.hour }
       let_it_be(:pre_import_done_at) { Time.zone.now - 2.hours }
+      let_it_be(:skipped_at) { Time.zone.now - 3.hours }
 
       subject { repository.last_import_step_done_at }
 
       before do
         repository.update_columns(
           migration_pre_import_done_at: pre_import_done_at,
-          migration_aborted_at: aborted_at
+          migration_aborted_at: aborted_at,
+          migration_skipped_at: skipped_at
         )
       end
 

spec/workers/container_registry/migration/enqueuer_worker_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe ContainerRegistry::Migration::EnqueuerWorker, :aggregate_failures, :clean_gitlab_redis_shared_state do
+  using RSpec::Parameterized::TableSyntax
   include ExclusiveLeaseHelpers
 
   let_it_be_with_reload(:container_repository) { create(:container_repository, created_at: 2.days.ago) }
@@ -131,14 +132,34 @@ RSpec.describe ContainerRegistry::Migration::EnqueuerWorker, :aggregate_failures
     end
 
     context 'too soon before previous completed import step' do
-      before do
-        create(:container_repository, :import_done, migration_import_done_at: 1.minute.ago)
-        allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(1.hour)
+      where(:state, :timestamp) do
+        :import_done     | :migration_import_done_at
+        :pre_import_done | :migration_pre_import_done_at
+        :import_aborted  | :migration_aborted_at
+        :import_skipped  | :migration_skipped_at
       end
 
-      it_behaves_like 'no action' do
+      with_them do
         before do
-          expect_log_extra_metadata(waiting_time_passed: false, current_waiting_time_setting: 1.hour)
+          allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(1.hour)
+          create(:container_repository, state, timestamp => 1.minute.ago)
+        end
+
+        it_behaves_like 'no action' do
+          before do
+            expect_log_extra_metadata(waiting_time_passed: false, current_waiting_time_setting: 1.hour)
+          end
+        end
+      end
+
+      context 'when last completed repository has nil timestamps' do
+        before do
+          allow(ContainerRegistry::Migration).to receive(:enqueue_waiting_time).and_return(1.hour)
+          create(:container_repository, migration_state: 'import_done')
+        end
+
+        it 'continues to try the next import' do
+          expect { subject }.to change { container_repository.reload.migration_state }
         end
       end
     end