From ded448f7da9bc36c840ea9b067000f32144632b5 Mon Sep 17 00:00:00 2001
From: Pedro Pombeiro <noreply@pedro.pombei.ro>
Date: Wed, 12 Jul 2023 23:10:38 +0000
Subject: [PATCH] GraphQL: Add MOST_ACTIVE_DESC sort order to runners resolver
EE: true
---
app/finders/ci/runners_finder.rb | 11 ++-
app/graphql/resolvers/ci/runners_resolver.rb | 3 +
app/graphql/types/ci/runner_sort_enum.rb | 2 +
app/models/ci/runner.rb | 2 +
doc/api/graphql/reference/index.md | 1 +
ee/app/finders/ee/ci/runners_finder.rb | 25 +++++++
.../ee/resolvers/ci/runners_resolver.rb | 46 +++++++++++++
.../graphql/ee/types/ci/runner_sort_enum.rb | 18 +++++
ee/app/models/ee/ci/runner.rb | 29 ++++++++
ee/spec/finders/ee/ci/runners_finder_spec.rb | 59 ++++++++++++++++
ee/spec/models/ee/ci/runner_spec.rb | 56 ++++++++++++++++
.../requests/api/graphql/ci/runners_spec.rb | 67 ++++++++++++++++++-
spec/finders/ci/runners_finder_spec.rb | 4 +-
.../resolvers/ci/runners_resolver_spec.rb | 2 +-
14 files changed, 318 insertions(+), 7 deletions(-)
create mode 100644 ee/app/finders/ee/ci/runners_finder.rb
create mode 100644 ee/app/graphql/ee/resolvers/ci/runners_resolver.rb
create mode 100644 ee/app/graphql/ee/types/ci/runner_sort_enum.rb
create mode 100644 ee/spec/finders/ee/ci/runners_finder_spec.rb
diff --git a/app/finders/ci/runners_finder.rb b/app/finders/ci/runners_finder.rb
index 5f03ae773382e..630be17e64b2b 100644
--- a/app/finders/ci/runners_finder.rb
+++ b/app/finders/ci/runners_finder.rb
@@ -4,7 +4,6 @@ module Ci
class RunnersFinder < UnionFinder
include Gitlab::Allowable
- ALLOWED_SORTS = %w[contacted_asc contacted_desc created_at_asc created_at_desc created_date token_expires_at_asc token_expires_at_desc].freeze
DEFAULT_SORT = 'created_at_desc'
def initialize(current_user:, params:)
@@ -31,11 +30,17 @@ def execute
end
def sort_key
- ALLOWED_SORTS.include?(@params[:sort]) ? @params[:sort] : DEFAULT_SORT
+ allowed_sorts.include?(@params[:sort]) ? @params[:sort] : DEFAULT_SORT
end
private
+ attr_reader :group, :project
+
+ def allowed_sorts
+ %w[contacted_asc contacted_desc created_at_asc created_at_desc created_date token_expires_at_asc token_expires_at_desc]
+ end
+
def search!
if @project
project_runners
@@ -128,3 +133,5 @@ def filter_by!(scope_name, available_scopes)
end
end
end
+
+Ci::RunnersFinder.prepend_mod
diff --git a/app/graphql/resolvers/ci/runners_resolver.rb b/app/graphql/resolvers/ci/runners_resolver.rb
index 735e38c1a5c38..632655d36813f 100644
--- a/app/graphql/resolvers/ci/runners_resolver.rb
+++ b/app/graphql/resolvers/ci/runners_resolver.rb
@@ -4,6 +4,7 @@ module Resolvers
module Ci
class RunnersResolver < BaseResolver
include LooksAhead
+ include Gitlab::Graphql::Authorize::AuthorizeResource
type Types::Ci::RunnerType.connection_type, null: true
@@ -105,3 +106,5 @@ def nested_preloads
end
end
end
+
+Resolvers::Ci::RunnersResolver.prepend_mod
diff --git a/app/graphql/types/ci/runner_sort_enum.rb b/app/graphql/types/ci/runner_sort_enum.rb
index 8f2a13bd69995..4195eb043ed0d 100644
--- a/app/graphql/types/ci/runner_sort_enum.rb
+++ b/app/graphql/types/ci/runner_sort_enum.rb
@@ -15,3 +15,5 @@ class RunnerSortEnum < BaseEnum
end
end
end
+
+Types::Ci::RunnerSortEnum.prepend_mod
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index c7896bb873dbc..4eb5c3c9ed23d 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -72,6 +72,7 @@ class Runner < Ci::ApplicationRecord
has_many :runner_managers, inverse_of: :runner
has_many :builds
+ has_many :running_builds, inverse_of: :runner
has_many :runner_projects, inverse_of: :runner, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
has_many :projects, through: :runner_projects, disable_joins: true
has_many :runner_namespaces, inverse_of: :runner, autosave: true
@@ -198,6 +199,7 @@ class Runner < Ci::ApplicationRecord
scope :order_created_at_desc, -> { order(created_at: :desc) }
scope :order_token_expires_at_asc, -> { order(token_expires_at: :asc) }
scope :order_token_expires_at_desc, -> { order(token_expires_at: :desc) }
+
scope :with_tags, -> { preload(:tags) }
scope :with_creator, -> { preload(:creator) }
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index 941e077ffaa3d..1db6f6b43a71c 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -25019,6 +25019,7 @@ Values for sorting runners.
| <a id="cirunnersortcontacted_desc"></a>`CONTACTED_DESC` | Ordered by contacted_at in descending order. |
| <a id="cirunnersortcreated_asc"></a>`CREATED_ASC` | Ordered by created_at in ascending order. |
| <a id="cirunnersortcreated_desc"></a>`CREATED_DESC` | Ordered by created_at in descending order. |
+| <a id="cirunnersortmost_active_desc"></a>`MOST_ACTIVE_DESC` **{warning-solid}** | **Introduced** in 16.2. This feature is an Experiment. It can be changed or removed at any time. Ordered by number of running jobs in descending order (only available on Ultimate plans). |
| <a id="cirunnersorttoken_expires_at_asc"></a>`TOKEN_EXPIRES_AT_ASC` | Ordered by token_expires_at in ascending order. |
| <a id="cirunnersorttoken_expires_at_desc"></a>`TOKEN_EXPIRES_AT_DESC` | Ordered by token_expires_at in descending order. |
diff --git a/ee/app/finders/ee/ci/runners_finder.rb b/ee/app/finders/ee/ci/runners_finder.rb
new file mode 100644
index 0000000000000..7f4e60b2e0351
--- /dev/null
+++ b/ee/app/finders/ee/ci/runners_finder.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+module EE
+ module Ci
+ module RunnersFinder
+ extend ::Gitlab::Utils::Override
+
+ private
+
+ override :allowed_sorts
+ def allowed_sorts
+ super + ['most_active_desc']
+ end
+
+ override :sort!
+ def sort!
+ if sort_key == 'most_active_desc' && (project || group)
+ raise ArgumentError, 'most_active_desc can only be used on instance runners'
+ end
+
+ super
+ end
+ end
+ end
+end
diff --git a/ee/app/graphql/ee/resolvers/ci/runners_resolver.rb b/ee/app/graphql/ee/resolvers/ci/runners_resolver.rb
new file mode 100644
index 0000000000000..5569171625b27
--- /dev/null
+++ b/ee/app/graphql/ee/resolvers/ci/runners_resolver.rb
@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+module EE
+ module Resolvers
+ module Ci
+ module RunnersResolver
+ extend ::Gitlab::Utils::Override
+
+ prepended do
+ include ::Gitlab::Graphql::Authorize::AuthorizeResource
+ end
+
+ override :ready?
+ def ready?(sort: nil, type: nil, **args)
+ check_sort_conditions(type) if sort == :most_active_desc
+
+ super
+ end
+
+ def resolve(**args)
+ # keyset pagination doesn't really make sense for most_active_desc sorting
+ # as it requires counting ci_running_builds anyway
+ # and it's very hard to implement
+ return offset_pagination(super) if args[:sort]&.to_s == 'most_active_desc'
+
+ super
+ end
+
+ private
+
+ def check_sort_conditions(type)
+ if type != 'instance_type'
+ raise ::Gitlab::Graphql::Errors::ArgumentError,
+ 'MOST_ACTIVE_DESC sorting is only available when type is INSTANCE_TYPE'
+ end
+
+ return if License.feature_available?(:runner_performance_insights)
+
+ raise_resource_not_available_error!(
+ 'runner_performance_insights feature is required for MOST_ACTIVE_DESC sorting'
+ )
+ end
+ end
+ end
+ end
+end
diff --git a/ee/app/graphql/ee/types/ci/runner_sort_enum.rb b/ee/app/graphql/ee/types/ci/runner_sort_enum.rb
new file mode 100644
index 0000000000000..a13b4da698589
--- /dev/null
+++ b/ee/app/graphql/ee/types/ci/runner_sort_enum.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+module EE
+ module Types
+ module Ci
+ module RunnerSortEnum
+ extend ActiveSupport::Concern
+
+ prepended do
+ value 'MOST_ACTIVE_DESC',
+ 'Ordered by number of running jobs in descending order (only available on Ultimate plans).',
+ value: :most_active_desc,
+ alpha: { milestone: '16.2' }
+ end
+ end
+ end
+ end
+end
diff --git a/ee/app/models/ee/ci/runner.rb b/ee/app/models/ee/ci/runner.rb
index 34e9d02fc8a75..cce852083c768 100644
--- a/ee/app/models/ee/ci/runner.rb
+++ b/ee/app/models/ee/ci/runner.rb
@@ -5,9 +5,24 @@ module Ci
module Runner
extend ActiveSupport::Concern
+ MOST_ACTIVE_RUNNERS_BUILDS_LIMIT = 1000
+
prepended do
has_one :cost_settings, class_name: 'Ci::Minutes::CostSetting', foreign_key: :runner_id, inverse_of: :runner
+ scope :order_most_active_desc, -> do
+ joins(
+ <<~SQL
+ INNER JOIN (
+ SELECT "runner_id", ROW_NUMBER() OVER(PARTITION BY "runner_id" ORDER BY "runner_id") as rn
+ FROM "ci_running_builds"
+ ) as "limited_builds" ON "limited_builds"."runner_id" = "ci_runners"."id"
+ AND "limited_builds".rn <= #{MOST_ACTIVE_RUNNERS_BUILDS_LIMIT}
+ SQL
+ ).group(:id)
+ .reorder('COUNT(limited_builds.runner_id) DESC NULLS LAST', arel_table['id'].desc)
+ end
+
def self.any_shared_runners_with_enabled_cost_factor?(project)
if project.public?
instance_type.where('public_projects_minutes_cost_factor > 0').exists?
@@ -48,6 +63,20 @@ def cost_factor
::Gitlab::Ci::Minutes::CostFactor.new(runner_matcher)
end
end
+
+ class_methods do
+ extend ::Gitlab::Utils::Override
+
+ override :order_by
+ def order_by(order)
+ case order
+ when 'most_active_desc'
+ order_most_active_desc
+ else
+ super
+ end
+ end
+ end
end
end
end
diff --git a/ee/spec/finders/ee/ci/runners_finder_spec.rb b/ee/spec/finders/ee/ci/runners_finder_spec.rb
new file mode 100644
index 0000000000000..1265ea26efd75
--- /dev/null
+++ b/ee/spec/finders/ee/ci/runners_finder_spec.rb
@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::RunnersFinder, feature_category: :runner_fleet do
+ describe '#execute' do
+ subject(:execute) do
+ described_class.new(current_user: user, params: params).execute
+ end
+
+ context 'when sorting' do
+ let(:params) { { sort: sort_key } }
+
+ context 'with sort param equal to most_active_desc' do
+ let_it_be(:runners) { create_list(:ci_runner, 6) }
+ let_it_be(:project) { create(:project) }
+
+ let(:sort_key) { 'most_active_desc' }
+
+ before_all do
+ runners.map.with_index do |runner, number_of_builds|
+ create_list(:ci_build, number_of_builds, runner: runner, project: project).each do |build|
+ create(:ci_running_build, runner: build.runner, build: build, project: project)
+ end
+ end
+ end
+
+ context 'when admin', :enable_admin_mode do
+ let_it_be(:admin) { create(:user, :admin) }
+
+ let(:user) { admin }
+
+ it 'returns runners with the most running builds' do
+ is_expected.to eq(runners[1..5].reverse)
+ end
+ end
+
+ context 'with user as group owner' do
+ let_it_be(:group) { create(:group) }
+ let_it_be(:user) { create(:user) }
+
+ before_all do
+ group.add_owner(user)
+ end
+
+ context 'with sort param set to most_active_desc' do
+ let(:params) do
+ { group: group, sort: 'most_active_desc' }
+ end
+
+ it 'raises an error' do
+ expect { execute }.to raise_error(ArgumentError, 'most_active_desc can only be used on instance runners')
+ end
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/models/ee/ci/runner_spec.rb b/ee/spec/models/ee/ci/runner_spec.rb
index f4688d247855d..3aef8ac5e58bf 100644
--- a/ee/spec/models/ee/ci/runner_spec.rb
+++ b/ee/spec/models/ee/ci/runner_spec.rb
@@ -208,4 +208,60 @@
it { is_expected.to be_truthy }
end
end
+
+ describe '.order_most_active_desc' do
+ let_it_be(:project) { create(:project) }
+ let_it_be(:runners) { create_list(:ci_runner, 5) }
+
+ before_all do
+ runners.map.with_index do |runner, idx|
+ create_list(:ci_build, 10 - idx, runner: runner, project: project).each do |build|
+ create(:ci_running_build, runner: build.runner, build: build, project: project)
+ end
+ end
+ end
+
+ it 'returns runners sorted by running builds' do
+ expect(described_class.order_most_active_desc.pluck(:id)).to eq(runners.pluck(:id))
+ end
+
+ it 'limits the number of running builds counted and sorts by id desc' do
+ stub_const("EE::Ci::Runner::MOST_ACTIVE_RUNNERS_BUILDS_LIMIT", 8)
+
+ runner_ids = runners.pluck(:id)
+
+ expected_runner_ids = runner_ids[0..2].reverse + runner_ids[3..5]
+
+ expect(described_class.order_most_active_desc.pluck(:id)).to eq(expected_runner_ids)
+ end
+ end
+
+ describe '.order_by' do
+ subject(:order_by) do
+ described_class.order_by(sort_key)
+ end
+
+ context 'when sort_key is most_active_desc' do
+ let_it_be(:project) { create(:project) }
+ let_it_be(:runners) { create_list(:ci_runner, 3) }
+
+ let(:sort_key) { 'most_active_desc' }
+
+ context 'with no running builds' do
+ it { is_expected.to be_empty }
+ end
+
+ context 'with running builds' do
+ before_all do
+ runners.each_with_index do |runner, running_build_count|
+ create_list(:ci_running_build, running_build_count, runner: runner, project: project)
+ end
+ end
+
+ it 'contains the runners in the correct order' do
+ is_expected.to eq([runners[2], runners[1]])
+ end
+ end
+ end
+ end
end
diff --git a/ee/spec/requests/api/graphql/ci/runners_spec.rb b/ee/spec/requests/api/graphql/ci/runners_spec.rb
index eb79c2b55fda9..2074be072cfab 100644
--- a/ee/spec/requests/api/graphql/ci/runners_spec.rb
+++ b/ee/spec/requests/api/graphql/ci/runners_spec.rb
@@ -18,8 +18,6 @@
before do
stub_runner_releases(%w[14.0.0 14.0.1])
-
- post_graphql(query, current_user: current_user)
end
context 'with upgradeStatus argument' do
@@ -37,6 +35,10 @@
)
end
+ before do
+ post_graphql(query, current_user: current_user)
+ end
+
context 'with deprecated CiRunnerUpgradeStatusType enum type' do
let(:upgrade_status_graphql_type) { 'CiRunnerUpgradeStatusType' }
@@ -75,6 +77,10 @@
)
end
+ before do
+ post_graphql(query, current_user: current_user)
+ end
+
context 'with deprecated RunnerMembershipFilter enum type' do
let(:membership_graphql_type) { 'RunnerMembershipFilter' }
@@ -91,5 +97,62 @@
end
end
end
+
+ context 'when sorting by MOST_ACTIVE_DESC' do
+ let_it_be(:runners) { create_list(:ci_runner, 6) }
+
+ before_all do
+ runners.map.with_index do |runner, number_of_builds|
+ create_list(:ci_build, number_of_builds, runner: runner, project: project).each do |build|
+ create(:ci_running_build, runner: build.runner, build: build, project: project)
+ end
+ end
+ end
+
+ it_behaves_like 'sorted paginated query' do
+ before do
+ stub_licensed_features(runner_performance_insights: true)
+ end
+
+ def pagination_query(params)
+ graphql_query_for(:runners, params.merge(type: :INSTANCE_TYPE), "#{page_info} nodes { id }")
+ end
+
+ def pagination_results_data(runners)
+ runners.map { |runner| GitlabSchema.parse_gid(runner['id'], expected_type: ::Ci::Runner).model_id.to_i }
+ end
+
+ let(:sort_param) { :MOST_ACTIVE_DESC }
+ let(:first_param) { 2 }
+ let(:all_records) { runners[1..5].reverse.map(&:id) }
+ let(:data_path) { [:runners] }
+ end
+
+ it 'when requesting not instance_type runners' do
+ stub_licensed_features(runner_performance_insights: true)
+ query = graphql_query_for(:runners, { type: :GROUP_TYPE, sort: :MOST_ACTIVE_DESC }, "nodes { id }")
+ post_graphql(query, current_user: current_user)
+
+ expect(graphql_errors).to include(a_hash_including(
+ 'message' => 'MOST_ACTIVE_DESC sorting is only available when type is INSTANCE_TYPE'))
+ end
+
+ it 'when requesting not runners without type' do
+ stub_licensed_features(runner_performance_insights: true)
+ query = graphql_query_for(:runners, { sort: :MOST_ACTIVE_DESC }, "nodes { id }")
+ post_graphql(query, current_user: current_user)
+
+ expect(graphql_errors).to include(a_hash_including(
+ 'message' => 'MOST_ACTIVE_DESC sorting is only available when type is INSTANCE_TYPE'))
+ end
+
+ it 'returns error when feature is not enabled' do
+ query = graphql_query_for(:runners, params.merge(type: :INSTANCE_TYPE, sort: :MOST_ACTIVE_DESC), "nodes { id }")
+ post_graphql(query, current_user: current_user)
+
+ expect(graphql_errors).to include(a_hash_including(
+ 'message' => 'runner_performance_insights feature is required for MOST_ACTIVE_DESC sorting'))
+ end
+ end
end
end
diff --git a/spec/finders/ci/runners_finder_spec.rb b/spec/finders/ci/runners_finder_spec.rb
index 77260bb4c5c38..e57ad5bc76d1a 100644
--- a/spec/finders/ci/runners_finder_spec.rb
+++ b/spec/finders/ci/runners_finder_spec.rb
@@ -302,7 +302,7 @@ def execute
end
describe '#execute' do
- subject { described_class.new(current_user: user, params: params).execute }
+ subject(:execute) { described_class.new(current_user: user, params: params).execute }
shared_examples 'membership equal to :descendants' do
it 'returns all descendant runners' do
@@ -321,7 +321,7 @@ def execute
context 'with :group as target group' do
let(:target_group) { group }
- context 'passing no params' do
+ context 'passing no membership params' do
it_behaves_like 'membership equal to :descendants'
end
diff --git a/spec/graphql/resolvers/ci/runners_resolver_spec.rb b/spec/graphql/resolvers/ci/runners_resolver_spec.rb
index e4620b96cae6f..02fc7d28255a9 100644
--- a/spec/graphql/resolvers/ci/runners_resolver_spec.rb
+++ b/spec/graphql/resolvers/ci/runners_resolver_spec.rb
@@ -9,7 +9,7 @@
let(:obj) { nil }
let(:args) { {} }
- subject do
+ subject(:resolve_scope) do
resolve(described_class, obj: obj, ctx: { current_user: user }, args: args,
arg_style: :internal)
end
--
GitLab