Commit 443ea09b authored by Mehmet Emin INAC's avatar Mehmet Emin INAC

Retry failed vulnerability export jobs up to 3 times

Previously we were marking the export object as failed if it fails in
the first export try and were suppressing the exception to fool sidekiq
to not to retry the job.

We decided to change this behaviour and let sidekiq to try the job if
it fails.
parent 38332ea9
...@@ -34,6 +34,10 @@ module Vulnerabilities ...@@ -34,6 +34,10 @@ module Vulnerabilities
transition [:created, :running] => :failed transition [:created, :running] => :failed
end end
event :reset_state do
transition running: :created
end
state :created state :created
state :running state :running
state :finished state :finished
......
...@@ -28,8 +28,7 @@ module VulnerabilityExports ...@@ -28,8 +28,7 @@ module VulnerabilityExports
attr_accessor :vulnerability_export attr_accessor :vulnerability_export
delegate :exportable, to: :vulnerability_export, private: true delegate :exportable, :format, to: :vulnerability_export, private: true
delegate :format, to: :vulnerability_export, private: true
def lease_key def lease_key
"#{LEASE_NAMESPACE}:#{vulnerability_export.id}" "#{LEASE_NAMESPACE}:#{vulnerability_export.id}"
...@@ -39,9 +38,9 @@ module VulnerabilityExports ...@@ -39,9 +38,9 @@ module VulnerabilityExports
vulnerability_export.start! vulnerability_export.start!
generate_export_file generate_export_file
vulnerability_export.finish! vulnerability_export.finish!
rescue => error rescue StandardError
vulnerability_export.failed! vulnerability_export.reset_state!
raise(error) raise
ensure ensure
schedule_export_deletion schedule_export_deletion
end end
......
...@@ -5,20 +5,22 @@ module VulnerabilityExports ...@@ -5,20 +5,22 @@ module VulnerabilityExports
include ApplicationWorker include ApplicationWorker
include ::Gitlab::ExclusiveLeaseHelpers include ::Gitlab::ExclusiveLeaseHelpers
LEASE_TIMEOUT = 1.hour sidekiq_options dead: false
feature_category :vulnerability_management feature_category :vulnerability_management
worker_resource_boundary :cpu worker_resource_boundary :cpu
idempotent! idempotent!
sidekiq_retries_exhausted do |job|
Vulnerabilities::Export.find_by_id(job['args'].last).failed!
end
def perform(_exportable_id = nil, vulnerability_export_id) # rubocop:disable Style/OptionalArguments def perform(_exportable_id = nil, vulnerability_export_id) # rubocop:disable Style/OptionalArguments
vulnerability_export = Vulnerabilities::Export.find_by_id(vulnerability_export_id) vulnerability_export = Vulnerabilities::Export.find_by_id(vulnerability_export_id)
return unless vulnerability_export return unless vulnerability_export
ExportService.export(vulnerability_export) ExportService.export(vulnerability_export)
rescue => error
logger.error class: self.class.name, message: error.message
end end
end end
end end
...@@ -66,9 +66,9 @@ RSpec.describe VulnerabilityExports::ExportService do ...@@ -66,9 +66,9 @@ RSpec.describe VulnerabilityExports::ExportService do
allow(service_object).to receive(:generate_export_file).and_raise(error) allow(service_object).to receive(:generate_export_file).and_raise(error)
end end
it 'marks the export object as `failed` and propagates the error to the caller' do it 'sets the state of export back to `created`' do
expect { export }.to raise_error(error) expect { export }.to raise_error(error)
expect(vulnerability_export.failed?).to be_truthy expect(vulnerability_export.reload.created?).to be_truthy
end end
it 'schedules the export deletion background job' do it 'schedules the export deletion background job' do
......
...@@ -3,9 +3,9 @@ ...@@ -3,9 +3,9 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe VulnerabilityExports::ExportWorker, type: :worker do RSpec.describe VulnerabilityExports::ExportWorker, type: :worker do
describe '#perform' do
let(:worker) { described_class.new } let(:worker) { described_class.new }
describe '#perform' do
subject(:export_vulnerabilities) { worker.perform(vulnerability_export_id) } subject(:export_vulnerabilities) { worker.perform(vulnerability_export_id) }
before do before do
...@@ -36,24 +36,36 @@ RSpec.describe VulnerabilityExports::ExportWorker, type: :worker do ...@@ -36,24 +36,36 @@ RSpec.describe VulnerabilityExports::ExportWorker, type: :worker do
expect(VulnerabilityExports::ExportService).to have_received(:export).with(vulnerability_export) expect(VulnerabilityExports::ExportService).to have_received(:export).with(vulnerability_export)
end end
end
end
context 'when export fails' do describe 'sidekiq_retries_exhausted callback' do
let(:error_message) { 'Foo' } let(:sidekiq_retry_handler) { Sidekiq::JobRetry.new }
let(:vulnerability_export) { create(:vulnerability_export, :created, :csv) }
let(:default_job_payload) { { 'class' => described_class.name, 'args' => [vulnerability_export.id] } }
before do subject(:run_job) do
allow(VulnerabilityExports::ExportService).to receive(:export).and_raise(error_message) sidekiq_retry_handler.local(worker, job_payload, 'default') do
raise 'Foo'
end end
it 'does not raise exception' do
expect { export_vulnerabilities }.not_to raise_error
end end
it 'logs error' do context 'when the max retry count is not reached' do
export_vulnerabilities let(:job_payload) { default_job_payload.merge('retry_count' => 1) }
expect(Sidekiq.logger).to have_received(:error).with(class: described_class.name, message: error_message) it 'does not mark the vulnerability export object as failed' do
expect { run_job }.to raise_error(Sidekiq::JobRetry::Skip)
.and not_change { vulnerability_export.reload.failed? }.from(false)
end end
end end
context 'when the max retry count is reached' do
let(:job_payload) { default_job_payload.merge('retry_count' => 2) }
it 'marks the vulnerability export object as failed' do
expect { run_job }.to raise_error(Sidekiq::JobRetry::Skip)
.and change { vulnerability_export.reload.failed? }.from(false).to(true)
end
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment