From a789932180cb21157bf76674aed4bb4bd5097adf Mon Sep 17 00:00:00 2001 From: Sam Word Date: Fri, 26 Jan 2024 16:30:37 -0500 Subject: [PATCH 1/9] Added RefreshImportWorker to Bitbucket Cloud/Server stage workers Adding RefreshImportWorker to Bitbucket Cloud and Server importer stage workers so that their job ids are refreshed when a stage begins preventing StuckImportWroker from mistakenly thinking a slow import is stuck. --- .../gitlab/bitbucket_import/stage_methods.rb | 2 ++ .../bitbucket_server_import/stage_methods.rb | 2 ++ .../stage_methods_shared_examples.rb | 11 ++++++++++ .../stage_methods_shared_examples.rb | 22 +++++++++++++++++++ 4 files changed, 37 insertions(+) diff --git a/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb b/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb index 51e2f5cff227df..bc29255974249f 100644 --- a/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb @@ -33,6 +33,8 @@ def perform(project_id) return unless project + GithubImport::RefreshImportJidWorker.perform_in_the_future(project_id, jid) + import(project) info(project_id, message: 'stage finished') diff --git a/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb b/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb index cf5710e6108c3e..1538e9fe793e25 100644 --- a/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb @@ -31,6 +31,8 @@ def perform(project_id) return unless (project = find_project(project_id)) + GithubImport::RefreshImportJidWorker.perform_in_the_future(project_id, jid) + import(project) info(project_id, message: 'stage finished') diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb index f128aa92a53177..1e1ac20f12222b 100644 --- a/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb @@ -25,5 +25,16 @@ worker.perform(project.id) end + + it 'queues RefreshImportJidWorker' do + allow(worker).to receive(:import) + allow(worker).to receive(:jid).and_return('mock_jid') + + expect(Gitlab::GithubImport::RefreshImportJidWorker) + .to receive(:perform_in_the_future) + .with(project.id, 'mock_jid') + + worker.perform(project.id) + end end end diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb index 1246dd2979b9cf..f824e29784c57f 100644 --- a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb @@ -15,4 +15,26 @@ described_class.sidekiq_retries_exhausted_block.call(job, StandardError.new) end end + + describe '.perform' do + let(:worker) { described_class.new } + + it 'executes the import' do + expect(worker).to receive(:import).with(project).once + expect(Gitlab::BitbucketServerImport::Logger).to receive(:info).twice + + worker.perform(project.id) + end + + it 'queues RefreshImportJidWorker' do + allow(worker).to receive(:import) + allow(worker).to receive(:jid).and_return('mock_jid') + + expect(Gitlab::GithubImport::RefreshImportJidWorker) + .to receive(:perform_in_the_future) + .with(project.id, 'mock_jid') + + worker.perform(project.id) + end + end end -- GitLab From 546fd57568fdbbb38fa7cd77dbf91366fcd50f8b Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 29 Jan 2024 18:44:27 -0500 Subject: [PATCH 2/9] Set longer expir time for stage workers, removed redundant refreshes --- app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb | 2 ++ .../concerns/gitlab/bitbucket_server_import/stage_methods.rb | 2 ++ .../gitlab/bitbucket_import/stage/import_issues_notes_worker.rb | 2 -- .../gitlab/bitbucket_import/stage/import_issues_worker.rb | 2 -- .../gitlab/bitbucket_import/stage/import_lfs_objects_worker.rb | 2 -- .../bitbucket_import/stage/import_pull_requests_notes_worker.rb | 2 -- .../bitbucket_import/stage/import_pull_requests_worker.rb | 2 -- .../bitbucket_server_import/stage/import_lfs_objects_worker.rb | 2 -- .../gitlab/bitbucket_server_import/stage/import_notes_worker.rb | 2 -- .../stage/import_pull_requests_worker.rb | 2 -- 10 files changed, 4 insertions(+), 16 deletions(-) diff --git a/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb b/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb index bc29255974249f..590b21561bb003 100644 --- a/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb @@ -16,6 +16,8 @@ module StageMethods sidekiq_options dead: false, retry: 6 + sidekiq_options status_expiration: Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION + sidekiq_retries_exhausted do |msg, e| Gitlab::Import::ImportFailureService.track( project_id: msg['args'][0], diff --git a/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb b/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb index 1538e9fe793e25..b2b8f10fa45594 100644 --- a/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb @@ -16,6 +16,8 @@ module StageMethods sidekiq_options dead: false, retry: 6 + sidekiq_options status_expiration: Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION + sidekiq_retries_exhausted do |msg, e| Gitlab::Import::ImportFailureService.track( project_id: msg['args'][0], diff --git a/app/workers/gitlab/bitbucket_import/stage/import_issues_notes_worker.rb b/app/workers/gitlab/bitbucket_import/stage/import_issues_notes_worker.rb index cbd67099086a81..af7927b80a8194 100644 --- a/app/workers/gitlab/bitbucket_import/stage/import_issues_notes_worker.rb +++ b/app/workers/gitlab/bitbucket_import/stage/import_issues_notes_worker.rb @@ -12,8 +12,6 @@ class ImportIssuesNotesWorker # rubocop:disable Scalability/IdempotentWorker def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, diff --git a/app/workers/gitlab/bitbucket_import/stage/import_issues_worker.rb b/app/workers/gitlab/bitbucket_import/stage/import_issues_worker.rb index 31a11d802c7373..d2dac824e81390 100644 --- a/app/workers/gitlab/bitbucket_import/stage/import_issues_worker.rb +++ b/app/workers/gitlab/bitbucket_import/stage/import_issues_worker.rb @@ -12,8 +12,6 @@ class ImportIssuesWorker # rubocop:disable Scalability/IdempotentWorker def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, diff --git a/app/workers/gitlab/bitbucket_import/stage/import_lfs_objects_worker.rb b/app/workers/gitlab/bitbucket_import/stage/import_lfs_objects_worker.rb index c88a1be34466be..328a66dcdca9e6 100644 --- a/app/workers/gitlab/bitbucket_import/stage/import_lfs_objects_worker.rb +++ b/app/workers/gitlab/bitbucket_import/stage/import_lfs_objects_worker.rb @@ -12,8 +12,6 @@ class ImportLfsObjectsWorker # rubocop:disable Scalability/IdempotentWorker def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, diff --git a/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_notes_worker.rb b/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_notes_worker.rb index 36d60c7246c583..fcabe80abc49d0 100644 --- a/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_notes_worker.rb +++ b/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_notes_worker.rb @@ -12,8 +12,6 @@ class ImportPullRequestsNotesWorker # rubocop:disable Scalability/IdempotentWork def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, diff --git a/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_worker.rb b/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_worker.rb index 3f85c832d50983..373dfcdb1f7878 100644 --- a/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_worker.rb +++ b/app/workers/gitlab/bitbucket_import/stage/import_pull_requests_worker.rb @@ -12,8 +12,6 @@ class ImportPullRequestsWorker # rubocop:disable Scalability/IdempotentWorker def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, diff --git a/app/workers/gitlab/bitbucket_server_import/stage/import_lfs_objects_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/import_lfs_objects_worker.rb index 1002047225c93f..020205dcddbe99 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/import_lfs_objects_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/import_lfs_objects_worker.rb @@ -12,8 +12,6 @@ class ImportLfsObjectsWorker # rubocop:disable Scalability/IdempotentWorker def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, diff --git a/app/workers/gitlab/bitbucket_server_import/stage/import_notes_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/import_notes_worker.rb index b30f93058298a2..8020dc7ab3af9b 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/import_notes_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/import_notes_worker.rb @@ -12,8 +12,6 @@ class ImportNotesWorker # rubocop:disable Scalability/IdempotentWorker def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, diff --git a/app/workers/gitlab/bitbucket_server_import/stage/import_pull_requests_worker.rb b/app/workers/gitlab/bitbucket_server_import/stage/import_pull_requests_worker.rb index 9e3d570e20dfb7..c39665b6829654 100644 --- a/app/workers/gitlab/bitbucket_server_import/stage/import_pull_requests_worker.rb +++ b/app/workers/gitlab/bitbucket_server_import/stage/import_pull_requests_worker.rb @@ -12,8 +12,6 @@ class ImportPullRequestsWorker # rubocop:disable Scalability/IdempotentWorker def import(project) waiter = importer_class.new(project).execute - project.import_state.refresh_jid_expiration - AdvanceStageWorker.perform_async( project.id, { waiter.key => waiter.jobs_remaining }, -- GitLab From a603ff1c00d75d63ad4017da9133461601eedc36 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 5 Feb 2024 12:09:23 -0500 Subject: [PATCH 3/9] Moved RefreshImportJidWorker to generic namespace --- .../worker_data_consistency.yml | 1 + .rubocop_todo/style/arguments_forwarding.yml | 1 - .rubocop_todo/style/guard_clause.yml | 2 +- .../style/inline_disable_annotation.yml | 1 + app/workers/all_queues.yml | 9 ++ .../gitlab/bitbucket_import/stage_methods.rb | 2 +- .../bitbucket_server_import/stage_methods.rb | 2 +- .../gitlab/github_import/stage_methods.rb | 2 +- .../refresh_import_jid_worker.rb | 37 +----- .../import/refresh_import_jid_worker.rb | 52 +++++++++ config/sidekiq_queues.yml | 2 + lib/gitlab/github_import/parallel_importer.rb | 2 +- spec/support/rspec_order_todo.yml | 2 +- .../stage_methods_shared_examples.rb | 2 +- .../stage_methods_shared_examples.rb | 2 +- .../stage_methods_shared_examples.rb | 2 +- spec/workers/every_sidekiq_worker_spec.rb | 2 +- .../refresh_import_jid_worker_spec.rb | 98 +--------------- .../import/refresh_import_jid_worker_spec.rb | 107 ++++++++++++++++++ 19 files changed, 189 insertions(+), 139 deletions(-) create mode 100644 app/workers/gitlab/import/refresh_import_jid_worker.rb create mode 100644 spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb diff --git a/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml b/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml index 865ec2f025d4f5..71e2d7b54d3192 100644 --- a/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml +++ b/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml @@ -128,6 +128,7 @@ SidekiqLoadBalancing/WorkerDataConsistency: - 'app/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker.rb' - 'app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb' - 'app/workers/gitlab/github_import/stage/import_repository_worker.rb' + - 'app/workers/gitlab/import/refresh_import_jid_worker.rb' - 'app/workers/gitlab/jira_import/advance_stage_worker.rb' - 'app/workers/gitlab/jira_import/import_issue_worker.rb' - 'app/workers/gitlab/jira_import/stage/start_import_worker.rb' diff --git a/.rubocop_todo/style/arguments_forwarding.yml b/.rubocop_todo/style/arguments_forwarding.yml index b72751ed27946b..53ae5e4a96c71d 100644 --- a/.rubocop_todo/style/arguments_forwarding.yml +++ b/.rubocop_todo/style/arguments_forwarding.yml @@ -47,7 +47,6 @@ Style/ArgumentsForwarding: - 'app/workers/concerns/limited_capacity/worker.rb' - 'app/workers/concerns/reactive_cacheable_worker.rb' - 'app/workers/concerns/reenqueuer.rb' - - 'app/workers/gitlab/github_import/refresh_import_jid_worker.rb' - 'app/workers/pages_worker.rb' - 'config/initializers/6_labkit_middleware.rb' - 'config/initializers/active_record_table_definition.rb' diff --git a/.rubocop_todo/style/guard_clause.yml b/.rubocop_todo/style/guard_clause.yml index 9896356b1f603d..946be7f16395de 100644 --- a/.rubocop_todo/style/guard_clause.yml +++ b/.rubocop_todo/style/guard_clause.yml @@ -234,7 +234,7 @@ Style/GuardClause: - 'app/workers/container_registry/migration/guard_worker.rb' - 'app/workers/deployments/hooks_worker.rb' - 'app/workers/deployments/link_merge_request_worker.rb' - - 'app/workers/gitlab/github_import/refresh_import_jid_worker.rb' + - 'app/workers/gitlab/import/refresh_import_jid_worker.rb' - 'app/workers/google_cloud/create_cloudsql_instance_worker.rb' - 'app/workers/packages/cleanup/execute_policy_worker.rb' - 'app/workers/packages/maven/metadata/sync_worker.rb' diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml index c1711c1ca4d04b..75c708fcc22afe 100644 --- a/.rubocop_todo/style/inline_disable_annotation.yml +++ b/.rubocop_todo/style/inline_disable_annotation.yml @@ -952,6 +952,7 @@ Style/InlineDisableAnnotation: - 'app/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker.rb' - 'app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb' - 'app/workers/gitlab/github_import/stage/import_repository_worker.rb' + - 'app/workers/gitlab/import/refresh_import_jid_worker.rb' - 'app/workers/gitlab/import/stuck_import_job.rb' - 'app/workers/gitlab/import/stuck_project_import_jobs_worker.rb' - 'app/workers/gitlab/jira_import/advance_stage_worker.rb' diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index a82d46f7b18102..f5edcd30de863d 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3180,6 +3180,15 @@ :weight: 2 :idempotent: true :tags: [] +- :name: import_refresh_import_jid + :worker_name: Gitlab::Import::RefreshImportJidWorker + :feature_category: :importers + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: false + :tags: [] - :name: incident_management_close_incident :worker_name: IncidentManagement::CloseIncidentWorker :feature_category: :incident_management diff --git a/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb b/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb index 590b21561bb003..3af4f24438aff0 100644 --- a/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/bitbucket_import/stage_methods.rb @@ -35,7 +35,7 @@ def perform(project_id) return unless project - GithubImport::RefreshImportJidWorker.perform_in_the_future(project_id, jid) + Import::RefreshImportJidWorker.perform_in_the_future(project_id, jid) import(project) diff --git a/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb b/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb index b2b8f10fa45594..304547ac1cc358 100644 --- a/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/bitbucket_server_import/stage_methods.rb @@ -33,7 +33,7 @@ def perform(project_id) return unless (project = find_project(project_id)) - GithubImport::RefreshImportJidWorker.perform_in_the_future(project_id, jid) + Import::RefreshImportJidWorker.perform_in_the_future(project_id, jid) import(project) diff --git a/app/workers/concerns/gitlab/github_import/stage_methods.rb b/app/workers/concerns/gitlab/github_import/stage_methods.rb index 69cf6f424af02f..f6a46f50ca388c 100644 --- a/app/workers/concerns/gitlab/github_import/stage_methods.rb +++ b/app/workers/concerns/gitlab/github_import/stage_methods.rb @@ -62,7 +62,7 @@ def perform(project_id) return end - RefreshImportJidWorker.perform_in_the_future(project.id, jid) + Import::RefreshImportJidWorker.perform_in_the_future(project.id, jid) client = GithubImport.new_client_for(project) diff --git a/app/workers/gitlab/github_import/refresh_import_jid_worker.rb b/app/workers/gitlab/github_import/refresh_import_jid_worker.rb index dfc581f201b69f..ee7e45889bd841 100644 --- a/app/workers/gitlab/github_import/refresh_import_jid_worker.rb +++ b/app/workers/gitlab/github_import/refresh_import_jid_worker.rb @@ -9,42 +9,11 @@ class RefreshImportJidWorker # rubocop:disable Scalability/IdempotentWorker include GithubImport::Queue - sidekiq_options retry: 5 - - # The interval to schedule new instances of this job at. - INTERVAL = 5.minutes.to_i - def self.perform_in_the_future(*args) - perform_in(INTERVAL, *args) - end - - # project_id - The ID of the project that is being imported. - # check_job_id - The ID of the job for which to check the status. - def perform(project_id, check_job_id) - import_state = find_import_state(project_id) - return unless import_state - - if SidekiqStatus.running?(check_job_id) - # As long as the worker is running we want to keep refreshing - # the worker's JID as well as the import's JID. - Gitlab::SidekiqStatus.expire(check_job_id, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - Gitlab::SidekiqStatus.set(import_state.jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - - self.class.perform_in_the_future(project_id, check_job_id) - end - - # If the job is no longer running there's nothing else we need to do. If - # the clone job completed successfully it will have scheduled the next - # stage, if it died there's nothing we can do anyway. - end - - # rubocop: disable CodeReuse/ActiveRecord - def find_import_state(project_id) - ProjectImportState.select(:jid) - .with_status(:started) - .find_by(project_id: project_id) + # Delegate to new version of this job so stale sidekiq nodes can still + # run instead of no-op + Gitlab::Import::RefreshImportJidWorker.perform_in_the_future(*args) end - # rubocop: enable CodeReuse/ActiveRecord end end end diff --git a/app/workers/gitlab/import/refresh_import_jid_worker.rb b/app/workers/gitlab/import/refresh_import_jid_worker.rb new file mode 100644 index 00000000000000..b5d464547671d5 --- /dev/null +++ b/app/workers/gitlab/import/refresh_import_jid_worker.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module Gitlab + module Import + class RefreshImportJidWorker # rubocop:disable Scalability/IdempotentWorker + include ApplicationWorker + + data_consistency :always + + feature_category :importers + sidekiq_options dead: false + + sidekiq_options retry: 5 + + # The interval to schedule new instances of this job at. + INTERVAL = 5.minutes.to_i + + def self.perform_in_the_future(*args) + perform_in(INTERVAL, *args) + end + + # project_id - The ID of the project that is being imported. + # check_job_id - The ID of the job for which to check the status. + # params - to avoid multiple releases if parameters change + def perform(project_id, check_job_id, params = {}) # rubocop:disable Lint/UnusedMethodArgument -- for future flexibily + import_state = find_import_state(project_id) + return unless import_state + + if SidekiqStatus.running?(check_job_id) + # As long as the worker is running we want to keep refreshing + # the worker's JID as well as the import's JID. + Gitlab::SidekiqStatus.expire(check_job_id, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) + Gitlab::SidekiqStatus.set(import_state.jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) + + self.class.perform_in_the_future(project_id, check_job_id) + end + + # If the job is no longer running there's nothing else we need to do. If + # the clone job completed successfully it will have scheduled the next + # stage, if it died there's nothing we can do anyway. + end + + # rubocop:disable CodeReuse/ActiveRecord + def find_import_state(project_id) + ProjectImportState.select(:jid) + .with_status(:started) + .find_by(project_id: project_id) + end + # rubocop:enable CodeReuse/ActiveRecord + end + end +end diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 1383ed290f2227..98bbc100ba0d32 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -371,6 +371,8 @@ - 1 - - import_issues_csv - 2 +- - import_refresh_import_jid + - 1 - - incident_management - 2 - - incident_management_apply_incident_sla_exceeded_label diff --git a/lib/gitlab/github_import/parallel_importer.rb b/lib/gitlab/github_import/parallel_importer.rb index a71590c02f82e8..e045c424fe1bb9 100644 --- a/lib/gitlab/github_import/parallel_importer.rb +++ b/lib/gitlab/github_import/parallel_importer.rb @@ -35,7 +35,7 @@ def initialize(project) def execute Gitlab::Import::SetAsyncJid.set_jid(project.import_state) - # We need to track this job's status for use by Gitlab::GithubImport::RefreshImportJidWorker. + # We need to track this job's status for use by Gitlab::Import::RefreshImportJidWorker. Stage::ImportRepositoryWorker .with_status .perform_async(project.id) diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index 2d69d6612b45ec..c0592c79e593bb 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -9500,7 +9500,6 @@ - './spec/workers/gitlab/github_import/import_issue_worker_spec.rb' - './spec/workers/gitlab/github_import/import_note_worker_spec.rb' - './spec/workers/gitlab/github_import/import_pull_request_worker_spec.rb' -- './spec/workers/gitlab/github_import/refresh_import_jid_worker_spec.rb' - './spec/workers/gitlab/github_import/stage/finish_import_worker_spec.rb' - './spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb' - './spec/workers/gitlab/github_import/stage/import_issue_events_worker_spec.rb' @@ -9511,6 +9510,7 @@ - './spec/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker_spec.rb' - './spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb' - './spec/workers/gitlab/github_import/stage/import_repository_worker_spec.rb' +- './spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb' - './spec/workers/gitlab/import/stuck_import_job_spec.rb' - './spec/workers/gitlab/import/stuck_project_import_jobs_worker_spec.rb' - './spec/workers/gitlab/jira_import/import_issue_worker_spec.rb' diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb index 1e1ac20f12222b..b2c842c5085073 100644 --- a/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb @@ -30,7 +30,7 @@ allow(worker).to receive(:import) allow(worker).to receive(:jid).and_return('mock_jid') - expect(Gitlab::GithubImport::RefreshImportJidWorker) + expect(Gitlab::Import::RefreshImportJidWorker) .to receive(:perform_in_the_future) .with(project.id, 'mock_jid') diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb index f824e29784c57f..bc781d6b8c3f1d 100644 --- a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb @@ -30,7 +30,7 @@ allow(worker).to receive(:import) allow(worker).to receive(:jid).and_return('mock_jid') - expect(Gitlab::GithubImport::RefreshImportJidWorker) + expect(Gitlab::Import::RefreshImportJidWorker) .to receive(:perform_in_the_future) .with(project.id, 'mock_jid') diff --git a/spec/support/shared_examples/workers/gitlab/github_import/stage_methods_shared_examples.rb b/spec/support/shared_examples/workers/gitlab/github_import/stage_methods_shared_examples.rb index b5e3589d86c50c..6d064e0c999d34 100644 --- a/spec/support/shared_examples/workers/gitlab/github_import/stage_methods_shared_examples.rb +++ b/spec/support/shared_examples/workers/gitlab/github_import/stage_methods_shared_examples.rb @@ -100,7 +100,7 @@ allow(worker).to receive(:import) allow(worker).to receive(:jid).and_return('mock_jid') - expect(Gitlab::GithubImport::RefreshImportJidWorker) + expect(Gitlab::Import::RefreshImportJidWorker) .to receive(:perform_in_the_future) .with(project.id, 'mock_jid') diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb index 4918c4e00824b9..b30ce5a584f2b9 100644 --- a/spec/workers/every_sidekiq_worker_spec.rb +++ b/spec/workers/every_sidekiq_worker_spec.rb @@ -278,7 +278,6 @@ 'Gitlab::GithubImport::PullRequests::ImportReviewWorker' => 5, 'Gitlab::GithubImport::PullRequests::ImportMergedByWorker' => 5, 'Gitlab::GithubImport::ImportPullRequestWorker' => 5, - 'Gitlab::GithubImport::RefreshImportJidWorker' => 5, 'Gitlab::GithubImport::ReplayEventsWorker' => 5, 'Gitlab::GithubImport::Stage::FinishImportWorker' => 6, 'Gitlab::GithubImport::Stage::ImportBaseDataWorker' => 6, @@ -297,6 +296,7 @@ 'Gitlab::GithubGistsImport::ImportGistWorker' => 5, 'Gitlab::GithubGistsImport::StartImportWorker' => 5, 'Gitlab::GithubGistsImport::FinishImportWorker' => 5, + 'Gitlab::Import::RefreshImportJidWorker' => 5, 'Gitlab::JiraImport::AdvanceStageWorker' => 6, 'Gitlab::JiraImport::ImportIssueWorker' => 5, 'Gitlab::JiraImport::Stage::FinishImportWorker' => 6, diff --git a/spec/workers/gitlab/github_import/refresh_import_jid_worker_spec.rb b/spec/workers/gitlab/github_import/refresh_import_jid_worker_spec.rb index 5d0cb05c8d5ed7..70f958b60f6a1f 100644 --- a/spec/workers/gitlab/github_import/refresh_import_jid_worker_spec.rb +++ b/spec/workers/gitlab/github_import/refresh_import_jid_worker_spec.rb @@ -6,102 +6,12 @@ let(:worker) { described_class.new } describe '.perform_in_the_future' do - it 'schedules a job in the future' do - expect(described_class) - .to receive(:perform_in) - .with(5.minutes.to_i, 10, '123') + it 'calls Gitlab::Import::RefreshImportJidWorker#perform_in_the_future' do + expect(Gitlab::Import::RefreshImportJidWorker) + .to receive(:perform_in_the_future) + .with(10, '123') described_class.perform_in_the_future(10, '123') end end - - describe '#perform' do - let(:project) { create(:project) } - let(:import_state) { create(:import_state, project: project, jid: '123abc') } - - context 'when the project does not exist' do - it 'does nothing' do - expect(Gitlab::SidekiqStatus) - .not_to receive(:running?) - - worker.perform(-1, '123') - end - end - - context 'when the job is running' do - it 'refreshes the import JID and reschedules itself' do - allow(worker) - .to receive(:find_import_state) - .with(project.id) - .and_return(import_state) - - expect(Gitlab::SidekiqStatus) - .to receive(:running?) - .with('123') - .and_return(true) - - expect(Gitlab::SidekiqStatus) - .to receive(:expire) - .with('123', Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - - expect(Gitlab::SidekiqStatus) - .to receive(:set) - .with(import_state.jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - - expect(worker.class) - .to receive(:perform_in_the_future) - .with(project.id, '123') - - worker.perform(project.id, '123') - end - end - - context 'when the job is no longer running' do - it 'returns' do - allow(worker) - .to receive(:find_import_state) - .with(project.id) - .and_return(project) - - expect(Gitlab::SidekiqStatus) - .to receive(:running?) - .with('123') - .and_return(false) - - expect(Gitlab::SidekiqStatus) - .not_to receive(:expire) - - expect(Gitlab::SidekiqStatus) - .not_to receive(:set) - - worker.perform(project.id, '123') - end - end - end - - describe '#find_import_state' do - it 'returns a ProjectImportState' do - project = create(:project, :import_started) - - expect(worker.find_import_state(project.id)).to be_an_instance_of(ProjectImportState) - end - - # it 'only selects the import JID field' do - # project = create(:project, :import_started) - # project.import_state.update_attributes(jid: '123abc') - # - # expect(worker.find_project(project.id).attributes) - # .to eq({ 'id' => nil, 'import_jid' => '123abc' }) - # end - - it 'returns nil for a import state for which the import process failed' do - project = create(:project, :import_failed) - - expect(worker.find_import_state(project.id)).to be_nil - end - - it 'returns nil for a non-existing find_import_state' do - expect(worker.find_import_state(-1)).to be_nil - end - end end diff --git a/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb new file mode 100644 index 00000000000000..bf715db3851550 --- /dev/null +++ b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Import::RefreshImportJidWorker, feature_category: :importers do + let(:worker) { described_class.new } + + describe '.perform_in_the_future' do + it 'schedules a job in the future' do + expect(described_class) + .to receive(:perform_in) + .with(5.minutes.to_i, 10, '123') + + described_class.perform_in_the_future(10, '123') + end + end + + describe '#perform' do + let(:project) { create(:project) } + let(:import_state) { create(:import_state, project: project, jid: '123abc') } + + context 'when the project does not exist' do + it 'does nothing' do + expect(Gitlab::SidekiqStatus) + .not_to receive(:running?) + + worker.perform(-1, '123') + end + end + + context 'when the job is running' do + it 'refreshes the import JID and reschedules itself' do + allow(worker) + .to receive(:find_import_state) + .with(project.id) + .and_return(import_state) + + expect(Gitlab::SidekiqStatus) + .to receive(:running?) + .with('123') + .and_return(true) + + expect(Gitlab::SidekiqStatus) + .to receive(:expire) + .with('123', Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) + + expect(Gitlab::SidekiqStatus) + .to receive(:set) + .with(import_state.jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) + + expect(worker.class) + .to receive(:perform_in_the_future) + .with(project.id, '123') + + worker.perform(project.id, '123') + end + end + + context 'when the job is no longer running' do + it 'returns' do + allow(worker) + .to receive(:find_import_state) + .with(project.id) + .and_return(project) + + expect(Gitlab::SidekiqStatus) + .to receive(:running?) + .with('123') + .and_return(false) + + expect(Gitlab::SidekiqStatus) + .not_to receive(:expire) + + expect(Gitlab::SidekiqStatus) + .not_to receive(:set) + + worker.perform(project.id, '123') + end + end + end + + describe '#find_import_state' do + it 'returns a ProjectImportState' do + project = create(:project, :import_started) + + expect(worker.find_import_state(project.id)).to be_an_instance_of(ProjectImportState) + end + + # it 'only selects the import JID field' do + # project = create(:project, :import_started) + # project.import_state.update_attributes(jid: '123abc') + # + # expect(worker.find_project(project.id).attributes) + # .to eq({ 'id' => nil, 'import_jid' => '123abc' }) + # end + + it 'returns nil for a import state for which the import process failed' do + project = create(:project, :import_failed) + + expect(worker.find_import_state(project.id)).to be_nil + end + + it 'returns nil for a non-existing find_import_state' do + expect(worker.find_import_state(-1)).to be_nil + end + end +end -- GitLab From e217032504bb4479c77846826310ad43a6a088a6 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 5 Feb 2024 15:29:53 -0500 Subject: [PATCH 4/9] Fixed style issues, confirmed idempotency --- .../style/inline_disable_annotation.yml | 1 - app/workers/all_queues.yml | 2 +- .../import/refresh_import_jid_worker.rb | 17 ++--- .../stage_methods_shared_examples.rb | 2 +- .../import/refresh_import_jid_worker_spec.rb | 66 +++++++------------ 5 files changed, 30 insertions(+), 58 deletions(-) diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml index 75c708fcc22afe..c1711c1ca4d04b 100644 --- a/.rubocop_todo/style/inline_disable_annotation.yml +++ b/.rubocop_todo/style/inline_disable_annotation.yml @@ -952,7 +952,6 @@ Style/InlineDisableAnnotation: - 'app/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker.rb' - 'app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb' - 'app/workers/gitlab/github_import/stage/import_repository_worker.rb' - - 'app/workers/gitlab/import/refresh_import_jid_worker.rb' - 'app/workers/gitlab/import/stuck_import_job.rb' - 'app/workers/gitlab/import/stuck_project_import_jobs_worker.rb' - 'app/workers/gitlab/jira_import/advance_stage_worker.rb' diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index f5edcd30de863d..e63f719c3b6a63 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -3187,7 +3187,7 @@ :urgency: :low :resource_boundary: :unknown :weight: 1 - :idempotent: false + :idempotent: true :tags: [] - :name: incident_management_close_incident :worker_name: IncidentManagement::CloseIncidentWorker diff --git a/app/workers/gitlab/import/refresh_import_jid_worker.rb b/app/workers/gitlab/import/refresh_import_jid_worker.rb index b5d464547671d5..d581f8c6fac0dc 100644 --- a/app/workers/gitlab/import/refresh_import_jid_worker.rb +++ b/app/workers/gitlab/import/refresh_import_jid_worker.rb @@ -2,10 +2,11 @@ module Gitlab module Import - class RefreshImportJidWorker # rubocop:disable Scalability/IdempotentWorker + class RefreshImportJidWorker include ApplicationWorker data_consistency :always + idempotent! feature_category :importers sidekiq_options dead: false @@ -23,14 +24,14 @@ def self.perform_in_the_future(*args) # check_job_id - The ID of the job for which to check the status. # params - to avoid multiple releases if parameters change def perform(project_id, check_job_id, params = {}) # rubocop:disable Lint/UnusedMethodArgument -- for future flexibily - import_state = find_import_state(project_id) - return unless import_state + import_state_jid = ProjectImportState.jid_by(project_id: project_id, status: :started)&.jid + return unless import_state_jid if SidekiqStatus.running?(check_job_id) # As long as the worker is running we want to keep refreshing # the worker's JID as well as the import's JID. Gitlab::SidekiqStatus.expire(check_job_id, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - Gitlab::SidekiqStatus.set(import_state.jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) + Gitlab::SidekiqStatus.set(import_state_jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) self.class.perform_in_the_future(project_id, check_job_id) end @@ -39,14 +40,6 @@ def perform(project_id, check_job_id, params = {}) # rubocop:disable Lint/Unused # the clone job completed successfully it will have scheduled the next # stage, if it died there's nothing we can do anyway. end - - # rubocop:disable CodeReuse/ActiveRecord - def find_import_state(project_id) - ProjectImportState.select(:jid) - .with_status(:started) - .find_by(project_id: project_id) - end - # rubocop:enable CodeReuse/ActiveRecord end end end diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb index b2c842c5085073..46771afaa3422d 100644 --- a/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/bitbucket_import/stage_methods_shared_examples.rb @@ -16,7 +16,7 @@ end end - describe '.perform' do + describe '#perform' do let(:worker) { described_class.new } it 'executes the import' do diff --git a/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb index bf715db3851550..c6a849d0e699bc 100644 --- a/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb +++ b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb @@ -16,30 +16,35 @@ end describe '#perform' do - let(:project) { create(:project) } - let(:import_state) { create(:import_state, project: project, jid: '123abc') } + let_it_be(:project) { create(:project) } + let(:import_state) { create(:import_state, project: project, jid: '123abc', status: :started) } context 'when the project does not exist' do + let(:job_args) { [-1, '123'] } + + include_examples 'an idempotent worker' + it 'does nothing' do expect(Gitlab::SidekiqStatus) .not_to receive(:running?) - worker.perform(-1, '123') + worker.perform(*job_args) end end context 'when the job is running' do - it 'refreshes the import JID and reschedules itself' do - allow(worker) - .to receive(:find_import_state) - .with(project.id) - .and_return(import_state) + let(:job_args) { [project.id, '123'] } - expect(Gitlab::SidekiqStatus) + before do + allow(Gitlab::SidekiqStatus) .to receive(:running?) .with('123') .and_return(true) + end + + include_examples 'an idempotent worker' + it 'refreshes the import JID and reschedules itself' do expect(Gitlab::SidekiqStatus) .to receive(:expire) .with('123', Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) @@ -52,56 +57,31 @@ .to receive(:perform_in_the_future) .with(project.id, '123') - worker.perform(project.id, '123') + worker.perform(*job_args) end end context 'when the job is no longer running' do - it 'returns' do - allow(worker) - .to receive(:find_import_state) - .with(project.id) - .and_return(project) + let(:job_args) { [project.id, '123'] } - expect(Gitlab::SidekiqStatus) + before do + allow(Gitlab::SidekiqStatus) .to receive(:running?) .with('123') .and_return(false) + end + + include_examples 'an idempotent worker' + it 'returns' do expect(Gitlab::SidekiqStatus) .not_to receive(:expire) expect(Gitlab::SidekiqStatus) .not_to receive(:set) - worker.perform(project.id, '123') + worker.perform(*job_args) end end end - - describe '#find_import_state' do - it 'returns a ProjectImportState' do - project = create(:project, :import_started) - - expect(worker.find_import_state(project.id)).to be_an_instance_of(ProjectImportState) - end - - # it 'only selects the import JID field' do - # project = create(:project, :import_started) - # project.import_state.update_attributes(jid: '123abc') - # - # expect(worker.find_project(project.id).attributes) - # .to eq({ 'id' => nil, 'import_jid' => '123abc' }) - # end - - it 'returns nil for a import state for which the import process failed' do - project = create(:project, :import_failed) - - expect(worker.find_import_state(project.id)).to be_nil - end - - it 'returns nil for a non-existing find_import_state' do - expect(worker.find_import_state(-1)).to be_nil - end - end end -- GitLab From fc66c55d0dcd146d5287c0e0867dde131610f77c Mon Sep 17 00:00:00 2001 From: Sam Word Date: Mon, 5 Feb 2024 16:44:04 -0500 Subject: [PATCH 5/9] Removed new rubocop_todo, added inline disable instead --- .../sidekiq_load_balancing/worker_data_consistency.yml | 1 - app/workers/gitlab/import/refresh_import_jid_worker.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml b/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml index 71e2d7b54d3192..865ec2f025d4f5 100644 --- a/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml +++ b/.rubocop_todo/sidekiq_load_balancing/worker_data_consistency.yml @@ -128,7 +128,6 @@ SidekiqLoadBalancing/WorkerDataConsistency: - 'app/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker.rb' - 'app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb' - 'app/workers/gitlab/github_import/stage/import_repository_worker.rb' - - 'app/workers/gitlab/import/refresh_import_jid_worker.rb' - 'app/workers/gitlab/jira_import/advance_stage_worker.rb' - 'app/workers/gitlab/jira_import/import_issue_worker.rb' - 'app/workers/gitlab/jira_import/stage/start_import_worker.rb' diff --git a/app/workers/gitlab/import/refresh_import_jid_worker.rb b/app/workers/gitlab/import/refresh_import_jid_worker.rb index d581f8c6fac0dc..b2d8a01673ae2c 100644 --- a/app/workers/gitlab/import/refresh_import_jid_worker.rb +++ b/app/workers/gitlab/import/refresh_import_jid_worker.rb @@ -5,7 +5,7 @@ module Import class RefreshImportJidWorker include ApplicationWorker - data_consistency :always + data_consistency :always # rubocop:disable SidekiqLoadBalancing/WorkerDataConsistency -- Use primary DB to check for refresh since StuckImportJob will use it to fail imports idempotent! feature_category :importers -- GitLab From d8d1572c1e48e40354ce6acb2e4240e8e622f7a9 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 7 Feb 2024 11:03:37 -0500 Subject: [PATCH 6/9] Fixed more rubocop issues, use it_behaves_like, cleanup etc. --- .../import/refresh_import_jid_worker.rb | 20 ++++++++----------- .../stage_methods_shared_examples.rb | 2 +- .../import/refresh_import_jid_worker_spec.rb | 6 +++--- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/app/workers/gitlab/import/refresh_import_jid_worker.rb b/app/workers/gitlab/import/refresh_import_jid_worker.rb index b2d8a01673ae2c..9ce4d92cf7c73d 100644 --- a/app/workers/gitlab/import/refresh_import_jid_worker.rb +++ b/app/workers/gitlab/import/refresh_import_jid_worker.rb @@ -5,7 +5,7 @@ module Import class RefreshImportJidWorker include ApplicationWorker - data_consistency :always # rubocop:disable SidekiqLoadBalancing/WorkerDataConsistency -- Use primary DB to check for refresh since StuckImportJob will use it to fail imports + data_consistency :delayed idempotent! feature_category :importers @@ -24,21 +24,17 @@ def self.perform_in_the_future(*args) # check_job_id - The ID of the job for which to check the status. # params - to avoid multiple releases if parameters change def perform(project_id, check_job_id, params = {}) # rubocop:disable Lint/UnusedMethodArgument -- for future flexibily + return if SidekiqStatus.running?(check_job_id) + import_state_jid = ProjectImportState.jid_by(project_id: project_id, status: :started)&.jid return unless import_state_jid - if SidekiqStatus.running?(check_job_id) - # As long as the worker is running we want to keep refreshing - # the worker's JID as well as the import's JID. - Gitlab::SidekiqStatus.expire(check_job_id, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - Gitlab::SidekiqStatus.set(import_state_jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - - self.class.perform_in_the_future(project_id, check_job_id) - end + # As long as the worker is running we want to keep refreshing + # the worker's JID as well as the import's JID. + Gitlab::SidekiqStatus.expire(check_job_id, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) + Gitlab::SidekiqStatus.set(import_state_jid, Gitlab::Import::StuckImportJob::IMPORT_JOBS_EXPIRATION) - # If the job is no longer running there's nothing else we need to do. If - # the clone job completed successfully it will have scheduled the next - # stage, if it died there's nothing we can do anyway. + self.class.perform_in_the_future(project_id, check_job_id) end end end diff --git a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb index bc781d6b8c3f1d..c73c9f6028228f 100644 --- a/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/bitbucket_server_import/stage_methods_shared_examples.rb @@ -16,7 +16,7 @@ end end - describe '.perform' do + describe '#perform' do let(:worker) { described_class.new } it 'executes the import' do diff --git a/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb index c6a849d0e699bc..ebb171db1bfb9e 100644 --- a/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb +++ b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb @@ -22,7 +22,7 @@ context 'when the project does not exist' do let(:job_args) { [-1, '123'] } - include_examples 'an idempotent worker' + it_behaves_like 'an idempotent worker' it 'does nothing' do expect(Gitlab::SidekiqStatus) @@ -42,7 +42,7 @@ .and_return(true) end - include_examples 'an idempotent worker' + it_behaves_like 'an idempotent worker' it 'refreshes the import JID and reschedules itself' do expect(Gitlab::SidekiqStatus) @@ -71,7 +71,7 @@ .and_return(false) end - include_examples 'an idempotent worker' + it_behaves_like 'an idempotent worker' it 'returns' do expect(Gitlab::SidekiqStatus) -- GitLab From 9327a41950fb0bc9076b4ff98a5470ef625cda53 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 7 Feb 2024 11:43:21 -0500 Subject: [PATCH 7/9] Denote unused params for future flexibility to fix rubocop --- app/workers/gitlab/import/refresh_import_jid_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/gitlab/import/refresh_import_jid_worker.rb b/app/workers/gitlab/import/refresh_import_jid_worker.rb index 9ce4d92cf7c73d..46ea7b80190a08 100644 --- a/app/workers/gitlab/import/refresh_import_jid_worker.rb +++ b/app/workers/gitlab/import/refresh_import_jid_worker.rb @@ -23,7 +23,7 @@ def self.perform_in_the_future(*args) # project_id - The ID of the project that is being imported. # check_job_id - The ID of the job for which to check the status. # params - to avoid multiple releases if parameters change - def perform(project_id, check_job_id, params = {}) # rubocop:disable Lint/UnusedMethodArgument -- for future flexibily + def perform(project_id, check_job_id, _params = {}) return if SidekiqStatus.running?(check_job_id) import_state_jid = ProjectImportState.jid_by(project_id: project_id, status: :started)&.jid -- GitLab From 4654c84bd53c7f515ddb19f28e7d224a37073d5d Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 7 Feb 2024 12:52:46 -0500 Subject: [PATCH 8/9] Fixed logic issue and newly broken spec --- app/workers/gitlab/import/refresh_import_jid_worker.rb | 2 +- spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/workers/gitlab/import/refresh_import_jid_worker.rb b/app/workers/gitlab/import/refresh_import_jid_worker.rb index 46ea7b80190a08..f9a7bf870afcb6 100644 --- a/app/workers/gitlab/import/refresh_import_jid_worker.rb +++ b/app/workers/gitlab/import/refresh_import_jid_worker.rb @@ -24,7 +24,7 @@ def self.perform_in_the_future(*args) # check_job_id - The ID of the job for which to check the status. # params - to avoid multiple releases if parameters change def perform(project_id, check_job_id, _params = {}) - return if SidekiqStatus.running?(check_job_id) + return unless SidekiqStatus.running?(check_job_id) import_state_jid = ProjectImportState.jid_by(project_id: project_id, status: :started)&.jid return unless import_state_jid diff --git a/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb index ebb171db1bfb9e..be4a585d60ed66 100644 --- a/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb +++ b/spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb @@ -26,7 +26,7 @@ it 'does nothing' do expect(Gitlab::SidekiqStatus) - .not_to receive(:running?) + .not_to receive(:expire) worker.perform(*job_args) end -- GitLab From ba5b753e8a82861de0117cbcb923a0d399064d31 Mon Sep 17 00:00:00 2001 From: Sam Word Date: Wed, 7 Feb 2024 17:40:38 -0500 Subject: [PATCH 9/9] Removed unnecessary rubocop exceptions --- .rubocop_todo/style/guard_clause.yml | 1 - spec/support/rspec_order_todo.yml | 1 - 2 files changed, 2 deletions(-) diff --git a/.rubocop_todo/style/guard_clause.yml b/.rubocop_todo/style/guard_clause.yml index 946be7f16395de..3ff511a25b76c6 100644 --- a/.rubocop_todo/style/guard_clause.yml +++ b/.rubocop_todo/style/guard_clause.yml @@ -234,7 +234,6 @@ Style/GuardClause: - 'app/workers/container_registry/migration/guard_worker.rb' - 'app/workers/deployments/hooks_worker.rb' - 'app/workers/deployments/link_merge_request_worker.rb' - - 'app/workers/gitlab/import/refresh_import_jid_worker.rb' - 'app/workers/google_cloud/create_cloudsql_instance_worker.rb' - 'app/workers/packages/cleanup/execute_policy_worker.rb' - 'app/workers/packages/maven/metadata/sync_worker.rb' diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index c0592c79e593bb..4c138dbc35ba20 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -9510,7 +9510,6 @@ - './spec/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker_spec.rb' - './spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb' - './spec/workers/gitlab/github_import/stage/import_repository_worker_spec.rb' -- './spec/workers/gitlab/import/refresh_import_jid_worker_spec.rb' - './spec/workers/gitlab/import/stuck_import_job_spec.rb' - './spec/workers/gitlab/import/stuck_project_import_jobs_worker_spec.rb' - './spec/workers/gitlab/jira_import/import_issue_worker_spec.rb' -- GitLab