From 05bb08cfb1c4d8a1c4a15cd38bbd83a46fc2fbf1 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Wed, 6 Nov 2024 16:13:39 +0000 Subject: [PATCH 1/3] Load placeholder references after BBS import For user contribution mapping, we should make sure all placeholder references are loaded before finalising the import. References will be generated in a follow-up MR, this is just a logical slice for reviewability's sake. Related issue: https://gitlab.com/gitlab-org/gitlab/-/issues/466356 Changelog: other --- .../stage/finish_import_worker.rb | 14 +++++++++- .../stage/finish_import_worker_spec.rb | 26 ++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb index e3c8508815f852..3335e4a3ee0cfb 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb @@ -8,8 +8,20 @@ class FinishImportWorker # rubocop:disable Scalability/IdempotentWorker private - # project - An instance of Project. + # @param project [Project] def import(project) + placeholder_reference_store = project.placeholder_reference_store + + if placeholder_reference_store&.any? + ::Import::LoadPlaceholderReferencesWorker.perform_async( + project.import_type, + project.import_state.id, + { current_user_id: project.creator.id } + ) + + return self.class.perform_in(30.seconds, project.id) + end + project.after_import Gitlab::Import::Metrics.new(:bitbucket_server_importer, project).track_finished_import diff --git a/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb b/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb index cb61ee13dd1d23..0ec4182831854e 100644 --- a/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb +++ b/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Gitlab::BitbucketServerImport::Stage::FinishImportWorker, feature_category: :importers do - let_it_be(:project) { create(:project, :import_started) } + let_it_be(:project) { create(:project, :import_started, import_type: :bitbucket_server) } subject(:worker) { described_class.new } @@ -19,5 +19,29 @@ expect(project.import_state.reload).to be_finished end + + context 'when there are unloaded placeholder references' do + before do + allow_next_instance_of(::Import::PlaceholderReferences::Store) do |store| + allow(store).to receive(:any?).and_return(true) + end + end + + it 'queues LoadPlaceholderReferencesWorker' do + expect(Import::LoadPlaceholderReferencesWorker).to receive(:perform_async).with( + project.import_type, + project.import_state.id, + { current_user_id: project.creator.id } + ) + + worker.perform(project.id) + end + + it 'schedules itself to run again after 30 seconds' do + expect(described_class).to receive(:perform_in).with(30.seconds, project.id) + + worker.perform(project.id) + end + end end end -- GitLab From 87a02e691973db75007167dcf365853a3bd5687b Mon Sep 17 00:00:00 2001 From: James Nutt Date: Wed, 6 Nov 2024 22:15:28 +0000 Subject: [PATCH 2/3] Apply reviewer feedback --- .../stage/finish_import_worker_spec.rb | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb b/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb index 0ec4182831854e..6903a9bb06c446 100644 --- a/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb +++ b/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb @@ -20,6 +20,26 @@ expect(project.import_state.reload).to be_finished end + context 'when there are no unloaded placeholder references' do + before do + allow_next_instance_of(::Import::PlaceholderReferences::Store) do |store| + allow(store).to receive(:any?).and_return(false) + end + end + + it 'does not queue LoadPlaceholderReferencesWorker' do + expect(Import::LoadPlaceholderReferencesWorker).not_to receive(:perform_async) + + worker.perform(project.id) + end + + it 'does not re-enqueue itself' do + expect(described_class).not_to receive(:perform_in) + + worker.perform(project.id) + end + end + context 'when there are unloaded placeholder references' do before do allow_next_instance_of(::Import::PlaceholderReferences::Store) do |store| -- GitLab From 24f953d77f1345a88ac52ab325d8026efaee4e80 Mon Sep 17 00:00:00 2001 From: James Nutt Date: Thu, 7 Nov 2024 10:29:51 +0000 Subject: [PATCH 3/3] Give up after 5 minutes --- .../stage/finish_import_worker.rb | 24 +++++++++++++++---- .../load_placeholder_references_worker.rb | 23 ++++++------------ lib/import/placeholder_references/store.rb | 4 ++++ .../placeholder_references/store_spec.rb | 8 +++++++ .../stage/finish_import_worker_spec.rb | 19 ++++++++++++++- ...load_placeholder_references_worker_spec.rb | 8 ++++--- 6 files changed, 61 insertions(+), 25 deletions(-) diff --git a/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb index 3335e4a3ee0cfb..e769d05653b833 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/finish_import_worker.rb @@ -6,6 +6,8 @@ module Stage class FinishImportWorker # rubocop:disable Scalability/IdempotentWorker include StageMethods + PLACEHOLDER_WAIT_INTERVAL = 30.seconds + private # @param project [Project] @@ -13,19 +15,31 @@ def import(project) placeholder_reference_store = project.placeholder_reference_store if placeholder_reference_store&.any? - ::Import::LoadPlaceholderReferencesWorker.perform_async( - project.import_type, - project.import_state.id, - { current_user_id: project.creator.id } + info( + project.id, + message: 'Delaying finalization as placeholder references are pending', + placeholder_store_count: placeholder_reference_store.count ) - return self.class.perform_in(30.seconds, project.id) + reschedule(project) + + return end project.after_import Gitlab::Import::Metrics.new(:bitbucket_server_importer, project).track_finished_import end + + def reschedule(project) + ::Import::LoadPlaceholderReferencesWorker.perform_async( + project.import_type, + project.import_state.id, + { current_user_id: project.creator_id } + ) + + self.class.perform_in(PLACEHOLDER_WAIT_INTERVAL.seconds, project.id) + end end end end diff --git a/app/workers/import/load_placeholder_references_worker.rb b/app/workers/import/load_placeholder_references_worker.rb index e846bbb4bebf6c..cb15e72a8212d0 100644 --- a/app/workers/import/load_placeholder_references_worker.rb +++ b/app/workers/import/load_placeholder_references_worker.rb @@ -25,8 +25,11 @@ def perform(import_source, import_uid, params = {}) end def perform_failure(exception, import_source, import_uid) - fail_import(import_source, import_uid) + log_failure(exception, import_source, import_uid) + clear_placeholder_reference_store(import_source, import_uid) + end + def log_failure(exception, import_source, import_uid) ::Import::Framework::Logger.error( message: 'Failed to load all references to placeholder user contributions', error: exception.message, @@ -35,21 +38,9 @@ def perform_failure(exception, import_source, import_uid) ) end - private - - def fail_import(import_source, import_uid) - import_state = nil - - case import_source - when 'gitlab' - import_state = BulkImport.find_by_id(import_uid) - when 'github', 'bitbucket', 'bitbucket_server', 'fogbugz', 'gitea', 'gitlab_project' - import_state = ProjectImportState.find_by_id(import_uid) - when 'import_group_from_file' - import_state = GroupImportState.find_by_group_id(import_uid) - end - - import_state&.fail_op + def clear_placeholder_reference_store(import_source, import_uid) + store = PlaceholderReferences::Store.new(import_source: import_source, import_uid: import_uid) + store.clear! end end end diff --git a/lib/import/placeholder_references/store.rb b/lib/import/placeholder_references/store.rb index b3d07c736b44e8..c7822f30ddacda 100644 --- a/lib/import/placeholder_references/store.rb +++ b/lib/import/placeholder_references/store.rb @@ -34,6 +34,10 @@ def any? !empty? end + def clear! + cache.del(cache_key) + end + private attr_reader :import_source, :import_uid diff --git a/spec/lib/import/placeholder_references/store_spec.rb b/spec/lib/import/placeholder_references/store_spec.rb index 5fed3c2dbcae6e..a7f7afc81d173a 100644 --- a/spec/lib/import/placeholder_references/store_spec.rb +++ b/spec/lib/import/placeholder_references/store_spec.rb @@ -72,6 +72,14 @@ end end + describe '#clear!' do + it 'removes the values from the cache' do + store.add('foo') + + expect { store.clear! }.to change { store.count }.from(1).to(0) + end + end + def cache Gitlab::Cache::Import::Caching end diff --git a/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb b/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb index 6903a9bb06c446..2001b39ad635d1 100644 --- a/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb +++ b/spec/workers/gitlab/bitbucket_server_import/stage/finish_import_worker_spec.rb @@ -43,7 +43,7 @@ context 'when there are unloaded placeholder references' do before do allow_next_instance_of(::Import::PlaceholderReferences::Store) do |store| - allow(store).to receive(:any?).and_return(true) + allow(store).to receive_messages(any?: true, count: 5) end end @@ -62,6 +62,23 @@ worker.perform(project.id) end + + it 'writes a log entry' do + allow(Gitlab::BitbucketServerImport::Logger).to receive(:info) + + expect(Gitlab::BitbucketServerImport::Logger) + .to receive(:info) + .with( + a_hash_including( + message: 'Delaying finalization as placeholder references are pending', + import_stage: 'Gitlab::BitbucketServerImport::Stage::FinishImportWorker', + placeholder_store_count: 5, + project_id: project.id + ) + ).once + + worker.perform(project.id) + end end end end diff --git a/spec/workers/import/load_placeholder_references_worker_spec.rb b/spec/workers/import/load_placeholder_references_worker_spec.rb index b6c07940a61ddf..80e35d210acedf 100644 --- a/spec/workers/import/load_placeholder_references_worker_spec.rb +++ b/spec/workers/import/load_placeholder_references_worker_spec.rb @@ -54,7 +54,7 @@ let_it_be(:project) { create(:project) } shared_examples 'failed user contribution mapping' do - it 'logs the failure and fails the import status record', :aggregate_failures do + it 'logs the failure and clears the placeholder cache', :aggregate_failures do exception = StandardError.new('Some error') expect(::Import::Framework::Logger).to receive(:error).with({ @@ -64,9 +64,11 @@ import_uid: uid }) - described_class.sidekiq_retries_exhausted_block.call({ 'args' => [import_source, uid] }, exception) + expect_next_instance_of(Import::PlaceholderReferences::Store) do |store| + expect(store).to receive(:clear!) + end - expect(import_status_record.reload).to be_failed + described_class.sidekiq_retries_exhausted_block.call({ 'args' => [import_source, uid] }, exception) end # This case should not happen, but in case it does, there should still be a relevant error log anyway -- GitLab