Merge branch 'bvl-add-context-to-pages-workers' into 'master'

Add context to the pages domain cron workers

See merge request gitlab-org/gitlab!24623

app/models/pages_domain.rb

@@ -62,6 +62,8 @@ class PagesDomain < ApplicationRecord
 
   scope :for_removal, -> { where("remove_at < ?", Time.now) }
 
+  scope :with_logging_info, -> { includes(project: [:namespace, :route]) }
+
   def verified?
     !!verified_at
   end
@@ -285,3 +287,5 @@ class PagesDomain < ApplicationRecord
     !auto_ssl_enabled? && project&.pages_https_only?
   end
 end
+
+PagesDomain.prepend_if_ee('::EE::PagesDomain')

app/workers/pages_domain_removal_cron_worker.rb

@@ -2,14 +2,14 @@
 
 class PagesDomainRemovalCronWorker
   include ApplicationWorker
-  include CronjobQueue # rubocop:disable Scalability/CronWorkerContext
+  include CronjobQueue
 
   feature_category :pages
   worker_resource_boundary :cpu
 
   def perform
-    PagesDomain.for_removal.find_each do |domain|
-      domain.destroy!
+    PagesDomain.for_removal.with_logging_info.find_each do |domain|
+      with_context(project: domain.project) { domain.destroy! }
     rescue => e
       Gitlab::ErrorTracking.track_exception(e)
     end

app/workers/pages_domain_ssl_renewal_cron_worker.rb

@@ -2,15 +2,17 @@
 
 class PagesDomainSslRenewalCronWorker
   include ApplicationWorker
-  include CronjobQueue # rubocop:disable Scalability/CronWorkerContext
+  include CronjobQueue
 
   feature_category :pages
 
   def perform
     return unless ::Gitlab::LetsEncrypt.enabled?
 
-    PagesDomain.need_auto_ssl_renewal.find_each do |domain|
-      PagesDomainSslRenewalWorker.perform_async(domain.id)
+    PagesDomain.need_auto_ssl_renewal.with_logging_info.find_each do |domain|
+      with_context(project: domain.project) do
+        PagesDomainSslRenewalWorker.perform_async(domain.id)
+      end
     end
   end
 end

app/workers/pages_domain_verification_cron_worker.rb

@@ -2,15 +2,17 @@
 
 class PagesDomainVerificationCronWorker
   include ApplicationWorker
-  include CronjobQueue # rubocop:disable Scalability/CronWorkerContext
+  include CronjobQueue
 
   feature_category :pages
 
   def perform
     return if Gitlab::Database.read_only?
 
-    PagesDomain.needs_verification.find_each do |domain|
-      PagesDomainVerificationWorker.perform_async(domain.id)
+    PagesDomain.needs_verification.with_logging_info.find_each do |domain|
+      with_context(project: domain.project) do
+        PagesDomainVerificationWorker.perform_async(domain.id)
+      end
     end
   end
 end

ee/app/models/ee/pages_domain.rb

+# frozen_string_literal: true
+
+module EE
+  module PagesDomain
+    extend ActiveSupport::Concern
+
+    prepended do
+      scope :with_logging_info, -> { includes(project: [:route, { namespace: [:plan, :gitlab_subscription] }]) }
+    end
+  end
+end

spec/factories/pages_domains.rb

@@ -380,5 +380,9 @@ x6zG6WoibsbsJMj70nwseUnPTBQNDP+j61RJjC/r
       scope { :instance }
       usage { :serverless }
     end
+
+    trait :with_project do
+      association :project
+    end
   end
 end

spec/support/shared_examples/workers/pages_domain_cron_worker_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'a pages cronjob scheduling jobs with context' do |scheduled_worker_class|
+  let(:worker) { described_class.new }
+
+  it 'does not cause extra queries for multiple domains' do
+    control = ActiveRecord::QueryRecorder.new { worker.perform }
+
+    extra_domain
+
+    expect { worker.perform }.not_to exceed_query_limit(control)
+  end
+
+  it 'schedules the renewal with a context' do
+    extra_domain
+
+    worker.perform
+
+    expect(scheduled_worker_class.jobs.last).to include("meta.project" => extra_domain.project.full_path)
+  end
+end

spec/workers/pages_domain_ssl_renewal_cron_worker_spec.rb

@@ -12,7 +12,7 @@ describe PagesDomainSslRenewalCronWorker do
   end
 
   describe '#perform' do
-    let(:project) { create :project }
+    let_it_be(:project) { create :project }
     let!(:domain) { create(:pages_domain, project: project, auto_ssl_enabled: false) }
     let!(:domain_with_enabled_auto_ssl) { create(:pages_domain, project: project, auto_ssl_enabled: true) }
     let!(:domain_with_obtained_letsencrypt) do
@@ -35,12 +35,16 @@ describe PagesDomainSslRenewalCronWorker do
 
       [domain,
        domain_with_obtained_letsencrypt].each do |domain|
-        expect(PagesDomainVerificationWorker).not_to receive(:perform_async).with(domain.id)
+        expect(PagesDomainSslRenewalWorker).not_to receive(:perform_async).with(domain.id)
       end
 
       worker.perform
     end
 
+    it_behaves_like 'a pages cronjob scheduling jobs with context', PagesDomainSslRenewalWorker do
+      let(:extra_domain) { create(:pages_domain, :with_project, auto_ssl_enabled: true) }
+    end
+
     shared_examples 'does nothing' do
       it 'does nothing' do
         expect(PagesDomainSslRenewalWorker).not_to receive(:perform_async)

spec/workers/pages_domain_verification_cron_worker_spec.rb

@@ -5,9 +5,9 @@ require 'spec_helper'
 describe PagesDomainVerificationCronWorker do
   subject(:worker) { described_class.new }
 
-  describe '#perform' do
+  describe '#perform', :sidekiq do
     let!(:verified) { create(:pages_domain) }
-    let!(:reverify) { create(:pages_domain, :reverify) }
+    let!(:reverify) { create(:pages_domain, :reverify, :with_project) }
     let!(:disabled) { create(:pages_domain, :disabled) }
 
     it 'does nothing if the database is read-only' do
@@ -26,5 +26,9 @@ describe PagesDomainVerificationCronWorker do
 
       worker.perform
     end
+
+    it_behaves_like 'a pages cronjob scheduling jobs with context', PagesDomainVerificationWorker do
+      let(:extra_domain) { create(:pages_domain, :reverify, :with_project) }
+    end
   end
 end