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
inowned_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
inowned_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
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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