Merge branch 'sc1-maildelivery-shard-awareness' into 'master'

Add Sidekiq shard-support for active jobs

See merge request !148637



Merged-by: Drew Blessing <drew@gitlab.com>
Approved-by: Heinrich Lee Yu <heinrich@gitlab.com>
Approved-by: Drew Blessing <drew@gitlab.com>
Reviewed-by: Drew Blessing <drew@gitlab.com>
Reviewed-by: Heinrich Lee Yu <heinrich@gitlab.com>
Co-authored-by: Sylvester Chin <schin@gitlab.com>

.rubocop.yml

@@ -1046,7 +1046,6 @@ Cop/RedisQueueUsage:
     - 'lib/gitlab/redis.rb'
     - 'lib/system_check/app/redis_version_check.rb'
     - 'lib/gitlab/mail_room.rb'
-    - 'config/initializers/sidekiq_route_patch.rb'
     - 'lib/gitlab/sidekiq_sharding/scheduled_enq.rb'
     - 'lib/gitlab/sidekiq_sharding/router.rb'
     - 'app/workers/concerns/application_worker.rb'
@@ -1064,11 +1063,11 @@ Cop/SidekiqApiUsage:
     - 'lib/gitlab/sidekiq_queue.rb'
     - 'config/initializers/sidekiq.rb'
     - 'config/initializers/forbid_sidekiq_in_transactions.rb'
-    - 'config/initializers/sidekiq_route_patch.rb'
     - 'lib/gitlab/sidekiq_sharding/scheduled_enq.rb'
     - 'lib/gitlab/sidekiq_sharding/router.rb'
     - 'lib/gitlab/redis/queues.rb'
     - 'app/workers/concerns/application_worker.rb'
+    - 'config/initializers/active_job_shard_support.rb'
 
 Cop/FeatureFlagUsage:
   Include:

.rubocop_todo/rspec/feature_category.yml

@@ -2465,7 +2465,6 @@ RSpec/FeatureCategory:
     - 'spec/initializers/hashie_mash_permitted_patch_spec.rb'
     - 'spec/initializers/lograge_spec.rb'
     - 'spec/initializers/mail_encoding_patch_spec.rb'
-    - 'spec/initializers/mailer_retries_spec.rb'
     - 'spec/initializers/microsoft_graph_mailer_spec.rb'
     - 'spec/initializers/net_http_patch_spec.rb'
     - 'spec/initializers/omniauth_spec.rb'

config/initializers/active_job_shard_support.rb

+# frozen_string_literal: true
+
+# As discussed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/148637#note_1850247875,
+# Rails 7.1 introduces enqueue_all which is not covered in this patch.
+if Gem::Version.new(Rails.gem_version) >= Gem::Version.new('7.1')
+  raise 'New version of Rails detected, please remove or update this patch'
+end
+
+# We deliver emails using the `deliver_later` method and it uses ActiveJob
+# under the hood, which later processes the email via the defined ActiveJob adapter's `enqueue` method.
+# For GitLab, the ActiveJob adapter is Sidekiq (in development and production environments).
+# We need to set the following up to override the ActiveJob adapater
+# so as to ensure that mailer jobs are enqueued in a shard-aware manner.
+module ActiveJob
+  module QueueAdapters
+    module ActiveJobShardSupport
+      %i[enqueue enqueue_at].each do |name|
+        define_method(name) do |*args|
+          return super(*args) unless Gitlab::SidekiqSharding::Router.enabled?
+
+          store_name = ActionMailer::MailDeliveryJob.sidekiq_options['store']
+          _, pool = Gitlab::SidekiqSharding::Router.get_shard_instance(store_name)
+
+          Sidekiq::Client.via(pool) do
+            super(*args)
+          end
+        end
+      end
+    end
+
+    # This adapter is used in development & production environments.
+    class SidekiqAdapter
+      prepend ActiveJobShardSupport
+    end
+  end
+end

config/initializers/mailer_retries.rb → config/initializers/mailer_options.rb

@@ -4,4 +4,14 @@
   ActionMailer::MailDeliveryJob.sidekiq_options retry: 3
   ActionMailer::MailDeliveryJob.include(WorkerAttributes)
   ActionMailer::MailDeliveryJob.data_consistency :delayed
+
+  # ActionMailer::MailDeliveryJob is made compatible with the WorkerRouter using the DummyWorker class
+  klass = Gitlab::SidekiqConfig::DEFAULT_WORKERS['ActionMailer::MailDeliveryJob'].klass
+
+  # Assigns store once during initialisation instead of during active job enqueue
+  store_name = Gitlab::SidekiqConfig::WorkerRouter.global.store(klass)
+  ActionMailer::MailDeliveryJob.sidekiq_options store: store_name
+
+  # Assigns store for JobWrapper class for accuracy of client-side metric's store label
+  ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper.sidekiq_options store: store_name
 end

spec/initializers/active_job_shard_support_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'ActionMailer::MailDeliveryJob', :sidekiq_mailers, feature_category: :scalability do
+  let(:mailer_class) do
+    Class.new(ApplicationMailer) do
+      def self.name
+        'Notify'
+      end
+
+      def test_mail; end
+    end
+  end
+
+  before do
+    # The client middleware is invoked in a Sidekiq::Client.push, where
+    # ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper is passed into the Router.
+    allow(Gitlab::SidekiqSharding::Router).to receive(:get_shard_instance).with(nil).and_call_original
+  end
+
+  context 'when routing is not enabled' do
+    before do
+      allow(Gitlab::SidekiqSharding::Router).to receive(:enabled?).and_return(false)
+    end
+
+    it 'does not check for shard instance' do
+      expect(ActionMailer::MailDeliveryJob).not_to receive(:sidekiq_options)
+
+      mailer_class.test_mail.deliver_later
+    end
+  end
+
+  context 'when routing is enabled' do
+    before do
+      allow(Gitlab::SidekiqSharding::Router).to receive(:enabled?).and_return(true)
+    end
+
+    it 'checks for shard instance and sets Sidekiq redis pool' do
+      expect(ActionMailer::MailDeliveryJob).to receive(:sidekiq_options).ordered.and_call_original
+      expect(Gitlab::SidekiqSharding::Router).to receive(:get_shard_instance).ordered.and_call_original
+      expect(Sidekiq::Client).to receive(:via).ordered.and_call_original
+
+      mailer_class.test_mail.deliver_later
+    end
+  end
+end

spec/initializers/mailer_options_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Mailer retries', :sidekiq_mailers, feature_category: :shared do
+  subject(:mail) { DeviseMailer.user_admin_approval(create(:user)).deliver_later }
+
+  it 'sets retries for mailers to 3' do
+    mail
+
+    expect(Sidekiq::Queues['mailers'].first).to include('retry' => 3)
+  end
+
+  it 'sets data consistency for mailers to :delayed' do
+    mail
+
+    expect(Sidekiq::Queues['mailers'].first).to include('worker_data_consistency' => 'delayed')
+    expect(ActionMailer::MailDeliveryJob.get_data_consistency).to eq(:delayed)
+  end
+
+  it 'sets store for mailers to ActionMailer::MailDeliveryJob routing target' do
+    mail
+
+    # The store name depends on config/gitlab.yml's sidekiq.routingRules. This is set in the
+    # initializers which makes it unwieldy to stub.
+    store_name = Gitlab::SidekiqConfig::WorkerRouter.global.store(
+      Gitlab::SidekiqConfig::DEFAULT_WORKERS['ActionMailer::MailDeliveryJob'].klass
+    )
+
+    expect(ActionMailer::MailDeliveryJob.sidekiq_options['store']).to eq(store_name)
+  end
+
+  it 'sets store for mailers to ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper routing target' do
+    mail
+
+    # The store name depends on config/gitlab.yml's sidekiq.routingRules. This is set in the
+    # initializers which makes it unwieldy to stub.
+    store_name = Gitlab::SidekiqConfig::WorkerRouter.global.store(
+      Gitlab::SidekiqConfig::DEFAULT_WORKERS['ActionMailer::MailDeliveryJob'].klass
+    )
+
+    expect(ActiveJob::QueueAdapters::SidekiqAdapter::JobWrapper.sidekiq_options['store']).to eq(store_name)
+  end
+end

spec/initializers/mailer_retries_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe 'Mailer retries', :sidekiq_mailers do
-  it 'sets retries for mailers to 3' do
-    DeviseMailer.user_admin_approval(create(:user)).deliver_later
-
-    expect(Sidekiq::Queues['mailers'].first).to include('retry' => 3)
-  end
-end

spec/support/rspec_order_todo.yml

@@ -4816,7 +4816,7 @@
 - './spec/initializers/google_api_client_spec.rb'
 - './spec/initializers/lograge_spec.rb'
 - './spec/initializers/mail_encoding_patch_spec.rb'
-- './spec/initializers/mailer_retries_spec.rb'
+- './spec/initializers/mailer_options_spec.rb'
 - './spec/initializers/memory_watchdog_spec.rb'
 - './spec/initializers/net_http_patch_spec.rb'
 - './spec/initializers/net_http_response_patch_spec.rb'