Skip to content

Shortcircuit expensive queries in Runner#can_pick? [RUN ALL RSPEC] [RUN AS-IF-FOSS]

What does this MR do?

Related to #322618 (closed)

In Ci::RegisterJobService when a runner connects to pick a job, we filter jobs based on the runner type. After selecting the first job, we run Runner#can_pick? method which checks if the runner matches some build criteria (protected, tags) as well as whether the runner is in the list of runners available for the given job. This last check is redundant.

Runner#can_pick? in turns uses assignable_for?(build) which uses owned_or_instance_wide scope:

module Ci
  class Runner
    scope :owned_or_instance_wide, -> (project_id) do
      from_union(
        [
          belonging_to_project(project_id),
          belonging_to_parent_group_of_project(project_id),
          instance_type
        ],
        remove_duplicates: false
      )
    end

    def assignable_for?(project_id)
      self.class.owned_or_instance_wide(project_id).where(id: self.id).any?
    end
  end
end
  • if it's a shared runner we consider all jobs where projects have shared runners enabled - the scope instance_type in owned_or_instance_wide covers this case.
  • if it's a project specific runner we select all the jobs for the projects the runner is associated to. - the scope belonging_to_project in owned_or_instance_wide covers this case.
  • if it's a group specific runner we select all the jobs for the groups the runner is associated to. - the scope belonging_to_parent_group_of_project covers this case.

In all 3 cases above, the query we use for selecting jobs is already stricter than the checks assignable_for? runs (e.g. it doesn't consider whether the project has shared runners enabled, or group has group runners enabled, doesn't consider if project is pending delete, etc.).

Feature flag

This change is deployed behind feature flag ci_runners_short_circuit_assignable_for

Rollout issue: #323317 (closed)

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Fabio Pitino

Merge request reports