Commit b5b41f1b authored by Bob Van Landuyt's avatar Bob Van Landuyt

Add cops for workers that need context information

This adds 2 cops for jobs scheduled that will likely miss context
information:

*CronWorkerContext*

Cron-Workers themselves run instance wide, so they aren't scoped to
users, namespaces, projects or other resources that should be added to
the context.

However, they often schedule other jobs that _do_ require context.

That is why the there needs to be an indication of context somewhere
in the worker. This can be done by using one of the following methods:

1. Wrap the code that schedules jobs in the `with_context` helper:

  def perform
    deletion_cutoff = Gitlab::CurrentSettings
                        .deletion_adjourned_period.days.ago.to_date
    projects = Project.with_route.with_namespace
                 .aimed_for_deletion(deletion_cutoff)

    projects.find_each(batch_size: 100).with_index do |project, index|
      delay = index * INTERVAL

      with_context(project: project) do
        AdjournedProjectDeletionWorker.perform_in(delay, project.id)
      end
    end
  end

2. Use the a batch scheduling method that provides context

  def schedule_projects_in_batch(projects)
    ProjectImportScheduleWorker.bulk_perform_async_with_contexts(
      projects,
      arguments_proc: -> (project) { project.id },
      context_proc: -> (project) { { project: project } }
    )
  end

*BulkPerformWithContext*

Often, when scheduling jobs in bulk, these jobs should have a separate
context rather than the overarching context.

If that is the case, `bulk_perform_async` can be replaced by the
`bulk_perform_async_with_context` helper, and instead of
`bulk_perform_in` use `bulk_perform_in_with_context`.

For example:

    ProjectImportScheduleWorker.bulk_perform_async_with_contexts(
      projects,
      arguments_proc: -> (project) { project.id },
      context_proc: -> (project) { { project: project } }
    )

Each object from the array in the first argument is yielded into 2
blocks:

The `arguments_proc` which needs to the return the list of arguments the
job needs to be scheduled with.

The `context_proc` which needs to return a hash with the context
information for the job.

When providing objects for a context, please make sure to load the
relevant relations. For routables (namespace, project), use
`.with_route`, for projects also include the namespace using
`.with_namespace`.
parent 90072022
# frozen_string_literal: true
require_relative '../../migration_helpers'
require_relative '../../code_reuse_helpers'
module RuboCop
module Cop
module Scalability
class BulkPerformWithContext < RuboCop::Cop::Cop
include RuboCop::MigrationHelpers
include RuboCop::CodeReuseHelpers
MSG = <<~MSG
Prefer using `Worker.bulk_perform_async_with_contexts` and
`Worker.bulk_perform_in_with_context` over the methods without a context
if your worker deals with specific projects or namespaces
The context is required to add metadata to our logs.
If there is already a parent context that will apply to the jobs
being scheduled, please disable this cop with a comment explaing which
context will be applied.
Read more about it https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#worker-context
MSG
def_node_matcher :schedules_in_batch_without_context?, <<~PATTERN
(send (...) {:bulk_perform_async :bulk_perform_in} (...))
PATTERN
def on_send(node)
return if in_migration?(node)
return unless schedules_in_batch_without_context?(node)
return if name_of_receiver(node) == "BackgroundMigrationWorker"
add_offense(node, location: :expression)
end
end
end
end
end
# frozen_string_literal: true
module RuboCop
module Cop
module Scalability
class CronWorkerContext < RuboCop::Cop::Cop
MSG = <<~MSG
Manually define an ApplicationContext for cronjob-workers. The context
is required to add metadata to our logs.
If there is no relevant metadata, please disable the cop with a comment
explaining this.
Read more about it https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#worker-context
MSG
def_node_matcher :includes_cronjob_queue?, <<~PATTERN
(send nil? :include (const nil? :CronjobQueue))
PATTERN
def_node_search :defines_contexts?, <<~PATTERN
(send nil? :with_context _)
PATTERN
def_node_search :schedules_with_batch_context?, <<~PATTERN
(send (...) {:bulk_perform_async_with_contexts :bulk_perform_in_with_contexts} (...))
PATTERN
def on_send(node)
return unless includes_cronjob_queue?(node)
return if defines_contexts?(node.parent)
return if schedules_with_batch_context?(node.parent)
add_offense(node.arguments.first, location: :expression)
end
end
end
end
end
......@@ -44,6 +44,8 @@ require_relative 'cop/qa/element_with_pattern'
require_relative 'cop/qa/ambiguous_page_object_name'
require_relative 'cop/sidekiq_options_queue'
require_relative 'cop/scalability/file_uploads'
require_relative 'cop/scalability/bulk_perform_with_context'
require_relative 'cop/scalability/cron_worker_context'
require_relative 'cop/destroy_all'
require_relative 'cop/ruby_interpolation_in_translation'
require_relative 'code_reuse_helpers'
......
# frozen_string_literal: true
require 'fast_spec_helper'
require 'rubocop'
require_relative '../../../support/helpers/expect_offense'
require_relative '../../../../rubocop/cop/scalability/bulk_perform_with_context'
describe RuboCop::Cop::Scalability::BulkPerformWithContext do
include CopHelper
include ExpectOffense
subject(:cop) { described_class.new }
it "adds an offense when calling bulk_perform_async" do
inspect_source(<<~CODE.strip_indent)
Worker.bulk_perform_async(args)
CODE
expect(cop.offenses.size).to eq(1)
end
it "adds an offense when calling bulk_perform_in" do
inspect_source(<<~CODE.strip_indent)
Worker.bulk_perform_in(args)
CODE
expect(cop.offenses.size).to eq(1)
end
it "does not add an offense for migrations" do
allow(cop).to receive(:in_migration?).and_return(true)
inspect_source(<<~CODE.strip_indent)
Worker.bulk_perform_in(args)
CODE
expect(cop.offenses.size).to eq(0)
end
it "does not add an offense for scheduling BackgroundMigrations" do
inspect_source(<<~CODE.strip_indent)
BackgroundMigrationWorker.bulk_perform_in(args)
CODE
expect(cop.offenses.size).to eq(0)
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
require 'rubocop'
require_relative '../../../support/helpers/expect_offense'
require_relative '../../../../rubocop/cop/scalability/cron_worker_context'
describe RuboCop::Cop::Scalability::CronWorkerContext do
include CopHelper
include ExpectOffense
subject(:cop) { described_class.new }
it 'adds an offense when including CronjobQueue' do
inspect_source(<<~CODE.strip_indent)
class SomeWorker
include CronjobQueue
end
CODE
expect(cop.offenses.size).to eq(1)
end
it 'does not add offenses for other workers' do
expect_no_offenses(<<~CODE.strip_indent)
class SomeWorker
end
CODE
end
it 'does not add an offense when the class defines a context' do
expect_no_offenses(<<~CODE.strip_indent)
class SomeWorker
include CronjobQueue
with_context user: 'bla'
end
CODE
end
it 'does not add an offense when the worker calls `with_context`' do
expect_no_offenses(<<~CODE.strip_indent)
class SomeWorker
include CronjobQueue
def perform
with_context(user: 'bla') do
# more work
end
end
end
CODE
end
it 'does not add an offense when the worker calls `bulk_perform_async_with_contexts`' do
expect_no_offenses(<<~CODE.strip_indent)
class SomeWorker
include CronjobQueue
def perform
SomeOtherWorker.bulk_perform_async_with_contexts(contexts_for_arguments)
end
end
CODE
end
it 'does not add an offense when the worker calls `bulk_perform_in_with_contexts`' do
expect_no_offenses(<<~CODE.strip_indent)
class SomeWorker
include CronjobQueue
def perform
SomeOtherWorker.bulk_perform_in_with_contexts(contexts_for_arguments)
end
end
CODE
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