Merge branch...

Merge branch '332565-rename-gitlabusagepingworker-to-gitlabservicepingworker-and-refactor-fix-specs' into 'master'

Rename GitlabUsagePingWorker

See merge request gitlab-org/gitlab!64244

.rubocop_manual_todo.yml

@@ -1756,6 +1756,7 @@ Gitlab/NamespacedClass:
     - 'app/workers/git_garbage_collect_worker.rb'
     - 'app/workers/gitlab_performance_bar_stats_worker.rb'
     - 'app/workers/gitlab_shell_worker.rb'
+    - 'app/workers/gitlab_service_ping_worker.rb'
     - 'app/workers/gitlab_usage_ping_worker.rb'
     - 'app/workers/group_destroy_worker.rb'
     - 'app/workers/group_export_worker.rb'

app/workers/all_queues.yml

@@ -265,6 +265,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: cronjob:gitlab_service_ping
+  :worker_name: GitlabServicePingWorker
+  :feature_category: :service_ping
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: cronjob:gitlab_usage_ping
   :worker_name: GitlabUsagePingWorker
   :feature_category: :service_ping

app/workers/gitlab_service_ping_worker.rb

+# frozen_string_literal: true
+
+class GitlabServicePingWorker # rubocop:disable Scalability/IdempotentWorker
+  LEASE_KEY = 'gitlab_service_ping_worker:ping'
+  LEASE_TIMEOUT = 86400
+
+  include ApplicationWorker
+  include CronjobQueue # rubocop:disable Scalability/CronWorkerContext
+  include Gitlab::ExclusiveLeaseHelpers
+
+  feature_category :service_ping
+  sidekiq_options retry: 3, dead: false
+  sidekiq_retry_in { |count| (count + 1) * 8.hours.to_i }
+
+  def perform
+    # Disable service ping for GitLab.com
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/292929 for details
+    return if Gitlab.com?
+
+    # Multiple Sidekiq workers could run this. We should only do this at most once a day.
+    in_lock(LEASE_KEY, ttl: LEASE_TIMEOUT) do
+      # Splay the request over a minute to avoid thundering herd problems.
+      sleep(rand(0.0..60.0).round(3))
+
+      ServicePing::SubmitService.new.execute
+    end
+  end
+end

app/workers/gitlab_usage_ping_worker.rb

 # frozen_string_literal: true
 
-class GitlabUsagePingWorker # rubocop:disable Scalability/IdempotentWorker
-  LEASE_KEY = 'gitlab_usage_ping_worker:ping'
-  LEASE_TIMEOUT = 86400
-
-  include ApplicationWorker
-  include CronjobQueue # rubocop:disable Scalability/CronWorkerContext
-  include Gitlab::ExclusiveLeaseHelpers
-
-  feature_category :service_ping
-  sidekiq_options retry: 3, dead: false
-  sidekiq_retry_in { |count| (count + 1) * 8.hours.to_i }
-
-  def perform
-    # Disable usage ping for GitLab.com
-    # See https://gitlab.com/gitlab-org/gitlab/-/issues/292929 for details
-    return if Gitlab.com?
-
-    # Multiple Sidekiq workers could run this. We should only do this at most once a day.
-    in_lock(LEASE_KEY, ttl: LEASE_TIMEOUT) do
-      # Splay the request over a minute to avoid thundering herd problems.
-      sleep(rand(0.0..60.0).round(3))
-
-      ServicePing::SubmitService.new.execute
-    end
-  end
+class GitlabUsagePingWorker < GitlabServicePingWorker # rubocop:disable Scalability/IdempotentWorker
 end

config/initializers/1_settings.rb

@@ -501,6 +501,9 @@ Settings.cron_jobs['stuck_export_jobs_worker']['job_class'] = 'StuckExportJobsWo
 Settings.cron_jobs['gitlab_usage_ping_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['gitlab_usage_ping_worker']['cron'] ||= nil # This is dynamically loaded in the sidekiq initializer
 Settings.cron_jobs['gitlab_usage_ping_worker']['job_class'] = 'GitlabUsagePingWorker'
+Settings.cron_jobs['gitlab_service_ping_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['gitlab_service_ping_worker']['cron'] ||= nil # This is dynamically loaded in the sidekiq initializer
+Settings.cron_jobs['gitlab_service_ping_worker']['job_class'] = 'GitlabServicePingWorker'
 Settings.cron_jobs['stuck_merge_jobs_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['stuck_merge_jobs_worker']['cron'] ||= '0 */2 * * *'
 Settings.cron_jobs['stuck_merge_jobs_worker']['job_class'] = 'StuckMergeJobsWorker'

config/settings.rb

@@ -163,7 +163,7 @@ class Settings < Settingslogic
     end
 
     def load_dynamic_cron_schedules!
-      cron_jobs['gitlab_usage_ping_worker']['cron'] ||= cron_for_usage_ping
+      cron_jobs['gitlab_service_ping_worker']['cron'] ||= cron_for_service_ping
     end
 
     private
@@ -197,7 +197,7 @@ class Settings < Settingslogic
     # Runs at a consistent random time of day on a day of the week based on
     # the instance UUID. This is to balance the load on the service receiving
     # these pings. The sidekiq job handles temporary http failures.
-    def cron_for_usage_ping
+    def cron_for_service_ping
       # Set a default UUID for the case when the UUID hasn't been initialized.
       uuid = Gitlab::CurrentSettings.uuid || 'uuid-not-set'
 

db/migrate/20210617180131_migrate_usage_ping_sidekiq_queue.rb

+# frozen_string_literal: true
+
+class MigrateUsagePingSidekiqQueue < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  # rubocop:disable Migration/SidekiqQueueMigrate
+  def up
+    sidekiq_queue_migrate 'cronjob:gitlab_usage_ping', to: 'cronjob:gitlab_service_ping'
+  end
+
+  def down
+    sidekiq_queue_migrate 'cronjob:gitlab_service_ping', to: 'cronjob:gitlab_usage_ping'
+  end
+  # rubocop:enable Migration/SidekiqQueueMigrate
+end

db/schema_migrations/20210617180131

+2adb38e71c6173350d1f98f3237b692e4f12c8a073115be23f3a713f69cde911
\ No newline at end of file

lib/tasks/gitlab/usage_data.rake

@@ -19,7 +19,7 @@ namespace :gitlab do
 
     desc 'GitLab | UsageData | Generate usage ping and send it to Versions Application'
     task generate_and_send: :environment do
-      result = SubmitUsagePingService.new.execute
+      result = ServicePing::SubmitService.new.execute
 
       puts Gitlab::Json.pretty_generate(result.attributes)
     end

spec/config/settings_spec.rb

@@ -113,12 +113,12 @@ RSpec.describe Settings do
     end
   end
 
-  describe '.cron_for_usage_ping' do
+  describe '.cron_for_service_ping' do
     it 'returns correct crontab for some manually calculated example' do
       allow(Gitlab::CurrentSettings)
         .to receive(:uuid) { 'd9e2f4e8-db1f-4e51-b03d-f427e1965c4a'}
 
-      expect(described_class.send(:cron_for_usage_ping)).to eq('21 18 * * 4')
+      expect(described_class.send(:cron_for_service_ping)).to eq('21 18 * * 4')
     end
 
     it 'returns min, hour, day in the valid range' do
@@ -126,7 +126,7 @@ RSpec.describe Settings do
         .to receive(:uuid) { SecureRandom.uuid }
 
       10.times do
-        cron = described_class.send(:cron_for_usage_ping).split(/\s/)
+        cron = described_class.send(:cron_for_service_ping).split(/\s/)
 
         expect(cron[0].to_i).to be_between(0, 59)
         expect(cron[1].to_i).to be_between(0, 23)

spec/workers/every_sidekiq_worker_spec.rb

@@ -287,6 +287,7 @@ RSpec.describe 'Every Sidekiq worker' do
         'Gitlab::PhabricatorImport::ImportTasksWorker' => 5,
         'GitlabPerformanceBarStatsWorker' => 3,
         'GitlabShellWorker' => 3,
+        'GitlabServicePingWorker' => 3,
         'GitlabUsagePingWorker' => 3,
         'GroupDestroyWorker' => 3,
         'GroupExportWorker' => false,

spec/workers/gitlab_service_ping_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GitlabServicePingWorker, :clean_gitlab_redis_shared_state do
+  before do
+    allow_next_instance_of(ServicePing::SubmitService) { |service| allow(service).to receive(:execute) }
+    allow(subject).to receive(:sleep)
+  end
+
+  it 'does not run for GitLab.com' do
+    allow(Gitlab).to receive(:com?).and_return(true)
+    expect(ServicePing::SubmitService).not_to receive(:new)
+
+    subject.perform
+  end
+
+  it 'delegates to ServicePing::SubmitService' do
+    expect_next_instance_of(ServicePing::SubmitService) { |service| expect(service).to receive(:execute) }
+
+    subject.perform
+  end
+
+  it "obtains a #{described_class::LEASE_TIMEOUT} second exclusive lease" do
+    expect(Gitlab::ExclusiveLeaseHelpers::SleepingLock)
+      .to receive(:new)
+      .with(described_class::LEASE_KEY, hash_including(timeout: described_class::LEASE_TIMEOUT))
+      .and_call_original
+
+    subject.perform
+  end
+
+  it 'sleeps for between 0 and 60 seconds' do
+    expect(subject).to receive(:sleep).with(0..60)
+
+    subject.perform
+  end
+
+  context 'when lease is not obtained' do
+    before do
+      Gitlab::ExclusiveLease.new(described_class::LEASE_KEY, timeout: described_class::LEASE_TIMEOUT).try_obtain
+    end
+
+    it 'does not invoke ServicePing::SubmitService' do
+      allow_next_instance_of(ServicePing::SubmitService) { |service| expect(service).not_to receive(:execute) }
+
+      expect { subject.perform }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
+    end
+  end
+end