From 8d17c4dae6b4662dddffe9e2ddca8100e8cd3d0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Wed, 21 Aug 2019 12:03:42 +0200 Subject: Properly handle `sidekiq` skip Transform `CancelledError` into `JobRetry::Skip` --- config/initializers/sidekiq.rb | 7 +++---- lib/gitlab/sidekiq_middleware/monitor.rb | 3 +++ lib/gitlab/sidekiq_monitor.rb | 12 ++++++------ spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb | 12 ++++++++++++ spec/lib/gitlab/sidekiq_monitor_spec.rb | 14 +++++++------- 5 files changed, 31 insertions(+), 17 deletions(-) diff --git a/config/initializers/sidekiq.rb b/config/initializers/sidekiq.rb index e145af5e2d5..9f3e104bc2b 100644 --- a/config/initializers/sidekiq.rb +++ b/config/initializers/sidekiq.rb @@ -28,12 +28,13 @@ if Rails.env.development? end enable_json_logs = Gitlab.config.sidekiq.log_format == 'json' +enable_sidekiq_monitor = ENV.fetch("SIDEKIQ_MONITOR_WORKER", 0).to_i.nonzero? Sidekiq.configure_server do |config| config.redis = queues_config_hash config.server_middleware do |chain| - chain.add Gitlab::SidekiqMiddleware::Monitor + chain.add Gitlab::SidekiqMiddleware::Monitor if enable_sidekiq_monitor chain.add Gitlab::SidekiqMiddleware::Metrics if Settings.monitoring.sidekiq_exporter chain.add Gitlab::SidekiqMiddleware::ArgumentsLogger if ENV['SIDEKIQ_LOG_ARGUMENTS'] && !enable_json_logs chain.add Gitlab::SidekiqMiddleware::MemoryKiller if ENV['SIDEKIQ_MEMORY_KILLER_MAX_RSS'] @@ -59,9 +60,7 @@ Sidekiq.configure_server do |config| # Sidekiq (e.g. in an initializer). ActiveRecord::Base.clear_all_connections! - if ENV.fetch("SIDEKIQ_MONITOR_WORKER", 0).to_i.nonzero? - Gitlab::SidekiqMonitor.instance.start - end + Gitlab::SidekiqMonitor.instance.start if enable_sidekiq_monitor end if enable_reliable_fetch? diff --git a/lib/gitlab/sidekiq_middleware/monitor.rb b/lib/gitlab/sidekiq_middleware/monitor.rb index 2d0e5a6d635..0d88fe760d3 100644 --- a/lib/gitlab/sidekiq_middleware/monitor.rb +++ b/lib/gitlab/sidekiq_middleware/monitor.rb @@ -7,6 +7,9 @@ module Gitlab Gitlab::SidekiqMonitor.instance.within_job(job['jid'], queue) do yield end + rescue Gitlab::SidekiqMonitor::CancelledError + # ignore retries + raise Sidekiq::JobRetry::Skip end end end diff --git a/lib/gitlab/sidekiq_monitor.rb b/lib/gitlab/sidekiq_monitor.rb index 9ec3be48adb..9842f1f53f7 100644 --- a/lib/gitlab/sidekiq_monitor.rb +++ b/lib/gitlab/sidekiq_monitor.rb @@ -30,7 +30,7 @@ module Gitlab if cancelled?(jid) Sidekiq.logger.warn( - class: self.class, + class: self.class.to_s, action: 'run', queue: queue, jid: jid, @@ -62,7 +62,7 @@ module Gitlab def start_working Sidekiq.logger.info( - class: self.class, + class: self.class.to_s, action: 'start', message: 'Starting Monitor Daemon' ) @@ -74,7 +74,7 @@ module Gitlab ensure Sidekiq.logger.warn( - class: self.class, + class: self.class.to_s, action: 'stop', message: 'Stopping Monitor Daemon' ) @@ -94,7 +94,7 @@ module Gitlab end rescue Exception => e # rubocop:disable Lint/RescueException Sidekiq.logger.warn( - class: self.class, + class: self.class.to_s, action: 'exception', message: e.message ) @@ -105,7 +105,7 @@ module Gitlab def process_message(message) Sidekiq.logger.info( - class: self.class, + class: self.class.to_s, channel: NOTIFICATION_CHANNEL, message: 'Received payload on channel', payload: message @@ -139,7 +139,7 @@ module Gitlab # running job find_thread_with_lock(jid) do |thread| Sidekiq.logger.warn( - class: self.class, + class: self.class.to_s, action: 'cancel', message: 'Canceling thread with CancelledError', jid: jid, diff --git a/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb b/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb index 3ca2ddf3cb1..2933d26a387 100644 --- a/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/monitor_spec.rb @@ -25,5 +25,17 @@ describe Gitlab::SidekiqMiddleware::Monitor do expect(result).to eq('value') end + + context 'when cancel happens' do + subject do + monitor.call(worker, job, queue) do + raise Gitlab::SidekiqMonitor::CancelledError + end + end + + it 'does skip this job' do + expect { subject }.to raise_error(Sidekiq::JobRetry::Skip) + end + end end end diff --git a/spec/lib/gitlab/sidekiq_monitor_spec.rb b/spec/lib/gitlab/sidekiq_monitor_spec.rb index 62baa326887..bbd7bf90217 100644 --- a/spec/lib/gitlab/sidekiq_monitor_spec.rb +++ b/spec/lib/gitlab/sidekiq_monitor_spec.rb @@ -54,7 +54,7 @@ describe Gitlab::SidekiqMonitor do it 'logs start message' do expect(Sidekiq.logger).to receive(:info) .with( - class: described_class, + class: described_class.to_s, action: 'start', message: 'Starting Monitor Daemon') @@ -66,7 +66,7 @@ describe Gitlab::SidekiqMonitor do it 'logs stop message' do expect(Sidekiq.logger).to receive(:warn) .with( - class: described_class, + class: described_class.to_s, action: 'stop', message: 'Stopping Monitor Daemon') @@ -78,7 +78,7 @@ describe Gitlab::SidekiqMonitor do it 'logs StandardError message' do expect(Sidekiq.logger).to receive(:warn) .with( - class: described_class, + class: described_class.to_s, action: 'exception', message: 'My Exception') @@ -91,7 +91,7 @@ describe Gitlab::SidekiqMonitor do it 'logs and raises Exception message' do expect(Sidekiq.logger).to receive(:warn) .with( - class: described_class, + class: described_class.to_s, action: 'exception', message: 'My Exception') @@ -131,13 +131,13 @@ describe Gitlab::SidekiqMonitor do expect(Sidekiq.logger).to receive(:info) .with( - class: described_class, + class: described_class.to_s, action: 'start', message: 'Starting Monitor Daemon') expect(Sidekiq.logger).to receive(:info) .with( - class: described_class, + class: described_class.to_s, channel: described_class::NOTIFICATION_CHANNEL, message: 'Received payload on channel', payload: payload @@ -215,7 +215,7 @@ describe Gitlab::SidekiqMonitor do it 'does log cancellation message' do expect(Sidekiq.logger).to receive(:warn) .with( - class: described_class, + class: described_class.to_s, action: 'cancel', message: 'Canceling thread with CancelledError', jid: 'my-jid', -- cgit v1.2.3